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

Nishanth Menon
2009-09-24T00:47:39Z

Wolfgang Denk said the following on 09/23/2009 10:51 PM:
> Dear Nishanth Menon,
>
> In message <1253326918-1670-7-git-send-email-nm@ti.com> you wrote:
>
>> --===============1247028818==
>>
>> From: David Brownell <david-b@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@gmx.de>
>> Nishanth Menon <nm@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