[edk2-devel] [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy

[Masami Hiramatsu]

Hi Leif,

2021年11月27日(土) 2:47 Leif Lindholm <leif at nuviainc.com>:
>
> On Fri, Nov 05, 2021 at 18:23:28 +0900, Masami Hiramatsu wrote:
> > If an EFI application frequently repeats SetTime and GetTime,
> > the I2C bus can be busy and failed to start. To fix this issue,
> > add waiting loop for the bus busy status. (Usually, it is
> > enough to read 3 times for checking, but for safety this
> > sets 10 for timeout.)
> >
> > This also clean up the code path a bit so that it is easy to
> > understand what should do on each combinations of BSR.BB and
> > BCR.MSS.
> >
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu at linaro.org>
> > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko at socionext.com>
> > ---
> >  .../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c      |   38 ++++++++++++++------
> >  1 file changed, 26 insertions(+), 12 deletions(-)
> >
> > diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> > index 31f6e3072f..380eba8059 100644
> > --- a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> > +++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> > @@ -16,6 +16,8 @@
> >  //
> >  #define WAIT_FOR_INTERRUPT_TIMEOUT    50000
> >
> > +#define WAIT_FOR_BUS_BUSY_TIMEOUT        10
> > +
>
> I think it would be more clear English to say that we are waiting
> _for_ the bus to be *ready* - meaning that we are waiting _while_ the
> bus is *busy*.
>
> So I suggest
> WAIT_FOR_BUS_BUSY_TIMEOUT ->
> WAIT_FOR_BUS_READY_TIMEOUT

Oops, indeed. It is waiting for the bus "ready", not "busy" ...

>
> >  /**
> >    Set the frequency for the I2C clock line.
> >
> > @@ -152,6 +154,7 @@ SynQuacerI2cMasterStart (
> >    IN  EFI_I2C_OPERATION           *Op
> >    )
> >  {
> > +  UINTN                   Timeout = WAIT_FOR_BUS_BUSY_TIMEOUT;
>
> This indentation does not match the subsequent lines.

OK, I'll fix that.

Thank you!

>
> /
>     Leif
>
> >    UINT8                       Bsr;
> >    UINT8                       Bcr;
> >
> > @@ -167,24 +170,35 @@ SynQuacerI2cMasterStart (
> >    Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
> >    Bcr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BCR);
> >
> > -  if ((Bsr & F_I2C_BSR_BB) && !(Bcr & F_I2C_BCR_MSS)) {
> > -    DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
> > -    return EFI_ALREADY_STARTED;
> > -  }
> > +  if (!(Bcr & F_I2C_BCR_MSS)) {
> >
> > -  if (Bsr & F_I2C_BSR_BB) { // Bus is busy
> > -    DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
> > -    MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
> > -  } else {
> > -    if (Bcr & F_I2C_BCR_MSS) {
> > -      DEBUG ((DEBUG_WARN,
> > -        "%a: is not in master mode\n", __FUNCTION__));
> > -      return EFI_DEVICE_ERROR;
> > +    if (Bsr & F_I2C_BSR_BB) { // Bus is busy
> > +        do {
> > +          Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
> > +        } while (Timeout-- && (Bsr & F_I2C_BSR_BB));
> > +
> > +        if (Bsr & F_I2C_BSR_BB) {
> > +          DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
> > +          return EFI_ALREADY_STARTED;
> > +        }
> >      }
> > +
> >      DEBUG ((DEBUG_INFO, "%a: Start Condition\n", __FUNCTION__));
> >      MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR,
> >                  Bcr | F_I2C_BCR_MSS | F_I2C_BCR_INTE | F_I2C_BCR_BEIE);
> > +
> > +  } else { // F_I2C_BCR_MSS is set
> > +
> > +    if (!(Bsr & F_I2C_BSR_BB)) {
> > +      DEBUG ((DEBUG_WARN,
> > +        "%a: is not in master mode\n", __FUNCTION__));
> > +      return EFI_DEVICE_ERROR;
> > +    }
> > +
> > +    DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
> > +    MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
> >    }
> > +
> >    return EFI_SUCCESS;
> >  }
> >
> >



-- 
Masami Hiramatsu


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84111): https://edk2.groups.io/g/devel/message/84111
Mute This Topic: https://groups.io/mt/86836377/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-