[PATCH v2 0/4] hw/i2c: Adds pca954x i2c mux switch device

[PATCH v2 0/4] hw/i2c: Adds pca954x i2c mux switch device

[Patrick Venture]

The i2c mux device pca954x implements two devices:
 - the pca9546 and pca9548.

v2:
 - the core i2c bus now calls a match method on each i2c child, which
 by default will only check for a match against itself.
 - the pca954x device overrides the i2c device match method to search
 the children for each of its buses that are active.
 - the pca954x device now owns an i2c bus for each channel, allowing
 the normal device model to attach devices to the channels.

Patrick Venture (4):
  hw/i2c: name I2CNode list in I2CBus
  hw/i2c: add match method for device search
  hw/i2c: move search to i2c_scan_bus method
  hw/i2c: add pca954x i2c-mux switch

 MAINTAINERS                      |   6 +
 hw/i2c/Kconfig                   |   4 +
 hw/i2c/core.c                    |  55 ++++--
 hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
 hw/i2c/meson.build               |   1 +
 hw/i2c/trace-events              |   5 +
 include/hw/i2c/i2c.h             |  16 +-
 include/hw/i2c/i2c_mux_pca954x.h |  19 ++
 8 files changed, 382 insertions(+), 14 deletions(-)
 create mode 100644 hw/i2c/i2c_mux_pca954x.c
 create mode 100644 include/hw/i2c/i2c_mux_pca954x.h

-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH v2 1/4] hw/i2c: name I2CNode list in I2CBus

[Patrick Venture]

To enable passing the current_devs field as a parameter, we need to use
a named struct type.

Tested: BMC firmware with i2c devices booted to userspace.

Signed-off-by: Patrick Venture <venture@google.com>
Reviewed-by: Hao Wu <wuhaotsh@google.com>
---
 include/hw/i2c/i2c.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 277dd9f2d6..1f7c268c86 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -58,9 +58,11 @@ struct I2CNode {
     QLIST_ENTRY(I2CNode) next;
 };
 
+typedef QLIST_HEAD(I2CNodeList, I2CNode) I2CNodeList;
+
 struct I2CBus {
     BusState qbus;
-    QLIST_HEAD(, I2CNode) current_devs;
+    I2CNodeList current_devs;
     uint8_t saved_address;
     bool broadcast;
 };
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH v2 2/4] hw/i2c: add match method for device search

[Patrick Venture]

At the start of an i2c transaction, the i2c bus searches its list of
children to identify which devices correspond to the address (or
broadcast).  Now the I2CSlave device has a method "match" that
encapsulates the lookup behavior. This allows the behavior to be changed
to support devices, such as i2c muxes.

Tested: A BMC firmware was booted to userspace and i2c devices were
detected.

Signed-off-by: Patrick Venture <venture@google.com>
Reviewed-by: Hao Wu <wuhaotsh@google.com>
---
 hw/i2c/core.c        | 23 +++++++++++++++++++----
 include/hw/i2c/i2c.h | 11 +++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 21ec52ac5a..d03b0eea5c 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -118,10 +118,9 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
         QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
             DeviceState *qdev = kid->child;
             I2CSlave *candidate = I2C_SLAVE(qdev);
-            if ((candidate->address == address) || (bus->broadcast)) {
-                node = g_malloc(sizeof(struct I2CNode));
-                node->elt = candidate;
-                QLIST_INSERT_HEAD(&bus->current_devs, node, next);
+            sc = I2C_SLAVE_GET_CLASS(candidate);
+            if (sc->match_and_add(candidate, address, bus->broadcast,
+                                  &bus->current_devs)) {
                 if (!bus->broadcast) {
                     break;
                 }
@@ -290,12 +289,28 @@ I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr)
     return dev;
 }
 
+static bool i2c_slave_match(I2CSlave *candidate, uint8_t address,
+                            bool broadcast, I2CNodeList *current_devs)
+{
+    if ((candidate->address == address) || (broadcast)) {
+        I2CNode *node = g_malloc(sizeof(struct I2CNode));
+        node->elt = candidate;
+        QLIST_INSERT_HEAD(current_devs, node, next);
+        return true;
+    }
+
+    /* Not found and not broadcast. */
+    return false;
+}
+
 static void i2c_slave_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
     set_bit(DEVICE_CATEGORY_MISC, k->categories);
     k->bus_type = TYPE_I2C_BUS;
     device_class_set_props(k, i2c_props);
