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

[Kuo-Jung Su]

2013/11/29 Alexey Brodkin <Alexey.Brodkin at synopsys.com>:
> Hi Kuo-Jung,
>
> On Fri, 2013-11-29 at 08:59 +0800, Kuo-Jung Su wrote:
>> No it's a common misunderstanding, I also made the same mistake before.
>> Please check my bug fix for Faraday FTI2C010 I2C driver:
>>
>> http://patchwork.ozlabs.org/patch/294732/
>>
>> The address should be rebuild inside the i2c driver in u-boot's I2C model.
>>
>> Another example could be found at drivers/i2c/soft_i2c.c, line 382 - 389
>>
>> static int soft_i2c_read (...)
>> {
>>     ......
>>     shift = (alen-1) * 8;
>>     while(alen-- > 0) {
>>         if(write_byte(addr >> shift)) {
>>             PRINTD("i2c_read, address not <ACK>ed\n");
>>             return(1);
>>         }
>>         shift -= 8;
>>     }
>>     ......
>> }
>>
>> However the root cause is the u-boot i2c driver model, the address should
>> never be passed to i2c_read/i2c_write in uint format.
>>
>> Please take a peak at how ecos defineds the relative i2c routines:
>>
>> externC cyg_uint32  cyg_i2c_transaction_tx(const cyg_i2c_device*,
>> cyg_bool, const cyg_uint8*, cyg_uint32, cyg_bool);
>> externC cyg_uint32  cyg_i2c_transaction_rx(const cyg_i2c_device*,
>> cyg_bool, cyg_uint8*, cyg_uint32, cyg_bool, cyg_bool);
>>
>> In this way, the eeprom address would be forced to rebuild in address
>> pointer (i.e., addr[]) as what we did in SPI mode.
>
> Unfortunately I still cannot agree with you.
> In my opinion I2C driver has nothing to do with current situation.
>

Yes, that's why I said the root cause is U-Boot's I2C model.
The address should never be rebuilt/reformated inside the I2C driver.
The better solution is to update the i2c_read/i2c_write to:

int i2c_read(struct i2c_adapter *adap, u8 dev, uint *addr, int alen,
uchar *buf, int len)

or

int i2c_read(struct i2c_adapter *adap, u8 dev, uchar *buf, int len)

i.e., drop the use of uint 'addr'

> It's purely how manufacturers of EEPROM use I2C.
> For example I we use "PCF8594C-2" EEPROM.
> Here's a datasheet -
> http://www.nxp.com/documents/data_sheet/PCF8594C_2.pdf
>
> Its capacity is 512 bytes. And as you may see from "Fig 3. Device
> addressing." Each IC houses 2 virtual EEPROMs (each housing 256 bytes of
> data).
>

Yes, it looks like a special case which use BIT0 of device address
for page selection. Which means we can't directly pass the device address
to i2c_read/i2c_write. But it's still o.k to directly the 'offset' as
what we did before.

> And if you accept this philosophy you'll understand why "I2C chip
> address" is modified and why following approach is used in
> "cmd_eeprom.c" for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1" (which says
> that EEPROM has address length of 1 byte):
> ==================
> addr[0] = offset >> 8;  /* block number */
> addr[1] = blk_off;      /* block offset */
> addr[0] |= dev_addr;    /* insert device address */
> ==================
>
> From source code above you see that virtually it addresses multiple 256
> byte EEPROMs.
>
> Regards,
> Alexey
>



-- 
Best wishes,
Kuo-Jung Su