[U-Boot] [PATCH v6] Add support for the digsy MTC board.

[Wolfgang Denk]

Dear Grzegorz Bernacki,

In message <12345332203460-git-send-email-gjb at semihalf.com> you wrote:
> 
> Signed-off-by: Grzegorz Bernacki <gjb at semihalf.com>

I was about to apply this as John probably has very little time to
care about this.

But I have a few comments / requests first:

...
> +void static set_ethaddr(void)
> +{
...

Eventually this should be rebased against the "next" branch, and
board_get_enetaddr() should be used here?

> +	for (n = 0; n < 6 ; n++) {
> +		addr = addr_of_eth_addr + n;
> +		chip = EEPROM_ADDR + ((addr & 0x300)>>8);
> +		i2c_read(chip, (addr & 0xFF), 1, (uchar *)&eth_addr[n], 1);
> +	}

Can we not do this with a single i2c_read() call with length = 6 ?

> +	sprintf(str_eth_addr, "%02X:%02X:%02X:%02X:%02X:%02X",
> +		eth_addr[0], eth_addr[1], eth_addr[2],
> +		eth_addr[3], eth_addr[4], eth_addr[5]);
> +	setenv("ethaddr", str_eth_addr);

eth_setenv_enetaddr() should be used here.

See commit 3754a3b3 (in the "next" branch) for details.


> +int last_stage_init (void)
> +{
> +	if (getenv("ethaddr") == NULL)
> +		set_ethaddr();
> +	return 0;
> +}

I think this should be removed in the new context.

...
> +void init_ide_reset(void)
> +{
> +	debug ("init_ide_reset\n");
> +
> +	/* set gpio output value to 1 */
> +	out_be32((void *)MPC5XXX_WU_GPIO_DATA_O,
> +		in_be32((void *)MPC5XXX_WU_GPIO_DATA_O) | (1 << 25));

Should that not be replaced by the (much easier to read)

	setbits_be32((void *)MPC5XXX_WU_GPIO_DATA_O, (1 << 25));

?

> +	/* open drain output */
> +	out_be32((void *)MPC5XXX_WU_GPIO_ODE,
> +		in_be32((void *)MPC5XXX_WU_GPIO_ODE) | (1 << 25));
> +	/* direction output */
> +	out_be32((void *)MPC5XXX_WU_GPIO_DIR,
> +		in_be32((void *)MPC5XXX_WU_GPIO_DIR) | (1 << 25));
> +	/* enable gpio */
> +	out_be32((void *)MPC5XXX_WU_GPIO_ENABLE,
> +		in_be32((void *)MPC5XXX_WU_GPIO_ENABLE) | (1 << 25));

Ditto here and elsewhere.

> +void ide_set_reset(int idereset)
> +{
> +	debug ("ide_reset(%d)\n", idereset);
> +
> +	/* set gpio output value to 0 */
> +	out_be32((void *)MPC5XXX_WU_GPIO_DATA_O,
> +		in_be32((void *)MPC5XXX_WU_GPIO_DATA_O) & ~(1 << 25));

And this should probably be replaced by

	clrbits_be32((void *)MPC5XXX_WU_GPIO_DATA_O, (1 << 25));

?


> diff --git a/cpu/mpc5xxx/ide.c b/cpu/mpc5xxx/ide.c
> index df5b4ac..88da199 100644
> --- a/cpu/mpc5xxx/ide.c
> +++ b/cpu/mpc5xxx/ide.c
> @@ -42,7 +42,7 @@ int ide_preinit (void)
>  	struct mpc5xxx_sdma *psdma = (struct mpc5xxx_sdma *) MPC5XXX_SDMA;
>  
>  	reg = *(vu_long *) MPC5XXX_GPS_PORT_CONFIG;
> -#if defined(CONFIG_TOTAL5200)
> +#if defined(CONFIG_TOTAL5200) || defined(CONFIG_DIGSY_MTC)
>  	/* ATA cs0/1 on i2c2 clk/io */
>  	reg = (reg & ~0x03000000ul) | 0x02000000ul;
>  #else

Instead of adding board-specific #ifdef's here, we should change it
to a feature-specific one, and #define some CONFIG_ATA_CS_ON_I2C2 or
similar in the two board config files.


What do you think?

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
My challenge to the goto-less programmer  is  to  recode  tcp_input()
without any gotos ... without any loss of efficiency (there has to be
a catch).                                             - W. R. Stevens