diff mbox series

[v9,07/10] ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0

Message ID 20240402000424.4650-8-laurent.pinchart@ideasonboard.com (mailing list archive)
State New
Headers show
Series media: Add driver for the Raspberry Pi <5 CSI-2 receiver | expand

Commit Message

Laurent Pinchart April 2, 2024, 12:04 a.m. UTC 
From: Uwe Kleine-König <uwe@kleine-koenig.org>

BCM2711-based Raspberry Pi boards (4B, CM4 and 400) multiplex the I2C0
controller over two sets of pins, GPIO0+1 and GPIO44+45. The former is
exposed on the 40-pin header, while the latter is used for the CSI and
DSI connectors.

Add a pinctrl-based I2C bus multiplexer to bcm2711-rpi.dtsi to model
this multiplexing. The two child buses are named i2c0_0 and i2c0_1.

Note that if you modified the dts before to add devices to the i2c bus
appearing on pins gpio0 + gpio1 (either directly in the dts or using an
overlay), you have to put these into the i2c0_0 node introduced here
now.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
Changes since v7:

- Rename i2c0mux to i2c-mux0

Changes since v6:

- Don't disable i2c0mux by default

Changes since v3:

- Split addition of the RTC to a separate patch
- Move the mux to bcm2711-rpi.dtsi
---
 arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi | 29 +++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Florian Fainelli April 2, 2024, 8:45 p.m. UTC | #1 
From: Florian Fainelli <f.fainelli@gmail.com>

On Tue,  2 Apr 2024 03:04:14 +0300, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> From: Uwe Kleine-König <uwe@kleine-koenig.org>
> 
> BCM2711-based Raspberry Pi boards (4B, CM4 and 400) multiplex the I2C0
> controller over two sets of pins, GPIO0+1 and GPIO44+45. The former is
> exposed on the 40-pin header, while the latter is used for the CSI and
> DSI connectors.
> 
> Add a pinctrl-based I2C bus multiplexer to bcm2711-rpi.dtsi to model
> this multiplexing. The two child buses are named i2c0_0 and i2c0_1.
> 
> Note that if you modified the dts before to add devices to the i2c bus
> appearing on pins gpio0 + gpio1 (either directly in the dts or using an
> overlay), you have to put these into the i2c0_0 node introduced here
> now.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---

Applied to https://github.com/Broadcom/stblinux/commits/devicetree/next, thanks!
--
Florian
Nícolas F. R. A. Prado April 22, 2024, 3:03 p.m. UTC | #2 
On Tue, Apr 02, 2024 at 03:04:14AM +0300, Laurent Pinchart wrote:
[..]
> +
> +	i2c0mux: i2c-mux0 {
> +		compatible = "i2c-mux-pinctrl";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		i2c-parent = <&i2c0>;
> +
> +		pinctrl-names = "i2c0", "i2c0-vc";
> +		pinctrl-0 = <&i2c0_gpio0>;
> +		pinctrl-1 = <&i2c0_gpio44>;
> +
> +		i2c0_0: i2c@0 {
> +			reg = <0>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
> +		i2c0_1: i2c@1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +	};

Hi Laurent,

I noticed you added this new DT node that binds to a driver, but didn't enable
the corresponding driver in the arm64 defconfig. We're running the DT kselftest
in KernelCI which reports DT nodes that haven't bound to a driver and this node
now shows up as a failure. Consider enabling the driver in the defconfig so we
can continually validate that the driver correctly probes this device and we'll
be able to report if it breaks in the future :).

Thanks,
Nícolas

PS: I've included the full test output for this platform below if you'd like to
check it out. There's one single other device that fails to probe,
/soc/mailbox@7e00b840, though that needs CONFIG_BCM2835_VCHIQ, which is on
staging, so I'm guessing not something we should be enabling in the defconfig.


