[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