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@xxxxxxxxxx>
Thanks, Andi for the comments.
I will send out v4 to address those.
- Quan
