Hi Wolfgang, Thanks for checking the patch, Please see inline.
On Fri, 09 Apr 2010 18:51:06 -0400, Wolfgang Denk <w...@denx.de> wrote: > Dear "David Wu", > > In message <op.vatgy2wjqig...@cyprus.local> you wrote: >> Signed-off-by: David Wu <davi...@arcturusnetworks.com> >> --- >> board/Mercury/ep2500/Makefile | 44 ++++++ >> board/Mercury/ep2500/config.mk | 23 +++ >> board/Mercury/ep2500/ep2500.c | 191 +++++++++++++++++++++++++ >> board/Mercury/ep2500/u-boot.lds | 140 ++++++++++++++++++ >> include/configs/EP2500.h | 297 >> +++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 695 insertions(+), 0 deletions(-) >> create mode 100644 board/Mercury/ep2500/Makefile >> create mode 100644 board/Mercury/ep2500/config.mk >> create mode 100644 board/Mercury/ep2500/ep2500.c >> create mode 100644 board/Mercury/ep2500/u-boot.lds >> create mode 100644 include/configs/EP2500.h > > Your Subject (= commit message tile) is WAY too long. Let me know the size of subject line. > Don't try to press everything in a single line. > > Entries to MAINTAINERS and MAKEALL are missing. Will add them in. My patch is actually applied on top of another set of patches from TsiChung. >> +void board_get_enetaddr(uchar *enet) >> +{ >> + int i; >> + unsigned char buff[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; >> + >> + /* Read MAC address (6 bytes) in EEPROM */ >> + if (i2c_read >> + (CONFIG_SYS_I2C_EEPROM_ADDR, MAC_ADDRESS_OFFSET_IN_EEPROM, >> + CONFIG_SYS_I2C_EEPROM_ADDR_LEN, buff, 6) != 0) { >> + puts("Error reading the EEPROM.\n"); >> + } >> + >> + /* >> + * When buff returns all 0xFF the EEPROM has not >> + * been programed with a valid MAC. In this case >> + * we set enet to all 0x00 as 0xFF is not valid >> + * for this usage model. >> + */ >> + if (buff[0] == 0xff && buff[1] == 0xff && buff[2] == 0xff && >> + buff[3] == 0xff && buff[4] == 0xff && buff[5] == 0xff) { > > We have standard functions for such checks. Please use them. It would be nice if you can let me know the function. I am grep'ing in u-boot and it's so hard to find it. >> + for (i = 0; i < 6; i++) >> + enet[i] = 0; >> + } else { >> + for (i = 0; i < 6; i++) >> + enet[i] = buff[i]; >> + } > > Hm... the whole code makes no sense to me, as neiter all-ones nor > all-zeros are legal MAC addresses. All ones are valid broadcast MAC address. It is just not valid for source MAC address. I want to set to all zeros instead in case of all FFs. Is this against any rules? > > >> +int misc_init_r(void) >> +{ >> +#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C) >> + uchar enetaddr[6]; >> + >> + if (!eth_getenv_enetaddr("ethaddr", enetaddr)) { >> + board_get_enetaddr(enetaddr); >> + eth_setenv_enetaddr("ethaddr", enetaddr); > > Please see recent discussion about this topic. Could you let me know the thread -- I am new here. > >> +void i2c_init_board(void) >> +{ >> + struct fsl_i2c *dev; >> + >> + dev = (struct fsl_i2c *)(CONFIG_SYS_IMMR + CONFIG_SYS_I2C_OFFSET); >> + >> + if (readb(&dev->sr) & I2C_SR_MBB) { >> + writeb(0, &dev->cr); >> + writeb(0xa0, &dev->cr); >> + readb(&dev->dr); >> + writeb(0x0, &dev->sr); >> + writeb(0, &dev->cr); >> + writeb(I2C_CR_MEN, &dev->cr); > > Please add comments what you are doing. Replace 0x0A by a symbolic You mean comments for "writeb(0xa0, &dev->cr)" -- OK. > name. Use a consistent style (i. e. wither use "0x0" or "0", but > don't mix both styles. >> +#ifdef CONFIG_MCFFEC >> +# define CONFIG_ETHADDR 00:00:00:00:00:00 >> +# define CONFIG_IPADDR 192.168.1.2 >> +# define CONFIG_NETMASK 255.255.255.0 >> +# define CONFIG_SERVERIP 192.168.1.1 >> +# define CONFIG_GATEWAYIP 192.168.1.1 >> +# define CONFIG_OVERWRITE_ETHADDR_ONCE >> +/*#define CONFIG_ENV_OVERWRITE */ > > Please remove this, it will not be accepted. Are you talking about the last line. Sure I can remove it. Otherwise would you mind tell me the reason? format is wrong, some defines are wrong or ifdef (or use if defined instead)? Kind regards, David > > Best regards, > > Wolfgang Denk _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot