diff mbox series

[v7,3/3] i2c: aspeed: Assert NAK when slave is busy

Message ID 20220422040803.2524940-4-quan@os.amperecomputing.com (mailing list archive)
State New, archived
Headers show
Series Add SSIF BMC driver | expand

Commit Message

Quan Nguyen April 22, 2022, 4:08 a.m. UTC
When processing I2C_SLAVE_WRITE_REQUESTED event, if slave returns
-EBUSY, i2c controller should issue RxCmdLast command to assert NAK
on the bus.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v7:
  + None

v6:
  + New introduced in v6                      [Quan]

 drivers/i2c/busses/i2c-aspeed.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Quan Nguyen April 22, 2022, 4:17 a.m. UTC | #1
Added Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
as I'm not aware of the email change
- Quan


On 22/04/2022 11:08, Quan Nguyen wrote:
> When processing I2C_SLAVE_WRITE_REQUESTED event, if slave returns
> -EBUSY, i2c controller should issue RxCmdLast command to assert NAK
> on the bus.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v7:
>    + None
> 
> v6:
>    + New introduced in v6                      [Quan]
> 
>   drivers/i2c/busses/i2c-aspeed.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 771e53d3d197..ebc2b92656c8 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -244,6 +244,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>   	u32 command, irq_handled = 0;
>   	struct i2c_client *slave = bus->slave;
>   	u8 value;
> +	int ret;
>   
>   	if (!slave)
>   		return 0;
> @@ -311,7 +312,9 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>   		break;
>   	case ASPEED_I2C_SLAVE_WRITE_REQUESTED:
>   		bus->slave_state = ASPEED_I2C_SLAVE_WRITE_RECEIVED;
> -		i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> +		ret = i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> +		if (ret == -EBUSY)
> +			writel(ASPEED_I2CD_M_S_RX_CMD_LAST, bus->base + ASPEED_I2C_CMD_REG);
>   		break;
>   	case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
>   		i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
Wolfram Sang May 14, 2022, 2:31 p.m. UTC | #2
On Fri, Apr 22, 2022 at 11:08:03AM +0700, Quan Nguyen wrote:
> When processing I2C_SLAVE_WRITE_REQUESTED event, if slave returns
> -EBUSY, i2c controller should issue RxCmdLast command to assert NAK
> on the bus.

That should be I2C_SLAVE_WRITE_RECEIVED and it should be NAKed on all
errnos. Have you tested it?
Quan Nguyen May 16, 2022, 2:32 a.m. UTC | #3
On 14/05/2022 21:31, Wolfram Sang wrote:
> On Fri, Apr 22, 2022 at 11:08:03AM +0700, Quan Nguyen wrote:
>> When processing I2C_SLAVE_WRITE_REQUESTED event, if slave returns
>> -EBUSY, i2c controller should issue RxCmdLast command to assert NAK
>> on the bus.
> 
> That should be I2C_SLAVE_WRITE_RECEIVED and it should be NAKed on all
> errnos. Have you tested it?
> 

Dear Wolfram,

Thanks for the comment.

Yes, we have tested this patch with ast2500 and see it works well 
without the need of the ugly slave_enable/disable() as before.

When tested with ast2500, it is observed that there's always a 
I2C_SLAVE_WRITE_REQUESTED comes first then other 
I2C_SLAVE_WRITE_RECEIVED's follow for all transactions.

In case slave is busy, the NAK will be asserted on the first occurrence 
of I2C_SLAVE_WRITE_REQUESTED make host to stop the current transaction 
(host later will retry with other transaction) until slave ready.

This behavior is expected as we want host to drop all transactions while 
slave is busy on working on the response. That is why we choose to 
assert NAK on the first I2C_SLAVE_WRITE_REQUESTED of the transaction 
instead of I2C_SLAVE_WRITE_RECEIVED.

As we are interested in this specific case, ie: to assert NAK only when 
slave busy, we dont want to force the current aspeed's slave to assert 
NAK in all errno's. That is why we choose to NAK only when there is an 
explicitly -EBUSY return from slave.

