[BISECTED] BeagleBone Black doesn't boot after a58147c2dbbf

[Nishanth Menon]

On 11:28-20220818, Matwey V. Kornilov wrote:
> I've played a little and now I believe that the issue is that EEPROM read addr
> pointer is somehow corrupted due to 1-byte address write. The EEPROM is
> definitely have two-byte read address accoring the datasheet.
> I've failed to unravel exact rule what is happening when only one address byte
> is set, but was able to read random places of EEPROM.
> 
> 
> However, the following diff makes the board bootable.
> 
> 
> diff --git a/board/ti/common/board_detect.c b/board/ti/common/board_detect.c
> index ed34991377..26edddccc6 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);
> +       rc = dm_i2c_read(dev, 0, ep, size);
> 
>         /* Corrupted data??? */
> -       if (hdr_read != header) {
> +       if (rc || (*((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,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);
> +               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;

This does work. I tested a few variations of boards to check this
concept out.. but anyways.. on beaglebone black (element 14 boards):

NOTE: This will improve detection times for 1 byte eeprom based boot,
since there is no retry..

However for boards with 2 byte addressing eeproms:

master branch: https://pasteboard.co/n3P8yhSq6pem.png
Time from first attempt to read eeprom to actual trigger of final eeprom
read attempt: ~4ms

With this patch: https://pasteboard.co/IVQzHwMuhc4p.png
Time from first attempt to read eeprom to actual trigger of final eeprom
read attempt: ~18ms

IMHO, 14ms penalty is'nt a bad deal for dealing with variations of
eeproms we are seeing in the wild.

You can find the data (analog+digital capture) here:
https://github.com/nmenon/data-captures/tree/main/i2c-eeprom-1byte-captures
Tool used to capture (and view): https://www.saleae.com/downloads/

Tom, Robert, folks: what do you folks think?


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