+    sc->match_and_add = i2c_slave_match;
 }
 
 static const TypeInfo i2c_slave_type_info = {
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 1f7c268c86..9b8b95ff4a 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -16,6 +16,7 @@ enum i2c_event {
     I2C_NACK /* Masker NACKed a receive byte.  */
 };
 
+typedef struct I2CNodeList I2CNodeList;
 
 #define TYPE_I2C_SLAVE "i2c-slave"
 OBJECT_DECLARE_TYPE(I2CSlave, I2CSlaveClass,
@@ -39,6 +40,16 @@ struct I2CSlaveClass {
      * return code is not used and should be zero.
      */
     int (*event)(I2CSlave *s, enum i2c_event event);
+
+    /*
+     * Check if this device matches the address provided.  Returns bool of
+     * true if it matches (or broadcast), and updates the device list, false
+     * otherwise.
+     *
+     * If broadcast is true, match should add the device and return true.
+     */
+    bool (*match_and_add)(I2CSlave *candidate, uint8_t address, bool broadcast,
+                          I2CNodeList *current_devs);
 };
 
 struct I2CSlave {
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH v2 3/4] hw/i2c: move search to i2c_scan_bus method

[Patrick Venture]

Moves the search for matching devices on an i2c bus into a separate
method.  This allows for an object that owns an I2CBus can avoid
duplicating this method.

Tested: A BMC firmware was booted to userspace and i2c devices were
detected.

Signed-off-by: Patrick Venture <venture@google.com>
Reviewed-by: Hao Wu <wuhaotsh@google.com>
---
 hw/i2c/core.c        | 38 ++++++++++++++++++++++++++------------
 include/hw/i2c/i2c.h |  1 +
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d03b0eea5c..12981cea2d 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -77,6 +77,30 @@ int i2c_bus_busy(I2CBus *bus)
     return !QLIST_EMPTY(&bus->current_devs);
 }
 
+bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast)
+{
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
+        DeviceState *qdev = kid->child;
+        I2CSlave *candidate = I2C_SLAVE(qdev);
+        I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(candidate);
+
+        if (sc->match_and_add(candidate, address, broadcast,
+                              &bus->current_devs)) {
+            if (!broadcast) {
+                return true;
+            }
+        }
+    }
+
+    /*
+     * If broadcast was true, and the list was full or empty, return true. If
+     * broadcast was false, return false.
+     */
+    return broadcast;
+}
+
 /* TODO: Make this handle multiple masters.  */
 /*
  * Start or continue an i2c transaction.  When this is called for the
@@ -93,7 +117,6 @@ int i2c_bus_busy(I2CBus *bus)
  */
 int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
 {
-    BusChild *kid;
     I2CSlaveClass *sc;
     I2CNode *node;
     bool bus_scanned = false;
@@ -115,17 +138,8 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
      * terminating the previous transaction.
      */
     if (QLIST_EMPTY(&bus->current_devs)) {
-        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
-            DeviceState *qdev = kid->child;
-            I2CSlave *candidate = I2C_SLAVE(qdev);
-            sc = I2C_SLAVE_GET_CLASS(candidate);
-            if (sc->match_and_add(candidate, address, bus->broadcast,
-                                  &bus->current_devs)) {
-                if (!bus->broadcast) {
-                    break;
-                }
-            }
-        }
+        /* Disregard whether devices were found. */
+        (void)i2c_scan_bus(bus, address, bus->broadcast);
         bus_scanned = true;
     }
 
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 9b8b95ff4a..45915f3303 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -87,6 +87,7 @@ void i2c_nack(I2CBus *bus);
 int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
 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);
 
 /**
  * Create an I2C slave device on the heap.
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Patrick Venture]

The pca954x is an i2c mux, and this adds support for two variants of
this device: the pca9546 and pca9548.

This device is very common on BMCs to route a different channel to each
PCIe i2c bus downstream from the BMC.

Signed-off-by: Patrick Venture <venture@google.com>
Reviewed-by: Hao Wu <wuhaotsh@google.com>
Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
---
 MAINTAINERS                      |   6 +
 hw/i2c/Kconfig                   |   4 +
 hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
 hw/i2c/meson.build               |   1 +
 hw/i2c/trace-events              |   5 +
 include/hw/i2c/i2c_mux_pca954x.h |  19 ++
 6 files changed, 325 insertions(+)
 create mode 100644 hw/i2c/i2c_mux_pca954x.c
 create mode 100644 include/hw/i2c/i2c_mux_pca954x.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 58f342108e..5ea0b60b8a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2039,6 +2039,12 @@ S: Maintained
 F: hw/net/tulip.c
 F: hw/net/tulip.h
 
+pca954x
+M: Patrick Venture <venture@google.com>
+S: Maintained
+F: hw/i2c/i2c_mux_pca954x.c
+F: include/hw/i2c/i2c_mux_pca954x.h
+
 Generic Loader
 M: Alistair Francis <alistair@alistair23.me>
 S: Maintained
diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
index 09642a6dcb..8d120a25d5 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -28,3 +28,7 @@ config IMX_I2C
 config MPC_I2C
     bool
     select I2C
+
+config PCA954X
+    bool
+    select I2C
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
new file mode 100644
index 0000000000..9aa1a8872f
--- /dev/null
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -0,0 +1,290 @@
+/*
+ * I2C multiplexer for PCA954x series of I2C multiplexer/switch chips.
+ *
+ * Copyright 2021 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/i2c_mux_pca954x.h"
+#include "hw/i2c/smbus_slave.h"
+#include "hw/qdev-core.h"
+#include "hw/sysbus.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/queue.h"
+#include "qom/object.h"
+#include "trace.h"
+
+#define PCA9548_CHANNEL_COUNT 8
+#define PCA9546_CHANNEL_COUNT 4
+
+/*
+ * struct Pca954xChannel - The i2c mux device will have N of these states
+ * that own the i2c channel bus.
+ * @bus: The owned channel bus.
+ * @enabled: Is this channel active?
+ */
+typedef struct Pca954xChannel {
+    SysBusDevice parent;
+
+    I2CBus       *bus;
+
+    bool         enabled;
+} Pca954xChannel;
+
+#define TYPE_PCA954X_CHANNEL "pca954x-channel"
+#define PCA954X_CHANNEL(obj) \
+    OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
+
+/*
+ * struct Pca954xState - The pca954x state object.
+ * @control: The value written to the mux control.
+ * @channel: The set of i2c channel buses that act as channels which own the
+ * i2c children.
+ */
+typedef struct Pca954xState {
+    SMBusDevice parent;
+
+    uint8_t control;
+
+    /* The channel i2c buses. */
+    Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
+} Pca954xState;
+
+/*
+ * struct Pca954xClass - The pca954x class object.
+ * @nchans: The number of i2c channels this device has.
+ */
+typedef struct Pca954xClass {
+    SMBusDeviceClass parent;
+
+    uint8_t nchans;
+} Pca954xClass;
+
+#define TYPE_PCA954X "pca954x"
+OBJECT_DECLARE_TYPE(Pca954xState, Pca954xClass, PCA954X)
+
+/*
+ * For each channel, if it's enabled, recursively call match on those children.
+ */
+static bool pca954x_match(I2CSlave *candidate, uint8_t address,
+                          bool broadcast,
+                          I2CNodeList *current_devs)
+{
+    Pca954xState *mux = PCA954X(candidate);
+    Pca954xClass *mc = PCA954X_GET_CLASS(mux);
+    int i;
+
+    /* They are talking to the mux itself (or all devices enabled. */
+    if ((candidate->address == address) || broadcast) {
+        I2CNode *node = g_malloc(sizeof(struct I2CNode));
+        node->elt = candidate;
+        QLIST_INSERT_HEAD(current_devs, node, next);
+        if (!broadcast) {
+            return true;
+        }
+    }
+
+    for (i = 0; i < mc->nchans; i++) {
+        if (!mux->channel[i].enabled) {
+            continue;
+        }
+
+        if (i2c_scan_bus(mux->channel[i].bus, address, broadcast)) {
+            if (!broadcast) {
+                return true;
+            }
+        }
+    }
+
+    /* If we arrived here we didn't find a match, return broadcast. */
+    return broadcast;
+}
+
+static void pca954x_enable_channel(Pca954xState *s, uint8_t enable_mask)
+{
+    Pca954xClass *mc = PCA954X_GET_CLASS(s);
+    int i;
+
+    /*
+     * For each channel, check if their bit is set in enable_mask and if yes,
+     * enable it, otherwise disable, hide it.
+     */
+    for (i = 0; i < mc->nchans; i++) {
+        if (enable_mask & (1 << i)) {
+            s->channel[i].enabled = true;
+        } else {
+            s->channel[i].enabled = false;
+        }
+    }
+}
+
+static void pca954x_write(Pca954xState *s, uint8_t data)
+{
+    s->control = data;
+    pca954x_enable_channel(s, data);
+
+    trace_pca954x_write_bytes(data);
+}
+
+static int pca954x_write_data(SMBusDevice *d, uint8_t *buf, uint8_t len)
+{
+    Pca954xState *s = PCA954X(d);
+
+    if (len == 0) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: writing empty data\n", __func__);
+        return -1;
+    }
+
+    /*
+     * len should be 1, because they write one byte to enable/disable channels.
+     */
+    if (len > 1) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+            "%s: extra data after channel selection mask\n",
+            __func__);
+        return -1;
+    }
+
+    pca954x_write(s, buf[0]);
+    return 0;
+}
+
+static uint8_t pca954x_read_byte(SMBusDevice *d)
+{
+    Pca954xState *s = PCA954X(d);
+    uint8_t data = s->control;
+    trace_pca954x_read_data(data);
+    return data;
+}
+
+static void pca954x_enter_reset(Object *obj, ResetType type)
+{
+    Pca954xState *s = PCA954X(obj);
+    /* Reset will disable all channels. */
+    pca954x_write(s, 0);
+}
+
+I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel)
+{
+    Pca954xClass *pc = PCA954X_GET_CLASS(mux);
+    Pca954xState *pca954x = PCA954X(mux);
+
+    g_assert(channel < pc->nchans);
+    return I2C_BUS(qdev_get_child_bus(DEVICE(&pca954x->channel[channel]),
+                                      "i2c-bus"));
+}
+
+static void pca954x_smbus_init(Object *obj)
+{
+    Pca954xChannel *s = PCA954X_CHANNEL(obj);
+    s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
+
+    /* Start all channels as disabled. */
+    s->enabled = false;
+}
+
+static void pca954x_channel_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    dc->desc = "Pca954x Channel";
+}
+
+static void pca9546_class_init(ObjectClass *klass, void *data)
+{
+    Pca954xClass *s = PCA954X_CLASS(klass);
+    s->nchans = PCA9546_CHANNEL_COUNT;
+}
+
+static void pca9548_class_init(ObjectClass *oc, void *data)
+{
+    Pca954xClass *s = PCA954X_CLASS(oc);
+    s->nchans = PCA9548_CHANNEL_COUNT;
+}
+
+static void pca954x_realize(DeviceState *dev, Error **errp)
+{
+    Pca954xState *s = PCA954X(dev);
+    Pca954xClass *c = PCA954X_GET_CLASS(s);
+    int i;
+
+    /* SMBus modules. Cannot fail. */
+    for (i = 0; i < c->nchans; i++) {
+        Object *obj = OBJECT(&s->channel[i]);
+        sysbus_realize(SYS_BUS_DEVICE(obj), &error_abort);
+    }
+}
+
+static void pca954x_init(Object *obj)
+{
+    int i;
+    Pca954xState *s = PCA954X(obj);
+    Pca954xClass *c = PCA954X_GET_CLASS(obj);
+
+    /* Only initialize the children we expect. */
+    for (i = 0; i < c->nchans; i++) {
+        object_initialize_child(obj, "channel[*]", &s->channel[i],
+                                TYPE_PCA954X_CHANNEL);
+    }
+}
+
+static void pca954x_class_init(ObjectClass *klass, void *data)
+{
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SMBusDeviceClass *k = SMBUS_DEVICE_CLASS(klass);
+
+    sc->match_and_add = pca954x_match;
+
+    rc->phases.enter = pca954x_enter_reset;
+
+    dc->desc = "Pca954x i2c-mux";
+    dc->realize = pca954x_realize;
+
+    k->write_data = pca954x_write_data;
+    k->receive_byte = pca954x_read_byte;
+}
+
+static const TypeInfo pca954x_info[] = {
+    {
+        .name          = TYPE_PCA954X,
+        .parent        = TYPE_SMBUS_DEVICE,
+        .instance_size = sizeof(Pca954xState),
+        .instance_init = pca954x_init,
+        .class_size    = sizeof(Pca954xClass),
+        .class_init    = pca954x_class_init,
+        .abstract      = true,
+    },
+    {
+        .name          = TYPE_PCA9546,
+        .parent        = TYPE_PCA954X,
+        .class_init    = pca9546_class_init,
+    },
+    {
+        .name          = TYPE_PCA9548,
+        .parent        = TYPE_PCA954X,
+        .class_init    = pca9548_class_init,
+    },
+    {
+        .name = TYPE_PCA954X_CHANNEL,
+        .parent = TYPE_SYS_BUS_DEVICE,
+        .class_init = pca954x_channel_class_init,
+        .instance_size = sizeof(Pca954xChannel),
+        .instance_init = pca954x_smbus_init,
+    }
+};
+
+DEFINE_TYPES(pca954x_info)
diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
index cdcd694a7f..dd3aef02b2 100644
--- a/hw/i2c/meson.build
+++ b/hw/i2c/meson.build
@@ -14,4 +14,5 @@ i2c_ss.add(when: 'CONFIG_SMBUS_EEPROM', if_true: files('smbus_eeprom.c'))
 i2c_ss.add(when: 'CONFIG_VERSATILE_I2C', if_true: files('versatile_i2c.c'))
 i2c_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_i2c.c'))
 i2c_ss.add(when: 'CONFIG_PPC4XX', if_true: files('ppc4xx_i2c.c'))
