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

Wolfgang Denk
2009-09-23T19:51:31Z

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.

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

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