[U-Boot] [PATCH v3 4/4] cmd_eeprom: bug fix for i2c read/write

[Kuo-Jung Su]

2013/12/2 Alexey Brodkin <Alexey.Brodkin at synopsys.com>:
> On Mon, 2013-12-02 at 16:02 +0800, Kuo-Jung Su wrote:
>> From: Kuo-Jung Su <dantesu at 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
>



-- 
Best wishes,
Kuo-Jung Su