From mboxrd@z Thu Jan 1 00:00:00 1970
Received: from mail-il1-f174.google.com (mail-il1-f174.google.com [209.85.166.174])
by mx.groups.io with SMTP id smtpd.web09.32013.1637981137774992098
for <devel@edk2.groups.io>;
Fri, 26 Nov 2021 18:45:38 -0800
Authentication-Results: mx.groups.io;
dkim=pass header.i=@linaro.org header.s=google header.b=Bqf5ByPm;
spf=pass (domain: linaro.org, ip: 209.85.166.174, mailfrom: masami.hiramatsu@linaro.org)
Received: by mail-il1-f174.google.com with SMTP id 15so1767648ilq.2
for <devel@edk2.groups.io>; Fri, 26 Nov 2021 18:45:37 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=linaro.org; s=google;
h=mime-version:references:in-reply-to:from:date:message-id:subject:to
:cc:content-transfer-encoding;
bh=rye+mXA+diFBC70APATJ4VQ5MXL/XVxJw95BBI5G+vc=;
b=Bqf5ByPmvR/j7wQcsxpxEAbdthz80a7QeadQb79F4D7En7xlsCYHq1+OiJl/gnTHWC
LzqnLlsFvu7CG7g2YIpMlveJEdjzW3kRWIegdv0x5UEoKJ251NmNf9VuO99vpzLFPpfV
1cnT5RjDa2xNLD8N+k2xtFb7YCn53UK6DraMVn52dubGcClEG6NBXw41WrKgxjAGwGOD
g8+8sRi44G3w7kNtLtPqXSjqNHm3i067heKd0QxLRQz0dTKGwEv3XpNvkbYlslZNtdKm
v6IPCp0BE6fKsS8vh9QIzUhG2LKre+VuOuXU36OAKVZIvj/R2agzQNLSi84ugGZoRqZl
kXxQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20210112;
h=x-gm-message-state:mime-version:references:in-reply-to:from:date
:message-id:subject:to:cc:content-transfer-encoding;
bh=rye+mXA+diFBC70APATJ4VQ5MXL/XVxJw95BBI5G+vc=;
b=o4wSQUyKPxJTWADh6l1U/NikOx0dRR3owbu3IRE/mrT1qMdHJ5YMo7GPtz6r6O9Fi/
5GU5cSGufquiP162mYWD+QLdCLGBsoRloKbahJLvwYTYQ8mY/EKNFXop/pgK2aRh++Ro
ym4WYaR2oi1ixOqSVyNUlp6c0asSy/VxQyErlmE/J/uGdDqenDcIsmP+rS7UW8ih8Z8w
9qbuRKMNa7Qqin+YwFYR9mIfakgvZnmmmUvTQTn03fstocujnFSuNfNwFf+dAdwPBo/0
JWGqea0CJ+NNcHw1mo40Vitbt4JWqkTRVzuyJ6d4dIIPBjIXHTaX6yNhwsDeISUtBf2V
wssQ==
X-Gm-Message-State: AOAM531KZ73moxA4JwGefgl3Os6wzLi0zRp57HkvaXy+yyX1ezvJyO7V
jaVra+lOPBk35nHGLCDpy+xLs2jgPIy4xI4NTgPn4g==
X-Google-Smtp-Source: ABdhPJx9sB9CuGHMr6aYQBpx8DYcgj004uiBv2dGKEGX7w2+kRMKwqnH2K7yu0PCEZuQX5I/pPTZR8me6XYZqjDVopM=
X-Received: by 2002:a05:6e02:1b8a:: with SMTP id h10mr33899793ili.14.1637981137120;
Fri, 26 Nov 2021 18:45:37 -0800 (PST)
MIME-Version: 1.0
References: <163610419943.391624.9289897029386201296.stgit@localhost>
<163610420797.391624.17492204385268340229.stgit@localhost> <YaEdw1/PBpbLKMTk@leviathan>
In-Reply-To: <YaEdw1/PBpbLKMTk@leviathan>
From: "Masami Hiramatsu" <masami.hiramatsu@linaro.org>
Date: Sat, 27 Nov 2021 11:45:26 +0900
Message-ID: <CAA93ih1qSiG4Bs4hX_+UoupLC+o8HqAJk6w6JOFG-axuwd=7oQ@mail.gmail.com>
Subject: Re: [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy
To: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb@kernel.org>, devel@edk2.groups.io,
Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>,
Masahisa Kojima <masahisa.kojima@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Hi Leif,
2021=E5=B9=B411=E6=9C=8827=E6=97=A5(=E5=9C=9F) 2:47 Leif Lindholm <leif@nuv=
iainc.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/SynQua=
cerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacer=
I2cDxe.c
> > index 31f6e3072f..380eba8059 100644
> > --- a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cD=
xe.c
> > +++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cD=
xe.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 =3D 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 =3D MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
> > Bcr =3D 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 =3D 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;
> > }
> >
> >
--=20
Masami Hiramatsu