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

[Peter Delevoryas <pdel@fb.com>]

> On Mar 31, 2022, at 9:57 AM, Klaus Jensen <its@irrelevant.dk> wrote:
> 
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Hi all,
> 
> This RFC series adds I2C "slave mode" support for the Aspeed I2C
> controller as well as the necessary infrastructure in the i2c core to
> support this.
> 
> Background
> ~~~~~~~~~~
> We are working on an emulated NVM Express Management Interface[1] for
> testing and validation purposes. NVMe-MI is based on the MCTP
> protocol[2] which may use a variety of underlying transports. The one we
> are interested in is I2C[3].
> 
> The first general trickery here is that all MCTP transactions are based
> on the SMBus Block Write bus protocol[4]. This means that the slave must
> be able to master the bus to communicate. As you know, hw/i2c/core.c
> currently does not support this use case.

This is great, I’m attempting to use your changes right now for the same thing (MCTP).

> 
> The second issue is how to interact with these mastering devices. Jeremy
> and Matt (CC'ed) have been working on an MCTP stack for the Linux Kernel
> (already upstream) and an I2C binding driver[5] is currently under
> review. This binding driver relies on I2C slave mode support in the I2C
> controller.
> 
> This series
> ~~~~~~~~~~~
> Patch 1 adds support for multiple masters in the i2c core, allowing
> slaves to master the bus and safely issue i2c_send/recv(). Patch 2 adds
> an asynchronous send i2c_send_async(I2CBus *, uint8) on the bus that
> must be paired with an explicit ack using i2c_ack(I2CBus *).
> 
> Patch 3 adds the slave mode functionality to the emulated Aspeed I2C
> controller. The implementation is probably buggy since I had to rely on
> the implementation of the kernel driver to reverse engineer the behavior
> of the controller slave mode (I do not have access to a spec sheet for
> the Aspeed, but maybe someone can help me out with that?).
> 
> Finally, patch 4 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

When I try this with my system, it seems like the i2c-echo device takes over
the bus but never echoes the data to the EEPROM. Am I missing something to
make this work? It seems like the “i2c_send_async” calls aren’t happening,
which must be because the bottom half isn’t being scheduled, right? After
the i2c_do_start_transfer, how is the bottom half supposed to be scheduled
again? Is the slave receiving (the EEPROM) supposed to call i2c_ack or something?

root@bmc-oob:~# echo 24c02 0x1064 > /sys/bus/i2c/devices/i2c-8/new_device
[  135.559719] at24 8-1064: 256 byte 24c02 EEPROM, writable, 1 bytes/write
[  135.562661] i2c i2c-8: new_device: Instantiated device 24c02 at 0x64
root@bmc-oob:~# i2cset -y 8 0x42 0x64 0x00 0xaa i
i2c_echo_event: start send
i2c_echo_send: data[0] = 0x64
i2c_echo_send: data[1] = 0x00
i2c_echo_send: data[2] = 0xaa
i2c_echo_event: scheduling bottom-half
i2c_echo_bh: attempting to gain mastery of bus
i2c_echo_bh: starting a send to address 0x64
root@bmc-oob:~# hexdump -C /sys/bus/i2c/devices/8-1064/eeprom
00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000100

Thanks again for this, it’s exactly what I needed.

> 
>  [1]: https://nvmexpress.org/developers/nvme-mi-specification/ 
>  [2]: https://www.dmtf.org/sites/default/files/standards/documents/DSP0236_1.3.1.pdf 
>  [3]: https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.2.0.pdf 
>  [4]: http://www.smbus.org/specs/SMBus_3_1_20180319.pdf 
>  [5]: https://lore.kernel.org/linux-i2c/20220218055106.1944485-1-matt@codeconstruct.com.au/
> 
> Klaus Jensen (4):
>  hw/i2c: support multiple masters
>  hw/i2c: add async send
>  hw/i2c: add slave mode for aspeed_i2c
>  hw/misc: add a toy i2c echo device
> 
> hw/i2c/aspeed_i2c.c         |  95 +++++++++++++++++++++---
> hw/i2c/core.c               |  57 +++++++++++++-
> hw/i2c/trace-events         |   2 +-
> hw/misc/i2c-echo.c          | 144 ++++++++++++++++++++++++++++++++++++
> hw/misc/meson.build         |   2 +
> include/hw/i2c/aspeed_i2c.h |   8 ++
> include/hw/i2c/i2c.h        |  19 +++++
> 7 files changed, 316 insertions(+), 11 deletions(-)
> create mode 100644 hw/misc/i2c-echo.c
> 
> -- 
> 2.35.1
> 
>