[PATCH 0/3] board: ti: common: Fixups and optimizations for eeprom handling

[PATCH 0/3] board: ti: common: Fixups and optimizations for eeprom handling

[Nishanth Menon]

Hi,

These are few of the fixes picked up from U-boot tree done for
BeagleBone-AI64 and others.

Additional testing is much appreciated - my test environment is
currently limited :(

Nishanth Menon (3):
  board: ti: common: Optimize boot when detecting consecutive bad
    records
  board: ti: common: Handle the legacy eeprom address width properly
  board: ti: common: board_detect: Do 1byte address checks first.

 board/ti/common/board_detect.c | 39 ++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 14 deletions(-)

-- 
2.31.1

[PATCH 1/3] board: ti: common: Optimize boot when detecting consecutive bad records

[Nishanth Menon]

The eeprom data area is much bigger than the data we intend to store,
however, with bad programming, we might end up reading bad records over
and over till we run out of eeprom space. instead just exit when 10
consecutive records are read.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/ti/common/board_detect.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c
index de92eb0981f9..381cddc00ad1 100644
--- a/board/ti/common/board_detect.c
+++ b/board/ti/common/board_detect.c
@@ -434,6 +434,7 @@ int __maybe_unused ti_i2c_eeprom_am6_get(int bus_addr, int dev_addr,
 	struct ti_am6_eeprom_record_board_id board_id;
 	struct ti_am6_eeprom_record record;
 	int rc;
+	int consecutive_bad_records = 0;
 
 	/* Initialize with a known bad marker for i2c fails.. */
 	memset(ep, 0, sizeof(*ep));
@@ -470,7 +471,7 @@ int __maybe_unused ti_i2c_eeprom_am6_get(int bus_addr, int dev_addr,
 	 */
 	eeprom_addr = sizeof(board_id);
 
-	while (true) {
+	while (consecutive_bad_records < 10) {
 		rc = dm_i2c_read(dev, eeprom_addr, (uint8_t *)&record.header,
 				 sizeof(record.header));
 		if (rc)
@@ -506,6 +507,7 @@ int __maybe_unused ti_i2c_eeprom_am6_get(int bus_addr, int dev_addr,
 				pr_err("%s: EEPROM parsing error!\n", __func__);
 				return rc;
 			}
+			consecutive_bad_records = 0;
 		} else {
 			/*
 			 * We may get here in case of larger records which
@@ -513,6 +515,7 @@ int __maybe_unused ti_i2c_eeprom_am6_get(int bus_addr, int dev_addr,
 			 */
 			pr_err("%s: Ignoring record id %u\n", __func__,
 			       record.header.id);
+			consecutive_bad_records++;
 		}
 
 		eeprom_addr += record.header.len;
-- 
2.31.1

[PATCH 2/3] board: ti: common: Handle the legacy eeprom address width properly

[Nishanth Menon]

Due to supply chain issues, we are starting to see a mixture of eeprom
usage including the smaller 7-bit addressing eeproms such as 24c04
used for eeproms.

These eeproms don't respond well to 2 byte addressing and fail the
read operation. We do have a check to ensure that we are reading the
alternate addressing size, however the valid failure prevents us
from checking at 1 byte anymore.

Rectify the same by falling through and depend on header data comparison
to ensure that we have valid data.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/ti/common/board_detect.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c
index 381cddc00ad1..0806dea11ed5 100644
--- a/board/ti/common/board_detect.c
+++ b/board/ti/common/board_detect.c
@@ -86,7 +86,7 @@ __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;
+	u32 hdr_read = 0xdeadbeef;
 	int rc;
 
 #if CONFIG_IS_ENABLED(DM_I2C)
@@ -107,9 +107,13 @@ 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);
-	if (rc)
-		return rc;
+	/*
+	 * Skip checking result here since this could be a valid i2c read fail
+	 * on some boards that use 1 byte addressing.
+	 * We must allow for fall through to check the data if 1 byte
+	 * addressing works
+	 */
+	(void)dm_i2c_read(dev, 0, (uint8_t *)&hdr_read, 4);
 
 	/* Corrupted data??? */
 	if (hdr_read != header) {
@@ -144,9 +148,13 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr,
 	 */
 	byte = 2;
 
-	rc = i2c_read(dev_addr, 0x0, byte, (uint8_t *)&hdr_read, 4);
-	if (rc)
-		return rc;
+	/*
+	 * Skip checking result here since this could be a valid i2c read fail
+	 * on some boards that use 1 byte addressing.
+	 * We must allow for fall through to check the data if 1 byte
+	 * addressing works
+	 */
+	(void)i2c_read(dev_addr, 0x0, byte, (uint8_t *)&hdr_read, 4);
 
 	/* Corrupted data??? */
 	if (hdr_read != header) {
-- 
2.31.1

[PATCH 3/3] board: ti: common: board_detect: Do 1byte address checks first.

[Nishanth Menon]

Do 1 byte address checks first prior to doing 2 byte address checks.
When performing 2 byte addressing on 1 byte addressing eeprom, the
second byte is taken in as a write operation and ends up erasing the
eeprom region we want to preserve.

While we could have theoretically handled this by ensuring the write
protect of the eeproms are properly managed, this is not true in case
where board are updated with 1 byte eeproms to handle supply status.

Flipping the checks by checking for 1 byte addressing prior to 2 byte
addressing check prevents this problem at the minor cost of additional
overhead for boards with 2 byte addressing eeproms.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/ti/common/board_detect.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c
index 0806dea11ed5..ed34991377ee 100644
--- a/board/ti/common/board_detect.c
+++ b/board/ti/common/board_detect.c
@@ -103,14 +103,14 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr,
 	/*
 	 * Read the header first then only read the other contents.
 	 */
-	rc = i2c_set_chip_offset_len(dev, 2);
+	rc = i2c_set_chip_offset_len(dev, 1);
 	if (rc)
 		return rc;
 
 	/*
 	 * Skip checking result here since this could be a valid i2c read fail
-	 * on some boards that use 1 byte addressing.
-	 * We must allow for fall through to check the data if 1 byte
+	 * on some boards that use 2 byte addressing.
+	 * 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);
@@ -119,9 +119,9 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr,
 	if (hdr_read != header) {
 		/*
 		 * read the eeprom header using i2c again, but use only a
-		 * 1 byte address (some legacy boards need this..)
+		 * 2 byte address (some newer boards need this..)
 		 */
-		rc = i2c_set_chip_offset_len(dev, 1);
+		rc = i2c_set_chip_offset_len(dev, 2);
 		if (rc)
 			return rc;
 
@@ -146,12 +146,12 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr,
 	/*
 	 * Read the header first then only read the other contents.
 	 */
-	byte = 2;
+	byte = 1;
 
 	/*
 	 * Skip checking result here since this could be a valid i2c read fail
-	 * on some boards that use 1 byte addressing.
-	 * We must allow for fall through to check the data if 1 byte
+	 * on some boards that use 2 byte addressing.
+	 * 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);
@@ -160,9 +160,9 @@ static int __maybe_unused ti_i2c_eeprom_get(int bus_addr, int dev_addr,
 	if (hdr_read != header) {
 		/*
 		 * read the eeprom header using i2c again, but use only a
-		 * 1 byte address (some legacy boards need this..)
+		 * 2 byte address (some newer boards need this..)
 		 */
-		byte = 1;
+		byte = 2;
 		rc = i2c_read(dev_addr, 0x0, byte, (uint8_t *)&hdr_read,
 			      4);
 		if (rc)
-- 
2.31.1

Re: [PATCH 1/3] board: ti: common: Optimize boot when detecting consecutive bad records

[Tom Rini]

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

On Fri, Jun 17, 2022 at 01:26:10PM -0500, Nishanth Menon wrote:

> The eeprom data area is much bigger than the data we intend to store,
> however, with bad programming, we might end up reading bad records over
> and over till we run out of eeprom space. instead just exit when 10
> consecutive records are read.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>

Why not just stop at the first bad record?  Otherwise 10 seems like a
fine, small, arbitrary number.  If it's not arbitrary but number of
total records, do we already enum the total number of records or
something where we could say that we tried to read all possible records,
everyone was bad, stop?

-- 
Tom

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

Re: [PATCH 1/3] board: ti: common: Optimize boot when detecting consecutive bad records

[Nishanth Menon]

On 14:50-20220617, Tom Rini wrote:
> On Fri, Jun 17, 2022 at 01:26:10PM -0500, Nishanth Menon wrote:
> 
> > The eeprom data area is much bigger than the data we intend to store,
> > however, with bad programming, we might end up reading bad records over
> > and over till we run out of eeprom space. instead just exit when 10
> > consecutive records are read.
> > 
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> 
> Why not just stop at the first bad record?  Otherwise 10 seems like a

Because it could be just a couple of bad ones where the header.len
does'nt match up with record data. Some folks use a spreadsheet to
generate the records, some manually and some script it up - so,
attempting to get the best possible success chance while warning
invalid data to get people to fix things made sense.

> fine, small, arbitrary number.  If it's not arbitrary but number of
> total records, do we already enum the total number of records or
> something where we could say that we tried to read all possible records,
> everyone was bad, stop?

It is arbitrary small value - for all practical purposes, we have 6
types of records atm in u-boot, even looking ahead, we have'nt had
more than that I know of (I think display, camera and few other misc
types got added). The structure however, is by design flexible
with the END_LIST marker denoting the last record - and depending on
the eeprom size, you could theoretically have a large variation.

Considering this is attempting to recover from bad programming, the
chances are better when attempting a few more entries, but I dont
think going aggressive with a single record or conservative (as it is
right now) in scanning the entire eeprom is necessary. That leaves us
with some sort of practical number.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 1/3] board: ti: common: Optimize boot when detecting consecutive bad records

[Tom Rini]

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

On Fri, Jun 17, 2022 at 02:19:57PM -0500, Nishanth Menon wrote:
> On 14:50-20220617, Tom Rini wrote:
> > On Fri, Jun 17, 2022 at 01:26:10PM -0500, Nishanth Menon wrote:
> > 
> > > The eeprom data area is much bigger than the data we intend to store,
> > > however, with bad programming, we might end up reading bad records over
> > > and over till we run out of eeprom space. instead just exit when 10
> > > consecutive records are read.
> > > 
> > > Signed-off-by: Nishanth Menon <nm@ti.com>
> > 
> > Why not just stop at the first bad record?  Otherwise 10 seems like a
> 
> Because it could be just a couple of bad ones where the header.len
> does'nt match up with record data. Some folks use a spreadsheet to
> generate the records, some manually and some script it up - so,
> attempting to get the best possible success chance while warning
> invalid data to get people to fix things made sense.
> 
> > fine, small, arbitrary number.  If it's not arbitrary but number of
> > total records, do we already enum the total number of records or
> > something where we could say that we tried to read all possible records,
> > everyone was bad, stop?
> 
> It is arbitrary small value - for all practical purposes, we have 6
> types of records atm in u-boot, even looking ahead, we have'nt had
> more than that I know of (I think display, camera and few other misc
> types got added). The structure however, is by design flexible
> with the END_LIST marker denoting the last record - and depending on
> the eeprom size, you could theoretically have a large variation.
> 
> Considering this is attempting to recover from bad programming, the
> chances are better when attempting a few more entries, but I dont
> think going aggressive with a single record or conservative (as it is
> right now) in scanning the entire eeprom is necessary. That leaves us
> with some sort of practical number.

OK, thanks for explaining.

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

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

Re: [PATCH 2/3] board: ti: common: Handle the legacy eeprom address width properly

[Tom Rini]

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

On Fri, Jun 17, 2022 at 01:26:11PM -0500, Nishanth Menon wrote:

> Due to supply chain issues, we are starting to see a mixture of eeprom
> usage including the smaller 7-bit addressing eeproms such as 24c04
> used for eeproms.
> 
> These eeproms don't respond well to 2 byte addressing and fail the
> read operation. We do have a check to ensure that we are reading the
> alternate addressing size, however the valid failure prevents us
> from checking at 1 byte anymore.
> 
> Rectify the same by falling through and depend on header data comparison
> to ensure that we have valid data.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

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

Re: [PATCH 3/3] board: ti: common: board_detect: Do 1byte address checks first.

[Tom Rini]

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

On Fri, Jun 17, 2022 at 01:26:12PM -0500, Nishanth Menon wrote:

> Do 1 byte address checks first prior to doing 2 byte address checks.
> When performing 2 byte addressing on 1 byte addressing eeprom, the
> second byte is taken in as a write operation and ends up erasing the
> eeprom region we want to preserve.
> 
> While we could have theoretically handled this by ensuring the write
> protect of the eeproms are properly managed, this is not true in case
> where board are updated with 1 byte eeproms to handle supply status.
> 
> Flipping the checks by checking for 1 byte addressing prior to 2 byte
> addressing check prevents this problem at the minor cost of additional
> overhead for boards with 2 byte addressing eeproms.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

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

Re: [PATCH 1/3] board: ti: common: Optimize boot when detecting consecutive bad records

[Tom Rini]

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

On Fri, Jun 17, 2022 at 01:26:10PM -0500, Nishanth Menon wrote:

> The eeprom data area is much bigger than the data we intend to store,
> however, with bad programming, we might end up reading bad records over
> and over till we run out of eeprom space. instead just exit when 10
> consecutive records are read.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/next, thanks!

-- 
Tom

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

Re: [PATCH 2/3] board: ti: common: Handle the legacy eeprom address width properly

[Tom Rini]

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

On Fri, Jun 17, 2022 at 01:26:11PM -0500, Nishanth Menon wrote:

> Due to supply chain issues, we are starting to see a mixture of eeprom
> usage including the smaller 7-bit addressing eeproms such as 24c04
> used for eeproms.
> 
> These eeproms don't respond well to 2 byte addressing and fail the
> read operation. We do have a check to ensure that we are reading the
> alternate addressing size, however the valid failure prevents us
> from checking at 1 byte anymore.
> 
> Rectify the same by falling through and depend on header data comparison
> to ensure that we have valid data.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/next, thanks!

-- 
Tom

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

Re: [PATCH 3/3] board: ti: common: board_detect: Do 1byte address checks first.

[Tom Rini]

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

On Fri, Jun 17, 2022 at 01:26:12PM -0500, Nishanth Menon wrote:

> Do 1 byte address checks first prior to doing 2 byte address checks.
> When performing 2 byte addressing on 1 byte addressing eeprom, the
> second byte is taken in as a write operation and ends up erasing the
> eeprom region we want to preserve.
> 
> While we could have theoretically handled this by ensuring the write
> protect of the eeproms are properly managed, this is not true in case
> where board are updated with 1 byte eeproms to handle supply status.
> 
> Flipping the checks by checking for 1 byte addressing prior to 2 byte
> addressing check prevents this problem at the minor cost of additional
> overhead for boards with 2 byte addressing eeproms.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/next, thanks!

-- 
Tom

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