[PATCH v4 0/2] ARM: dts: bcm2711-rpi-cm4-io: Add rtc on a pinctrl-muxed i2c bus

[PATCH v4 0/2] ARM: dts: bcm2711-rpi-cm4-io: Add rtc on a pinctrl-muxed i2c bus

[Laurent Pinchart]

Hello,

This series is an attempt to revive support for pinmuxed I2C0 on the
Raspberry Pi BCM2711-based board.

On BCM2711-based boards, the I2C0 controller can be muxed between pins
0+1 or 44+45. The former is exposed through the 40-pins GPIO connector,
and the latter is used for the RTC on the CM4 I/O board, but also routed
to the display and camera connectors on the Raspberry Pi 4B board. The
other BCM2711-based board, the Raspberry Pi 400, does not expose or
connect anything to pins 44+45.

A previous version was posted ([1]) a year and a half ago by Uwe. It
bundled the pinmuxing and RTC in a single patch, with the mux added to
the CM4 I/O board device tree. This version splits this in two, and
moves the pinumxing to the bcm2711-rpi.dtsi to also support the
Raspberry Pi 4B.

The Raspberry Pi downstream kernel has a more complex DT architecture in
order to support different I2C0 pinmuxing for other boards. Two files,
bcm283x-rpi-i2c0mux_0_28.dtsi and bcm283x-rpi-i2c0mux_0_44.dtsi, define
the two I2C0 pinxmuxing options (pins 0+1 and 28+29, or pins 0+1 and
44+45). Each board .dts then includes the appropriate file. I'm hoping
to avoid this additional complexity for now, by addressing the I2C0
pinmuxing for BCM2711-based boards only. If/when support for I2C0
pinmuxing on boards will be needed, we can revisit this topic.

Compared to the Raspberry Pi downstream kernel, the two muxed I2C buses
are labelled i2c0_0 and i2c0_1 instead of i2c0 and i2c_csi_dsi. This
change was made to keep the naming of the I2C controller labels
consistent, avoiding renaming of the I2C0 controller's label from i2c0
to i2c0if.

Dave, are you fine with the differences between this patch series and
the downstream kernel, or do you expect them to cause issues ?

[1] https://lore.kernel.org/linux-arm-kernel/20211231115109.94626-1-uwe@kleine-koenig.org/

Uwe Kleine-König (2):
  ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0
  ARM: dts: bcm2711-rpi-cm4-io: Add RTC on I2C0

 arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts | 16 ++++++++++++
 arch/arm/boot/dts/bcm2711-rpi.dtsi       | 31 ++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

-- 
Regards,

Laurent Pinchart

[PATCH v4 1/2] ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0

[Laurent Pinchart]

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>
---
Changes since v3:

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

diff --git a/arch/arm/boot/dts/bcm2711-rpi.dtsi b/arch/arm/boot/dts/bcm2711-rpi.dtsi
index 5e95e2321218..7eb9f131cfd1 100644
--- a/arch/arm/boot/dts/bcm2711-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2711-rpi.dtsi
@@ -16,6 +16,32 @@ aliases {
 		pcie0 = &pcie0;
 		blconfig = &blconfig;
 	};
