[PATCH] misc: i2c_eeprom: implement different probe test eeprom offset

[Eugen.Hristev at microchip.com]

On 07.05.2020 13:00, Heiko Schocher wrote:
> Hello Baruch,
> 
> Am 06.05.2020 um 18:47 schrieb Baruch Siach:
>> Hi Eugen,
>>
>> On Wed, May 06 2020, Eugen Hristev wrote:
>>> Because of this commit :
>>> 5ae84860b0 ("misc: i2c_eeprom: verify that the chip is functional at 
>>> probe()")
>>> at probe time, each eeprom is tested for read at offset 0.
>>>
>>> The Atmel AT24MAC402 eeprom has different mapping. One i2c slave 
>>> address is
>>> used for the lower 0x80 bytes and another i2c slave address is used 
>>> for the
>>> upper 0x80 bytes. Because of this basically the i2c master sees 2 
>>> different
>>> slaves. We need the upper bytes because we read the unique MAC 
>>> address from
>>> this EEPROM area.
>>>
>>> However this implies that our slave address will return error on reads
>>> from address 0x0 to 0x80.
>>>
>>> To solve this, implemented a way to figure out from the compatible 
>>> udevice_id
>>> 'data' field which offset we should attempt to read.
>>> Added the offset as one byte in the data field (byte number 3).
>>> Created two macros that will convert offset to 'data' field and back.
>>>
>>> The probe function will now read this offset and use it, instead of 
>>> blindly
>>> checking offset 0.
>>>
>>> This will fix the regression noticed on these EEPROMs since the commit
>>> abovementioned that introduces the probe failed issue.
>>
>> Judging from the Linux at24 driver that commit appears to break a number
>> of other eeprom chips. I suggest to just revert it.
> 
> Hmm... how does the linux driver than work, as it has exact the same read?

Hi Heiko,

My feeling is that Linux driver is much more enhanced... it adds dummy 
devices for secondary addresses to be able to read the second part of 
the eeproms and adjusts the offset

  https://elixir.bootlin.com/linux/latest/source/drivers/misc/eeprom
/at24.c#L530

and

https://elixir.bootlin.com/linux/latest/source/drivers/misc/eeprom/at24.c#L673

In our u-boot case our driver is much simpler so we would have to cope 
with this scenario.

Heiko can you please look at the v2 of this patch. My fix is even more 
simpler with the new changes in U-boot. This patch is based on old u-boot

Thanks !

> 
> See:
> https://elixir.bootlin.com/linux/latest/source/drivers/misc/eeprom/at24.c#L710 
> 
> 
> So the fix from Eugen seems not too bad to me... but it should be
> done also in linux ... or?
> 
> bye,
> Heiko
>>
>> baruch
>>
>>>
>>> The offset field can for sure be updated later at a 2byte offset if 
>>> needed
>>> by anyone.
>>>
>>> Signed-off-by: Eugen Hristev <eugen.hristev at microchip.com>
>>> ---
>>>   drivers/misc/i2c_eeprom.c | 18 ++++++++++++++++--
>>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/misc/i2c_eeprom.c b/drivers/misc/i2c_eeprom.c
>>> index 3755dbf74b..79805cc46e 100644
>>> --- a/drivers/misc/i2c_eeprom.c
>>> +++ b/drivers/misc/i2c_eeprom.c
>>> @@ -11,6 +11,13 @@
>>>   #include <i2c.h>
>>>   #include <i2c_eeprom.h>
>>>
>>> +/* These macros are used to encode/decode the starting EEPROM offset 
>>> into the
>>> + * udevice_id structure's data field 3rd byte.
>>> + * Lower 2 bytes of the data field are used for pagewidth.
>>> + */
>>> +#define I2C_EEPROM_OFFSET_TO_DATA(v) ((v) << 16)
>>> +#define I2C_EEPROM_DATA_TO_OFFSET(v) ((v) >> 16)
>>> +
>>>   int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf, 
>>> int size)
>>>   {
>>>      const struct i2c_eeprom_ops *ops = device_get_ops(dev);
>>> @@ -85,11 +92,17 @@ static int 
>>> i2c_eeprom_std_ofdata_to_platdata(struct udevice *dev)
>>>
>>>   static int i2c_eeprom_std_probe(struct udevice *dev)
>>>   {
>>> +    u64 data = dev_get_driver_data(dev);
>>>      u8 test_byte;
>>>      int ret;
>>>
>>>      /* Verify that the chip is functional */
>>> -    ret = i2c_eeprom_read(dev, 0, &test_byte, 1);
>>> +    /*
>>> +     * Not all eeproms start from offset 0. Valid offset is encoded in
>>> +     * upper bits of the data (byte 3).
>>> +     */
>>> +    ret = i2c_eeprom_read(dev, I2C_EEPROM_DATA_TO_OFFSET(data) & 0xFF,
>>> +                          &test_byte, 1);
>>>      if (ret)
>>>              return -ENODEV;
>>>
>>> @@ -105,7 +118,8 @@ static const struct udevice_id 
>>> i2c_eeprom_std_ids[] = {
>>>      { .compatible = "atmel,24c08", .data = 4 },
>>>      { .compatible = "atmel,24c08a", .data = 4 },
>>>      { .compatible = "atmel,24c16a", .data = 4 },
>>> -    { .compatible = "atmel,24mac402", .data = 4 },
>>> +    { .compatible = "atmel,24mac402",
>>> +            .data = (4 | I2C_EEPROM_OFFSET_TO_DATA(0x80))},
>>>      { .compatible = "atmel,24c32", .data = 5 },
>>>      { .compatible = "atmel,24c64", .data = 5 },
>>>      { .compatible = "atmel,24c128", .data = 6 },
>>
>>
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de