Re: [BISECTED] BeagleBone Black doesn't boot after a58147c2dbbf

[Robert Nelson <robertcnelson@gmail.com>]

On Fri, Aug 19, 2022 at 4:30 AM Nishanth Menon <nm@ti.com> wrote:
>
> 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?


I'm okay with the delay. if we only had 'one' production run it would
be a different story.. But with 10 years of manufacturing, parts will
EOL and vary. Let's go with the safe slower route..

Regards,

-- 
Robert Nelson
https://rcn-ee.com/