+
+	i2c0mux: i2c0mux {
+		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>;
+
+		status = "disabled";
+
+		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 {
@@ -48,6 +74,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
-- 
Regards,

Laurent Pinchart

[PATCH v4 2/2] ARM: dts: bcm2711-rpi-cm4-io: Add RTC on I2C0

[Laurent Pinchart]

From: Uwe Kleine-König <uwe@kleine-koenig.org>

The cm4-io board comes with a PCF85063 on I2C0, connected to the GPIO44
and GPIO45 pins. Add it to the device tree.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v3:

- Separate addition of the RTC to a patch of its own
---
 arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts b/arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts
index d7ba02f586d3..0d9b328a215a 100644
--- a/arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts
+++ b/arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts
@@ -101,6 +101,22 @@ &genet {
 	status = "okay";
 };
 
+&i2c0 {
+	status = "okay";
+};
+
+&i2c0_1 {
+	rtc@51 {
+		/* Attention: An alarm resets the machine */
+		compatible = "nxp,pcf85063";
+		reg = <0x51>;
+	};
+};
+
+&i2c0mux {
+	status = "okay";
+};
+
 &led_act {
 	gpios = <&gpio 42 GPIO_ACTIVE_HIGH>;
 };
-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/2] ARM: dts: bcm2711-rpi-cm4-io: Add rtc on a pinctrl-muxed i2c bus

[Dave Stevenson]

Hi Laurent

On Mon, 31 Jul 2023 at 22:55, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> This series is an attempt to revive support for pinmuxed I2C0 on the
> Raspberry Pi BCM2711-based board.
>
> On BCM2711-based boards, the I2C0 controller can be muxed between pins
> 0+1 or 44+45. The former is exposed through the 40-pins GPIO connector,
> and the latter is used for the RTC on the CM4 I/O board, but also routed
> to the display and camera connectors on the Raspberry Pi 4B board. The
> other BCM2711-based board, the Raspberry Pi 400, does not expose or
> connect anything to pins 44+45.
>
> A previous version was posted ([1]) a year and a half ago by Uwe. It
> bundled the pinmuxing and RTC in a single patch, with the mux added to
> the CM4 I/O board device tree. This version splits this in two, and
> moves the pinumxing to the bcm2711-rpi.dtsi to also support the
> Raspberry Pi 4B.
>
> The Raspberry Pi downstream kernel has a more complex DT architecture in
> order to support different I2C0 pinmuxing for other boards. Two files,
> bcm283x-rpi-i2c0mux_0_28.dtsi and bcm283x-rpi-i2c0mux_0_44.dtsi, define
> the two I2C0 pinxmuxing options (pins 0+1 and 28+29, or pins 0+1 and
> 44+45). Each board .dts then includes the appropriate file. I'm hoping
> to avoid this additional complexity for now, by addressing the I2C0
> pinmuxing for BCM2711-based boards only. If/when support for I2C0
> pinmuxing on boards will be needed, we can revisit this topic.
>
> Compared to the Raspberry Pi downstream kernel, the two muxed I2C buses
> are labelled i2c0_0 and i2c0_1 instead of i2c0 and i2c_csi_dsi. This
> change was made to keep the naming of the I2C controller labels
> consistent, avoiding renaming of the I2C0 controller's label from i2c0
> to i2c0if.
>
> Dave, are you fine with the differences between this patch series and
> the downstream kernel, or do you expect them to cause issues ?

I've checked with Phil. There's nothing too untoward that will cause
us any significant grief.

Phil has commented that the RTC is an PCF85063AT, so that compatible
string should be "nxp,pcf85063a" if you actually want to make use of
the alarm output.
Then again the driver support for the alarm output appears to want it
routed to an IRQ rather than as a system reset/wakeup, so it probably
makes little difference. It llargely depends on how exact you want to
be in your hardware description.

  Dave

> [1] https://lore.kernel.org/linux-arm-kernel/20211231115109.94626-1-uwe@kleine-koenig.org/
>
> Uwe Kleine-König (2):
>   ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0
>   ARM: dts: bcm2711-rpi-cm4-io: Add RTC on I2C0
>
>  arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts | 16 ++++++++++++
>  arch/arm/boot/dts/bcm2711-rpi.dtsi       | 31 ++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v4 0/2] ARM: dts: bcm2711-rpi-cm4-io: Add rtc on a pinctrl-muxed i2c bus

[Laurent Pinchart]

Hi Dave,

On Tue, Aug 01, 2023 at 03:22:17PM +0100, Dave Stevenson wrote:
> On Mon, 31 Jul 2023 at 22:55, Laurent Pinchart wrote:
> >
> > Hello,
> >
> > This series is an attempt to revive support for pinmuxed I2C0 on the
> > Raspberry Pi BCM2711-based board.
> >
> > On BCM2711-based boards, the I2C0 controller can be muxed between pins
> > 0+1 or 44+45. The former is exposed through the 40-pins GPIO connector,
> > and the latter is used for the RTC on the CM4 I/O board, but also routed
> > to the display and camera connectors on the Raspberry Pi 4B board. The
> > other BCM2711-based board, the Raspberry Pi 400, does not expose or
> > connect anything to pins 44+45.
> >
> > A previous version was posted ([1]) a year and a half ago by Uwe. It
> > bundled the pinmuxing and RTC in a single patch, with the mux added to
> > the CM4 I/O board device tree. This version splits this in two, and
> > moves the pinumxing to the bcm2711-rpi.dtsi to also support the
> > Raspberry Pi 4B.
> >
> > The Raspberry Pi downstream kernel has a more complex DT architecture in
> > order to support different I2C0 pinmuxing for other boards. Two files,
> > bcm283x-rpi-i2c0mux_0_28.dtsi and bcm283x-rpi-i2c0mux_0_44.dtsi, define
> > the two I2C0 pinxmuxing options (pins 0+1 and 28+29, or pins 0+1 and
> > 44+45). Each board .dts then includes the appropriate file. I'm hoping
> > to avoid this additional complexity for now, by addressing the I2C0
> > pinmuxing for BCM2711-based boards only. If/when support for I2C0
> > pinmuxing on boards will be needed, we can revisit this topic.
> >
> > Compared to the Raspberry Pi downstream kernel, the two muxed I2C buses
> > are labelled i2c0_0 and i2c0_1 instead of i2c0 and i2c_csi_dsi. This
> > change was made to keep the naming of the I2C controller labels
> > consistent, avoiding renaming of the I2C0 controller's label from i2c0
> > to i2c0if.
> >
> > Dave, are you fine with the differences between this patch series and
> > the downstream kernel, or do you expect them to cause issues ?
> 
> I've checked with Phil. There's nothing too untoward that will cause
> us any significant grief.

