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

[Wolfgang Denk]

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.

On the other hand, I'm missing explanations what SDP3430 might be?


> 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.

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.

> diff --git a/MAKEALL b/MAKEALL
> index f0ed8ea..53620eb 100755
> --- a/MAKEALL
> +++ b/MAKEALL
> @@ -588,6 +588,7 @@ LIST_ARM_CORTEX_A8="		\
>  	omap3_pandora		\
>  	omap3_zoom1		\
>  	omap3_zoom2		\
> +	omap3_sdp		\
>  "

Ditto.

>  #########################################################################
> diff --git a/Makefile b/Makefile
> index 5a4a109..2626147 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3172,6 +3172,9 @@ omap3_zoom1_config :	unconfig
>  omap3_zoom2_config :	unconfig
>  	@$(MKCONFIG) $(@:_config=) arm arm_cortexa8 zoom2 logicpd omap3
>  
> +omap3_sdp_config :	unconfig
> +	@$(MKCONFIG) $(@:_config=) arm arm_cortexa8 sdp3430 ti omap3

Here again. Please check all other similar places.

...
> +static const u32 gpmc_sdp_nor[] = {
> +    0x00001200,	/*CONF1 */
> +    0x001F1F00,	/*CONF2 */
> +    0x00080802,	/*CONF3 */
> +    0x1C091C09,	/*CONF4 */
> +    0x01131F1F,	/*CONF5 */
> +    0x1F0F03C2,	/*CONF6 */
> +    /*CONF7- computed as params */
> +};
> +static const u32 gpmc_sdp_debug[] = {
> +    0x00611200,	/*CONF1 FPGA needs WAIT1 line-active Low to function now.. */
> +    0x001F1F01,	/*CONF2 */
> +    0x00080803,	/*CONF3 */
> +    0x1D091D09,	/*CONF4 */
> +    0x041D1F1F,	/*CONF5 */
> +    0x1D0904C4,	/*CONF6 */
> +    /*CONF7- computed as params */
> +};
> +static const u32 gpmc_sdp_onenand[] = {
> +    0x00001200,	/*CONF1 */
> +    0x000F0F01,	/*CONF2 */
> +    0x00030301,	/*CONF3 */
> +    0x0F040F04,	/*CONF4 */
> +    0x010F1010,	/*CONF5 */
> +    0x1F060000,	/*CONF6 */
> +    /*CONF7- computed as params */
> +};
> +
> +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.

> +/******************************************************************************
> + * Routine: board_init
> + * Description: Early hardware init.
> + *****************************************************************************/

Incorrect multiline comment style. Please fix globally.

...
> 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.

What exacty is the purpose of the comment? It does not carry any
information. Seems just a waste of line length to me?

> 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?

> 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.

...
> +/*
> + * 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.

> +#if 0
> +#define CONSOLE_J9			/* else J8/UART1 (innermost) */
> +#endif

Please delete - don't add dead code.

> +/* 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.

> +/* OMITTED:  single 2 Gbit KFM2G16 OneNAND flash */
> +
> +

Only a single blank line in places like thsi, please.

> +/* 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.

...
> +#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?

> +#undef	CONFIG_SYS_CLKS_IN_HZ		/* everything, incl board info, in Hz */

There is no need to undefine non-existent variables.

> +#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...]


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Everything should be made as simple as possible, but not simpler."
                                                    - Albert Einstein