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

[Florian Fainelli <f.fainelli@gmail.com>]

On 1/19/2022 1:44 AM, Dave Stevenson wrote:
> Hi Laurent and Uwe.
> 
> On Tue, 18 Jan 2022 at 22:59, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Uwe,
>>
>> On Tue, Jan 18, 2022 at 11:41:19PM +0100, Uwe Kleine-König wrote:
>>> On 1/18/22 21:47, Laurent Pinchart wrote:
>>>> On Tue, Jan 18, 2022 at 12:00:50PM -0800, Florian Fainelli wrote:
>>>>> On 1/18/22 11:45 AM, Jean-Michel Hautbois wrote:
>>>>>> This is also needed for camera and display support.
>>>>>> I tested it successfully with imx219 + unicam on mainline.
>>>>>
>>>>> Thanks for testing, can you reply with a Tested-by tag so it could be
>>>>> applied to the commit message when this gets picked up?
>>>>
>>>> Well, this also points out that there's an issue: if the mux is needed
>>>> for other devices, it shouldn't be in bcm2711-rpi-cm4-io.dts :-) We
>>>> could move it to bcm2711-rpi.dtsi (so far all bcm2711-based boards use
>>>> either I/O pins 0+1 or 44+45)
>>>
>>> If I understand correctly it's not used on rpi-4-b, so bcm2711-rpi.dtsi
>>> would be wrong.
>>
>> rpi-4-b muxes I2C0 on pins 0+1 and 44+45. The latter is wired to the
>> camera connector, and used for the camera sensor. Same thing on rpi-cm4.
>> rpi-400 has no camera connector, but I believe the display I2C bus is
>> also on pins 44+45 (at least according to the downstream DT sources,
>> rpi-400 muxes I2C0 on 0+1 and 44+45 too).
> 
> Minor correction - Pi 400 has no camera or display connector.
> I'm double checking with the hardware folk, but AFAIK 44&45 aren't
> used for any other purpose, so leaving the i2c0mux defined to them
> doesn't cause any issues.
> 
> Camera and DSI display connectors on all Raspberry Pi regular boards
> share the same I2C GPIOs muxing. (The original Pi model A & B didn't
> route I2C to the display).
> On a CMIO board, CAM0 & DISP0 connectors share an I2C muxing (normally
> 0&1), and CAM1 & DISP1 connectors share a muxing (either 28&29, or
> 44&45).
> 
>>>> , or move it to per-board files.
>>>
>>> It is in an board file now?! So I don't understand your suggestion here.
>>
>> Sorry, I meant have it in per-board files, not more it there.
>>
>>>> In the
>>>> latter case, instead of duplicating the same block everywhere, it could
>>>> be moved to a .dtsi included in those board files. This is what the
>>>> downstream kernel does.
>>>
>>> How does it call the dtsi file? I wonder if that is sensible expecting
>>> that the devices on the bus are different for different boards?!
>>
>> Downstream has a bcm283x-rpi-i2c0mux_0_44.dtsi that just contains
>>
>> &i2c0mux {
>>          pinctrl-0 = <&i2c0_gpio0>;
>>          pinctrl-1 = <&i2c0_gpio44>;
>> };
>>
>> with i2c0mux defined in bcm283x.dtsi as
>>
>>          i2c0mux: i2c0mux {
>>                  compatible = "i2c-mux-pinctrl";
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>>
>>                  i2c-parent = <&i2c0if>;
>>
>>                  pinctrl-names = "i2c0", "i2c_csi_dsi";
>>
>>                  status = "disabled";
>>
>>                  i2c0: i2c@0 {
>>                          reg = <0>;
>>                          #address-cells = <1>;
>>                          #size-cells = <0>;
>>                  };
>>
>>                  i2c_csi_dsi: i2c@1 {
>>                          reg = <1>;
>>                          #address-cells = <1>;
>>                          #size-cells = <0>;
>>                  };
>>          };
>>
>> The following board files #include "bcm283x-rpi-i2c0mux_0_44.dtsi":
>>
>> - bcm2710-rpi-3-b.dts
>> - bcm2710-rpi-3-b-plus.dts
>> - bcm2710-rpi-zero-2-w.dts
>> - bcm2711-rpi-400.dts
>> - bcm2711-rpi-4-b.dts
>> - bcm2711-rpi-4-b.dts.orig
>> - bcm2711-rpi-cm4.dts
> 
> For completeness, the earlier boards use a
> bcm283x-rpi-i2c0mux_0_28.dtsi file as they use GPIOs 28&29 for the
> camera & display I2C. If my memory serves correctly, it had to move to
> allow connecting the SDIO interface to the Wifi chip on the boards you
> list above.
> The one awkward one is the very original rev1 256MB Model B (and 128MB
> model A?) which routed GPIOs 2&3 and BSC1 to the camera connector.
> Life's never easy!
> 
>    Dave
> 
>> We don't have to replicate the exact same mechanism and use the same
>> names, but for rpi-4-b and rpi-cm4, to enable camera support (which
>> we're working on, Jean-Michel has posted a driver for the Unicam CSI-2
>> receiver to the linux-media mailing list, the ISP will follow), we need
>> the mux. Given that those two boards have a camera connector, I think it
>> makes sense to define the mux in a different file than
>> bcm2711-rpi-cm4-io.dts. The RTC node can stay in bcm2711-rpi-cm4-io.dts.

Uwe, were you going to follow Laurent's suggestion and create a specific 
file that other boards can include? You have a few more days before I 
sent the pull request for 5.18. Thanks!
-- 
Florian