+i2c_ss.add(when: 'CONFIG_PCA954X', if_true: files('i2c_mux_pca954x.c'))
 softmmu_ss.add_all(when: 'CONFIG_I2C', if_true: i2c_ss)
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 82fe6f965f..82f19e6a2d 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -26,3 +26,8 @@ npcm7xx_smbus_recv_byte(const char *id, uint8_t value) "%s recv byte: 0x%02x"
 npcm7xx_smbus_stop(const char *id) "%s stopping"
 npcm7xx_smbus_nack(const char *id) "%s nacking"
 npcm7xx_smbus_recv_fifo(const char *id, uint8_t received, uint8_t expected) "%s recv fifo: received %u, expected %u"
+
+# i2c-mux-pca954x.c
+
+pca954x_write_bytes(uint8_t value) "PCA954X write data: 0x%02x"
+pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x"
diff --git a/include/hw/i2c/i2c_mux_pca954x.h b/include/hw/i2c/i2c_mux_pca954x.h
new file mode 100644
index 0000000000..8aaf9bbc39
--- /dev/null
+++ b/include/hw/i2c/i2c_mux_pca954x.h
@@ -0,0 +1,19 @@
+#ifndef QEMU_I2C_MUX_PCA954X
+#define QEMU_I2C_MUX_PCA954X
+
+#include "hw/i2c/i2c.h"
+
+#define TYPE_PCA9546 "pca9546"
+#define TYPE_PCA9548 "pca9548"
+
+/**
+ * Retrieves the i2c bus associated with the specified channel on this i2c
+ * mux.
+ * @mux: an i2c mux device.
+ * @channel: the i2c channel requested
+ *
+ * Returns: a pointer to the associated i2c bus.
+ */
+I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel);
+
+#endif
-- 
2.31.1.295.g9ea45b61b8-goog

Re: [PATCH v2 1/4] hw/i2c: name I2CNode list in I2CBus

[Philippe Mathieu-Daudé]

On 4/9/21 6:25 PM, Patrick Venture wrote:
> To enable passing the current_devs field as a parameter, we need to use
> a named struct type.
> 
> Tested: BMC firmware with i2c devices booted to userspace.
> 
> Signed-off-by: Patrick Venture <venture@google.com>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> ---
>  include/hw/i2c/i2c.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Philippe Mathieu-Daudé]

Hi Patrick,

On 4/9/21 6:25 PM, Patrick Venture wrote:
> The pca954x is an i2c mux, and this adds support for two variants of
> this device: the pca9546 and pca9548.
> 
> This device is very common on BMCs to route a different channel to each
> PCIe i2c bus downstream from the BMC.
> 
> Signed-off-by: Patrick Venture <venture@google.com>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
> ---
>  MAINTAINERS                      |   6 +
>  hw/i2c/Kconfig                   |   4 +
>  hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
>  hw/i2c/meson.build               |   1 +
>  hw/i2c/trace-events              |   5 +
>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
>  6 files changed, 325 insertions(+)
>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h

> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> index 09642a6dcb..8d120a25d5 100644
> --- a/hw/i2c/Kconfig
> +++ b/hw/i2c/Kconfig
> @@ -28,3 +28,7 @@ config IMX_I2C
>  config MPC_I2C
>      bool
>      select I2C
> +
> +config PCA954X
> +    bool
> +    select I2C

Do you have a circular dependency when also using:

       depends on I2C

?

> +static void pca954x_realize(DeviceState *dev, Error **errp)
> +{
> +    Pca954xState *s = PCA954X(dev);
> +    Pca954xClass *c = PCA954X_GET_CLASS(s);
> +    int i;
> +
> +    /* SMBus modules. Cannot fail. */
> +    for (i = 0; i < c->nchans; i++) {
> +        Object *obj = OBJECT(&s->channel[i]);
> +        sysbus_realize(SYS_BUS_DEVICE(obj), &error_abort);

No need to cast to Object:

           sysbus_realize(SYS_BUS_DEVICE(&s->channel[i]), &error_abort);

> +    }
> +}

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Patrick Venture]

On Fri, Apr 9, 2021 at 9:51 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Patrick,
>
> On 4/9/21 6:25 PM, Patrick Venture wrote:
> > The pca954x is an i2c mux, and this adds support for two variants of
> > this device: the pca9546 and pca9548.
> >
> > This device is very common on BMCs to route a different channel to each
> > PCIe i2c bus downstream from the BMC.
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
> > ---
> >  MAINTAINERS                      |   6 +
> >  hw/i2c/Kconfig                   |   4 +
> >  hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
> >  hw/i2c/meson.build               |   1 +
> >  hw/i2c/trace-events              |   5 +
> >  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
> >  6 files changed, 325 insertions(+)
> >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
>
> > diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> > index 09642a6dcb..8d120a25d5 100644
> > --- a/hw/i2c/Kconfig
> > +++ b/hw/i2c/Kconfig
> > @@ -28,3 +28,7 @@ config IMX_I2C
> >  config MPC_I2C
> >      bool
> >      select I2C
> > +
> > +config PCA954X
> > +    bool
> > +    select I2C
>
> Do you have a circular dependency when also using:
>
>        depends on I2C
>
> ?

