[PATCH] i2c: aspeed: Acknowledge Tx ack late when in SLAVE_READ_PROCESSED

[PATCH] i2c: aspeed: Acknowledge Tx ack late when in SLAVE_READ_PROCESSED

[Cosmo Chou]

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);
 	readl(bus->base + ASPEED_I2C_INTR_STS_REG);
 	irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
 	irq_remaining = irq_received;
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: aspeed: Acknowledge Tx ack late when in SLAVE_READ_PROCESSED

[Andrew Jeffery]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: aspeed: Acknowledge Tx ack late when in SLAVE_READ_PROCESSED

[Cosmo Chou]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: aspeed: Acknowledge Tx ack late when in SLAVE_READ_PROCESSED

[Quan Nguyen]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: aspeed: Acknowledge Tx ack late when in SLAVE_READ_PROCESSED

[Andrew Jeffery]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: aspeed: Acknowledge Tx ack late when in SLAVE_READ_PROCESSED

[Quan Nguyen]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: aspeed: Acknowledge Tx ack late when in SLAVE_READ_PROCESSED

[Andi Shyti]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel