[U-Boot] [PATCH 6/6] ARM:OMAP3:SDP3430: initial support

[Nishanth Menon]

Wolfgang Denk said the following on 09/23/2009 10:51 PM:
> Dear Nishanth Menon,
>
> In message <1253326918-1670-7-git-send-email-nm at ti.com> you wrote:
>   
>> --===============1247028818==
>>
>> From: David Brownell <david-b at pacbell.net>
>>
>> Start of SDP3430 support in "mainline"
>> u-boot mainline code
>>
>> Original Patch written by David Brownell
>>     
>
> Um... this seems redundant information to me (the "From:" line and
> the Signed-off-by: line already say that David Brownell is the
> author.
>   
Acked as previously discussed.
> On the other hand, I'm missing explanations what SDP3430 might be?
>   
hmm.. my bad. will fix. I should really be adding a few more lines to
README.omap3. I will do that along with this change.
>
>   
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e9db278..adc8a63 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -619,6 +619,7 @@ Guennadi Liakhovetski <g.liakhovetski at gmx.de>
>>  Nishanth Menon <nm at ti.com>
>>  
>>  	omap3_zoom1	ARM CORTEX-A8 (OMAP3xx SoC)
>> +	omap3_sdp	ARM CORTEX-A8 (OMAP3xx SoC)
>>     
>
> Please keep lists sorted.
>   
Ack
> General remark:
>
> The board name is "SDP3430", right? The board directory name is
> board/ti/sdp3430/, which is ok. But then the configuration name should
> be "sdp3430", too.
>   
Thanks.
>> +
>> +static const u32 gpmc_sdp_nand[] = {
>> +    0x00000800,	/*CONF1 */
>> +    0x00141400,	/*CONF2 */
>> +    0x00141400,	/*CONF3 */
>> +    0x0F010F01,	/*CONF4 */
>> +    0x010C1414,	/*CONF5 */
>> +    0x1F040A80,	/*CONF6 */
>> +    /*CONF7- computed as params */
>> +};
>>     
>
> Please comment what all these magic numbers mean.
>
>   
these are configuration register values for GPMC (General Purpose Memory
Controller)
I should be indeed adding proper comments here. will do.
>> +/******************************************************************************
>> + * Routine: board_init
>> + * Description: Early hardware init.
>> + *****************************************************************************/
>>     
>
> Incorrect multiline comment style. Please fix globally.
>   
Ack
> ...
>   
>> diff --git a/board/ti/sdp3430/sdp.h b/board/ti/sdp3430/sdp.h
>> new file mode 100644
>> index 0000000..5ad2920
>> --- /dev/null
>> +++ b/board/ti/sdp3430/sdp.h
>>     
> ...
>   
>> +#define MUX_SDP3430()\
>> + /*SDRC*/\
>> + MUX_VAL(CP(SDRC_D0), (IEN | PTD | DIS | M0)) /*SDRC_D0*/\
>> + MUX_VAL(CP(SDRC_D1), (IEN | PTD | DIS | M0)) /*SDRC_D1*/\
>>     
> ...
>
> Incorrect indentation.
>   
(having spend almost an hr cleaning that piece of mux header up)
groan.. any recommendations? is it because of the "  "?
> What exacty is the purpose of the comment? It does not carry any
> information. Seems just a waste of line length to me?
>   
you mean /* SDRC_D0 */? hmmm.. might actually make sense if
I am not using default mux mode 0 to note why I am using a new
value.
>   
>> diff --git a/board/ti/sdp3430/u-boot.lds b/board/ti/sdp3430/u-boot.lds
>> new file mode 100644
>> index 0000000..4ecc6dd
>> --- /dev/null
>> +++ b/board/ti/sdp3430/u-boot.lds
>>     
>
> Is it really necessary that this board uses a custom linke rscript?
> Cannot we use a generic one for several boards?
>   
Ack.
>   
>> diff --git a/include/configs/omap3_sdp.h b/include/configs/omap3_sdp.h
>> new file mode 100644
>> index 0000000..176617a
>> --- /dev/null
>> +++ b/include/configs/omap3_sdp.h
>>     
>
> This should be include/configs/sdp3430.h, accorsing to the board name.
>   
Ack. I guess this is a hangover  from the days where we wanted all omap3
boards to look similar.
> ...
>   
>> +/*
>> + * Size of malloc() pool
>> + */
>> +#define CONFIG_ENV_SIZE			SZ_256K	/* Total Size Environment */
>>     
>
> Please do not use any of these SZ_ defines; they will be removed soon.
>   
Ack..
>   
>> +#if 0
>> +#define CONSOLE_J9			/* else J8/UART1 (innermost) */
>> +#endif
>>     
>
> Please delete - don't add dead code.
>   
CONSOLE_J9 is really an option, but I get your point here I should be using
#undef even if i wanted to allow folks to tweak around.. #if 0s are *evil*
>   
>> +/* timeout values are in ticks */
>> +#define CONFIG_SYS_FLASH_ERASE_TOUT	(100 * CONFIG_SYS_HZ)
>> +#define CONFIG_SYS_FLASH_WRITE_TOUT	(100 * CONFIG_SYS_HZ)
>>     
>
> Strictly speaking the comment is wrong. The timeouts are in
> milliseconds.
>   
gotcha. Ack.
>   
>> +/* OMITTED:  single 2 Gbit KFM2G16 OneNAND flash */
>> +
>> +
>>     
>
> Only a single blank line in places like thsi, please.
>   
ok ok.. though I think you are nit picking ;)..
>   
>> +/* commands to include */
>> +#include <config_cmd_default.h>
>> +
>> +#define CONFIG_CMD_NET
>> +#define CONFIG_CMD_DHCP		/* DHCP Support			*/
>> +#define CONFIG_CMD_I2C		/* I2C serial bus support	*/
>> +#define CONFIG_CMD_JFFS2	/* JFFS2 Support		*/
>> +#define CONFIG_CMD_MMC		/* MMC support			*/
>> +#define CONFIG_CMD_FAT		/* FAT support			*/
>> +#define CONFIG_CMD_EXT2		/* EXT2 Support			*/
>>     
>
> We consider it good style to keep such lists sorted.
>   
I suppose Alphabetical sort?
> ...
>   
>> +#define CONFIG_SYS_MEMTEST_START	(OMAP34XX_SDRC_CS0)	/* memtest */
>> +								/* works on */
>> +#define CONFIG_SYS_MEMTEST_END		(OMAP34XX_SDRC_CS0 + \
>> +					0x01F00000) /* 31MB */
>>     
>
> Has this been tested? Can you really overwrite low memory? No
> exception vectors needed there?
>   
yep it does boot :D.. exception vectors are stored in SRAM(which is a
different address range)..
but you have a point here -> will my sdram test actually overwrite my
u-boot itself - heh heh, wont be much use then ..
will check and fix. thanks
>   
>> +#undef	CONFIG_SYS_CLKS_IN_HZ		/* everything, incl board info, in Hz */
>>     
>
> There is no need to undefine non-existent variables.
>   
yep, will check
>   
>> +#define CONFIG_SYS_LOAD_ADDR		(OMAP34XX_SDRC_CS0)	/* default */
>> +							/* load address */
>>     
>
> Does this really work? Is low memory unused on this CPU? [Dorry for
> asking stupid questions, just want to be sure...]
>
>   
yes, it does -> see  previous comment.
Regards,
Nishanth Menon