I'm somewhat new to qemu -- I don't know what you mean, since I2C
doesn't depend on pca954x, I don't imagine there could be a circular
dependency.

>
> > +static void pca954x_realize(DeviceState *dev, Error **errp)
> > +{
> > +    Pca954xState *s = PCA954X(dev);
> > +    Pca954xClass *c = PCA954X_GET_CLASS(s);
> > +    int i;
> > +
> > +    /* SMBus modules. Cannot fail. */
> > +    for (i = 0; i < c->nchans; i++) {
> > +        Object *obj = OBJECT(&s->channel[i]);
> > +        sysbus_realize(SYS_BUS_DEVICE(obj), &error_abort);
>
> No need to cast to Object:
>
>            sysbus_realize(SYS_BUS_DEVICE(&s->channel[i]), &error_abort);
>

Ack, will fix in v3.

> > +    }
> > +}

Re: [PATCH v2 0/4] hw/i2c: Adds pca954x i2c mux switch device

[Corey Minyard]

On Fri, Apr 09, 2021 at 09:25:41AM -0700, Patrick Venture wrote:
> The i2c mux device pca954x implements two devices:
>  - the pca9546 and pca9548.
> 
> v2:
>  - the core i2c bus now calls a match method on each i2c child, which
>  by default will only check for a match against itself.
>  - the pca954x device overrides the i2c device match method to search
>  the children for each of its buses that are active.
>  - the pca954x device now owns an i2c bus for each channel, allowing
>  the normal device model to attach devices to the channels.

I like this design.  Avoiding hacking into the bus code is a bonus.

Can these devices really have multiple channels enabled at the same
time?  That seems strange, but I guess that could be useful.

I'm not sure if you need to add a vmstate structure for this.  In
general most new devices have them; if it's ever included on an x86
system (or a system with vmstate transfer capability, probably more than
x86) that will become an issue.  I'm not sure what the expectations are,
though.

-corey

> 
> Patrick Venture (4):
>   hw/i2c: name I2CNode list in I2CBus
>   hw/i2c: add match method for device search
>   hw/i2c: move search to i2c_scan_bus method
>   hw/i2c: add pca954x i2c-mux switch
> 
>  MAINTAINERS                      |   6 +
>  hw/i2c/Kconfig                   |   4 +
>  hw/i2c/core.c                    |  55 ++++--
>  hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
>  hw/i2c/meson.build               |   1 +
>  hw/i2c/trace-events              |   5 +
>  include/hw/i2c/i2c.h             |  16 +-
>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
>  8 files changed, 382 insertions(+), 14 deletions(-)
>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> 
> -- 
> 2.31.1.295.g9ea45b61b8-goog
> 

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Corey Minyard]

On Fri, Apr 09, 2021 at 09:25:45AM -0700, Patrick Venture wrote:
> The pca954x is an i2c mux, and this adds support for two variants of
> this device: the pca9546 and pca9548.
> 
> This device is very common on BMCs to route a different channel to each
> PCIe i2c bus downstream from the BMC.
> 
> Signed-off-by: Patrick Venture <venture@google.com>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
> ---
>  MAINTAINERS                      |   6 +
>  hw/i2c/Kconfig                   |   4 +
>  hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
>  hw/i2c/meson.build               |   1 +
>  hw/i2c/trace-events              |   5 +
>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
>  6 files changed, 325 insertions(+)
>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58f342108e..5ea0b60b8a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2039,6 +2039,12 @@ S: Maintained
>  F: hw/net/tulip.c
>  F: hw/net/tulip.h
>  
> +pca954x
> +M: Patrick Venture <venture@google.com>
> +S: Maintained
> +F: hw/i2c/i2c_mux_pca954x.c
> +F: include/hw/i2c/i2c_mux_pca954x.h
> +
>  Generic Loader
>  M: Alistair Francis <alistair@alistair23.me>
>  S: Maintained
> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> index 09642a6dcb..8d120a25d5 100644
> --- a/hw/i2c/Kconfig
> +++ b/hw/i2c/Kconfig
> @@ -28,3 +28,7 @@ config IMX_I2C
>  config MPC_I2C
>      bool
>      select I2C
> +
> +config PCA954X
> +    bool
> +    select I2C
> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> new file mode 100644
> index 0000000000..9aa1a8872f
> --- /dev/null
> +++ b/hw/i2c/i2c_mux_pca954x.c
> @@ -0,0 +1,290 @@
> +/*
> + * I2C multiplexer for PCA954x series of I2C multiplexer/switch chips.
> + *
> + * Copyright 2021 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/i2c_mux_pca954x.h"
> +#include "hw/i2c/smbus_slave.h"
> +#include "hw/qdev-core.h"
> +#include "hw/sysbus.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/queue.h"
> +#include "qom/object.h"
> +#include "trace.h"
> +
> +#define PCA9548_CHANNEL_COUNT 8
> +#define PCA9546_CHANNEL_COUNT 4
> +
> +/*
> + * struct Pca954xChannel - The i2c mux device will have N of these states
> + * that own the i2c channel bus.
> + * @bus: The owned channel bus.
> + * @enabled: Is this channel active?
> + */
> +typedef struct Pca954xChannel {
> +    SysBusDevice parent;
> +
> +    I2CBus       *bus;
> +
> +    bool         enabled;
> +} Pca954xChannel;
> +
> +#define TYPE_PCA954X_CHANNEL "pca954x-channel"
> +#define PCA954X_CHANNEL(obj) \
> +    OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
> +
> +/*
> + * struct Pca954xState - The pca954x state object.
> + * @control: The value written to the mux control.
> + * @channel: The set of i2c channel buses that act as channels which own the
> + * i2c children.
> + */
> +typedef struct Pca954xState {
> +    SMBusDevice parent;
> +
> +    uint8_t control;
> +
> +    /* The channel i2c buses. */
> +    Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
> +} Pca954xState;
> +
> +/*
> + * struct Pca954xClass - The pca954x class object.
> + * @nchans: The number of i2c channels this device has.
> + */
> +typedef struct Pca954xClass {
> +    SMBusDeviceClass parent;
> +
> +    uint8_t nchans;
> +} Pca954xClass;
> +
> +#define TYPE_PCA954X "pca954x"
> +OBJECT_DECLARE_TYPE(Pca954xState, Pca954xClass, PCA954X)
> +
> +/*
> + * For each channel, if it's enabled, recursively call match on those children.
> + */
> +static bool pca954x_match(I2CSlave *candidate, uint8_t address,
> +                          bool broadcast,
> +                          I2CNodeList *current_devs)
> +{
> +    Pca954xState *mux = PCA954X(candidate);
> +    Pca954xClass *mc = PCA954X_GET_CLASS(mux);
> +    int i;
> +
> +    /* They are talking to the mux itself (or all devices enabled. */

Missing close paren in the comment above.  Really minor nit :)