Thanks for checking.

In the meantime, I realized that the CM4S is 2711-based and, according
to the downstream DT, multiplexes I2C0 on pins 28+29, not 44+45 :-(
Umang and Kieran also told me that we want to test camera support on the
Pi 3B. It looks like the only viable approach to support all that will
be to include per-board I2C0 pinmux .dtsi as done in the downstream
kernel. I'll send a v5.

> Phil has commented that the RTC is an PCF85063AT, so that compatible
> string should be "nxp,pcf85063a" if you actually want to make use of
> the alarm output.
> Then again the driver support for the alarm output appears to want it
> routed to an IRQ rather than as a system reset/wakeup, so it probably
> makes little difference. It llargely depends on how exact you want to
> be in your hardware description.

I'll update the compatible string, it's an easy change and it's nice to
be accurate.

> > [1] https://lore.kernel.org/linux-arm-kernel/20211231115109.94626-1-uwe@kleine-koenig.org/
> >
> > Uwe Kleine-König (2):
> >   ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0
> >   ARM: dts: bcm2711-rpi-cm4-io: Add RTC on I2C0
> >
> >  arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts | 16 ++++++++++++
> >  arch/arm/boot/dts/bcm2711-rpi.dtsi       | 31 ++++++++++++++++++++++++
> >  2 files changed, 47 insertions(+)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/2] ARM: dts: bcm2711-rpi-cm4-io: Add rtc on a pinctrl-muxed i2c bus

[Laurent Pinchart]

Hi Dave,

On Tue, Aug 01, 2023 at 05:43:15PM +0300, Laurent Pinchart wrote:
> On Tue, Aug 01, 2023 at 03:22:17PM +0100, Dave Stevenson wrote:
> > On Mon, 31 Jul 2023 at 22:55, Laurent Pinchart wrote:
> > >
> > > Hello,
> > >
> > > This series is an attempt to revive support for pinmuxed I2C0 on the
> > > Raspberry Pi BCM2711-based board.
> > >
> > > On BCM2711-based boards, the I2C0 controller can be muxed between pins
> > > 0+1 or 44+45. The former is exposed through the 40-pins GPIO connector,
> > > and the latter is used for the RTC on the CM4 I/O board, but also routed
> > > to the display and camera connectors on the Raspberry Pi 4B board. The
> > > other BCM2711-based board, the Raspberry Pi 400, does not expose or
> > > connect anything to pins 44+45.
> > >
> > > A previous version was posted ([1]) a year and a half ago by Uwe. It
> > > bundled the pinmuxing and RTC in a single patch, with the mux added to
> > > the CM4 I/O board device tree. This version splits this in two, and
> > > moves the pinumxing to the bcm2711-rpi.dtsi to also support the
> > > Raspberry Pi 4B.
> > >
> > > The Raspberry Pi downstream kernel has a more complex DT architecture in
> > > order to support different I2C0 pinmuxing for other boards. Two files,
> > > bcm283x-rpi-i2c0mux_0_28.dtsi and bcm283x-rpi-i2c0mux_0_44.dtsi, define
> > > the two I2C0 pinxmuxing options (pins 0+1 and 28+29, or pins 0+1 and
> > > 44+45). Each board .dts then includes the appropriate file. I'm hoping
> > > to avoid this additional complexity for now, by addressing the I2C0
> > > pinmuxing for BCM2711-based boards only. If/when support for I2C0
> > > pinmuxing on boards will be needed, we can revisit this topic.
> > >
> > > Compared to the Raspberry Pi downstream kernel, the two muxed I2C buses
> > > are labelled i2c0_0 and i2c0_1 instead of i2c0 and i2c_csi_dsi. This
> > > change was made to keep the naming of the I2C controller labels
> > > consistent, avoiding renaming of the I2C0 controller's label from i2c0
> > > to i2c0if.
> > >
> > > Dave, are you fine with the differences between this patch series and
> > > the downstream kernel, or do you expect them to cause issues ?
> > 
> > I've checked with Phil. There's nothing too untoward that will cause
> > us any significant grief.
> 
> Thanks for checking.
> 
> In the meantime, I realized that the CM4S is 2711-based and, according
> to the downstream DT, multiplexes I2C0 on pins 28+29, not 44+45 :-(
> Umang and Kieran also told me that we want to test camera support on the
> Pi 3B. It looks like the only viable approach to support all that will
> be to include per-board I2C0 pinmux .dtsi as done in the downstream
> kernel. I'll send a v5.
> 
> > Phil has commented that the RTC is an PCF85063AT, so that compatible
> > string should be "nxp,pcf85063a" if you actually want to make use of
> > the alarm output.
> > Then again the driver support for the alarm output appears to want it
> > routed to an IRQ rather than as a system reset/wakeup, so it probably
> > makes little difference. It llargely depends on how exact you want to
> > be in your hardware description.
> 
> I'll update the compatible string, it's an easy change and it's nice to
> be accurate.

The driver will print a warning if the quartz-load-femtofarads property
isn't set in DT (it can be either 7pF or 12.5pF). If I'm not mistaken,
the quartz oscillator on the CM4 I/O board is calibrated for a 6pF load
capacitance, so 7pF is the closest and best value. Could you confirm
that ?

> > > [1] https://lore.kernel.org/linux-arm-kernel/20211231115109.94626-1-uwe@kleine-koenig.org/
> > >
> > > Uwe Kleine-König (2):
> > >   ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0
> > >   ARM: dts: bcm2711-rpi-cm4-io: Add RTC on I2C0
> > >
> > >  arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts | 16 ++++++++++++
> > >  arch/arm/boot/dts/bcm2711-rpi.dtsi       | 31 ++++++++++++++++++++++++
> > >  2 files changed, 47 insertions(+)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/2] ARM: dts: bcm2711-rpi-cm4-io: Add rtc on a pinctrl-muxed i2c bus