Thank you for the review and hope to see further comments.
Thanks,
- Quan
Wolfram Sang June 15, 2022, 8:32 p.m. UTC | #4
Hi Quan,

> When tested with ast2500, it is observed that there's always a
> I2C_SLAVE_WRITE_REQUESTED comes first then other I2C_SLAVE_WRITE_RECEIVED's
> follow for all transactions.

Yes, that's the design of the interface :)

> In case slave is busy, the NAK will be asserted on the first occurrence of
> I2C_SLAVE_WRITE_REQUESTED make host to stop the current transaction (host
> later will retry with other transaction) until slave ready.
> 
> This behavior is expected as we want host to drop all transactions while
> slave is busy on working on the response. That is why we choose to assert
> NAK on the first I2C_SLAVE_WRITE_REQUESTED of the transaction instead of
> I2C_SLAVE_WRITE_RECEIVED.

From Documentation/i2c/slave-interface.rst:

===

About ACK/NACK
--------------

It is good behaviour to always ACK the address phase, so the master knows if a
device is basically present or if it mysteriously disappeared. Using NACK to
state being busy is troublesome. SMBus demands to always ACK the address phase,
while the I2C specification is more loose on that. Most I2C controllers also
automatically ACK when detecting their slave addresses, so there is no option
to NACK them. For those reasons, this API does not support NACK in the address
phase.

===

So, the proper design is to NACK on the first received byte. All EEPROMs
do it this way when they are busy because of erasing a page.

All the best,

   Wolfram
Quan Nguyen June 16, 2022, 7:16 a.m. UTC | #5
On 16/06/2022 03:32, Wolfram Sang wrote:
> Hi Quan,
> 
>> When tested with ast2500, it is observed that there's always a
>> I2C_SLAVE_WRITE_REQUESTED comes first then other I2C_SLAVE_WRITE_RECEIVED's
>> follow for all transactions.
> 
> Yes, that's the design of the interface :)
> 
>> In case slave is busy, the NAK will be asserted on the first occurrence of
>> I2C_SLAVE_WRITE_REQUESTED make host to stop the current transaction (host
>> later will retry with other transaction) until slave ready.
>>
>> This behavior is expected as we want host to drop all transactions while
>> slave is busy on working on the response. That is why we choose to assert
>> NAK on the first I2C_SLAVE_WRITE_REQUESTED of the transaction instead of
>> I2C_SLAVE_WRITE_RECEIVED.
> 
>  From Documentation/i2c/slave-interface.rst:
> 
> ===
> 
> About ACK/NACK
> --------------
> 
> It is good behaviour to always ACK the address phase, so the master knows if a
> device is basically present or if it mysteriously disappeared. Using NACK to
> state being busy is troublesome. SMBus demands to always ACK the address phase,
> while the I2C specification is more loose on that. Most I2C controllers also
> automatically ACK when detecting their slave addresses, so there is no option
> to NACK them. For those reasons, this API does not support NACK in the address
> phase.
> 
> ===
> 
> So, the proper design is to NACK on the first received byte. All EEPROMs
> do it this way when they are busy because of erasing a page.
> 

Thanks Wolfram for the review.

On the first occurrence of I2C_SLAVE_WRITE_REQUESTED, the address is 
already received with ACK. So if slave return -EBUSY, the NAK will occur 
on the next Rx byte (on I2C_SLAVE_WRITE_RECEIVED event).

Tested this patch and capture using Saleae tool, it always shows ACK on 
the address and NAK on the first byte follow when slave return -EBUSY, 
ie: the byte follow the address, which is single part read command 
(0x03) in my case.

+ When slave return -EBUSY:
   S-> Aw(ACK)-> RxD(NAK)-> P
       0x10      0x03 (Singlepart read)

+ When slave ready:
   S-> Aw(ACK)-> RxD(ACK)-> Sr-> Ar-> TxD(ACK)-> ... -> TxD(NAK)-> P
       0x10      0x03                 0x07       ...    0xDE

Using the Logic 2 (with Saleae tool) to capture, we could see the log as 
below:

write to 0x10 ack data: 0x03    <= when slave return -EBUSY
write to 0x10 ack data: 0x03    <= when slave return -EBUSY
write to 0x10 ack data: 0x03    <= when slave return -EBUSY
...
write to 0x10 ack data: 0x03    <= when slave return -EBUSY
write to 0x10 ack data: 0x03    <= when slave is ready
read to 0x10 ack data: 0x07 0xF4 0x1D 0x00 0x01 0x00 0x00 0x00 0xDE

Thanks,
- Quan
Wolfram Sang June 16, 2022, 12:29 p.m. UTC | #6
Hi Quan,

> On the first occurrence of I2C_SLAVE_WRITE_REQUESTED, the address is already
> received with ACK. So if slave return -EBUSY, the NAK will occur on the next
> Rx byte (on I2C_SLAVE_WRITE_RECEIVED event).

This is exactly why I2C_SLAVE_WRITE_RECEIVED allows for an error code.
From the docs:

===

* I2C_SLAVE_WRITE_RECEIVED (mandatory)

  'val': bus driver delivers received byte

  'ret': 0 if the byte should be acked, some errno if the byte should be nacked

Another I2C master has sent a byte to us which needs to be set in 'val'. If 'ret'
is zero, the bus driver should ack this byte. If 'ret' is an errno, then the byte
should be nacked.

===

'ret' is used to ACK/NACK the current byte in 'val'. That's exactly what
you need, or? Does the aspeed driver not support acking the current
byte?
Quan Nguyen June 17, 2022, 7:08 a.m. UTC | #7
On 16/06/2022 19:29, Wolfram Sang wrote:
> Hi Quan,
> 
>> On the first occurrence of I2C_SLAVE_WRITE_REQUESTED, the address is already
>> received with ACK. So if slave return -EBUSY, the NAK will occur on the next
>> Rx byte (on I2C_SLAVE_WRITE_RECEIVED event).
> 
> This is exactly why I2C_SLAVE_WRITE_RECEIVED allows for an error code.
>  From the docs:
> 
> ===
> 
> * I2C_SLAVE_WRITE_RECEIVED (mandatory)
> 
>    'val': bus driver delivers received byte
> 
>    'ret': 0 if the byte should be acked, some errno if the byte should be nacked
> 
> Another I2C master has sent a byte to us which needs to be set in 'val'. If 'ret'
> is zero, the bus driver should ack this byte. If 'ret' is an errno, then the byte
> should be nacked.
> 
> ===
> 
> 'ret' is used to ACK/NACK the current byte in 'val'. That's exactly what
> you need, or? Does the aspeed driver not support acking the current
> byte?
>

It is true that aspeed driver does not support acking the current byte. 
Setting ASPEED_I2CD_M_S_RX_CMD_LAST will take effect on the next Rx byte 
as per my observation.

S-> Aw(ACK)-> RxD(ACK)-> Sr-> Ar-> TxD(ACK)-> ... -> TxD(NAK)-> P
      (1)        (2)

Currently, setting ASPEED_I2CD_M_S_RX_CMD_LAST in (1), on 
I2C_SLAVE_WRITE_REQUESTED event, will make the NAK happen in (2) and 
make the read stop.

If setting ASPEED_I2CD_M_S_RX_CMD_LAST on (2), ie: on 
I2C_SLAVE_WRITE_RECEIVED event, the read from Master is never NAK 
because there is no next Rx byte and Master is already switch to read 
from Slave.

I understands that the return of
i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value) is always 0 as 
in Documentation/i2c/slave-interface.rst. But with this case, this is 
the way to NAK on the first byte and I'm wonder if this particular case 
would be supported somehow.

