[U-Boot] [PATCH 3/7] added Mercury EP2500 board support It uses the mcf5282 processor with real time clock and EEPROM.

[David Wu]

Hi Wolfgang,

Thanks for checking the patch, Please see inline.

On Fri, 09 Apr 2010 18:51:06 -0400, Wolfgang Denk <wd at denx.de> wrote:

> Dear "David Wu",
>
> In message <op.vatgy2wjqigx4y at cyprus.local> you wrote:
>> Signed-off-by: David Wu <davidwu at 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