[Dave Stevenson]

Hi Laurent

On Tue, 1 Aug 2023 at 21:33, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> On Tue, Aug 01, 2023 at 05:43:15PM +0300, Laurent Pinchart wrote:
> > On Tue, Aug 01, 2023 at 03:22:17PM +0100, Dave Stevenson wrote:
> > > On Mon, 31 Jul 2023 at 22:55, Laurent Pinchart wrote:
> > > >
> > > > Hello,
> > > >
> > > > This series is an attempt to revive support for pinmuxed I2C0 on the
> > > > Raspberry Pi BCM2711-based board.
> > > >
> > > > On BCM2711-based boards, the I2C0 controller can be muxed between pins
> > > > 0+1 or 44+45. The former is exposed through the 40-pins GPIO connector,
> > > > and the latter is used for the RTC on the CM4 I/O board, but also routed
> > > > to the display and camera connectors on the Raspberry Pi 4B board. The
> > > > other BCM2711-based board, the Raspberry Pi 400, does not expose or
> > > > connect anything to pins 44+45.
> > > >
> > > > A previous version was posted ([1]) a year and a half ago by Uwe. It
> > > > bundled the pinmuxing and RTC in a single patch, with the mux added to
> > > > the CM4 I/O board device tree. This version splits this in two, and
> > > > moves the pinumxing to the bcm2711-rpi.dtsi to also support the
> > > > Raspberry Pi 4B.
> > > >
> > > > The Raspberry Pi downstream kernel has a more complex DT architecture in
> > > > order to support different I2C0 pinmuxing for other boards. Two files,
> > > > bcm283x-rpi-i2c0mux_0_28.dtsi and bcm283x-rpi-i2c0mux_0_44.dtsi, define
> > > > the two I2C0 pinxmuxing options (pins 0+1 and 28+29, or pins 0+1 and
> > > > 44+45). Each board .dts then includes the appropriate file. I'm hoping
> > > > to avoid this additional complexity for now, by addressing the I2C0
> > > > pinmuxing for BCM2711-based boards only. If/when support for I2C0
> > > > pinmuxing on boards will be needed, we can revisit this topic.
> > > >
> > > > Compared to the Raspberry Pi downstream kernel, the two muxed I2C buses
> > > > are labelled i2c0_0 and i2c0_1 instead of i2c0 and i2c_csi_dsi. This
> > > > change was made to keep the naming of the I2C controller labels
> > > > consistent, avoiding renaming of the I2C0 controller's label from i2c0
> > > > to i2c0if.
> > > >
> > > > Dave, are you fine with the differences between this patch series and
> > > > the downstream kernel, or do you expect them to cause issues ?
> > >
> > > I've checked with Phil. There's nothing too untoward that will cause
> > > us any significant grief.
> >
> > Thanks for checking.
> >
> > In the meantime, I realized that the CM4S is 2711-based and, according
> > to the downstream DT, multiplexes I2C0 on pins 28+29, not 44+45 :-(
> > Umang and Kieran also told me that we want to test camera support on the
> > Pi 3B. It looks like the only viable approach to support all that will
> > be to include per-board I2C0 pinmux .dtsi as done in the downstream
> > kernel. I'll send a v5.
> >
> > > Phil has commented that the RTC is an PCF85063AT, so that compatible
> > > string should be "nxp,pcf85063a" if you actually want to make use of
> > > the alarm output.
> > > Then again the driver support for the alarm output appears to want it
> > > routed to an IRQ rather than as a system reset/wakeup, so it probably
> > > makes little difference. It llargely depends on how exact you want to
> > > be in your hardware description.
> >
> > I'll update the compatible string, it's an easy change and it's nice to
> > be accurate.
>
> The driver will print a warning if the quartz-load-femtofarads property
> isn't set in DT (it can be either 7pF or 12.5pF).

Really? The bindings [1] don't list it as required, and just trying it
hasn't logged an error.
The return value from of_property_read_u32 isn't checked at [2], so
load remains unchanged at 7000 if not present.

> If I'm not mistaken,
> the quartz oscillator on the CM4 I/O board is calibrated for a 6pF load
> capacitance, so 7pF is the closest and best value. Could you confirm
> that ?

The relevant hardware person isn't in at present, but schematics for
the RTC are on page 13 of the cm4io-datasheet [4].
Crystal is listed as a X32K768S300, and Farnell's copy of the
datasheet [4] say that has a 6pF load capacitance.

  Dave

[1] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/rtc/nxp%2Cpcf85063.yaml
[2] https://github.com/torvalds/linux/blob/master/drivers/rtc/rtc-pcf85063.c#L344
[3] https://datasheets.raspberrypi.com/cm4io/cm4io-datasheet.pdf
[4] https://www.farnell.com/datasheets/1881558.pdf

> > > > [1] https://lore.kernel.org/linux-arm-kernel/20211231115109.94626-1-uwe@kleine-koenig.org/
> > > >
> > > > Uwe Kleine-König (2):
> > > >   ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0
> > > >   ARM: dts: bcm2711-rpi-cm4-io: Add RTC on I2C0
> > > >
> > > >  arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts | 16 ++++++++++++
> > > >  arch/arm/boot/dts/bcm2711-rpi.dtsi       | 31 ++++++++++++++++++++++++
> > > >  2 files changed, 47 insertions(+)
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v4 0/2] ARM: dts: bcm2711-rpi-cm4-io: Add rtc on a pinctrl-muxed i2c bus

