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

[RFC PATCH v2 0/6] 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.

v2 changes
~~~~~~~~~~
I finally got around to working on this again. I'm sorry for not
bringing a v2 to the table earlier.

Mad props to Peter and Jonathan for putting this series to work and
pushing it forward! Thanks!

This series is based off Cédric's aspeed-7.1 tree, so it includes the
register fields. This is all "old register mode", but Peter seems to
have added support in new mode.

There are some loose ends of course, i.e send_async doesn't handle
broadcast and asynchronous slaves being sent stuff can't nack. But I
wanted to get some feedback on the interface before I tackle that.

This series
~~~~~~~~~~~
Patch 1 and 2 are small Aspeed I2C changes/additions.

Patch 3 adds support for multiple masters in the i2c core, allowing
slaves to master the bus and (safely) issue i2c_send/recv().

Patch 4 adds an asynchronous send i2c_send_async(I2CBus *, uint8) on the
bus that must be paired with an explicit ack using i2c_ack(I2CBus *). We
have previously discussed how we wanted to handle the issue that some
slaves implement this and some do not. Using a QOM interface was up, but
couldn't figure out a good way to do it. I ended up decided against it
since I believe this have to be a run-time check anyway. The problem is
that a slave can master the bus and try to communicate with *anyone* on
the bus - and there is no reason why we should only allow asynchronous
slaves on the bus in that case, or whatever we would want to do when
devices are plugged. So, instead, the current master can issue an
i2c_start_send() and if that fails (because it isnt implemented by the
target slave) it can either bail out or use i2c_start_send_async() if it
itself supports it. This works the other way around as well of course,
but it is probably simpler to handle slaves that respond to
i2c_start_send(). This approach relies on adding a new i2c_event, which
is why a bunch of other devices needs changes in their event handling.

Patch 5 adds *partial* slave mode functionality to the emulated Aspeed
I2C controller, that is, it only supports asynchronous sends started by
another slave that is currently mastering the bus. No asynchronous
receive.

Finally, patch 6 adds an example device using this new API. The device
is a simple "echo" device that upon being sent a set of bytes uses the
first byte as the address of the slave to echo to.

With this combined I am able to boot up Linux on an emulated Aspeed 2600
evaluation board and have the i2c echo device write into a Linux slave
EEPROM. Assuming the echo device is on address 0x42:

  # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-15/new_device
  i2c i2c-15: new_device: Instantiated device slave-24c02 at 0x64
  # i2cset -y 15 0x42 0x64 0x00 0xaa i
  # hexdump /sys/bus/i2c/devices/15-1064/slave-eeprom
  0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
  0000010 ffff ffff ffff ffff ffff ffff ffff ffff
  *
  0000100

Klaus Jensen (6):
  hw/i2c/aspeed: rework raise interrupt trace event
  hw/i2c/aspeed: add DEV_ADDR in old register mode
  hw/i2c: support multiple masters
  hw/i2c: add asynchronous send
  hw/i2c/aspeed: add slave device in old register mode
  hw/misc: add a toy i2c echo device [DO NOT PULL]

 hw/arm/pxa2xx.c             |   2 +
 hw/display/sii9022.c        |   2 +
 hw/display/ssd0303.c        |   2 +
 hw/i2c/aspeed_i2c.c         | 152 ++++++++++++++++++++++++++++-----
 hw/i2c/core.c               |  70 +++++++++++++++-
 hw/i2c/smbus_slave.c        |   4 +
 hw/i2c/trace-events         |   4 +-
 hw/misc/i2c-echo.c          | 162 ++++++++++++++++++++++++++++++++++++
 hw/misc/ibm-cffps.c         |   2 +
 hw/misc/ir35221.c           |   2 +
 hw/misc/meson.build         |   2 +
 hw/nvram/eeprom_at24c.c     |   2 +
 hw/sensor/lsm303dlhc_mag.c  |   2 +
 include/hw/i2c/aspeed_i2c.h |  16 ++++
 include/hw/i2c/i2c.h        |  30 +++++++
 15 files changed, 428 insertions(+), 26 deletions(-)
 create mode 100644 hw/misc/i2c-echo.c

-- 
2.36.1

[RFC PATCH v2 1/6] hw/i2c/aspeed: rework raise interrupt trace event

[Klaus Jensen]

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

Build a single string instead of having several parameters on the trace
event.

Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/i2c/aspeed_i2c.c | 55 +++++++++++++++++++++++++++++++++++----------
 hw/i2c/trace-events |  2 +-
 2 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 5fce516517a5..576425898b09 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -21,6 +21,7 @@
 #include "qemu/osdep.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
+#include "qemu/cutils.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/error-report.h"
@@ -31,6 +32,9 @@
 #include "hw/registerfields.h"
 #include "trace.h"
 
+#define ASPEED_I2C_TRACE_INTR_TEMPLATE \
+    "pktdone|nak|ack|done|normal|abnormal|"
+
 static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
 {
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
@@ -38,23 +42,50 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
     uint32_t intr_ctrl_reg = aspeed_i2c_bus_intr_ctrl_offset(bus);
     bool raise_irq;
 
-    trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts],
-        aspeed_i2c_bus_pkt_mode_en(bus) &&
-        ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE) ?
-                                                               "pktdone|" : "",
-        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK) ? "nak|" : "",
-        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK) ? "ack|" : "",
-        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE) ? "done|"
-                                                                  : "",
-        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP) ?
-                                                                "normal|" : "",
-        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL) ? "abnormal"
-                                                                   : "");
+    if (trace_event_get_state_backends(TRACE_ASPEED_I2C_BUS_RAISE_INTERRUPT)) {
+        static const size_t BUF_SIZE = strlen(ASPEED_I2C_TRACE_INTR_TEMPLATE);
+        g_autofree char *buf = g_malloc0(BUF_SIZE);
+
+        /*
+         * Remember to update ASPEED_I2C_TRACE_INTR_TEMPLATE if you add a new
+         * status string.
+         */
+
+        if (aspeed_i2c_bus_pkt_mode_en(bus) &&
+            ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE)) {
+            pstrcat(buf, BUF_SIZE, "pktdone|");
+        }
+
+        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK)) {
+            pstrcat(buf, BUF_SIZE, "nak|");
+        }
+
+        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK)) {
+            pstrcat(buf, BUF_SIZE, "ack|");
+        }
+
+        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE)) {
+            pstrcat(buf, BUF_SIZE, "done|");
+        }
+
+        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP)) {
+            pstrcat(buf, BUF_SIZE, "normal|");
+        }
+
+        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL)) {
+            pstrcat(buf, BUF_SIZE, "abnormal|");
+        }
+
+        trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts], buf);
+    }
+
     raise_irq = bus->regs[reg_intr_sts] & bus->regs[intr_ctrl_reg];
+
     /* In packet mode we don't mask off INTR_STS */
     if (!aspeed_i2c_bus_pkt_mode_en(bus)) {
         bus->regs[reg_intr_sts] &= bus->regs[intr_ctrl_reg];
     }
+
     if (raise_irq) {
         bus->controller->intr_status |= 1 << bus->id;
         qemu_irq_raise(aic->bus_get_irq(bus));
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 85e4bddff936..209275ed2dc8 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, const char *str6) "handled intr=0x%x %s%s%s%s%s%s"
+aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *s) "handled intr=0x%x %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"
-- 
2.36.1

[RFC PATCH v2 2/6] hw/i2c/aspeed: add DEV_ADDR in old register mode

[Klaus Jensen]

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

Add support for writing and reading the device address register in old
register mode.

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

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 576425898b09..5a7eb5579b01 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -104,6 +104,7 @@ static uint64_t aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
     case A_I2CD_AC_TIMING2:
     case A_I2CD_INTR_CTRL:
     case A_I2CD_INTR_STS:
+    case A_I2CD_DEV_ADDR:
     case A_I2CD_POOL_CTRL:
     case A_I2CD_BYTE_BUF:
         /* Value is already set, don't do anything. */
@@ -741,8 +742,7 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
         }
         break;
     case A_I2CD_DEV_ADDR:
-        qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
-                      __func__);
+        bus->regs[R_I2CD_DEV_ADDR] = value;
         break;
     case A_I2CD_POOL_CTRL:
         bus->regs[R_I2CD_POOL_CTRL] &= ~0xffffff;
@@ -1060,6 +1060,7 @@ static void aspeed_i2c_bus_reset(DeviceState *dev)
 
     s->regs[R_I2CD_INTR_CTRL] = 0;
     s->regs[R_I2CD_INTR_STS] = 0;
+    s->regs[R_I2CD_DEV_ADDR] = 0;
     s->regs[R_I2CD_CMD] = 0;
     s->regs[R_I2CD_BYTE_BUF] = 0;
     s->regs[R_I2CD_DMA_ADDR] = 0;
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 79c6779c6c1e..03fe829a3a57 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -297,6 +297,14 @@ static inline uint32_t aspeed_i2c_bus_cmd_offset(AspeedI2CBus *bus)
     return R_I2CD_CMD;
 }
 
