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

[Eugen.Hristev at microchip.com <Eugen.Hristev@microchip.com>]

On 28.05.2020 06:59, Heiko Schocher wrote:
> Hello Eugen,
> 
> Am 07.05.2020 um 17:08 schrieb Eugen.Hristev at microchip.com:
>> On 07.05.2020 18:02, Baruch Siach wrote:
>>> Hi Heiko,
>>>
>>> On Thu, May 07 2020, Heiko Schocher wrote:
>>>> Am 07.05.2020 um 10:53 schrieb Eugen Hristev:
>>>>> 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 an offset field inside platform data 
>>>>> that is by
>>>>> default 0 (as it is used now), but can be changed in the compatible 
>>>>> table.
>>>>>
>>>>> The probe function will now read at 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.
>>>>>
>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>>> ---
>>>>>
>>>>> Please disregard patch
>>>>> [PATCH] misc: i2c_eeprom: implement different probe test eeprom offset
>>>>>
>>>>> as that patch was mistakenly done on an older u-boot version.
>>>>> This v2 patch applies correctly on u-boot/master
>>>>>
>>>>> Changes in v2:
>>>>> - adapt to latest changes in driver. v1 was done on u-boot 2020.01 
>>>>> accidentaly.
>>>>>
>>>>> ??? drivers/misc/i2c_eeprom.c | 8 +++++++-
>>>>> ??? 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> Thanks for rebasing!
>>>>
>>>> I prefer to fix the issue instead reverting commit:
>>>>
>>>> 5ae84860b0 ("misc: i2c_eeprom: verify that the chip is functional at 
>>>> probe()")
>>>>
>>>> Reviewed-by: Heiko Schocher <hs@denx.de>
>>>>
>>>> @Baruch, is this Ok for you?
>>>
>>> According to the Linux driver there are more devices that need read
>>> offset. 24cs32 and 24cs64 are affected. This patch does not fix the
>>> regression for those devices.
>>>
>>> Eugen, would it be possible for you to extend the fix to 24cs32/64 and
>>> test on real hardware?
>>
>> Hi Baruch,
>>
>> This patch fixes my issue at hand and the eeprom which I have at my
>> disposal. I will look to see if I have those, but, most likely not.
>> I can extend the fix, but I cannot test, so, I rather not go blind 
>> with it.
> 
> Any news here?

Hello Heiko,

I do not have any other memory in the compatible list at my disposal 
right now. I assumed that you would take this patch as-is. If I hit the 
issue on another memory, I will send another patch.
Right now with the lockdown this is the only memory I have.

Eugen
> 
> Thanks!
> 
> bye,
> HEiko
>>
>> Eugen
>>>
>>> baruch
>>>
>>>>> diff --git a/drivers/misc/i2c_eeprom.c b/drivers/misc/i2c_eeprom.c
>>>>> index ef5f103c98..32a1b20856 100644
>>>>> --- a/drivers/misc/i2c_eeprom.c
>>>>> +++ b/drivers/misc/i2c_eeprom.c
>>>>> @@ -17,6 +17,7 @@ struct i2c_eeprom_drv_data {
>>>>> ?????? u32 pagesize; /* page size in bytes */
>>>>> ?????? u32 addr_offset_mask; /* bits in addr used for offset 
>>>>> overflow */
>>>>> ?????? u32 offset_len; /* size in bytes of offset */
>>>>> +??? u32 start_offset; /* valid start offset inside memory, by 
>>>>> default 0 */
>>>>> ??? };
>>>>> ????? int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t 
>>>>> *buf, int
>>>>> size)
>>>>> @@ -147,7 +148,11 @@ static int i2c_eeprom_std_probe(struct udevice 
>>>>> *dev)
>>>>> ?????? i2c_set_chip_addr_offset_mask(dev, data->addr_offset_mask);
>>>>> ?????? /* 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 available
>>>>> +???? * in the platform data struct.
>>>>> +???? */
>>>>> +??? ret = i2c_eeprom_read(dev, data->start_offset, &test_byte, 1);
>>>>> ?????? if (ret)
>>>>> ?????????????? return -ENODEV;
>>>>> ??? @@ -215,6 +220,7 @@ static const struct i2c_eeprom_drv_data
>>>>> atmel24mac402_data = {
>>>>> ?????? .pagesize = 16,
>>>>> ?????? .addr_offset_mask = 0,
>>>>> ?????? .offset_len = 1,
>>>>> +??? .start_offset = 0x80,
>>>>> ??? };
>>>>> ????? static const struct i2c_eeprom_drv_data atmel24c32_data = {
>>>>>
>>>
>>>
>>> -- 
>>> ?????????????????????????????????????????????????????? ~. .~?? Tk 
>>> Open Systems
>>> =}------------------------------------------------ooO--U--Ooo------------{= 
>>>
>>> ???? - baruch at tkos.co.il - tel: +972.52.368.4656, 
>>> http://www.tkos.co.il -
>>>
>>
> 
> -- 
> 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