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