> +    if ((candidate->address == address) || broadcast) {
> +        I2CNode *node = g_malloc(sizeof(struct I2CNode));
> +        node->elt = candidate;
> +        QLIST_INSERT_HEAD(current_devs, node, next);
> +        if (!broadcast) {
> +            return true;
> +        }
> +    }
> +
> +    for (i = 0; i < mc->nchans; i++) {
> +        if (!mux->channel[i].enabled) {
> +            continue;
> +        }
> +
> +        if (i2c_scan_bus(mux->channel[i].bus, address, broadcast)) {
> +            if (!broadcast) {
> +                return true;
> +            }
> +        }
> +    }
> +
> +    /* If we arrived here we didn't find a match, return broadcast. */
> +    return broadcast;
> +}
> +
> +static void pca954x_enable_channel(Pca954xState *s, uint8_t enable_mask)
> +{
> +    Pca954xClass *mc = PCA954X_GET_CLASS(s);
> +    int i;
> +
> +    /*
> +     * For each channel, check if their bit is set in enable_mask and if yes,
> +     * enable it, otherwise disable, hide it.
> +     */
> +    for (i = 0; i < mc->nchans; i++) {
> +        if (enable_mask & (1 << i)) {
> +            s->channel[i].enabled = true;
> +        } else {
> +            s->channel[i].enabled = false;
> +        }
> +    }
> +}
> +
> +static void pca954x_write(Pca954xState *s, uint8_t data)
> +{
> +    s->control = data;
> +    pca954x_enable_channel(s, data);
> +
> +    trace_pca954x_write_bytes(data);
> +}
> +
> +static int pca954x_write_data(SMBusDevice *d, uint8_t *buf, uint8_t len)
> +{
> +    Pca954xState *s = PCA954X(d);
> +
> +    if (len == 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: writing empty data\n", __func__);
> +        return -1;
> +    }
> +
> +    /*
> +     * len should be 1, because they write one byte to enable/disable channels.
> +     */
> +    if (len > 1) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "%s: extra data after channel selection mask\n",
> +            __func__);
> +        return -1;
> +    }
> +
> +    pca954x_write(s, buf[0]);
> +    return 0;
> +}
> +
> +static uint8_t pca954x_read_byte(SMBusDevice *d)
> +{
> +    Pca954xState *s = PCA954X(d);
> +    uint8_t data = s->control;
> +    trace_pca954x_read_data(data);
> +    return data;
> +}
> +
> +static void pca954x_enter_reset(Object *obj, ResetType type)
> +{
> +    Pca954xState *s = PCA954X(obj);
> +    /* Reset will disable all channels. */
> +    pca954x_write(s, 0);
> +}
> +
> +I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel)
> +{
> +    Pca954xClass *pc = PCA954X_GET_CLASS(mux);
> +    Pca954xState *pca954x = PCA954X(mux);
> +
> +    g_assert(channel < pc->nchans);
> +    return I2C_BUS(qdev_get_child_bus(DEVICE(&pca954x->channel[channel]),
> +                                      "i2c-bus"));
> +}
> +
> +static void pca954x_smbus_init(Object *obj)
> +{
> +    Pca954xChannel *s = PCA954X_CHANNEL(obj);
> +    s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
> +
> +    /* Start all channels as disabled. */
> +    s->enabled = false;
> +}
> +
> +static void pca954x_channel_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    dc->desc = "Pca954x Channel";
> +}
> +
> +static void pca9546_class_init(ObjectClass *klass, void *data)
> +{
> +    Pca954xClass *s = PCA954X_CLASS(klass);
> +    s->nchans = PCA9546_CHANNEL_COUNT;
> +}
> +
> +static void pca9548_class_init(ObjectClass *oc, void *data)
> +{
> +    Pca954xClass *s = PCA954X_CLASS(oc);
> +    s->nchans = PCA9548_CHANNEL_COUNT;
> +}
> +
> +static void pca954x_realize(DeviceState *dev, Error **errp)
> +{
> +    Pca954xState *s = PCA954X(dev);
> +    Pca954xClass *c = PCA954X_GET_CLASS(s);
> +    int i;
> +
> +    /* SMBus modules. Cannot fail. */
> +    for (i = 0; i < c->nchans; i++) {
> +        Object *obj = OBJECT(&s->channel[i]);
> +        sysbus_realize(SYS_BUS_DEVICE(obj), &error_abort);
> +    }
> +}
> +
> +static void pca954x_init(Object *obj)
> +{
> +    int i;
> +    Pca954xState *s = PCA954X(obj);
> +    Pca954xClass *c = PCA954X_GET_CLASS(obj);
> +
> +    /* Only initialize the children we expect. */
> +    for (i = 0; i < c->nchans; i++) {
> +        object_initialize_child(obj, "channel[*]", &s->channel[i],
> +                                TYPE_PCA954X_CHANNEL);
> +    }
> +}
> +
> +static void pca954x_class_init(ObjectClass *klass, void *data)
> +{
> +    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SMBusDeviceClass *k = SMBUS_DEVICE_CLASS(klass);
> +
> +    sc->match_and_add = pca954x_match;
> +
> +    rc->phases.enter = pca954x_enter_reset;
> +
> +    dc->desc = "Pca954x i2c-mux";
> +    dc->realize = pca954x_realize;
> +
> +    k->write_data = pca954x_write_data;
> +    k->receive_byte = pca954x_read_byte;
> +}
> +
> +static const TypeInfo pca954x_info[] = {
> +    {
> +        .name          = TYPE_PCA954X,
> +        .parent        = TYPE_SMBUS_DEVICE,
> +        .instance_size = sizeof(Pca954xState),
> +        .instance_init = pca954x_init,
> +        .class_size    = sizeof(Pca954xClass),
> +        .class_init    = pca954x_class_init,
> +        .abstract      = true,
> +    },
> +    {
> +        .name          = TYPE_PCA9546,
> +        .parent        = TYPE_PCA954X,
> +        .class_init    = pca9546_class_init,
> +    },
> +    {
> +        .name          = TYPE_PCA9548,
> +        .parent        = TYPE_PCA954X,
> +        .class_init    = pca9548_class_init,
> +    },
> +    {
> +        .name = TYPE_PCA954X_CHANNEL,
> +        .parent = TYPE_SYS_BUS_DEVICE,
> +        .class_init = pca954x_channel_class_init,
> +        .instance_size = sizeof(Pca954xChannel),
> +        .instance_init = pca954x_smbus_init,
> +    }
> +};
> +
> +DEFINE_TYPES(pca954x_info)
> diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
> index cdcd694a7f..dd3aef02b2 100644
> --- a/hw/i2c/meson.build
> +++ b/hw/i2c/meson.build
> @@ -14,4 +14,5 @@ i2c_ss.add(when: 'CONFIG_SMBUS_EEPROM', if_true: files('smbus_eeprom.c'))
>  i2c_ss.add(when: 'CONFIG_VERSATILE_I2C', if_true: files('versatile_i2c.c'))
>  i2c_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_i2c.c'))
>  i2c_ss.add(when: 'CONFIG_PPC4XX', if_true: files('ppc4xx_i2c.c'))
> +i2c_ss.add(when: 'CONFIG_PCA954X', if_true: files('i2c_mux_pca954x.c'))
>  softmmu_ss.add_all(when: 'CONFIG_I2C', if_true: i2c_ss)
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 82fe6f965f..82f19e6a2d 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -26,3 +26,8 @@ npcm7xx_smbus_recv_byte(const char *id, uint8_t value) "%s recv byte: 0x%02x"
>  npcm7xx_smbus_stop(const char *id) "%s stopping"
>  npcm7xx_smbus_nack(const char *id) "%s nacking"
>  npcm7xx_smbus_recv_fifo(const char *id, uint8_t received, uint8_t expected) "%s recv fifo: received %u, expected %u"
> +
> +# i2c-mux-pca954x.c
> +
> +pca954x_write_bytes(uint8_t value) "PCA954X write data: 0x%02x"
> +pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x"
> diff --git a/include/hw/i2c/i2c_mux_pca954x.h b/include/hw/i2c/i2c_mux_pca954x.h
> new file mode 100644
> index 0000000000..8aaf9bbc39
> --- /dev/null
> +++ b/include/hw/i2c/i2c_mux_pca954x.h
> @@ -0,0 +1,19 @@
> +#ifndef QEMU_I2C_MUX_PCA954X
> +#define QEMU_I2C_MUX_PCA954X
> +
> +#include "hw/i2c/i2c.h"
> +
> +#define TYPE_PCA9546 "pca9546"
> +#define TYPE_PCA9548 "pca9548"
> +
> +/**
> + * Retrieves the i2c bus associated with the specified channel on this i2c
> + * mux.
> + * @mux: an i2c mux device.
> + * @channel: the i2c channel requested
> + *
> + * Returns: a pointer to the associated i2c bus.
> + */
> +I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel);

