| Message ID | 20200506160813.538933-1-eugen.hristev@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc: i2c_eeprom: implement different probe test eeprom offset | expand |
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 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 },
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 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 }, > > > -- > ~. .~ Tk Open Systems > =}------------------------------------------------ooO--U--Ooo------------{= > - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - >
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 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 }, > >
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
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 },
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 at microchip.com> --- drivers/misc/i2c_eeprom.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)