TAP version 13
1..1
# timeout set to 45
# selftests: dt: test_unprobed_devices.sh
# TAP version 13
# 1..69
# ok 1 / # SKIP
# ok 2 /arm-pmu
# ok 3 /clk-108M # SKIP
# ok 4 /clk-27M # SKIP
# ok 5 /clocks/clk-osc # SKIP
# ok 6 /clocks/clk-usb # SKIP
# ok 7 /cpus/cpu@0 # SKIP
# ok 8 /cpus/cpu@1 # SKIP
# ok 9 /cpus/cpu@2 # SKIP
# ok 10 /cpus/cpu@3 # SKIP
# ok 11 /cpus/l2-cache0 # SKIP
# ok 12 /emmc2bus
# ok 13 /emmc2bus/mmc@7e340000
# ok 14 /gpu
# not ok 15 /i2c-mux0
# ok 16 /leds
# ok 17 /phy
# ok 18 /regulator-cam1
# ok 19 /regulator-sd-io-1v8
# ok 20 /regulator-sd-vcc
# ok 21 /reserved-memory/linux,cma # SKIP
# ok 22 /reserved-memory/nvram@0
# ok 23 /scb
# ok 24 /scb/ethernet@7d580000
# ok 25 /scb/ethernet@7d580000/mdio@e14
# ok 26 /scb/gpu@7ec00000
# ok 27 /scb/pcie@7d500000
# ok 28 /soc
# ok 29 /soc/aux@7e215000
# ok 30 /soc/avs-monitor@7d5d2000 # SKIP
# ok 31 /soc/avs-monitor@7d5d2000/thermal
# ok 32 /soc/clock@7ef00000
# ok 33 /soc/cprman@7e101000
# ok 34 /soc/dma-controller@7e007000
# ok 35 /soc/firmware
# ok 36 /soc/firmware/clocks
# ok 37 /soc/firmware/gpio
# ok 38 /soc/firmware/reset
# ok 39 /soc/gpio@7e200000
# ok 40 /soc/hdmi@7ef00700
# ok 41 /soc/hdmi@7ef05700
# ok 42 /soc/hvs@7e400000
# ok 43 /soc/i2c@7e205000
# ok 44 /soc/i2c@7e804000
# ok 45 /soc/i2c@7ef04500
# ok 46 /soc/i2c@7ef09500
# ok 47 /soc/interrupt-controller@40000000 # SKIP
# ok 48 /soc/interrupt-controller@40041000 # SKIP
# ok 49 /soc/interrupt-controller@7ef00100
# not ok 50 /soc/mailbox@7e00b840
# ok 51 /soc/mailbox@7e00b880
# ok 52 /soc/mmc@7e300000
# ok 53 /soc/mmc@7e300000/wifi@1 # SKIP
# ok 54 /soc/pixelvalve@7e206000
# ok 55 /soc/pixelvalve@7e207000
# ok 56 /soc/pixelvalve@7e20a000
# ok 57 /soc/pixelvalve@7e216000
# ok 58 /soc/power
# ok 59 /soc/pwm@7e20c800
# ok 60 /soc/rng@7e104000
# ok 61 /soc/serial@7e201000
# ok 62 /soc/serial@7e201000/bluetooth
# ok 63 /soc/serial@7e215040
# ok 64 /soc/timer@7e003000 # SKIP
# ok 65 /soc/txp@7e004000
# ok 66 /soc/usb@7e980000
# ok 67 /soc/watchdog@7e100000
# ok 68 /timer # SKIP
# ok 69 /wifi-pwrseq
# # Totals: pass:50 fail:2 xfail:0 xpass:0 skip:17 error:0
not ok 1 selftests: dt: test_unprobed_devices.sh # exit=1
Laurent Pinchart April 22, 2024, 4:01 p.m. UTC | #3 
Hi Nícolas,

On Mon, Apr 22, 2024 at 11:03:27AM -0400, Nícolas F. R. A. Prado wrote:
> On Tue, Apr 02, 2024 at 03:04:14AM +0300, Laurent Pinchart wrote:
> [..]
> > +
> > +	i2c0mux: i2c-mux0 {
> > +		compatible = "i2c-mux-pinctrl";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		i2c-parent = <&i2c0>;
> > +
> > +		pinctrl-names = "i2c0", "i2c0-vc";
> > +		pinctrl-0 = <&i2c0_gpio0>;
> > +		pinctrl-1 = <&i2c0_gpio44>;
> > +
> > +		i2c0_0: i2c@0 {
> > +			reg = <0>;
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +		};
> > +
> > +		i2c0_1: i2c@1 {
> > +			reg = <1>;
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +		};
> > +	};
> 
> Hi Laurent,
> 
> I noticed you added this new DT node that binds to a driver, but didn't enable
> the corresponding driver in the arm64 defconfig. We're running the DT kselftest
> in KernelCI which reports DT nodes that haven't bound to a driver and this node
> now shows up as a failure. Consider enabling the driver in the defconfig so we
> can continually validate that the driver correctly probes this device and we'll
> be able to report if it breaks in the future :).

