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

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

[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 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.

The offset field can for sure be updated later at a 2byte offset if needed
by anyone.

Signed-off-by: Eugen Hristev <eugen.hristev@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 },
-- 
2.25.1

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

[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.

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@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 },


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

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

[Eugen.Hristev at microchip.com]

On 06.05.2020 19:47, Baruch Siach wrote:
> 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.
> 
> baruch
> 

Hi,

Please disregard this patch as it's done for u-boot 2020.01 . I did not 
notice driver changed in later versions. I sent a v2 that is done for 
the master u-boot.

Thanks !

>>
>> The offset field can for sure be updated later at a 2byte offset if needed
>> by anyone.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@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 },
> 
> 
> --
>                                                       ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>     - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
> 

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

[Heiko Schocher]

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?

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@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

[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@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