[U-Boot] [PATCH 1/2] ARM: omap3: added common configuration for Technexion TAM3517

[Wolfgang Denk]

Dear Stefano Babic,

In message <1322040416-11751-2-git-send-email-sbabic at denx.de> you wrote:
> The TAM3517 is a SOM module that can be used on custom boards.
> The patch add a common configuration file that is included
> by the boards using this module.
> 
> Signed-off-by: Stefano Babic <sbabic at denx.de>
> CC: Tapani Utrianen <tapani at technexion.com>
> CC: Tom Rini <tom.rini at gmail.com>
> CC: Sandeep Paulraj <s-paulraj at ti.com>
> ---
>  include/configs/tam3517-common.h |  334 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 334 insertions(+), 0 deletions(-)
>  create mode 100644 include/configs/tam3517-common.h
...
> +#define CONFIG_CMD_EXT2		/* EXT2 Support			*/
> +#define CONFIG_CMD_FAT		/* FAT support			*/
> +#define CONFIG_CMD_JFFS2	/* JFFS2 Support		*/
> +
> +#define CONFIG_CMD_I2C		/* I2C serial bus support	*/
> +#define CONFIG_CMD_MMC		/* MMC support			*/
> +#define CONFIG_CMD_FAT		/* FAT support			*/
> +#define CONFIG_CMD_NAND		/* NAND support			*/
> +#define CONFIG_CMD_USB
> +#define CONFIG_CMD_DHCP
> +#define CONFIG_CMD_PING
> +#define CONFIG_CMD_CACHE
> +#define CONFIG_CMD_GPIO

Maybe you want to sort this list.  And eventually remove entries that
are defined by default?

> +#undef CONFIG_CMD_FLASH		/* flinfo, erase, protect	*/
> +#undef CONFIG_CMD_IMI		/* iminfo			*/
> +#undef CONFIG_CMD_IMLS		/* List all found images	*/

Is there any good reason to disable the "iminfo" command?

> +#define CONFIG_SYS_NAND_ADDR		NAND_BASE	/* physical address */
> +							/* to access nand */
> +#define CONFIG_SYS_NAND_BASE		NAND_BASE	/* physical address */

Do we really need this double definitions?  Please get rid of
NAND_BASE.

...
> +/* memtest works on */
> +#define CONFIG_SYS_MEMTEST_START	(OMAP34XX_SDRC_CS0)
> +#define CONFIG_SYS_MEMTEST_END		(OMAP34XX_SDRC_CS0 + \
> +					0x01F00000) /* 31MB */

Has this been tested?

> +#define CONFIG_SYS_LOAD_ADDR		(OMAP34XX_SDRC_CS0) /* default load */
> +								/* address */

Don't we have any exception vectors or similar in low memory?

> +/*-----------------------------------------------------------------------
> + * Stack sizes
> + *
> + * The stack sizes are set up in start.S using the settings below
> + */

Incorrect multiline comment style. Please fix globally.

> +/* Configure the PISMO */
> +#define PISMO1_NAND_SIZE		GPMC_SIZE_128M

Don't we auto-detect the size?

> +#define CONFIG_ENV_IS_IN_NAND
> +#define SMNAND_ENV_OFFSET		0x180000 /* environment starts here */
> +
> +#define CONFIG_SYS_ENV_SECT_SIZE	(128 << 10)	/* 128 KiB */
> +#define CONFIG_ENV_OFFSET		SMNAND_ENV_OFFSET
> +#define CONFIG_ENV_ADDR			SMNAND_ENV_OFFSET

Please extend to support redundant environment, plus one or two
reserve sectors in case a sector goes bad.

> +/*-----------------------------------------------------------------------
> + * CFI FLASH driver setup
> + */
> +/* 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)

This seems bogus, as there is no NOR flash on these devices.

> +/* Flash banks JFFS2 should use */
> +#define CONFIG_SYS_MAX_MTD_BANKS	(CONFIG_SYS_MAX_FLASH_BANKS + \
> +					CONFIG_SYS_MAX_NAND_DEVICE)
> +#define CONFIG_SYS_JFFS2_MEM_NAND
> +/* use flash_info[2] */
> +#define CONFIG_SYS_JFFS2_FIRST_BANK	CONFIG_SYS_MAX_FLASH_BANKS
> +#define CONFIG_SYS_JFFS2_NUM_BANKS	1

All this probably does not work.

> +/*
> + * ethernet support
> + *

Please delete unneeded empty lines like this one.

...
> +#define CONFIG_SPL_NAND_WORKSPACE	0x8f07f000 /* below BSS */

Urghhh...  How do you guarantee this really works, no matter how the
image gets linked?

> +#define CONFIG_SPL_MAX_SIZE		0xB400  /* 45 K */

(45 << 10) would be way easier to read...

> +#define CONFIG_SPL_BSS_START_ADDR	0x8f080000 /* end of RAM */

Don't we auto-detect the RAM size?

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
You can't evaluate a man by logic alone.
	-- McCoy, "I, Mudd", stardate 4513.3