2013/12/2 Alexey Brodkin Alexey.Brodkin@synopsys.com:
On Mon, 2013-12-02 at 16:02 +0800, Kuo-Jung Su wrote:
From: Kuo-Jung Su dantesu@faraday-tech.com
diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c index 02539c4..3924805 100644 --- a/common/cmd_eeprom.c +++ b/common/cmd_eeprom.c @@ -161,7 +161,7 @@ int eeprom_read (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cnt #if defined(CONFIG_SPI) && !defined(CONFIG_ENV_EEPROM_IS_ON_I2C) spi_read (addr, alen, buffer, len); #else
if (i2c_read (addr[0], addr[1], alen-1, buffer, len) != 0)
if (i2c_read(addr[0], offset, alen - 1, buffer, len)) rcode = 1;#endif buffer += len;
I think this change is whether incomplete or improper. Let's look at source code above line 161: ============================= 125 #if CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1 && !defined(CONFIG_SPI_X) 126 uchar addr[2]; 127 128 blk_off = offset & 0xFF; /* block offset */ 129 130 addr[0] = offset >> 8; /* block number */ 131 addr[1] = blk_off; /* block offset */ 132 alen = 2; 133 #else 134 uchar addr[3]; 135 136 blk_off = offset & 0xFF; /* block offset */ 137 138 addr[0] = offset >> 16; /* block number */ 139 addr[1] = offset >> 8; /* upper address octet */ 140 addr[2] = blk_off; /* lower address octet */ 141 alen = 3; 142 #endif /* CONFIG_SYS_I2C_EEPROM_ADDR_LEN, CONFIG_SPI_X */ 143 144 addr[0] |= dev_addr; /* insert device address */ =============================
From these line you see that "addr[0]" is set like this:
If "CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1": addr[0] = offset >> 8;
If "CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1": addr[0] = offset >> 16;
And in both cases then OR with "dev_addr": addr[0] |= dev_addr; ===========
This is the reason why I said:
CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW is always enabled inside cmd_eeprom.c
so everything is O.K.
In other words it gets both real I2S slave address + MSB bits of offset. But note that "offset" value stays unchanged.
The comment bellow clearly explain the issue here.
soft_i2c.c: line 351 ~ 367:
#ifdef CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW /* * EEPROM chips that implement "address overflow" are ones * like Catalyst 24WC04/08/16 which has 9/10/11 bits of * address and the extra bits end up in the "chip address" * bit slots. This makes a 24WC08 (1Kbyte) chip look like * four 256 byte chips. * * Note that we consider the length of the address field to * still be one byte because the extra address bits are * hidden in the chip address. */ chip |= ((addr >> (alen * 8)) & CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW);
PRINTD("i2c_read: fix addr_overflow: chip %02X addr %02X\n", chip, addr); #endif
So if you pass both "addr[0]" (which already has MSB bits of "offset") and "offset" itself then you'll get completely incorrect I2C command.
@@ -339,7 +339,7 @@ int eeprom_write (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cn /* Write is enabled ... now write eeprom value. */ #endif
if (i2c_write (addr[0], addr[1], alen-1, buffer, len) != 0)
if (i2c_write(addr[0], offset, alen - 1, buffer, len)) rcode = 1;#endif
Same goes to "eeprom_write".
Moreover I'd say that this address/offset tricks are very EEPROM-specific and because of this we'd better keep it here and don't modify generic I2C code.
Yes,the address/offset tricks are device specific (not only EEPROM, it also applies to Audio Codecs..etc.) But this code was there over a decade. And if everything works just fine, why bother ?
Regards, Alexey