I2C slave mode for i2c-at91 driver

I2C slave mode for i2c-at91 driver

[Juergen Fitschen]

Hello,


Currently I'm working on a driver adding I2C slave support to some 
Atmel / Microchip MPUs. I already patched the i2c-at91.c and made the 
i2c-slave-eeprom backend working on an ATSAMA5D27. But this is still in 
the prototype status. My next steps are cleaning up the code and sharing 
the patch with you for review and merge into Linux.

But before I do so, I would like to hear your thoughts on some design 
decisions I'm going to make to fulfill the Linux I2C slave interface 
description [1] as good as possible:

1) According to [1] the return value of the I2C_SLAVE_WRITE_RECEIVED 
event determines whether the received byte shall be ACKed or NACKed. The 
problem with the Atmel hardware is that it is not possible to manipulate 
the ACK bit of the current byte in flight; it will be ACKed 
automatically. It is only possible to (N)ACK the following byte(s) since 
some FIFO magic is going on inside the hardware.
Do you think it is a valid approach to ignore the return value and 
always ACK received bytes? Or would you rather set the behaviour for the 
following bytes? That would delay the desired ACK bit by at least one 
byte.

2) The Atmel hardware does not support master and slave concurrently. 
The data sheet [2] examples disable master mode before entering the 
slave mode. (cf. p. 1362) Furthermore send and receive registers and 
their FIFOs are shared.
How would you implement blocking master transactions while slave mode 
is enabled? I would return EBUSY if master_xfer is called.


Best regards,
  Juergen Fitschen


[1] https://www.kernel.org/doc/Documentation/i2c/slave-interface
[2] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf

Re: I2C slave mode for i2c-at91 driver

[Uwe Kleine-König]

Hello,

On Tue, Oct 17, 2017 at 12:56:24PM +0200, Juergen Fitschen wrote:
> Currently I'm working on a driver adding I2C slave support to some Atmel /
> Microchip MPUs. I already patched the i2c-at91.c and made the
> i2c-slave-eeprom backend working on an ATSAMA5D27. But this is still in the
> prototype status. My next steps are cleaning up the code and sharing the
> patch with you for review and merge into Linux.

<halfknowledge> Isn't the at91 i2c hardware broken or unreliable such
that it's usually recommended to stick to i2c-gpio? If so maybe the same
applies to client mode and someone should better extend i2c-gpio to
handle the slave API?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: I2C slave mode for i2c-at91 driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]


> <halfknowledge> Isn't the at91 i2c hardware broken or unreliable such
> that it's usually recommended to stick to i2c-gpio? If so maybe the same

I thought later versions have been fixed? (half knowledge, too)

> applies to client mode and someone should better extend i2c-gpio to
> handle the slave API?

I wouldn't like that much. It means polling the GPIOs to scan the I2C
bus for the communication of the master. That is timing-critical and
does not like latency. Might even depend on the RT patchset ;)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: I2C slave mode for i2c-at91 driver

[Uwe Kleine-König]

On Wed, Oct 18, 2017 at 09:43:07AM +0200, Wolfram Sang wrote:
> 
> > <halfknowledge> Isn't the at91 i2c hardware broken or unreliable such
> > that it's usually recommended to stick to i2c-gpio? If so maybe the same
> 
> I thought later versions have been fixed? (half knowledge, too)
> 
> > applies to client mode and someone should better extend i2c-gpio to
> > handle the slave API?
> 
> I wouldn't like that much. It means polling the GPIOs to scan the I2C
> bus for the communication of the master.

GPIOs changing its level can trigger an irq, so the hardware does the
job of polling for you.  This doesn't necessarily makes this a good
idea, but it's not as bad as it could be.

> That is timing-critical and does not like latency. Might even depend
> on the RT patchset ;)

If the bus is operated slow enough it could work.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: I2C slave mode for i2c-at91 driver

[Juergen Fitschen]

Hi Uwe,

> <halfknowledge> Isn't the at91 i2c hardware broken or unreliable such
> that it's usually recommended to stick to i2c-gpio? If so maybe the 
> same
> applies to client mode and someone should better extend i2c-gpio to
> handle the slave API?

Can you describe what kind of unreliability you've observed? I had the 
master and slave mode running for some time and it seems to work quite 
well. No data losses. No undesired states.

Best regards,
   Juergen

Re: I2C slave mode for i2c-at91 driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2125 bytes --]

Hi Juergen,,

