| Message ID | 20231120091746.2866232-1-chou.cosmo@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: aspeed: Acknowledge Tx ack late when in SLAVE_READ_PROCESSED | expand |
On Mon, 2023-11-27 at 15:08 +0700, Quan Nguyen wrote: > > On 27/11/2023 14:04, Cosmo Chou wrote: > > Andrew Jeffery <andrew@codeconstruct.com.au> wrote on Mon, 2023-11-27 > > at 11:23 AM: > > > > > > On Mon, 2023-11-20 at 17:17 +0800, Cosmo Chou wrote: > > > > commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early > > > > in interrupt handler") moved most interrupt acknowledgments to the > > > > start of the interrupt handler to avoid race conditions. However, > > > > slave Tx ack status shouldn't be cleared before SLAVE_READ_PROCESSED > > > > is handled. > > > > > > > > Acknowledge Tx ack status after handling SLAVE_READ_PROCESSED to fix > > > > the problem that the next byte is not sent correctly. > > > > > > What does this mean in practice? Can you provide more details? It > > > sounds like you've seen concrete problems and it would be nice to > > > capture what it was that occurred. > > > > > > Andrew > > > > For a normal slave transaction, a master attempts to read out N bytes > > from BMC: (BMC addr: 0x20) > > [S] [21] [A] [1st_B] [1_ack] [2nd_B] [2_ack] ... [Nth_B] [N] [P] > > > > T1: when [21] [A]: Both INTR_SLAVE_MATCH and INTR_RX_DONE rise, > > INTR_RX_DONE is not cleared until BMC is ready to send the 1st_B: > > https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-aspeed.c#L294 > > That is, BMC stretches the SCL until ready to send the 1st_B. > > > > T2: when [1_ack]: INTR_TX_ACK rises, but it's cleared at the start of > > the ISR, so that BMC does not stretch the SCL, the master continues > > to read 2nd_B before BMC is ready to send the 2nd_B. > > > > To fix this, do not clear INTR_TX_ACK until BMC is ready to send data: > > https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-aspeed.c#L302 > > > > This looks like the same issue, but we chose to ack them late. Same with > INTR_RX_DONE. > > https://lore.kernel.org/all/20210616031046.2317-3-quan@os.amperecomputing.com/
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 28e2a5fc4528..c2d74e4b7e50 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -337,6 +337,12 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) break; } + /* Ack Tx ack */ + if (irq_handled & ASPEED_I2CD_INTR_TX_ACK) { + writel(ASPEED_I2CD_INTR_TX_ACK, bus->base + ASPEED_I2C_INTR_STS_REG); + readl(bus->base + ASPEED_I2C_INTR_STS_REG); + } + return irq_handled; } #endif /* CONFIG_I2C_SLAVE */ @@ -602,13 +608,18 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) { struct aspeed_i2c_bus *bus = dev_id; - u32 irq_received, irq_remaining, irq_handled; + u32 irq_received, irq_remaining, irq_handled, irq_acked; 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, - bus->base + ASPEED_I2C_INTR_STS_REG); + irq_acked = irq_received & ~ASPEED_I2CD_INTR_RX_DONE; +#if IS_ENABLED(CONFIG_I2C_SLAVE) + /* shouldn't ack Slave Tx Ack before it's handled */ + if (bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) + irq_acked &= ~ASPEED_I2CD_INTR_TX_ACK; +#endif + writel(irq_acked, bus->base + ASPEED_I2C_INTR_STS_REG); readl(bus->base + ASPEED_I2C_INTR_STS_REG); irq_received &= ASPEED_I2CD_INTR_RECV_MASK; irq_remaining = irq_received;
commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler") moved most interrupt acknowledgments to the start of the interrupt handler to avoid race conditions. However, slave Tx ack status shouldn't be cleared before SLAVE_READ_PROCESSED is handled. Acknowledge Tx ack status after handling SLAVE_READ_PROCESSED to fix the problem that the next byte is not sent correctly. Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler") Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com> --- drivers/i2c/busses/i2c-aspeed.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)