mbox series

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

Message ID 20220331165737.1073520-1-its@irrelevant.dk (mailing list archive)
Headers show
Series hw/i2c: i2c slave mode support | expand

Message

Klaus Jensen March 31, 2022, 4:57 p.m. UTC
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.

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

  [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

Comments

Corey Minyard March 31, 2022, 8:32 p.m. UTC | #1
On Thu, Mar 31, 2022 at 06:57:33PM +0200, Klaus Jensen 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.

I've been wondering when this would happen :).  I had put some thought
into how this would work, but hadn't come up with anything good.

The big disadvantage of this is you are adding an interface that is
incompatible with the current masters and slaves.  So you are using the
same I2C bus, but slaves written this way cannot talk to existing
masters, and masters written this way cannot talk to existing slave.
You could adapt the masters to be able to work either way, and I suppose
some slaves that could do it could have both an async send and a normal
send.  But you could not adapt a slave device for the Aspeed to do both.

But that said, I don't know of a better way to handle this.

You don't have the ability to nack a byte in what you have currently.
That's probably something that will be needed.

This is obviously not something useful by itself.  How do you plan to
tie this in to something else that would use it?

-corey

> 
> 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.
> 
> 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
> 
>   [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
> 
>
Klaus Jensen April 1, 2022, 6:29 a.m. UTC | #2
On Mar 31 15:32, Corey Minyard wrote:
> On Thu, Mar 31, 2022 at 06:57:33PM +0200, Klaus Jensen 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.
> 
> I've been wondering when this would happen :).  I had put some thought
> into how this would work, but hadn't come up with anything good.
> 
> The big disadvantage of this is you are adding an interface that is
> incompatible with the current masters and slaves.  So you are using the
> same I2C bus, but slaves written this way cannot talk to existing
> masters, and masters written this way cannot talk to existing slave.
> You could adapt the masters to be able to work either way, and I suppose
> some slaves that could do it could have both an async send and a normal
> send. 

Would it make sense to introduce a QOM Interface to differentiate
between the slave/master types?

> But you could not adapt a slave device for the Aspeed to do both.

Exactly, the Aspeed must be able to defer the ack, so it cannot
implement send(). Even if it buffered up the write, I don't think it
would be correct to Ack the transfer until the host has Acked it.

> But that said, I don't know of a better way to handle this.
> 
> You don't have the ability to nack a byte in what you have currently.
> That's probably something that will be needed.

True. Didn't consider that. Since the ack is basically defined as the
scheduling of the bh, I guess I have to come up with something where I
can also pass a "return value".

> 
> This is obviously not something useful by itself.  How do you plan to
> tie this in to something else that would use it?
> 

This is specifically for implementing an NVMe-MI device which uses MCTP
transactions (in which both requests and replies are master->slave
transfers). I just wanted to get a feel for how you maintaines would
envision this begin done before posting that. The NVMe-MI device will
function exactly like the example i2c echo device (i.e. receive an MCTP
transaction using the normal i2c slave interface, parse the
transaction/request, master the bus and start a new transfer).

Thanks for your comments Corey!
Damien Hedde April 1, 2022, 8:58 a.m. UTC | #3
On 4/1/22 08:29, Klaus Jensen wrote:
> On Mar 31 15:32, Corey Minyard wrote:
>> On Thu, Mar 31, 2022 at 06:57:33PM +0200, Klaus Jensen 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.
>>
>> I've been wondering when this would happen :).  I had put some thought
>> into how this would work, but hadn't come up with anything good.
>>
>> The big disadvantage of this is you are adding an interface that is
>> incompatible with the current masters and slaves.  So you are using the
>> same I2C bus, but slaves written this way cannot talk to existing
>> masters, and masters written this way cannot talk to existing slave.
>> You could adapt the masters to be able to work either way, and I suppose
>> some slaves that could do it could have both an async send and a normal
>> send.
> 
> Would it make sense to introduce a QOM Interface to differentiate
> between the slave/master types?
> 

Probably.

I expect a normal slave-only I2C device will be compatible with any 
master (having or having not this feature) in real life ?

It would be great if the compatibility between "a I2C slave requiring 
the slave-mode from the bus" and the bus could be checked during the 
device plug.

--
Damien
Klaus Jensen April 1, 2022, 9:05 a.m. UTC | #4
On Apr  1 10:58, Damien Hedde wrote:
> 
> On 4/1/22 08:29, Klaus Jensen wrote:
> > On Mar 31 15:32, Corey Minyard wrote:
> > > On Thu, Mar 31, 2022 at 06:57:33PM +0200, Klaus Jensen 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.
> > > 
> > > I've been wondering when this would happen :).  I had put some thought
> > > into how this would work, but hadn't come up with anything good.
> > > 
> > > The big disadvantage of this is you are adding an interface that is
> > > incompatible with the current masters and slaves.  So you are using the
> > > same I2C bus, but slaves written this way cannot talk to existing
> > > masters, and masters written this way cannot talk to existing slave.
> > > You could adapt the masters to be able to work either way, and I suppose
> > > some slaves that could do it could have both an async send and a normal
> > > send.
> > 
> > Would it make sense to introduce a QOM Interface to differentiate
> > between the slave/master types?
> > 
> 
> Probably.
> 
> I expect a normal slave-only I2C device will be compatible with any master
> (having or having not this feature) in real life ?
> 

Yeah, it's just that currently in the i2c core we cannot "suspend" the
sending of the ACK.

> It would be great if the compatibility between "a I2C slave requiring the
> slave-mode from the bus" and the bus could be checked during the device
> plug.
> 

Makes sense, I'll see what I can come up with for a v2 :)

Thanks!
Corey Minyard April 1, 2022, 1:06 p.m. UTC | #5
On Fri, Apr 01, 2022 at 08:29:03AM +0200, Klaus Jensen wrote:
> On Mar 31 15:32, Corey Minyard wrote:
> > On Thu, Mar 31, 2022 at 06:57:33PM +0200, Klaus Jensen 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.
> > 
> > I've been wondering when this would happen :).  I had put some thought
> > into how this would work, but hadn't come up with anything good.
> > 
> > The big disadvantage of this is you are adding an interface that is
> > incompatible with the current masters and slaves.  So you are using the
> > same I2C bus, but slaves written this way cannot talk to existing
> > masters, and masters written this way cannot talk to existing slave.
> > You could adapt the masters to be able to work either way, and I suppose
> > some slaves that could do it could have both an async send and a normal
> > send. 
> 
> Would it make sense to introduce a QOM Interface to differentiate
> between the slave/master types?

Yes, that would be a good idea, as Damien said.  You will have a type
that is capable of both for both sync and async for the master and the
slave, then types that are capable of one sync and async so the code
can sort out what can talk to what.

> 
> > But you could not adapt a slave device for the Aspeed to do both.
> 
> Exactly, the Aspeed must be able to defer the ack, so it cannot
> implement send(). Even if it buffered up the write, I don't think it
> would be correct to Ack the transfer until the host has Acked it.
> 
> > But that said, I don't know of a better way to handle this.
> > 
> > You don't have the ability to nack a byte in what you have currently.
> > That's probably something that will be needed.
> 
> True. Didn't consider that. Since the ack is basically defined as the
> scheduling of the bh, I guess I have to come up with something where I
> can also pass a "return value".
> 
> > 
> > This is obviously not something useful by itself.  How do you plan to
> > tie this in to something else that would use it?
> > 
> 
> This is specifically for implementing an NVMe-MI device which uses MCTP
> transactions (in which both requests and replies are master->slave
> transfers). I just wanted to get a feel for how you maintaines would
> envision this begin done before posting that. The NVMe-MI device will
> function exactly like the example i2c echo device (i.e. receive an MCTP
> transaction using the normal i2c slave interface, parse the
> transaction/request, master the bus and start a new transfer).

Ok, so you aren't planning to add some sort of interface that would
allow a net connection to hook up as an I2C master.

Someone submitted something a while ago for doing an I2C slave that way,
but there were some issues and nothing came of it.  It's tricky to do
because it has to be non-blocking.

IIRC, there was also some work that allowed two emulations to go on at a
time in a qemu instance, that could allow a BMC and a main processor to
run together.  This might be useful in that scenario.  My question was
really just more curiousity, wondering what else is coming in the
future.

Thanks,

-corey
Peter Delevoryas April 5, 2022, 8:52 p.m. UTC | #6
> 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
> 
>
Klaus Jensen April 6, 2022, 6:07 a.m. UTC | #7
On Apr  5 20:52, Peter Delevoryas wrote:
> 
> 
> > 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).
> 

Awesome!

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

Hmmm. The only obvious difference I see here is that I write
"slave-24c02" and not just "24c02" to new_device. Not sure if that has
any implications? Also, looks like your EEPROM is initialized with
zeroes, mine is all ones. This hints at the device being instantiated is
different. I'm also not seeing the 'at24', which upon looking in the
kernel code is a different device?
Peter Delevoryas April 6, 2022, 5:03 p.m. UTC | #8
> On Apr 5, 2022, at 11:07 PM, Klaus Jensen <its@irrelevant.dk> wrote:
> 
> On Apr 5 20:52, Peter Delevoryas wrote:
>> 
>> 
>>> 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).
>> 
> 
> Awesome!
> 
>>> 
>>> 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.
>> 
> 
> Hmmm. The only obvious difference I see here is that I write
> "slave-24c02" and not just "24c02" to new_device. Not sure if that has
> any implications? Also, looks like your EEPROM is initialized with
> zeroes, mine is all ones. This hints at the device being instantiated is
> different. I'm also not seeing the 'at24', which upon looking in the
> kernel code is a different device?