[Laurent Pinchart]

Hi Dave,

On Wed, Aug 02, 2023 at 11:20:32AM +0100, Dave Stevenson wrote:
> On Tue, 1 Aug 2023 at 21:33, Laurent Pinchart wrote:
> > On Tue, Aug 01, 2023 at 05:43:15PM +0300, Laurent Pinchart wrote:
> > > On Tue, Aug 01, 2023 at 03:22:17PM +0100, Dave Stevenson wrote:
> > > > On Mon, 31 Jul 2023 at 22:55, Laurent Pinchart wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > This series is an attempt to revive support for pinmuxed I2C0 on the
> > > > > Raspberry Pi BCM2711-based board.
> > > > >
> > > > > On BCM2711-based boards, the I2C0 controller can be muxed between pins
> > > > > 0+1 or 44+45. The former is exposed through the 40-pins GPIO connector,
> > > > > and the latter is used for the RTC on the CM4 I/O board, but also routed
> > > > > to the display and camera connectors on the Raspberry Pi 4B board. The
> > > > > other BCM2711-based board, the Raspberry Pi 400, does not expose or
> > > > > connect anything to pins 44+45.
> > > > >
> > > > > A previous version was posted ([1]) a year and a half ago by Uwe. It
> > > > > bundled the pinmuxing and RTC in a single patch, with the mux added to
> > > > > the CM4 I/O board device tree. This version splits this in two, and
> > > > > moves the pinumxing to the bcm2711-rpi.dtsi to also support the
> > > > > Raspberry Pi 4B.
> > > > >
> > > > > The Raspberry Pi downstream kernel has a more complex DT architecture in
> > > > > order to support different I2C0 pinmuxing for other boards. Two files,
> > > > > bcm283x-rpi-i2c0mux_0_28.dtsi and bcm283x-rpi-i2c0mux_0_44.dtsi, define
> > > > > the two I2C0 pinxmuxing options (pins 0+1 and 28+29, or pins 0+1 and
> > > > > 44+45). Each board .dts then includes the appropriate file. I'm hoping
> > > > > to avoid this additional complexity for now, by addressing the I2C0
> > > > > pinmuxing for BCM2711-based boards only. If/when support for I2C0
> > > > > pinmuxing on boards will be needed, we can revisit this topic.
> > > > >
> > > > > Compared to the Raspberry Pi downstream kernel, the two muxed I2C buses
> > > > > are labelled i2c0_0 and i2c0_1 instead of i2c0 and i2c_csi_dsi. This
> > > > > change was made to keep the naming of the I2C controller labels
> > > > > consistent, avoiding renaming of the I2C0 controller's label from i2c0
> > > > > to i2c0if.
> > > > >
> > > > > Dave, are you fine with the differences between this patch series and
> > > > > the downstream kernel, or do you expect them to cause issues ?
> > > >
> > > > I've checked with Phil. There's nothing too untoward that will cause
> > > > us any significant grief.
> > >
> > > Thanks for checking.
> > >
> > > In the meantime, I realized that the CM4S is 2711-based and, according
> > > to the downstream DT, multiplexes I2C0 on pins 28+29, not 44+45 :-(
> > > Umang and Kieran also told me that we want to test camera support on the
> > > Pi 3B. It looks like the only viable approach to support all that will
> > > be to include per-board I2C0 pinmux .dtsi as done in the downstream
> > > kernel. I'll send a v5.
> > >
> > > > Phil has commented that the RTC is an PCF85063AT, so that compatible
> > > > string should be "nxp,pcf85063a" if you actually want to make use of
> > > > the alarm output.
> > > > Then again the driver support for the alarm output appears to want it
> > > > routed to an IRQ rather than as a system reset/wakeup, so it probably
> > > > makes little difference. It llargely depends on how exact you want to
> > > > be in your hardware description.
> > >
> > > I'll update the compatible string, it's an easy change and it's nice to
> > > be accurate.
> >
> > The driver will print a warning if the quartz-load-femtofarads property
> > isn't set in DT (it can be either 7pF or 12.5pF).
> 
> Really? The bindings [1] don't list it as required, and just trying it
> hasn't logged an error.
> The return value from of_property_read_u32 isn't checked at [2], so
> load remains unchanged at 7000 if not present.

My bad, I've misread the code and missed the initialization of the
variable.

> > If I'm not mistaken,
> > the quartz oscillator on the CM4 I/O board is calibrated for a 6pF load
> > capacitance, so 7pF is the closest and best value. Could you confirm
> > that ?
> 
> The relevant hardware person isn't in at present, but schematics for
> the RTC are on page 13 of the cm4io-datasheet [4].
> Crystal is listed as a X32K768S300, and Farnell's copy of the
> datasheet [4] say that has a 6pF load capacitance.

That's what I found too :-) The default value of 7pF seems fine then.

> [1] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/rtc/nxp%2Cpcf85063.yaml
> [2] https://github.com/torvalds/linux/blob/master/drivers/rtc/rtc-pcf85063.c#L344
> [3] https://datasheets.raspberrypi.com/cm4io/cm4io-datasheet.pdf
> [4] https://www.farnell.com/datasheets/1881558.pdf
> 
> > > > > [1] https://lore.kernel.org/linux-arm-kernel/20211231115109.94626-1-uwe@kleine-koenig.org/
> > > > >
> > > > > Uwe Kleine-König (2):
> > > > >   ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0
> > > > >   ARM: dts: bcm2711-rpi-cm4-io: Add RTC on I2C0
> > > > >
> > > > >  arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts | 16 ++++++++++++
> > > > >  arch/arm/boot/dts/bcm2711-rpi.dtsi       | 31 ++++++++++++++++++++++++
> > > > >  2 files changed, 47 insertions(+)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/2] ARM: dts: bcm2711-rpi-cm4-io: Add rtc on a pinctrl-muxed i2c bus

