Re: [PATCH v2 RESEND 1/2] i2c: aspeed: Fix unhandled Tx done with NAK

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 29/11/2023 07:35, Andi Shyti wrote:
Hi Quan,

On Tue, Nov 28, 2023 at 02:52:35PM +0700, Quan Nguyen wrote:
Under normal conditions, after the last byte is sent by the Slave, the
TX_NAK interrupt is raised.  However, it is also observed that
sometimes the Master issues the next transaction too quickly while the
Slave IRQ handler is not yet invoked and the TX_NAK interrupt for the
last byte of the previous READ_PROCESSED state has not been ack’ed.
This TX_NAK interrupt is then raised together with SLAVE_MATCH interrupt
and RX_DONE interrupt of the next coming transaction from Master. The
Slave IRQ handler currently handles the SLAVE_MATCH and RX_DONE, but
ignores the TX_NAK, causing complaints such as
"aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. Expected
0x00000086, but was 0x00000084"

This commit adds code to handle this case by emitting a SLAVE_STOP event
for the TX_NAK before processing the RX_DONE for the coming transaction
from the Master.

Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
Signed-off-by: Quan Nguyen <quan@xxxxxxxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxxx/
---
  drivers/i2c/busses/i2c-aspeed.c | 5 +++++
  1 file changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 28e2a5fc4528..79476b46285b 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -253,6 +253,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
/* Slave was requested, restart state machine. */
  	if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
+		if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
+		    bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
+			irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
+			i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+		}

this is a duplicate of a later "if (...)" satement. What is the
need for having them both?

Thanks Andi for the review.

I assumed the if statement you mentioned is here in [1]. If so, then that is not duplicate.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-aspeed.c?h=v6.7-rc3#n287


The if statement is to process the case when Slave sending data to Master but being NAK, the I2C_SLAVE_STOP event will emit later in switch-case statement. But it is only for the case INTR_TX_NAK without INTR_SLAVE_MATCH.

The new code is for the case of INTR_TX_NAK with INTR_SLAVE_MATCH. What it does is to detect if there is a mix of INTR_TX_NAK of previous i2c transaction and the start of new i2c transaction, indicate by INTR_SLAVE_MATCH which is only raised when Slave found its address matched on the first byte it received. If so, the new code will try to emit the I2C_SLAVE_STOP first to complete the previous transaction and process the rest as a new request.

So if this was the case (with INTR_SLAVE_MATCH), the INTR_RX_DONE should always raise with INTR_SLAVE_MATCH because Slave did receive the data which matched with its Slave address. And this will be translated into either I2C_SLAVE_[READ|WRITE]_REQUESTED and that make the if statement you mentioned [1] evaluate to false and skip.

So, in short, the new code is trying to handle the case of INTR_TX_NAK with INTR_SLAVE_MATCH first before let the rest process as normal.

Thanks,
- Quan




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux