qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Patrick Venture
Subject: Re: [PATCH 0/2] hw/i2c: Adds pca954x i2c mux switch device
Date: Tue, 6 Apr 2021 08:41:50 -0700

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]