[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>:
> On Fri, 2013-11-29 at 17:32 +0800, Kuo-Jung Su wrote:
>> > 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'
>
> Well, even in current state of I2C core and some particular drivers
> (like DW I2C) no reformat of address happens as far as I can tell.
>

Did you mean designware_i2c.c?
Well... it does not support multi-bytes address, so it didn't reformat
the address in MSB order.

Please check line 208 ~ 211 & line 227 in following link:

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/designware_i2c.c;h=cb2ac04b609864412a8054888f3420bf35ca0287;hb=d19ad726bcd5d9106f7ba9c750462fcc369f1020

The soft_i2c.c is still the best example for this issue.

> But I cannot tell about each and every I2C driver in U-boot.
>

As I know both soft_i2c.c & zynq_i2c.c do the address reformat
(i.e., MSB shift)
I think the soft_i2c.c is the best example for this issue.
All the answer could be found from it.

e.g., CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW

is the solution for your EEPROM, but it's only get implemented
inside soft_i2c.c....

> Still if you'd like to modify "U-Boot's I2C model" it is up to you to
> start this discussion/work if you haven't start it yet (sorry I don't
> read entire U-boot mailing list daily).
>
> But then it's good to modify all drivers as well so nobody gets
> dead/not-built I2C drivers at some point, right?
>

No, it's merely a suggestion, not a critical bug fix request.
If the I2C devices supported in U-Boot are all use MSB address,
then it's no good to do it.
It simply looks weird....
e.g.,
CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW is only implemented
inside soft_i2c.c (i2c driver), not in cmd_eeprom.c
And thus some drivers (e.g., my fti2c010) might not support this feature.

>> > 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.
>
> Frankly I don't understand how passing entire "offset" will work in this
> particular corner-case. From my explanation you see that we have to
> mimic 8-bit address of target byte in EEPROM. And even if our I2C
> controller may use 10 bits for addressing it is something that we don't
> want use here - we need 8-bit as we do now.
>

Check soft_i2c.c, you could the answer in line 351 ~ 367

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/soft_i2c.c;h=396fea89af545bb2eba8a1827a21f423e717d40b;hb=d19ad726bcd5d9106f7ba9c750462fcc369f1020

Best Wishes
Dante Su

> Regards,
> Alexey
>



-- 
Best wishes,
Kuo-Jung Su