+static inline uint32_t aspeed_i2c_bus_dev_addr_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CS_DEV_ADDR;
+    }
+    return R_I2CD_DEV_ADDR;
+}
+
 static inline uint32_t aspeed_i2c_bus_intr_ctrl_offset(AspeedI2CBus *bus)
 {
     if (aspeed_i2c_is_new_mode(bus->controller)) {
-- 
2.36.1

[RFC PATCH v2 3/6] 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.36.1

[RFC PATCH v2 4/6] hw/i2c: add asynchronous send

[Klaus Jensen]

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

Add an asynchronous version of i2c_send() that requires the slave to
explicitly acknowledge on the bus with i2c_ack().

The current master must use the new i2c_start_send_async() to indicate
that it wants to do an asynchronous transfer. This allows the i2c core
to check if the target slave supports this or not. This approach relies
on adding a new enum i2c_event member, which is why a bunch of other
devices needs changes in their event handling switches.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/arm/pxa2xx.c            |  2 ++
 hw/display/sii9022.c       |  2 ++
 hw/display/ssd0303.c       |  2 ++
 hw/i2c/core.c              | 36 +++++++++++++++++++++++++++++++++++-
 hw/i2c/smbus_slave.c       |  4 ++++
 hw/i2c/trace-events        |  2 ++
 hw/misc/ibm-cffps.c        |  2 ++
 hw/misc/ir35221.c          |  2 ++
 hw/nvram/eeprom_at24c.c    |  2 ++
 hw/sensor/lsm303dlhc_mag.c |  2 ++
 include/hw/i2c/i2c.h       | 16 ++++++++++++++++
 11 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index f4f687df68ef..93dda83d7aa9 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1305,6 +1305,8 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
     case I2C_NACK:
         s->status |= 1 << 1;				/* set ACKNAK */
         break;
+    default:
+        return -1;
     }
     pxa2xx_i2c_update(s);
 
diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
index b591a5878901..664fd4046d82 100644
--- a/hw/display/sii9022.c
+++ b/hw/display/sii9022.c
@@ -76,6 +76,8 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
         break;
     case I2C_NACK:
         break;
+    default:
+        return -1;
     }
 
     return 0;
diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
index aeae22da9c29..d67b0ad7b529 100644
--- a/hw/display/ssd0303.c
+++ b/hw/display/ssd0303.c
@@ -196,6 +196,8 @@ static int ssd0303_event(I2CSlave *i2c, enum i2c_event event)
     case I2C_NACK:
         /* Nothing to do.  */
         break;
+    default:
+        return -1;
     }
 
     return 0;
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 145dce60782a..d4ba8146bffb 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -161,7 +161,8 @@ static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
            start condition.  */
 
         if (sc->event) {
-            trace_i2c_event("start", s->address);
+            trace_i2c_event(event == I2C_START_SEND ? "start" : "start_async",
+                            s->address);
             rv = sc->event(s, event);
             if (rv && !bus->broadcast) {
                 if (bus_scanned) {
@@ -212,6 +213,11 @@ int i2c_start_send(I2CBus *bus, uint8_t address)
     return i2c_do_start_transfer(bus, address, I2C_START_SEND);
 }
 
+int i2c_start_send_async(I2CBus *bus, uint8_t address)
+{
+    return i2c_do_start_transfer(bus, address, I2C_START_SEND_ASYNC);
+}
+
 void i2c_end_transfer(I2CBus *bus)
 {
     I2CSlaveClass *sc;
@@ -261,6 +267,23 @@ 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) {
+        return -1;
+    }
+
+    trace_i2c_send_async(slave->address, data);
+
+    sc->send_async(slave, data);
+
+    return 0;
+}
+
 uint8_t i2c_recv(I2CBus *bus)
 {
     uint8_t data = 0xff;
@@ -297,6 +320,17 @@ void i2c_nack(I2CBus *bus)
     }
 }
 
+void i2c_ack(I2CBus *bus)
+{
+    if (!bus->bh) {
+        return;
+    }
+
+    trace_i2c_ack();
+
+    qemu_bh_schedule(bus->bh);
+}
+
 static int i2c_slave_post_load(void *opaque, int version_id)
 {
     I2CSlave *dev = opaque;
diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 5d10e27664db..feb3ec633350 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -143,6 +143,10 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
             dev->mode = SMBUS_CONFUSED;
             break;
         }
+        break;
+
+    default:
+        return -1;
     }
 
     return 0;
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 209275ed2dc8..af181d43ee64 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -4,7 +4,9 @@
 
 i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
 i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
+i2c_send_async(uint8_t address, uint8_t data) "send_async(addr:0x%02x) data:0x%02x"
 i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
+i2c_ack(void) ""
 
 # aspeed_i2c.c
 
diff --git a/hw/misc/ibm-cffps.c b/hw/misc/ibm-cffps.c
index 042155bb7838..d69a727fd5f9 100644
--- a/hw/misc/ibm-cffps.c
+++ b/hw/misc/ibm-cffps.c
@@ -152,6 +152,8 @@ static int ibm_cffps_event(I2CSlave *i2c, enum i2c_event event)
     case I2C_FINISH:
          s->pointer = 0xFF;
         break;
+    default:
+        return -1;
     }
 
     s->len = 0;
diff --git a/hw/misc/ir35221.c b/hw/misc/ir35221.c
index 5e01d3655059..c46b9ee1c3bf 100644
--- a/hw/misc/ir35221.c
+++ b/hw/misc/ir35221.c
@@ -117,6 +117,8 @@ static int ir35221_event(I2CSlave *i2c, enum i2c_event event)
     case I2C_FINISH:
          s->pointer = 0xFF;
         break;
+    default:
+        return -1;
     }
 
     s->len = 0;
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 01a3093600fa..d695f6ae894a 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -75,6 +75,8 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
         break;
     case I2C_NACK:
         break;
+    default:
+        return -1;
     }
     return 0;
 }
diff --git a/hw/sensor/lsm303dlhc_mag.c b/hw/sensor/lsm303dlhc_mag.c
index 4c98ddbf207c..bb8d48b2fdb0 100644
--- a/hw/sensor/lsm303dlhc_mag.c
+++ b/hw/sensor/lsm303dlhc_mag.c
@@ -427,6 +427,8 @@ static int lsm303dlhc_mag_event(I2CSlave *i2c, enum i2c_event event)
         break;
     case I2C_NACK:
         break;
+    default:
+        return -1;
     }
 
     s->len = 0;
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index be8bb8b78a60..9b9581d23097 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -12,6 +12,7 @@
 enum i2c_event {
     I2C_START_RECV,
     I2C_START_SEND,
+    I2C_START_SEND_ASYNC,
     I2C_FINISH,
     I2C_NACK /* Masker NACKed a receive byte.  */
 };
@@ -28,6 +29,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 (asynchronous). Receiving slave must call i2c_ack(). */
+    void (*send_async)(I2CSlave *s, uint8_t data);
+
     /*
      * Slave to master.  This cannot fail, the device should always
      * return something here.
@@ -127,11 +131,23 @@ int i2c_start_recv(I2CBus *bus, uint8_t address);
  */
 int i2c_start_send(I2CBus *bus, uint8_t address);
 
+/**
+ * i2c_start_send_async: start an asynchronous 'send' transfer on an I2C bus.
+ *
+ * @bus: #I2CBus to be used
+ * @address: address of the slave
+ *
+ * Return: 0 on success, -1 on error
+ */
+int i2c_start_send_async(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.36.1

[RFC PATCH v2 5/6] hw/i2c/aspeed: add slave device in old register mode

[Klaus Jensen]

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

Add slave mode functionality for the Aspeed I2C controller in old
register mode. This is implemented by realizing an I2C slave device
owned by the I2C controller and attached to its own bus.

The I2C slave device only implements asynchronous sends on the bus, so
slaves not supporting that will not be able to communicate with it.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/i2c/aspeed_i2c.c         | 94 +++++++++++++++++++++++++++++++++----
 include/hw/i2c/aspeed_i2c.h |  8 ++++
 2 files changed, 92 insertions(+), 10 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 5a7eb5579b01..5904d5567bd2 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -33,7 +33,7 @@
 #include "trace.h"
 
 #define ASPEED_I2C_TRACE_INTR_TEMPLATE \
-    "pktdone|nak|ack|done|normal|abnormal|"
+    "pktdone|nak|ack|done|slave-match|normal|abnormal|"
 
 static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
 {
@@ -68,6 +68,10 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
             pstrcat(buf, BUF_SIZE, "done|");
         }
 
+        if (ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, SLAVE_ADDR_RX_MATCH)) {
+            pstrcat(buf, BUF_SIZE, "slave-match|");
+        }
+
         if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP)) {
             pstrcat(buf, BUF_SIZE, "normal|");
         }
@@ -710,9 +714,7 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
     switch (offset) {
     case A_I2CD_FUN_CTRL:
         if (SHARED_FIELD_EX32(value, SLAVE_EN)) {
-            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
-                          __func__);
-            break;
+            i2c_slave_set_address(bus->slave, bus->regs[R_I2CD_DEV_ADDR]);
         }
         bus->regs[R_I2CD_FUN_CTRL] = value & 0x0071C3FF;
         break;
@@ -733,12 +735,14 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
             bus->controller->intr_status &= ~(1 << bus->id);
             qemu_irq_lower(aic->bus_get_irq(bus));
         }
