Re: [RFC PATCH v2 5/7] ARM: dts: bcm2711: Add unicam CSI nodes

[Dave Stevenson]

Hi Laurent and Jean-Michel

On Fri, 21 Jan 2022 at 22:45, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> On Fri, Jan 21, 2022 at 09:18:08AM +0100, Jean-Michel Hautbois wrote:
> > Add both MIPI CSI-2 nodes in the core bcm2711 tree. Use the 3-cells
> > interrupt declaration, corresponding clocks and default as disabled.
> >
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxxxxxxxx>
> > ---
> >  arch/arm/boot/dts/bcm2711.dtsi | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
> > index dff18fc9a906..077141df7024 100644
> > --- a/arch/arm/boot/dts/bcm2711.dtsi
> > +++ b/arch/arm/boot/dts/bcm2711.dtsi
> > @@ -3,6 +3,7 @@
> >
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  #include <dt-bindings/soc/bcm2835-pm.h>
> > +#include <dt-bindings/power/raspberrypi-power.h>
> >
> >  / {
> >       compatible = "brcm,bcm2711";
> > @@ -293,6 +294,36 @@ hvs: hvs@7e400000 {
> >                       interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
> >               };
> >
> > +             csi0: csi1@7e800000 {
>
> The node name should be csi@7e800000, not csi1@7e800000. Now, this will
> probably cause issues with the firmware that looks for csi1 (and csi0 ?)
> to hand over control of the Unicam CSI-2 receiver to the kernel. I
> wonder if this is something that could be handled by a firmware update,
> to also recognize nodes named "csi" ?

It already looks for any node starting "csi". If you check the
downstream DT [1], then the nodes are "csi0: csi@7e800000" and "csi1:
csi@7e801000".

There is no actual action required to hand the peripheral over to the
kernel, it just prevents the firmware from using it and causing
problems (it masks out the interrupt, and that's checked as part of
the firmware initialising the peripheral).

If using imx219 or one of the other sensors supported by the firmware,
"vcgencmd get_camera" should report that the sensor isn't detected,
and "sudo vcdbg log msg" should have a line similar to
"020174.613: camsubs: Ignoring camera 0 as unicam device not available"

  Dave

[1] https://github.com/raspberrypi/linux/blob/rpi-5.10.y/arch/arm/boot/dts/bcm270x.dtsi#L88

> > +                     compatible = "brcm,bcm2835-unicam";
> > +                     reg = <0x7e800000 0x800>,
> > +                           <0x7e802000 0x4>;
> > +                     interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
> > +                     clocks = <&clocks BCM2835_CLOCK_CAM0>,
> > +                              <&firmware_clocks 4>;
> > +                     clock-names = "lp", "vpu";
> > +                     power-domains = <&power RPI_POWER_DOMAIN_UNICAM0>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     #clock-cells = <1>;
>
> Why do you need #address-cells, #size-cells and #clock-cells ? They're
> not mentioned in the binding.
>
> > +                     status="disabled";
>
> Missing spaces around the =.
>
> Same comment for the next node.
>
> > +             };
> > +
> > +             csi1: csi1@7e801000 {
> > +                     compatible = "brcm,bcm2835-unicam";
> > +                     reg = <0x7e801000 0x800>,
> > +                           <0x7e802004 0x4>;
> > +                     interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > +                     clocks = <&clocks BCM2835_CLOCK_CAM1>,
> > +                              <&firmware_clocks 4>;
> > +                     clock-names = "lp", "vpu";
> > +                     power-domains = <&power RPI_POWER_DOMAIN_UNICAM1>;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +                     #clock-cells = <1>;
> > +                     status="disabled";
> > +             };
> > +
> >               pixelvalve3: pixelvalve@7ec12000 {
> >                       compatible = "brcm,bcm2711-pixelvalve3";
> >                       reg = <0x7ec12000 0x100>;
>
> --
> Regards,
>
> Laurent Pinchart