Re: [PATCH v6 12/15] ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0

[Dave Stevenson <dave.stevenson@raspberrypi.com>]

Hi Laurent

On Fri, 1 Mar 2024 at 21:32, 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.

It's true for all Pis that I2C0 is exposed over 2 sets of gpios.
Seeing as we want to support cameras on Pi0-3, is there a reason not
to include the mux on those?

Looking back I had started this way back in [1] with all the variants.
I thought I'd posted the v2 follow up, but can't find it.
The original Pi 1 models A & B were the annoyances. The rev1 put the
camera on i2c1 GPIOs 2&3, with the rev2 on i2c0 with GPIOs 0&1.

Whilst it would be nice to have support for all platforms, this
doesn't stop us moving the mux into bcm283x-rpi.dtsi at a later date
to add support for the other devices.
I'm happy enough having the first step of getting Pi4 working, with
others being done later.

[1] https://linux-rpi-kernel.infradead.narkive.com/lmzYlT3c/rfc-arm-dts-add-i2cmux-pinctrl-config-to-raspberry-pi-i2c-0

> 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/broadcom/bcm2711-rpi.dtsi | 31 +++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi b/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
> index 86188eabeb24..826ed6efa9ff 100644
> --- a/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
> +++ b/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
> @@ -17,6 +17,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";

Why defaulting to disabled?

The current mainline DT defaults to i2c0 being enabled on GPIOs 0&1
(done via bcm2835-rpi.dtsi).
If the mux is disabled, then this change has left i2c0 being enabled
but with no pinctrl property, so it's not connected to the outside
world.
GPIOs 44&45 have never had any other user, therefore claiming them for
the mux isn't a regression in my view.


As long as we can enable the other platforms later, and with the minor
caveat over being enabled or not:

Acked-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Minor point that CONFIG_I2C_MUX_PINCTRL appears not to be in the arm64
defconfig. I don't know what the policy is there, but there seem to be
many other SoCs throwing modules in there for their configurations.
It is in arm/multi_v7_defconfig.

  Dave

> +
> +               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 +75,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
>