-        if (handle_rx && (SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD,
-                                                  M_RX_CMD) ||
-                      SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD,
-                                              M_S_RX_CMD_LAST))) {
-            aspeed_i2c_handle_rx_cmd(bus);
-            aspeed_i2c_bus_raise_interrupt(bus);
+        if (handle_rx) {
+            if (SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD, M_RX_CMD) ||
+                SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD, 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 A_I2CD_DEV_ADDR:
@@ -1054,6 +1058,73 @@ static const TypeInfo aspeed_i2c_info = {
     .abstract   = true,
 };
 
+static int aspeed_i2c_bus_slave_event(I2CSlave *slave, enum i2c_event event)
+{
+    BusState *qbus = qdev_get_parent_bus(DEVICE(slave));
+    AspeedI2CBus *bus = ASPEED_I2C_BUS(qbus->parent);
+    uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
+    uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
+    uint32_t value;
+
+    switch (event) {
+    case I2C_START_SEND_ASYNC:
+        value = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_byte_buf, TX_BUF);
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf, RX_BUF, value << 1);
+
+        ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, SLAVE_ADDR_RX_MATCH, 1);
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, RX_DONE, 1);
+
+        aspeed_i2c_set_state(bus, I2CD_STXD);
+
+        break;
+
+    case I2C_FINISH:
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, NORMAL_STOP, 1);
+
+        aspeed_i2c_set_state(bus, I2CD_IDLE);
+
+        break;
+
+    default:
+        return -1;
+    }
+
+    aspeed_i2c_bus_raise_interrupt(bus);
+
+    return 0;
+}
+
+static void aspeed_i2c_bus_slave_send_async(I2CSlave *slave, uint8_t data)
+{
+    BusState *qbus = qdev_get_parent_bus(DEVICE(slave));
+    AspeedI2CBus *bus = ASPEED_I2C_BUS(qbus->parent);
+    uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
+    uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
+
+    SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf, RX_BUF, data);
+    SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, RX_DONE, 1);
+
+    aspeed_i2c_bus_raise_interrupt(bus);
+}
+
+static void aspeed_i2c_bus_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_bus_slave_event;
+    sc->send_async = aspeed_i2c_bus_slave_send_async;
+}
+
+static const TypeInfo aspeed_i2c_bus_slave_info = {
+    .name           = TYPE_ASPEED_I2C_BUS_SLAVE,
+    .parent         = TYPE_I2C_SLAVE,
+    .instance_size  = sizeof(AspeedI2CBusSlave),
+    .class_init     = aspeed_i2c_bus_slave_class_init,
+};
+
 static void aspeed_i2c_bus_reset(DeviceState *dev)
 {
     AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
@@ -1084,6 +1155,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 = i2c_slave_create_simple(s->bus, TYPE_ASPEED_I2C_BUS_SLAVE,
+                                       0xff);
 
     memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
                           s, name, aic->reg_size);
@@ -1243,6 +1316,7 @@ static const TypeInfo aspeed_1030_i2c_info = {
 static void aspeed_i2c_register_types(void)
 {
     type_register_static(&aspeed_i2c_bus_info);
+    type_register_static(&aspeed_i2c_bus_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/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 03fe829a3a57..9e88f086131a 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -223,6 +223,9 @@ struct AspeedI2CBus {
 
     struct AspeedI2CState *controller;
 
+    /* slave mode */
+    I2CSlave *slave;
+
     MemoryRegion mr;
 
     I2CBus *bus;
@@ -251,6 +254,11 @@ struct AspeedI2CState {
     AddressSpace dram_as;
 };
 
+#define TYPE_ASPEED_I2C_BUS_SLAVE "aspeed.i2c.slave"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedI2CBusSlave, ASPEED_I2C_BUS_SLAVE)
+struct AspeedI2CBusSlave {
+    I2CSlave i2c;
+};
 
 struct AspeedI2CClass {
     SysBusDeviceClass parent_class;
-- 
2.36.1

[RFC PATCH v2 6/6] hw/misc: add a toy i2c echo device [DO NOT PULL]

[Klaus Jensen]

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

Add an example I2C device to demonstrate how a slave may master the bus
and send data asynchronously to another slave.

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  | 162 ++++++++++++++++++++++++++++++++++++++++++++
 hw/misc/meson.build |   2 +
 2 files changed, 164 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..27992eff8c5b
--- /dev/null
+++ b/hw/misc/i2c-echo.c
@@ -0,0 +1,162 @@
+#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_IDLE,
+    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_IDLE:
+        return;
+
+    case I2C_ECHO_STATE_REQUEST_MASTER:
+        i2c_bus_master(state->bus, state->bh);
+        state->state = I2C_ECHO_STATE_START_SEND;
+        return;
+
+    case I2C_ECHO_STATE_START_SEND:
+        if (i2c_start_send_async(state->bus, state->data[0])) {
+            goto release_bus;
+        }
+
+        state->pos++;
+        state->state = I2C_ECHO_STATE_ACK;
+        return;
+
+    case I2C_ECHO_STATE_ACK:
+        if (state->pos > 2) {
+            break;
+        }
+
+        if (i2c_send_async(state->bus, state->data[state->pos++])) {
+            break;
+        }
+
+        return;
+    }
+
+
+    i2c_end_transfer(state->bus);
+release_bus:
+    i2c_bus_release(state->bus);
+
+    state->state = I2C_ECHO_STATE_IDLE;
+}
+
+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;
+
+    default:
+        return -1;
+    }
+
+    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);
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 0135d1975ceb..4132fe5e0bf7 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -125,6 +125,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'))
-- 
2.36.1

Re: [RFC PATCH v2 3/6] hw/i2c: support multiple masters

[Corey Minyard]

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

On Wed, Jun 01, 2022 at 11:08:28PM +0200, Klaus Jensen wrote:
> 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
                      ^ half
> mastered the bus, the bottom halve is scheduled.
                               ^ half

"halve" is a verb that means to split something into two pieces.  Yes,
English is a strange language :).

Also, technically from an I2C point of view, masters master the bus and
slaves only respond.  The way it's phrased here and elsewhere sounds a
little strange from that point of view.

I'm ok with this patch.  It's straightforward.

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4332 bytes --]

Re: [RFC PATCH v2 4/6] hw/i2c: add asynchronous send

[Corey Minyard]

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

On Wed, Jun 01, 2022 at 11:08:29PM +0200, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Add an asynchronous version of i2c_send() that requires the slave to
> explicitly acknowledge on the bus with i2c_ack().
> 
> The current master must use the new i2c_start_send_async() to indicate
> that it wants to do an asynchronous transfer. This allows the i2c core
> to check if the target slave supports this or not. This approach relies
> on adding a new enum i2c_event member, which is why a bunch of other
> devices needs changes in their event handling switches.

This would be easier to read if you split out the default return of -1
in all the devices to a separate patch.

You've already pointed out the lack of nack support.

I think this is ok outside of that.

-corey

> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/arm/pxa2xx.c            |  2 ++
>  hw/display/sii9022.c       |  2 ++
>  hw/display/ssd0303.c       |  2 ++
>  hw/i2c/core.c              | 36 +++++++++++++++++++++++++++++++++++-
>  hw/i2c/smbus_slave.c       |  4 ++++
>  hw/i2c/trace-events        |  2 ++
>  hw/misc/ibm-cffps.c        |  2 ++
>  hw/misc/ir35221.c          |  2 ++
>  hw/nvram/eeprom_at24c.c    |  2 ++
>  hw/sensor/lsm303dlhc_mag.c |  2 ++
>  include/hw/i2c/i2c.h       | 16 ++++++++++++++++
>  11 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index f4f687df68ef..93dda83d7aa9 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -1305,6 +1305,8 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
>      case I2C_NACK:
>          s->status |= 1 << 1;				/* set ACKNAK */
>          break;
> +    default:
> +        return -1;
>      }
>      pxa2xx_i2c_update(s);
>  
> diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
> index b591a5878901..664fd4046d82 100644
> --- a/hw/display/sii9022.c
> +++ b/hw/display/sii9022.c
> @@ -76,6 +76,8 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
>          break;
>      case I2C_NACK:
>          break;
> +    default:
> +        return -1;
>      }
>  
>      return 0;
> diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
> index aeae22da9c29..d67b0ad7b529 100644
> --- a/hw/display/ssd0303.c
> +++ b/hw/display/ssd0303.c
> @@ -196,6 +196,8 @@ static int ssd0303_event(I2CSlave *i2c, enum i2c_event event)
>      case I2C_NACK:
>          /* Nothing to do.  */
>          break;
> +    default:
> +        return -1;
>      }
>  
>      return 0;
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 145dce60782a..d4ba8146bffb 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -161,7 +161,8 @@ static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
>             start condition.  */
>  
>          if (sc->event) {
> -            trace_i2c_event("start", s->address);
> +            trace_i2c_event(event == I2C_START_SEND ? "start" : "start_async",
> +                            s->address);
>              rv = sc->event(s, event);
>              if (rv && !bus->broadcast) {
>                  if (bus_scanned) {
> @@ -212,6 +213,11 @@ int i2c_start_send(I2CBus *bus, uint8_t address)
>      return i2c_do_start_transfer(bus, address, I2C_START_SEND);
>  }
>  
> +int i2c_start_send_async(I2CBus *bus, uint8_t address)
> +{
> +    return i2c_do_start_transfer(bus, address, I2C_START_SEND_ASYNC);
> +}
> +
>  void i2c_end_transfer(I2CBus *bus)
>  {
>      I2CSlaveClass *sc;
> @@ -261,6 +267,23 @@ 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) {
> +        return -1;
> +    }
> +
> +    trace_i2c_send_async(slave->address, data);
> +
> +    sc->send_async(slave, data);
> +
> +    return 0;
> +}
> +
>  uint8_t i2c_recv(I2CBus *bus)
>  {
>      uint8_t data = 0xff;
> @@ -297,6 +320,17 @@ void i2c_nack(I2CBus *bus)
>      }
>  }
>  
> +void i2c_ack(I2CBus *bus)
> +{
> +    if (!bus->bh) {
> +        return;
> +    }
> +
> +    trace_i2c_ack();
> +
> +    qemu_bh_schedule(bus->bh);
> +}
> +
>  static int i2c_slave_post_load(void *opaque, int version_id)
>  {
>      I2CSlave *dev = opaque;
> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
> index 5d10e27664db..feb3ec633350 100644
> --- a/hw/i2c/smbus_slave.c
> +++ b/hw/i2c/smbus_slave.c
> @@ -143,6 +143,10 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
>              dev->mode = SMBUS_CONFUSED;
>              break;
>          }
> +        break;
> +
> +    default:
> +        return -1;
>      }
>  
>      return 0;
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 209275ed2dc8..af181d43ee64 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -4,7 +4,9 @@
>  
>  i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
>  i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
> +i2c_send_async(uint8_t address, uint8_t data) "send_async(addr:0x%02x) data:0x%02x"
>  i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
> +i2c_ack(void) ""
>  
>  # aspeed_i2c.c
>  
> diff --git a/hw/misc/ibm-cffps.c b/hw/misc/ibm-cffps.c
> index 042155bb7838..d69a727fd5f9 100644
> --- a/hw/misc/ibm-cffps.c
> +++ b/hw/misc/ibm-cffps.c
> @@ -152,6 +152,8 @@ static int ibm_cffps_event(I2CSlave *i2c, enum i2c_event event)
>      case I2C_FINISH:
>           s->pointer = 0xFF;
>          break;
> +    default:
> +        return -1;
>      }
>  
>      s->len = 0;
> diff --git a/hw/misc/ir35221.c b/hw/misc/ir35221.c
> index 5e01d3655059..c46b9ee1c3bf 100644
> --- a/hw/misc/ir35221.c
> +++ b/hw/misc/ir35221.c
> @@ -117,6 +117,8 @@ static int ir35221_event(I2CSlave *i2c, enum i2c_event event)
>      case I2C_FINISH:
>           s->pointer = 0xFF;
>          break;
> +    default:
> +        return -1;
>      }
>  
>      s->len = 0;
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index 01a3093600fa..d695f6ae894a 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -75,6 +75,8 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
>          break;
>      case I2C_NACK:
>          break;
> +    default:
> +        return -1;
>      }
>      return 0;
>  }
> diff --git a/hw/sensor/lsm303dlhc_mag.c b/hw/sensor/lsm303dlhc_mag.c
> index 4c98ddbf207c..bb8d48b2fdb0 100644
> --- a/hw/sensor/lsm303dlhc_mag.c
> +++ b/hw/sensor/lsm303dlhc_mag.c
> @@ -427,6 +427,8 @@ static int lsm303dlhc_mag_event(I2CSlave *i2c, enum i2c_event event)
>          break;
>      case I2C_NACK:
>          break;
> +    default:
> +        return -1;
>      }
>  
>      s->len = 0;
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index be8bb8b78a60..9b9581d23097 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -12,6 +12,7 @@
>  enum i2c_event {
>      I2C_START_RECV,
>      I2C_START_SEND,
> +    I2C_START_SEND_ASYNC,
>      I2C_FINISH,
>      I2C_NACK /* Masker NACKed a receive byte.  */
>  };
> @@ -28,6 +29,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 (asynchronous). Receiving slave must call i2c_ack(). */
> +    void (*send_async)(I2CSlave *s, uint8_t data);
> +
>      /*
>       * Slave to master.  This cannot fail, the device should always
>       * return something here.
> @@ -127,11 +131,23 @@ int i2c_start_recv(I2CBus *bus, uint8_t address);
>   */
>  int i2c_start_send(I2CBus *bus, uint8_t address);
>  
> +/**
> + * i2c_start_send_async: start an asynchronous 'send' transfer on an I2C bus.
> + *
> + * @bus: #I2CBus to be used
> + * @address: address of the slave
> + *
> + * Return: 0 on success, -1 on error
> + */
> +int i2c_start_send_async(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.36.1
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4332 bytes --]

Re: [RFC PATCH v2 1/6] hw/i2c/aspeed: rework raise interrupt trace event

[Cédric Le Goater]

On 6/1/22 23:08, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Build a single string instead of having several parameters on the trace
> event.
> 
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/i2c/aspeed_i2c.c | 55 +++++++++++++++++++++++++++++++++++----------
>   hw/i2c/trace-events |  2 +-
>   2 files changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 5fce516517a5..576425898b09 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -21,6 +21,7 @@
>   #include "qemu/osdep.h"
>   #include "hw/sysbus.h"
>   #include "migration/vmstate.h"
> +#include "qemu/cutils.h"
>   #include "qemu/log.h"
>   #include "qemu/module.h"
>   #include "qemu/error-report.h"
> @@ -31,6 +32,9 @@
>   #include "hw/registerfields.h"
>   #include "trace.h"
>   
> +#define ASPEED_I2C_TRACE_INTR_TEMPLATE \
> +    "pktdone|nak|ack|done|normal|abnormal|"
> +
>   static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>   {
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
> @@ -38,23 +42,50 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>       uint32_t intr_ctrl_reg = aspeed_i2c_bus_intr_ctrl_offset(bus);
>       bool raise_irq;
>   
> -    trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts],
> -        aspeed_i2c_bus_pkt_mode_en(bus) &&
> -        ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE) ?
> -                                                               "pktdone|" : "",
> -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK) ? "nak|" : "",
> -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK) ? "ack|" : "",
> -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE) ? "done|"
> -                                                                  : "",
> -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP) ?
> -                                                                "normal|" : "",
> -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL) ? "abnormal"
> -                                                                   : "");
> +    if (trace_event_get_state_backends(TRACE_ASPEED_I2C_BUS_RAISE_INTERRUPT)) {
> +        static const size_t BUF_SIZE = strlen(ASPEED_I2C_TRACE_INTR_TEMPLATE);
> +        g_autofree char *buf = g_malloc0(BUF_SIZE);
> +
> +        /*
> +         * Remember to update ASPEED_I2C_TRACE_INTR_TEMPLATE if you add a new
> +         * status string.
> +         */
> +
> +        if (aspeed_i2c_bus_pkt_mode_en(bus) &&
> +            ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE)) {
> +            pstrcat(buf, BUF_SIZE, "pktdone|");
> +        }
> +
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK)) {
> +            pstrcat(buf, BUF_SIZE, "nak|");
> +        }
> +
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK)) {
> +            pstrcat(buf, BUF_SIZE, "ack|");
> +        }
> +
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE)) {
> +            pstrcat(buf, BUF_SIZE, "done|");
> +        }
> +
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP)) {
> +            pstrcat(buf, BUF_SIZE, "normal|");
> +        }
> +
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL)) {
> +            pstrcat(buf, BUF_SIZE, "abnormal|");
> +        }
> +
> +        trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts], buf);
> +    }
> +

How about :

     if (trace_event_get_state_backends(TRACE_ASPEED_I2C_BUS_RAISE_INTERRUPT)) {
         g_autofree char *buf = g_strdup_printf("%s%s%s%s%s%s",
                aspeed_i2c_bus_pkt_mode_en(bus) &&
                ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE) ? "pktdone|" : "",
                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK)? "nak|" : "",
                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK), "ack|" : "",
                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE) ? "done|" : "",
                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP)? "normal|" : "",
	       SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL) ? "abnormal"  : "");
	
	       trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts], buf);
     }


Thanks,

C.




>       raise_irq = bus->regs[reg_intr_sts] & bus->regs[intr_ctrl_reg];
> +
>       /* In packet mode we don't mask off INTR_STS */
>       if (!aspeed_i2c_bus_pkt_mode_en(bus)) {
>           bus->regs[reg_intr_sts] &= bus->regs[intr_ctrl_reg];
>       }
> +
>       if (raise_irq) {
>           bus->controller->intr_status |= 1 << bus->id;
>           qemu_irq_raise(aic->bus_get_irq(bus));
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 85e4bddff936..209275ed2dc8 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, const char *str6) "handled intr=0x%x %s%s%s%s%s%s"
> +aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *s) "handled intr=0x%x %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"

Re: [RFC PATCH v2 1/6] hw/i2c/aspeed: rework raise interrupt trace event

[Klaus Jensen]

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