Thanks,
-- Quan
Quan Nguyen July 5, 2022, 2:45 a.m. UTC | #8
On 17/06/2022 14:08, Quan Nguyen wrote:
> On 16/06/2022 19:29, Wolfram Sang wrote:
>> Hi Quan,
>>
>>> On the first occurrence of I2C_SLAVE_WRITE_REQUESTED, the address is 
>>> already
>>> received with ACK. So if slave return -EBUSY, the NAK will occur on 
>>> the next
>>> Rx byte (on I2C_SLAVE_WRITE_RECEIVED event).
>>
>> This is exactly why I2C_SLAVE_WRITE_RECEIVED allows for an error code.
>>  From the docs:
>>
>> ===
>>
>> * I2C_SLAVE_WRITE_RECEIVED (mandatory)
>>
>>    'val': bus driver delivers received byte
>>
>>    'ret': 0 if the byte should be acked, some errno if the byte should 
>> be nacked
>>
>> Another I2C master has sent a byte to us which needs to be set in 
>> 'val'. If 'ret'
>> is zero, the bus driver should ack this byte. If 'ret' is an errno, 
>> then the byte
>> should be nacked.
>>
>> ===
>>
>> 'ret' is used to ACK/NACK the current byte in 'val'. That's exactly what
>> you need, or? Does the aspeed driver not support acking the current
>> byte?
>>
> 
> It is true that aspeed driver does not support acking the current byte. 
> Setting ASPEED_I2CD_M_S_RX_CMD_LAST will take effect on the next Rx byte 
> as per my observation.
> 
> S-> Aw(ACK)-> RxD(ACK)-> Sr-> Ar-> TxD(ACK)-> ... -> TxD(NAK)-> P
>       (1)        (2)
> 
> Currently, setting ASPEED_I2CD_M_S_RX_CMD_LAST in (1), on 
> I2C_SLAVE_WRITE_REQUESTED event, will make the NAK happen in (2) and 
> make the read stop.
> 
> If setting ASPEED_I2CD_M_S_RX_CMD_LAST on (2), ie: on 
> I2C_SLAVE_WRITE_RECEIVED event, the read from Master is never NAK 
> because there is no next Rx byte and Master is already switch to read 
> from Slave.
> 
> I understands that the return of
> i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value) is always 0 as 
> in Documentation/i2c/slave-interface.rst. But with this case, this is 
> the way to NAK on the first byte and I'm wonder if this particular case 
> would be supported somehow.
> 

Dear Wolfram,

 From my particular case, as it seems not to be able to nak on the 
current byte on I2C_SLAVE_WRITE_RECEIVED event (i2c-aspeed), is it 
possible to somehow allow slave to return some errno on the 
I2C_SLAVE_WRITE_REQUESTED event so that bus driver knows what to do for 
the next incoming byte if slave is busy?

As from the docs:

===

* I2C_SLAVE_WRITE_REQUESTED (mandatory)

   'val': unused

   'ret': always 0

Another I2C master wants to write data to us. This event should be sent once
our own address and the write bit was detected. The data did not arrive 
yet, so
there is nothing to process or return. Wakeup or initialization probably 
needs
to be done, though.

===

As the "Wakeup or initialization probably needs to be done" in the slave 
side, in case slave is fail to either wake up or initialization or busy 
in this particular case, there is no way for slave to let the bus driver 
know the status if the 'ret' is always 0.

I'm also agree that it should be always ack on I2C_SLAVE_WRITE_REQUESTED 
event (when the slave address matched and the write bit detected). But 
if slave know it is fail to either wakeup or initialization or busy, the 
bus driver should be able to automatically to nak on the first write 
incoming byte.

Hence, my thinking is to change the above:

   'ret': always 0

to:

   'ret': 0 otherwise some errno if slave is busy or fail to wakeup or 
initialization

Thank you and best regards,
- Quan
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 771e53d3d197..ebc2b92656c8 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -244,6 +244,7 @@  static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 	u32 command, irq_handled = 0;
 	struct i2c_client *slave = bus->slave;
 	u8 value;
+	int ret;
 
 	if (!slave)
 		return 0;
@@ -311,7 +312,9 @@  static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 		break;
 	case ASPEED_I2C_SLAVE_WRITE_REQUESTED:
 		bus->slave_state = ASPEED_I2C_SLAVE_WRITE_RECEIVED;
-		i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+		ret = i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+		if (ret == -EBUSY)
+			writel(ASPEED_I2CD_M_S_RX_CMD_LAST, bus->base + ASPEED_I2C_CMD_REG);
 		break;
 	case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
 		i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);