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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dave,

On Mon, Mar 18, 2024 at 02:56:33PM +0000, Dave Stevenson wrote:
> On Fri, 1 Mar 2024 at 21:32, Laurent Pinchart wrote:
> >
> > From: Uwe Kleine-König <uwe@xxxxxxxxxxxxxxxxx>
> >
> > 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?

Simplicity :-) I got lost in the maze of differences in .dtsi files
between the upstream and downstream kernels. Given that not all Pi's
have device trees upstream, I decided to start simple(r).

> 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.

Thanks :-) I would also be happy for other boards to get I2C0 mux
support 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@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > 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.

I don't recall why I disabled it. Your explanation makes sense, I'll
drop the status property.

> 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@xxxxxxxxxxxxxxx>

Thank you. I'll send a new version of the series soon, Florian wanted to
pick the DT integration sooner than later.

> 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.

Good question.

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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux