[PATCH v3 0/2] i2c: aspeed: Late ack Tx done irqs and handle coalesced start with stop conditions

[PATCH v3 0/2] i2c: aspeed: Late ack Tx done irqs and handle coalesced start with stop conditions

[Quan Nguyen]

This series consists of two patches to handle the below issues observed
when testing with slave mode:
  + The coalesced stop condition with the start conditions
  + Early ack'ed of Tx done (ACK and NAK) causing "Unexpected Ack on
  read request".

This series was verified with ast2500 and ast2600.

The prior discussion could be found at:
https://lore.kernel.org/all/20231128075236.2724038-1-quan@os.amperecomputing.com/

v3:
  + Fix the unconditional write when ack the irqs              [Andrew]
  + Handle the coalesced stop condition with the
start conditions                                               [Andrew]
  + Refactor the code to enhance code readability                [Quan]
  + Revised commit message                                       [Quan]

v2:
  + Split these patches to separate series                       [Joel]
  + Added the Fixes lines                                        [Joel]
  + Fixed multiline comment                                      [Joel]
  + Refactor irq clearing code                          [Joel, Guenter]
  + Revised commit message                                 [Joel, Quan]

v1:
  + These patches are first introduced from this disscusstion
https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/

Quan Nguyen (2):
  i2c: aspeed: Handle the coalesced stop conditions with the start
    conditions.
  i2c: aspeed: Acknowledge Tx done with and without ACK irq late

 drivers/i2c/busses/i2c-aspeed.c | 68 +++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 24 deletions(-)

-- 
2.35.1

[PATCH v3 1/2] i2c: aspeed: Handle the coalesced stop conditions with the start conditions.

[Quan Nguyen]

Some masters may drive the transfers with low enough latency between
the nak/stop phase of the current command and the start/address phase
of the following command that the interrupts are coalesced by the
time we process them.
Handle the stop conditions before processing SLAVE_MATCH to fix the
complaints that sometimes occur below.

"aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. Expected
0x00000086, but was 0x00000084"

Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v3:
  + Change to handle the coalesced stop condition with the start
conditions                                                            [Andrew]
  + Revised commit message                                              [Quan]

v2:
  + Split to separate series                                            [Joel]
  + Added the Fixes line                                                [Joel]
  + Revised commit message                                              [Quan]

v1:
  + First introduced in
https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
---
 drivers/i2c/busses/i2c-aspeed.c | 47 ++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 28e2a5fc4528..1c2a4f4c4e1b 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -249,18 +249,45 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 	if (!slave)
 		return 0;
 
-	command = readl(bus->base + ASPEED_I2C_CMD_REG);
+	/*
+	 * Handle stop conditions early, prior to SLAVE_MATCH. Some masters may drive
+	 * transfers with low enough latency between the nak/stop phase of the current
+	 * command and the start/address phase of the following command that the
+	 * interrupts are coalesced by the time we process them.
+	 */
+	if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+		irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
+		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+	}
 
-	/* Slave was requested, restart state machine. */
+	if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
+	    bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
+		irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
+		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+	}
+
+	/* Propagate any stop conditions to the slave implementation. */
+	if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
+		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+		bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
+	}
+	/*
+	 * Now that we've dealt with any potentially coalesced stop conditions,
+	 * address any start conditions.
+	 */
 	if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
 		irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
 		bus->slave_state = ASPEED_I2C_SLAVE_START;
 	}
 
-	/* Slave is not currently active, irq was for someone else. */
+	/*
+	 * If the slave has been stopped and not started then slave interrupt
+	 * handling is complete.
+	 */
 	if (bus->slave_state == ASPEED_I2C_SLAVE_INACTIVE)
 		return irq_handled;
 
+	command = readl(bus->base + ASPEED_I2C_CMD_REG);
 	dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
 		irq_status, command);
 
@@ -279,17 +306,6 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 		irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
 	}
 
-	/* Slave was asked to stop. */
-	if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
-		irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
-		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
-	}
-	if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
-	    bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
-		irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
-		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
-	}
-
 	switch (bus->slave_state) {
 	case ASPEED_I2C_SLAVE_READ_REQUESTED:
 		if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_ACK))
@@ -324,8 +340,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 		i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
 		break;
 	case ASPEED_I2C_SLAVE_STOP:
-		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
-		bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
+		/* Stop event handling is done early. Unreachable. */
 		break;
 	case ASPEED_I2C_SLAVE_START:
 		/* Slave was just started. Waiting for the next event. */;
-- 
2.35.1

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

[Quan Nguyen]

Commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in
interrupt handler") acknowledges most interrupts early before the slave
irq handler is executed, except for the "Receive Done Interrupt status"
which is acknowledged late in the interrupt.
However, it has been observed that the early acknowledgment of "Transmit
Done Interrupt Status" (with ACK or NACK) often causes the interrupt to
be raised in READ REQUEST state, that shows the
"Unexpected ACK on read request." complaint messages.

Assuming that the "Transmit Done" interrupt should only be acknowledged
once it is truly processed, this commit fixes that issue by acknowledging
interrupts for both ACK and NACK cases late in the interrupt handler.

Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler")
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v3:
  + Fix the unconditinal write when ack the irqs               [Andrew]
  + Refactor the code to enhance code readability                [Quan]
  + Fix grammar in commit message                                [Quan]

v2:
  + Split to separate series                                     [Joel]
  + Added the Fixes line                                         [Joel]
  + Fixed multiline comment                                      [Joel]
  + Refactor irq clearing code                          [Joel, Guenter]
  + Revised commit message                                       [Joel]
  + Revised commit message                                       [Quan]
  + About a note to remind why the readl() should immediately follow the
writel() to fix the race condition when clearing irq status from commit
c926c87b8e36 ("i2c: aspeed: Avoid i2c interrupt status clear race
condition"), I think it looks straight forward in this patch and decided
not to add that note.                                            [Joel]

v1:
  + First introduced in
https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
---
 drivers/i2c/busses/i2c-aspeed.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 1c2a4f4c4e1b..967a26dd4ffa 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -617,13 +617,19 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
 {
 	struct aspeed_i2c_bus *bus = dev_id;
-	u32 irq_received, irq_remaining, irq_handled;
+	u32 irq_received, irq_remaining, irq_handled, irq_ack_last;
 
 	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,
-	       bus->base + ASPEED_I2C_INTR_STS_REG);
+
+	/*
+	 * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to
+	 * start receiving or sending new data, and this may cause a race condition
+	 * as the irq handler has not yet handled these irqs but is being acked.
+	 * Let's ack them late at the end of the irq handler when those are truly processed.
+	 */
+	irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK;
+	writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG);
 	readl(bus->base + ASPEED_I2C_INTR_STS_REG);
 	irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
 	irq_remaining = irq_received;
@@ -667,12 +673,11 @@ 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);
+	if (irq_received & irq_ack_last) {
+		writel(irq_received & irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG);
 		readl(bus->base + ASPEED_I2C_INTR_STS_REG);
 	}
+
 	spin_unlock(&bus->lock);
 	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
 }
-- 
2.35.1

Re: [PATCH v3 1/2] i2c: aspeed: Handle the coalesced stop conditions with the start conditions.

[Andrew Jeffery]

On Fri, 2023-12-08 at 10:31 +0700, Quan Nguyen wrote:
> Some masters may drive the transfers with low enough latency between
> the nak/stop phase of the current command and the start/address phase
> of the following command that the interrupts are coalesced by the
> time we process them.
> Handle the stop conditions before processing SLAVE_MATCH to fix the
> complaints that sometimes occur below.
> 
> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. Expected
> 0x00000086, but was 0x00000084"
> 
> Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + Change to handle the coalesced stop condition with the start
> conditions                                                            [Andrew]
>   + Revised commit message                                              [Quan]
> 
> v2:
>   + Split to separate series                                            [Joel]
>   + Added the Fixes line                                                [Joel]
>   + Revised commit message                                              [Quan]
> 
> v1:
>   + First introduced in
> https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 47 ++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 28e2a5fc4528..1c2a4f4c4e1b 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -249,18 +249,45 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  	if (!slave)
>  		return 0;
>  
> -	command = readl(bus->base + ASPEED_I2C_CMD_REG);
> +	/*
> +	 * Handle stop conditions early, prior to SLAVE_MATCH. Some masters may drive
> +	 * transfers with low enough latency between the nak/stop phase of the current
> +	 * command and the start/address phase of the following command that the
> +	 * interrupts are coalesced by the time we process them.
> +	 */
> +	if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> +		irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
> +		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> +	}
>  
> -	/* Slave was requested, restart state machine. */
> +	if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
> +	    bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
> +		irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
> +		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> +	}
> +
> +	/* Propagate any stop conditions to the slave implementation. */
> +	if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> +		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
> +		bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
> +	}
> +	/*

If there's a reason to do a v4 then an extra empty line above the
comment would be nice. But let's not get hung up on that if everyone
else is happy.

Thanks for the fixes!

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

> +	 * Now that we've dealt with any potentially coalesced stop conditions,
> +	 * address any start conditions.
> +	 */
>  	if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>  		irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>  		bus->slave_state = ASPEED_I2C_SLAVE_START;
>  	}
>  

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

[Andrew Jeffery]

On Fri, 2023-12-08 at 10:31 +0700, Quan Nguyen wrote:
> Commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in
> interrupt handler") acknowledges most interrupts early before the slave
> irq handler is executed, except for the "Receive Done Interrupt status"
> which is acknowledged late in the interrupt.
> However, it has been observed that the early acknowledgment of "Transmit
> Done Interrupt Status" (with ACK or NACK) often causes the interrupt to
> be raised in READ REQUEST state, that shows the
> "Unexpected ACK on read request." complaint messages.
> 
> Assuming that the "Transmit Done" interrupt should only be acknowledged
> once it is truly processed, this commit fixes that issue by acknowledging
> interrupts for both ACK and NACK cases late in the interrupt handler.
> 
> Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler")
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + Fix the unconditinal write when ack the irqs               [Andrew]
>   + Refactor the code to enhance code readability                [Quan]
>   + Fix grammar in commit message                                [Quan]
> 
> v2:
>   + Split to separate series                                     [Joel]
>   + Added the Fixes line                                         [Joel]
>   + Fixed multiline comment                                      [Joel]
>   + Refactor irq clearing code                          [Joel, Guenter]
>   + Revised commit message                                       [Joel]
>   + Revised commit message                                       [Quan]
>   + About a note to remind why the readl() should immediately follow the
> writel() to fix the race condition when clearing irq status from commit
> c926c87b8e36 ("i2c: aspeed: Avoid i2c interrupt status clear race
> condition"), I think it looks straight forward in this patch and decided
> not to add that note.                                            [Joel]
> 
> v1:
>   + First introduced in
> https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 1c2a4f4c4e1b..967a26dd4ffa 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -617,13 +617,19 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>  {
>  	struct aspeed_i2c_bus *bus = dev_id;
> -	u32 irq_received, irq_remaining, irq_handled;
> +	u32 irq_received, irq_remaining, irq_handled, irq_ack_last;

`irq_ack_last` might be better as a macro, but you're probably saved by
the optimiser anyway. If there's another reason to do a v4 or others
are unhappy with it then consider fixing it, otherwise:

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Thanks.

Re: [PATCH v3 1/2] i2c: aspeed: Handle the coalesced stop conditions with the start conditions.

[Andi Shyti]

Hi Quan,

On Fri, Dec 08, 2023 at 10:31:41AM +0700, Quan Nguyen wrote:
> Some masters may drive the transfers with low enough latency between
> the nak/stop phase of the current command and the start/address phase
> of the following command that the interrupts are coalesced by the
> time we process them.
> Handle the stop conditions before processing SLAVE_MATCH to fix the
> complaints that sometimes occur below.
> 
> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. Expected
> 0x00000086, but was 0x00000084"
> 
> Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

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

[Andi Shyti]

Hi Quan,

[...]

> -	/* Ack all interrupts except for Rx done */
> -	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> -	       bus->base + ASPEED_I2C_INTR_STS_REG);
> +
> +	/*
> +	 * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to
> +	 * start receiving or sending new data, and this may cause a race condition
> +	 * as the irq handler has not yet handled these irqs but is being acked.
> +	 * Let's ack them late at the end of the irq handler when those are truly processed.
> +	 */
> +	irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK;
> +	writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG);

I like Andrews suggestion of having irq_ack_last as a define that
is already negated, instead of negating it in the writel, which
makes it a bit difficult to read.

Besides, ack_last, as a name is not very meaningful, I'd rather
call it irq_ack_rx_tx (or something similar).

But I'm not going to block it for this, up to you if you want to
send a new version.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

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

[Quan Nguyen]

