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
> /**
> 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.
/
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;
> }
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84100): https://edk2.groups.io/g/devel/message/84100
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]
-=-=-=-=-=-=-=-=-=-=-=-