On Jun  2 08:49, Cédric Le Goater wrote:
> On 6/1/22 23:08, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Build a single string instead of having several parameters on the trace
> > event.
> > 
> > Suggested-by: Cédric Le Goater <clg@kaod.org>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/i2c/aspeed_i2c.c | 55 +++++++++++++++++++++++++++++++++++----------
> >   hw/i2c/trace-events |  2 +-
> >   2 files changed, 44 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > index 5fce516517a5..576425898b09 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -21,6 +21,7 @@
> >   #include "qemu/osdep.h"
> >   #include "hw/sysbus.h"
> >   #include "migration/vmstate.h"
> > +#include "qemu/cutils.h"
> >   #include "qemu/log.h"
> >   #include "qemu/module.h"
> >   #include "qemu/error-report.h"
> > @@ -31,6 +32,9 @@
> >   #include "hw/registerfields.h"
> >   #include "trace.h"
> > +#define ASPEED_I2C_TRACE_INTR_TEMPLATE \
> > +    "pktdone|nak|ack|done|normal|abnormal|"
> > +
> >   static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
> >   {
> >       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
> > @@ -38,23 +42,50 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
> >       uint32_t intr_ctrl_reg = aspeed_i2c_bus_intr_ctrl_offset(bus);
> >       bool raise_irq;
> > -    trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts],
> > -        aspeed_i2c_bus_pkt_mode_en(bus) &&
> > -        ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE) ?
> > -                                                               "pktdone|" : "",
> > -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK) ? "nak|" : "",
> > -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK) ? "ack|" : "",
> > -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE) ? "done|"
> > -                                                                  : "",
> > -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP) ?
> > -                                                                "normal|" : "",
> > -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL) ? "abnormal"
> > -                                                                   : "");
> > +    if (trace_event_get_state_backends(TRACE_ASPEED_I2C_BUS_RAISE_INTERRUPT)) {
> > +        static const size_t BUF_SIZE = strlen(ASPEED_I2C_TRACE_INTR_TEMPLATE);
> > +        g_autofree char *buf = g_malloc0(BUF_SIZE);
> > +
> > +        /*
> > +         * Remember to update ASPEED_I2C_TRACE_INTR_TEMPLATE if you add a new
> > +         * status string.
> > +         */
> > +
> > +        if (aspeed_i2c_bus_pkt_mode_en(bus) &&
> > +            ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE)) {
> > +            pstrcat(buf, BUF_SIZE, "pktdone|");
> > +        }
> > +
> > +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK)) {
> > +            pstrcat(buf, BUF_SIZE, "nak|");
> > +        }
> > +
> > +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK)) {
> > +            pstrcat(buf, BUF_SIZE, "ack|");
> > +        }
> > +
> > +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE)) {
> > +            pstrcat(buf, BUF_SIZE, "done|");
> > +        }
> > +
> > +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP)) {
> > +            pstrcat(buf, BUF_SIZE, "normal|");
> > +        }
> > +
> > +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL)) {
> > +            pstrcat(buf, BUF_SIZE, "abnormal|");
> > +        }
> > +
> > +        trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts], buf);
> > +    }
> > +
> 
> How about :
> 
>     if (trace_event_get_state_backends(TRACE_ASPEED_I2C_BUS_RAISE_INTERRUPT)) {
>         g_autofree char *buf = g_strdup_printf("%s%s%s%s%s%s",
>                aspeed_i2c_bus_pkt_mode_en(bus) &&
>                ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE) ? "pktdone|" : "",
>                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK)? "nak|" : "",
>                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK), "ack|" : "",
>                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE) ? "done|" : "",
>                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP)? "normal|" : "",
> 	       SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL) ? "abnormal"  : "");
> 	
> 	       trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts], buf);
>     }
> 
> 

Uhm, yeah - that's way better :)

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

Re: [RFC PATCH v2 2/6] hw/i2c/aspeed: add DEV_ADDR in old register mode

[Cédric Le Goater]

On 6/1/22 23:08, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Add support for writing and reading the device address register in old
> register mode.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/i2c/aspeed_i2c.c         | 5 +++--
>   include/hw/i2c/aspeed_i2c.h | 8 ++++++++
>   2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 576425898b09..5a7eb5579b01 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -104,6 +104,7 @@ static uint64_t aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
>       case A_I2CD_AC_TIMING2:
>       case A_I2CD_INTR_CTRL:
>       case A_I2CD_INTR_STS:
> +    case A_I2CD_DEV_ADDR:
>       case A_I2CD_POOL_CTRL:
>       case A_I2CD_BYTE_BUF:
>           /* Value is already set, don't do anything. */
> @@ -741,8 +742,7 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
>           }
>           break;
>       case A_I2CD_DEV_ADDR:
> -        qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> -                      __func__);
> +        bus->regs[R_I2CD_DEV_ADDR] = value;
>           break;
>       case A_I2CD_POOL_CTRL:
>           bus->regs[R_I2CD_POOL_CTRL] &= ~0xffffff;
> @@ -1060,6 +1060,7 @@ static void aspeed_i2c_bus_reset(DeviceState *dev)
>   
>       s->regs[R_I2CD_INTR_CTRL] = 0;
>       s->regs[R_I2CD_INTR_STS] = 0;
> +    s->regs[R_I2CD_DEV_ADDR] = 0;
>       s->regs[R_I2CD_CMD] = 0;
>       s->regs[R_I2CD_BYTE_BUF] = 0;
>       s->regs[R_I2CD_DMA_ADDR] = 0;
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 79c6779c6c1e..03fe829a3a57 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -297,6 +297,14 @@ static inline uint32_t aspeed_i2c_bus_cmd_offset(AspeedI2CBus *bus)
>       return R_I2CD_CMD;
>   }
>   
> +static inline uint32_t aspeed_i2c_bus_dev_addr_offset(AspeedI2CBus *bus)

This routine seems unused.

Thanks,

C.

