BSC0 config for I2C_MUX_PINCTRL to drive camera/display and GPIO

[6by9]

Trying to ressurrect these patches to add the pinctrl i2c mux onto BSC0 so we can access both camera/display and HAT ID pins simultaneously.

Tested on Pi3B+ & Pi4 with bcm27xx dt, and Pi3B+ with bcm2837-rpi-3-b-plus (downstream kernel image, but with device_tree=bcm2837-rpi-3-b-plus.dtb override).
Need to test other variants, particularly the old versions where i2c1 was used for the camera, or it wasn't present on the header. I suspect we need to use delete-node to remove i2c10mux totally.

[6by9]

@pelwell for comment.

Also need to decide what to do with CM and CM3 where normally we don't assign GPIOs for resources unless really needed.

[pelwell]

These comments and questions need addressing before scrutinising the individual overlay patches.

[pelwell]

We should include arch/arm64/configs/bcmrpi3_defconfig as well.

[6by9]

OK

[6by9]

Hmm, I need to do some double checking. Comparing against a Tegra board that uses i2c-mux-pinctrl, that needs to clear down the pinmuxing as well as setting up the new ones.

I currently haven't done that (just setting up the new pinctrl in each case), and that appeared to work. I'm a little confused as to how it did.

https://github.com/raspberrypi/linux/blob/rpi-5.4.y/arch/arm/boot/dts/tegra20-seaboard.dts#L288

[pelwell]

I've pushed a rebase and a whole bunch of changes in a fixups commit to get your opinion.

[6by9]

I'm surprised you can push to a branch in my repo, but that's Github for you.

So you're:

• renaming the parent node to i2c0if, and leaving it as a dynamically assigned i2c bus number
• renaming the mux to i2c0mux.
• renumbering i2c_csi_dsi to i2c10 (was i2c9).
• cleaning up a couple of overlay errors.

Yes, I'm happy on that lot. I'll squash them back into the parent commits and retest. Seeing as several of these files are upstream ones, and mainline should soonish be getting CSI and DSI support, it'd be nice to have them in a shape that can be upstreamed.

I've gone through my stash of devices and have a 0W, B+, 2B, 3B, 3B+, 4B, and CM3 here, so can check something sensible happens on all of them. (I think my 256MB B is sitting in the office for the awkward case). So apart from the hassle of building the 3 variants of kernels, I should be able to test out the majority that we care about.

[6by9]

On further checking of the pinmuxing, something is clearing down each selection.
Checking /sys/kernel/debug/pinctrl/fe200000.gpio-pinctrl-bcm2835/pinmux-pins, the pins are going from MUX UNCLAIMED to soc:i2c10mux, and then returning to MUX UNCLAIMED when selecting the other muxing again.

So whilst the bindings imply that you need to reset all the pin-ctrl groups, reality seems different. I guess specifying them all stops some other user coming along and claiming the mux.

What's your view? Explicitly state them in all states, or leave it as it is?

[pelwell]

My guess is that the Tegra DTS is explicitly setting functions for the unused pins to avoid the default UNCLAIMED state, which seems to be "input". I think the current behaviour without explicitly reserving the other pins is fine.

[pelwell]

That still leaves us with the thorny issue of the rev1 model B, which would ideally have a mux on i2c1. Instead of the firmware creating device-specific aliases/labels (i2c_vc, i2c_arm etc.) on all boards, it might be cleaner and more flexible to build the "normal" values into the base DTBs and apply a magic "modelb-rev1" overlay to patch things up. That does depend on us being able to undo/disable any unwanted settings of the base DTB from an overlay.

[6by9]

That still leaves us with the thorny issue of the rev1 model B, which would ideally have a mux on i2c1.

There is no requirement for a mux on rev1 B - i2c1 only goes to the camera (GPIOs 2&3), and i2c0 only goes to the 26 pin header as pins 3&5. The alternate muxings via 28&29 and 44&45 aren't used. The mux needs removing, and pinctrl setup restored to the current setup.
I believe that can be done with a /delete-node/ i2c0mux, adding the pin-ctrl to i2c0if, and altering the alias for i2c0 to pointing at i2c0if.
The alias for i2c_vc would already be pointing at i2c1, but I guess the alias for i2c_arm is pointing at i2c0mux port0 (named i2c0) which is wrong. Or does the dtoverlay_set_synonym look for an alias instead of a label? You may know the answer, but I'd have to try it.

Unless having a mux with only one port for consistency is nicer. I think in this case it's uglier as it'd be an i2c1mux so you still need to do the above and then add a second mux.

[pelwell]

I think that the only sane way to get the right end result is to fork the Model B .dts as upstream have done, but using bcm2708-model-b-rev1 and bcm2708-model-b (as opposed to bcm2708-model-b and bcm2708-model-b-rev2) for backwards-compatibility. If we bake in the i2c_vc and i2c_arm aliases that's one less thing for the firmware to do.

[pelwell]

And I can add i2c_cam, to distinguish it from the i2c_vc on the header.

[6by9]

OK, that sounds like a plan.

I'm being hassled to get DSI stuff working, so I'm going to shelve this temporarily whilst using it myself.

i2c_cam is also i2c_dsi_display, hence the current name i2c_csi_dsi. i2c_cam_display if you want to rename it?
(That is of course on all except the Model B rev 1 that forgot to route I2C to the display connector)

[pelwell]

I'll do the necessary rework and push it as clean patches, if that's OK with you.

You're right about the label/alias - i2c_csi_dsi is accurate, and descriptive enough.

[6by9]

Hang fire then as I've merged all your earlier changes into the base commits, but I don't think I've pushed that. No point in duplicating that work.

[pelwell]

That's fine - I've not squashed anything yet.

[6by9]

Rebased and repushed with the updates I'd applied from your fixups.
I've pushed the branch that you had fixed up to rpi-5.4.y-unicam-pe as a backup.

I haven't touched the B rev1 config yet, not run exhaustive tests on the various boards.

[6by9]

I'm confused by some of the extra changes

[6by9]

We have

			i2c0_gpio0: i2c0_gpio0 {
				brcm,pins = <0 1>;
				brcm,function = <BCM2835_FSEL_ALT0>;
			};

in bcm283x.dtsi. Any reason to duplicate?

[pelwell]

Because existing overlays target i2c0_pins.

[6by9]

bcm283x-rpi-i2c0mux_0_28.dtsi is included which sets

&i2c0mux {
	pinctrl-0 = <&i2c0_gpio0>;
	pinctrl-1 = <&i2c0_gpio28>;
};

Do we need to duplicate? Or is there a commit missing here?

[pelwell]

i2c0_pins is a downstream entity so I don't want to refer to it from an upstream file, but I do want to retain the name, hence the apparent duplication.

[pelwell]

Force-pushed to update the commit message for the rev1 addition. If you have no objections I'll squash the fixups with their respective targets and prepare to merge.

[6by9]

If you're happy then I'm happy :-)