Interesting, I wasn't aware of the requirement to enable in the
defconfig all drivers that are used by an upstream DT. I'll send a patch
to fix that.

> PS: I've included the full test output for this platform below if you'd like to
> check it out. There's one single other device that fails to probe,
> /soc/mailbox@7e00b840, though that needs CONFIG_BCM2835_VCHIQ, which is on
> staging, so I'm guessing not something we should be enabling in the defconfig.

Probably not. We're working on getting it out of staging, it should be
enabled then. I've CC'ed Umang for awareness.

> TAP version 13
> 1..1
> # timeout set to 45
> # selftests: dt: test_unprobed_devices.sh
> # TAP version 13
> # 1..69
> # ok 1 / # SKIP
> # ok 2 /arm-pmu
> # ok 3 /clk-108M # SKIP
> # ok 4 /clk-27M # SKIP
> # ok 5 /clocks/clk-osc # SKIP
> # ok 6 /clocks/clk-usb # SKIP
> # ok 7 /cpus/cpu@0 # SKIP
> # ok 8 /cpus/cpu@1 # SKIP
> # ok 9 /cpus/cpu@2 # SKIP
> # ok 10 /cpus/cpu@3 # SKIP
> # ok 11 /cpus/l2-cache0 # SKIP
> # ok 12 /emmc2bus
> # ok 13 /emmc2bus/mmc@7e340000
> # ok 14 /gpu
> # not ok 15 /i2c-mux0
> # ok 16 /leds
> # ok 17 /phy
> # ok 18 /regulator-cam1
> # ok 19 /regulator-sd-io-1v8
> # ok 20 /regulator-sd-vcc
> # ok 21 /reserved-memory/linux,cma # SKIP
> # ok 22 /reserved-memory/nvram@0
> # ok 23 /scb
> # ok 24 /scb/ethernet@7d580000
> # ok 25 /scb/ethernet@7d580000/mdio@e14
> # ok 26 /scb/gpu@7ec00000
> # ok 27 /scb/pcie@7d500000
> # ok 28 /soc
> # ok 29 /soc/aux@7e215000
> # ok 30 /soc/avs-monitor@7d5d2000 # SKIP
> # ok 31 /soc/avs-monitor@7d5d2000/thermal
> # ok 32 /soc/clock@7ef00000
> # ok 33 /soc/cprman@7e101000
> # ok 34 /soc/dma-controller@7e007000
> # ok 35 /soc/firmware
> # ok 36 /soc/firmware/clocks
> # ok 37 /soc/firmware/gpio
> # ok 38 /soc/firmware/reset
> # ok 39 /soc/gpio@7e200000
> # ok 40 /soc/hdmi@7ef00700
> # ok 41 /soc/hdmi@7ef05700
> # ok 42 /soc/hvs@7e400000
> # ok 43 /soc/i2c@7e205000
> # ok 44 /soc/i2c@7e804000
> # ok 45 /soc/i2c@7ef04500
> # ok 46 /soc/i2c@7ef09500
> # ok 47 /soc/interrupt-controller@40000000 # SKIP
> # ok 48 /soc/interrupt-controller@40041000 # SKIP
> # ok 49 /soc/interrupt-controller@7ef00100
> # not ok 50 /soc/mailbox@7e00b840
> # ok 51 /soc/mailbox@7e00b880
> # ok 52 /soc/mmc@7e300000
> # ok 53 /soc/mmc@7e300000/wifi@1 # SKIP
> # ok 54 /soc/pixelvalve@7e206000
> # ok 55 /soc/pixelvalve@7e207000
> # ok 56 /soc/pixelvalve@7e20a000
> # ok 57 /soc/pixelvalve@7e216000
> # ok 58 /soc/power
> # ok 59 /soc/pwm@7e20c800
> # ok 60 /soc/rng@7e104000
> # ok 61 /soc/serial@7e201000
> # ok 62 /soc/serial@7e201000/bluetooth
> # ok 63 /soc/serial@7e215040
> # ok 64 /soc/timer@7e003000 # SKIP
> # ok 65 /soc/txp@7e004000
> # ok 66 /soc/usb@7e980000
> # ok 67 /soc/watchdog@7e100000
> # ok 68 /timer # SKIP
> # ok 69 /wifi-pwrseq
> # # Totals: pass:50 fail:2 xfail:0 xpass:0 skip:17 error:0
> not ok 1 selftests: dt: test_unprobed_devices.sh # exit=1
Nícolas F. R. A. Prado April 22, 2024, 5:21 p.m. UTC | #4 
On Mon, Apr 22, 2024 at 07:01:40PM +0300, Laurent Pinchart wrote:
> Hi Nícolas,
> 
> On Mon, Apr 22, 2024 at 11:03:27AM -0400, Nícolas F. R. A. Prado wrote:
> > On Tue, Apr 02, 2024 at 03:04:14AM +0300, Laurent Pinchart wrote:
> > [..]
> > > +
> > > +	i2c0mux: i2c-mux0 {
> > > +		compatible = "i2c-mux-pinctrl";
> > > +		#address-cells = <1>;
> > > +		#size-cells = <0>;
> > > +
> > > +		i2c-parent = <&i2c0>;
> > > +
> > > +		pinctrl-names = "i2c0", "i2c0-vc";
> > > +		pinctrl-0 = <&i2c0_gpio0>;
> > > +		pinctrl-1 = <&i2c0_gpio44>;
> > > +
> > > +		i2c0_0: i2c@0 {
> > > +			reg = <0>;
> > > +			#address-cells = <1>;
> > > +			#size-cells = <0>;
> > > +		};
> > > +
> > > +		i2c0_1: i2c@1 {
> > > +			reg = <1>;
> > > +			#address-cells = <1>;
> > > +			#size-cells = <0>;
> > > +		};
> > > +	};
> > 
> > Hi Laurent,
> > 
> > I noticed you added this new DT node that binds to a driver, but didn't enable
> > the corresponding driver in the arm64 defconfig. We're running the DT kselftest
> > in KernelCI which reports DT nodes that haven't bound to a driver and this node
> > now shows up as a failure. Consider enabling the driver in the defconfig so we
> > can continually validate that the driver correctly probes this device and we'll
> > be able to report if it breaks in the future :).
> 
> Interesting, I wasn't aware of the requirement to enable in the
> defconfig all drivers that are used by an upstream DT. I'll send a patch
> to fix that.

