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

Quan Nguyen
2023-11-30T06:53:53Z

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