2022-01-22 00:40:25

by Jean-Michel Hautbois

[permalink] [raw]
Subject: [RFC PATCH v2 3/7] media: dt-bindings: media: Add bindings for bcm2835-unicam

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

Signed-off-by: Dave Stevenson <[email protected]>
Signed-off-by: Naushir Patuck <[email protected]>
Signed-off-by: Jean-Michel Hautbois <[email protected]>
---
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 <[email protected]>"
- 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 <[email protected]>
+
+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.
+
+properties:
+ compatible:
+ const: brcm,bcm2835-unicam
+
+ reg:
+ description:
+ physical base address and length of the register sets for the device.
+ maxItems: 1
+
+ interrupts:
+ description: the IRQ line for this Unicam instance.
+ maxItems: 1
+
+ clocks:
+ description: |-
+ list of clock specifiers, corresponding to entries in clock-names
+ property.
+
+ 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>;
+ clocks = <&clocks BCM2835_CLOCK_CAM1>,
+ <&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>;
+ };
+ };
+ };
+ };
+...
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 <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
+
BROADCOM BCM47XX MIPS ARCHITECTURE
M: Hauke Mehrtens <[email protected]>
M: Rafał Miłecki <[email protected]>
--
2.32.0


2022-01-22 18:40:09

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [RFC PATCH v2 3/7] media: dt-bindings: media: Add bindings for bcm2835-unicam

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.
>
> Signed-off-by: Dave Stevenson <[email protected]>
> Signed-off-by: Naushir Patuck <[email protected]>
> Signed-off-by: Jean-Michel Hautbois <[email protected]>
> ---
> 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 <[email protected]>"
> - 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 <[email protected]>
> +
> +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 <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> +
> BROADCOM BCM47XX MIPS ARCHITECTURE
> M: Hauke Mehrtens <[email protected]>
> M: Rafał Miłecki <[email protected]>

--
Regards,

Laurent Pinchart

2022-01-23 14:53:50

by Jean-Michel Hautbois

[permalink] [raw]
Subject: Re: [RFC PATCH v2 3/7] media: dt-bindings: media: Add bindings for bcm2835-unicam

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 <[email protected]>
>> Signed-off-by: Naushir Patuck <[email protected]>
>> Signed-off-by: Jean-Michel Hautbois <[email protected]>
>> ---
>> 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 <[email protected]>"
>> - 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 <[email protected]>
>> +
>> +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 <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
>> +
>> BROADCOM BCM47XX MIPS ARCHITECTURE
>> M: Hauke Mehrtens <[email protected]>
>> M: Rafał Miłecki <[email protected]>
>