Hi Leif, 2021年11月27日(土) 2:47 Leif Lindholm <l...@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.hirama...@linaro.org> > > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuh...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-