Re: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late

[Quan Nguyen]

On 20/05/2021 06:43, Joel Stanley wrote:
> On Wed, 19 May 2021 at 07:50, Quan Nguyen <quan@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > With Tx done w/wo ACK are ack'ed early at beginning of irq handler,
>
> Is w/wo a typo? If not, please write the full words ("with and without")
It is "with and without", will fix in next version

> > it is observed that, usually, the Tx done with Ack irq raises in the
> > READ REQUESTED state. This is unexpected and complaint as below appear:
> > "Unexpected Ack on read request"
> >
> > Assumed that Tx done should only be ack'ed once it was truly processed,
> > switch to late ack'ed this two irqs and seen this issue go away through
> > test with AST2500..
>
> Please read Guneter's commit message
> 2be6b47211e17e6c90ead40d24d2a5cc815f2d5c to confirm that your changes
> do not invalidate the fix that they made.  Add them to CC for review.
>
> Again, this is a fix that is independent of the ssif work. Please send
> it separately with a Fixes line.
Will do and separate this patch into other series in next version.

> > Signed-off-by: Quan Nguyen <quan@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > v3:
> >    + First introduce in v3 [Quan]
> >
> >   drivers/i2c/busses/i2c-aspeed.c | 26 ++++++++++++++++++--------
> >   1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> > index 3fb37c3f23d4..b2e9c8f0ddf7 100644
> > --- a/drivers/i2c/busses/i2c-aspeed.c
> > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > @@ -606,8 +606,12 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >
> >          spin_lock(&bus->lock);
> >          irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> > -       /* Ack all interrupts except for Rx done */
> > -       writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> > +       /*
> > +        * Ack all interrupts except for Rx done and
> > +        * Tx done with/without ACK
>
> Nit: this comment can be on one line.
Thanks, will fix.
> > +        */
> > +       writel(irq_received &
> > +              ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK),
> >                 bus->base + ASPEED_I2C_INTR_STS_REG);
> >          readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >          irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
> > @@ -652,12 +656,18 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >                          "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> >                          irq_received, irq_handled);
> >
> > -       /* Ack Rx done */
> > -       if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
> > -               writel(ASPEED_I2CD_INTR_RX_DONE,
> > -                      bus->base + ASPEED_I2C_INTR_STS_REG);
> > -               readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> > -       }
> > +       /* Ack Rx done and Tx done with/without ACK */
> > +       /* Note: Re-use irq_handled variable */
>
> I'm not sure what this note means.
>
> > +       irq_handled = 0;
> > +       if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> > +               irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
> > +       if (irq_received & ASPEED_I2CD_INTR_TX_ACK)
> > +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
> > +       if (irq_received & ASPEED_I2CD_INTR_TX_NAK)
> > +               irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
> > +       writel(irq_handled, bus->base + ASPEED_I2C_INTR_STS_REG);
>
> Are you intentionally only acking the bits that are set when we read
> from STS_REG at the start of the handler? If not, we could write this
> instead:
>
> writel(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK |
> ASPEED_I2CD_INTR_TX_NAK,
>          bus->base + ASPEED_I2C_INTR_STS_REG);
>
> If you only want to ack the bits that are set, then do this:
>
>    writel(irq_received &
>              (ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK |
> ASPEED_I2CD_INTR_TX_NAK),
>           bus->base + ASPEED_I2C_INTR_STS_REG);
>
> That way, you can avoid all of the tests.
Thanks, will fix this in next version.

> > +       readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>
> When you move this, please add a comment that reminds us why we do a
> write-then-read (see commit c926c87b8e36dcc0ea5c2a0a0227ed4f32d0516a).
Will fix in next version.
> > +
> >          spin_unlock(&bus->lock);
> >          return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> >   }
> > --
> > 2.28.0