[PATCH 0/3] board: ti: common: Fixups and optimizations for eeprom handling
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(-)
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;
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?
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.
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
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!
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) {
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
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!
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)
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
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!
participants (2)
-
Nishanth Menon -
Tom Rini