Are you letting the kernel control the EEPROM?

If I actually let the kernel control it, then I can’t use i2cset, because
the kernel seems to be keeping the bus busy/etc. I tried i2c bus 9 this time.

root@bmc-oob:~# i2cset -y 9 0x64 0x00 0xaa i
Error: Could not set address to 0x64: Device or resource busy

However, if I don’t instantiate a kernel device, and I just use i2cset/i2cget,
I can control the EEPROM:

root@bmc-oob:~# i2cset -y 9 0x64 0x00 0xcc i
root@bmc-oob:~# i2cget -y 9 0x64 0x00 i
0xcc 0xb9 0x4d 0xe1 0x42 0x56 0x00 0x00 0xc5 0x5b 0x28 0xe1 0x42 0x56 0x00 0x00 0x00 0x61 0x13 0xe2 0x42 0x56 0x00 0x00 0xb7 0x64 0x28 0xe1 0x42
 0x56 0x00 0x00

Unfortunately, i2c-echo still doesn’t seem to send its data:

root@bmc-oob:~# i2cset -y 9 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

What is the exact sequence of events once i2c-echo
starts a new transfer? Does the slave device ACK
the start, or does it just wait for data to be sent?

And then if I try to read the EEPROM:

root@bmc-oob:~# i2cget -y 9 0x64 0x00 i
smbus: error: Unexpected send start condition in state 1
smbus: error: Unexpected write in state -1
smbus: error: Unexpected recv start condition in state -1
smbus: error: Unexpected read in state -1
smbus: error: Unexpected read in state -1
smbus: error: Unexpected read in state -1