[Laurent Pinchart]

On Tue, Aug 01, 2023 at 05:43:15PM +0300, Laurent Pinchart wrote:
> On Tue, Aug 01, 2023 at 03:22:17PM +0100, Dave Stevenson wrote:
> > On Mon, 31 Jul 2023 at 22:55, Laurent Pinchart wrote:
> > >
> > > Hello,
> > >
> > > This series is an attempt to revive support for pinmuxed I2C0 on the
> > > Raspberry Pi BCM2711-based board.
> > >
> > > On BCM2711-based boards, the I2C0 controller can be muxed between pins
> > > 0+1 or 44+45. The former is exposed through the 40-pins GPIO connector,
> > > and the latter is used for the RTC on the CM4 I/O board, but also routed
> > > to the display and camera connectors on the Raspberry Pi 4B board. The
> > > other BCM2711-based board, the Raspberry Pi 400, does not expose or
> > > connect anything to pins 44+45.
> > >
> > > A previous version was posted ([1]) a year and a half ago by Uwe. It
> > > bundled the pinmuxing and RTC in a single patch, with the mux added to
> > > the CM4 I/O board device tree. This version splits this in two, and
> > > moves the pinumxing to the bcm2711-rpi.dtsi to also support the
> > > Raspberry Pi 4B.
> > >
> > > The Raspberry Pi downstream kernel has a more complex DT architecture in
> > > order to support different I2C0 pinmuxing for other boards. Two files,
> > > bcm283x-rpi-i2c0mux_0_28.dtsi and bcm283x-rpi-i2c0mux_0_44.dtsi, define
> > > the two I2C0 pinxmuxing options (pins 0+1 and 28+29, or pins 0+1 and
> > > 44+45). Each board .dts then includes the appropriate file. I'm hoping
> > > to avoid this additional complexity for now, by addressing the I2C0
> > > pinmuxing for BCM2711-based boards only. If/when support for I2C0
> > > pinmuxing on boards will be needed, we can revisit this topic.
> > >
> > > Compared to the Raspberry Pi downstream kernel, the two muxed I2C buses
> > > are labelled i2c0_0 and i2c0_1 instead of i2c0 and i2c_csi_dsi. This
> > > change was made to keep the naming of the I2C controller labels
> > > consistent, avoiding renaming of the I2C0 controller's label from i2c0
> > > to i2c0if.
> > >
> > > Dave, are you fine with the differences between this patch series and
> > > the downstream kernel, or do you expect them to cause issues ?
> > 
> > I've checked with Phil. There's nothing too untoward that will cause
> > us any significant grief.
> 
> Thanks for checking.
> 
> In the meantime, I realized that the CM4S is 2711-based and, according
> to the downstream DT, multiplexes I2C0 on pins 28+29, not 44+45 :-(
> Umang and Kieran also told me that we want to test camera support on the
> Pi 3B. It looks like the only viable approach to support all that will
> be to include per-board I2C0 pinmux .dtsi as done in the downstream
> kernel. I'll send a v5.

This I2C pin muxing is getting trickier than I initially thought :-S
Naming the child buses i2c0_0 and i2c0_1 means that boards exposing I2C0
on the 40-pins GPIO headers would have different labels for that I2C0
interface depending on whether they multiplex I2C0 on other pins (e.g.
on pins 44+45 for the camera connector of the Pi 4B) or not (e.g. the Pi
400). Overlays would thus not be compatible, which I suppose would be
annoying for users.

Before I send a new version of the series, I'd like to know what
direction I should take. Is overlay compatibility between the two cases
listed above important ? If so, I can imagine at least two solutions:

- On boards that use I2C0 pinmuxing, we can rename the physical
  interface to i2c0if and name the two child buses i2c0 and i2c_csi_dsi.
  This is the solution used in the downstream kernel. It is implemented
  with a crude (in my opinion) hack though: board files have constructs
  such as

  #define i2c0 i2c0if
  #include "bcm2711.dtsi"
  #undef i2c0

- Always mux the I2C0 bus, on pins 0+1 and 28+29 or 0+1 and 44+45,
  depending on the board. Boards that don't connect anything to the I2C
  bus on pins 28+29 or 44+45 would still pick one of the two options. As
  there will be no I2C device on the second bus, the corresponding
  pinctrl entry will never be selected, so this should be harmless. The
  downside is that there will be an I2C mux instantiated in the kernel,
  adding a level of indirection to the I2C operations.

Other solutions may be possible.

What would be everybody's favourite option ?

> > Phil has commented that the RTC is an PCF85063AT, so that compatible
> > string should be "nxp,pcf85063a" if you actually want to make use of
> > the alarm output.
> > Then again the driver support for the alarm output appears to want it
> > routed to an IRQ rather than as a system reset/wakeup, so it probably
> > makes little difference. It llargely depends on how exact you want to
> > be in your hardware description.
> 
> I'll update the compatible string, it's an easy change and it's nice to
> be accurate.
> 
> > > [1] https://lore.kernel.org/linux-arm-kernel/20211231115109.94626-1-uwe@kleine-koenig.org/
> > >
> > > Uwe Kleine-König (2):
> > >   ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0
> > >   ARM: dts: bcm2711-rpi-cm4-io: Add RTC on I2C0
> > >
> > >  arch/arm/boot/dts/bcm2711-rpi-cm4-io.dts | 16 ++++++++++++
> > >  arch/arm/boot/dts/bcm2711-rpi.dtsi       | 31 ++++++++++++++++++++++++
> > >  2 files changed, 47 insertions(+)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/2] ARM: dts: bcm2711-rpi-cm4-io: Add rtc on a pinctrl-muxed i2c bus

[Stefan Wahren]

Hi Laurent,

Am 01.08.23 um 23:05 schrieb Laurent Pinchart:
> On Tue, Aug 01, 2023 at 05:43:15PM +0300, Laurent Pinchart wrote:
>> On Tue, Aug 01, 2023 at 03:22:17PM +0100, Dave Stevenson wrote:
>>> On Mon, 31 Jul 2023 at 22:55, Laurent Pinchart wrote:
>>>>
>>>> Hello,
>>>>
>>>> This series is an attempt to revive support for pinmuxed I2C0 on the
>>>> Raspberry Pi BCM2711-based board.
>>>>
>>>> On BCM2711-based boards, the I2C0 controller can be muxed between pins
>>>> 0+1 or 44+45. The former is exposed through the 40-pins GPIO connector,
>>>> and the latter is used for the RTC on the CM4 I/O board, but also routed
>>>> to the display and camera connectors on the Raspberry Pi 4B board. The
>>>> other BCM2711-based board, the Raspberry Pi 400, does not expose or
>>>> connect anything to pins 44+45.
>>>>
>>>> A previous version was posted ([1]) a year and a half ago by Uwe. It
>>>> bundled the pinmuxing and RTC in a single patch, with the mux added to
>>>> the CM4 I/O board device tree. This version splits this in two, and
>>>> moves the pinumxing to the bcm2711-rpi.dtsi to also support the
>>>> Raspberry Pi 4B.
>>>>
>>>> The Raspberry Pi downstream kernel has a more complex DT architecture in
>>>> order to support different I2C0 pinmuxing for other boards. Two files,
>>>> bcm283x-rpi-i2c0mux_0_28.dtsi and bcm283x-rpi-i2c0mux_0_44.dtsi, define
>>>> the two I2C0 pinxmuxing options (pins 0+1 and 28+29, or pins 0+1 and
>>>> 44+45). Each board .dts then includes the appropriate file. I'm hoping
>>>> to avoid this additional complexity for now, by addressing the I2C0
>>>> pinmuxing for BCM2711-based boards only. If/when support for I2C0
>>>> pinmuxing on boards will be needed, we can revisit this topic.
>>>>
>>>> Compared to the Raspberry Pi downstream kernel, the two muxed I2C buses
>>>> are labelled i2c0_0 and i2c0_1 instead of i2c0 and i2c_csi_dsi. This
>>>> change was made to keep the naming of the I2C controller labels
>>>> consistent, avoiding renaming of the I2C0 controller's label from i2c0
>>>> to i2c0if.
>>>>
>>>> Dave, are you fine with the differences between this patch series and
>>>> the downstream kernel, or do you expect them to cause issues ?
>>>
>>> I've checked with Phil. There's nothing too untoward that will cause
>>> us any significant grief.
>>
>> Thanks for checking.
>>
>> In the meantime, I realized that the CM4S is 2711-based and, according
>> to the downstream DT, multiplexes I2C0 on pins 28+29, not 44+45 :-(
>> Umang and Kieran also told me that we want to test camera support on the
>> Pi 3B. It looks like the only viable approach to support all that will
>> be to include per-board I2C0 pinmux .dtsi as done in the downstream
>> kernel. I'll send a v5.
>
> This I2C pin muxing is getting trickier than I initially thought :-S
> Naming the child buses i2c0_0 and i2c0_1 means that boards exposing I2C0
> on the 40-pins GPIO headers would have different labels for that I2C0
> interface depending on whether they multiplex I2C0 on other pins (e.g.
> on pins 44+45 for the camera connector of the Pi 4B) or not (e.g. the Pi
> 400). Overlays would thus not be compatible, which I suppose would be
> annoying for users.
>
> Before I send a new version of the series, I'd like to know what
> direction I should take. Is overlay compatibility between the two cases
> listed above important ? If so, I can imagine at least two solutions:
>
> - On boards that use I2C0 pinmuxing, we can rename the physical
>    interface to i2c0if and name the two child buses i2c0 and i2c_csi_dsi.
>    This is the solution used in the downstream kernel. It is implemented
>    with a crude (in my opinion) hack though: board files have constructs
>    such as
>
>    #define i2c0 i2c0if
>    #include "bcm2711.dtsi"
>    #undef i2c0
>
> - Always mux the I2C0 bus, on pins 0+1 and 28+29 or 0+1 and 44+45,
>    depending on the board. Boards that don't connect anything to the I2C
>    bus on pins 28+29 or 44+45 would still pick one of the two options. As
>    there will be no I2C device on the second bus, the corresponding
>    pinctrl entry will never be selected, so this should be harmless. The
>    downside is that there will be an I2C mux instantiated in the kernel,
>    adding a level of indirection to the I2C operations.
>
> Other solutions may be possible.
>
> What would be everybody's favourite option ?
>

sorry that you didn't get a reply to this question so far. Yesterday i
tried to get my official rpi touchscreen running and noticed those many
missing puzzle pieces (like this one) in Mainline.

My gut feeling would tend to the second option.

Best regards