It is observed that in normal condition, when the last byte sent by
slave, the Tx Done with NAK irq will raise.
But it is also observed that sometimes master issues next transaction
too quick while the slave irq handler is not yet invoked and Tx Done
with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
This Tx Done with NAK irq is raised together with the Slave Match and
Rx Done irq of the next coming transaction from master.
Unfortunately, the current slave irq handler handles the Slave Match and
Rx Done only in higher priority and ignore the Tx Done with NAK, causing
the complain as below:
"aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
0x00000086, but was 0x00000084"
This commit handles this case by emitting a Slave Stop event for the
Tx Done with NAK before processing Slave Match and Rx Done for the
coming transaction from master.
Signed-off-by: Quan Nguyen <[email protected]>
---
v3:
+ First introduce in v3 [Quan]
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
Ryan, can you please review this change?
On Wed, 19 May 2021 at 07:50, Quan Nguyen <[email protected]> wrote:
>
> It is observed that in normal condition, when the last byte sent by
> slave, the Tx Done with NAK irq will raise.
> But it is also observed that sometimes master issues next transaction
> too quick while the slave irq handler is not yet invoked and Tx Done
> with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
> This Tx Done with NAK irq is raised together with the Slave Match and
> Rx Done irq of the next coming transaction from master.
> Unfortunately, the current slave irq handler handles the Slave Match and
> Rx Done only in higher priority and ignore the Tx Done with NAK, causing
> the complain as below:
> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
> 0x00000086, but was 0x00000084"
>
> This commit handles this case by emitting a Slave Stop event for the
> Tx Done with NAK before processing Slave Match and Rx Done for the
> coming transaction from master.
It sounds like this patch is independent of the rest of the series,
and can go in on it's own. Please send it separately to the i2c
maintainers and add a suitable Fixes line, such as:
Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
>
> Signed-off-by: Quan Nguyen <[email protected]>
> ---
> v3:
> + First introduce in v3 [Quan]
>
> 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) {
Can you explain why you need to do this handing inside the SLAVE_MATCH case?
Could you instead move the TX_NAK handling to be above the SLAVE_MATCH case?
> + if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
> + bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
Either way, this needs a comment to explain what we're working around.
> + 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
>
> -----Original Message-----
> From: Joel Stanley <[email protected]>
> Sent: Thursday, May 20, 2021 7:29 AM
> To: Quan Nguyen <[email protected]>; Ryan Chen
> <[email protected]>
> Cc: Corey Minyard <[email protected]>; Rob Herring <[email protected]>;
> Andrew Jeffery <[email protected]>; Brendan Higgins
> <[email protected]>; Benjamin Herrenschmidt
> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
> <[email protected]>; [email protected];
> devicetree <[email protected]>; Linux ARM
> <[email protected]>; linux-aspeed
> <[email protected]>; Linux Kernel Mailing List
> <[email protected]>; [email protected]; Open Source
> Submission <[email protected]>; Phong Vo
> <[email protected]>; Thang Q . Nguyen
> <[email protected]>; OpenBMC Maillist
> <[email protected]>
> Subject: Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK
>
> Ryan, can you please review this change?
>
> On Wed, 19 May 2021 at 07:50, Quan Nguyen
> <[email protected]> wrote:
> >
> > It is observed that in normal condition, when the last byte sent by
> > slave, the Tx Done with NAK irq will raise.
> > But it is also observed that sometimes master issues next transaction
> > too quick while the slave irq handler is not yet invoked and Tx Done
> > with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
> > This Tx Done with NAK irq is raised together with the Slave Match and
> > Rx Done irq of the next coming transaction from master.
> > Unfortunately, the current slave irq handler handles the Slave Match
> > and Rx Done only in higher priority and ignore the Tx Done with NAK,
> > causing the complain as below:
> > "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
> > 0x00000086, but was 0x00000084"
> >
> > This commit handles this case by emitting a Slave Stop event for the
> > Tx Done with NAK before processing Slave Match and Rx Done for the
> > coming transaction from master.
>
> It sounds like this patch is independent of the rest of the series, and can go in
> on it's own. Please send it separately to the i2c maintainers and add a suitable
> Fixes line, such as:
>
> Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C
> driver")
>
> >
> > Signed-off-by: Quan Nguyen <[email protected]>
> > ---
> > v3:
> > + First introduce in v3 [Quan]
> >
> > 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) {
>
> Can you explain why you need to do this handing inside the SLAVE_MATCH
> case?
>
> Could you instead move the TX_NAK handling to be above the SLAVE_MATCH
> case?
>
> > + if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
> > + bus->slave_state ==
> > + ASPEED_I2C_SLAVE_READ_PROCESSED) {
>
> Either way, this needs a comment to explain what we're working around.
>
> > + irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
> > + i2c_slave_event(slave, I2C_SLAVE_STOP,
> &value);
According the patch assume slave receive TX_NAK will be go to SLAVE_STOP state?
> > + }
> > irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> > bus->slave_state = ASPEED_I2C_SLAVE_START;
> > }
> > --
> > 2.28.0
> >
On 20/05/2021 06:28, Joel Stanley wrote:
> Ryan, can you please review this change?
>
> On Wed, 19 May 2021 at 07:50, Quan Nguyen <[email protected]> wrote:
>>
>> It is observed that in normal condition, when the last byte sent by
>> slave, the Tx Done with NAK irq will raise.
>> But it is also observed that sometimes master issues next transaction
>> too quick while the slave irq handler is not yet invoked and Tx Done
>> with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
>> This Tx Done with NAK irq is raised together with the Slave Match and
>> Rx Done irq of the next coming transaction from master.
>> Unfortunately, the current slave irq handler handles the Slave Match and
>> Rx Done only in higher priority and ignore the Tx Done with NAK, causing
>> the complain as below:
>> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
>> 0x00000086, but was 0x00000084"
>>
>> This commit handles this case by emitting a Slave Stop event for the
>> Tx Done with NAK before processing Slave Match and Rx Done for the
>> coming transaction from master.
>
> It sounds like this patch is independent of the rest of the series,
> and can go in on it's own. Please send it separately to the i2c
> maintainers and add a suitable Fixes line, such as:
>
> Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
>
Will separate this patch into independent series in next version.
>>
>> Signed-off-by: Quan Nguyen <[email protected]>
>> ---
>> v3:
>> + First introduce in v3 [Quan]
>>
>> 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) {
>
> Can you explain why you need to do this handing inside the SLAVE_MATCH case?
> Could you instead move the TX_NAK handling to be above the SLAVE_MATCH case?
>
>> + if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
>> + bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
>
> Either way, this needs a comment to explain what we're working around.
>
Let me explain with the two examples below in normal case and the case
where this patch is for:
In normal case:
The first transaction is Slave send (Master read):
20(addr) 03(singlepart read) 03 1c 2e d5
Then the second Master write follow as below:
20(addr) 02(singlepart write) 02 18 08 59
The irq will raise in sequence below:
irq data from-state to-state
00000084 20 INACTIVE WRITE_RECEIVED
00000004 03 WRITE_RECEIVED WRITE_RECEIVED <= RX_DONE
00000084 03 WRITE_RECEIVED READ_PROCESSED
00000001 1c READ_PROCESSED READ_PROCESSED <= TX_ACK
00000001 2e READ_PROCESSED READ_PROCESSED
00000001 d5 READ_PROCESSED READ_PROCESSED
00000002 xx READ_PROCESSED INACTIVE <= TX_NAK
00000084 20 INACTIVE WRITE_RECEIVED <= SLAVE_MATCH & RX_DONE
00000004 02 WRITE_RECEIVED WRITE_RECEIVED
00000084 02 WRITE_RECEIVED WRITE_RECEIVED
00000004 18 WRITE_RECEIVED WRITE_RECEIVED
00000004 08 WRITE_RECEIVED WRITE_RECEIVED
00000004 59 WRITE_RECEIVED WRITE_RECEIVED
00000010 xx WRITE_RECEIVED INACTIVE
But sometimes:
The first transaction is Slave send (Master read):
20(addr) 03(singlepart read) 03 1c 42 cc a5
Then the second Master write follow as below:
20(addr) 02(singlepart write) 03 18 42 0c 63
The irq will raise in sequence below:
irq data from-state to-state
00000084 20 INACTIVE WRITE_RECEIVED
00000004 03 WRITE_RECEIVED WRITE_RECEIVED
00000084 03 WRITE_RECEIVED READ_PROCESSED
00000001 1c READ_PROCESSED READ_PROCESSED
00000001 42 READ_PROCESSED READ_PROCESSED
00000001 0c READ_PROCESSED READ_PROCESSED
00000001 63 READ_PROCESSED READ_PROCESSED
00000086 20 READ_PROCESSED WRITE_RECEIVED <= both 3 irqs raised
00000004 02 WRITE_RECEIVED WRITE_RECEIVED
00000084 03 WRITE_RECEIVED WRITE_RECEIVED
00000004 18 WRITE_RECEIVED WRITE_RECEIVED
00000004 42 WRITE_RECEIVED WRITE_RECEIVED
00000004 0c WRITE_RECEIVED WRITE_RECEIVED
00000004 63 WRITE_RECEIVED WRITE_RECEIVED
00000010 xx WRITE_RECEIVED INACTIVE
This patch is to address this case where TX_NAK, SLAVE_MATCH and RX_DONE
are raised together.
>> + 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
>>
On 20/05/2021 18:28, Ryan Chen wrote:
>> -----Original Message-----
>> From: Joel Stanley <[email protected]>
>> Sent: Thursday, May 20, 2021 7:29 AM
>> To: Quan Nguyen <[email protected]>; Ryan Chen
>> <[email protected]>
>> Cc: Corey Minyard <[email protected]>; Rob Herring <[email protected]>;
>> Andrew Jeffery <[email protected]>; Brendan Higgins
>> <[email protected]>; Benjamin Herrenschmidt
>> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
>> <[email protected]>; [email protected];
>> devicetree <[email protected]>; Linux ARM
>> <[email protected]>; linux-aspeed
>> <[email protected]>; Linux Kernel Mailing List
>> <[email protected]>; [email protected]; Open Source
>> Submission <[email protected]>; Phong Vo
>> <[email protected]>; Thang Q . Nguyen
>> <[email protected]>; OpenBMC Maillist
>> <[email protected]>
>> Subject: Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK
>>
>> Ryan, can you please review this change?
>>
>> On Wed, 19 May 2021 at 07:50, Quan Nguyen
>> <[email protected]> wrote:
>>>
>>> It is observed that in normal condition, when the last byte sent by
>>> slave, the Tx Done with NAK irq will raise.
>>> But it is also observed that sometimes master issues next transaction
>>> too quick while the slave irq handler is not yet invoked and Tx Done
>>> with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
>>> This Tx Done with NAK irq is raised together with the Slave Match and
>>> Rx Done irq of the next coming transaction from master.
>>> Unfortunately, the current slave irq handler handles the Slave Match
>>> and Rx Done only in higher priority and ignore the Tx Done with NAK,
>>> causing the complain as below:
>>> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
>>> 0x00000086, but was 0x00000084"
>>>
>>> This commit handles this case by emitting a Slave Stop event for the
>>> Tx Done with NAK before processing Slave Match and Rx Done for the
>>> coming transaction from master.
>>
>> It sounds like this patch is independent of the rest of the series, and can go in
>> on it's own. Please send it separately to the i2c maintainers and add a suitable
>> Fixes line, such as:
>>
>> Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C
>> driver")
>>
>>>
>>> Signed-off-by: Quan Nguyen <[email protected]>
>>> ---
>>> v3:
>>> + First introduce in v3 [Quan]
>>>
>>> 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) {
>>
>> Can you explain why you need to do this handing inside the SLAVE_MATCH
>> case?
>>
>> Could you instead move the TX_NAK handling to be above the SLAVE_MATCH
>> case?
>>
>>> + if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
>>> + bus->slave_state ==
>>> + ASPEED_I2C_SLAVE_READ_PROCESSED) {
>>
>> Either way, this needs a comment to explain what we're working around.
>>
>>> + irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>>> + i2c_slave_event(slave, I2C_SLAVE_STOP,
>> &value);
>
> According the patch assume slave receive TX_NAK will be go to SLAVE_STOP state?
>
Hi Ryan,
As per my explain in other email, we need to emit one SLAVE_STOP event
to complete the previous transaction before start processing for the
next transaction.
- Quan
>>> + }
>>> irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>>> bus->slave_state = ASPEED_I2C_SLAVE_START;
>>> }
>>> --
>>> 2.28.0
>>>