| Message ID | 20220823160534.16390-1-matwey.kornilov@gmail.com |
|---|---|
| State | Accepted |
| Commit | bf6376642fe8295f887144999713ebe644388205 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | board: ti: common: board_detect: Fix EEPROM read quirk | expand |
On 19:05-20220823, Matwey V. Kornilov wrote: > There are three different kinds of EEPROM possibly present on boards. > 1. 1byte address. For those we should avoid 2byte address in order > not to rewrite the data. Second byte of the address can potentially > be interpreted as the data to write. > 2. 2byte address with defined behaviour. When we try to use 1byte > address they just return "FF FF FF FF ... FF" > 3. 2byte address with undefined behaviour (for instance, 24LC32AI). > When we try to use 1byte address, then their internal read > pointer is changed to some value. Subsequential reads may be > broken. > > To gracefully handle both case #1 and case #3 we read all required > data from EEPROM at once (about 80 bytes). So either all the data is > valid or we fallback to 2byte address. I would suggest to add a note that this was measured as adding extra time in startup time. With that: Acked-by: Nishanth Menon <nm@ti.com> > > Cc: Nishanth Menon <nm@ti.com> > Fixes: a58147c2dbbf ("board: ti: common: board_detect: Do 1byte address checks first.") > Reference: https://lore.kernel.org/all/CAJs94Ebdd4foOjhGFu9Bop0v=B1US9neDLxfhgcY23ukgLzFOQ@mail.gmail.com/ > Signed-off-by: Matwey V. Kornilov <matwey.kornilov@gmail.com> > --- > board/ti/common/board_detect.c | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-) > > diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c > index ed34991377..fdf83fcfb0 100644 > --- a/board/ti/common/board_detect.c > +++ b/board/ti/common/board_detect.c > @@ -86,7 +86,6 @@ __weak void gpi2c_init(void) > static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, > u32 header, u32 size, uint8_t *ep) > { > - u32 hdr_read = 0xdeadbeef; > int rc; > > #if CONFIG_IS_ENABLED(DM_I2C) > @@ -113,10 +112,10 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, > * We must allow for fall through to check the data if 2 byte > * addressing works > */ > - (void)dm_i2c_read(dev, 0, (uint8_t *)&hdr_read, 4); > + (void)dm_i2c_read(dev, 0, ep, size); > > /* Corrupted data??? */ > - if (hdr_read != header) { > + if (*((u32 *)ep) != header) { > /* > * read the eeprom header using i2c again, but use only a > * 2 byte address (some newer boards need this..) > @@ -125,16 +124,12 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, > if (rc) > return rc; > > - rc = dm_i2c_read(dev, 0, (uint8_t *)&hdr_read, 4); > + rc = dm_i2c_read(dev, 0, ep, size); > if (rc) > return rc; > } > - if (hdr_read != header) > + if (*((u32 *)ep) != header) > return -1; > - > - rc = dm_i2c_read(dev, 0, ep, size); > - if (rc) > - return rc; > #else > u32 byte; > > @@ -154,26 +149,21 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, > * We must allow for fall through to check the data if 2 byte > * addressing works > */ > - (void)i2c_read(dev_addr, 0x0, byte, (uint8_t *)&hdr_read, 4); > + (void)i2c_read(dev_addr, 0x0, byte, ep, size); > > /* Corrupted data??? */ > - if (hdr_read != header) { > + if (*((u32 *)ep) != header) { > /* > * read the eeprom header using i2c again, but use only a > * 2 byte address (some newer boards need this..) > */ > byte = 2; > - rc = i2c_read(dev_addr, 0x0, byte, (uint8_t *)&hdr_read, > - 4); > + rc = i2c_read(dev_addr, 0x0, byte, ep, size); > if (rc) > return rc; > } > - if (hdr_read != header) > + if (*((u32 *)ep) != header) > return -1; > - > - rc = i2c_read(dev_addr, 0x0, byte, ep, size); > - if (rc) > - return rc; > #endif > return 0; > } > -- > 2.26.2 >
On Tue, Aug 23, 2022 at 07:05:34PM +0300, Matwey V. Kornilov wrote: > There are three different kinds of EEPROM possibly present on boards. > 1. 1byte address. For those we should avoid 2byte address in order > not to rewrite the data. Second byte of the address can potentially > be interpreted as the data to write. > 2. 2byte address with defined behaviour. When we try to use 1byte > address they just return "FF FF FF FF ... FF" > 3. 2byte address with undefined behaviour (for instance, 24LC32AI). > When we try to use 1byte address, then their internal read > pointer is changed to some value. Subsequential reads may be > broken. > > To gracefully handle both case #1 and case #3 we read all required > data from EEPROM at once (about 80 bytes). So either all the data is > valid or we fallback to 2byte address. > > Cc: Nishanth Menon <nm@ti.com> > Fixes: a58147c2dbbf ("board: ti: common: board_detect: Do 1byte address checks first.") > Reference: https://lore.kernel.org/all/CAJs94Ebdd4foOjhGFu9Bop0v=B1US9neDLxfhgcY23ukgLzFOQ@mail.gmail.com/ > Signed-off-by: Matwey V. Kornilov <matwey.kornilov@gmail.com> > Acked-by: Nishanth Menon <nm@ti.com> Applied to u-boot/master, thanks!
diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c index ed34991377..fdf83fcfb0 100644 --- a/board/ti/common/board_detect.c +++ b/board/ti/common/board_detect.c @@ -86,7 +86,6 @@ __weak void gpi2c_init(void) static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, u32 header, u32 size, uint8_t *ep) { - u32 hdr_read = 0xdeadbeef; int rc; #if CONFIG_IS_ENABLED(DM_I2C) @@ -113,10 +112,10 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, * We must allow for fall through to check the data if 2 byte * addressing works */ - (void)dm_i2c_read(dev, 0, (uint8_t *)&hdr_read, 4); + (void)dm_i2c_read(dev, 0, ep, size); /* Corrupted data??? */ - if (hdr_read != header) { + if (*((u32 *)ep) != header) { /* * read the eeprom header using i2c again, but use only a * 2 byte address (some newer boards need this..) @@ -125,16 +124,12 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, if (rc) return rc; - rc = dm_i2c_read(dev, 0, (uint8_t *)&hdr_read, 4); + rc = dm_i2c_read(dev, 0, ep, size); if (rc) return rc; } - if (hdr_read != header) + if (*((u32 *)ep) != header) return -1; - - rc = dm_i2c_read(dev, 0, ep, size); - if (rc) - return rc; #else u32 byte; @@ -154,26 +149,21 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, * We must allow for fall through to check the data if 2 byte * addressing works */ - (void)i2c_read(dev_addr, 0x0, byte, (uint8_t *)&hdr_read, 4); + (void)i2c_read(dev_addr, 0x0, byte, ep, size); /* Corrupted data??? */ - if (hdr_read != header) { + if (*((u32 *)ep) != header) { /* * read the eeprom header using i2c again, but use only a * 2 byte address (some newer boards need this..) */ byte = 2; - rc = i2c_read(dev_addr, 0x0, byte, (uint8_t *)&hdr_read, - 4); + rc = i2c_read(dev_addr, 0x0, byte, ep, size); if (rc) return rc; } - if (hdr_read != header) + if (*((u32 *)ep) != header) return -1; - - rc = i2c_read(dev_addr, 0x0, byte, ep, size); - if (rc) - return rc; #endif return 0; }
There are three different kinds of EEPROM possibly present on boards. 1. 1byte address. For those we should avoid 2byte address in order not to rewrite the data. Second byte of the address can potentially be interpreted as the data to write. 2. 2byte address with defined behaviour. When we try to use 1byte address they just return "FF FF FF FF ... FF" 3. 2byte address with undefined behaviour (for instance, 24LC32AI). When we try to use 1byte address, then their internal read pointer is changed to some value. Subsequential reads may be broken. To gracefully handle both case #1 and case #3 we read all required data from EEPROM at once (about 80 bytes). So either all the data is valid or we fallback to 2byte address. Cc: Nishanth Menon <nm@ti.com> Fixes: a58147c2dbbf ("board: ti: common: board_detect: Do 1byte address checks first.") Reference: https://lore.kernel.org/all/CAJs94Ebdd4foOjhGFu9Bop0v=B1US9neDLxfhgcY23ukgLzFOQ@mail.gmail.com/ Signed-off-by: Matwey V. Kornilov <matwey.kornilov@gmail.com> --- board/ti/common/board_detect.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)