On 08/12/2023 11:00, Andrew Jeffery wrote:
> On Fri, 2023-12-08 at 10:31 +0700, Quan Nguyen wrote:
>> Commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in
>> interrupt handler") acknowledges most interrupts early before the slave
>> irq handler is executed, except for the "Receive Done Interrupt status"
>> which is acknowledged late in the interrupt.
>> However, it has been observed that the early acknowledgment of "Transmit
>> Done Interrupt Status" (with ACK or NACK) often causes the interrupt to
>> be raised in READ REQUEST state, that shows the
>> "Unexpected ACK on read request." complaint messages.
>>
>> Assuming that the "Transmit Done" interrupt should only be acknowledged
>> once it is truly processed, this commit fixes that issue by acknowledging
>> interrupts for both ACK and NACK cases late in the interrupt handler.
>>
>> Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler")
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + Fix the unconditinal write when ack the irqs               [Andrew]
>>    + Refactor the code to enhance code readability                [Quan]
>>    + Fix grammar in commit message                                [Quan]
>>
>> v2:
>>    + Split to separate series                                     [Joel]
>>    + Added the Fixes line                                         [Joel]
>>    + Fixed multiline comment                                      [Joel]
>>    + Refactor irq clearing code                          [Joel, Guenter]
>>    + Revised commit message                                       [Joel]
>>    + Revised commit message                                       [Quan]
>>    + About a note to remind why the readl() should immediately follow the
>> writel() to fix the race condition when clearing irq status from commit
>> c926c87b8e36 ("i2c: aspeed: Avoid i2c interrupt status clear race
>> condition"), I think it looks straight forward in this patch and decided
>> not to add that note.                                            [Joel]
>>
>> v1:
>>    + First introduced in
>> https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
>> ---
>>   drivers/i2c/busses/i2c-aspeed.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 1c2a4f4c4e1b..967a26dd4ffa 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -617,13 +617,19 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>   static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>   {
>>   	struct aspeed_i2c_bus *bus = dev_id;
>> -	u32 irq_received, irq_remaining, irq_handled;
>> +	u32 irq_received, irq_remaining, irq_handled, irq_ack_last;
> 
> `irq_ack_last` might be better as a macro, but you're probably saved by
> the optimiser anyway. If there's another reason to do a v4 or others
> are unhappy with it then consider fixing it, otherwise:
> 
> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> 

I'll send out the v4 to use the defined macro instead as below:

Thanks Andi, for the suggestion on the macro name.

diff --git a/drivers/i2c/busses/i2c-aspeed.c 
b/drivers/i2c/busses/i2c-aspeed.c
index 5511fd46a65e..0f67218cf04a 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -93,6 +93,10 @@
  		 ASPEED_I2CD_INTR_RX_DONE | \
  		 ASPEED_I2CD_INTR_TX_NAK |  \
  		 ASPEED_I2CD_INTR_TX_ACK)
+#define ASPEED_I2CD_INTR_ACK_RX_TX	    \
+		(ASPEED_I2CD_INTR_RX_DONE | \
+		 ASPEED_I2CD_INTR_TX_ACK |  \
+		 ASPEED_I2CD_INTR_TX_NAK)

Thanks for the review,
- Quan

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

[Quan Nguyen]

On 08/12/2023 10:31, Quan Nguyen wrote:
> Commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in
> interrupt handler") acknowledges most interrupts early before the slave
> irq handler is executed, except for the "Receive Done Interrupt status"
> which is acknowledged late in the interrupt.
> However, it has been observed that the early acknowledgment of "Transmit
> Done Interrupt Status" (with ACK or NACK) often causes the interrupt to
> be raised in READ REQUEST state, that shows the
> "Unexpected ACK on read request." complaint messages.
> 
> Assuming that the "Transmit Done" interrupt should only be acknowledged
> once it is truly processed, this commit fixes that issue by acknowledging
> interrupts for both ACK and NACK cases late in the interrupt handler.
> 
> Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler")
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>    + Fix the unconditinal write when ack the irqs               [Andrew]
>    + Refactor the code to enhance code readability                [Quan]
>    + Fix grammar in commit message                                [Quan]
> 
> v2:
>    + Split to separate series                                     [Joel]
>    + Added the Fixes line                                         [Joel]
>    + Fixed multiline comment                                      [Joel]
>    + Refactor irq clearing code                          [Joel, Guenter]
>    + Revised commit message                                       [Joel]
>    + Revised commit message                                       [Quan]
>    + About a note to remind why the readl() should immediately follow the
> writel() to fix the race condition when clearing irq status from commit
> c926c87b8e36 ("i2c: aspeed: Avoid i2c interrupt status clear race
> condition"), I think it looks straight forward in this patch and decided
> not to add that note.                                            [Joel]
> 
> v1:
>    + First introduced in
> https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
> ---
>   drivers/i2c/busses/i2c-aspeed.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 1c2a4f4c4e1b..967a26dd4ffa 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -617,13 +617,19 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>   static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>   {
>   	struct aspeed_i2c_bus *bus = dev_id;
> -	u32 irq_received, irq_remaining, irq_handled;
> +	u32 irq_received, irq_remaining, irq_handled, irq_ack_last;
>   
>   	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,
> -	       bus->base + ASPEED_I2C_INTR_STS_REG);
> +
> +	/*
> +	 * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to
> +	 * start receiving or sending new data, and this may cause a race condition
> +	 * as the irq handler has not yet handled these irqs but is being acked.
> +	 * Let's ack them late at the end of the irq handler when those are truly processed.
> +	 */
> +	irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK;
> +	writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG);

As there will be v4, I'd like to improve this line of code a bit.

During debug, there are significant number of times when the 
irq_received has the value of 0x00000004 (INTR_RX_DONE) or 0x00000001 
(INTR_TX_ACK). This makes the "irq_received & ~irq_ack_last" turn out to 
be 0.

AFAIK, it does not make sense to keep repeatedly writing the 0 to the HW 
then try to re-read it with readl() every time. I'd like to change this 
HW access conditionally to avoid those unnecessary access.

The change would looks like below:

diff --git a/drivers/i2c/busses/i2c-aspeed.c 
b/drivers/i2c/busses/i2c-aspeed.c
index 5511fd46a65e..0f67218cf04a 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -93,6 +93,10 @@
  		 ASPEED_I2CD_INTR_RX_DONE | \
  		 ASPEED_I2CD_INTR_TX_NAK |  \
  		 ASPEED_I2CD_INTR_TX_ACK)
