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

[Stefano Babic <sbabic@denx.de>]

On 23/11/2011 17:21, Wolfgang Denk wrote:
>> +#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?

I will do it

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

Yes - the command relies on NOR flash. In fact, in cmd_bootm.c do_imls()
needs CONFIG_SYS_MAX_FLASH_BANKS, that has no sense on this SOM, because
I have not CFI flash.

Also defining CONFIG_SYS_MAX_FLASH_BANKS does not work, because cfi.h is
still included. I prefer disabling IMLS as adding fake CFI values.

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

NAND_BASE is the address of the controller defined in cpu.h. That iis
correct.

CONFIG_SYS_NAND_ADDR seems to me obsolete and not used anymore - a lot
of boards define it, I think it should be globally removed - I start
dropping from here.

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

Yes - the only issue here is that only 31 MB (the SOM has 256 MB RAM)
are tested as default. Personally I do not like to set as default value
the whole RAM (subtracting the size of the bootloader code, of course
!), because the test takes too much time for a fast run. The user can
always provide parameters for the mtest command if he wants to test the
whole RAM.

>> +/*-----------------------------------------------------------------------
>> + * Stack sizes
>> + *
>> + * The stack sizes are set up in start.S using the settings below
>> + */
> 
> Incorrect multiline comment style. Please fix globally.

Thanks, I will fix it.

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

Good point, I will do it.

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

This is completely wrong, bad cut&paste ;-(. I will fix it.

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

Confirmed, it does not work and I will clean up.

>> +#define CONFIG_SPL_MAX_SIZE		0xB400  /* 45 K */
> 
> (45 << 10) would be way easier to read...

ok

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
=====================================================================