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

[Cédric Le Goater]

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.

> 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.

Thanks

C.


> Klaus Jensen (6):
>    hw/i2c/aspeed: rework raise interrupt trace event
>    hw/i2c/aspeed: add DEV_ADDR in old register mode
>    hw/i2c: support multiple masters
>    hw/i2c: add asynchronous send
>    hw/i2c/aspeed: add slave device in old register mode
>    hw/misc: add a toy i2c echo device [DO NOT PULL]
>
>   hw/arm/pxa2xx.c             |   2 +
>   hw/display/sii9022.c        |   2 +
>   hw/display/ssd0303.c        |   2 +
>   hw/i2c/aspeed_i2c.c         | 152 ++++++++++++++++++++++++++++-----
>   hw/i2c/core.c               |  70 +++++++++++++++-
>   hw/i2c/smbus_slave.c        |   4 +
>   hw/i2c/trace-events         |   4 +-
>   hw/misc/i2c-echo.c          | 162 ++++++++++++++++++++++++++++++++++++
>   hw/misc/ibm-cffps.c         |   2 +
>   hw/misc/ir35221.c           |   2 +
>   hw/misc/meson.build         |   2 +
>   hw/nvram/eeprom_at24c.c     |   2 +
>   hw/sensor/lsm303dlhc_mag.c  |   2 +
>   include/hw/i2c/aspeed_i2c.h |  16 ++++
>   include/hw/i2c/i2c.h        |  30 +++++++
>   15 files changed, 428 insertions(+), 26 deletions(-)
>   create mode 100644 hw/misc/i2c-echo.c