[PATCH] misc: i2c_eeprom: fix at24c32 offset_len

[PATCH] misc: i2c_eeprom: fix at24c32 offset_len

[Eugen Hristev]

According to at24c32 datasheet:

RANDOM READ: A random read requires a “dummy” byte write sequence to load in
the dataword address. Once the device address word and data word address are
clocked in and acknowledged by the EEPROM, the microcontroller must generate
another start condition.

BYTE WRITE: A write operation requires two 8-bit data word addresses following
the device address word and acknowledgment. Upon receipt of this address, the
EEPROM will again respond with a zero and then clock in the first 8-bit data
word.

From this, my understanding is that dataword is 1 byte, and when reading the
offset is just 1 byte.

By having an offset len of 2 bytes, reading from the eeprom at boot time to
read the stored MAC address fails on sam9x60ek board which has this eeprom.

When changing the offset len to 1 byte, reading the MAC address from the offset
inside the EEPROM works correctly.

Fixes: 821c982e35 ("misc: i2c_eeprom: set offset len and chip addr offset mask")
Reported-by: Sergiu Moga <sergiu.moga@microchip.com>
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---

Hi everyone,

We tested this on sam9x60ek board. I see in DT that various other boards
use this eeprom.
I have added several people to the patch e-mail, however not comprehensively
everyone.
I do not wish to affect functionality for other boards, even though this
patch appears correct.
Some other tests would be welcome, on other boards having this EEPROM.

Thanks!
Eugen

 drivers/misc/i2c_eeprom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/i2c_eeprom.c b/drivers/misc/i2c_eeprom.c
index 89a450d0f8..c8c67cf028 100644
--- a/drivers/misc/i2c_eeprom.c
+++ b/drivers/misc/i2c_eeprom.c
@@ -230,7 +230,7 @@ static const struct i2c_eeprom_drv_data atmel24c32_data = {
 	.size = 4096,
 	.pagesize = 32,
 	.addr_offset_mask = 0,
-	.offset_len = 2,
+	.offset_len = 1,
 };
 
 static const struct i2c_eeprom_drv_data atmel24c64_data = {
-- 
2.25.1

Re: [PATCH] misc: i2c_eeprom: fix at24c32 offset_len

[Rasmus Villemoes]

On 28/04/2022 13.11, Eugen Hristev wrote:
> According to at24c32 datasheet:
> 
> RANDOM READ: A random read requires a “dummy” byte write sequence to load in
> the dataword address. Once the device address word and data word address are
> clocked in and acknowledged by the EEPROM, the microcontroller must generate
> another start condition.
> 
> BYTE WRITE: A write operation requires two 8-bit data word addresses following
> the device address word and acknowledgment. Upon receipt of this address, the
> EEPROM will again respond with a zero and then clock in the first 8-bit data
> word.
> 
> From this, my understanding is that dataword is 1 byte, and when reading the
> offset is just 1 byte.

Yes, you read data byte by byte, but that doesn't mean all those bytes
can be addressed using a single byte...

>  drivers/misc/i2c_eeprom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/i2c_eeprom.c b/drivers/misc/i2c_eeprom.c
> index 89a450d0f8..c8c67cf028 100644
> --- a/drivers/misc/i2c_eeprom.c
> +++ b/drivers/misc/i2c_eeprom.c
> @@ -230,7 +230,7 @@ static const struct i2c_eeprom_drv_data atmel24c32_data = {
>  	.size = 4096,
>  	.pagesize = 32,
>  	.addr_offset_mask = 0,
> -	.offset_len = 2,
> +	.offset_len = 1,
>  };

But this can't be correct, because how could one then possibly address
all 4096 bytes? Note that some eeproms larger than 256 bytes exist but
still use a 1-byte address; that's because they then respond to multiple
i2c addresses - that's what the "addr_offset_mask" is about.

Something else must be going on in your case, I think. Are you sure the
device tree is correct, i.e. that the eeprom is actually that one and
not one that does indeed use 1-byte addressing? I got curious and
downloaded the "SAM9X60-EK Board Design Files" from
https://www.microchip.com/en-us/development-tool/DT100126#Documentation,
inside which one finds mention of a "MCHP MEMORY SERIAL EEPROM 2Kb I2C
24AA025E48T-I/OT SOT-23-6", and 2Kb == 256 byte.

If you have the physical board handy, I'd try to locate the eeprom and
see what's printed on it.

Rasmus

Re: [PATCH] misc: i2c_eeprom: fix at24c32 offset_len

[Eugen.Hristev]

On 4/28/22 2:35 PM, Rasmus Villemoes wrote:
> On 28/04/2022 13.11, Eugen Hristev wrote:
>> According to at24c32 datasheet:
>>
>> RANDOM READ: A random read requires a “dummy” byte write sequence to load in
>> the dataword address. Once the device address word and data word address are
>> clocked in and acknowledged by the EEPROM, the microcontroller must generate
>> another start condition.
>>
>> BYTE WRITE: A write operation requires two 8-bit data word addresses following
>> the device address word and acknowledgment. Upon receipt of this address, the
>> EEPROM will again respond with a zero and then clock in the first 8-bit data
>> word.
>>
>>  From this, my understanding is that dataword is 1 byte, and when reading the
>> offset is just 1 byte.
> 
> Yes, you read data byte by byte, but that doesn't mean all those bytes
> can be addressed using a single byte...
> 
>>   drivers/misc/i2c_eeprom.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/i2c_eeprom.c b/drivers/misc/i2c_eeprom.c
>> index 89a450d0f8..c8c67cf028 100644
>> --- a/drivers/misc/i2c_eeprom.c
>> +++ b/drivers/misc/i2c_eeprom.c
>> @@ -230,7 +230,7 @@ static const struct i2c_eeprom_drv_data atmel24c32_data = {
>>        .size = 4096,
>>        .pagesize = 32,
>>        .addr_offset_mask = 0,
>> -     .offset_len = 2,
>> +     .offset_len = 1,
>>   };
> 
> But this can't be correct, because how could one then possibly address
> all 4096 bytes? Note that some eeproms larger than 256 bytes exist but
> still use a 1-byte address; that's because they then respond to multiple
> i2c addresses - that's what the "addr_offset_mask" is about.
> 
> Something else must be going on in your case, I think. Are you sure the
> device tree is correct, i.e. that the eeprom is actually that one and
> not one that does indeed use 1-byte addressing? I got curious and
> downloaded the "SAM9X60-EK Board Design Files" from
> https://www.microchip.com/en-us/development-tool/DT100126#Documentation,
> inside which one finds mention of a "MCHP MEMORY SERIAL EEPROM 2Kb I2C
> 24AA025E48T-I/OT SOT-23-6", and 2Kb == 256 byte.
> 
> If you have the physical board handy, I'd try to locate the eeprom and
> see what's printed on it.


Hi,

You are right. I thought that this EEPROM was handled by the same 
compatible, since this is how it's been in DT and the same in Linux DT. 
Then I looked at the 24c32 datasheet and could not understand what was 
the problem there, since it worked... except offset addressing.
Using a 1byte offset len makes it work great, but as you said, it will 
break boards with the real 24c32 trying to access beyond 1 byte addresses.
I will try to change the compatible to the correct 24aa025e48 and try 
this again, it will likely work because in the driver 24aa02e48 really 
has an offset_len of 1 byte.

24aa02e48 and 24aa025e48 differ in page size, 8 bytes vs 16 bytes, so I 
will add 24aa025e48 with a page size of 16 first and then change the DT 
for the board.

Thanks for helping with this ! (so the patch can be dropped)

Eugen

> 
> Rasmus
>