+#define ASPEED_I2CD_INTR_ACK_RX_TX         \
+		(ASPEED_I2CD_INTR_RX_DONE | \
+		 ASPEED_I2CD_INTR_TX_ACK |  \
+		 ASPEED_I2CD_INTR_TX_NAK)

  /* 0x14 : I2CD Command/Status Register   */
  #define ASPEED_I2CD_SCL_LINE_STS			BIT(18)
@@ -622,10 +626,19 @@ 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,
-	       bus->base + ASPEED_I2C_INTR_STS_REG);
-	readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+
+	/*
+	 * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would
+        ...
+	 */
+	if (irq_received & ~ASPEED_I2CD_INTR_ACK_RX_TX) {
+		writel(irq_received & ~ASPEED_I2CD_INTR_ACK_RX_TX,
+		       bus->base + ASPEED_I2C_INTR_STS_REG);
+		readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+	}
+
  	irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
  	irq_remaining = irq_received;

Thanks,
- Quan

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

[Quan Nguyen]

On 10/12/2023 03:44, Andi Shyti wrote:
> Hi Quan,
> 
> [...]
> 
>> -	/* Ack all interrupts except for Rx done */
>> -	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
>> -	       bus->base + ASPEED_I2C_INTR_STS_REG);
>> +
>> +	/*
>> +	 * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to
>> +	 * start receiving or sending new data, and this may cause a race condition
>> +	 * as the irq handler has not yet handled these irqs but is being acked.
>> +	 * Let's ack them late at the end of the irq handler when those are truly processed.
>> +	 */
>> +	irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK;
>> +	writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> I like Andrews suggestion of having irq_ack_last as a define that
> is already negated, instead of negating it in the writel, which
> makes it a bit difficult to read.
> 

Yes, but the it still need to negate again when do the write to late ack 
them later in the end of irq handler. So I'll keep the define as below 
in my v4:

+#define ASPEED_I2CD_INTR_ACK_RX_TX	    \
+		(ASPEED_I2CD_INTR_RX_DONE | \
+		 ASPEED_I2CD_INTR_TX_ACK |  \
+		 ASPEED_I2CD_INTR_TX_NAK)