> Currently I'm working on a driver adding I2C slave support to some Atmel /
> Microchip MPUs. I already patched the i2c-at91.c and made the
> i2c-slave-eeprom backend working on an ATSAMA5D27. But this is still in the
> prototype status. My next steps are cleaning up the code and sharing the
> patch with you for review and merge into Linux.

Nice!

> 
> 1) According to [1] the return value of the I2C_SLAVE_WRITE_RECEIVED event
> determines whether the received byte shall be ACKed or NACKed. The problem
> with the Atmel hardware is that it is not possible to manipulate the ACK bit
> of the current byte in flight; it will be ACKed automatically. It is only
> possible to (N)ACK the following byte(s) since some FIFO magic is going on
> inside the hardware.

So, it is not possible to NACK the last byte?

> Do you think it is a valid approach to ignore the return value and always
> ACK received bytes? Or would you rather set the behaviour for the following
> bytes? That would delay the desired ACK bit by at least one byte.

Tricky, since both options are really sub-optimal. I tend to think that
reporting the error a bit later is the slightly better option. Most
client drivers will act on the fact that the whole transfer failed
somehow. Where it fails is not so essential. We don't have proper means
to report the exact position of the failure currently anyhow.

> 2) The Atmel hardware does not support master and slave concurrently. The
> data sheet [2] examples disable master mode before entering the slave mode.
> (cf. p. 1362) Furthermore send and receive registers and their FIFOs are
> shared.
> How would you implement blocking master transactions while slave mode is
> enabled? I would return EBUSY if master_xfer is called.

The i2c-designware driver has the same problem. We decided to have
seperate struct i2c_algorithms for master and slave. The core helper
i2c_detect_slave_mode() can help you to determine which mode should be
used. The designware drive might give you some inspiration.

Hope that helps?

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: I2C slave mode for i2c-at91 driver

[Juergen Fitschen]

Hi Wolfram,

>> 1) According to [1] the return value of the I2C_SLAVE_WRITE_RECEIVED 
>> event
>> determines whether the received byte shall be ACKed or NACKed. The 
>> problem
>> with the Atmel hardware is that it is not possible to manipulate the 
>> ACK bit
>> of the current byte in flight; it will be ACKed automatically. It is 
>> only
>> possible to (N)ACK the following byte(s) since some FIFO magic is 
>> going on
>> inside the hardware.
>
> So, it is not possible to NACK the last byte?

If you would like to receive 3 bytes from a remote master and the last 
byte shall be NACKed, the backend must have the following behaviour to 
be compatible with the SAMA5 MPUs:

                                  +-----+-+-----+-+-----+-+
   Data on wire:                  |BYTE1|A|BYTE2|A|BYTE3|N|
                                  +-----+-+-----+-+-----+-+
   I2C_SLAVE_WRITE_RECEIVED fired:        ^       ^       ^
   Event's return value:                  A       N       X  (X=don't 
care)

But if I understand the documentation correctly, the return value 
corresponds to the currently received byte and not the next byte. So the 
following backend behaviour is described by the docs:

                                  +-----+-+-----+-+-----+-+
   Data on wire:                  |BYTE1|A|BYTE2|A|BYTE3|N|
                                  +-----+-+-----+-+-----+-+
   I2C_SLAVE_WRITE_RECEIVED fired:      ^       ^       ^
   Event's return value:                A       A       N

Or am I wrong?


>> Do you think it is a valid approach to ignore the return value and 
>> always
>> ACK received bytes? Or would you rather set the behaviour for the 
>> following
>> bytes? That would delay the desired ACK bit by at least one byte.
>
> Tricky, since both options are really sub-optimal. I tend to think 
> that
> reporting the error a bit later is the slightly better option. Most
> client drivers will act on the fact that the whole transfer failed
> somehow. Where it fails is not so essential. We don't have proper 
> means
> to report the exact position of the failure currently anyhow.

I had a look into the i2c-designware driver sources. As far as I 
understand it also does not respect the return value of the 
I2C_SLAVE_WRITE_RECEIVED event. It just prints out debug messages if the 
received byte shall be ACKed.


>> How would you implement blocking master transactions while slave 
>> mode is
>> enabled? I would return EBUSY if master_xfer is called.
>
> The i2c-designware driver has the same problem. We decided to have
> seperate struct i2c_algorithms for master and slave. The core helper
> i2c_detect_slave_mode() can help you to determine which mode should 
> be
> used. The designware drive might give you some inspiration.

Thank you for the hint! This really helps.


Kind regards,
   Juergen