[PATCH v2] board: ti: common: board_detect: Fix EEPROM read quirk for 2-byte

[PATCH v2] board: ti: common: board_detect: Fix EEPROM read quirk for 2-byte

[Neha Malcom Francis]

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)
Fixes: bf6376642fe8 (board: ti: common: board_detect: Fix EEPROM read quirk)
---
 board/ti/common/board_detect.c | 46 +++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 12 deletions(-)

Changes since v1:
	- modified algorithm for boards that do not have CONFIG_DM_I2C
	  enabled as well

diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c
index c37629fe8a..9b32076ab9 100644
--- a/board/ti/common/board_detect.c
+++ b/board/ti/common/board_detect.c
@@ -87,6 +87,8 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr,
 					    u32 header, u32 size, uint8_t *ep)
 {
 	int rc;
+	uint8_t offset_test;
+	bool one_byte_addressing = true;
 
 #if CONFIG_IS_ENABLED(DM_I2C)
 	struct udevice *dev;
@@ -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..)
@@ -151,8 +168,23 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr,
 	 */
 	(void)i2c_read(dev_addr, 0x0, byte, 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 = i2c_read(dev, 0x1, byte, &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 +476,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

Re: [PATCH v2] board: ti: common: board_detect: Fix EEPROM read quirk for 2-byte

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]

On Tue, Dec 13, 2022 at 11:57:34AM +0530, Neha Malcom Francis wrote:

> 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)
> Fixes: bf6376642fe8 (board: ti: common: board_detect: Fix EEPROM read quirk)

There's not a great choice here between take this now for v2023.01 (when
lots of people are going to be busy and not catch a new possible corner
case) or put this in -next and have it be in v2023.04 (as places might
pick v2023.01 to base on for a while). But, I'm leaning towards putting
this in -next since it should be easy to cherry-pick this as needed.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2] board: ti: common: board_detect: Fix EEPROM read quirk for 2-byte

[Matwey V. Kornilov]

вт, 13 дек. 2022 г. в 09:27, 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)
> Fixes: 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 | 46 +++++++++++++++++++++++++---------
>  1 file changed, 34 insertions(+), 12 deletions(-)
>
> Changes since v1:
>         - modified algorithm for boards that do not have CONFIG_DM_I2C
>           enabled as well
>
> diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c
> index c37629fe8a..9b32076ab9 100644
> --- a/board/ti/common/board_detect.c
> +++ b/board/ti/common/board_detect.c
> @@ -87,6 +87,8 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr,
>                                             u32 header, u32 size, uint8_t *ep)
>  {
>         int rc;
> +       uint8_t offset_test;
> +       bool one_byte_addressing = true;
>
>  #if CONFIG_IS_ENABLED(DM_I2C)
>         struct udevice *dev;
> @@ -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..)
> @@ -151,8 +168,23 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr,
>          */
>         (void)i2c_read(dev_addr, 0x0, byte, 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 = i2c_read(dev, 0x1, byte, &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 +476,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
>


-- 
With best regards,
Matwey V. Kornilov

Re: [PATCH v2] board: ti: common: board_detect: Fix EEPROM read quirk for 2-byte

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]

On Tue, Dec 13, 2022 at 11:57:34AM +0530, Neha Malcom Francis wrote:

> 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)
> Fixes: bf6376642fe8 (board: ti: common: board_detect: Fix EEPROM read quirk)
> Tested-By: Matwey V. Kornilov <matwey.kornilov@gmail.com>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]