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

[Kuo-Jung Su]

2013/12/10 Alexey Brodkin <Alexey.Brodkin at synopsys.com>:
> On Mon, 2013-12-09 at 12:21 +0100, Heiko Schocher wrote:
>> I thought the v3 patch just rolls things back as patch comment states:
>> ------------------------
>>   Changes for v3:
>>    - It turns out that what we did before 2013-11-13
>>      (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM)
>>      is still the best one, this patch simply rollback to it with coding
>>      style fix.
>> ------------------------
>>
>> Without this roll back, I think, some boards are broken now in current
>> mainline... so first we should go this step back.
>
> I'd say it's very questionable. For example if you look at I2C driver
> that Kuo-Jung refers to ("soft_i2c") you'll find that with my patch
> applied (the one you've just rolled back) it will work flawlessly
> because:
> 1. "chip" address there has double applied MSB bits of offset (the first
> time it gets applied in "cmd_eeprom")
> 2. Only LSB byte of offset gets written in I2C device if addr_len = 1.

But if addr_len = 2, everything goes wrong. This is the real problem here...

>
> This one makes IMHO much more sense because it excludes an extra "chip"
> address modification - so it's clear that it will be done by particular
> I2C driver so people won't be confused with their expectations.
>
>>  > BTW what I cannot understand is why "soft_i2c_read" has this "chip"
>>  > modification part while "soft_i2c_write" doesn't? Is it done on purpose?
>>  > And how it actually works at all then?
>>
>> Good question ... maybe currently only used on i2c reads ?
>
> Frankly I have only 1 supposition regarding this strange state of things
> in "soft_i2c_write". Because without any doubts the same modification of
> "chip" address is applicable to both "read" and "write" because it is an
> integral part of data addressing.
>
> But due to discussed a lot in this thread "double chip address
> modification" (application of MSB bits of offset) proper support of
> "chip" address was never implemented in "soft_i2c_write". As you
> correctly mentioned - "real ancient code" works and nobody has problems
> with it. Well, just because we have current implementation of
> "eeprom_write" that does "chip" address modification "soft_i2c" may not
> have this feature in both "read" and "write".
>
> So I'd prefer to go with previous version of patch sent by Kuo-Jung
> http://patchwork.ozlabs.org/patch/294733/
>
> And indeed this will "break" functionality of currently incomplete
> implementation of "soft_i2c_write".
>

No, it will works just fine.

1. eeprom_read:
The overflowed address was shifted to chip address by cmd_eeprom.c, and then
the same operations would be repeated by soft_i2c.c, but it's no harm
to do a OR operation twice
on the same bits, so everything is all right.

2. eeprom_write:
The overflowed address was shifted to chip address by cmd_eeprom.c,
and get passed to soft_i2c.c
without further modification. So everything is all right.

The 'CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW' in soft_i2c.c is actually a
redundant code snippet.
And it would be better to move/copied its comment to cmd_eeprom.c.

Which means if we don't rollback, none of the EEPROMs with addr_len >
1 will work on u-boot.
And this rollback actually does no harm to the EEPROMs with addr_len = 1 or 2.

P.S: The designware_i2c.c should work just fine with this rollback, too....

-- 
Best wishes,
Kuo-Jung Su