I’ll try debugging/refactoring further to see why it’s not working.

By the way, this is my i2c_init code in QEMU to ensure
a QEMU EEPROM model is present:

static void fby35_i2c_init(AspeedMachineState *bmc)
{
    I2CBus *i2c[16];

    for (int i = 0; i < 16; i++) {
        i2c[i] = aspeed_i2c_get_bus(&bmc->soc.i2c, i);
        assert(i2c[i] != NULL);
    }

    i2c_slave_create_simple(i2c[9], "i2c-echo", 0x42);
    uint8_t buf[256] = {0xff};
    smbus_eeprom_init_one(i2c[9], 0x64, buf);
}

This is an AST2600-based board too.
Klaus Jensen April 6, 2022, 6:41 p.m. UTC | #9
On Apr  6 17:03, Peter Delevoryas wrote:
> 
> 
> > On Apr 5, 2022, at 11:07 PM, Klaus Jensen <its@irrelevant.dk> wrote:
> > 
> > On Apr 5 20:52, Peter Delevoryas wrote:
> >> 
> >> 
> >>> 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).
> >> 
> > 
> > Awesome!
> > 
> >>> 
> >>> 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.
> >> 
> > 
> > Hmmm. The only obvious difference I see here is that I write
> > "slave-24c02" and not just "24c02" to new_device. Not sure if that has
> > any implications? Also, looks like your EEPROM is initialized with
> > zeroes, mine is all ones. This hints at the device being instantiated is
> > different. I'm also not seeing the 'at24', which upon looking in the
> > kernel code is a different device?
> 
> Are you letting the kernel control the EEPROM?
> 
> If I actually let the kernel control it, then I can’t use i2cset, because
> the kernel seems to be keeping the bus busy/etc. I tried i2c bus 9 this time.
> 
> root@bmc-oob:~# i2cset -y 9 0x64 0x00 0xaa i
> Error: Could not set address to 0x64: Device or resource busy
> 
> However, if I don’t instantiate a kernel device, and I just use i2cset/i2cget,
> I can control the EEPROM:
> 
> root@bmc-oob:~# i2cset -y 9 0x64 0x00 0xcc i
> root@bmc-oob:~# i2cget -y 9 0x64 0x00 i
> 0xcc 0xb9 0x4d 0xe1 0x42 0x56 0x00 0x00 0xc5 0x5b 0x28 0xe1 0x42 0x56 0x00 0x00 0x00 0x61 0x13 0xe2 0x42 0x56 0x00 0x00 0xb7 0x64 0x28 0xe1 0x42
>  0x56 0x00 0x00
> 
> Unfortunately, i2c-echo still doesn’t seem to send its data:
> 
> root@bmc-oob:~# i2cset -y 9 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
> 
> What is the exact sequence of events once i2c-echo
> starts a new transfer? Does the slave device ACK
> the start, or does it just wait for data to be sent?
> 
> And then if I try to read the EEPROM:
> 
> root@bmc-oob:~# i2cget -y 9 0x64 0x00 i
> smbus: error: Unexpected send start condition in state 1
> smbus: error: Unexpected write in state -1
> smbus: error: Unexpected recv start condition in state -1
> smbus: error: Unexpected read in state -1
> smbus: error: Unexpected read in state -1
> smbus: error: Unexpected read in state -1
> 
> I’ll try debugging/refactoring further to see why it’s not working.
> 
> By the way, this is my i2c_init code in QEMU to ensure
> a QEMU EEPROM model is present:
> 
> static void fby35_i2c_init(AspeedMachineState *bmc)
> {
>     I2CBus *i2c[16];
> 
>     for (int i = 0; i < 16; i++) {
>         i2c[i] = aspeed_i2c_get_bus(&bmc->soc.i2c, i);
>         assert(i2c[i] != NULL);
>     }
> 
>     i2c_slave_create_simple(i2c[9], "i2c-echo", 0x42);
>     uint8_t buf[256] = {0xff};
>     smbus_eeprom_init_one(i2c[9], 0x64, buf);
> }
> 
> This is an AST2600-based board too.
> 

Oh. You are trying to echo to an actual EEPROM device on the board? In
that case yes. The new async API currently only works with the slave
device on the i2c controller. The i2c echo device cannot talk to any
other slave devices since they do not implement the asynchronous send.
Peter Delevoryas April 6, 2022, 10:06 p.m. UTC | #10
> On Apr 6, 2022, at 11:41 AM, Klaus Jensen <its@irrelevant.dk> wrote:
> 
> On Apr  6 17:03, Peter Delevoryas wrote:
>> 
>> 
>>> On Apr 5, 2022, at 11:07 PM, Klaus Jensen <its@irrelevant.dk> wrote:
>>> 
>>> On Apr 5 20:52, Peter Delevoryas wrote:
>>>> 
>>>> 
>>>>> 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).
>>>> 
>>> 
>>> Awesome!
>>> 
>>>>> 
>>>>> 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.
>>>> 
>>> 
>>> Hmmm. The only obvious difference I see here is that I write
>>> "slave-24c02" and not just "24c02" to new_device. Not sure if that has
>>> any implications? Also, looks like your EEPROM is initialized with
>>> zeroes, mine is all ones. This hints at the device being instantiated is
>>> different. I'm also not seeing the 'at24', which upon looking in the
>>> kernel code is a different device?
>> 
>> Are you letting the kernel control the EEPROM?
>> 
>> If I actually let the kernel control it, then I can’t use i2cset, because
>> the kernel seems to be keeping the bus busy/etc. I tried i2c bus 9 this time.
>> 
>> root@bmc-oob:~# i2cset -y 9 0x64 0x00 0xaa i
>> Error: Could not set address to 0x64: Device or resource busy
>> 
>> However, if I don’t instantiate a kernel device, and I just use i2cset/i2cget,
>> I can control the EEPROM:
>> 
>> root@bmc-oob:~# i2cset -y 9 0x64 0x00 0xcc i
>> root@bmc-oob:~# i2cget -y 9 0x64 0x00 i
>> 0xcc 0xb9 0x4d 0xe1 0x42 0x56 0x00 0x00 0xc5 0x5b 0x28 0xe1 0x42 0x56 0x00 0x00 0x00 0x61 0x13 0xe2 0x42 0x56 0x00 0x00 0xb7 0x64 0x28 0xe1 0x42
>> 0x56 0x00 0x00
>> 
>> Unfortunately, i2c-echo still doesn’t seem to send its data:
>> 
>> root@bmc-oob:~# i2cset -y 9 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
>> 
>> What is the exact sequence of events once i2c-echo
>> starts a new transfer? Does the slave device ACK
>> the start, or does it just wait for data to be sent?
>> 
>> And then if I try to read the EEPROM:
>> 
>> root@bmc-oob:~# i2cget -y 9 0x64 0x00 i
>> smbus: error: Unexpected send start condition in state 1
>> smbus: error: Unexpected write in state -1
>> smbus: error: Unexpected recv start condition in state -1
>> smbus: error: Unexpected read in state -1
>> smbus: error: Unexpected read in state -1
>> smbus: error: Unexpected read in state -1
>> 
>> I’ll try debugging/refactoring further to see why it’s not working.
>> 
>> By the way, this is my i2c_init code in QEMU to ensure
>> a QEMU EEPROM model is present:
>> 
>> static void fby35_i2c_init(AspeedMachineState *bmc)
>> {
>>    I2CBus *i2c[16];
>> 
>>    for (int i = 0; i < 16; i++) {
>>        i2c[i] = aspeed_i2c_get_bus(&bmc->soc.i2c, i);
>>        assert(i2c[i] != NULL);
>>    }
>> 
>>    i2c_slave_create_simple(i2c[9], "i2c-echo", 0x42);
>>    uint8_t buf[256] = {0xff};
>>    smbus_eeprom_init_one(i2c[9], 0x64, buf);
>> }
>> 
>> This is an AST2600-based board too.
>> 
> 
> Oh. You are trying to echo to an actual EEPROM device on the board? In

Ohhh erg yes, I was.

> that case yes. The new async API currently only works with the slave
> device on the i2c controller. The i2c echo device cannot talk to any
> other slave devices since they do not implement the asynchronous send.

Oh that makes total sense.

I was not aware of the whole “slave-eeprom” backend, I thought you
were instantiating a regular eeprom. After enabling that driver in
my Kconfig and using “slave-eeprom”, it all worked! Thanks, sorry
for the confusion. I’m excited to test this out with some more things
now!

root@bmc-oob:~# hexdump -C /sys/bus/i2c/devices/i2c-9/9-0064/slave-eeprom
00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
*
00000100
root@bmc-oob:~# i2cset -y 9 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
i2c_ack: ack'd slave async send
i2c_echo_bh: async sending data[1] (0x00)
i2c_send_async: slave 0x64 data=0x00
i2c_ack: ack'd slave async send
i2c_echo_bh: async sending data[2] (0xaa)
i2c_send_async: slave 0x64 data=0xaa
i2c_ack: ack'd slave async send
root@bmc-oob:~# hexdump -C /sys/bus/i2c/devices/i2c-9/9-0064/slave-eeprom
00000000  aa ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
*
00000100
Jonathan Cameron May 6, 2022, 2:07 p.m. UTC | #11
On Thu, 31 Mar 2022 18:57:33 +0200
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.
> 
> 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.

Hi Klaus,

Just thought I'd mention I'm also interested in MCTP over I2C emulation
for a couple of projects:

1) DMTF SPDM - mostly as a second transport for the kernel stack alongside
   PCI DOE.
2) CXL FM-API - adding support for the Fabric Manager interfaces
   on emulated CXL switches which is also typically carried over
   MCTP.

I was thinking of emulating a MCTP over PCI VDM but this has saved me
going to the effort of doing that for now at least :)

I have hacked a really really basic MCTP device together using this
series and it all seems to be working with the kernel stack (subject to a
few kernel driver bugs that I'll report / send fixes for next week).
I'm cheating all over the place so far, (lots of hard coded values) but
would be interested in a more flexible solution that might perhaps
share infrastructure with your NVMe-MI work.

Thanks,

Jonathan


> 
> 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
> 
>   [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
>
Cédric Le Goater May 6, 2022, 4:49 p.m. UTC | #12
Hello Jonathan,

On 5/6/22 16:07, Jonathan Cameron wrote:
> On Thu, 31 Mar 2022 18:57:33 +0200
> 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.
>>
>> 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.
> 
> Hi Klaus,
> 
> Just thought I'd mention I'm also interested in MCTP over I2C emulation
> for a couple of projects:

Klaus is working on a v2 :

   http://patchwork.ozlabs.org/project/qemu-devel/patch/20220503225925.1798324-2-pdel@fb.com/

Thanks,

C.


> 
> 1) DMTF SPDM - mostly as a second transport for the kernel stack alongside
>     PCI DOE.
> 2) CXL FM-API - adding support for the Fabric Manager interfaces
>     on emulated CXL switches which is also typically carried over
>     MCTP.
> 
> I was thinking of emulating a MCTP over PCI VDM but this has saved me
> going to the effort of doing that for now at least :)
> 
> I have hacked a really really basic MCTP device together using this
> series and it all seems to be working with the kernel stack (subject to a
> few kernel driver bugs that I'll report / send fixes for next week).
> I'm cheating all over the place so far, (lots of hard coded values) but
> would be interested in a more flexible solution that might perhaps
> share infrastructure with your NVMe-MI work.