Oh no, this isn't a requirement at all. I'm just pointing out that by doing it
you enable more testing to get done on the platform automatically, which I
thought you'd appreciate (and we do too!). So yes, please add it to the
defconfig if you'd like to have the driver probe tested in KernelCI and thank
you.

> 
> > PS: I've included the full test output for this platform below if you'd like to
> > check it out. There's one single other device that fails to probe,
> > /soc/mailbox@7e00b840, though that needs CONFIG_BCM2835_VCHIQ, which is on
> > staging, so I'm guessing not something we should be enabling in the defconfig.
> 
> Probably not. We're working on getting it out of staging, it should be
> enabled then. I've CC'ed Umang for awareness.

That's good to hear, thank you for the information!

Thanks,
Nícolas
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi b/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
index 86188eabeb24..6bf4241fe3b7 100644
--- a/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
@@ -17,6 +17,30 @@  aliases {
 		pcie0 = &pcie0;
 		blconfig = &blconfig;
 	};
+
+	i2c0mux: i2c-mux0 {
+		compatible = "i2c-mux-pinctrl";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c-parent = <&i2c0>;
+
+		pinctrl-names = "i2c0", "i2c0-vc";
+		pinctrl-0 = <&i2c0_gpio0>;
+		pinctrl-1 = <&i2c0_gpio44>;
+
+		i2c0_0: i2c@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c0_1: i2c@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+	};
 };
 
 &firmware {
@@ -49,6 +73,11 @@  &hvs {
 	clocks = <&firmware_clocks 4>;
 };
 
+&i2c0 {
+	/delete-property/ pinctrl-names;
+	/delete-property/ pinctrl-0;
+};
+
 &rmem {
 	/*
 	 * RPi4's co-processor will copy the board's bootloader configuration