Message ID | 20220121081810.155500-1-jeanmichel.hautbois@ideasonboard.com |
---|---|
Headers | show |
Series | Add support for BCM2835 camera interface (unicam) | expand |
Hi Laurent, On 22/01/2022 00:27, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Fri, Jan 21, 2022 at 09:18:06AM +0100, Jean-Michel Hautbois wrote: >> Introduce the dt-bindinds documentation for bcm2835 CCP2/CSI2 camera > > s/bindinds/bindings/ > > I'd mention "Unicam" somewhere here. > >> interface. Also add a MAINTAINERS entry for it. Oh my, I tis not the right dts bindings patch, I mixed up my trees... :-/ Sorry for this I will send a v2.1 soon... >> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> >> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> Dave: I assumed you were the maintainer for this file, as I based it on the >> bcm2835-unicam.txt file. Are you happy to be added directly as the >> maintainer, or should this be specified as "Raspberry Pi Kernel >> Maintenance <kernel-list@raspberrypi.com>" >> - in v2: multiple corrections to pass the bot checking as Rob kindly >> told me. >> --- >> .../bindings/media/brcm,bcm2835-unicam.yaml | 103 ++++++++++++++++++ >> MAINTAINERS | 6 + >> 2 files changed, 109 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml >> >> diff --git a/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml >> new file mode 100644 >> index 000000000000..1427514142cf >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml >> @@ -0,0 +1,103 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/brcm,bcm2835-unicam.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Broadcom BCM283x Camera Interface (Unicam) >> + >> +maintainers: >> + - Dave Stevenson <dave.stevenson@raspberrypi.com> >> + >> +description: |- >> + The Unicam block on BCM283x SoCs is the receiver for either >> + CSI-2 or CCP2 data from image sensors or similar devices. >> + >> + The main platform using this SoC is the Raspberry Pi family of boards. >> + On the Pi the VideoCore firmware can also control this hardware block, >> + and driving it from two different processors will cause issues. >> + To avoid this, the firmware checks the device tree configuration >> + during boot. If it finds device tree nodes called csi0 or csi1 then >> + it will stop the firmware accessing the block, and it can then >> + safely be used via the device tree binding. > > As mentioned in the review of the DT integration, the nodes should > ideally be called just "csi", not "csi0" and "csi1" (maybe Rob could > confirm this ?). Dave, is there a way the firmware could be updated to > also hand over control of the Unicam instances to Linux when a "csi" > node is found, not just "csi0" or "csi1" ? > > Given that the node names are significant, they should be enforced in > the YAML schema. > >> + >> +properties: >> + compatible: >> + const: brcm,bcm2835-unicam >> + >> + reg: >> + description: >> + physical base address and length of the register sets for the device. > > This can be dropped. > >> + maxItems: 1 > > There are two items in the example below. How does this validate ? > >> + >> + interrupts: >> + description: the IRQ line for this Unicam instance. > > This can be dropped. > >> + maxItems: 1 >> + >> + clocks: >> + description: |- >> + list of clock specifiers, corresponding to entries in clock-names >> + property. > > clocks: > items: > - description: The clock for ... > - description: The clock for ... > > (with the two descriptions matching the LP and VPU clocks, I don't know > what they are). > >> + >> + clock-names: >> + items: >> + - const: lp >> + - const: vpu >> + >> + port: >> + $ref: /schemas/graph.yaml#/properties/port >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - clocks >> + - clock-names >> + - port >> + >> +additionalProperties: False >> + >> +examples: >> + - | >> + csi1: csi1@7e801000 { >> + compatible = "brcm,bcm2835-unicam"; >> + reg = <0x7e801000 0x800>, >> + <0x7e802004 0x4>; >> + interrupts = <2 7>; > > Let's use the Pi 4 device tree as an example, as that's what we're > upstreaming first. > >> + clocks = <&clocks BCM2835_CLOCK_CAM1>, > > This will fail to compile without a proper #include, did you get this to > pass validation ? > >> + <&firmware_clocks 4>; >> + clock-names = "lp", "vpu"; >> + port { >> + csi1_ep: endpoint { >> + remote-endpoint = <&tc358743_0>; >> + data-lanes = <1 2>; >> + }; >> + }; >> + }; >> + >> + i2c0: i2c@7e205000 { >> + tc358743: csi-hdmi-bridge@0f { >> + compatible = "toshiba,tc358743"; >> + reg = <0x0f>; >> + clocks = <&tc358743_clk>; >> + clock-names = "refclk"; >> + >> + tc358743_clk: bridge-clk { >> + compatible = "fixed-clock"; >> + #clock-cells = <0>; >> + clock-frequency = <27000000>; >> + }; >> + >> + port { >> + tc358743_0: endpoint { >> + remote-endpoint = <&csi1_ep>; >> + clock-lanes = <0>; >> + data-lanes = <1 2>; >> + clock-noncontinuous; >> + link-frequencies = >> + /bits/ 64 <297000000>; >> + }; >> + }; >> + }; >> + }; > > I'd drop this node completely. > >> +... >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 33f75892f98e..07f238fd5ff9 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -3679,6 +3679,12 @@ F: Documentation/media/v4l-drivers/bcm2835-isp.rst >> F: drivers/staging/vc04_services/bcm2835-isp >> F: include/uapi/linux/bcm2835-isp.h >> >> +BROADCOM BCM2835 CAMERA DRIVER >> +M: Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com> >> +L: linux-media@vger.kernel.org >> +S: Maintained >> +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml >> + >> BROADCOM BCM47XX MIPS ARCHITECTURE >> M: Hauke Mehrtens <hauke@hauke-m.de> >> M: Rafał Miłecki <zajec5@gmail.com> >
Hi Dave, On Mon, Jan 24, 2022 at 12:31:34PM +0000, Dave Stevenson wrote: > On Fri, 21 Jan 2022 at 22:45, Laurent Pinchart wrote: > > 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@ideasonboard.com> > > > --- > > > 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". Oops, indeed. I think I was misled by https://github.com/raspberrypi/linux/blob/rpi-5.10.y/Documentation/devicetree/bindings/media/bcm2835-unicam.txt that mentions "csi0" and "csi1". It's all good then. Jean-Michel, can you update the DT bindings in the next iteration of the series to correct the DT node naming ? > 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>;