I assume your machine-specific code will be referencing this, which is
the way it should work.

I'm happy with this.

-corey

> +
> +#endif
> -- 
> 2.31.1.295.g9ea45b61b8-goog
> 

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Patrick Venture]

On Fri, Apr 9, 2021 at 11:34 AM Corey Minyard <cminyard@mvista.com> wrote:
>
> On Fri, Apr 09, 2021 at 09:25:45AM -0700, Patrick Venture wrote:
> > The pca954x is an i2c mux, and this adds support for two variants of
> > this device: the pca9546 and pca9548.
> >
> > This device is very common on BMCs to route a different channel to each
> > PCIe i2c bus downstream from the BMC.
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
> > ---
> >  MAINTAINERS                      |   6 +
> >  hw/i2c/Kconfig                   |   4 +
> >  hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
> >  hw/i2c/meson.build               |   1 +
> >  hw/i2c/trace-events              |   5 +
> >  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
> >  6 files changed, 325 insertions(+)
> >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 58f342108e..5ea0b60b8a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2039,6 +2039,12 @@ S: Maintained
> >  F: hw/net/tulip.c
> >  F: hw/net/tulip.h
> >
> > +pca954x
> > +M: Patrick Venture <venture@google.com>
> > +S: Maintained
> > +F: hw/i2c/i2c_mux_pca954x.c
> > +F: include/hw/i2c/i2c_mux_pca954x.h
> > +
> >  Generic Loader
> >  M: Alistair Francis <alistair@alistair23.me>
> >  S: Maintained
> > diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> > index 09642a6dcb..8d120a25d5 100644
> > --- a/hw/i2c/Kconfig
> > +++ b/hw/i2c/Kconfig
> > @@ -28,3 +28,7 @@ config IMX_I2C
> >  config MPC_I2C
> >      bool
> >      select I2C
> > +
> > +config PCA954X
> > +    bool
> > +    select I2C
> > diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> > new file mode 100644
> > index 0000000000..9aa1a8872f
> > --- /dev/null
> > +++ b/hw/i2c/i2c_mux_pca954x.c
> > @@ -0,0 +1,290 @@
> > +/*
> > + * I2C multiplexer for PCA954x series of I2C multiplexer/switch chips.
> > + *
> > + * Copyright 2021 Google LLC
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> > + * for more details.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "hw/i2c/i2c.h"
> > +#include "hw/i2c/i2c_mux_pca954x.h"
> > +#include "hw/i2c/smbus_slave.h"
> > +#include "hw/qdev-core.h"
> > +#include "hw/sysbus.h"
> > +#include "qemu/log.h"
> > +#include "qemu/module.h"
> > +#include "qemu/queue.h"
> > +#include "qom/object.h"
> > +#include "trace.h"
> > +
> > +#define PCA9548_CHANNEL_COUNT 8
> > +#define PCA9546_CHANNEL_COUNT 4
> > +
> > +/*
> > + * struct Pca954xChannel - The i2c mux device will have N of these states
> > + * that own the i2c channel bus.
> > + * @bus: The owned channel bus.
> > + * @enabled: Is this channel active?
> > + */
> > +typedef struct Pca954xChannel {
> > +    SysBusDevice parent;
> > +
> > +    I2CBus       *bus;
> > +
> > +    bool         enabled;
> > +} Pca954xChannel;
> > +
> > +#define TYPE_PCA954X_CHANNEL "pca954x-channel"
> > +#define PCA954X_CHANNEL(obj) \
> > +    OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
> > +
> > +/*
> > + * struct Pca954xState - The pca954x state object.
> > + * @control: The value written to the mux control.
> > + * @channel: The set of i2c channel buses that act as channels which own the
> > + * i2c children.
> > + */
> > +typedef struct Pca954xState {
> > +    SMBusDevice parent;
> > +
> > +    uint8_t control;
> > +
> > +    /* The channel i2c buses. */
> > +    Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
> > +} Pca954xState;
> > +
> > +/*
> > + * struct Pca954xClass - The pca954x class object.
> > + * @nchans: The number of i2c channels this device has.
> > + */
> > +typedef struct Pca954xClass {
> > +    SMBusDeviceClass parent;
> > +
> > +    uint8_t nchans;
> > +} Pca954xClass;
> > +
> > +#define TYPE_PCA954X "pca954x"
> > +OBJECT_DECLARE_TYPE(Pca954xState, Pca954xClass, PCA954X)
> > +
> > +/*
> > + * For each channel, if it's enabled, recursively call match on those children.
> > + */
> > +static bool pca954x_match(I2CSlave *candidate, uint8_t address,
> > +                          bool broadcast,
> > +                          I2CNodeList *current_devs)
> > +{
> > +    Pca954xState *mux = PCA954X(candidate);
> > +    Pca954xClass *mc = PCA954X_GET_CLASS(mux);
> > +    int i;
> > +
> > +    /* They are talking to the mux itself (or all devices enabled. */
>
> Missing close paren in the comment above.  Really minor nit :)

Ack, will fix in v3.

