| Message ID | 20210403222810.3481372-1-venture@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/i2c: Adds pca954x i2c mux switch device | expand |
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 >
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 > >
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 > > >
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 > > >
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 > > > > >
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 > > > > > > >
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 > > > > > > > > >
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 > > > > > > > > > > >