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

[Tom Rini]

On Wednesday, November 23, 2011, Wolfgang Denk <wd at denx.de> wrote:
> 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?

All of the above is a copy/paste poor example.  I've put some low grade
thought into it, and it's on my 2012.03 wish list to have be good, or at
least better.

>> +/* Configure the PISMO */
>> +#define PISMO1_NAND_SIZE             GPMC_SIZE_128M
>
> Don't we auto-detect the size?

In this case (config space stuff iirc), no.  But it's also a semi-constant,
but I don't have my TRM handy.

>> +#define CONFIG_SPL_BSS_START_ADDR    0x8f080000 /* end of RAM */
>
> Don't we auto-detect the RAM size?

Kinda?  We have a few cases:
- all board revs have the same size, just code. It
- a few board revs, and we can tell rev a from b, so hard code that way

We don't have a "ROM says..." like some other platforms.

-- 
Tom

-- 
Tom