The early ack will look like this:

+		writel(irq_received & ~ASPEED_I2CD_INTR_ACK_RX_TX,
+		       bus->base + ASPEED_I2C_INTR_STS_REG);
+		readl(bus->base + ASPEED_I2C_INTR_STS_REG);

And the late ack:

-	/* Ack Rx done */
-	if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
-		writel(ASPEED_I2CD_INTR_RX_DONE,
+	if (irq_received & ASPEED_I2CD_INTR_ACK_RX_TX) {
+		writel(irq_received & ASPEED_I2CD_INTR_ACK_RX_TX,
  		       bus->base + ASPEED_I2C_INTR_STS_REG);
  		readl(bus->base + ASPEED_I2C_INTR_STS_REG);
  	}

> Besides, ack_last, as a name is not very meaningful, I'd rather
> call it irq_ack_rx_tx (or something similar).
> 
> But I'm not going to block it for this, up to you if you want to
> send a new version.
> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> 

Thanks, Andi for the comments.

I will send out v4 to address those.

- Quan

Re: [PATCH v3 1/2] i2c: aspeed: Handle the coalesced stop conditions with the start conditions.

[Quan Nguyen]

On 08/12/2023 10:56, Andrew Jeffery wrote:
> On Fri, 2023-12-08 at 10:31 +0700, Quan Nguyen wrote:
>> Some masters may drive the transfers with low enough latency between
>> the nak/stop phase of the current command and the start/address phase
>> of the following command that the interrupts are coalesced by the
>> time we process them.
>> Handle the stop conditions before processing SLAVE_MATCH to fix the
>> complaints that sometimes occur below.
>>
>> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. Expected
>> 0x00000086, but was 0x00000084"
>>
>> Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + Change to handle the coalesced stop condition with the start
>> conditions                                                            [Andrew]
>>    + Revised commit message                                              [Quan]
>>
>> v2:
>>    + Split to separate series                                            [Joel]
>>    + Added the Fixes line                                                [Joel]
>>    + Revised commit message                                              [Quan]
>>
>> v1:
>>    + First introduced in
>> https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
>> ---
>>   drivers/i2c/busses/i2c-aspeed.c | 47 ++++++++++++++++++++++-----------
>>   1 file changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 28e2a5fc4528..1c2a4f4c4e1b 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -249,18 +249,45 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>   	if (!slave)
>>   		return 0;
>>   
>> -	command = readl(bus->base + ASPEED_I2C_CMD_REG);
>> +	/*
>> +	 * Handle stop conditions early, prior to SLAVE_MATCH. Some masters may drive
>> +	 * transfers with low enough latency between the nak/stop phase of the current
>> +	 * command and the start/address phase of the following command that the
>> +	 * interrupts are coalesced by the time we process them.
>> +	 */
>> +	if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> +		irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> +		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> +	}
>>   
>> -	/* Slave was requested, restart state machine. */
>> +	if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
>> +	    bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
>> +		irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>> +		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> +	}
>> +
>> +	/* Propagate any stop conditions to the slave implementation. */
>> +	if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
>> +		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
>> +		bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
>> +	}
>> +	/*
> 
> If there's a reason to do a v4 then an extra empty line above the
> comment would be nice. But let's not get hung up on that if everyone
> else is happy.
> 
> Thanks for the fixes!
> 
> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> 

Thanks Andrew,

I'll add your Reviewed-by in my v4 with that extra empty lime before the 
comment.

Thanks for the review
- Quan

Re: [PATCH v3 1/2] i2c: aspeed: Handle the coalesced stop conditions with the start conditions.

[Quan Nguyen]

On 10/12/2023 03:28, Andi Shyti wrote:
> Hi Quan,
> 
> On Fri, Dec 08, 2023 at 10:31:41AM +0700, Quan Nguyen wrote:
>> Some masters may drive the transfers with low enough latency between
>> the nak/stop phase of the current command and the start/address phase
>> of the following command that the interrupts are coalesced by the
>> time we process them.
>> Handle the stop conditions before processing SLAVE_MATCH to fix the
>> complaints that sometimes occur below.
>>
>> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. Expected
>> 0x00000086, but was 0x00000084"
>>
>> Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> 

Thanks Andi,
I'll add your Reviewed-by in v4

Thanks,
- Quan