| 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-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
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 Cosmo
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/ - Quan
Hi Cosmo, On Mon, Nov 20, 2023 at 05:17:46PM +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. > > 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(-) > > 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); which branch are you? You don't look like being in the latest. Please update your branch. Andi > readl(bus->base + ASPEED_I2C_INTR_STS_REG); > irq_received &= ASPEED_I2CD_INTR_RECV_MASK; > irq_remaining = irq_received; > -- > 2.34.1 >
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/ From a brief inspection I prefer the descriptions in your series Quan. Looks like we dropped the ball a bit there though on the review - can you resend your series based on 6.7-rc1 or so and Cc Cosmo? Cheers, Andrew
On 28/11/2023 06:00, Andrew Jeffery wrote: > 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/ > > From a brief inspection I prefer the descriptions in your series Quan. > Looks like we dropped the ball a bit there though on the review - can > you resend your series based on 6.7-rc1 or so and Cc Cosmo? > Yes, sure, I'll rebase on v6.7 and resend the series shortly. Thanks, - Quan
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(-)