Hi Leif,
2021年11月27日(土) 2:47 Leif Lindholm <[email protected]>:
>
> 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 <[email protected]>
> > Reported-by: Kazuhiko Sakamoto <[email protected]>
> > ---
> > .../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/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