>
> > +    if ((candidate->address == address) || broadcast) {
> > +        I2CNode *node = g_malloc(sizeof(struct I2CNode));
> > +        node->elt = candidate;
> > +        QLIST_INSERT_HEAD(current_devs, node, next);
> > +        if (!broadcast) {
> > +            return true;
> > +        }
> > +    }
> > +
> > +    for (i = 0; i < mc->nchans; i++) {
> > +        if (!mux->channel[i].enabled) {
> > +            continue;
> > +        }
> > +
> > +        if (i2c_scan_bus(mux->channel[i].bus, address, broadcast)) {
> > +            if (!broadcast) {
> > +                return true;
> > +            }
> > +        }
> > +    }
> > +
> > +    /* If we arrived here we didn't find a match, return broadcast. */
> > +    return broadcast;
> > +}
> > +
> > +static void pca954x_enable_channel(Pca954xState *s, uint8_t enable_mask)
> > +{
> > +    Pca954xClass *mc = PCA954X_GET_CLASS(s);
> > +    int i;
> > +
> > +    /*
> > +     * For each channel, check if their bit is set in enable_mask and if yes,
> > +     * enable it, otherwise disable, hide it.
> > +     */
> > +    for (i = 0; i < mc->nchans; i++) {
> > +        if (enable_mask & (1 << i)) {
> > +            s->channel[i].enabled = true;
> > +        } else {
> > +            s->channel[i].enabled = false;
> > +        }
> > +    }
> > +}
> > +
> > +static void pca954x_write(Pca954xState *s, uint8_t data)
> > +{
> > +    s->control = data;
> > +    pca954x_enable_channel(s, data);
> > +
> > +    trace_pca954x_write_bytes(data);
> > +}
> > +
> > +static int pca954x_write_data(SMBusDevice *d, uint8_t *buf, uint8_t len)
> > +{
> > +    Pca954xState *s = PCA954X(d);
> > +
> > +    if (len == 0) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: writing empty data\n", __func__);
> > +        return -1;
> > +    }
> > +
> > +    /*
> > +     * len should be 1, because they write one byte to enable/disable channels.
> > +     */
> > +    if (len > 1) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +            "%s: extra data after channel selection mask\n",
> > +            __func__);
> > +        return -1;
> > +    }
> > +
> > +    pca954x_write(s, buf[0]);
> > +    return 0;
> > +}
> > +
> > +static uint8_t pca954x_read_byte(SMBusDevice *d)
> > +{
> > +    Pca954xState *s = PCA954X(d);
> > +    uint8_t data = s->control;
> > +    trace_pca954x_read_data(data);
> > +    return data;
> > +}
> > +
> > +static void pca954x_enter_reset(Object *obj, ResetType type)
> > +{
> > +    Pca954xState *s = PCA954X(obj);
> > +    /* Reset will disable all channels. */
> > +    pca954x_write(s, 0);
> > +}
> > +
> > +I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel)
> > +{
> > +    Pca954xClass *pc = PCA954X_GET_CLASS(mux);
> > +    Pca954xState *pca954x = PCA954X(mux);
> > +
> > +    g_assert(channel < pc->nchans);
> > +    return I2C_BUS(qdev_get_child_bus(DEVICE(&pca954x->channel[channel]),
> > +                                      "i2c-bus"));
> > +}
> > +
> > +static void pca954x_smbus_init(Object *obj)
> > +{
> > +    Pca954xChannel *s = PCA954X_CHANNEL(obj);
> > +    s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
> > +
> > +    /* Start all channels as disabled. */
> > +    s->enabled = false;
> > +}
> > +
> > +static void pca954x_channel_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    dc->desc = "Pca954x Channel";
> > +}
> > +
> > +static void pca9546_class_init(ObjectClass *klass, void *data)
> > +{
> > +    Pca954xClass *s = PCA954X_CLASS(klass);
> > +    s->nchans = PCA9546_CHANNEL_COUNT;
> > +}
> > +
> > +static void pca9548_class_init(ObjectClass *oc, void *data)
> > +{
> > +    Pca954xClass *s = PCA954X_CLASS(oc);
> > +    s->nchans = PCA9548_CHANNEL_COUNT;
> > +}
> > +
> > +static void pca954x_realize(DeviceState *dev, Error **errp)
> > +{
> > +    Pca954xState *s = PCA954X(dev);
> > +    Pca954xClass *c = PCA954X_GET_CLASS(s);
> > +    int i;
> > +
> > +    /* SMBus modules. Cannot fail. */
> > +    for (i = 0; i < c->nchans; i++) {
> > +        Object *obj = OBJECT(&s->channel[i]);
> > +        sysbus_realize(SYS_BUS_DEVICE(obj), &error_abort);
> > +    }
> > +}
> > +
> > +static void pca954x_init(Object *obj)
> > +{
> > +    int i;
> > +    Pca954xState *s = PCA954X(obj);
> > +    Pca954xClass *c = PCA954X_GET_CLASS(obj);
> > +
> > +    /* Only initialize the children we expect. */
> > +    for (i = 0; i < c->nchans; i++) {
> > +        object_initialize_child(obj, "channel[*]", &s->channel[i],
> > +                                TYPE_PCA954X_CHANNEL);
> > +    }
> > +}
> > +
> > +static void pca954x_class_init(ObjectClass *klass, void *data)
> > +{
> > +    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> > +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    SMBusDeviceClass *k = SMBUS_DEVICE_CLASS(klass);
> > +
> > +    sc->match_and_add = pca954x_match;
> > +
> > +    rc->phases.enter = pca954x_enter_reset;
> > +
> > +    dc->desc = "Pca954x i2c-mux";
> > +    dc->realize = pca954x_realize;
> > +
> > +    k->write_data = pca954x_write_data;
> > +    k->receive_byte = pca954x_read_byte;
> > +}
> > +
> > +static const TypeInfo pca954x_info[] = {
> > +    {
> > +        .name          = TYPE_PCA954X,
> > +        .parent        = TYPE_SMBUS_DEVICE,
> > +        .instance_size = sizeof(Pca954xState),
> > +        .instance_init = pca954x_init,
> > +        .class_size    = sizeof(Pca954xClass),
> > +        .class_init    = pca954x_class_init,
> > +        .abstract      = true,
> > +    },
> > +    {
> > +        .name          = TYPE_PCA9546,
> > +        .parent        = TYPE_PCA954X,
> > +        .class_init    = pca9546_class_init,
> > +    },
> > +    {
> > +        .name          = TYPE_PCA9548,
> > +        .parent        = TYPE_PCA954X,
> > +        .class_init    = pca9548_class_init,
> > +    },
> > +    {
> > +        .name = TYPE_PCA954X_CHANNEL,
> > +        .parent = TYPE_SYS_BUS_DEVICE,
> > +        .class_init = pca954x_channel_class_init,
> > +        .instance_size = sizeof(Pca954xChannel),
> > +        .instance_init = pca954x_smbus_init,
> > +    }
> > +};
> > +
> > +DEFINE_TYPES(pca954x_info)
> > diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
> > index cdcd694a7f..dd3aef02b2 100644
> > --- a/hw/i2c/meson.build
> > +++ b/hw/i2c/meson.build
> > @@ -14,4 +14,5 @@ i2c_ss.add(when: 'CONFIG_SMBUS_EEPROM', if_true: files('smbus_eeprom.c'))
> >  i2c_ss.add(when: 'CONFIG_VERSATILE_I2C', if_true: files('versatile_i2c.c'))
> >  i2c_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_i2c.c'))
> >  i2c_ss.add(when: 'CONFIG_PPC4XX', if_true: files('ppc4xx_i2c.c'))
> > +i2c_ss.add(when: 'CONFIG_PCA954X', if_true: files('i2c_mux_pca954x.c'))
> >  softmmu_ss.add_all(when: 'CONFIG_I2C', if_true: i2c_ss)
> > diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> > index 82fe6f965f..82f19e6a2d 100644
> > --- a/hw/i2c/trace-events
> > +++ b/hw/i2c/trace-events
> > @@ -26,3 +26,8 @@ npcm7xx_smbus_recv_byte(const char *id, uint8_t value) "%s recv byte: 0x%02x"
> >  npcm7xx_smbus_stop(const char *id) "%s stopping"
> >  npcm7xx_smbus_nack(const char *id) "%s nacking"
> >  npcm7xx_smbus_recv_fifo(const char *id, uint8_t received, uint8_t expected) "%s recv fifo: received %u, expected %u"
> > +
> > +# i2c-mux-pca954x.c
> > +
> > +pca954x_write_bytes(uint8_t value) "PCA954X write data: 0x%02x"
> > +pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x"
> > diff --git a/include/hw/i2c/i2c_mux_pca954x.h b/include/hw/i2c/i2c_mux_pca954x.h
> > new file mode 100644
> > index 0000000000..8aaf9bbc39
> > --- /dev/null
> > +++ b/include/hw/i2c/i2c_mux_pca954x.h
> > @@ -0,0 +1,19 @@
> > +#ifndef QEMU_I2C_MUX_PCA954X
> > +#define QEMU_I2C_MUX_PCA954X
> > +
> > +#include "hw/i2c/i2c.h"
> > +
> > +#define TYPE_PCA9546 "pca9546"
> > +#define TYPE_PCA9548 "pca9548"
> > +
> > +/**
> > + * Retrieves the i2c bus associated with the specified channel on this i2c
> > + * mux.
> > + * @mux: an i2c mux device.
> > + * @channel: the i2c channel requested
> > + *
> > + * Returns: a pointer to the associated i2c bus.
> > + */
> > +I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel);
>
> I assume your machine-specific code will be referencing this, which is
> the way it should work.

Yes. The ARM board inits create these devices, and then have a pointer
to then request specific channels.

>
> I'm happy with this.
>
> -corey
>
> > +
> > +#endif
> > --
> > 2.31.1.295.g9ea45b61b8-goog
> >

