* [PATCH v2 0/2] i2c: aspeed: Late ack Tx done irqs and fix unhandled Tx done with NAK
@ 2021-06-16 3:10 ` Quan Nguyen
0 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Wolfram Sang, Jae Hyun Yoo, Guenter Roeck,
linux-i2c, openbmc, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Open Source Submission, Phong Vo, Thang Q . Nguyen
This series consists of two patches to fix the below issues observed
when testing with slave mode:
+ Unhandled Tx done with NAK
+ Early ack'ed of Tx done (ACK and NAK) causing "Unexpected Ack on
read request".
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 in
https://lkml.org/lkml/2021/5/19/205
Quan Nguyen (2):
i2c: aspeed: Fix unhandled Tx done with NAK
i2c: aspeed: Acknowledge Tx done with and without ACK irq late
drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
--
2.28.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 0/2] i2c: aspeed: Late ack Tx done irqs and fix unhandled Tx done with NAK
@ 2021-06-16 3:10 ` Quan Nguyen
0 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Wolfram Sang, Jae Hyun Yoo, Guenter Roeck,
linux-i2c, openbmc, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Open Source Submission, Thang Q . Nguyen, Phong Vo
This series consists of two patches to fix the below issues observed
when testing with slave mode:
+ Unhandled Tx done with NAK
+ Early ack'ed of Tx done (ACK and NAK) causing "Unexpected Ack on
read request".
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 in
https://lkml.org/lkml/2021/5/19/205
Quan Nguyen (2):
i2c: aspeed: Fix unhandled Tx done with NAK
i2c: aspeed: Acknowledge Tx done with and without ACK irq late
drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 0/2] i2c: aspeed: Late ack Tx done irqs and fix unhandled Tx done with NAK
@ 2021-06-16 3:10 ` Quan Nguyen
0 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Wolfram Sang, Jae Hyun Yoo, Guenter Roeck,
linux-i2c, openbmc, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Open Source Submission, Phong Vo, Thang Q . Nguyen
This series consists of two patches to fix the below issues observed
when testing with slave mode:
+ Unhandled Tx done with NAK
+ Early ack'ed of Tx done (ACK and NAK) causing "Unexpected Ack on
read request".
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 in
https://lkml.org/lkml/2021/5/19/205
Quan Nguyen (2):
i2c: aspeed: Fix unhandled Tx done with NAK
i2c: aspeed: Acknowledge Tx done with and without ACK irq late
drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] i2c: aspeed: Fix unhandled Tx done with NAK
2021-06-16 3:10 ` Quan Nguyen
(?)
(?)
@ 2021-06-16 3:10 ` Quan Nguyen
-1 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: linux-aspeed
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@os.amperecomputing.com>
---
v2:
+ Split to separate series [Joel]
+ Added the Fixes line [Joel]
+ Revised commit message [Quan]
v1:
+ First introduced in https://lkml.org/lkml/2021/5/19/205
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) {
+ 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);
+ }
irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
bus->slave_state = ASPEED_I2C_SLAVE_START;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 1/2] i2c: aspeed: Fix unhandled Tx done with NAK
@ 2021-06-16 3:10 ` Quan Nguyen
0 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Wolfram Sang, Jae Hyun Yoo, Guenter Roeck,
linux-i2c, openbmc, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Open Source Submission, Phong Vo, Thang Q . Nguyen
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@os.amperecomputing.com>
---
v2:
+ Split to separate series [Joel]
+ Added the Fixes line [Joel]
+ Revised commit message [Quan]
v1:
+ First introduced in https://lkml.org/lkml/2021/5/19/205
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) {
+ 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);
+ }
irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
bus->slave_state = ASPEED_I2C_SLAVE_START;
}
--
2.28.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 1/2] i2c: aspeed: Fix unhandled Tx done with NAK
@ 2021-06-16 3:10 ` Quan Nguyen
0 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Wolfram Sang, Jae Hyun Yoo, Guenter Roeck,
linux-i2c, openbmc, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Open Source Submission, Thang Q . Nguyen, Phong Vo
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@os.amperecomputing.com>
---
v2:
+ Split to separate series [Joel]
+ Added the Fixes line [Joel]
+ Revised commit message [Quan]
v1:
+ First introduced in https://lkml.org/lkml/2021/5/19/205
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) {
+ 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);
+ }
irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
bus->slave_state = ASPEED_I2C_SLAVE_START;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 1/2] i2c: aspeed: Fix unhandled Tx done with NAK
@ 2021-06-16 3:10 ` Quan Nguyen
0 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Wolfram Sang, Jae Hyun Yoo, Guenter Roeck,
linux-i2c, openbmc, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Open Source Submission, Phong Vo, Thang Q . Nguyen
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@os.amperecomputing.com>
---
v2:
+ Split to separate series [Joel]
+ Added the Fixes line [Joel]
+ Revised commit message [Quan]
v1:
+ First introduced in https://lkml.org/lkml/2021/5/19/205
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) {
+ 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);
+ }
irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
bus->slave_state = ASPEED_I2C_SLAVE_START;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] i2c: aspeed: Acknowledge Tx done with and without ACK irq late
2021-06-16 3:10 ` Quan Nguyen
(?)
(?)
@ 2021-06-16 3:10 ` Quan Nguyen
-1 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: linux-aspeed
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 is 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, resulting in "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 this issue by acknowledging
this interrupt for both ACK and NACK cases late in the interrupt handler
also.
Fixes: 2be6b47211e1 (i2c: aspeed: Acknowledge most interrupts early in interrupt handler)
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
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://lkml.org/lkml/2021/5/19/205
drivers/i2c/busses/i2c-aspeed.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 3fb37c3f23d4..0f82b46866a8 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -606,8 +606,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 */
+ 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;
@@ -652,12 +653,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);
+
spin_unlock(&bus->lock);
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 2/2] i2c: aspeed: Acknowledge Tx done with and without ACK irq late
@ 2021-06-16 3:10 ` Quan Nguyen
0 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Wolfram Sang, Jae Hyun Yoo, Guenter Roeck,
linux-i2c, openbmc, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Open Source Submission, Phong Vo, Thang Q . 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 is 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, resulting in "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 this issue by acknowledging
this interrupt for both ACK and NACK cases late in the interrupt handler
also.
Fixes: 2be6b47211e1 (i2c: aspeed: Acknowledge most interrupts early in interrupt handler)
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
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://lkml.org/lkml/2021/5/19/205
drivers/i2c/busses/i2c-aspeed.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 3fb37c3f23d4..0f82b46866a8 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -606,8 +606,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 */
+ 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;
@@ -652,12 +653,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);
+
spin_unlock(&bus->lock);
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
--
2.28.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 2/2] i2c: aspeed: Acknowledge Tx done with and without ACK irq late
@ 2021-06-16 3:10 ` Quan Nguyen
0 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Wolfram Sang, Jae Hyun Yoo, Guenter Roeck,
linux-i2c, openbmc, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Open Source Submission, Thang Q . Nguyen, Phong Vo
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 is 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, resulting in "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 this issue by acknowledging
this interrupt for both ACK and NACK cases late in the interrupt handler
also.
Fixes: 2be6b47211e1 (i2c: aspeed: Acknowledge most interrupts early in interrupt handler)
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
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://lkml.org/lkml/2021/5/19/205
drivers/i2c/busses/i2c-aspeed.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 3fb37c3f23d4..0f82b46866a8 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -606,8 +606,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 */
+ 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;
@@ -652,12 +653,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);
+
spin_unlock(&bus->lock);
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 2/2] i2c: aspeed: Acknowledge Tx done with and without ACK irq late
@ 2021-06-16 3:10 ` Quan Nguyen
0 siblings, 0 replies; 16+ messages in thread
From: Quan Nguyen @ 2021-06-16 3:10 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Wolfram Sang, Jae Hyun Yoo, Guenter Roeck,
linux-i2c, openbmc, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Open Source Submission, Phong Vo, Thang Q . 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 is 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, resulting in "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 this issue by acknowledging
this interrupt for both ACK and NACK cases late in the interrupt handler
also.
Fixes: 2be6b47211e1 (i2c: aspeed: Acknowledge most interrupts early in interrupt handler)
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
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://lkml.org/lkml/2021/5/19/205
drivers/i2c/busses/i2c-aspeed.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 3fb37c3f23d4..0f82b46866a8 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -606,8 +606,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 */
+ 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;
@@ -652,12 +653,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);
+
spin_unlock(&bus->lock);
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 0/2] i2c: aspeed: Late ack Tx done irqs and fix unhandled Tx done with NAK
2021-06-16 3:10 ` Quan Nguyen
(?)
(?)
@ 2021-11-29 19:20 ` Wolfram Sang
-1 siblings, 0 replies; 16+ messages in thread
From: Wolfram Sang @ 2021-11-29 19:20 UTC (permalink / raw)
To: linux-aspeed
On Wed, Jun 16, 2021 at 10:10:44AM +0700, Quan Nguyen wrote:
> This series consists of two patches to fix the below issues observed
> when testing with slave mode:
> + Unhandled Tx done with NAK
> + Early ack'ed of Tx done (ACK and NAK) causing "Unexpected Ack on
> read request".
>
aspeed maintainers, are you happy with this series now?
> 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 in
> https://lkml.org/lkml/2021/5/19/205
>
> Quan Nguyen (2):
> i2c: aspeed: Fix unhandled Tx done with NAK
> i2c: aspeed: Acknowledge Tx done with and without ACK irq late
>
> drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> --
> 2.28.0
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20211129/c46f05c2/attachment.sig>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/2] i2c: aspeed: Late ack Tx done irqs and fix unhandled Tx done with NAK
@ 2021-11-29 19:20 ` Wolfram Sang
0 siblings, 0 replies; 16+ messages in thread
From: Wolfram Sang @ 2021-11-29 19:20 UTC (permalink / raw)
To: Quan Nguyen
Cc: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Jae Hyun Yoo, Guenter Roeck, linux-i2c, openbmc,
linux-arm-kernel, linux-aspeed, linux-kernel,
Open Source Submission, Phong Vo, Thang Q . Nguyen
[-- Attachment #1.1: Type: text/plain, Size: 969 bytes --]
On Wed, Jun 16, 2021 at 10:10:44AM +0700, Quan Nguyen wrote:
> This series consists of two patches to fix the below issues observed
> when testing with slave mode:
> + Unhandled Tx done with NAK
> + Early ack'ed of Tx done (ACK and NAK) causing "Unexpected Ack on
> read request".
>
aspeed maintainers, are you happy with this series now?
> 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 in
> https://lkml.org/lkml/2021/5/19/205
>
> Quan Nguyen (2):
> i2c: aspeed: Fix unhandled Tx done with NAK
> i2c: aspeed: Acknowledge Tx done with and without ACK irq late
>
> drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> --
> 2.28.0
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/2] i2c: aspeed: Late ack Tx done irqs and fix unhandled Tx done with NAK
@ 2021-11-29 19:20 ` Wolfram Sang
0 siblings, 0 replies; 16+ messages in thread
From: Wolfram Sang @ 2021-11-29 19:20 UTC (permalink / raw)
To: Quan Nguyen
Cc: linux-arm-kernel, Jae Hyun Yoo, linux-aspeed, Andrew Jeffery,
openbmc, Thang Q . Nguyen, Brendan Higgins, linux-kernel,
Phong Vo, Open Source Submission, Guenter Roeck, linux-i2c
[-- Attachment #1: Type: text/plain, Size: 969 bytes --]
On Wed, Jun 16, 2021 at 10:10:44AM +0700, Quan Nguyen wrote:
> This series consists of two patches to fix the below issues observed
> when testing with slave mode:
> + Unhandled Tx done with NAK
> + Early ack'ed of Tx done (ACK and NAK) causing "Unexpected Ack on
> read request".
>
aspeed maintainers, are you happy with this series now?
> 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 in
> https://lkml.org/lkml/2021/5/19/205
>
> Quan Nguyen (2):
> i2c: aspeed: Fix unhandled Tx done with NAK
> i2c: aspeed: Acknowledge Tx done with and without ACK irq late
>
> drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> --
> 2.28.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/2] i2c: aspeed: Late ack Tx done irqs and fix unhandled Tx done with NAK
@ 2021-11-29 19:20 ` Wolfram Sang
0 siblings, 0 replies; 16+ messages in thread
From: Wolfram Sang @ 2021-11-29 19:20 UTC (permalink / raw)
To: Quan Nguyen
Cc: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Jae Hyun Yoo, Guenter Roeck, linux-i2c, openbmc,
linux-arm-kernel, linux-aspeed, linux-kernel,
Open Source Submission, Phong Vo, Thang Q . Nguyen
[-- Attachment #1: Type: text/plain, Size: 969 bytes --]
On Wed, Jun 16, 2021 at 10:10:44AM +0700, Quan Nguyen wrote:
> This series consists of two patches to fix the below issues observed
> when testing with slave mode:
> + Unhandled Tx done with NAK
> + Early ack'ed of Tx done (ACK and NAK) causing "Unexpected Ack on
> read request".
>
aspeed maintainers, are you happy with this series now?
> 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 in
> https://lkml.org/lkml/2021/5/19/205
>
> Quan Nguyen (2):
> i2c: aspeed: Fix unhandled Tx done with NAK
> i2c: aspeed: Acknowledge Tx done with and without ACK irq late
>
> drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> --
> 2.28.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread