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

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

[Klaus Jensen]

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

-- 
2.35.1

[RFC PATCH 1/4] hw/i2c: support multiple masters

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Allow slaves to master the bus by registering a bottom halve. If the bus
is busy, the bottom halve is queued up. When a slave has succesfully
mastered the bus, the bottom halve is scheduled.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/i2c/core.c        | 34 +++++++++++++++++++++++++++++++++-
 include/hw/i2c/i2c.h | 14 ++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d0cb2d32fa44..145dce60782a 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -13,6 +13,7 @@
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "qemu/main-loop.h"
 #include "trace.h"
 
 #define I2C_BROADCAST 0x00
@@ -62,6 +63,7 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
 
     bus = I2C_BUS(qbus_new(TYPE_I2C_BUS, parent, name));
     QLIST_INIT(&bus->current_devs);
+    QSIMPLEQ_INIT(&bus->pending_masters);
     vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_i2c_bus, bus);
     return bus;
 }
@@ -74,7 +76,7 @@ void i2c_slave_set_address(I2CSlave *dev, uint8_t address)
 /* Return nonzero if bus is busy.  */
 int i2c_bus_busy(I2CBus *bus)
 {
-    return !QLIST_EMPTY(&bus->current_devs);
+    return !QLIST_EMPTY(&bus->current_devs) || bus->bh;
 }
 
 bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
@@ -180,6 +182,26 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
                                                : I2C_START_SEND);
 }
 
+void i2c_bus_master(I2CBus *bus, QEMUBH *bh)
+{
+    if (i2c_bus_busy(bus)) {
+        I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
+        node->bh = bh;
+
+        QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
+
+        return;
+    }
+
+    bus->bh = bh;
+    qemu_bh_schedule(bus->bh);
+}
+
+void i2c_bus_release(I2CBus *bus)
+{
+    bus->bh = NULL;
+}
+
 int i2c_start_recv(I2CBus *bus, uint8_t address)
 {
     return i2c_do_start_transfer(bus, address, I2C_START_RECV);
@@ -206,6 +228,16 @@ void i2c_end_transfer(I2CBus *bus)
         g_free(node);
     }
     bus->broadcast = false;
+
+    if (!QSIMPLEQ_EMPTY(&bus->pending_masters)) {
+        I2CPendingMaster *node = QSIMPLEQ_FIRST(&bus->pending_masters);
+        bus->bh = node->bh;
+
+        QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
+        g_free(node);
+
+        qemu_bh_schedule(bus->bh);
+    }
 }
 
 int i2c_send(I2CBus *bus, uint8_t data)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 5ca3b708c0be..be8bb8b78a60 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -69,13 +69,25 @@ struct I2CNode {
     QLIST_ENTRY(I2CNode) next;
 };
 
+typedef struct I2CPendingMaster I2CPendingMaster;
+
+struct I2CPendingMaster {
+    QEMUBH *bh;
+    QSIMPLEQ_ENTRY(I2CPendingMaster) entry;
+};
+
 typedef QLIST_HEAD(I2CNodeList, I2CNode) I2CNodeList;
+typedef QSIMPLEQ_HEAD(I2CPendingMasters, I2CPendingMaster) I2CPendingMasters;
 
 struct I2CBus {
     BusState qbus;
     I2CNodeList current_devs;
+    I2CPendingMasters pending_masters;
     uint8_t saved_address;
     bool broadcast;
+
+    /* Set from slave currently mastering the bus. */
+    QEMUBH *bh;
 };
 
 I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
@@ -117,6 +129,8 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
 
 void i2c_end_transfer(I2CBus *bus);
 void i2c_nack(I2CBus *bus);
+void i2c_bus_master(I2CBus *bus, QEMUBH *bh);
+void i2c_bus_release(I2CBus *bus);
 int i2c_send(I2CBus *bus, uint8_t data);
 uint8_t i2c_recv(I2CBus *bus);
 bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
-- 
2.35.1

[RFC PATCH 2/4] hw/i2c: add async send

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Add an asynchronous version of of i2c_send that requires an explicit
acknowledgement on the bus.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/i2c/core.c        | 23 +++++++++++++++++++++++
 include/hw/i2c/i2c.h |  5 +++++
 2 files changed, 28 insertions(+)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 145dce60782a..344d764d7eaa 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -261,6 +261,20 @@ int i2c_send(I2CBus *bus, uint8_t data)
     return ret ? -1 : 0;
 }
 
+int i2c_send_async(I2CBus *bus, uint8_t data)
+{
+    I2CNode *node = QLIST_FIRST(&bus->current_devs);
+    I2CSlave *slave = node->elt;
+    I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(slave);
+
+    if (sc->send_async) {
+        sc->send_async(slave, data);
+        return 0;
+    }
+
+    return -1;
+}
+
 uint8_t i2c_recv(I2CBus *bus)
 {
     uint8_t data = 0xff;
@@ -297,6 +311,15 @@ void i2c_nack(I2CBus *bus)
     }
 }
 
+void i2c_ack(I2CBus *bus)
+{
+    if (!bus->bh) {
+        return;
+    }
+
+    qemu_bh_schedule(bus->bh);
+}
+
 static int i2c_slave_post_load(void *opaque, int version_id)
 {
     I2CSlave *dev = opaque;
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index be8bb8b78a60..ae58e4151585 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -28,6 +28,9 @@ struct I2CSlaveClass {
     /* Master to slave. Returns non-zero for a NAK, 0 for success. */
     int (*send)(I2CSlave *s, uint8_t data);
 
+    /* Master to slave. */
+    void (*send_async)(I2CSlave *s, uint8_t data);
+
     /*
      * Slave to master.  This cannot fail, the device should always
      * return something here.
@@ -129,9 +132,11 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
 
 void i2c_end_transfer(I2CBus *bus);
 void i2c_nack(I2CBus *bus);
+void i2c_ack(I2CBus *bus);
 void i2c_bus_master(I2CBus *bus, QEMUBH *bh);
 void i2c_bus_release(I2CBus *bus);
 int i2c_send(I2CBus *bus, uint8_t data);
+int i2c_send_async(I2CBus *bus, uint8_t data);
 uint8_t i2c_recv(I2CBus *bus);
 bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
                   I2CNodeList *current_devs);
-- 
2.35.1

[RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Add slave mode functionality for the Aspeed I2C controller. This is
implemented by creating an Aspeed I2C Slave device that attaches to the
bus.

This i2c slave device only implements the asynchronous version of
i2c_send() and the event callback.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/i2c/aspeed_i2c.c         | 95 +++++++++++++++++++++++++++++++++----
 hw/i2c/trace-events         |  2 +-
 hw/misc/meson.build         |  2 +
 include/hw/i2c/aspeed_i2c.h |  8 ++++
 4 files changed, 97 insertions(+), 10 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 03a4f5a91010..61b6424434f7 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -163,10 +163,15 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
           bus->intr_status & I2CD_INTR_TX_NAK ? "nak|" : "",
           bus->intr_status & I2CD_INTR_TX_ACK ? "ack|" : "",
           bus->intr_status & I2CD_INTR_RX_DONE ? "done|" : "",
+          bus->intr_status & I2CD_INTR_SLAVE_ADDR_RX_MATCH ? "slave-match|" : "",
           bus->intr_status & I2CD_INTR_NORMAL_STOP ? "normal|" : "",
           bus->intr_status & I2CD_INTR_ABNORMAL ? "abnormal" : "");
 
-    bus->intr_status &= bus->intr_ctrl;
+    /*
+     * WORKAROUND: the Linux Aspeed I2C driver masks SLAVE_ADDR_RX_MATCH for
+     * some reason, not sure if it is a bug...
+     */
+    bus->intr_status &= (bus->intr_ctrl | I2CD_INTR_SLAVE_ADDR_RX_MATCH);
     if (bus->intr_status) {
         bus->controller->intr_status |= 1 << bus->id;
         qemu_irq_raise(aic->bus_get_irq(bus));
@@ -196,6 +201,9 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
     case I2CD_INTR_STS_REG:
         value = bus->intr_status;
         break;
+    case I2CD_DEV_ADDR_REG:
+        value = bus->dev_addr;
+        break;
     case I2CD_POOL_CTRL_REG:
         value = bus->pool_ctrl;
         break;
@@ -535,10 +543,9 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
     switch (offset) {
     case I2CD_FUN_CTRL_REG:
         if (value & I2CD_SLAVE_EN) {
-            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
-                          __func__);
-            break;
+            i2c_slave_set_address(&bus->slave->i2c, bus->dev_addr);
         }
+
         bus->ctrl = value & 0x0071C3FF;
         break;
     case I2CD_AC_TIMING_REG1:
@@ -558,14 +565,19 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
             bus->controller->intr_status &= ~(1 << bus->id);
             qemu_irq_lower(aic->bus_get_irq(bus));
         }
-        if (handle_rx && (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
-            aspeed_i2c_handle_rx_cmd(bus);
-            aspeed_i2c_bus_raise_interrupt(bus);
+
+        if (handle_rx) {
+            if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
+                aspeed_i2c_handle_rx_cmd(bus);
+                aspeed_i2c_bus_raise_interrupt(bus);
+            } else if (aspeed_i2c_get_state(bus) == I2CD_STXD) {
+                i2c_ack(bus->bus);
+            }
         }
+
         break;
     case I2CD_DEV_ADDR_REG:
-        qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
-                      __func__);
+        bus->dev_addr = value;
         break;
     case I2CD_POOL_CTRL_REG:
         bus->pool_ctrl &= ~0xffffff;
@@ -852,12 +864,74 @@ static const TypeInfo aspeed_i2c_info = {
     .abstract   = true,
 };
 
+static int aspeed_i2c_slave_event(I2CSlave *slave, enum i2c_event event)
+{
+    AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
+    AspeedI2CBus *bus = s->bus;
+
+    switch (event) {
+    case I2C_START_SEND:
+        bus->buf = bus->dev_addr << 1;
+
+        bus->buf &= I2CD_BYTE_BUF_RX_MASK;
+        bus->buf <<= I2CD_BYTE_BUF_RX_SHIFT;
+
+        bus->intr_status |= (I2CD_INTR_SLAVE_ADDR_RX_MATCH | I2CD_INTR_RX_DONE);
+        aspeed_i2c_set_state(bus, I2CD_STXD);
+
+        break;
+
+    case I2C_FINISH:
+        bus->intr_status |= I2CD_INTR_NORMAL_STOP;
+        aspeed_i2c_set_state(bus, I2CD_IDLE);
+
+        break;
+
+    default:
+        return -1;
+    }
+
+    aspeed_i2c_bus_raise_interrupt(bus);
+
+    return 0;
+}
+
+static void aspeed_i2c_slave_send_async(I2CSlave *slave, uint8_t data)
+{
+    AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
+    AspeedI2CBus *bus = s->bus;
+
+    bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
+    bus->intr_status |= I2CD_INTR_RX_DONE;
+
+    aspeed_i2c_bus_raise_interrupt(bus);
+}
+
+static void aspeed_i2c_slave_class_init(ObjectClass *klass, void *Data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+
+    dc->desc = "Aspeed I2C Bus Slave";
+
+    sc->event = aspeed_i2c_slave_event;
+    sc->send_async = aspeed_i2c_slave_send_async;
+}
+
+static const TypeInfo aspeed_i2c_slave_info = {
+    .name          = TYPE_ASPEED_I2C_SLAVE,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(AspeedI2CSlave),
+    .class_init    = aspeed_i2c_slave_class_init,
+};
+
 static void aspeed_i2c_bus_reset(DeviceState *dev)
 {
     AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
 
     s->intr_ctrl = 0;
     s->intr_status = 0;
+    s->dev_addr = 0;
     s->cmd = 0;
     s->buf = 0;
     s->dma_addr = 0;
@@ -881,6 +955,8 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
     sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
 
     s->bus = i2c_init_bus(dev, name);
+    s->slave = ASPEED_I2C_SLAVE(i2c_slave_create_simple(s->bus, TYPE_ASPEED_I2C_SLAVE, 0xff));
+    s->slave->bus = s;
 
     memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
                           s, name, aic->reg_size);
@@ -1016,6 +1092,7 @@ static const TypeInfo aspeed_2600_i2c_info = {
 static void aspeed_i2c_register_types(void)
 {
     type_register_static(&aspeed_i2c_bus_info);
+    type_register_static(&aspeed_i2c_slave_info);
     type_register_static(&aspeed_i2c_info);
     type_register_static(&aspeed_2400_i2c_info);
     type_register_static(&aspeed_2500_i2c_info);
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 7d8907c1eede..85e4bddff936 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -9,7 +9,7 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
 # aspeed_i2c.c
 
 aspeed_i2c_bus_cmd(uint32_t cmd, const char *cmd_flags, uint32_t count, uint32_t intr_status) "handling cmd=0x%x %s count=%d intr=0x%x"
-aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5) "handled intr=0x%x %s%s%s%s%s"
+aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5, const char *str6) "handled intr=0x%x %s%s%s%s%s%s"
 aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
 aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
 aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send %d/%d 0x%02x"
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 6fb69612e064..c1c1abea41dd 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -122,6 +122,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
 
 softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
 
+softmmu_ss.add(when: 'CONFIG_I2C', if_true: files('i2c-echo.c'))
+
 specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c'))
 
 specific_ss.add(when: 'CONFIG_IMX', if_true: files('imx6_src.c'))
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 4b9be09274c7..3f1a9c07a00b 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -42,6 +42,7 @@ struct AspeedI2CBus {
     SysBusDevice parent_obj;
 
     struct AspeedI2CState *controller;
+    struct AspeedI2CSlave *slave;
 
     MemoryRegion mr;
 
@@ -53,6 +54,7 @@ struct AspeedI2CBus {
     uint32_t timing[2];
     uint32_t intr_ctrl;
     uint32_t intr_status;
+    uint32_t dev_addr;
     uint32_t cmd;
     uint32_t buf;
     uint32_t pool_ctrl;
@@ -76,6 +78,12 @@ struct AspeedI2CState {
     AddressSpace dram_as;
 };
 
+#define TYPE_ASPEED_I2C_SLAVE "aspeed.i2c.slave"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedI2CSlave, ASPEED_I2C_SLAVE)
+struct AspeedI2CSlave {
+    I2CSlave i2c;
+    AspeedI2CBus *bus;
+};
 
 struct AspeedI2CClass {
     SysBusDeviceClass parent_class;
-- 
2.35.1

[RFC PATCH 4/4] hw/misc: add a toy i2c echo device

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Add an example i2c device that can master the bus. The device will echo
whatever it is sent to the device identified by the first byte received.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/misc/i2c-echo.c | 144 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)
 create mode 100644 hw/misc/i2c-echo.c

diff --git a/hw/misc/i2c-echo.c b/hw/misc/i2c-echo.c
new file mode 100644
index 000000000000..72155d850d5f
--- /dev/null
+++ b/hw/misc/i2c-echo.c
@@ -0,0 +1,144 @@
+#include "qemu/osdep.h"
+#include "qemu/timer.h"
+#include "qemu/main-loop.h"
+#include "block/aio.h"
+#include "hw/i2c/i2c.h"
+
+#define TYPE_I2C_ECHO "i2c-echo"
+OBJECT_DECLARE_SIMPLE_TYPE(I2CEchoState, I2C_ECHO)
+
+enum i2c_echo_state {
+    I2C_ECHO_STATE_REQUEST_MASTER,
+    I2C_ECHO_STATE_START_SEND,
+    I2C_ECHO_STATE_ACK,
+};
+
+typedef struct I2CEchoState {
+    I2CSlave parent_obj;
+
+    I2CBus *bus;
+
+    enum i2c_echo_state state;
+    QEMUBH *bh;
+
+    unsigned int pos;
+    uint8_t data[3];
+} I2CEchoState;
+
+static void i2c_echo_bh(void *opaque)
+{
+    I2CEchoState *state = opaque;
+
+    switch (state->state) {
+    case I2C_ECHO_STATE_REQUEST_MASTER:
+        i2c_bus_master(state->bus, state->bh);
+        state->state = I2C_ECHO_STATE_START_SEND;
+        break;
+
+    case I2C_ECHO_STATE_START_SEND:
+        i2c_start_send(state->bus, state->data[0]);
+        state->pos++;
+        state->state = I2C_ECHO_STATE_ACK;
+        break;
+
+    case I2C_ECHO_STATE_ACK:
+        if (state->pos > 2) {
+            i2c_end_transfer(state->bus);
+            i2c_bus_release(state->bus);
+            break;
+        }
+
+        i2c_send_async(state->bus, state->data[state->pos++]);
+        break;
+    }
+}
+
+static int i2c_echo_event(I2CSlave *s, enum i2c_event event)
+{
+    I2CEchoState *state = I2C_ECHO(s);
+
+    switch (event) {
+    case I2C_START_RECV:
+        state->pos = 0;
+
+        break;
+
+    case I2C_START_SEND:
+        state->pos = 0;
+
+        break;
+
+    case I2C_FINISH:
+        state->pos = 0;
+        state->state = I2C_ECHO_STATE_REQUEST_MASTER;
+        qemu_bh_schedule(state->bh);
+
+        break;
+
+    case I2C_NACK:
+        break;
+    }
+
+    return 0;
+}
+
+static uint8_t i2c_echo_recv(I2CSlave *s)
+{
+    I2CEchoState *state = I2C_ECHO(s);
+
+    if (state->pos > 2) {
+        return 0xff;
+    }
+
+    return state->data[state->pos++];
+}
+
+static int i2c_echo_send(I2CSlave *s, uint8_t data)
+{
+    I2CEchoState *state = I2C_ECHO(s);
+
+    if (state->pos > 2) {
+        return -1;
+    }
+
+    state->data[state->pos++] = data;
+
+    return 0;
+}
+
+static void i2c_echo_realize(DeviceState *dev, Error **errp)
+{
+    I2CEchoState *state = I2C_ECHO(dev);
+    BusState *bus = qdev_get_parent_bus(dev);
+
+    state->bus = I2C_BUS(bus);
+    state->bh = qemu_bh_new(i2c_echo_bh, state);
+
+    return;
+}
+
+static void i2c_echo_class_init(ObjectClass *oc, void *data)
+{
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = i2c_echo_realize;
+
+    sc->event = i2c_echo_event;
+    sc->recv = i2c_echo_recv;
+    sc->send = i2c_echo_send;
+}
+
+static const TypeInfo i2c_echo = {
+    .name = TYPE_I2C_ECHO,
+    .parent = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(I2CEchoState),
+    .class_init = i2c_echo_class_init,
+};
+
+static void register_types(void)
+{
+    type_register_static(&i2c_echo);
+}
+
+type_init(register_types);
-- 
2.35.1

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

[Corey Minyard]

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

Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c

[Philippe Mathieu-Daudé]

On 31/3/22 18:57, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Add slave mode functionality for the Aspeed I2C controller. This is
> implemented by creating an Aspeed I2C Slave device that attaches to the
> bus.
> 
> This i2c slave device only implements the asynchronous version of
> i2c_send() and the event callback.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/i2c/aspeed_i2c.c         | 95 +++++++++++++++++++++++++++++++++----
>   hw/i2c/trace-events         |  2 +-
>   hw/misc/meson.build         |  2 +
>   include/hw/i2c/aspeed_i2c.h |  8 ++++
>   4 files changed, 97 insertions(+), 10 deletions(-)

> @@ -558,14 +565,19 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>               bus->controller->intr_status &= ~(1 << bus->id);
>               qemu_irq_lower(aic->bus_get_irq(bus));
>           }
> -        if (handle_rx && (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> -            aspeed_i2c_handle_rx_cmd(bus);
> -            aspeed_i2c_bus_raise_interrupt(bus);
> +
> +        if (handle_rx) {
> +            if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> +                aspeed_i2c_handle_rx_cmd(bus);
> +                aspeed_i2c_bus_raise_interrupt(bus);

Eventually split this hunk into a separate patch to have better readability.

> +            }
>           }

> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 6fb69612e064..c1c1abea41dd 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -122,6 +122,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
>   
>   softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
>   
> +softmmu_ss.add(when: 'CONFIG_I2C', if_true: files('i2c-echo.c'))

This change belongs to the next patch.

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

[Klaus Jensen]

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

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!

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

Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c

[Klaus Jensen]

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

On Mar 31 22:44, Philippe Mathieu-Daudé wrote:
> On 31/3/22 18:57, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Add slave mode functionality for the Aspeed I2C controller. This is
> > implemented by creating an Aspeed I2C Slave device that attaches to the
> > bus.
> > 
> > This i2c slave device only implements the asynchronous version of
> > i2c_send() and the event callback.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/i2c/aspeed_i2c.c         | 95 +++++++++++++++++++++++++++++++++----
> >   hw/i2c/trace-events         |  2 +-
> >   hw/misc/meson.build         |  2 +
> >   include/hw/i2c/aspeed_i2c.h |  8 ++++
> >   4 files changed, 97 insertions(+), 10 deletions(-)
> 
> > @@ -558,14 +565,19 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> >               bus->controller->intr_status &= ~(1 << bus->id);
> >               qemu_irq_lower(aic->bus_get_irq(bus));
> >           }
> > -        if (handle_rx && (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> > -            aspeed_i2c_handle_rx_cmd(bus);
> > -            aspeed_i2c_bus_raise_interrupt(bus);
> > +
> > +        if (handle_rx) {
> > +            if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> > +                aspeed_i2c_handle_rx_cmd(bus);
> > +                aspeed_i2c_bus_raise_interrupt(bus);
> 
> Eventually split this hunk into a separate patch to have better readability.
> 

Alright.

> > +            }
> >           }
> 
> > diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> > index 6fb69612e064..c1c1abea41dd 100644
> > --- a/hw/misc/meson.build
> > +++ b/hw/misc/meson.build
> > @@ -122,6 +122,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
> >   softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
> > +softmmu_ss.add(when: 'CONFIG_I2C', if_true: files('i2c-echo.c'))
> 
> This change belongs to the next patch.

Woops. Chopping up my changes went a bit too fast I think :)

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

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

[Damien Hedde]

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

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

[Klaus Jensen]

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

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!

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

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

[Corey Minyard]

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

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

[Peter Delevoryas]

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

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

[Klaus Jensen]

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

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?

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

Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c

[Cédric Le Goater]

Hello Klaus,

On 3/31/22 18:57, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Add slave mode functionality for the Aspeed I2C controller. This is
> implemented by creating an Aspeed I2C Slave device that attaches to the
> bus.
> 
> This i2c slave device only implements the asynchronous version of
> i2c_send() and the event callback.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/i2c/aspeed_i2c.c         | 95 +++++++++++++++++++++++++++++++++----
>   hw/i2c/trace-events         |  2 +-
>   hw/misc/meson.build         |  2 +
>   include/hw/i2c/aspeed_i2c.h |  8 ++++
>   4 files changed, 97 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 03a4f5a91010..61b6424434f7 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -163,10 +163,15 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>             bus->intr_status & I2CD_INTR_TX_NAK ? "nak|" : "",
>             bus->intr_status & I2CD_INTR_TX_ACK ? "ack|" : "",
>             bus->intr_status & I2CD_INTR_RX_DONE ? "done|" : "",
> +          bus->intr_status & I2CD_INTR_SLAVE_ADDR_RX_MATCH ? "slave-match|" : "",
>             bus->intr_status & I2CD_INTR_NORMAL_STOP ? "normal|" : "",
>             bus->intr_status & I2CD_INTR_ABNORMAL ? "abnormal" : "");

Troy introduced a similar change in his "new mode" proposal. I think
it is time to change the 'aspeed_i2c_bus_raise_interrupt' trace event

Could you please update trace_aspeed_i2c_bus_raise_interrupt() to take
a single status string ?

> -    bus->intr_status &= bus->intr_ctrl;
> +    /*
> +     * WORKAROUND: the Linux Aspeed I2C driver masks SLAVE_ADDR_RX_MATCH for
> +     * some reason, not sure if it is a bug...
> +     */
> +    bus->intr_status &= (bus->intr_ctrl | I2CD_INTR_SLAVE_ADDR_RX_MATCH);

It comes from the initial support for the AST2400 SoC.

We should introduce a 'intr_ctrl_mask' attribute in AspeedI2CClass and
fix the AST24000 value to 0x7FFF ...

>       if (bus->intr_status) {
>           bus->controller->intr_status |= 1 << bus->id;
>           qemu_irq_raise(aic->bus_get_irq(bus));
> @@ -196,6 +201,9 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
>       case I2CD_INTR_STS_REG:
>           value = bus->intr_status;
>           break;
> +    case I2CD_DEV_ADDR_REG:
> +        value = bus->dev_addr;
> +        break;

You can introduce support for this register in a preliminary patch but
keep the slave activation for later (I2CD_SLAVE_EN bit)

>       case I2CD_POOL_CTRL_REG:
>           value = bus->pool_ctrl;
>           break;
> @@ -535,10 +543,9 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>       switch (offset) {
>       case I2CD_FUN_CTRL_REG:
>           if (value & I2CD_SLAVE_EN) {
> -            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> -                          __func__);
> -            break;
> +            i2c_slave_set_address(&bus->slave->i2c, bus->dev_addr);
>           }
> +
>           bus->ctrl = value & 0x0071C3FF;
>           break;
>       case I2CD_AC_TIMING_REG1:
> @@ -558,14 +565,19 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>               bus->controller->intr_status &= ~(1 << bus->id);
>               qemu_irq_lower(aic->bus_get_irq(bus));
>           }
> -        if (handle_rx && (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> -            aspeed_i2c_handle_rx_cmd(bus);
> -            aspeed_i2c_bus_raise_interrupt(bus);
> +
> +        if (handle_rx) {
> +            if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> +                aspeed_i2c_handle_rx_cmd(bus);
> +                aspeed_i2c_bus_raise_interrupt(bus);
> +            } else if (aspeed_i2c_get_state(bus) == I2CD_STXD) {
> +                i2c_ack(bus->bus);
> +            }
>           }
> +
>
>           break;
>       case I2CD_DEV_ADDR_REG:
> -        qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> -                      __func__);
> +        bus->dev_addr = value;
>           break;
>       case I2CD_POOL_CTRL_REG:
>           bus->pool_ctrl &= ~0xffffff;
> @@ -852,12 +864,74 @@ static const TypeInfo aspeed_i2c_info = {
>       .abstract   = true,
>   };
>   
> +static int aspeed_i2c_slave_event(I2CSlave *slave, enum i2c_event event)
> +{
> +    AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
> +    AspeedI2CBus *bus = s->bus;
> +
> +    switch (event) {
> +    case I2C_START_SEND:
> +        bus->buf = bus->dev_addr << 1;
> +
> +        bus->buf &= I2CD_BYTE_BUF_RX_MASK;
> +        bus->buf <<= I2CD_BYTE_BUF_RX_SHIFT;
> +
> +        bus->intr_status |= (I2CD_INTR_SLAVE_ADDR_RX_MATCH | I2CD_INTR_RX_DONE);
> +        aspeed_i2c_set_state(bus, I2CD_STXD);
> +
> +        break;
> +
> +    case I2C_FINISH:
> +        bus->intr_status |= I2CD_INTR_NORMAL_STOP;
> +        aspeed_i2c_set_state(bus, I2CD_IDLE);
> +
> +        break;
> +
> +    default:
> +        return -1;
> +    }
> +
> +    aspeed_i2c_bus_raise_interrupt(bus);
> +
> +    return 0;
> +}
> +
> +static void aspeed_i2c_slave_send_async(I2CSlave *slave, uint8_t data)
> +{
> +    AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
> +    AspeedI2CBus *bus = s->bus;
> +
> +    bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> +    bus->intr_status |= I2CD_INTR_RX_DONE;
> +
> +    aspeed_i2c_bus_raise_interrupt(bus);
> +}
> +
> +static void aspeed_i2c_slave_class_init(ObjectClass *klass, void *Data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> +
> +    dc->desc = "Aspeed I2C Bus Slave";
> +
> +    sc->event = aspeed_i2c_slave_event;
> +    sc->send_async = aspeed_i2c_slave_send_async;
> +}
> +
> +static const TypeInfo aspeed_i2c_slave_info = {
> +    .name          = TYPE_ASPEED_I2C_SLAVE,
> +    .parent        = TYPE_I2C_SLAVE,
> +    .instance_size = sizeof(AspeedI2CSlave),
> +    .class_init    = aspeed_i2c_slave_class_init,
> +};
> +
>   static void aspeed_i2c_bus_reset(DeviceState *dev)
>   {
>       AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
>   
>       s->intr_ctrl = 0;
>       s->intr_status = 0;
> +    s->dev_addr = 0;

Please include the new reg in vmstate.

>       s->cmd = 0;
>       s->buf = 0;
>       s->dma_addr = 0;
> @@ -881,6 +955,8 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
>       sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>   
>       s->bus = i2c_init_bus(dev, name);
> +    s->slave = ASPEED_I2C_SLAVE(i2c_slave_create_simple(s->bus, TYPE_ASPEED_I2C_SLAVE, 0xff));
> +    s->slave->bus = s;
>   
>       memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
>                             s, name, aic->reg_size);
> @@ -1016,6 +1092,7 @@ static const TypeInfo aspeed_2600_i2c_info = {
>   static void aspeed_i2c_register_types(void)
>   {
>       type_register_static(&aspeed_i2c_bus_info);
> +    type_register_static(&aspeed_i2c_slave_info);
>       type_register_static(&aspeed_i2c_info);
>       type_register_static(&aspeed_2400_i2c_info);
>       type_register_static(&aspeed_2500_i2c_info);
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 7d8907c1eede..85e4bddff936 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -9,7 +9,7 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
>   # aspeed_i2c.c
>   
>   aspeed_i2c_bus_cmd(uint32_t cmd, const char *cmd_flags, uint32_t count, uint32_t intr_status) "handling cmd=0x%x %s count=%d intr=0x%x"
> -aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5) "handled intr=0x%x %s%s%s%s%s"
> +aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5, const char *str6) "handled intr=0x%x %s%s%s%s%s%s"
>   aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
>   aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
>   aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send %d/%d 0x%02x"
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 6fb69612e064..c1c1abea41dd 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -122,6 +122,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
>   
>   softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
>   
> +softmmu_ss.add(when: 'CONFIG_I2C', if_true: files('i2c-echo.c'))

That's for another patch.

> +
>   specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c'))
>   
>   specific_ss.add(when: 'CONFIG_IMX', if_true: files('imx6_src.c'))
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 4b9be09274c7..3f1a9c07a00b 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -42,6 +42,7 @@ struct AspeedI2CBus {
>       SysBusDevice parent_obj;
>   
>       struct AspeedI2CState *controller;
> +    struct AspeedI2CSlave *slave;
>   
>       MemoryRegion mr;
>   
> @@ -53,6 +54,7 @@ struct AspeedI2CBus {
>       uint32_t timing[2];
>       uint32_t intr_ctrl;
>       uint32_t intr_status;
> +    uint32_t dev_addr;
>       uint32_t cmd;
>       uint32_t buf;
>       uint32_t pool_ctrl;
> @@ -76,6 +78,12 @@ struct AspeedI2CState {
>       AddressSpace dram_as;
>   };
>   
> +#define TYPE_ASPEED_I2C_SLAVE "aspeed.i2c.slave"
> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedI2CSlave, ASPEED_I2C_SLAVE)
> +struct AspeedI2CSlave {
> +    I2CSlave i2c;
> +    AspeedI2CBus *bus;
> +};

AFAICT, AspeedI2CSlave is not that useful since it doesn't maintain any
state. The QOM interface proposal looks like a better approach.

>   struct AspeedI2CClass {
>       SysBusDeviceClass parent_class;


If you could send these 3 patches :
  
   - aspeed_i2c_bus_raise_interrupt trace event rework
   - intr_ctrl_mask class attribute
   - dev_addr support

I will queue them quickly and we will focus on adding slave support only.


Thanks,

C.

Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c

[Klaus Jensen]

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

On Apr  6 08:14, Cédric Le Goater wrote:
> Hello Klaus,
> 
> On 3/31/22 18:57, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Add slave mode functionality for the Aspeed I2C controller. This is
> > implemented by creating an Aspeed I2C Slave device that attaches to the
> > bus.
> > 
> > This i2c slave device only implements the asynchronous version of
> > i2c_send() and the event callback.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/i2c/aspeed_i2c.c         | 95 +++++++++++++++++++++++++++++++++----
> >   hw/i2c/trace-events         |  2 +-
> >   hw/misc/meson.build         |  2 +
> >   include/hw/i2c/aspeed_i2c.h |  8 ++++
> >   4 files changed, 97 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > index 03a4f5a91010..61b6424434f7 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -163,10 +163,15 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
> >             bus->intr_status & I2CD_INTR_TX_NAK ? "nak|" : "",
> >             bus->intr_status & I2CD_INTR_TX_ACK ? "ack|" : "",
> >             bus->intr_status & I2CD_INTR_RX_DONE ? "done|" : "",
> > +          bus->intr_status & I2CD_INTR_SLAVE_ADDR_RX_MATCH ? "slave-match|" : "",
> >             bus->intr_status & I2CD_INTR_NORMAL_STOP ? "normal|" : "",
> >             bus->intr_status & I2CD_INTR_ABNORMAL ? "abnormal" : "");
> 
> Troy introduced a similar change in his "new mode" proposal. I think
> it is time to change the 'aspeed_i2c_bus_raise_interrupt' trace event
> 
> Could you please update trace_aspeed_i2c_bus_raise_interrupt() to take
> a single status string ?
> 

I'm not sure it will be "prettier". But I'll give it a shot.

> > -    bus->intr_status &= bus->intr_ctrl;
> > +    /*
> > +     * WORKAROUND: the Linux Aspeed I2C driver masks SLAVE_ADDR_RX_MATCH for
> > +     * some reason, not sure if it is a bug...
> > +     */
> > +    bus->intr_status &= (bus->intr_ctrl | I2CD_INTR_SLAVE_ADDR_RX_MATCH);
> 
> It comes from the initial support for the AST2400 SoC.
> 
> We should introduce a 'intr_ctrl_mask' attribute in AspeedI2CClass and
> fix the AST24000 value to 0x7FFF ...
> 

I'm not sure I understand. Do you suggest that we always use a fixed
mask here and disregard what the host sets in intr_ctrl?

In any case, isn't it a bug in the Linux kernel driver that it neglects
to set bit 7 (slave match) in the INTR_CTRL register?

> >       if (bus->intr_status) {
> >           bus->controller->intr_status |= 1 << bus->id;
> >           qemu_irq_raise(aic->bus_get_irq(bus));
> > @@ -196,6 +201,9 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
> >       case I2CD_INTR_STS_REG:
> >           value = bus->intr_status;
> >           break;
> > +    case I2CD_DEV_ADDR_REG:
> > +        value = bus->dev_addr;
> > +        break;
> 
> You can introduce support for this register in a preliminary patch but
> keep the slave activation for later (I2CD_SLAVE_EN bit)
> 

Understood.

> >       case I2CD_POOL_CTRL_REG:
> >           value = bus->pool_ctrl;
> >           break;
> > @@ -535,10 +543,9 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> >       switch (offset) {
> >       case I2CD_FUN_CTRL_REG:
> >           if (value & I2CD_SLAVE_EN) {
> > -            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> > -                          __func__);
> > -            break;
> > +            i2c_slave_set_address(&bus->slave->i2c, bus->dev_addr);
> >           }
> > +
> >           bus->ctrl = value & 0x0071C3FF;
> >           break;
> >       case I2CD_AC_TIMING_REG1:
> > @@ -558,14 +565,19 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> >               bus->controller->intr_status &= ~(1 << bus->id);
> >               qemu_irq_lower(aic->bus_get_irq(bus));
> >           }
> > -        if (handle_rx && (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> > -            aspeed_i2c_handle_rx_cmd(bus);
> > -            aspeed_i2c_bus_raise_interrupt(bus);
> > +
> > +        if (handle_rx) {
> > +            if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> > +                aspeed_i2c_handle_rx_cmd(bus);
> > +                aspeed_i2c_bus_raise_interrupt(bus);
> > +            } else if (aspeed_i2c_get_state(bus) == I2CD_STXD) {
> > +                i2c_ack(bus->bus);
> > +            }
> >           }
> > +
> > 
> >           break;
> >       case I2CD_DEV_ADDR_REG:
> > -        qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> > -                      __func__);
> > +        bus->dev_addr = value;
> >           break;
> >       case I2CD_POOL_CTRL_REG:
> >           bus->pool_ctrl &= ~0xffffff;
> > @@ -852,12 +864,74 @@ static const TypeInfo aspeed_i2c_info = {
> >       .abstract   = true,
> >   };
> > +static int aspeed_i2c_slave_event(I2CSlave *slave, enum i2c_event event)
> > +{
> > +    AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
> > +    AspeedI2CBus *bus = s->bus;
> > +
> > +    switch (event) {
> > +    case I2C_START_SEND:
> > +        bus->buf = bus->dev_addr << 1;
> > +
> > +        bus->buf &= I2CD_BYTE_BUF_RX_MASK;
> > +        bus->buf <<= I2CD_BYTE_BUF_RX_SHIFT;
> > +
> > +        bus->intr_status |= (I2CD_INTR_SLAVE_ADDR_RX_MATCH | I2CD_INTR_RX_DONE);
> > +        aspeed_i2c_set_state(bus, I2CD_STXD);
> > +
> > +        break;
> > +
> > +    case I2C_FINISH:
> > +        bus->intr_status |= I2CD_INTR_NORMAL_STOP;
> > +        aspeed_i2c_set_state(bus, I2CD_IDLE);
> > +
> > +        break;
> > +
> > +    default:
> > +        return -1;
> > +    }
> > +
> > +    aspeed_i2c_bus_raise_interrupt(bus);
> > +
> > +    return 0;
> > +}
> > +
> > +static void aspeed_i2c_slave_send_async(I2CSlave *slave, uint8_t data)
> > +{
> > +    AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
> > +    AspeedI2CBus *bus = s->bus;
> > +
> > +    bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> > +    bus->intr_status |= I2CD_INTR_RX_DONE;
> > +
> > +    aspeed_i2c_bus_raise_interrupt(bus);
> > +}
> > +
> > +static void aspeed_i2c_slave_class_init(ObjectClass *klass, void *Data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> > +
> > +    dc->desc = "Aspeed I2C Bus Slave";
> > +
> > +    sc->event = aspeed_i2c_slave_event;
> > +    sc->send_async = aspeed_i2c_slave_send_async;
> > +}
> > +
> > +static const TypeInfo aspeed_i2c_slave_info = {
> > +    .name          = TYPE_ASPEED_I2C_SLAVE,
> > +    .parent        = TYPE_I2C_SLAVE,
> > +    .instance_size = sizeof(AspeedI2CSlave),
> > +    .class_init    = aspeed_i2c_slave_class_init,
> > +};
> > +
> >   static void aspeed_i2c_bus_reset(DeviceState *dev)
> >   {
> >       AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
> >       s->intr_ctrl = 0;
> >       s->intr_status = 0;
> > +    s->dev_addr = 0;
> 
> Please include the new reg in vmstate.
> 

Understood.

> >       s->cmd = 0;
> >       s->buf = 0;
> >       s->dma_addr = 0;
> > @@ -881,6 +955,8 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
> >       sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
> >       s->bus = i2c_init_bus(dev, name);
> > +    s->slave = ASPEED_I2C_SLAVE(i2c_slave_create_simple(s->bus, TYPE_ASPEED_I2C_SLAVE, 0xff));
> > +    s->slave->bus = s;
> >       memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
> >                             s, name, aic->reg_size);
> > @@ -1016,6 +1092,7 @@ static const TypeInfo aspeed_2600_i2c_info = {
> >   static void aspeed_i2c_register_types(void)
> >   {
> >       type_register_static(&aspeed_i2c_bus_info);
> > +    type_register_static(&aspeed_i2c_slave_info);
> >       type_register_static(&aspeed_i2c_info);
> >       type_register_static(&aspeed_2400_i2c_info);
> >       type_register_static(&aspeed_2500_i2c_info);
> > diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> > index 7d8907c1eede..85e4bddff936 100644
> > --- a/hw/i2c/trace-events
> > +++ b/hw/i2c/trace-events
> > @@ -9,7 +9,7 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
> >   # aspeed_i2c.c
> >   aspeed_i2c_bus_cmd(uint32_t cmd, const char *cmd_flags, uint32_t count, uint32_t intr_status) "handling cmd=0x%x %s count=%d intr=0x%x"
> > -aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5) "handled intr=0x%x %s%s%s%s%s"
> > +aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5, const char *str6) "handled intr=0x%x %s%s%s%s%s%s"
> >   aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
> >   aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
> >   aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send %d/%d 0x%02x"
> > diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> > index 6fb69612e064..c1c1abea41dd 100644
> > --- a/hw/misc/meson.build
> > +++ b/hw/misc/meson.build
> > @@ -122,6 +122,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
> >   softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
> > +softmmu_ss.add(when: 'CONFIG_I2C', if_true: files('i2c-echo.c'))
> 
> That's for another patch.
> 

Yeah, was a mistake on my part when chopping up my work.

> > +
> >   specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c'))
> >   specific_ss.add(when: 'CONFIG_IMX', if_true: files('imx6_src.c'))
> > diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> > index 4b9be09274c7..3f1a9c07a00b 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -42,6 +42,7 @@ struct AspeedI2CBus {
> >       SysBusDevice parent_obj;
> >       struct AspeedI2CState *controller;
> > +    struct AspeedI2CSlave *slave;
> >       MemoryRegion mr;
> > @@ -53,6 +54,7 @@ struct AspeedI2CBus {
> >       uint32_t timing[2];
> >       uint32_t intr_ctrl;
> >       uint32_t intr_status;
> > +    uint32_t dev_addr;
> >       uint32_t cmd;
> >       uint32_t buf;
> >       uint32_t pool_ctrl;
> > @@ -76,6 +78,12 @@ struct AspeedI2CState {
> >       AddressSpace dram_as;
> >   };
> > +#define TYPE_ASPEED_I2C_SLAVE "aspeed.i2c.slave"
> > +OBJECT_DECLARE_SIMPLE_TYPE(AspeedI2CSlave, ASPEED_I2C_SLAVE)
> > +struct AspeedI2CSlave {
> > +    I2CSlave i2c;
> > +    AspeedI2CBus *bus;
> > +};
> 
> AFAICT, AspeedI2CSlave is not that useful since it doesn't maintain any
> state. The QOM interface proposal looks like a better approach.
> 

Agree.

> >   struct AspeedI2CClass {
> >       SysBusDeviceClass parent_class;
> 
> 
> If you could send these 3 patches :
>   - aspeed_i2c_bus_raise_interrupt trace event rework
>   - intr_ctrl_mask class attribute
>   - dev_addr support
> 
> I will queue them quickly and we will focus on adding slave support only.
> 

Cool, but please see my question on the intr_ctrl_mask.

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

Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c

[Cédric Le Goater]

On 4/6/22 09:40, Klaus Jensen wrote:
> On Apr  6 08:14, Cédric Le Goater wrote:
>> Hello Klaus,
>>
>> On 3/31/22 18:57, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> Add slave mode functionality for the Aspeed I2C controller. This is
>>> implemented by creating an Aspeed I2C Slave device that attaches to the
>>> bus.
>>>
>>> This i2c slave device only implements the asynchronous version of
>>> i2c_send() and the event callback.
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> ---
>>>    hw/i2c/aspeed_i2c.c         | 95 +++++++++++++++++++++++++++++++++----
>>>    hw/i2c/trace-events         |  2 +-
>>>    hw/misc/meson.build         |  2 +
>>>    include/hw/i2c/aspeed_i2c.h |  8 ++++
>>>    4 files changed, 97 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>> index 03a4f5a91010..61b6424434f7 100644
>>> --- a/hw/i2c/aspeed_i2c.c
>>> +++ b/hw/i2c/aspeed_i2c.c
>>> @@ -163,10 +163,15 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>>>              bus->intr_status & I2CD_INTR_TX_NAK ? "nak|" : "",
>>>              bus->intr_status & I2CD_INTR_TX_ACK ? "ack|" : "",
>>>              bus->intr_status & I2CD_INTR_RX_DONE ? "done|" : "",
>>> +          bus->intr_status & I2CD_INTR_SLAVE_ADDR_RX_MATCH ? "slave-match|" : "",
>>>              bus->intr_status & I2CD_INTR_NORMAL_STOP ? "normal|" : "",
>>>              bus->intr_status & I2CD_INTR_ABNORMAL ? "abnormal" : "");
>>
>> Troy introduced a similar change in his "new mode" proposal. I think
>> it is time to change the 'aspeed_i2c_bus_raise_interrupt' trace event
>>
>> Could you please update trace_aspeed_i2c_bus_raise_interrupt() to take
>> a single status string ?
>>
> 
> I'm not sure it will be "prettier". But I'll give it a shot.

It will be easier to extend. We have 3 different patchsets modifying this
same event on the mailing list :)

>>> -    bus->intr_status &= bus->intr_ctrl;
>>> +    /*
>>> +     * WORKAROUND: the Linux Aspeed I2C driver masks SLAVE_ADDR_RX_MATCH for
>>> +     * some reason, not sure if it is a bug...
>>> +     */
>>> +    bus->intr_status &= (bus->intr_ctrl | I2CD_INTR_SLAVE_ADDR_RX_MATCH);
>>
>> It comes from the initial support for the AST2400 SoC.
>>
>> We should introduce a 'intr_ctrl_mask' attribute in AspeedI2CClass and
>> fix the AST24000 value to 0x7FFF ...
>>
> 
> I'm not sure I understand. Do you suggest that we always use a fixed
> mask here and disregard what the host sets in intr_ctrl?

No. sorry. There are multiple fixes required I think.

The layout of the Interrupt Control Register (0x0C) differs on the
AST2400, AST2500, AST2600 SoCs. We need to clarify that first.
bits 11:7 and 31:16 are reserved on all. AST2400 lacks bit 15 which
enables a slave timeout interrupt on AST2500 and AST2600.

Then, the Interrupt Status Register differs also. The AST2400 doesn't
have the Slave Address match indicator and the Slave Address Receiving
pending bits. On the AST2600, there are 3 possibles addresses, only
2 on the AST2500 (and only 1 on the AST2400). So that modifies the
I2CD_INTR_SLAVE_ADDR_RX_MATCH bit.

Since these 2 or 3 bits are read-only. I wonder how this is impacting
your slave implementation. is it ? If not, may be slave address match can
wait for now.
> In any case, isn't it a bug in the Linux kernel driver that it neglects
> to set bit 7 (slave match) in the INTR_CTRL register?>
> 
>>>        if (bus->intr_status) {
>>>            bus->controller->intr_status |= 1 << bus->id;
>>>            qemu_irq_raise(aic->bus_get_irq(bus));
>>> @@ -196,6 +201,9 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
>>>        case I2CD_INTR_STS_REG:
>>>            value = bus->intr_status;
>>>            break;
>>> +    case I2CD_DEV_ADDR_REG:
>>> +        value = bus->dev_addr;
>>> +        break;
>>
>> You can introduce support for this register in a preliminary patch but
>> keep the slave activation for later (I2CD_SLAVE_EN bit)
>>
> 
> Understood.

thanks, we will address the full layout of this register later when needed.
But there are fields for each address.


C.


>>>        case I2CD_POOL_CTRL_REG:
>>>            value = bus->pool_ctrl;
>>>            break;
>>> @@ -535,10 +543,9 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>>>        switch (offset) {
>>>        case I2CD_FUN_CTRL_REG:
>>>            if (value & I2CD_SLAVE_EN) {
>>> -            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>>> -                          __func__);
>>> -            break;
>>> +            i2c_slave_set_address(&bus->slave->i2c, bus->dev_addr);
>>>            }
>>> +
>>>            bus->ctrl = value & 0x0071C3FF;
>>>            break;
>>>        case I2CD_AC_TIMING_REG1:
>>> @@ -558,14 +565,19 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>>>                bus->controller->intr_status &= ~(1 << bus->id);
>>>                qemu_irq_lower(aic->bus_get_irq(bus));
>>>            }
>>> -        if (handle_rx && (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
>>> -            aspeed_i2c_handle_rx_cmd(bus);
>>> -            aspeed_i2c_bus_raise_interrupt(bus);
>>> +
>>> +        if (handle_rx) {
>>> +            if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
>>> +                aspeed_i2c_handle_rx_cmd(bus);
>>> +                aspeed_i2c_bus_raise_interrupt(bus);
>>> +            } else if (aspeed_i2c_get_state(bus) == I2CD_STXD) {
>>> +                i2c_ack(bus->bus);
>>> +            }
>>>            }
>>> +
>>>
>>>            break;
>>>        case I2CD_DEV_ADDR_REG:
>>> -        qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>>> -                      __func__);
>>> +        bus->dev_addr = value;
>>>            break;
>>>        case I2CD_POOL_CTRL_REG:
>>>            bus->pool_ctrl &= ~0xffffff;
>>> @@ -852,12 +864,74 @@ static const TypeInfo aspeed_i2c_info = {
>>>        .abstract   = true,
>>>    };
>>> +static int aspeed_i2c_slave_event(I2CSlave *slave, enum i2c_event event)
>>> +{
>>> +    AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
>>> +    AspeedI2CBus *bus = s->bus;
>>> +
>>> +    switch (event) {
>>> +    case I2C_START_SEND:
>>> +        bus->buf = bus->dev_addr << 1;
>>> +
>>> +        bus->buf &= I2CD_BYTE_BUF_RX_MASK;
>>> +        bus->buf <<= I2CD_BYTE_BUF_RX_SHIFT;
>>> +
>>> +        bus->intr_status |= (I2CD_INTR_SLAVE_ADDR_RX_MATCH | I2CD_INTR_RX_DONE);
>>> +        aspeed_i2c_set_state(bus, I2CD_STXD);
>>> +
>>> +        break;
>>> +
>>> +    case I2C_FINISH:
>>> +        bus->intr_status |= I2CD_INTR_NORMAL_STOP;
>>> +        aspeed_i2c_set_state(bus, I2CD_IDLE);
>>> +
>>> +        break;
>>> +
>>> +    default:
>>> +        return -1;
>>> +    }
>>> +
>>> +    aspeed_i2c_bus_raise_interrupt(bus);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void aspeed_i2c_slave_send_async(I2CSlave *slave, uint8_t data)
>>> +{
>>> +    AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
>>> +    AspeedI2CBus *bus = s->bus;
>>> +
>>> +    bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> +    bus->intr_status |= I2CD_INTR_RX_DONE;
>>> +
>>> +    aspeed_i2c_bus_raise_interrupt(bus);
>>> +}
>>> +
>>> +static void aspeed_i2c_slave_class_init(ObjectClass *klass, void *Data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
>>> +
>>> +    dc->desc = "Aspeed I2C Bus Slave";
>>> +
>>> +    sc->event = aspeed_i2c_slave_event;
>>> +    sc->send_async = aspeed_i2c_slave_send_async;
>>> +}
>>> +
>>> +static const TypeInfo aspeed_i2c_slave_info = {
>>> +    .name          = TYPE_ASPEED_I2C_SLAVE,
>>> +    .parent        = TYPE_I2C_SLAVE,
>>> +    .instance_size = sizeof(AspeedI2CSlave),
>>> +    .class_init    = aspeed_i2c_slave_class_init,
>>> +};
>>> +
>>>    static void aspeed_i2c_bus_reset(DeviceState *dev)
>>>    {
>>>        AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
>>>        s->intr_ctrl = 0;
>>>        s->intr_status = 0;
>>> +    s->dev_addr = 0;
>>
>> Please include the new reg in vmstate.
>>
> 
> Understood.
> 
>>>        s->cmd = 0;
>>>        s->buf = 0;
>>>        s->dma_addr = 0;
>>> @@ -881,6 +955,8 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
>>>        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>>>        s->bus = i2c_init_bus(dev, name);
>>> +    s->slave = ASPEED_I2C_SLAVE(i2c_slave_create_simple(s->bus, TYPE_ASPEED_I2C_SLAVE, 0xff));
>>> +    s->slave->bus = s;
>>>        memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
>>>                              s, name, aic->reg_size);
>>> @@ -1016,6 +1092,7 @@ static const TypeInfo aspeed_2600_i2c_info = {
>>>    static void aspeed_i2c_register_types(void)
>>>    {
>>>        type_register_static(&aspeed_i2c_bus_info);
>>> +    type_register_static(&aspeed_i2c_slave_info);
>>>        type_register_static(&aspeed_i2c_info);
>>>        type_register_static(&aspeed_2400_i2c_info);
>>>        type_register_static(&aspeed_2500_i2c_info);
>>> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
>>> index 7d8907c1eede..85e4bddff936 100644
>>> --- a/hw/i2c/trace-events
>>> +++ b/hw/i2c/trace-events
>>> @@ -9,7 +9,7 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
>>>    # aspeed_i2c.c
>>>    aspeed_i2c_bus_cmd(uint32_t cmd, const char *cmd_flags, uint32_t count, uint32_t intr_status) "handling cmd=0x%x %s count=%d intr=0x%x"
>>> -aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5) "handled intr=0x%x %s%s%s%s%s"
>>> +aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5, const char *str6) "handled intr=0x%x %s%s%s%s%s%s"
>>>    aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
>>>    aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
>>>    aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send %d/%d 0x%02x"
>>> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
>>> index 6fb69612e064..c1c1abea41dd 100644
>>> --- a/hw/misc/meson.build
>>> +++ b/hw/misc/meson.build
>>> @@ -122,6 +122,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
>>>    softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
>>> +softmmu_ss.add(when: 'CONFIG_I2C', if_true: files('i2c-echo.c'))
>>
>> That's for another patch.
>>
> 
> Yeah, was a mistake on my part when chopping up my work.
> 
>>> +
>>>    specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c'))
>>>    specific_ss.add(when: 'CONFIG_IMX', if_true: files('imx6_src.c'))
>>> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
>>> index 4b9be09274c7..3f1a9c07a00b 100644
>>> --- a/include/hw/i2c/aspeed_i2c.h
>>> +++ b/include/hw/i2c/aspeed_i2c.h
>>> @@ -42,6 +42,7 @@ struct AspeedI2CBus {
>>>        SysBusDevice parent_obj;
>>>        struct AspeedI2CState *controller;
>>> +    struct AspeedI2CSlave *slave;
>>>        MemoryRegion mr;
>>> @@ -53,6 +54,7 @@ struct AspeedI2CBus {
>>>        uint32_t timing[2];
>>>        uint32_t intr_ctrl;
>>>        uint32_t intr_status;
>>> +    uint32_t dev_addr;
>>>        uint32_t cmd;
>>>        uint32_t buf;
>>>        uint32_t pool_ctrl;
>>> @@ -76,6 +78,12 @@ struct AspeedI2CState {
>>>        AddressSpace dram_as;
>>>    };
>>> +#define TYPE_ASPEED_I2C_SLAVE "aspeed.i2c.slave"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedI2CSlave, ASPEED_I2C_SLAVE)
>>> +struct AspeedI2CSlave {
>>> +    I2CSlave i2c;
>>> +    AspeedI2CBus *bus;
>>> +};
>>
>> AFAICT, AspeedI2CSlave is not that useful since it doesn't maintain any
>> state. The QOM interface proposal looks like a better approach.
>>
> 
> Agree.
> 
>>>    struct AspeedI2CClass {
>>>        SysBusDeviceClass parent_class;
>>
>>
>> If you could send these 3 patches :
>>    - aspeed_i2c_bus_raise_interrupt trace event rework
>>    - intr_ctrl_mask class attribute
>>    - dev_addr support
>>
>> I will queue them quickly and we will focus on adding slave support only.
>>
> 
> Cool, but please see my question on the intr_ctrl_mask.

Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c

[Klaus Jensen]

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

On Apr  6 10:52, Cédric Le Goater wrote:
> On 4/6/22 09:40, Klaus Jensen wrote:
> > On Apr  6 08:14, Cédric Le Goater wrote:
> > > Hello Klaus,
> > > 
> > > On 3/31/22 18:57, Klaus Jensen wrote:
> > > > From: Klaus Jensen <k.jensen@samsung.com>
> > > > 
> > > > Add slave mode functionality for the Aspeed I2C controller. This is
> > > > implemented by creating an Aspeed I2C Slave device that attaches to the
> > > > bus.
> > > > 
> > > > This i2c slave device only implements the asynchronous version of
> > > > i2c_send() and the event callback.
> > > > 
> > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > > ---
> > > >    hw/i2c/aspeed_i2c.c         | 95 +++++++++++++++++++++++++++++++++----
> > > >    hw/i2c/trace-events         |  2 +-
> > > >    hw/misc/meson.build         |  2 +
> > > >    include/hw/i2c/aspeed_i2c.h |  8 ++++
> > > >    4 files changed, 97 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > > > index 03a4f5a91010..61b6424434f7 100644
> > > > --- a/hw/i2c/aspeed_i2c.c
> > > > +++ b/hw/i2c/aspeed_i2c.c
> > > > @@ -163,10 +163,15 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
> > > >              bus->intr_status & I2CD_INTR_TX_NAK ? "nak|" : "",
> > > >              bus->intr_status & I2CD_INTR_TX_ACK ? "ack|" : "",
> > > >              bus->intr_status & I2CD_INTR_RX_DONE ? "done|" : "",
> > > > +          bus->intr_status & I2CD_INTR_SLAVE_ADDR_RX_MATCH ? "slave-match|" : "",
> > > >              bus->intr_status & I2CD_INTR_NORMAL_STOP ? "normal|" : "",
> > > >              bus->intr_status & I2CD_INTR_ABNORMAL ? "abnormal" : "");
> > > 
> > > Troy introduced a similar change in his "new mode" proposal. I think
> > > it is time to change the 'aspeed_i2c_bus_raise_interrupt' trace event
> > > 
> > > Could you please update trace_aspeed_i2c_bus_raise_interrupt() to take
> > > a single status string ?
> > > 
> > 
> > I'm not sure it will be "prettier". But I'll give it a shot.
> 
> It will be easier to extend. We have 3 different patchsets modifying this
> same event on the mailing list :)
> 

True :)

> > > > -    bus->intr_status &= bus->intr_ctrl;
> > > > +    /*
> > > > +     * WORKAROUND: the Linux Aspeed I2C driver masks SLAVE_ADDR_RX_MATCH for
> > > > +     * some reason, not sure if it is a bug...
> > > > +     */
> > > > +    bus->intr_status &= (bus->intr_ctrl | I2CD_INTR_SLAVE_ADDR_RX_MATCH);
> > > 
> > > It comes from the initial support for the AST2400 SoC.
> > > 
> > > We should introduce a 'intr_ctrl_mask' attribute in AspeedI2CClass and
> > > fix the AST24000 value to 0x7FFF ...
> > > 
> > 
> > I'm not sure I understand. Do you suggest that we always use a fixed
> > mask here and disregard what the host sets in intr_ctrl?
> 
> No. sorry. There are multiple fixes required I think.
> 
> The layout of the Interrupt Control Register (0x0C) differs on the
> AST2400, AST2500, AST2600 SoCs. We need to clarify that first.
> bits 11:7 and 31:16 are reserved on all. AST2400 lacks bit 15 which
> enables a slave timeout interrupt on AST2500 and AST2600.
> 
> Then, the Interrupt Status Register differs also. The AST2400 doesn't
> have the Slave Address match indicator and the Slave Address Receiving
> pending bits. On the AST2600, there are 3 possibles addresses, only
> 2 on the AST2500 (and only 1 on the AST2400). So that modifies the
> I2CD_INTR_SLAVE_ADDR_RX_MATCH bit.
> 

Ugh. I'm heavily burden by the fact that I do not have a spec sheet
available... I'll try to request one from Aspeed. I reversed this from
the Linux driver, so I'm just tried to match up with how that behaves.

With Slave Address Match indicator, you mean bit 31? There is only one
bit, so how does it work with the third address?

> Since these 2 or 3 bits are read-only. I wonder how this is impacting
> your slave implementation. is it ? If not, may be slave address match can
> wait for now.

As far as I can tell, the kernel driver uses bit 7 (called
SLAVE_ADDR_RX_MATCH in QEMU and SLAVE_MATCH in Linux) to start the slave
state machinery. It does not use bit 31 (SLAVE_ADDR_MATCH in QEMU). The
naming for bit 7 in Linux is probably off and should be ranamed to match
QEMU?

I understand that this shouldn't assume to only work with the slave
machinery in Linux, but that is the only platfrom I can currently
experiment with.

> > In any case, isn't it a bug in the Linux kernel driver that it neglects
> > to set bit 7 (slave match) in the INTR_CTRL register?>
> > 
> > > >        if (bus->intr_status) {
> > > >            bus->controller->intr_status |= 1 << bus->id;
> > > >            qemu_irq_raise(aic->bus_get_irq(bus));
> > > > @@ -196,6 +201,9 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
> > > >        case I2CD_INTR_STS_REG:
> > > >            value = bus->intr_status;
> > > >            break;
> > > > +    case I2CD_DEV_ADDR_REG:
> > > > +        value = bus->dev_addr;
> > > > +        break;
> > > 
> > > You can introduce support for this register in a preliminary patch but
> > > keep the slave activation for later (I2CD_SLAVE_EN bit)
> > > 
> > 
> > Understood.
> 
> thanks, we will address the full layout of this register later when needed.
> But there are fields for each address.
>

Right. Again, I just reversed from the kernel driver and it only sets
the lower byte.

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

Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c

[Cédric Le Goater]

On 4/6/22 11:16, Klaus Jensen wrote:
> On Apr  6 10:52, Cédric Le Goater wrote:
>> On 4/6/22 09:40, Klaus Jensen wrote:
>>> On Apr  6 08:14, Cédric Le Goater wrote:
>>>> Hello Klaus,
>>>>
>>>> On 3/31/22 18:57, Klaus Jensen wrote:
>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>
>>>>> Add slave mode functionality for the Aspeed I2C controller. This is
>>>>> implemented by creating an Aspeed I2C Slave device that attaches to the
>>>>> bus.
>>>>>
>>>>> This i2c slave device only implements the asynchronous version of
>>>>> i2c_send() and the event callback.
>>>>>
>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>> ---
>>>>>     hw/i2c/aspeed_i2c.c         | 95 +++++++++++++++++++++++++++++++++----
>>>>>     hw/i2c/trace-events         |  2 +-
>>>>>     hw/misc/meson.build         |  2 +
>>>>>     include/hw/i2c/aspeed_i2c.h |  8 ++++
>>>>>     4 files changed, 97 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>>>> index 03a4f5a91010..61b6424434f7 100644
>>>>> --- a/hw/i2c/aspeed_i2c.c
>>>>> +++ b/hw/i2c/aspeed_i2c.c
>>>>> @@ -163,10 +163,15 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>>>>>               bus->intr_status & I2CD_INTR_TX_NAK ? "nak|" : "",
>>>>>               bus->intr_status & I2CD_INTR_TX_ACK ? "ack|" : "",
>>>>>               bus->intr_status & I2CD_INTR_RX_DONE ? "done|" : "",
>>>>> +          bus->intr_status & I2CD_INTR_SLAVE_ADDR_RX_MATCH ? "slave-match|" : "",
>>>>>               bus->intr_status & I2CD_INTR_NORMAL_STOP ? "normal|" : "",
>>>>>               bus->intr_status & I2CD_INTR_ABNORMAL ? "abnormal" : "");
>>>>
>>>> Troy introduced a similar change in his "new mode" proposal. I think
>>>> it is time to change the 'aspeed_i2c_bus_raise_interrupt' trace event
>>>>
>>>> Could you please update trace_aspeed_i2c_bus_raise_interrupt() to take
>>>> a single status string ?
>>>>
>>>
>>> I'm not sure it will be "prettier". But I'll give it a shot.
>>
>> It will be easier to extend. We have 3 different patchsets modifying this
>> same event on the mailing list :)
>>
> 
> True :)
> 
>>>>> -    bus->intr_status &= bus->intr_ctrl;
>>>>> +    /*
>>>>> +     * WORKAROUND: the Linux Aspeed I2C driver masks SLAVE_ADDR_RX_MATCH for
>>>>> +     * some reason, not sure if it is a bug...
>>>>> +     */
>>>>> +    bus->intr_status &= (bus->intr_ctrl | I2CD_INTR_SLAVE_ADDR_RX_MATCH);
>>>>
>>>> It comes from the initial support for the AST2400 SoC.
>>>>
>>>> We should introduce a 'intr_ctrl_mask' attribute in AspeedI2CClass and
>>>> fix the AST24000 value to 0x7FFF ...
>>>>
>>>
>>> I'm not sure I understand. Do you suggest that we always use a fixed
>>> mask here and disregard what the host sets in intr_ctrl?
>>
>> No. sorry. There are multiple fixes required I think.
>>
>> The layout of the Interrupt Control Register (0x0C) differs on the
>> AST2400, AST2500, AST2600 SoCs. We need to clarify that first.
>> bits 11:7 and 31:16 are reserved on all. AST2400 lacks bit 15 which
>> enables a slave timeout interrupt on AST2500 and AST2600.
>>
>> Then, the Interrupt Status Register differs also. The AST2400 doesn't
>> have the Slave Address match indicator and the Slave Address Receiving
>> pending bits. On the AST2600, there are 3 possibles addresses, only
>> 2 on the AST2500 (and only 1 on the AST2400). So that modifies the
>> I2CD_INTR_SLAVE_ADDR_RX_MATCH bit.
>>
> 
> Ugh. I'm heavily burden by the fact that I do not have a spec sheet
> available... I'll try to request one from Aspeed. I reversed this from
> the Linux driver, so I'm just tried to match up with how that behaves.
> 
> With Slave Address Match indicator, you mean bit 31? There is only one
> bit, so how does it work with the third address?

On the AST2400 (only 1 slave address)

   * no upper bits

On the AST2500 (2 possible slave addresses),

   * bit[31] : Slave Address match indicator
   * bit[30] : Slave Address Receiving pending

On the AST2600 (3 possible slave addresses),

   * bit[31-30] : Slave Address match indicator
   * bit[29] : Slave Address Receiving pending

>> Since these 2 or 3 bits are read-only. I wonder how this is impacting
>> your slave implementation. is it ? If not, may be slave address match can
>> wait for now.
> 
> As far as I can tell, the kernel driver uses bit 7 (called
> SLAVE_ADDR_RX_MATCH in QEMU and SLAVE_MATCH in Linux) to start the slave
> state machinery. It does not use bit 31 (SLAVE_ADDR_MATCH in QEMU). The
> naming for bit 7 in Linux is probably off and should be ranamed to match
> QEMU?

No. This is a mess :) Both are correct. bit 7 is an interrupt enable/status.
bit 31 "indicates" which address : 1, 2, 3.

> I understand that this shouldn't assume to only work with the slave
> machinery in Linux, but that is the only platfrom I can currently
> experiment with.
> 
>>> In any case, isn't it a bug in the Linux kernel driver that it neglects
>>> to set bit 7 (slave match) in the INTR_CTRL register?>
>>>
>>>>>         if (bus->intr_status) {
>>>>>             bus->controller->intr_status |= 1 << bus->id;
>>>>>             qemu_irq_raise(aic->bus_get_irq(bus));
>>>>> @@ -196,6 +201,9 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
>>>>>         case I2CD_INTR_STS_REG:
>>>>>             value = bus->intr_status;
>>>>>             break;
>>>>> +    case I2CD_DEV_ADDR_REG:
>>>>> +        value = bus->dev_addr;
>>>>> +        break;
>>>>
>>>> You can introduce support for this register in a preliminary patch but
>>>> keep the slave activation for later (I2CD_SLAVE_EN bit)
>>>>
>>>
>>> Understood.
>>
>> thanks, we will address the full layout of this register later when needed.
>> But there are fields for each address.
>>
> 
> Right. Again, I just reversed from the kernel driver and it only sets
> the lower byte.

That's fine for now. the first 7bits in each byte represents a slave address.

Thanks,

C.

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

[Peter Delevoryas]

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

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

[Klaus Jensen]

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

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.

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

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

[Peter Delevoryas]

> 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

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

[Jonathan Cameron via]

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
> 

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

[Cédric Le Goater]

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.