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

Wolfgang Denk
2009-03-12T12:56:01Z

Dear Grzegorz Bernacki,

In message <12345332203460-git-send-email-gjb@semihalf.com> you wrote:
>
> Signed-off-by: Grzegorz Bernacki <gjb@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