[U-Boot] [PATCH 2/4] arm, omap3: Add support for TechNexion modules

[Wolfgang Denk]

Dear Tapani Utriainen,

In message <20111121164401.5b816ce0 at myhost> you wrote:
> 
> Add support for TechNexion TAM3517 SoM
> 
> Signed-off-by: Tapani Utriainen <tapani at technexion.com>
> CC: Sandeep Paulraj <s-paulraj at ti.com>
> ---
>  arch/arm/include/asm/mach-types.h  |    1 
>  board/technexion/tam3517/Makefile  |   49 ++++
>  board/technexion/tam3517/tam3517.c |  150 ++++++++++++
>  board/technexion/tam3517/tam3517.h |  388 +++++++++++++++++++++++++++++++++
>  boards.cfg                         |    1 
>  include/configs/tam3517.h          |  427 +++++++++++++++++++++++++++++++++++++
>  6 files changed, 1016 insertions(+)
...
> +#if defined(CONFIG_DRIVER_TI_EMAC)
> +	/* allow the PHY to stabilize and settle down */
> +	do {
> +		udelay(1000);
> +		ctr++;
> +	} while (ctr < 300);

It is not a good idea to delay each and every boot process by 300
milliseconds even if nobody is trying to use the network interface at
all.  Please move this into the network init parts.

> +	reset = readl(AM3517_IP_SW_RESET);
> +	reset &= (~CPGMACSS_SW_RST);
> +	writel(reset, AM3517_IP_SW_RESET);

Please use clrbits*() instead.

> +int board_eth_init(bd_t *bis)
> +{
> +	int n_eth = 0;
> +
> +#if defined(CONFIG_DRIVER_TI_EMAC)
> +	davinci_emac_initialize();
> +	n_eth++;
> +#endif
> +
> +
> +#if defined(CONFIG_SMC911X)
> +	/* init cs for extern lan */
> +	writel(NET_GPMC_CONFIG1, &gpmc_cfg->cs[5].config1);
> +	writel(NET_GPMC_CONFIG2, &gpmc_cfg->cs[5].config2);
> +	writel(NET_GPMC_CONFIG3, &gpmc_cfg->cs[5].config3);
> +	writel(NET_GPMC_CONFIG4, &gpmc_cfg->cs[5].config4);
> +	writel(NET_GPMC_CONFIG5, &gpmc_cfg->cs[5].config5);
> +	writel(NET_GPMC_CONFIG6, &gpmc_cfg->cs[5].config6);
> +	writel(NET_GPMC_CONFIG7, &gpmc_cfg->cs[5].config7);
> +
> +	if (smc911x_initialize(0, CONFIG_SMC911X_BASE) <= 0)
> +		n_eth--;

So eventually  n_eth  will become negative?


> +#endif
> +	return 0;

I guess this should be  "return  n_eth" ??

Did you code pass a compile test without warnings?  I seriously doubt
that.  Please fix globally.

> +const omap3_sysinfo sysinfo = {
> +	DDR_DISCRETE,
> +	"TAM3517 Twister Board",
> +	"NAND",
> +};

This may or may not be correct.  Please keep in mind that the TAM3517
is a SoM, which may be used on a variety of carrier boards.  The
"TAM3517 Twister Board" information is only correct for use on the
"Twister" board, but wrong for all other uses.


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

Do we need both?

> +/*
> +	For LG 4.3" panel use
> +	"panel-generic.disp_timings=8000,480/8/4/41,272/4/2/10,24\0" \
> +*/

Incorrect multiline comment.  Please fix globally.

> +#define CONFIG_AUTO_COMPLETE
> +#define CONFIG_AUTOCOMPLETE

Why are you defining bogus vaiables like CONFIG_AUTOCOMPLETE?  This is
not used anywhere in the code.   Please check and clean up globally.


> +#define V_PROMPT			"TAM3517 # "

What is this good for?  Please omit adding unneeded #defines.

> +#define CONFIG_SYS_LONGHELP		/* undef to save memory */
> +#define CONFIG_SYS_HUSH_PARSER		/* use "hush" command parser */
> +#define CONFIG_SYS_PROMPT_HUSH_PS2	"> "
> +#define CONFIG_SYS_PROMPT		V_PROMPT

Just insert the value here.

> +#define CONFIG_SYS_CBSIZE		2048	/* Console I/O Buffer Size */
> +#define CONFIG_CMD_RUN

It might make sense to combin all "CONFIG_CMD" definitions in one
place.  Also, it makes little sense to add definitions here that are
on by default anyway.  Plerase clkean up.

> +
> +#define CONFIG_SYS_FLASH_BASE		boot_flash_base

Is there a version oif TAM3517 modules with NOR flash?  Or what is
this needed/used for?

> +/* Monitor at start of flash */
> +#define CONFIG_SYS_MONITOR_BASE		CONFIG_SYS_FLASH_BASE

???

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

I don't think you have NOR flash on the modules, or do you?

> +/* Flash banks JFFS2 should use */
> +#define CONFIG_SYS_MAX_MTD_BANKS	(CONFIG_SYS_MAX_FLASH_BANKS + \
> +					CONFIG_SYS_MAX_NAND_DEVICE)

Has this been tested?

> +#if defined(CONFIG_CMD_NET)
> +/* Disable u-boot support for EMAC LAN, due compile problems
> +   Linux kernel support is enough to enable the interface

Please fix the compile problems instead.

> +#define CONFIG_DRIVER_TI_EMAC
> +#define CONFIG_DRIVER_TI_EMAC_USE_RMII
> +#define CONFIG_EMAC_MDIO_PHY_NUM   0
> +#define CONFIG_ARM926EJS	1

CONFIG_ARM926EJS?  for an OMAP3 processor?

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
Always try to do things in chronological order; it's  less  confusing
that way.