Re: [RFC PATCH v2 0/6] hw/i2c: i2c slave mode support

[Klaus Jensen <its@irrelevant.dk>]

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

On Jun  2 09:52, Cédric Le Goater wrote:
> On 6/1/22 23:08, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Hi all,
> > 
> > This RFC series adds I2C "slave mode" support for the Aspeed I2C
> 
> I think you can remove the RFC prefix.
> 
> > controller as well as the necessary infrastructure in the i2c core to
> > support this.
> > 
> > v2 changes
> > ~~~~~~~~~~
> > I finally got around to working on this again. I'm sorry for not
> > bringing a v2 to the table earlier.
> > 
> > Mad props to Peter and Jonathan for putting this series to work and
> > pushing it forward! Thanks!
> > 
> > This series is based off Cédric's aspeed-7.1 tree, so it includes the
> > register fields. This is all "old register mode", but Peter seems to
> > have added support in new mode.
> > 
> > There are some loose ends of course, i.e send_async doesn't handle
> > broadcast and asynchronous slaves being sent stuff can't nack. But I
> > wanted to get some feedback on the interface before I tackle that.
> > 
> > This series
> > ~~~~~~~~~~~
> > Patch 1 and 2 are small Aspeed I2C changes/additions.
> > 
> > Patch 3 adds support for multiple masters in the i2c core, allowing
> > slaves to master the bus and (safely) issue i2c_send/recv().
> > 
> > Patch 4 adds an asynchronous send i2c_send_async(I2CBus *, uint8) on the
> > bus that must be paired with an explicit ack using i2c_ack(I2CBus *). We
> > have previously discussed how we wanted to handle the issue that some
> > slaves implement this and some do not. Using a QOM interface was up, but
> > couldn't figure out a good way to do it. I ended up decided against it
> > since I believe this have to be a run-time check anyway. The problem is
> > that a slave can master the bus and try to communicate with *anyone* on
> > the bus - and there is no reason why we should only allow asynchronous
> > slaves on the bus in that case, or whatever we would want to do when
> > devices are plugged. So, instead, the current master can issue an
> > i2c_start_send() and if that fails (because it isnt implemented by the
> > target slave) it can either bail out or use i2c_start_send_async() if it
> > itself supports it. This works the other way around as well of course,
> > but it is probably simpler to handle slaves that respond to
> > i2c_start_send(). This approach relies on adding a new i2c_event, which
> > is why a bunch of other devices needs changes in their event handling.
> > 
> > Patch 5 adds *partial* slave mode functionality to the emulated Aspeed
> > I2C controller, that is, it only supports asynchronous sends started by
> > another slave that is currently mastering the bus. No asynchronous
> > receive.
> 
> If there are no objections, I think this is a good way to move forward
> and improve this initial implementation when the need arises.
> 

There is an outstanding issue with the SLAVE_ADDR_RX_MATCH interrupt bit
(bit 7). Remember from my first series I had a workaround to make sure
it wasnt masked.

I posted this upstream to linux

  https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/

Not sure if that is the right way to fix it. You mentioned something
about "fixing" a mask on the ast2600?

But with the above patch, all works an intended and no "workaround"
required.

> > Finally, patch 6 adds an example device using this new API. The device
> > is a simple "echo" device that upon being sent a set of bytes uses the
> > first byte as the address of the slave to echo to.
> > 
> > With this combined I am able to boot up Linux on an emulated Aspeed 2600
> > evaluation board and have the i2c echo device write into a Linux slave
> > EEPROM. Assuming the echo device is on address 0x42:
> > 
> >    # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-15/new_device
> >    i2c i2c-15: new_device: Instantiated device slave-24c02 at 0x64
> >    # i2cset -y 15 0x42 0x64 0x00 0xaa i
> >    # hexdump /sys/bus/i2c/devices/15-1064/slave-eeprom
> >    0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
> >    0000010 ffff ffff ffff ffff ffff ffff ffff ffff
> >    *
> >    0000100
> 
> I have started working on buildroot images  :
> 
>   https://github.com/legoater/buildroot/commits/aspeed
> 
> The resulting files are quite small :
> 
>     $ ll output/images/
>     total 86040
>     drwxr-xr-x 2 legoater legoater     4096 Jun  1 20:01 ./
>     drwxrwxr-x 6 legoater legoater     4096 Jun  1 19:40 ../
>     -rwxr-xr-x 1 legoater legoater    36837 Jun  1 20:01 aspeed-ast2600-evb.dtb*
>     -rw-r--r-- 1 legoater legoater 67108864 Jun  1 20:01 flash.img
>     -rw-r--r-- 1 legoater legoater  6682796 Jun  1 20:01 image.itb
>     -rw-r--r-- 1 legoater legoater     1846 Jun  1 20:01 image.its
>     -rw-r--r-- 1 legoater legoater  3168768 Jun  1 20:01 rootfs.cpio
>     -rw-r--r-- 1 legoater legoater  1026660 Jun  1 20:01 rootfs.cpio.xz
>     -rw-r--r-- 1 legoater legoater  3788800 Jun  1 20:01 rootfs.tar
>     -rw-r--r-- 1 legoater legoater   653777 Jun  1 20:00 u-boot.bin
>     -rw-r--r-- 1 legoater legoater  5617280 Jun  1 20:01 zImage
> 
> I will probably host them on GH and we could use them under avocado
> to extend the tests.
> 
> 
> They should boot real HW. I will submit the defconfigs to buildroot
> after more tests and cleanups.
> 

Nice!

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