Re: [RFC PATCH v3 03/11] media: dt-bindings: media: Add bindings for bcm2835-unicam

Hi Jean-Michel and Laurent

n Wed, 2 Feb 2022 at 22:36, Laurent Pinchart
[off-list ref] wrote:

Hi Jean-Michel,

On Wed, Feb 02, 2022 at 11:09:20PM +0100, Jean-Michel Hautbois wrote:

quoted

On 02/02/2022 19:33, Stefan Wahren wrote:

quoted

Hi Jean-Michel,

please drop the first "media:" before dt-bindings.

Am 02.02.22 um 18:56 schrieb Jean-Michel Hautbois:

quoted

Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
camera interface. Also add a MAINTAINERS entry for it.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Naushir Patuck <redacted>
Signed-off-by: Jean-Michel Hautbois <redacted>
---
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 [off-list ref]"

Probably easiest to switch to "Raspberry Pi Kernel Maintenance
[off-list ref]".
That list didn't exist when I originally wrote the doc, and it just
makes life easier should I decide to move on (not planning it). Naush
is on that list too.

quoted

quoted

quoted

---
  .../bindings/media/brcm,bcm2835-unicam.yaml   | 107 ++++++++++++++++++
  MAINTAINERS                                   |   7 ++
  2 files changed, 114 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..5bf41a8834fa
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml

@@ -0,0 +1,107 @@
+# 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 starting by csi then
+  it will stop the firmware accessing the block, and it can then
+  safely be used via the device tree binding.
+
+properties:
+  compatible:
+    const: brcm,bcm2835-unicam
+
+  reg:
+    maxItems: 2

I would be nice to have reg-names here similar to the clocks.

Sure, I just don't know what the names are ;-).

Please discuss this with the Rasperry Pi developers to figure out then.

It's the "Unicam" and "Clock Manager Image" (CMI) blocks respectively.

CMI is only 4 registers. It provides high speed clock source selection
for the two Unicam blocks, a camera test block that has never been
used, and one of the USB controllers. Each peripheral is controlled by
a separate register.
It was discussed previously and viewed as not worthwhile creating a
full clock driver for it.

quoted

quoted

quoted

+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Clock for the camera.

This also seems weird, as far as I know the SoC doesn't output a clock
for the camera sensor (and it should be specified in the camera sensor
DT node if it did anyway).

It's the clocks to Unicam, not to the camera / sensor.

The LP clock drives the LP state machine of Unicam for the relevant
DPHY state transitions.
The VPU or core clock is needed to ensure that the other bus systems
are running fast enough for the data generated.

  Dave

quoted

quoted

quoted

+      - description: Clock for the vpu.
+
+  clock-names:
+    items:
+      - const: lp
+      - const: vpu
+
+  power-domains:
+    items:
+      - description: Unicam power domain
+
+  num-data-lanes:

This is a vendor-specific property and thus requires a vendor prefix.

quoted

quoted

quoted

+    items:
+      - enum: [ 2, 4 ]
+
+  port:
+    additionalProperties: false
+    $ref: /schemas/graph.yaml#/$defs/port-base
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes: true
+          link-frequencies: true
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - power-domains
+  - num-data-lanes
+  - port
+
+additionalProperties: False
+
+examples:
+  - |
+    #include <dt-bindings/clock/bcm2835.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/raspberrypi-power.h>
+    csi1: csi@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>;
+        num-data-lanes = <2>;
+        port {
+                csi1_ep: endpoint {
+                        remote-endpoint = <&imx219_0>;
+                        data-lanes = <1 2>;
+                        link-frequencies = /bits/ 64 <456000000>;
+                };
+        };
+    };
+...

diff --git a/MAINTAINERS b/MAINTAINERS
index a0770a861ca4..29344ea86847 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -3670,6 +3670,13 @@ N:  bcm113*
  N:       bcm216*
  N:       kona

+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
+F:        arch/arm/boot/dts/bcm283x*
+

I suggest to make the MAINTAINERS changes a single separate patch
instead of small incremental changes.

I can make it a separate patch, indeed.

quoted

quoted

  BROADCOM BCM47XX MIPS ARCHITECTURE
  M:       Hauke Mehrtens [off-list ref]
  M:       Rafał Miłecki [off-list ref]

--
Regards,

Laurent Pinchart