> +{
> +    if (aspeed_i2c_is_new_mode(bus->controller)) {
> +        return R_I2CS_DEV_ADDR;
> +    }
> +    return R_I2CD_DEV_ADDR;
> +}
> +
>   static inline uint32_t aspeed_i2c_bus_intr_ctrl_offset(AspeedI2CBus *bus)
>   {
>       if (aspeed_i2c_is_new_mode(bus->controller)) {

Re: [RFC PATCH v2 4/6] hw/i2c: add asynchronous send

[Cédric Le Goater]

On 6/2/22 00:05, Corey Minyard wrote:
> On Wed, Jun 01, 2022 at 11:08:29PM +0200, Klaus Jensen wrote:
>> From: Klaus Jensen <k.jensen@samsung.com>
>>
>> Add an asynchronous version of i2c_send() that requires the slave to
>> explicitly acknowledge on the bus with i2c_ack().
>>
>> The current master must use the new i2c_start_send_async() to indicate
>> that it wants to do an asynchronous transfer. This allows the i2c core
>> to check if the target slave supports this or not. This approach relies
>> on adding a new enum i2c_event member, which is why a bunch of other
>> devices needs changes in their event handling switches.
> 
> This would be easier to read if you split out the default return of -1
> in all the devices to a separate patch.

yes and please drop ibm-cffps.c and ir35221.c which are not in mainline.
I will address them myself.

Thanks,

C.


> 
> You've already pointed out the lack of nack support.
> 
> I think this is ok outside of that.
> 
> -corey
> 
>>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> ---
>>   hw/arm/pxa2xx.c            |  2 ++
>>   hw/display/sii9022.c       |  2 ++
>>   hw/display/ssd0303.c       |  2 ++
>>   hw/i2c/core.c              | 36 +++++++++++++++++++++++++++++++++++-
>>   hw/i2c/smbus_slave.c       |  4 ++++
>>   hw/i2c/trace-events        |  2 ++
>>   hw/misc/ibm-cffps.c        |  2 ++
>>   hw/misc/ir35221.c          |  2 ++
>>   hw/nvram/eeprom_at24c.c    |  2 ++
>>   hw/sensor/lsm303dlhc_mag.c |  2 ++
>>   include/hw/i2c/i2c.h       | 16 ++++++++++++++++
>>   11 files changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>> index f4f687df68ef..93dda83d7aa9 100644
>> --- a/hw/arm/pxa2xx.c
>> +++ b/hw/arm/pxa2xx.c
>> @@ -1305,6 +1305,8 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
>>       case I2C_NACK:
>>           s->status |= 1 << 1;				/* set ACKNAK */
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>       pxa2xx_i2c_update(s);
>>   
>> diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
>> index b591a5878901..664fd4046d82 100644
>> --- a/hw/display/sii9022.c
>> +++ b/hw/display/sii9022.c
>> @@ -76,6 +76,8 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
>>           break;
>>       case I2C_NACK:
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>   
>>       return 0;
>> diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
>> index aeae22da9c29..d67b0ad7b529 100644
>> --- a/hw/display/ssd0303.c
>> +++ b/hw/display/ssd0303.c
>> @@ -196,6 +196,8 @@ static int ssd0303_event(I2CSlave *i2c, enum i2c_event event)
>>       case I2C_NACK:
>>           /* Nothing to do.  */
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>   
>>       return 0;
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index 145dce60782a..d4ba8146bffb 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -161,7 +161,8 @@ static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
>>              start condition.  */
>>   
>>           if (sc->event) {
>> -            trace_i2c_event("start", s->address);
>> +            trace_i2c_event(event == I2C_START_SEND ? "start" : "start_async",
>> +                            s->address);
>>               rv = sc->event(s, event);
>>               if (rv && !bus->broadcast) {
>>                   if (bus_scanned) {
>> @@ -212,6 +213,11 @@ int i2c_start_send(I2CBus *bus, uint8_t address)
>>       return i2c_do_start_transfer(bus, address, I2C_START_SEND);
>>   }
>>   
>> +int i2c_start_send_async(I2CBus *bus, uint8_t address)
>> +{
>> +    return i2c_do_start_transfer(bus, address, I2C_START_SEND_ASYNC);
>> +}
>> +
>>   void i2c_end_transfer(I2CBus *bus)
>>   {
>>       I2CSlaveClass *sc;
>> @@ -261,6 +267,23 @@ 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) {
>> +        return -1;
>> +    }
>> +
>> +    trace_i2c_send_async(slave->address, data);
>> +
>> +    sc->send_async(slave, data);
>> +
>> +    return 0;
>> +}
>> +
>>   uint8_t i2c_recv(I2CBus *bus)
>>   {
>>       uint8_t data = 0xff;
>> @@ -297,6 +320,17 @@ void i2c_nack(I2CBus *bus)
>>       }
>>   }
>>   
>> +void i2c_ack(I2CBus *bus)
>> +{
>> +    if (!bus->bh) {
>> +        return;
>> +    }
>> +
>> +    trace_i2c_ack();
>> +
>> +    qemu_bh_schedule(bus->bh);
>> +}
>> +
>>   static int i2c_slave_post_load(void *opaque, int version_id)
>>   {
>>       I2CSlave *dev = opaque;
>> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
>> index 5d10e27664db..feb3ec633350 100644
>> --- a/hw/i2c/smbus_slave.c
>> +++ b/hw/i2c/smbus_slave.c
>> @@ -143,6 +143,10 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
>>               dev->mode = SMBUS_CONFUSED;
>>               break;
>>           }
>> +        break;
>> +
>> +    default:
>> +        return -1;
>>       }
>>   
>>       return 0;
>> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
>> index 209275ed2dc8..af181d43ee64 100644
>> --- a/hw/i2c/trace-events
>> +++ b/hw/i2c/trace-events
>> @@ -4,7 +4,9 @@
>>   
>>   i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
>>   i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
>> +i2c_send_async(uint8_t address, uint8_t data) "send_async(addr:0x%02x) data:0x%02x"
>>   i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
>> +i2c_ack(void) ""
>>   
>>   # aspeed_i2c.c
>>   
>> diff --git a/hw/misc/ibm-cffps.c b/hw/misc/ibm-cffps.c
>> index 042155bb7838..d69a727fd5f9 100644
>> --- a/hw/misc/ibm-cffps.c
>> +++ b/hw/misc/ibm-cffps.c
>> @@ -152,6 +152,8 @@ static int ibm_cffps_event(I2CSlave *i2c, enum i2c_event event)
>>       case I2C_FINISH:
>>            s->pointer = 0xFF;
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>   
>>       s->len = 0;
>> diff --git a/hw/misc/ir35221.c b/hw/misc/ir35221.c
>> index 5e01d3655059..c46b9ee1c3bf 100644
>> --- a/hw/misc/ir35221.c
>> +++ b/hw/misc/ir35221.c
>> @@ -117,6 +117,8 @@ static int ir35221_event(I2CSlave *i2c, enum i2c_event event)
>>       case I2C_FINISH:
>>            s->pointer = 0xFF;
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>   
>>       s->len = 0;
>> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
>> index 01a3093600fa..d695f6ae894a 100644
>> --- a/hw/nvram/eeprom_at24c.c
>> +++ b/hw/nvram/eeprom_at24c.c
>> @@ -75,6 +75,8 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
>>           break;
>>       case I2C_NACK:
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>       return 0;
>>   }
>> diff --git a/hw/sensor/lsm303dlhc_mag.c b/hw/sensor/lsm303dlhc_mag.c
>> index 4c98ddbf207c..bb8d48b2fdb0 100644
>> --- a/hw/sensor/lsm303dlhc_mag.c
>> +++ b/hw/sensor/lsm303dlhc_mag.c
>> @@ -427,6 +427,8 @@ static int lsm303dlhc_mag_event(I2CSlave *i2c, enum i2c_event event)
>>           break;
>>       case I2C_NACK:
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>   
>>       s->len = 0;
>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>> index be8bb8b78a60..9b9581d23097 100644
>> --- a/include/hw/i2c/i2c.h
>> +++ b/include/hw/i2c/i2c.h
>> @@ -12,6 +12,7 @@
>>   enum i2c_event {
>>       I2C_START_RECV,
>>       I2C_START_SEND,
>> +    I2C_START_SEND_ASYNC,
>>       I2C_FINISH,
>>       I2C_NACK /* Masker NACKed a receive byte.  */
>>   };
>> @@ -28,6 +29,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 (asynchronous). Receiving slave must call i2c_ack(). */
>> +    void (*send_async)(I2CSlave *s, uint8_t data);
>> +
>>       /*
>>        * Slave to master.  This cannot fail, the device should always
>>        * return something here.
>> @@ -127,11 +131,23 @@ int i2c_start_recv(I2CBus *bus, uint8_t address);
>>    */
>>   int i2c_start_send(I2CBus *bus, uint8_t address);
>>   
>> +/**
>> + * i2c_start_send_async: start an asynchronous 'send' transfer on an I2C bus.
>> + *
>> + * @bus: #I2CBus to be used
>> + * @address: address of the slave
>> + *
>> + * Return: 0 on success, -1 on error
>> + */
>> +int i2c_start_send_async(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.36.1
>>

Re: [RFC PATCH v2 2/6] hw/i2c/aspeed: add DEV_ADDR in old register mode

[Klaus Jensen]

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

On Jun  2 09:30, Cédric Le Goater wrote:
> On 6/1/22 23:08, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Add support for writing and reading the device address register in old
> > register mode.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/i2c/aspeed_i2c.c         | 5 +++--
> >   include/hw/i2c/aspeed_i2c.h | 8 ++++++++
> >   2 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > index 576425898b09..5a7eb5579b01 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -104,6 +104,7 @@ static uint64_t aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
> >       case A_I2CD_AC_TIMING2:
> >       case A_I2CD_INTR_CTRL:
> >       case A_I2CD_INTR_STS:
> > +    case A_I2CD_DEV_ADDR:
> >       case A_I2CD_POOL_CTRL:
> >       case A_I2CD_BYTE_BUF:
> >           /* Value is already set, don't do anything. */
> > @@ -741,8 +742,7 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
> >           }
> >           break;
> >       case A_I2CD_DEV_ADDR:
> > -        qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> > -                      __func__);
> > +        bus->regs[R_I2CD_DEV_ADDR] = value;
> >           break;
> >       case A_I2CD_POOL_CTRL:
> >           bus->regs[R_I2CD_POOL_CTRL] &= ~0xffffff;
> > @@ -1060,6 +1060,7 @@ static void aspeed_i2c_bus_reset(DeviceState *dev)
> >       s->regs[R_I2CD_INTR_CTRL] = 0;
> >       s->regs[R_I2CD_INTR_STS] = 0;
> > +    s->regs[R_I2CD_DEV_ADDR] = 0;
> >       s->regs[R_I2CD_CMD] = 0;
> >       s->regs[R_I2CD_BYTE_BUF] = 0;
> >       s->regs[R_I2CD_DMA_ADDR] = 0;
> > diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> > index 79c6779c6c1e..03fe829a3a57 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -297,6 +297,14 @@ static inline uint32_t aspeed_i2c_bus_cmd_offset(AspeedI2CBus *bus)
> >       return R_I2CD_CMD;
> >   }
> > +static inline uint32_t aspeed_i2c_bus_dev_addr_offset(AspeedI2CBus *bus)
> 
> This routine seems unused.
> 

It is, but I added it to align with other registers that are different
between old/new mode. But we can introduce it later if needed instead.

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

Re: [RFC PATCH v2 4/6] hw/i2c: add asynchronous send

[Klaus Jensen]

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

On Jun  2 09:32, Cédric Le Goater wrote:
> On 6/2/22 00:05, Corey Minyard wrote:
> > On Wed, Jun 01, 2022 at 11:08:29PM +0200, Klaus Jensen wrote:
> > > From: Klaus Jensen <k.jensen@samsung.com>
> > > 
> > > Add an asynchronous version of i2c_send() that requires the slave to
> > > explicitly acknowledge on the bus with i2c_ack().
> > > 
> > > The current master must use the new i2c_start_send_async() to indicate
> > > that it wants to do an asynchronous transfer. This allows the i2c core
> > > to check if the target slave supports this or not. This approach relies
> > > on adding a new enum i2c_event member, which is why a bunch of other
> > > devices needs changes in their event handling switches.
> > 
> > This would be easier to read if you split out the default return of -1
> > in all the devices to a separate patch.
> 
> yes and please drop ibm-cffps.c and ir35221.c which are not in mainline.
> I will address them myself.
> 

Roger.

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

Re: [RFC PATCH v2 6/6] hw/misc: add a toy i2c echo device [DO NOT PULL]

[Cédric Le Goater]

On 6/1/22 23:08, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Add an example I2C device to demonstrate how a slave may master the bus
> and send data asynchronously to another slave.
> 
> The device will echo whatever it is sent to the device identified by the
> first byte received.

I think this is useful and small enough to keep for the tests.

Thanks,

C.

> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/misc/i2c-echo.c  | 162 ++++++++++++++++++++++++++++++++++++++++++++
>   hw/misc/meson.build |   2 +
>   2 files changed, 164 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..27992eff8c5b
> --- /dev/null
> +++ b/hw/misc/i2c-echo.c
> @@ -0,0 +1,162 @@
> +#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_IDLE,
> +    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_IDLE:
> +        return;
> +
> +    case I2C_ECHO_STATE_REQUEST_MASTER:
> +        i2c_bus_master(state->bus, state->bh);
> +        state->state = I2C_ECHO_STATE_START_SEND;
> +        return;
> +
> +    case I2C_ECHO_STATE_START_SEND:
> +        if (i2c_start_send_async(state->bus, state->data[0])) {
> +            goto release_bus;
> +        }
> +
> +        state->pos++;
> +        state->state = I2C_ECHO_STATE_ACK;
> +        return;
> +
> +    case I2C_ECHO_STATE_ACK:
> +        if (state->pos > 2) {
> +            break;
> +        }
> +
> +        if (i2c_send_async(state->bus, state->data[state->pos++])) {
> +            break;
> +        }
> +
> +        return;
> +    }
> +
> +
> +    i2c_end_transfer(state->bus);
> +release_bus:
> +    i2c_bus_release(state->bus);
> +
> +    state->state = I2C_ECHO_STATE_IDLE;
> +}
> +
> +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;
> +
> +    default:
> +        return -1;
> +    }
> +
> +    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);
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 0135d1975ceb..4132fe5e0bf7 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -125,6 +125,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'))

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

