-----Original Message-----
From: Joel Stanley <joel@jms.id.au>
Sent: Thursday, May 20, 2021 7:29 AM
To: Quan Nguyen <quan@os.amperecomputing.com>; Ryan Chen
[off-list ref]
Cc: Corey Minyard <redacted>; Rob Herring <robh+dt@kernel.org>;
Andrew Jeffery [off-list ref]; Brendan Higgins
[off-list ref]; Benjamin Herrenschmidt
[off-list ref]; Wolfram Sang [off-list ref]; Philipp Zabel
[off-list ref]; openipmi-developer@lists.sourceforge.net;
devicetree [off-list ref]; Linux ARM
[off-list ref]; linux-aspeed
[off-list ref]; Linux Kernel Mailing List
[off-list ref]; linux-i2c@vger.kernel.org; Open Source
Submission [off-list ref]; Phong Vo
[off-list ref]; Thang Q . Nguyen
[off-list ref]; OpenBMC Maillist
[off-list ref]
Subject: Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK

Ryan, can you please review this change?

On Wed, 19 May 2021 at 07:50, Quan Nguyen
[off-list ref] wrote:

quoted

It is observed that in normal condition, when the last byte sent by
slave, the Tx Done with NAK irq will raise.
But it is also observed that sometimes master issues next transaction
too quick while the slave irq handler is not yet invoked and Tx Done
with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
This Tx Done with NAK irq is raised together with the Slave Match and
Rx Done irq of the next coming transaction from master.
Unfortunately, the current slave irq handler handles the Slave Match
and Rx Done only in higher priority and ignore the Tx Done with NAK,
causing the complain as below:
"aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
0x00000086, but was 0x00000084"

This commit handles this case by emitting a Slave Stop event for the
Tx Done with NAK before processing Slave Match and Rx Done for the
coming transaction from master.

It sounds like this patch is independent of the rest of the series, and can go in
on it's own. Please send it separately to the i2c maintainers and add a suitable
Fixes line, such as:

  Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C
driver")

quoted

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v3:
  + First introduce in v3 [Quan]

 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 724bf30600d6..3fb37c3f23d4
100644

--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c

@@ -254,6 +254,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) {

Can you explain why you need to do this handing inside the SLAVE_MATCH
case?

Could you instead move the TX_NAK handling to be above the SLAVE_MATCH
case?

quoted

+               if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
+                   bus->slave_state ==
+ ASPEED_I2C_SLAVE_READ_PROCESSED) {

Either way, this needs a comment to explain what we're working around.

quoted

+                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
+                       i2c_slave_event(slave, I2C_SLAVE_STOP,

&value);

According the patch assume slave receive TX_NAK will be go to SLAVE_STOP state?

quoted

+               }
                irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
                bus->slave_state = ASPEED_I2C_SLAVE_START;
        }
--
2.28.0