Dear Grzegorz Bernacki,
In message <12345332203460-git-send-email-...@semihalf.com> you wrote:
>
> Signed-off-by: Grzegorz Bernacki <g...@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 *)ð_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: w...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
