[U-Boot-Users] [PATCH V7] ARM: Add support for Lyrtech SFF-SDR board (ARM926EJS)

[Hugo Villeneuve]

wd at denx.de wrote:
> In message
> <1212760465-16024-1-git-send-email-hugo.villeneuve at lyrtech.com> you
> wrote:  
>> ARM: This patch adds support for the Lyrtech SFF-SDR
>> board, based on the TI DaVinci architecture (ARM926EJS).
> ...
>> +int read_mac_address(uint8_t *buf)
>> +{
>> +	u_int32_t value, mac[2], address;
>> +
>> +	/* Read Integrity data structure checkword. */
>> +	if (i2c_read(CFG_I2C_EEPROM_ADDR, INTEGRITY_CHECKWORD_OFFSET,
>> +		     CFG_I2C_EEPROM_ADDR_LEN, (uint8_t *) &value, 4)) {
>> +		printf("Read from EEPROM @ 0x%02x failed\n",
>> +		       CFG_I2C_EEPROM_ADDR);
>> +		return 1;
> 
> # 1
> # 2
> # 3
> # 4
> # 5
> Instead of repeating the same code 5 times, replace it by a "goto
> err;" 
> and add here:
> 
> err:
> 	printf("Read from EEPROM @ 0x%02x failed\n", CFG_I2C_EEPROM_ADDR);
> 	return 1;
Fixed.
 
>> +	/* Configure I2C switch (PCA9543) to enable channel 0. */
>> +	tmp[0] = CFG_I2C_PCA9543_ENABLE_CH0;
>> +	if (i2c_write(CFG_I2C_PCA9543_ADDR, 0,
>> +		      CFG_I2C_PCA9543_ADDR_LEN, tmp, 1))
>> +		printf("Write to MUX @ 0x%02x failed\n", CFG_I2C_PCA9543_ADDR);
> 
> And this is not an error that needs to be handled?
Fixed.

>> +	/* Set Ethernet MAC address from EEPROM if available. */
>> +	if (read_mac_address(buf) == 0) {
>> +		if ((buf[0] != 0xff) && (getenv("ethaddr") == NULL)) {
>> +			sprintf((char *)&tmp[0], +				"%02x:%02x:%02x:%02x:%02x:%02x",
>> +				buf[0], buf[1], buf[2], buf[3],
>> +				buf[4], buf[5]);
>> +			setenv("ethaddr", (char *)&tmp[0]);
>> +		}
>> +	}
> 
> Hm. I always thought this was documented somewhere (probably it's in
> the 
> CHANGELOG ?), but I didn't find it right now. The intended "standard"
> behaviour should be:
>
> If there is a MAC address in  the  environment,  and  if  it  is  not
> identical  to  the  MAC  address  in the ROM, then a warning shall be
> printed, and the MAC address from the environment be used.
> 
> If there is no MAC address in  the  environment,  then  it  shall  be
> initialized (silently) from the value in the ROM.
> 
> See for example drivers/net/cs8900.c (which is the first driver where
> we implemented this, IIRC).
Fixed. I used the exact same code found in drivers/net/cs8900.c.

>> +	/* For most DaVinci boards, U-Boot is copied in RAM by the UBL,
>> +	 * so we are always in the relocated state. This is
>> +	 * necessary for command history to work. */
>> +	gd->flags |= GD_FLG_RELOC;
> 
> "most" != "all". What happens for the N (N = all - most) remaining
> boards?
Fixed.


Hugo Villeneuve
Hardware developer | Concepteur matériel
Lyrtech
Phone/Tél. : (1) (418) 877-4644 #2395
Toll-free/Sans frais - Canada & USA : (1) (888) 922-4644 #2395
Fax/Téléc. : (1) (418) 877-7710
www.lyrtech.com
Infinite possibilities...TM