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
