Page MenuHomeFreeBSD

Add support for i2c bus mux hardware.
ClosedPublic

Authored by ian on Dec 20 2019, 5:06 PM.

Details

Reviewers
avg
mmel
Group Reviewers
ARM
manpages
Summary

An i2c bus can be divided into segments which can be selectively connected and disconnected from the main bus. This is usually done to enable using multiple slave devices having the same address, by isolating the devices onto separate bus segments, only one of which is connected to the main bus at once.

There are several types of i2c bus muxes, which break down into two general categories

  • Muxes which are themselves i2c slaves. These devices respond to i2c commands on their upstream bus, and based on those commands, connect various downstream buses to the upstream. In newbus terms, they are both a child of an iicbus and the parent of one or more iicbus instances.
  • Muxes which are not i2c devices themselves. Such devices are part of the i2c bus electrically, but in newbus terms their parent is some other bus. The association with the upstream bus must be established by separate metadata (such as FDT data).

In both cases, the mux driver has one or more iicbus child instances representing the downstream buses. The mux driver implements the iicbus_if interface, as if it were an iichb host bridge/i2c controller driver. It services the IO requests sent to it by forwarding them to the iicbus instance representing the upstream bus, after electrically connecting the upstream bus to the downstream bus that hosts the i2c slave device which made the IO request.

The net effect is automatic mux switching which is transparent to slaves on the downstream buses. They just do i2c IO they way they normally do, and the bus is electrically connected for the duration of the IO and then idled when it is complete.

The existing iicbus_if callback() method is enhanced so that the parameter passed to it can be a struct which contains a device_t for the requesting bus and slave devices. This change is done by adding a flag that indicates the extra values are present, and making the flags field the first field of a new args struct. If the flag is set, the iichb or mux driver can recast the pointer-to-flags into a pointer-to-struct and access the extra fields. Thus abi compatibility with older drivers is retained (but a mux cannot exist on the bus with the older iicbus driver in use.)

A new set of core support routines exists in iicbus.c. This code will help implement mux drivers for any type of mux hardware by supplying all the boilerplate code that forwards IO requests upstream. It also has code for parsing metadata and instantiating the child iicbus instances based on it.

Two new hardware mux drivers are added. The ltc430x driver supports the LTC4305/4306 mux chips which are controlled via i2c commands. The iic_gpiomux driver supports any mux hardware which is controlled by manipulating the state of one or more gpio pins.

Test Plan

Tested locally using a variety of mux'd bus configurations involving both ltc4305 and a homebrew gpio-controlled mux. Tested configurations included cascaded muxes (unlikely in the real world, but useful to prove that 'it all just works' in terms of the automatic switching and upstream forwarding of IO requests).

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 28420

Event Timeline

ray added inline comments.
sys/conf/files
1803

Forgot ".c" ?

sys/conf/files
1803

Oops, yep. I've fixed it locally, but I'm not going to update a new diff unless some other changes are needed.

Generally I like this, modulo the two relatively minor points. And the 'separate commit' items could be done with this or separately as your workflow allows.

share/man/man4/iic_gpiomux.4
76

what standard is this?

share/man/man4/ltc430x.4
70

Same comment here. I happen to know it's the standard described by Linux's Documentation/<mumble>/i2c/i2c-mux-ltx4306.txt, but it's not completely clear from the context.

sys/conf/NOTES
2325

Could be a separate commit, if you wanted to do that right now and reduce this diff.

2328

Could be a separate commit, if you wanted to do that right now and reduce this diff.

sys/dev/iicbus/iiconf.c
183

Would it make sense to have a wrapper to IIC_REQUEST_BUS, et at, that wrap things to get type safety?

share/man/man4/iic_gpiomux.4
76

Well, this is a problem I've been complaining about for a while, and nothing ever happens. I don't want to just retype the bindings docs into our manpages, but they exist in the outside world only in the linux kernel source right now. They are GPL'd (whatever that means since GPL speaks of code and object files and these are text documents). I suggested making a port of them, then writing an fdt_bindings manpage that gives a little overview of the bindings docs as a collection and mentions which port to install to get a local copy. That would make references from driver manapages make a lot more sense. But Manu doesn't want to make a port, he wants to try to get permission to just include them in base, but that opens up other problems like what systems to install them on (they don't need to be on an embedded board, they need to be on the desktop machine of the person who admins the board).

So, all in all, I'm just throwing these words into manpages for now, in the hopes that soon I'll get to add a .Xr fdt_manpages to make the words more useful.

sys/dev/iicbus/iiconf.c
183

This should be the one and only place this is called from, unless out-of-tree drivers exist (which is why I took some pains to keep things compatible with a flag). Basically, this call is already within the wrapper that everyone uses to request the bus.

Hmm, and that might be more obvious if I had used the right diff (the one with context). So maybe I should upload the right diff.

If you upload a new diff, I'll look at it...
I'm OK with the continued cast given the explanation, especially of it's clearer in a newer diff :)

share/man/man4/iic_gpiomux.4
76

would you be averse to using the following words instead?

'the standard device tree document <path>' where path is what you have now?

I agree in general we'd love to have these docs as a man page, but in the mean time I agree with your general approach of documenting them and think a couple of words could make things a lot clearer.

Uploaded a new diff with proper context.

Also, trivial comment changes in conf/NOTES were committed and aren't in this diff anymore, and the missing ".c" in conf/files was fixed.

share/man/man4/iic_gpiomux.4
76

I'm not sure what you mean. "<path>" that you mention would have to be a github url (and I'm pretty averse to sending someone reading a manpage off to look at linux kernel sources).

I don't want bindings as manpages, I just want the entire collection of bindings text files, in the exact tree structure they now exist (so that import/maintenence pain is minimal), available at /usr/share/doc/fdt_bindings or /usr/local/share/doc/fdt_bindings on the local system, so that manpages can refer to them in that location.

mmel added inline comments.
sys/conf/files
1803

Wouldn't the new option ( iicmux or so ) be better?

sys/dev/iicbus/mux/iicmux.h
59

Function pointers are slightly non-standard for driver-to-driver communication, should not be this converted to standard newbus method? Declared by mux driver and consumed in iicmux.c?

Remove the bus_select callback pointer and instead create an iicmux_if.m to define a bus_select method.

Rename the iicmux_add/delete_children() functions to iicmux_attach() and iicmux_detach().

Remove iicbus_softc_init() and roll its functionality into iicbus_attach().

Add support for sysctl dev.<chip>.<unit>.debugmux which will print out mux switching activities when they happen.

Add a separate entry to sys/conf/files for iicmux, instead of trying to make it automatically get build when any driver that needs it is included.

ian marked 2 inline comments as done.Jan 1 2020, 9:48 PM
ian added inline comments.
sys/conf/files
1803

This is leftover from when iicmux.c was just a bunch of functions that were called from other drivers, so I was trying to just make it invisible to users by getting compiled automatically whenever some driver that needed it was included. But after changing iicmux to be a driver, I should have just changed this to match.

sys/dev/iicbus/mux/iicmux.h
59

The original design had iicmux.c as a collection of functions that would be called from chip drivers. I realized that didn't work well if you wanted to use modules and had 2 different mux chip drivers in a system (duplicate public symbols). So everything about iicmux.c got changed to be a driver, except I forget about the function pointer callback thing. Now I've converted it to an iicmux method.

This revision is now accepted and ready to land.Jan 2 2020, 7:35 AM
ian marked 2 inline comments as done.

Committed (forgot to link to phab) as r356278.