Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220510091654.8498-1-warp5tw@gmail.com> <20220510091654.8498-8-warp5tw@gmail.com>
 <Yno7IaBNnR5U2GuF@smile.fi.intel.com>
In-Reply-To: <Yno7IaBNnR5U2GuF@smile.fi.intel.com>
From:   Tyrone Ting <warp5tw@gmail.com>
Date:   Wed, 11 May 2022 13:37:43 +0800
Message-ID: <CACD3sJYPrQoWzOkOKM5tWuTWS0uEertBSrwMa5QmX0cMAPF4EQ@mail.gmail.com>
Subject: Re: [PATCH v4 7/9] i2c: npcm: Handle spurious interrupts
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     avifishman70@gmail.com, tmaimon77@gmail.com, tali.perry1@gmail.com,
        venture@google.com, yuenn@google.com, benjaminfair@google.com,
        robh+dt@kernel.org, wsa@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, jarkko.nikula@linux.intel.com,
        semen.protsenko@linaro.org, sven@svenpeter.dev, jsd@semihalf.com,
        lukas.bulwahn@gmail.com, olof@lixom.net, arnd@arndb.de,
        tali.perry@nuvoton.com, Avi.Fishman@nuvoton.com,
        tomer.maimon@nuvoton.com, KWLIU@nuvoton.com, JJLIU0@nuvoton.com,
        kfting@nuvoton.com, openbmc@lists.ozlabs.org,
        linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Andy:

Thank you for your comments and they will be addressed.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> =E6=96=BC 2022=E5=B9=B4=
5=E6=9C=8810=E6=97=A5 =E9=80=B1=E4=BA=8C =E4=B8=8B=E5=8D=886:15=E5=AF=AB=E9=
=81=93=EF=BC=9A
>
> On Tue, May 10, 2022 at 05:16:52PM +0800, Tyrone Ting wrote:
> > From: Tali Perry <tali.perry1@gmail.com>
> >
> > On some platforms in rare cases (1 to 100,000 transactions),
> > the i2c gets a spurious interrupt which means that we enter an interrup=
t
> > but in the interrupt handler we don't find any status bit that points t=
o
> > the reason we got this interrupt.
> >
> > This may be a case of a rare HW issue or signal integrity issue that is
> > still under investigation.
> >
> > In order to overcome this we are doing the following:
> > 1. Disable incoming interrupts in master mode only when slave mode is n=
ot
> >    enabled.
> > 2. Clear end of busy (EOB) after every interrupt.
> > 3. Clear other status bits (just in case since we found them cleared)
> > 4. Return correct status during the interrupt that will finish the
> >    transaction.
> >
> > On next xmit transaction if the bus is still busy the master will issue=
 a
> > recovery process before issuing the new transaction.
>
> ...
>
> > +     /* clear status bits for spurious interrupts */
>
> Clear
>
> ...
>
> > +     /*
> > +      * if irq is not one of the above, make sure EOB is disabled and =
all
>
> If
>
> > +      * status bits are cleared.
> > +      */
>
> ...
>
> > +             /* verify no status bits are still set after bus is relea=
sed */
>
> Verify
>
> ...
>
> > +     /* check HW is OK: SDA and SCL should be high at this point. */
>
> Check
>
> ...
>
> > +     if ((npcm_i2c_get_SDA(&bus->adap) =3D=3D 0) ||
> > +         (npcm_i2c_get_SCL(&bus->adap) =3D=3D 0)) {
>
> This fits one line
>
> > +             dev_err(bus->dev, "I2C%d init fail: lines are low", bus->=
num);
> > +             dev_err(bus->dev, "SDA=3D%d SCL=3D%d", npcm_i2c_get_SDA(&=
bus->adap),
> > +                     npcm_i2c_get_SCL(&bus->adap));
>
> No '\n' at the end of each?!
>
> > +             return -ENXIO;
> > +     }
>
> ...
>
> > +     /* clear status bits for spurious interrupts */
>
> Clear
>
> ...
>
> > +     /* after any xfer, successful or not, stall and EOB must be disab=
led */
>
> After
>
> ...
>
> Maybe you chose the small letter for one-liner comments, but I see even i=
n the
> original code the inconsistent style. Please, add an explanation to the c=
over
> letter and follow it, assuming you add the patch at the end of the series=
 that
> makes comment style consistent (for the one-liners, for the multi-line co=
mments
> we have a clear understanding about the style).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Best Regards,
Tyrone