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

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

[Patrick Venture]

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

Patrick Venture (2):
  hw/i2c/core: add reachable state boolean
  hw/i2c: add pca954x i2c-mux switch

 MAINTAINERS                      |   6 +
 hw/i2c/Kconfig                   |   4 +
 hw/i2c/core.c                    |   6 +
 hw/i2c/i2c_mux_pca954x.c         | 182 +++++++++++++++++++++++++++++++
 hw/i2c/meson.build               |   1 +
 hw/i2c/trace-events              |   5 +
 include/hw/i2c/i2c.h             |   3 +
 include/hw/i2c/i2c_mux_pca954x.h |  60 ++++++++++
 8 files changed, 267 insertions(+)
 create mode 100644 hw/i2c/i2c_mux_pca954x.c
 create mode 100644 include/hw/i2c/i2c_mux_pca954x.h

-- 
2.31.0.208.g409f899ff0-goog

[PATCH 1/2] hw/i2c/core: add reachable state boolean

[Patrick Venture]

An i2c device can be reachable or not, controlled by some external
factor.  This field is leveraged by an i2c mux which presents the
devices on the parent bus when the associated channel is enabled and
otherwise not.

Signed-off-by: Patrick Venture <venture@google.com>
Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
Reviewed-by: Hao Wu <wuhaotsh@google.com>
---
 hw/i2c/core.c        | 6 ++++++
 include/hw/i2c/i2c.h | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 21ec52ac5a..fa7db4549d 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -18,6 +18,7 @@
 #define I2C_BROADCAST 0x00
 
 static Property i2c_props[] = {
+    DEFINE_PROP_BOOL("reachable", struct I2CSlave, reachable, true),
     DEFINE_PROP_UINT8("address", struct I2CSlave, address, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -118,6 +119,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->reachable) {
+                continue;
+            }
             if ((candidate->address == address) || (bus->broadcast)) {
                 node = g_malloc(sizeof(struct I2CNode));
                 node->elt = candidate;
@@ -262,6 +266,7 @@ const VMStateDescription vmstate_i2c_slave = {
     .minimum_version_id = 1,
     .post_load = i2c_slave_post_load,
     .fields = (VMStateField[]) {
+        VMSTATE_BOOL(reachable, I2CSlave),
         VMSTATE_UINT8(address, I2CSlave),
         VMSTATE_END_OF_LIST()
     }
@@ -272,6 +277,7 @@ I2CSlave *i2c_slave_new(const char *name, uint8_t addr)
     DeviceState *dev;
 
     dev = qdev_new(name);
+    qdev_prop_set_bit(dev, "reachable", true);
     qdev_prop_set_uint8(dev, "address", addr);
     return I2C_SLAVE(dev);
 }
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 277dd9f2d6..e5ca15e486 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -44,6 +44,9 @@ struct I2CSlaveClass {
 struct I2CSlave {
     DeviceState qdev;
 
+    /* Whether the i2c child device is reachable from this bus. */
+    bool reachable;
+
     /* Remaining fields for internal use by the I2C code.  */
     uint8_t address;
 };
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 2/2] 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: Havard Skinnemoen <hskinnemoen@google.com>
Reviewed-by: Hao Wu <wuhaotsh@google.com>
---
 MAINTAINERS                      |   6 +
 hw/i2c/Kconfig                   |   4 +
 hw/i2c/i2c_mux_pca954x.c         | 182 +++++++++++++++++++++++++++++++
 hw/i2c/meson.build               |   1 +
 hw/i2c/trace-events              |   5 +
 include/hw/i2c/i2c_mux_pca954x.h |  60 ++++++++++
 6 files changed, 258 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 69003cdc3c..6cec2a9320 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2040,6 +2040,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..8017913637
--- /dev/null
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -0,0 +1,182 @@
+/*
+ * 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_mux_pca954x.h"
+#include "hw/i2c/smbus_slave.h"
+#include "hw/qdev-core.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/queue.h"
+#include "trace.h"
+
+int pca954x_add_child(I2CSlave *mux, uint8_t channel, I2CSlave *child)
+{
+    g_autofree char *name = NULL;
+    /*
+     * Ok, so we need to try to add the i2c_dev to channel for mux.
+     * A channel can have multiple devices, we need a list for each channel.
+     */
+    Pca954xClass *pc = PCA954X_GET_CLASS(mux);
+    Pca954xState *pca954x = PCA954X(mux);
+    PcaMuxChild *controlled_device;
+
+    /* Is the channel legal? */
+    if (channel >= pc->nchans) {
+        return -1;
+    }
+
+    controlled_device = g_new0(PcaMuxChild, 1);
+    controlled_device->channel = channel;
+    controlled_device->child = child;
+    object_ref(OBJECT(controlled_device->child));
+
+    /* Hide the device. */
+    child->reachable = 0;
+
+    QSLIST_INSERT_HEAD(&pca954x->children, controlled_device, sibling);
+
+    name = g_strdup_printf("i2c@%u-child[%u]", channel,
+                           pca954x->count[channel]);
+    object_property_add_link(OBJECT(mux), name,
+                             object_get_typename(OBJECT(child)),
+                             (Object **)&controlled_device->child,
+                             NULL, /* read-only property */
+                             0);
+    pca954x->count[channel]++;
+
+    return 0;
+}
+
+static void pca954x_enable_channel(Pca954xState *s, uint8_t enable_mask)
+{
+    PcaMuxChild *kid;
+    I2CSlave *child;
+
+    /*
+     * For each child, check if their bit is set in data and if yes, enable
+     * them, otherwise disable, hide them.
+     */
+    QSLIST_FOREACH(kid, &s->children, sibling) {
+        child = I2C_SLAVE(kid->child);
+        if (enable_mask & (1 << kid->channel)) {
+            child->reachable = true;
+        } else {
+            child->reachable = 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 pca9546_class_init(ObjectClass *oc, void *data)
+{
+    Pca954xClass *s = PCA954X_CLASS(oc);
+    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_enter_reset(Object *obj, ResetType type)
+{
+    Pca954xState *s = PCA954X(obj);
+    /* Reset will disable all channels. */
+    pca954x_write(s, 0);
+}
+
+static void pca954x_init(Object *obj)
+{
+    Pca954xState *s = PCA954X(obj);
+    memset(s->count, 0x00, sizeof(s->count));
+}
+
+static void pca954x_class_init(ObjectClass *klass, void *data)
+{
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SMBusDeviceClass *k = SMBUS_DEVICE_CLASS(klass);
+
+    dc->desc = "Pca954x i2c-mux";
+    k->write_data = pca954x_write_data;
+    k->receive_byte = pca954x_read_byte;
+    rc->phases.enter = pca954x_enter_reset;
+}
+
+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,
+    },
+};
+
+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..4ea240af26
--- /dev/null
+++ b/include/hw/i2c/i2c_mux_pca954x.h
@@ -0,0 +1,60 @@
+#ifndef QEMU_I2C_MUX_PCA954X
+#define QEMU_I2C_MUX_PCA954X
+
+#include "hw/qdev-core.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/smbus_slave.h"
+
+#define PCA9548_CHANNEL_COUNT 8
+#define PCA9546_CHANNEL_COUNT 4
+
+/* The i2c mux shares ownership of a bus child. */
+typedef struct PcaMuxChild {
+    I2CSlave *child;
+
+    /* The channel on which this child lives. */
+    uint8_t channel;
+
+    QSLIST_ENTRY(PcaMuxChild) sibling;
+} PcaMuxChild;
+
+typedef struct Pca954xState {
+    SMBusDevice parent;
+
+    uint8_t control;
+
+    /* The children this mux co-owns with its parent bus. */
+    QSLIST_HEAD(, PcaMuxChild) children;
+
+    /* The number of children per channel. */
+    unsigned int count[PCA9548_CHANNEL_COUNT];
+} Pca954xState;
+
+typedef struct Pca954xClass {
+    SMBusDeviceClass parent;
+
+    /* The number of channels this mux has. */
+    uint8_t nchans;
+} Pca954xClass;
+
+#define TYPE_PCA9546 "pca9546"
+#define TYPE_PCA9548 "pca9548"
+
+#define TYPE_PCA954X "pca954x"
+
+#define PCA954X(obj) OBJECT_CHECK(Pca954xState, (obj), TYPE_PCA954X)
+#define PCA954X_CLASS(klass)                                                   \
+     OBJECT_CLASS_CHECK(Pca954xClass, (klass), TYPE_PCA954X)
+#define PCA954X_GET_CLASS(obj)                                                 \
+     OBJECT_GET_CLASS(Pca954xClass, (obj), TYPE_PCA954X)
+
+/**
+ * pca954x_add_child - Adds a child i2c device to the mux device on the
+ * specified channel.
+ * @mux - The i2c mux to control this child.
+ * @channel - The channel of the i2c mux that gates this child.
+ * @child - The i2c child device to add to the mux.
+ */
+int pca954x_add_child(I2CSlave *mux, uint8_t channel, I2CSlave *child);
+
+#endif
-- 
2.31.0.208.g409f899ff0-goog

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

[Corey Minyard]

On Sat, Apr 03, 2021 at 03:28:08PM -0700, Patrick Venture wrote:
> The i2c mux device pca954x implements two devices:
>  - the pca9546 and pca9548.
> 
> Patrick Venture (2):
>   hw/i2c/core: add reachable state boolean
>   hw/i2c: add pca954x i2c-mux switch

Looking this over, the code looks good, but I have a few general
questions:

* Can you register the same slave address on different channels?  That's
  something you could do with real hardware and might be required at
  some time.  It looks like to me that you can't with this patch set,
  but maybe I'm missing something.

* Can you add devices to the secondary I2C busses on the mux using the
  standard QEMU device model, or is the function call required?

I ask because I did a pca9540 and pca9541 device, but I've never
submitted it because I didn't think it would ever be needed.  It takes a
different tack on the problem; it creates the secondary busses as
standard QEMU I2C busses and bridges them.  You can see it at

   github.com:cminyard/qemu.git master-i2c-rebase

If you design can do the things I ask, then it's better.  If not, then
I'm not sure.

-corey

> 
>  MAINTAINERS                      |   6 +
>  hw/i2c/Kconfig                   |   4 +
>  hw/i2c/core.c                    |   6 +
>  hw/i2c/i2c_mux_pca954x.c         | 182 +++++++++++++++++++++++++++++++
>  hw/i2c/meson.build               |   1 +
>  hw/i2c/trace-events              |   5 +
>  include/hw/i2c/i2c.h             |   3 +
>  include/hw/i2c/i2c_mux_pca954x.h |  60 ++++++++++
>  8 files changed, 267 insertions(+)
>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> 
> -- 
> 2.31.0.208.g409f899ff0-goog
> 

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

[Patrick Venture]

On Mon, Apr 5, 2021 at 12:58 PM Corey Minyard <cminyard@mvista.com> wrote:
>
> On Sat, Apr 03, 2021 at 03:28:08PM -0700, Patrick Venture wrote:
> > The i2c mux device pca954x implements two devices:
> >  - the pca9546 and pca9548.
> >
> > Patrick Venture (2):
> >   hw/i2c/core: add reachable state boolean
> >   hw/i2c: add pca954x i2c-mux switch
>
> Looking this over, the code looks good, but I have a few general
> questions:
>
> * Can you register the same slave address on different channels?  That's
>   something you could do with real hardware and might be required at
>   some time.  It looks like to me that you can't with this patch set,
>   but maybe I'm missing something.

If I understand the hardware's implementation properly you can have
collisions, and this allows for collisions.  I'm not sure what you
mean by having both accessible.  For instance, on hardware you can
have a switch with N channels, and on two of the channels there is an
eeprom at 50.  But you're unable to talk to both eeproms at the same
time, because the addresses collide -- so how would the hardware know
which you're talking to?  My understanding of the behavior in this
collision case is that it just talks to the first one that responds
and can lead to unexpected things.

There is a board, the quanta-q71l where we had to set the
idle-disconnect because there were two muxes on the same bus, with
conflicting addresses, and so we had to use idle disconnect explicitly
to make the software happy talking to the hardware -- not ideal as
having two devices behind different channels, but ultimately it's the
same idea because the devices are conflicting.

>
> * Can you add devices to the secondary I2C busses on the mux using the
>   standard QEMU device model, or is the function call required?

I added the function call because I didn't see a clean way to bridge
the issue as well as, the quasi-arbitrary bus numbering used by the
kernel isn't how the hardware truly behaves, and my goal was to
implement closer to the hardware.  I thought about adding an I2cBus to
the device and then you'd be able to access it, but wasn't sure of a
nice clean way to plumb that through -- I considered adding/removing
devices from the parent i2c bus instead of the boolean reachable, but
that seemed way less clean - although do-able.

>
> I ask because I did a pca9540 and pca9541 device, but I've never
> submitted it because I didn't think it would ever be needed.  It takes a
> different tack on the problem; it creates the secondary busses as
> standard QEMU I2C busses and bridges them.  You can see it at
>
>    github.com:cminyard/qemu.git master-i2c-rebase
>

I'll have to take a look at your approach, but the idea that it
wouldn't be needed sounds bizarre to me as nearly all BMC-based qemu
boards leverage i2c muxes to handle their PCIe slot i2c routing.

> If you design can do the things I ask, then it's better.  If not, then
> I'm not sure.
>
> -corey
>
> >
> >  MAINTAINERS                      |   6 +
> >  hw/i2c/Kconfig                   |   4 +
> >  hw/i2c/core.c                    |   6 +
> >  hw/i2c/i2c_mux_pca954x.c         | 182 +++++++++++++++++++++++++++++++
> >  hw/i2c/meson.build               |   1 +
> >  hw/i2c/trace-events              |   5 +
> >  include/hw/i2c/i2c.h             |   3 +
> >  include/hw/i2c/i2c_mux_pca954x.h |  60 ++++++++++
> >  8 files changed, 267 insertions(+)
> >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> >
> > --
> > 2.31.0.208.g409f899ff0-goog
> >

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

[Patrick Venture]

On Tue, Apr 6, 2021 at 8:41 AM Patrick Venture <venture@google.com> wrote:
>
> On Mon, Apr 5, 2021 at 12:58 PM Corey Minyard <cminyard@mvista.com> wrote:
> >
> > On Sat, Apr 03, 2021 at 03:28:08PM -0700, Patrick Venture wrote:
> > > The i2c mux device pca954x implements two devices:
> > >  - the pca9546 and pca9548.
> > >
> > > Patrick Venture (2):
> > >   hw/i2c/core: add reachable state boolean
> > >   hw/i2c: add pca954x i2c-mux switch
> >
> > Looking this over, the code looks good, but I have a few general
> > questions:
> >
> > * Can you register the same slave address on different channels?  That's
> >   something you could do with real hardware and might be required at
> >   some time.  It looks like to me that you can't with this patch set,
> >   but maybe I'm missing something.
>
> If I understand the hardware's implementation properly you can have
> collisions, and this allows for collisions.  I'm not sure what you
> mean by having both accessible.  For instance, on hardware you can
> have a switch with N channels, and on two of the channels there is an
> eeprom at 50.  But you're unable to talk to both eeproms at the same
> time, because the addresses collide -- so how would the hardware know
> which you're talking to?  My understanding of the behavior in this
> collision case is that it just talks to the first one that responds
> and can lead to unexpected things.
>
> There is a board, the quanta-q71l where we had to set the
> idle-disconnect because there were two muxes on the same bus, with
> conflicting addresses, and so we had to use idle disconnect explicitly
> to make the software happy talking to the hardware -- not ideal as
> having two devices behind different channels, but ultimately it's the
> same idea because the devices are conflicting.
>
> >
> > * Can you add devices to the secondary I2C busses on the mux using the
> >   standard QEMU device model, or is the function call required?
>
> I added the function call because I didn't see a clean way to bridge
> the issue as well as, the quasi-arbitrary bus numbering used by the
> kernel isn't how the hardware truly behaves, and my goal was to
> implement closer to the hardware.  I thought about adding an I2cBus to
> the device and then you'd be able to access it, but wasn't sure of a
> nice clean way to plumb that through -- I considered adding/removing
> devices from the parent i2c bus instead of the boolean reachable, but
> that seemed way less clean - although do-able.
>
> >
> > I ask because I did a pca9540 and pca9541 device, but I've never
> > submitted it because I didn't think it would ever be needed.  It takes a
> > different tack on the problem; it creates the secondary busses as
> > standard QEMU I2C busses and bridges them.  You can see it at
> >
> >    github.com:cminyard/qemu.git master-i2c-rebase
> >
>
> I'll have to take a look at your approach, but the idea that it
> wouldn't be needed sounds bizarre to me as nearly all BMC-based qemu
> boards leverage i2c muxes to handle their PCIe slot i2c routing.
>
> > If you design can do the things I ask, then it's better.  If not, then
> > I'm not sure.

Corey,

looking at your design, I should be able to do something similar with
a small tweak.

I think my design follows the hardware where there can be conflicts,
etc, but what I didn't know how to do was add the faux I2cBuses in a
useful way -- but if I add the I2cBuses to the device, and then on
add/remove it registers the device on the parent bus -- i can still
use the reachable boolean to control whether it's present.  The faux
I2cBuses would be a simplification for adding/removing i2c devices --
and would act as the device list in my object.  So then setting the
channels would change to walking the devices held by the bus that
corresponds with the bit -- but _still_ using the reachable boolean.

If you'd like, I can update my patchset to use an i2cbus for the
purpose above, then it would satisfy the requirement of leveraging the
normal device process and no longer require the special function call.

Patrick

> >
> > -corey
> >
> > >
> > >  MAINTAINERS                      |   6 +
> > >  hw/i2c/Kconfig                   |   4 +
> > >  hw/i2c/core.c                    |   6 +
> > >  hw/i2c/i2c_mux_pca954x.c         | 182 +++++++++++++++++++++++++++++++
> > >  hw/i2c/meson.build               |   1 +
> > >  hw/i2c/trace-events              |   5 +
> > >  include/hw/i2c/i2c.h             |   3 +
> > >  include/hw/i2c/i2c_mux_pca954x.h |  60 ++++++++++
> > >  8 files changed, 267 insertions(+)
> > >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> > >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> > >
> > > --
> > > 2.31.0.208.g409f899ff0-goog
> > >

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

[Corey Minyard]

On Tue, Apr 06, 2021 at 08:55:14AM -0700, Patrick Venture wrote:
> On Tue, Apr 6, 2021 at 8:41 AM Patrick Venture <venture@google.com> wrote:
> >
> > On Mon, Apr 5, 2021 at 12:58 PM Corey Minyard <cminyard@mvista.com> wrote:
> > >
> > > On Sat, Apr 03, 2021 at 03:28:08PM -0700, Patrick Venture wrote:
> > > > The i2c mux device pca954x implements two devices:
> > > >  - the pca9546 and pca9548.
> > > >
> > > > Patrick Venture (2):
> > > >   hw/i2c/core: add reachable state boolean
> > > >   hw/i2c: add pca954x i2c-mux switch
> > >
> > > Looking this over, the code looks good, but I have a few general
> > > questions:
> > >
> > > * Can you register the same slave address on different channels?  That's
> > >   something you could do with real hardware and might be required at
> > >   some time.  It looks like to me that you can't with this patch set,
> > >   but maybe I'm missing something.
> >
> > If I understand the hardware's implementation properly you can have
> > collisions, and this allows for collisions.  I'm not sure what you
> > mean by having both accessible.  For instance, on hardware you can
> > have a switch with N channels, and on two of the channels there is an
> > eeprom at 50.  But you're unable to talk to both eeproms at the same
> > time, because the addresses collide -- so how would the hardware know
> > which you're talking to?  My understanding of the behavior in this
> > collision case is that it just talks to the first one that responds
> > and can lead to unexpected things.
> >
> > There is a board, the quanta-q71l where we had to set the
> > idle-disconnect because there were two muxes on the same bus, with
> > conflicting addresses, and so we had to use idle disconnect explicitly
> > to make the software happy talking to the hardware -- not ideal as
> > having two devices behind different channels, but ultimately it's the
> > same idea because the devices are conflicting.
> >
> > >
> > > * Can you add devices to the secondary I2C busses on the mux using the
> > >   standard QEMU device model, or is the function call required?
> >
> > I added the function call because I didn't see a clean way to bridge
> > the issue as well as, the quasi-arbitrary bus numbering used by the
> > kernel isn't how the hardware truly behaves, and my goal was to
> > implement closer to the hardware.  I thought about adding an I2cBus to
> > the device and then you'd be able to access it, but wasn't sure of a
> > nice clean way to plumb that through -- I considered adding/removing
> > devices from the parent i2c bus instead of the boolean reachable, but
> > that seemed way less clean - although do-able.
> >
> > >
> > > I ask because I did a pca9540 and pca9541 device, but I've never
> > > submitted it because I didn't think it would ever be needed.  It takes a
> > > different tack on the problem; it creates the secondary busses as
> > > standard QEMU I2C busses and bridges them.  You can see it at
> > >
> > >    github.com:cminyard/qemu.git master-i2c-rebase
> > >
> >
> > I'll have to take a look at your approach, but the idea that it
> > wouldn't be needed sounds bizarre to me as nearly all BMC-based qemu
> > boards leverage i2c muxes to handle their PCIe slot i2c routing.
> >
> > > If you design can do the things I ask, then it's better.  If not, then
> > > I'm not sure.
> 
> Corey,
> 
> looking at your design, I should be able to do something similar with
> a small tweak.
> 
> I think my design follows the hardware where there can be conflicts,
> etc, but what I didn't know how to do was add the faux I2cBuses in a
> useful way -- but if I add the I2cBuses to the device, and then on
> add/remove it registers the device on the parent bus -- i can still
> use the reachable boolean to control whether it's present.  The faux
> I2cBuses would be a simplification for adding/removing i2c devices --
> and would act as the device list in my object.  So then setting the
> channels would change to walking the devices held by the bus that
> corresponds with the bit -- but _still_ using the reachable boolean.
> 
> If you'd like, I can update my patchset to use an i2cbus for the
> purpose above, then it would satisfy the requirement of leveraging the
> normal device process and no longer require the special function call.

That sounds reasonable.  Your implementation is quite a bit simpler than
mine, which is a bonus.

-corey

> 
> Patrick
> 
> > >
> > > -corey
> > >
> > > >
> > > >  MAINTAINERS                      |   6 +
> > > >  hw/i2c/Kconfig                   |   4 +
> > > >  hw/i2c/core.c                    |   6 +
> > > >  hw/i2c/i2c_mux_pca954x.c         | 182 +++++++++++++++++++++++++++++++
> > > >  hw/i2c/meson.build               |   1 +
> > > >  hw/i2c/trace-events              |   5 +
> > > >  include/hw/i2c/i2c.h             |   3 +
> > > >  include/hw/i2c/i2c_mux_pca954x.h |  60 ++++++++++
> > > >  8 files changed, 267 insertions(+)
> > > >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> > > >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> > > >
> > > > --
> > > > 2.31.0.208.g409f899ff0-goog
> > > >
> 

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

[Patrick Venture]

On Tue, Apr 6, 2021 at 11:36 AM Corey Minyard <minyard@acm.org> wrote:
>
> On Tue, Apr 06, 2021 at 08:55:14AM -0700, Patrick Venture wrote:
> > On Tue, Apr 6, 2021 at 8:41 AM Patrick Venture <venture@google.com> wrote:
> > >
> > > On Mon, Apr 5, 2021 at 12:58 PM Corey Minyard <cminyard@mvista.com> wrote:
> > > >
> > > > On Sat, Apr 03, 2021 at 03:28:08PM -0700, Patrick Venture wrote:
> > > > > The i2c mux device pca954x implements two devices:
> > > > >  - the pca9546 and pca9548.
> > > > >
> > > > > Patrick Venture (2):
> > > > >   hw/i2c/core: add reachable state boolean
> > > > >   hw/i2c: add pca954x i2c-mux switch
> > > >
> > > > Looking this over, the code looks good, but I have a few general
> > > > questions:
> > > >
> > > > * Can you register the same slave address on different channels?  That's
> > > >   something you could do with real hardware and might be required at
> > > >   some time.  It looks like to me that you can't with this patch set,
> > > >   but maybe I'm missing something.
> > >
> > > If I understand the hardware's implementation properly you can have
> > > collisions, and this allows for collisions.  I'm not sure what you
> > > mean by having both accessible.  For instance, on hardware you can
> > > have a switch with N channels, and on two of the channels there is an
> > > eeprom at 50.  But you're unable to talk to both eeproms at the same
> > > time, because the addresses collide -- so how would the hardware know
> > > which you're talking to?  My understanding of the behavior in this
> > > collision case is that it just talks to the first one that responds
> > > and can lead to unexpected things.
> > >
> > > There is a board, the quanta-q71l where we had to set the
> > > idle-disconnect because there were two muxes on the same bus, with
> > > conflicting addresses, and so we had to use idle disconnect explicitly
> > > to make the software happy talking to the hardware -- not ideal as
> > > having two devices behind different channels, but ultimately it's the
> > > same idea because the devices are conflicting.
> > >
> > > >
> > > > * Can you add devices to the secondary I2C busses on the mux using the
> > > >   standard QEMU device model, or is the function call required?
> > >
> > > I added the function call because I didn't see a clean way to bridge
> > > the issue as well as, the quasi-arbitrary bus numbering used by the
> > > kernel isn't how the hardware truly behaves, and my goal was to
> > > implement closer to the hardware.  I thought about adding an I2cBus to
> > > the device and then you'd be able to access it, but wasn't sure of a
> > > nice clean way to plumb that through -- I considered adding/removing
> > > devices from the parent i2c bus instead of the boolean reachable, but
> > > that seemed way less clean - although do-able.
> > >
> > > >
> > > > I ask because I did a pca9540 and pca9541 device, but I've never
> > > > submitted it because I didn't think it would ever be needed.  It takes a
> > > > different tack on the problem; it creates the secondary busses as
> > > > standard QEMU I2C busses and bridges them.  You can see it at
> > > >
> > > >    github.com:cminyard/qemu.git master-i2c-rebase
> > > >
> > >
> > > I'll have to take a look at your approach, but the idea that it
> > > wouldn't be needed sounds bizarre to me as nearly all BMC-based qemu
> > > boards leverage i2c muxes to handle their PCIe slot i2c routing.
> > >
> > > > If you design can do the things I ask, then it's better.  If not, then
> > > > I'm not sure.
> >
> > Corey,
> >
> > looking at your design, I should be able to do something similar with
> > a small tweak.
> >
> > I think my design follows the hardware where there can be conflicts,
> > etc, but what I didn't know how to do was add the faux I2cBuses in a
> > useful way -- but if I add the I2cBuses to the device, and then on
> > add/remove it registers the device on the parent bus -- i can still
> > use the reachable boolean to control whether it's present.  The faux
> > I2cBuses would be a simplification for adding/removing i2c devices --
> > and would act as the device list in my object.  So then setting the
> > channels would change to walking the devices held by the bus that
> > corresponds with the bit -- but _still_ using the reachable boolean.
> >
> > If you'd like, I can update my patchset to use an i2cbus for the
> > purpose above, then it would satisfy the requirement of leveraging the
> > normal device process and no longer require the special function call.
>
> That sounds reasonable.  Your implementation is quite a bit simpler than
> mine, which is a bonus.

Corey;

I will send out the updated patches tomorrow, but I had to cherry-pick
your patch: https://github.com/cminyard/qemu/commit/c7f696d09af2d55f221a5c22900c8f71bc2244be
so that I can get the callbacks for the bus actions, in this case, did
you want to send that patch to the mailing list ahead?  Otherwise,
I'll try to incorporate it as a predecessor patch.

Patrick

>
> -corey
>
> >
> > Patrick
> >
> > > >
> > > > -corey
> > > >
> > > > >
> > > > >  MAINTAINERS                      |   6 +
> > > > >  hw/i2c/Kconfig                   |   4 +
> > > > >  hw/i2c/core.c                    |   6 +
> > > > >  hw/i2c/i2c_mux_pca954x.c         | 182 +++++++++++++++++++++++++++++++
> > > > >  hw/i2c/meson.build               |   1 +
> > > > >  hw/i2c/trace-events              |   5 +
> > > > >  include/hw/i2c/i2c.h             |   3 +
> > > > >  include/hw/i2c/i2c_mux_pca954x.h |  60 ++++++++++
> > > > >  8 files changed, 267 insertions(+)
> > > > >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> > > > >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> > > > >
> > > > > --
> > > > > 2.31.0.208.g409f899ff0-goog
> > > > >
> >

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

[Corey Minyard]

On Tue, Apr 06, 2021 at 03:21:18PM -0700, Patrick Venture wrote:
> On Tue, Apr 6, 2021 at 11:36 AM Corey Minyard <minyard@acm.org> wrote:
> >
> > On Tue, Apr 06, 2021 at 08:55:14AM -0700, Patrick Venture wrote:
> > > On Tue, Apr 6, 2021 at 8:41 AM Patrick Venture <venture@google.com> wrote:
> > > >
> > > > On Mon, Apr 5, 2021 at 12:58 PM Corey Minyard <cminyard@mvista.com> wrote:
> > > > >
> > > > > On Sat, Apr 03, 2021 at 03:28:08PM -0700, Patrick Venture wrote:
> > > > > > The i2c mux device pca954x implements two devices:
> > > > > >  - the pca9546 and pca9548.
> > > > > >
> > > > > > Patrick Venture (2):
> > > > > >   hw/i2c/core: add reachable state boolean
> > > > > >   hw/i2c: add pca954x i2c-mux switch
> > > > >
> > > > > Looking this over, the code looks good, but I have a few general
> > > > > questions:
> > > > >
> > > > > * Can you register the same slave address on different channels?  That's
> > > > >   something you could do with real hardware and might be required at
> > > > >   some time.  It looks like to me that you can't with this patch set,
> > > > >   but maybe I'm missing something.
> > > >
> > > > If I understand the hardware's implementation properly you can have
> > > > collisions, and this allows for collisions.  I'm not sure what you
> > > > mean by having both accessible.  For instance, on hardware you can
> > > > have a switch with N channels, and on two of the channels there is an
> > > > eeprom at 50.  But you're unable to talk to both eeproms at the same
> > > > time, because the addresses collide -- so how would the hardware know
> > > > which you're talking to?  My understanding of the behavior in this
> > > > collision case is that it just talks to the first one that responds
> > > > and can lead to unexpected things.
> > > >
> > > > There is a board, the quanta-q71l where we had to set the
> > > > idle-disconnect because there were two muxes on the same bus, with
> > > > conflicting addresses, and so we had to use idle disconnect explicitly
> > > > to make the software happy talking to the hardware -- not ideal as
> > > > having two devices behind different channels, but ultimately it's the
> > > > same idea because the devices are conflicting.
> > > >
> > > > >
> > > > > * Can you add devices to the secondary I2C busses on the mux using the
> > > > >   standard QEMU device model, or is the function call required?
> > > >
> > > > I added the function call because I didn't see a clean way to bridge
> > > > the issue as well as, the quasi-arbitrary bus numbering used by the
> > > > kernel isn't how the hardware truly behaves, and my goal was to
> > > > implement closer to the hardware.  I thought about adding an I2cBus to
> > > > the device and then you'd be able to access it, but wasn't sure of a
> > > > nice clean way to plumb that through -- I considered adding/removing
> > > > devices from the parent i2c bus instead of the boolean reachable, but
> > > > that seemed way less clean - although do-able.
> > > >
> > > > >
> > > > > I ask because I did a pca9540 and pca9541 device, but I've never
> > > > > submitted it because I didn't think it would ever be needed.  It takes a
> > > > > different tack on the problem; it creates the secondary busses as
> > > > > standard QEMU I2C busses and bridges them.  You can see it at
> > > > >
> > > > >    github.com:cminyard/qemu.git master-i2c-rebase
> > > > >
> > > >
> > > > I'll have to take a look at your approach, but the idea that it
> > > > wouldn't be needed sounds bizarre to me as nearly all BMC-based qemu
> > > > boards leverage i2c muxes to handle their PCIe slot i2c routing.
> > > >
> > > > > If you design can do the things I ask, then it's better.  If not, then
> > > > > I'm not sure.
> > >
> > > Corey,
> > >
> > > looking at your design, I should be able to do something similar with
> > > a small tweak.
> > >
> > > I think my design follows the hardware where there can be conflicts,
> > > etc, but what I didn't know how to do was add the faux I2cBuses in a
> > > useful way -- but if I add the I2cBuses to the device, and then on
> > > add/remove it registers the device on the parent bus -- i can still
> > > use the reachable boolean to control whether it's present.  The faux
> > > I2cBuses would be a simplification for adding/removing i2c devices --
> > > and would act as the device list in my object.  So then setting the
> > > channels would change to walking the devices held by the bus that
> > > corresponds with the bit -- but _still_ using the reachable boolean.
> > >
> > > If you'd like, I can update my patchset to use an i2cbus for the
> > > purpose above, then it would satisfy the requirement of leveraging the
> > > normal device process and no longer require the special function call.
> >
> > That sounds reasonable.  Your implementation is quite a bit simpler than
> > mine, which is a bonus.
> 
> Corey;
> 
> I will send out the updated patches tomorrow, but I had to cherry-pick
> your patch: https://github.com/cminyard/qemu/commit/c7f696d09af2d55f221a5c22900c8f71bc2244be
> so that I can get the callbacks for the bus actions, in this case, did
> you want to send that patch to the mailing list ahead?  Otherwise,
> I'll try to incorporate it as a predecessor patch.

Go ahead and incorporate it in your set so the reviewers can see why
it's necessary.

It would also be possible to do this by modifying the i2c bus code, but
I'm not sure what the maintainers there would like.

-corey

> 
> Patrick
> 
> >
> > -corey
> >
> > >
> > > Patrick
> > >
> > > > >
> > > > > -corey
> > > > >
> > > > > >
> > > > > >  MAINTAINERS                      |   6 +
> > > > > >  hw/i2c/Kconfig                   |   4 +
> > > > > >  hw/i2c/core.c                    |   6 +
> > > > > >  hw/i2c/i2c_mux_pca954x.c         | 182 +++++++++++++++++++++++++++++++
> > > > > >  hw/i2c/meson.build               |   1 +
> > > > > >  hw/i2c/trace-events              |   5 +
> > > > > >  include/hw/i2c/i2c.h             |   3 +
> > > > > >  include/hw/i2c/i2c_mux_pca954x.h |  60 ++++++++++
> > > > > >  8 files changed, 267 insertions(+)
> > > > > >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> > > > > >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> > > > > >
> > > > > > --
> > > > > > 2.31.0.208.g409f899ff0-goog
> > > > > >
> > >

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

[Patrick Venture]

On Tue, Apr 6, 2021 at 4:39 PM Corey Minyard <minyard@acm.org> wrote:
>
> On Tue, Apr 06, 2021 at 03:21:18PM -0700, Patrick Venture wrote:
> > On Tue, Apr 6, 2021 at 11:36 AM Corey Minyard <minyard@acm.org> wrote:
> > >
> > > On Tue, Apr 06, 2021 at 08:55:14AM -0700, Patrick Venture wrote:
> > > > On Tue, Apr 6, 2021 at 8:41 AM Patrick Venture <venture@google.com> wrote:
> > > > >
> > > > > On Mon, Apr 5, 2021 at 12:58 PM Corey Minyard <cminyard@mvista.com> wrote:
> > > > > >
> > > > > > On Sat, Apr 03, 2021 at 03:28:08PM -0700, Patrick Venture wrote:
> > > > > > > The i2c mux device pca954x implements two devices:
> > > > > > >  - the pca9546 and pca9548.
> > > > > > >
> > > > > > > Patrick Venture (2):
> > > > > > >   hw/i2c/core: add reachable state boolean
> > > > > > >   hw/i2c: add pca954x i2c-mux switch
> > > > > >
> > > > > > Looking this over, the code looks good, but I have a few general
> > > > > > questions:
> > > > > >
> > > > > > * Can you register the same slave address on different channels?  That's
> > > > > >   something you could do with real hardware and might be required at
> > > > > >   some time.  It looks like to me that you can't with this patch set,
> > > > > >   but maybe I'm missing something.
> > > > >
> > > > > If I understand the hardware's implementation properly you can have
> > > > > collisions, and this allows for collisions.  I'm not sure what you
> > > > > mean by having both accessible.  For instance, on hardware you can
> > > > > have a switch with N channels, and on two of the channels there is an
> > > > > eeprom at 50.  But you're unable to talk to both eeproms at the same
> > > > > time, because the addresses collide -- so how would the hardware know
> > > > > which you're talking to?  My understanding of the behavior in this
> > > > > collision case is that it just talks to the first one that responds
> > > > > and can lead to unexpected things.
> > > > >
> > > > > There is a board, the quanta-q71l where we had to set the
> > > > > idle-disconnect because there were two muxes on the same bus, with
> > > > > conflicting addresses, and so we had to use idle disconnect explicitly
> > > > > to make the software happy talking to the hardware -- not ideal as
> > > > > having two devices behind different channels, but ultimately it's the
> > > > > same idea because the devices are conflicting.
> > > > >
> > > > > >
> > > > > > * Can you add devices to the secondary I2C busses on the mux using the
> > > > > >   standard QEMU device model, or is the function call required?
> > > > >
> > > > > I added the function call because I didn't see a clean way to bridge
> > > > > the issue as well as, the quasi-arbitrary bus numbering used by the
> > > > > kernel isn't how the hardware truly behaves, and my goal was to
> > > > > implement closer to the hardware.  I thought about adding an I2cBus to
> > > > > the device and then you'd be able to access it, but wasn't sure of a
> > > > > nice clean way to plumb that through -- I considered adding/removing
> > > > > devices from the parent i2c bus instead of the boolean reachable, but
> > > > > that seemed way less clean - although do-able.
> > > > >
> > > > > >
> > > > > > I ask because I did a pca9540 and pca9541 device, but I've never
> > > > > > submitted it because I didn't think it would ever be needed.  It takes a
> > > > > > different tack on the problem; it creates the secondary busses as
> > > > > > standard QEMU I2C busses and bridges them.  You can see it at
> > > > > >
> > > > > >    github.com:cminyard/qemu.git master-i2c-rebase
> > > > > >
> > > > >
> > > > > I'll have to take a look at your approach, but the idea that it
> > > > > wouldn't be needed sounds bizarre to me as nearly all BMC-based qemu
> > > > > boards leverage i2c muxes to handle their PCIe slot i2c routing.
> > > > >
> > > > > > If you design can do the things I ask, then it's better.  If not, then
> > > > > > I'm not sure.
> > > >
> > > > Corey,
> > > >
> > > > looking at your design, I should be able to do something similar with
> > > > a small tweak.
> > > >
> > > > I think my design follows the hardware where there can be conflicts,
> > > > etc, but what I didn't know how to do was add the faux I2cBuses in a
> > > > useful way -- but if I add the I2cBuses to the device, and then on
> > > > add/remove it registers the device on the parent bus -- i can still
> > > > use the reachable boolean to control whether it's present.  The faux
> > > > I2cBuses would be a simplification for adding/removing i2c devices --
> > > > and would act as the device list in my object.  So then setting the
> > > > channels would change to walking the devices held by the bus that
> > > > corresponds with the bit -- but _still_ using the reachable boolean.
> > > >
> > > > If you'd like, I can update my patchset to use an i2cbus for the
> > > > purpose above, then it would satisfy the requirement of leveraging the
> > > > normal device process and no longer require the special function call.
> > >
> > > That sounds reasonable.  Your implementation is quite a bit simpler than
> > > mine, which is a bonus.
> >
> > Corey;
> >
> > I will send out the updated patches tomorrow, but I had to cherry-pick
> > your patch: https://github.com/cminyard/qemu/commit/c7f696d09af2d55f221a5c22900c8f71bc2244be
> > so that I can get the callbacks for the bus actions, in this case, did
> > you want to send that patch to the mailing list ahead?  Otherwise,
> > I'll try to incorporate it as a predecessor patch.
>
> Go ahead and incorporate it in your set so the reviewers can see why
> it's necessary.
>
> It would also be possible to do this by modifying the i2c bus code, but
> I'm not sure what the maintainers there would like.

Corey;

Just wanted to give you an update. I reworked everything but wasn't
super happy with the outcome, so I had a quick discussion and came up
with what I think will be clean and make everybody happy.  The rework
is sufficiently time consuming that I don't think it'll be ready until
tomorrow.

>
> -corey
>
> >
> > Patrick
> >
> > >
> > > -corey
> > >
> > > >
> > > > Patrick
> > > >
> > > > > >
> > > > > > -corey
> > > > > >
> > > > > > >
> > > > > > >  MAINTAINERS                      |   6 +
> > > > > > >  hw/i2c/Kconfig                   |   4 +
> > > > > > >  hw/i2c/core.c                    |   6 +
> > > > > > >  hw/i2c/i2c_mux_pca954x.c         | 182 +++++++++++++++++++++++++++++++
> > > > > > >  hw/i2c/meson.build               |   1 +
> > > > > > >  hw/i2c/trace-events              |   5 +
> > > > > > >  include/hw/i2c/i2c.h             |   3 +
> > > > > > >  include/hw/i2c/i2c_mux_pca954x.h |  60 ++++++++++
> > > > > > >  8 files changed, 267 insertions(+)
> > > > > > >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> > > > > > >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> > > > > > >
> > > > > > > --
> > > > > > > 2.31.0.208.g409f899ff0-goog
> > > > > > >
> > > >

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

[Corey Minyard]

On Tue, Apr 06, 2021 at 08:41:50AM -0700, Patrick Venture wrote:
> On Mon, Apr 5, 2021 at 12:58 PM Corey Minyard <cminyard@mvista.com> wrote:
> >
> > On Sat, Apr 03, 2021 at 03:28:08PM -0700, Patrick Venture wrote:
> > > The i2c mux device pca954x implements two devices:
> > >  - the pca9546 and pca9548.
> > >
> > > Patrick Venture (2):
> > >   hw/i2c/core: add reachable state boolean
> > >   hw/i2c: add pca954x i2c-mux switch
> >
> > Looking this over, the code looks good, but I have a few general
> > questions:
> >
> > * Can you register the same slave address on different channels?  That's
> >   something you could do with real hardware and might be required at
> >   some time.  It looks like to me that you can't with this patch set,
> >   but maybe I'm missing something.
> 
> If I understand the hardware's implementation properly you can have
> collisions, and this allows for collisions.  I'm not sure what you
> mean by having both accessible.  For instance, on hardware you can
> have a switch with N channels, and on two of the channels there is an
> eeprom at 50.  But you're unable to talk to both eeproms at the same
> time, because the addresses collide -- so how would the hardware know
> which you're talking to?  My understanding of the behavior in this
> collision case is that it just talks to the first one that responds
> and can lead to unexpected things.

I wasn't talking about the collision case, I was talking about two
devices at the same address on two different channels.  (In a collision,
BTW, both devices will generaly be active and you will get undefined
results.)

My understanding of what you are doing, and I may be wrong, is that you
are adding the devices to the main bus and using an enable/disable to
turn the devices on/off depending on which channel is enabled.

It does look like you can add multiple devices to the same bus at the
same address, so I do think that works.

> 
> There is a board, the quanta-q71l where we had to set the
> idle-disconnect because there were two muxes on the same bus, with
> conflicting addresses, and so we had to use idle disconnect explicitly
> to make the software happy talking to the hardware -- not ideal as
> having two devices behind different channels, but ultimately it's the
> same idea because the devices are conflicting.
> 
> >
> > * Can you add devices to the secondary I2C busses on the mux using the
> >   standard QEMU device model, or is the function call required?
> 
> I added the function call because I didn't see a clean way to bridge
> the issue as well as, the quasi-arbitrary bus numbering used by the
> kernel isn't how the hardware truly behaves, and my goal was to
> implement closer to the hardware.  I thought about adding an I2cBus to
> the device and then you'd be able to access it, but wasn't sure of a
> nice clean way to plumb that through -- I considered adding/removing
> devices from the parent i2c bus instead of the boolean reachable, but
> that seemed way less clean - although do-able.

The only way I can think of with the method that you are using would be
to add a mux and channel to the i2c device, but that's not very natural.

The patch I did implements it by plumbing through, like you say.  It's a
little bit of a hack, but not too bad.

> 
> >
> > I ask because I did a pca9540 and pca9541 device, but I've never
> > submitted it because I didn't think it would ever be needed.  It takes a
> > different tack on the problem; it creates the secondary busses as
> > standard QEMU I2C busses and bridges them.  You can see it at
> >
> >    github.com:cminyard/qemu.git master-i2c-rebase
> >
> 
> I'll have to take a look at your approach, but the idea that it
> wouldn't be needed sounds bizarre to me as nearly all BMC-based qemu
> boards leverage i2c muxes to handle their PCIe slot i2c routing.

Yeah, I don't work in that world :).  I can see the need there, and
nobody has asked up til now.  I wish I had pushed it in earlier, then
your job would have been a lot easier.

-corey

> 
> > If you design can do the things I ask, then it's better.  If not, then
> > I'm not sure.
> >
> > -corey
> >
> > >
> > >  MAINTAINERS                      |   6 +
> > >  hw/i2c/Kconfig                   |   4 +
> > >  hw/i2c/core.c                    |   6 +
> > >  hw/i2c/i2c_mux_pca954x.c         | 182 +++++++++++++++++++++++++++++++
> > >  hw/i2c/meson.build               |   1 +
> > >  hw/i2c/trace-events              |   5 +
> > >  include/hw/i2c/i2c.h             |   3 +
> > >  include/hw/i2c/i2c_mux_pca954x.h |  60 ++++++++++
> > >  8 files changed, 267 insertions(+)
> > >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> > >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> > >
> > > --
> > > 2.31.0.208.g409f899ff0-goog
> > >