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

["Leif Lindholm" <leif@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@linaro.org>
> Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@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;
>  }
>  
>