Re: [PATCH v2 0/4] hw/i2c: Adds pca954x i2c mux switch device

[Patrick Venture]

On Fri, Apr 9, 2021 at 11:31 AM Corey Minyard <cminyard@mvista.com> wrote:
>
> On Fri, Apr 09, 2021 at 09:25:41AM -0700, Patrick Venture wrote:
> > The i2c mux device pca954x implements two devices:
> >  - the pca9546 and pca9548.
> >
> > v2:
> >  - the core i2c bus now calls a match method on each i2c child, which
> >  by default will only check for a match against itself.
> >  - the pca954x device overrides the i2c device match method to search
> >  the children for each of its buses that are active.
> >  - the pca954x device now owns an i2c bus for each channel, allowing
> >  the normal device model to attach devices to the channels.
>
> I like this design.  Avoiding hacking into the bus code is a bonus.
>
> Can these devices really have multiple channels enabled at the same
> time?  That seems strange, but I guess that could be useful.

I believe I saw that in the datasheet, and it seems reasonable that
someone might want to do that.

>
> I'm not sure if you need to add a vmstate structure for this.  In
> general most new devices have them; if it's ever included on an x86
> system (or a system with vmstate transfer capability, probably more than
> x86) that will become an issue.  I'm not sure what the expectations are,
> though.

I am perfectly willing to add the vmstate structure in a future
patchset.  My team is actively developing Qemu now for BMC automated
testing support, and we will be adding other pca mux configurations,
and other support, so this will be active.  I don't anticipate a host
system including this device yet, but that's a consideration I had not
considered.

>
> -corey
>
> >
> > Patrick Venture (4):
> >   hw/i2c: name I2CNode list in I2CBus
> >   hw/i2c: add match method for device search
> >   hw/i2c: move search to i2c_scan_bus method
> >   hw/i2c: add pca954x i2c-mux switch
> >
> >  MAINTAINERS                      |   6 +
> >  hw/i2c/Kconfig                   |   4 +
> >  hw/i2c/core.c                    |  55 ++++--
> >  hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
> >  hw/i2c/meson.build               |   1 +
> >  hw/i2c/trace-events              |   5 +
> >  include/hw/i2c/i2c.h             |  16 +-
> >  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
> >  8 files changed, 382 insertions(+), 14 deletions(-)
> >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> >
> > --
> > 2.31.1.295.g9ea45b61b8-goog
> >

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Philippe Mathieu-Daudé]

+Paolo/Thomas

On 4/9/21 7:21 PM, Patrick Venture wrote:
> On Fri, Apr 9, 2021 at 9:51 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi Patrick,
>>
>> On 4/9/21 6:25 PM, Patrick Venture wrote:
>>> The pca954x is an i2c mux, and this adds support for two variants of
>>> this device: the pca9546 and pca9548.
>>>
>>> This device is very common on BMCs to route a different channel to each
>>> PCIe i2c bus downstream from the BMC.
>>>
>>> Signed-off-by: Patrick Venture <venture@google.com>
>>> Reviewed-by: Hao Wu <wuhaotsh@google.com>
>>> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
>>> ---
>>>  MAINTAINERS                      |   6 +
>>>  hw/i2c/Kconfig                   |   4 +
>>>  hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
>>>  hw/i2c/meson.build               |   1 +
>>>  hw/i2c/trace-events              |   5 +
>>>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
>>>  6 files changed, 325 insertions(+)
>>>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
>>>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
>>
>>> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
>>> index 09642a6dcb..8d120a25d5 100644
>>> --- a/hw/i2c/Kconfig
>>> +++ b/hw/i2c/Kconfig
>>> @@ -28,3 +28,7 @@ config IMX_I2C
>>>  config MPC_I2C
>>>      bool
>>>      select I2C
>>> +
>>> +config PCA954X
>>> +    bool
>>> +    select I2C
>>
>> Do you have a circular dependency when also using:
>>
>>        depends on I2C
>>
>> ?
> 
> I'm somewhat new to qemu -- I don't know what you mean, since I2C
> doesn't depend on pca954x, I don't imagine there could be a circular
> dependency.

See
https://qemu-project.gitlab.io/qemu/devel/kconfig.html#guidelines-for-writing-kconfig-files

PCA954X is plugged on an I2C bus
-> depends on I2C

PCA954X provides I2C buses
-> select I2C

Your device is a particular case consuming and providing the Kconfig
'I2C' symbol. I expect a circular dependency problem. Easy to test with
your series but I haven't.

I suppose in this case, the "select" takes over on "depends on" so this
is OK.

Now (unrelated to your series) thinking at the graphical Kconfig tree
representation (like this one generated 2 years ago:
https://drive.google.com/open?id=1kvwl7guuAmCh2Y2UqeXynlA2HmjWcRs9),
I'd rather see a circular dep.

Regards,

Phil.

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Patrick Venture]

On Fri, Apr 9, 2021 at 2:20 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> +Paolo/Thomas
>
> On 4/9/21 7:21 PM, Patrick Venture wrote:
> > On Fri, Apr 9, 2021 at 9:51 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> Hi Patrick,
> >>
> >> On 4/9/21 6:25 PM, Patrick Venture wrote:
> >>> The pca954x is an i2c mux, and this adds support for two variants of
> >>> this device: the pca9546 and pca9548.
> >>>
> >>> This device is very common on BMCs to route a different channel to each
> >>> PCIe i2c bus downstream from the BMC.
> >>>
> >>> Signed-off-by: Patrick Venture <venture@google.com>
> >>> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> >>> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
> >>> ---
> >>>  MAINTAINERS                      |   6 +
> >>>  hw/i2c/Kconfig                   |   4 +
> >>>  hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
> >>>  hw/i2c/meson.build               |   1 +
> >>>  hw/i2c/trace-events              |   5 +
> >>>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
> >>>  6 files changed, 325 insertions(+)
> >>>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >>>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> >>
> >>> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> >>> index 09642a6dcb..8d120a25d5 100644
> >>> --- a/hw/i2c/Kconfig
> >>> +++ b/hw/i2c/Kconfig
> >>> @@ -28,3 +28,7 @@ config IMX_I2C
> >>>  config MPC_I2C
> >>>      bool
> >>>      select I2C
> >>> +
> >>> +config PCA954X
> >>> +    bool
> >>> +    select I2C
> >>
> >> Do you have a circular dependency when also using:
> >>
> >>        depends on I2C
> >>
> >> ?
> >
> > I'm somewhat new to qemu -- I don't know what you mean, since I2C
> > doesn't depend on pca954x, I don't imagine there could be a circular
> > dependency.
>
> See
> https://qemu-project.gitlab.io/qemu/devel/kconfig.html#guidelines-for-writing-kconfig-files
>
> PCA954X is plugged on an I2C bus
> -> depends on I2C
>
> PCA954X provides I2C buses
> -> select I2C

So from the guide it looks like my KConfig should have _depends_ on
I2C.  My board that I'm testing with selects PCA954X and doesn't
explicitly select I2C.  My device _does_ provide I2C buses, as you
say.

>
> Your device is a particular case consuming and providing the Kconfig
> 'I2C' symbol. I expect a circular dependency problem. Easy to test with
> your series but I haven't.
>
> I suppose in this case, the "select" takes over on "depends on" so this
> is OK.

I have to imagine there is a similar situation for PCIe bridges, as
they depend on PCI but also provide it.

>
> Now (unrelated to your series) thinking at the graphical Kconfig tree
> representation (like this one generated 2 years ago:
> https://drive.google.com/open?id=1kvwl7guuAmCh2Y2UqeXynlA2HmjWcRs9),
> I'd rather see a circular dep.
>
> Regards,
>
> Phil.