[Cédric Le Goater]

On 6/1/22 23:08, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Hi all,
> 
> This RFC series adds I2C "slave mode" support for the Aspeed I2C

I think you can remove the RFC prefix.

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

If there are no objections, I think this is a good way to move forward
and improve this initial implementation when the need arises.

> Finally, patch 6 adds an example device using this new API. The device
> is a simple "echo" device that upon being sent a set of bytes uses the
> first byte as the address of the slave to echo to.
> 
> With this combined I am able to boot up Linux on an emulated Aspeed 2600
> evaluation board and have the i2c echo device write into a Linux slave
> EEPROM. Assuming the echo device is on address 0x42:
> 
>    # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-15/new_device
>    i2c i2c-15: new_device: Instantiated device slave-24c02 at 0x64
>    # i2cset -y 15 0x42 0x64 0x00 0xaa i
>    # hexdump /sys/bus/i2c/devices/15-1064/slave-eeprom
>    0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
>    0000010 ffff ffff ffff ffff ffff ffff ffff ffff
>    *
>    0000100

I have started working on buildroot images  :

   https://github.com/legoater/buildroot/commits/aspeed

The resulting files are quite small :

     $ ll output/images/
     total 86040
     drwxr-xr-x 2 legoater legoater     4096 Jun  1 20:01 ./
     drwxrwxr-x 6 legoater legoater     4096 Jun  1 19:40 ../
     -rwxr-xr-x 1 legoater legoater    36837 Jun  1 20:01 aspeed-ast2600-evb.dtb*
     -rw-r--r-- 1 legoater legoater 67108864 Jun  1 20:01 flash.img
     -rw-r--r-- 1 legoater legoater  6682796 Jun  1 20:01 image.itb
     -rw-r--r-- 1 legoater legoater     1846 Jun  1 20:01 image.its
     -rw-r--r-- 1 legoater legoater  3168768 Jun  1 20:01 rootfs.cpio
     -rw-r--r-- 1 legoater legoater  1026660 Jun  1 20:01 rootfs.cpio.xz
     -rw-r--r-- 1 legoater legoater  3788800 Jun  1 20:01 rootfs.tar
     -rw-r--r-- 1 legoater legoater   653777 Jun  1 20:00 u-boot.bin
     -rw-r--r-- 1 legoater legoater  5617280 Jun  1 20:01 zImage

I will probably host them on GH and we could use them under avocado
to extend the tests.


They should boot real HW. I will submit the defconfigs to buildroot
after more tests and cleanups.

Thanks

C.


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

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

[Klaus Jensen]

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

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

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

I posted this upstream to linux

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

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

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

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

Nice!

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

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

[Cédric Le Goater]

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

That's weird. I would have thought it was already enabled [ Adding Jae ]

> You mentioned something about "fixing" a mask on the ast2600?

This can be addressed later.

