Re: [PATCH v2 RESEND 2/2] i2c: aspeed: Acknowledge Tx done with and without ACK irq late

[Quan Nguyen]

On 30/11/2023 05:44, Andrew Jeffery wrote:
> On Wed, 2023-11-29 at 16:02 +0700, Quan Nguyen wrote:
> > On 29/11/2023 07:33, Andrew Jeffery wrote:
> > > On Tue, 2023-11-28 at 14:52 +0700, Quan Nguyen wrote:
> > > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> > > > index 79476b46285b..3231f430e335 100644
> > > > --- a/drivers/i2c/busses/i2c-aspeed.c
> > > > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > > > @@ -611,8 +611,9 @@ 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 */
> > >
> > > I'm not a huge fan of this comment, it just says what the code does. It
> > > would be much better to explain *why* the code does what it does.
> > >
> > > I realise describing what the code does was already the gist of the
> > > comment and that you're just updating it to match the change to the
> > > code, but that's my entire problem with it. We'd be better off deleting
> > > it if we're not going to explain why the masking is necessary.
> >
> > Thanks for the comment Andrew.
> >
> > I would prefer to delete it.
> >
> > But if to put some comment, how about:
> >
> > /* Early ack INTR_RX_DONE, INTR_TX_[ACK|NAK] would indicate HW to start
> > receiving/sending new data and may cause a race condition as irq handler
> > not yet to handle these irqs but being acked. Let ack them late in the
> > end of irq handler when those are truly processed */
>
> Please update the patch with this comment. It at least goes some way to
> explain why.

Yes, will do in next version.

> > > > +	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;
> > > > @@ -657,12 +658,12 @@ 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 */
> > > > +	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);
> > >
> > > I'm not sure why the write was conditional, but I'm not sure that
> > > making it unconditional is valid either? Why the change? Why not add
> > > the extra interrupt bits to the condition in addition to the value mask
> > > for the write?
> >
> > In original code, only INTR_RX_DONE was acked late. So the check
> > (irq_received & ASPEED_I2CD_INTR_RX_DONE) is need and that help to save
> > one write() then read() if there was no such irq.
> >
> > In the new code, there is no such check and the drawback is that there
> > always be one write() and one read() for all cases, include the case
> > where there is no irq at all, ie writing 0 into ASPEED_I2C_INTR_STS_REG.
> >
> > And yes, your concern maybe right, we can not say of writing 0 into
> > ASPEED_I2C_INTR_STS_REG is good or not.
> >
> > I checked back my debug log and seeing that irq always come with at
> > least one of INTR_RX_DONE BIT(2), INTR_TX_ACK BIT(0), INTR_TX_NAK BIT(1)
> > raised. So it seems like the case of writing 0 into
> > ASPEED_I2C_INTR_STS_REG is indeed rarely to happen.
> >
> > Do you think we should change it to:
> >
> > if (irq_received & (INTR_RX_DONE | INTR_TX_ACK | INTR_TX_NAK)) {
> > 	writel( irq_received & (INTR_RX_DONE| INTR_TX_ACK| INTR_TX_NAK),
> > 		bus->base + ASPEED_I2C_INTR_STS_REG);
> > 	readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> > }
>
> This is less different from the existing strategy and doesn't require
> any explanation beyond what you're already trying to achieve in the
> patch, so I think you should switch to this approach.
>
> If someone wants to work out why it was done conditionally and argue
> for its removal then they can do that separately.

I agree, will update in next version.

Thanks
- Quan