| Message ID | 20221129065010.123065-1-n-francis@ti.com |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [RFC] board: ti: common: board_detect: Fix EEPROM read quirk for 2-byte | expand |
вт, 29 нояб. 2022 г. в 09:50, Neha Malcom Francis <n-francis@ti.com>: > > EEPROM detection logic in ti_i2c_eeprom_get() involves figuring out > whether addressing is 1-byte or 2-byte. There are currently different > behaviours seen across boards as documented in commit bf6376642fe8 > ("board: ti: common: board_detect: Fix EEPROM read quirk"). Adding to > the list, we see that there are 2-byte EEPROMs that read properly > with 1-byte addressing with no offset. > > For ti_i2c_eeprom_am6_get where eeprom parse operation is dynamic, the > earlier commit d2ab2a2bafd5 ("board: ti: common: board_detect: Fix > EEPROM read quirk for AM6 style data") tried to resolve this by running > ti_i2c_eeprom_get() twice. However this commit along with its former > commit fails on J7 platforms where EEPROM successfully return back the > header on 1-byte addressing and continues to do so until an offset is > introduced. So the second read incorrectly determines the EEPROM as > 1-byte addressing. > > A more generic solution is introduced here to solve > this issue: 1-byte read without offset and 1-byte read with offset. If > both passes, it follows 1-byte addressing else we proceed with 2-byte > addressing check. > > Tested on J721E, J7200, DRA7xx, AM64x I'll try to test this on the AM335x boards I have as soon as possible. > > Signed-off-by: Neha Malcom Francis <n-francis@ti.com> > Fixes: d2ab2a2bafd5 (board: ti: common: board_detect: Fix EEPROM read > quirk for AM6 style data) and bf6376642fe8 (board: ti: common: board_detect: > Fix EEPROM read quirk) > --- > board/ti/common/board_detect.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c > index c37629fe8a..b9f2ebf2a0 100644 > --- a/board/ti/common/board_detect.c > +++ b/board/ti/common/board_detect.c > @@ -91,6 +91,8 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, > #if CONFIG_IS_ENABLED(DM_I2C) Should #else branch also be modified according to the new algo? > struct udevice *dev; > struct udevice *bus; > + uint8_t offset_test; > + bool one_byte_addressing = true; > > rc = uclass_get_device_by_seq(UCLASS_I2C, bus_addr, &bus); > if (rc) > @@ -114,8 +116,23 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, > */ > (void)dm_i2c_read(dev, 0, ep, size); > > + if (*((u32 *)ep) != header) > + one_byte_addressing = false; > + > + /* > + * Handle case of bad 2 byte eeproms that responds to 1 byte addressing > + * but gets stuck in const addressing when read requests are performed > + * on offsets. We perform an offset test to make sure it is not a 2 byte > + * eeprom that works with 1 byte addressing but just without an offset > + */ > + > + rc = dm_i2c_read(dev, 0x1, &offset_test, sizeof(offset_test)); > + > + if (*((u32 *)ep) != (header & 0xFF)) > + one_byte_addressing = false; > + > /* Corrupted data??? */ > - if (*((u32 *)ep) != header) { > + if (!one_byte_addressing) { > /* > * read the eeprom header using i2c again, but use only a > * 2 byte address (some newer boards need this..) > @@ -444,16 +461,6 @@ int __maybe_unused ti_i2c_eeprom_am6_get(int bus_addr, int dev_addr, > if (rc) > return rc; > > - /* > - * Handle case of bad 2 byte eeproms that responds to 1 byte addressing > - * but gets stuck in const addressing when read requests are performed > - * on offsets. We re-read the board ID to ensure we have sane data back > - */ > - rc = ti_i2c_eeprom_get(bus_addr, dev_addr, TI_EEPROM_HEADER_MAGIC, > - sizeof(board_id), (uint8_t *)&board_id); > - if (rc) > - return rc; > - > if (board_id.header.id != TI_AM6_EEPROM_RECORD_BOARD_ID) { > pr_err("%s: Invalid board ID record!\n", __func__); > return -EINVAL; > -- > 2.34.1 >
On Tue, Nov 29, 2022 at 03:10:12PM +0300, Matwey V. Kornilov wrote: > вт, 29 нояб. 2022 г. в 09:50, Neha Malcom Francis <n-francis@ti.com>: > > > > EEPROM detection logic in ti_i2c_eeprom_get() involves figuring out > > whether addressing is 1-byte or 2-byte. There are currently different > > behaviours seen across boards as documented in commit bf6376642fe8 > > ("board: ti: common: board_detect: Fix EEPROM read quirk"). Adding to > > the list, we see that there are 2-byte EEPROMs that read properly > > with 1-byte addressing with no offset. > > > > For ti_i2c_eeprom_am6_get where eeprom parse operation is dynamic, the > > earlier commit d2ab2a2bafd5 ("board: ti: common: board_detect: Fix > > EEPROM read quirk for AM6 style data") tried to resolve this by running > > ti_i2c_eeprom_get() twice. However this commit along with its former > > commit fails on J7 platforms where EEPROM successfully return back the > > header on 1-byte addressing and continues to do so until an offset is > > introduced. So the second read incorrectly determines the EEPROM as > > 1-byte addressing. > > > > A more generic solution is introduced here to solve > > this issue: 1-byte read without offset and 1-byte read with offset. If > > both passes, it follows 1-byte addressing else we proceed with 2-byte > > addressing check. > > > > Tested on J721E, J7200, DRA7xx, AM64x > > I'll try to test this on the AM335x boards I have as soon as possible. I wonder if we should re-factor this code a bit and not have a single ti_i2c_eeprom_get but instead build for whichever sets of quirks a given family of boards has with their EEPROMs. I really worry that we're going to find that this change here breaks yet another different EEPROM than before.
Hi Matwey On 29/11/22 17:40, Matwey V. Kornilov wrote: > вт, 29 нояб. 2022 г. в 09:50, Neha Malcom Francis <n-francis@ti.com>: >> >> EEPROM detection logic in ti_i2c_eeprom_get() involves figuring out >> whether addressing is 1-byte or 2-byte. There are currently different >> behaviours seen across boards as documented in commit bf6376642fe8 >> ("board: ti: common: board_detect: Fix EEPROM read quirk"). Adding to >> the list, we see that there are 2-byte EEPROMs that read properly >> with 1-byte addressing with no offset. >> >> For ti_i2c_eeprom_am6_get where eeprom parse operation is dynamic, the >> earlier commit d2ab2a2bafd5 ("board: ti: common: board_detect: Fix >> EEPROM read quirk for AM6 style data") tried to resolve this by running >> ti_i2c_eeprom_get() twice. However this commit along with its former >> commit fails on J7 platforms where EEPROM successfully return back the >> header on 1-byte addressing and continues to do so until an offset is >> introduced. So the second read incorrectly determines the EEPROM as >> 1-byte addressing. >> >> A more generic solution is introduced here to solve >> this issue: 1-byte read without offset and 1-byte read with offset. If >> both passes, it follows 1-byte addressing else we proceed with 2-byte >> addressing check. >> >> Tested on J721E, J7200, DRA7xx, AM64x > > I'll try to test this on the AM335x boards I have as soon as possible. Thanks! > >> >> Signed-off-by: Neha Malcom Francis <n-francis@ti.com> >> Fixes: d2ab2a2bafd5 (board: ti: common: board_detect: Fix EEPROM read >> quirk for AM6 style data) and bf6376642fe8 (board: ti: common: board_detect: >> Fix EEPROM read quirk) >> --- >> board/ti/common/board_detect.c | 29 ++++++++++++++++++----------- >> 1 file changed, 18 insertions(+), 11 deletions(-) >> >> diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c >> index c37629fe8a..b9f2ebf2a0 100644 >> --- a/board/ti/common/board_detect.c >> +++ b/board/ti/common/board_detect.c >> @@ -91,6 +91,8 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, >> #if CONFIG_IS_ENABLED(DM_I2C) > > Should #else branch also be modified according to the new algo? Yeah you're right; let me try getting into that as well. > >> struct udevice *dev; >> struct udevice *bus; >> + uint8_t offset_test; >> + bool one_byte_addressing = true; >> >> rc = uclass_get_device_by_seq(UCLASS_I2C, bus_addr, &bus); >> if (rc) >> @@ -114,8 +116,23 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, >> */ >> (void)dm_i2c_read(dev, 0, ep, size); >> >> + if (*((u32 *)ep) != header) >> + one_byte_addressing = false; >> + >> + /* >> + * Handle case of bad 2 byte eeproms that responds to 1 byte addressing >> + * but gets stuck in const addressing when read requests are performed >> + * on offsets. We perform an offset test to make sure it is not a 2 byte >> + * eeprom that works with 1 byte addressing but just without an offset >> + */ >> + >> + rc = dm_i2c_read(dev, 0x1, &offset_test, sizeof(offset_test)); >> + >> + if (*((u32 *)ep) != (header & 0xFF)) >> + one_byte_addressing = false; >> + >> /* Corrupted data??? */ >> - if (*((u32 *)ep) != header) { >> + if (!one_byte_addressing) { >> /* >> * read the eeprom header using i2c again, but use only a >> * 2 byte address (some newer boards need this..) >> @@ -444,16 +461,6 @@ int __maybe_unused ti_i2c_eeprom_am6_get(int bus_addr, int dev_addr, >> if (rc) >> return rc; >> >> - /* >> - * Handle case of bad 2 byte eeproms that responds to 1 byte addressing >> - * but gets stuck in const addressing when read requests are performed >> - * on offsets. We re-read the board ID to ensure we have sane data back >> - */ >> - rc = ti_i2c_eeprom_get(bus_addr, dev_addr, TI_EEPROM_HEADER_MAGIC, >> - sizeof(board_id), (uint8_t *)&board_id); >> - if (rc) >> - return rc; >> - >> if (board_id.header.id != TI_AM6_EEPROM_RECORD_BOARD_ID) { >> pr_err("%s: Invalid board ID record!\n", __func__); >> return -EINVAL; >> -- >> 2.34.1 >> > >
Hi Tom On 29/11/22 19:01, Tom Rini wrote: > On Tue, Nov 29, 2022 at 03:10:12PM +0300, Matwey V. Kornilov wrote: >> вт, 29 нояб. 2022 г. в 09:50, Neha Malcom Francis <n-francis@ti.com>: >>> >>> EEPROM detection logic in ti_i2c_eeprom_get() involves figuring out >>> whether addressing is 1-byte or 2-byte. There are currently different >>> behaviours seen across boards as documented in commit bf6376642fe8 >>> ("board: ti: common: board_detect: Fix EEPROM read quirk"). Adding to >>> the list, we see that there are 2-byte EEPROMs that read properly >>> with 1-byte addressing with no offset. >>> >>> For ti_i2c_eeprom_am6_get where eeprom parse operation is dynamic, the >>> earlier commit d2ab2a2bafd5 ("board: ti: common: board_detect: Fix >>> EEPROM read quirk for AM6 style data") tried to resolve this by running >>> ti_i2c_eeprom_get() twice. However this commit along with its former >>> commit fails on J7 platforms where EEPROM successfully return back the >>> header on 1-byte addressing and continues to do so until an offset is >>> introduced. So the second read incorrectly determines the EEPROM as >>> 1-byte addressing. >>> >>> A more generic solution is introduced here to solve >>> this issue: 1-byte read without offset and 1-byte read with offset. If >>> both passes, it follows 1-byte addressing else we proceed with 2-byte >>> addressing check. >>> >>> Tested on J721E, J7200, DRA7xx, AM64x >> >> I'll try to test this on the AM335x boards I have as soon as possible. > > I wonder if we should re-factor this code a bit and not have a single > ti_i2c_eeprom_get but instead build for whichever sets of quirks a given > family of boards has with their EEPROMs. I really worry that we're going > to find that this change here breaks yet another different EEPROM than > before. > Yes that does make sense... considering a new behavior of EEPROM keeps showing up. I will try refactoring the logic that way.
> >>> Tested on J721E, J7200, DRA7xx, AM64x > >> > >> I'll try to test this on the AM335x boards I have as soon as possible. > > > > I wonder if we should re-factor this code a bit and not have a single > > ti_i2c_eeprom_get but instead build for whichever sets of quirks a given > > family of boards has with their EEPROMs. I really worry that we're going > > to find that this change here breaks yet another different EEPROM than > > before. > > > > Yes that does make sense... considering a new behavior of EEPROM keeps > showing up. I will try refactoring the logic that way. Due to part shorages, sadly the BeagleBone AI64 (J721E) has both 1byte and 2byte eeproms in user hands today.. While I think most other previous designs have stuck with one type of eeprom throughout their production life. So just one big outlier that I personally know of.. Regards,
ср, 30 нояб. 2022 г. в 18:36, Robert Nelson <robertcnelson@gmail.com>: > > > >>> Tested on J721E, J7200, DRA7xx, AM64x > > >> > > >> I'll try to test this on the AM335x boards I have as soon as possible. > > > > > > I wonder if we should re-factor this code a bit and not have a single > > > ti_i2c_eeprom_get but instead build for whichever sets of quirks a given > > > family of boards has with their EEPROMs. I really worry that we're going > > > to find that this change here breaks yet another different EEPROM than > > > before. > > > > > > > Yes that does make sense... considering a new behavior of EEPROM keeps > > showing up. I will try refactoring the logic that way. > > Due to part shorages, sadly the BeagleBone AI64 (J721E) has both 1byte > and 2byte eeproms in user hands today.. > > While I think most other previous designs have stuck with one type of > eeprom throughout their production life. So just one big outlier that > I personally know of.. As was mentioned during the work on bf6376642fe8 ("board: ti: common: board_detect: Fix EEPROM read quirk"), BeagleBone Black produced by different companies has different kinds of EEPROMs. > > Regards, > > -- > Robert Nelson > https://rcn-ee.com/
On Wed, Nov 30, 2022 at 06:38:43PM +0300, Matwey V. Kornilov wrote: > ср, 30 нояб. 2022 г. в 18:36, Robert Nelson <robertcnelson@gmail.com>: > > > > > >>> Tested on J721E, J7200, DRA7xx, AM64x > > > >> > > > >> I'll try to test this on the AM335x boards I have as soon as possible. > > > > > > > > I wonder if we should re-factor this code a bit and not have a single > > > > ti_i2c_eeprom_get but instead build for whichever sets of quirks a given > > > > family of boards has with their EEPROMs. I really worry that we're going > > > > to find that this change here breaks yet another different EEPROM than > > > > before. > > > > > > > > > > Yes that does make sense... considering a new behavior of EEPROM keeps > > > showing up. I will try refactoring the logic that way. > > > > Due to part shorages, sadly the BeagleBone AI64 (J721E) has both 1byte > > and 2byte eeproms in user hands today.. > > > > While I think most other previous designs have stuck with one type of > > eeprom throughout their production life. So just one big outlier that > > I personally know of.. > > As was mentioned during the work on bf6376642fe8 ("board: ti: common: > board_detect: Fix EEPROM read quirk"), BeagleBone Black produced by > different companies has different kinds of EEPROMs. I guess at this point before trying to refactor it's worth confirming that we do have the case of boards that aren't still having some part of the family produced, and so we can refactor the code in to "safe for legacy boards" and "can't be sure if it's 1byte or 2byte". And then if it's worth doing the split anyhow, sigh.
вт, 29 нояб. 2022 г. в 09:50, Neha Malcom Francis <n-francis@ti.com>: > > EEPROM detection logic in ti_i2c_eeprom_get() involves figuring out > whether addressing is 1-byte or 2-byte. There are currently different > behaviours seen across boards as documented in commit bf6376642fe8 > ("board: ti: common: board_detect: Fix EEPROM read quirk"). Adding to > the list, we see that there are 2-byte EEPROMs that read properly > with 1-byte addressing with no offset. > > For ti_i2c_eeprom_am6_get where eeprom parse operation is dynamic, the > earlier commit d2ab2a2bafd5 ("board: ti: common: board_detect: Fix > EEPROM read quirk for AM6 style data") tried to resolve this by running > ti_i2c_eeprom_get() twice. However this commit along with its former > commit fails on J7 platforms where EEPROM successfully return back the > header on 1-byte addressing and continues to do so until an offset is > introduced. So the second read incorrectly determines the EEPROM as > 1-byte addressing. > > A more generic solution is introduced here to solve > this issue: 1-byte read without offset and 1-byte read with offset. If > both passes, it follows 1-byte addressing else we proceed with 2-byte > addressing check. > > Tested on J721E, J7200, DRA7xx, AM64x > > Signed-off-by: Neha Malcom Francis <n-francis@ti.com> > Fixes: d2ab2a2bafd5 (board: ti: common: board_detect: Fix EEPROM read > quirk for AM6 style data) and bf6376642fe8 (board: ti: common: board_detect: > Fix EEPROM read quirk) Tested-By: Matwey V. Kornilov <matwey.kornilov@gmail.com> > --- > board/ti/common/board_detect.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c > index c37629fe8a..b9f2ebf2a0 100644 > --- a/board/ti/common/board_detect.c > +++ b/board/ti/common/board_detect.c > @@ -91,6 +91,8 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, > #if CONFIG_IS_ENABLED(DM_I2C) > struct udevice *dev; > struct udevice *bus; > + uint8_t offset_test; > + bool one_byte_addressing = true; > > rc = uclass_get_device_by_seq(UCLASS_I2C, bus_addr, &bus); > if (rc) > @@ -114,8 +116,23 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, > */ > (void)dm_i2c_read(dev, 0, ep, size); > > + if (*((u32 *)ep) != header) > + one_byte_addressing = false; > + > + /* > + * Handle case of bad 2 byte eeproms that responds to 1 byte addressing > + * but gets stuck in const addressing when read requests are performed > + * on offsets. We perform an offset test to make sure it is not a 2 byte > + * eeprom that works with 1 byte addressing but just without an offset > + */ > + > + rc = dm_i2c_read(dev, 0x1, &offset_test, sizeof(offset_test)); > + > + if (*((u32 *)ep) != (header & 0xFF)) > + one_byte_addressing = false; > + > /* Corrupted data??? */ > - if (*((u32 *)ep) != header) { > + if (!one_byte_addressing) { > /* > * read the eeprom header using i2c again, but use only a > * 2 byte address (some newer boards need this..) > @@ -444,16 +461,6 @@ int __maybe_unused ti_i2c_eeprom_am6_get(int bus_addr, int dev_addr, > if (rc) > return rc; > > - /* > - * Handle case of bad 2 byte eeproms that responds to 1 byte addressing > - * but gets stuck in const addressing when read requests are performed > - * on offsets. We re-read the board ID to ensure we have sane data back > - */ > - rc = ti_i2c_eeprom_get(bus_addr, dev_addr, TI_EEPROM_HEADER_MAGIC, > - sizeof(board_id), (uint8_t *)&board_id); > - if (rc) > - return rc; > - > if (board_id.header.id != TI_AM6_EEPROM_RECORD_BOARD_ID) { > pr_err("%s: Invalid board ID record!\n", __func__); > return -EINVAL; > -- > 2.34.1 >
diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c index c37629fe8a..b9f2ebf2a0 100644 --- a/board/ti/common/board_detect.c +++ b/board/ti/common/board_detect.c @@ -91,6 +91,8 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, #if CONFIG_IS_ENABLED(DM_I2C) struct udevice *dev; struct udevice *bus; + uint8_t offset_test; + bool one_byte_addressing = true; rc = uclass_get_device_by_seq(UCLASS_I2C, bus_addr, &bus); if (rc) @@ -114,8 +116,23 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr, */ (void)dm_i2c_read(dev, 0, ep, size); + if (*((u32 *)ep) != header) + one_byte_addressing = false; + + /* + * Handle case of bad 2 byte eeproms that responds to 1 byte addressing + * but gets stuck in const addressing when read requests are performed + * on offsets. We perform an offset test to make sure it is not a 2 byte + * eeprom that works with 1 byte addressing but just without an offset + */ + + rc = dm_i2c_read(dev, 0x1, &offset_test, sizeof(offset_test)); + + if (*((u32 *)ep) != (header & 0xFF)) + one_byte_addressing = false; + /* Corrupted data??? */ - if (*((u32 *)ep) != header) { + if (!one_byte_addressing) { /* * read the eeprom header using i2c again, but use only a * 2 byte address (some newer boards need this..) @@ -444,16 +461,6 @@ int __maybe_unused ti_i2c_eeprom_am6_get(int bus_addr, int dev_addr, if (rc) return rc; - /* - * Handle case of bad 2 byte eeproms that responds to 1 byte addressing - * but gets stuck in const addressing when read requests are performed - * on offsets. We re-read the board ID to ensure we have sane data back - */ - rc = ti_i2c_eeprom_get(bus_addr, dev_addr, TI_EEPROM_HEADER_MAGIC, - sizeof(board_id), (uint8_t *)&board_id); - if (rc) - return rc; - if (board_id.header.id != TI_AM6_EEPROM_RECORD_BOARD_ID) { pr_err("%s: Invalid board ID record!\n", __func__); return -EINVAL;
EEPROM detection logic in ti_i2c_eeprom_get() involves figuring out whether addressing is 1-byte or 2-byte. There are currently different behaviours seen across boards as documented in commit bf6376642fe8 ("board: ti: common: board_detect: Fix EEPROM read quirk"). Adding to the list, we see that there are 2-byte EEPROMs that read properly with 1-byte addressing with no offset. For ti_i2c_eeprom_am6_get where eeprom parse operation is dynamic, the earlier commit d2ab2a2bafd5 ("board: ti: common: board_detect: Fix EEPROM read quirk for AM6 style data") tried to resolve this by running ti_i2c_eeprom_get() twice. However this commit along with its former commit fails on J7 platforms where EEPROM successfully return back the header on 1-byte addressing and continues to do so until an offset is introduced. So the second read incorrectly determines the EEPROM as 1-byte addressing. A more generic solution is introduced here to solve this issue: 1-byte read without offset and 1-byte read with offset. If both passes, it follows 1-byte addressing else we proceed with 2-byte addressing check. Tested on J721E, J7200, DRA7xx, AM64x Signed-off-by: Neha Malcom Francis <n-francis@ti.com> Fixes: d2ab2a2bafd5 (board: ti: common: board_detect: Fix EEPROM read quirk for AM6 style data) and bf6376642fe8 (board: ti: common: board_detect: Fix EEPROM read quirk) --- board/ti/common/board_detect.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)