The model could be more precise since the driver is masking the value
already we should be fine. See commit 3fb2e2aeafb2 ("i2c: aspeed: disable
additional device addresses on ast2[56]xx") from Zeiv.

 From the datasheet.
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

Thanks,

C.

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

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

[Jae Hyun Yoo]

Hi Klaus,

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

Slave mode support in Aspeed I2C driver is already enabled and it has
worked well so far. The fix Klaus made in the link is incorrect.

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

The patch is adding ASPEED_I2CD_INTR_SLAVE_MATCH as a mask bit for
I2CD0C (Interrupt Control Register) but actually this bit is part of
I2CD10 (Interrupt Status Register). Means that the slave match interrupt
can be enabled without enabling any mask bit in I2CD0C.

Thanks,
Jae

>> You mentioned something about "fixing" a mask on the ast2600?
> 
> This can be addressed later.
> 
> The model could be more precise since the driver is masking the value
> already we should be fine. See commit 3fb2e2aeafb2 ("i2c: aspeed: disable
> additional device addresses on ast2[56]xx") from Zeiv.
> 
>  From the datasheet.
> 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
> 
> Thanks,
> 
> C.
> 
>>
>> But with the above patch, all works an intended and no "workaround"
>> required.
>>
>>>> Finally, patch 6 adds an example device using this new API. The device
>>>> is a simple "echo" device that upon being sent a set of bytes uses the
>>>> first byte as the address of the slave to echo to.
>>>>
>>>> With this combined I am able to boot up Linux on an emulated Aspeed 
>>>> 2600
>>>> evaluation board and have the i2c echo device write into a Linux slave
>>>> EEPROM. Assuming the echo device is on address 0x42:
>>>>
>>>>     # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-15/new_device
>>>>     i2c i2c-15: new_device: Instantiated device slave-24c02 at 0x64
>>>>     # i2cset -y 15 0x42 0x64 0x00 0xaa i
>>>>     # hexdump /sys/bus/i2c/devices/15-1064/slave-eeprom
>>>>     0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
>>>>     0000010 ffff ffff ffff ffff ffff ffff ffff ffff
>>>>     *
>>>>     0000100
>>>
>>> I have started working on buildroot images  :
>>>
>>>    https://github.com/legoater/buildroot/commits/aspeed
>>>
>>> The resulting files are quite small :
>>>
>>>      $ ll output/images/
>>>      total 86040
>>>      drwxr-xr-x 2 legoater legoater     4096 Jun  1 20:01 ./
>>>      drwxrwxr-x 6 legoater legoater     4096 Jun  1 19:40 ../
>>>      -rwxr-xr-x 1 legoater legoater    36837 Jun  1 20:01 
>>> aspeed-ast2600-evb.dtb*
>>>      -rw-r--r-- 1 legoater legoater 67108864 Jun  1 20:01 flash.img
>>>      -rw-r--r-- 1 legoater legoater  6682796 Jun  1 20:01 image.itb
>>>      -rw-r--r-- 1 legoater legoater     1846 Jun  1 20:01 image.its
>>>      -rw-r--r-- 1 legoater legoater  3168768 Jun  1 20:01 rootfs.cpio
>>>      -rw-r--r-- 1 legoater legoater  1026660 Jun  1 20:01 rootfs.cpio.xz
>>>      -rw-r--r-- 1 legoater legoater  3788800 Jun  1 20:01 rootfs.tar
>>>      -rw-r--r-- 1 legoater legoater   653777 Jun  1 20:00 u-boot.bin
>>>      -rw-r--r-- 1 legoater legoater  5617280 Jun  1 20:01 zImage
>>>
>>> I will probably host them on GH and we could use them under avocado
>>> to extend the tests.
>>>
>>>
>>> They should boot real HW. I will submit the defconfigs to buildroot
>>> after more tests and cleanups.
>>>
>>
>> Nice!
> 

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

[Cédric Le Goater]

On 6/2/22 16:29, Jae Hyun Yoo wrote:
> Hi Klaus,
> 
> On 6/2/2022 6:50 AM, Cédric Le Goater wrote:
>> On 6/2/22 10:21, Klaus Jensen wrote:
>>> On Jun  2 09:52, Cédric Le Goater wrote:
>>>> On 6/1/22 23:08, Klaus Jensen wrote:
>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>
>>>>> Hi all,
>>>>>
>>>>> This RFC series adds I2C "slave mode" support for the Aspeed I2C
>>>>
>>>> I think you can remove the RFC prefix.
>>>>
>>>>> controller as well as the necessary infrastructure in the i2c core to
>>>>> support this.
>>>>>
>>>>> v2 changes
>>>>> ~~~~~~~~~~
>>>>> I finally got around to working on this again. I'm sorry for not
>>>>> bringing a v2 to the table earlier.
>>>>>
>>>>> Mad props to Peter and Jonathan for putting this series to work and
>>>>> pushing it forward! Thanks!
>>>>>
>>>>> This series is based off Cédric's aspeed-7.1 tree, so it includes the
>>>>> register fields. This is all "old register mode", but Peter seems to
>>>>> have added support in new mode.
>>>>>
>>>>> There are some loose ends of course, i.e send_async doesn't handle
>>>>> broadcast and asynchronous slaves being sent stuff can't nack. But I
>>>>> wanted to get some feedback on the interface before I tackle that.
>>>>>
>>>>> This series
>>>>> ~~~~~~~~~~~
>>>>> Patch 1 and 2 are small Aspeed I2C changes/additions.
>>>>>
>>>>> Patch 3 adds support for multiple masters in the i2c core, allowing
>>>>> slaves to master the bus and (safely) issue i2c_send/recv().
>>>>>
>>>>> Patch 4 adds an asynchronous send i2c_send_async(I2CBus *, uint8) on the
>>>>> bus that must be paired with an explicit ack using i2c_ack(I2CBus *). We
>>>>> have previously discussed how we wanted to handle the issue that some
>>>>> slaves implement this and some do not. Using a QOM interface was up, but
>>>>> couldn't figure out a good way to do it. I ended up decided against it
>>>>> since I believe this have to be a run-time check anyway. The problem is
>>>>> that a slave can master the bus and try to communicate with *anyone* on
>>>>> the bus - and there is no reason why we should only allow asynchronous
>>>>> slaves on the bus in that case, or whatever we would want to do when
>>>>> devices are plugged. So, instead, the current master can issue an
>>>>> i2c_start_send() and if that fails (because it isnt implemented by the
>>>>> target slave) it can either bail out or use i2c_start_send_async() if it
>>>>> itself supports it. This works the other way around as well of course,
>>>>> but it is probably simpler to handle slaves that respond to
>>>>> i2c_start_send(). This approach relies on adding a new i2c_event, which
>>>>> is why a bunch of other devices needs changes in their event handling.
>>>>>
>>>>> Patch 5 adds *partial* slave mode functionality to the emulated Aspeed
>>>>> I2C controller, that is, it only supports asynchronous sends started by
>>>>> another slave that is currently mastering the bus. No asynchronous
>>>>> receive.
>>>>
>>>> If there are no objections, I think this is a good way to move forward
>>>> and improve this initial implementation when the need arises.
>>>>
>>>
>>> There is an outstanding issue with the SLAVE_ADDR_RX_MATCH interrupt bit
>>> (bit 7). Remember from my first series I had a workaround to make sure
>>> it wasnt masked.
>>>
>>> I posted this upstream to linux
>>>
>>> https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/
>>>
>>> Not sure if that is the right way to fix it. 
>>
>> That's weird. I would have thought it was already enabled [ Adding Jae ]
> 
> Slave mode support in Aspeed I2C driver is already enabled and it has
> worked well so far. The fix Klaus made in the link is incorrect.
> 
> https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/
> 
> The patch is adding ASPEED_I2CD_INTR_SLAVE_MATCH as a mask bit for
> I2CD0C (Interrupt Control Register) but actually this bit is part of
> I2CD10 (Interrupt Status Register). Means that the slave match interrupt
> can be enabled without enabling any mask bit in I2CD0C.

Thanks Jae.

So we should enable this interrupt always independently of the
Interrupt Control Register value.

I would simply extend the mask value (bus->regs[intr_ctrl_reg])
with the SLAVE_ADDR_RX_MATCH bit when interrupts are raised in
aspeed_i2c_bus_raise_interrupt().

Other topics,

The mask used in :

    case A_I2CD_INTR_CTRL:
         bus->regs[R_I2CD_INTR_CTRL] = value & 0x7FFF;
         break;
  
is incorrect. It should be:

   0x0000707f for ast2400
   0x0000f07f for ast2500 and ast2600

I think we can use 0x0000f07f for all to reduce a bit the
complexity.

aspeed_i2c_bus_reset() needs a fix to reset the new mode
registers also.

Thanks,
C.

> Thanks,
> Jae
> 
>>> You mentioned something about "fixing" a mask on the ast2600?
>>
>> This can be addressed later.
>>
>> The model could be more precise since the driver is masking the value
>> already we should be fine. See commit 3fb2e2aeafb2 ("i2c: aspeed: disable
>> additional device addresses on ast2[56]xx") from Zeiv.
>>
>>  From the datasheet.
>> 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
>>
>> Thanks,
>>
>> C.
>>
>>>
>>> But with the above patch, all works an intended and no "workaround"
>>> required.
>>>
>>>>> Finally, patch 6 adds an example device using this new API. The device
>>>>> is a simple "echo" device that upon being sent a set of bytes uses the
>>>>> first byte as the address of the slave to echo to.
>>>>>
>>>>> With this combined I am able to boot up Linux on an emulated Aspeed 2600
>>>>> evaluation board and have the i2c echo device write into a Linux slave
>>>>> EEPROM. Assuming the echo device is on address 0x42:
>>>>>
>>>>>     # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-15/new_device
>>>>>     i2c i2c-15: new_device: Instantiated device slave-24c02 at 0x64
>>>>>     # i2cset -y 15 0x42 0x64 0x00 0xaa i
>>>>>     # hexdump /sys/bus/i2c/devices/15-1064/slave-eeprom
>>>>>     0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
>>>>>     0000010 ffff ffff ffff ffff ffff ffff ffff ffff
>>>>>     *
>>>>>     0000100
>>>>
>>>> I have started working on buildroot images  :
>>>>
>>>>    https://github.com/legoater/buildroot/commits/aspeed
>>>>
>>>> The resulting files are quite small :
>>>>
>>>>      $ ll output/images/
>>>>      total 86040
>>>>      drwxr-xr-x 2 legoater legoater     4096 Jun  1 20:01 ./
>>>>      drwxrwxr-x 6 legoater legoater     4096 Jun  1 19:40 ../
>>>>      -rwxr-xr-x 1 legoater legoater    36837 Jun  1 20:01 aspeed-ast2600-evb.dtb*
>>>>      -rw-r--r-- 1 legoater legoater 67108864 Jun  1 20:01 flash.img
>>>>      -rw-r--r-- 1 legoater legoater  6682796 Jun  1 20:01 image.itb
>>>>      -rw-r--r-- 1 legoater legoater     1846 Jun  1 20:01 image.its
>>>>      -rw-r--r-- 1 legoater legoater  3168768 Jun  1 20:01 rootfs.cpio
>>>>      -rw-r--r-- 1 legoater legoater  1026660 Jun  1 20:01 rootfs.cpio.xz
>>>>      -rw-r--r-- 1 legoater legoater  3788800 Jun  1 20:01 rootfs.tar
>>>>      -rw-r--r-- 1 legoater legoater   653777 Jun  1 20:00 u-boot.bin
>>>>      -rw-r--r-- 1 legoater legoater  5617280 Jun  1 20:01 zImage
>>>>
>>>> I will probably host them on GH and we could use them under avocado
>>>> to extend the tests.
>>>>
>>>>
>>>> They should boot real HW. I will submit the defconfigs to buildroot
>>>> after more tests and cleanups.
>>>>
>>>
>>> Nice!
>>

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

[Klaus Jensen]

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

On Jun  2 17:40, Cédric Le Goater wrote:
> On 6/2/22 16:29, Jae Hyun Yoo wrote:
> > Hi Klaus,
> > 
> > On 6/2/2022 6:50 AM, Cédric Le Goater wrote:
> > > On 6/2/22 10:21, Klaus Jensen wrote:
> > > > 
> > > > There is an outstanding issue with the SLAVE_ADDR_RX_MATCH interrupt bit
> > > > (bit 7). Remember from my first series I had a workaround to make sure
> > > > it wasnt masked.
> > > > 
> > > > I posted this upstream to linux
> > > > 
> > > > https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/
> > > > 
> > > > Not sure if that is the right way to fix it.
> > > 
> > > That's weird. I would have thought it was already enabled [ Adding Jae ]
> > 
> > Slave mode support in Aspeed I2C driver is already enabled and it has
> > worked well so far. The fix Klaus made in the link is incorrect.
> > 
> > https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/
> > 
> > The patch is adding ASPEED_I2CD_INTR_SLAVE_MATCH as a mask bit for
> > I2CD0C (Interrupt Control Register) but actually this bit is part of
> > I2CD10 (Interrupt Status Register). Means that the slave match interrupt
> > can be enabled without enabling any mask bit in I2CD0C.
> 
> Thanks Jae.
> 
> So we should enable this interrupt always independently of the
> Interrupt Control Register value.
> 
> I would simply extend the mask value (bus->regs[intr_ctrl_reg])
> with the SLAVE_ADDR_RX_MATCH bit when interrupts are raised in
> aspeed_i2c_bus_raise_interrupt().
> 

Alright, so my "workaround" from v1 was actually the right fix - I'll
re-add it ;)


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

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

[Cédric Le Goater]

On 6/2/22 21:19, Klaus Jensen wrote:
> On Jun  2 17:40, Cédric Le Goater wrote:
>> On 6/2/22 16:29, Jae Hyun Yoo wrote:
>>> Hi Klaus,
>>>
>>> On 6/2/2022 6:50 AM, Cédric Le Goater wrote:
>>>> On 6/2/22 10:21, Klaus Jensen wrote:
>>>>>
>>>>> There is an outstanding issue with the SLAVE_ADDR_RX_MATCH interrupt bit
>>>>> (bit 7). Remember from my first series I had a workaround to make sure
>>>>> it wasnt masked.
>>>>>
>>>>> I posted this upstream to linux
>>>>>
>>>>> https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/
>>>>>
>>>>> Not sure if that is the right way to fix it.
>>>>
>>>> That's weird. I would have thought it was already enabled [ Adding Jae ]
>>>
>>> Slave mode support in Aspeed I2C driver is already enabled and it has
>>> worked well so far. The fix Klaus made in the link is incorrect.
>>>
>>> https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/
>>>
>>> The patch is adding ASPEED_I2CD_INTR_SLAVE_MATCH as a mask bit for
>>> I2CD0C (Interrupt Control Register) but actually this bit is part of
>>> I2CD10 (Interrupt Status Register). Means that the slave match interrupt
>>> can be enabled without enabling any mask bit in I2CD0C.
>>
>> Thanks Jae.
>>
>> So we should enable this interrupt always independently of the
>> Interrupt Control Register value.
>>
>> I would simply extend the mask value (bus->regs[intr_ctrl_reg])
>> with the SLAVE_ADDR_RX_MATCH bit when interrupts are raised in
>> aspeed_i2c_bus_raise_interrupt().
>>
> 
> Alright, so my "workaround" from v1 was actually the right fix - I'll
> re-add it ;)

yes :) but now we know why ! May be add a ALWAYS_ENABLE mask ?

Thanks,

C.

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

[Cédric Le Goater]

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

Uploaded here :

https://github.com/legoater/qemu-aspeed-boot/tree/master/images/ast2600-evb/buildroot-2022.05-rc2


C.