[RFC,v2,1/4] media: dt-bindings: media: Document RZ/G2L CSI-2 block

Commit Message

Lad Prabhakar Jan. 21, 2022, 1:05 a.m. UTC

Document the CSI-2 block which is part of CRU found in Renesas
RZ/G2L SoC.

Signed-off-by: Lad Prabhakar <[e-mail]>
---
Hi Geert/All,

vclk and pclk clocks are shared with CRU both CSI and CRU driver are using
pm_runtime. pclk clock is necessary for register access where as vclk clock
is only used for calculations. So would you suggest passing vclk as part of
clocks (as currently implemented) or pass the vclk clock rate as a dt property.

Cheers,
Prabhakar

v1->v2
* New patch
---
 .../bindings/media/renesas,rzg2l-csi2.yaml    | 151 ++++++++++++++++++
 1 file changed, 151 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml

Comments

Geert Uytterhoeven Jan. 21, 2022, 9:26 a.m. UTC | #1

Hi Prabhakar,

On Fri, Jan 21, 2022 at 2:06 AM Lad Prabhakar
<[e-mail]> wrote:
> Document the CSI-2 block which is part of CRU found in Renesas
> RZ/G2L SoC.
>
> Signed-off-by: Lad Prabhakar <[e-mail]>

Thanks for your patch!

> ---
> Hi Geert/All,
>
> vclk and pclk clocks are shared with CRU both CSI and CRU driver are using
> pm_runtime. pclk clock is necessary for register access where as vclk clock
> is only used for calculations. So would you suggest passing vclk as part of

What do you mean by "calculations"?
The bindings say this is the main clock?

> clocks (as currently implemented) or pass the vclk clock rate as a dt property.

Please do not specify clock rates in DT, but always pass clock
specifiers instead.
The clock subsystem handles sharing of clocks just fine.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Prabhakar Jan. 21, 2022, 11:52 a.m. UTC | #2

Hi Geert,

On Fri, Jan 21, 2022 at 9:26 AM Geert Uytterhoeven <[e-mail]> wrote:
>
> Hi Prabhakar,
>
> On Fri, Jan 21, 2022 at 2:06 AM Lad Prabhakar
> <[e-mail]> wrote:
> > Document the CSI-2 block which is part of CRU found in Renesas
> > RZ/G2L SoC.
> >
> > Signed-off-by: Lad Prabhakar <[e-mail]>
>
> Thanks for your patch!
>
> > ---
> > Hi Geert/All,
> >
> > vclk and pclk clocks are shared with CRU both CSI and CRU driver are using
> > pm_runtime. pclk clock is necessary for register access where as vclk clock
> > is only used for calculations. So would you suggest passing vclk as part of
>
> What do you mean by "calculations"?
To set the CSI2nMCT2 register bits (FRRSKW/FRRCLK), vclk clock rate is used.

> The bindings say this is the main clock?
>
That is because the RZG2L_clock_list_r02_02.xlsx mentions it as the main clock.

> > clocks (as currently implemented) or pass the vclk clock rate as a dt property.
>
> Please do not specify clock rates in DT, but always pass clock
> specifiers instead.
> The clock subsystem handles sharing of clocks just fine.
>
Agreed.

Cheers,
Prabhakar

Geert Uytterhoeven Jan. 21, 2022, 12:06 p.m. UTC | #3

Hi Prabhakar,

On Fri, Jan 21, 2022 at 12:52 PM Lad, Prabhakar
<[e-mail]> wrote:
> On Fri, Jan 21, 2022 at 9:26 AM Geert Uytterhoeven <[e-mail]> wrote:
> > On Fri, Jan 21, 2022 at 2:06 AM Lad Prabhakar
> > <[e-mail]> wrote:
> > > Document the CSI-2 block which is part of CRU found in Renesas
> > > RZ/G2L SoC.
> > >
> > > Signed-off-by: Lad Prabhakar <[e-mail]>
> >
> > Thanks for your patch!
> >
> > > ---
> > > Hi Geert/All,
> > >
> > > vclk and pclk clocks are shared with CRU both CSI and CRU driver are using
> > > pm_runtime. pclk clock is necessary for register access where as vclk clock
> > > is only used for calculations. So would you suggest passing vclk as part of
> >
> > What do you mean by "calculations"?
> To set the CSI2nMCT2 register bits (FRRSKW/FRRCLK), vclk clock rate is used.

Ah, clock rate calculations.  I (mis)understood that vclk clocked
a hardware calculation block, and was wondering what kind of heavy
calculations were involved ;-)

> > The bindings say this is the main clock?
> >
> That is because the RZG2L_clock_list_r02_02.xlsx mentions it as the main clock.
>
> > > clocks (as currently implemented) or pass the vclk clock rate as a dt property.
> >
> > Please do not specify clock rates in DT, but always pass clock
> > specifiers instead.
> > The clock subsystem handles sharing of clocks just fine.
> >
> Agreed.

So doing clk_get_rate() is fine.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Jacopo Mondi Feb. 15, 2022, 1:01 p.m. UTC | #4

Hi Prabhakar

On Fri, Jan 21, 2022 at 01:05:40AM +0000, Lad Prabhakar wrote:
> Document the CSI-2 block which is part of CRU found in Renesas
> RZ/G2L SoC.
>
> Signed-off-by: Lad Prabhakar <[e-mail]>
> ---
> Hi Geert/All,
>
> vclk and pclk clocks are shared with CRU both CSI and CRU driver are using
> pm_runtime. pclk clock is necessary for register access where as vclk clock
> is only used for calculations. So would you suggest passing vclk as part of
> clocks (as currently implemented) or pass the vclk clock rate as a dt property.
>
> Cheers,
> Prabhakar
>
> v1->v2
> * New patch
> ---
>  .../bindings/media/renesas,rzg2l-csi2.yaml    | 151 ++++++++++++++++++
>  1 file changed, 151 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml b/Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml
> new file mode 100644
> index 000000000000..bf907768a157
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml
> @@ -0,0 +1,151 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (C) 2022 Renesas Electronics Corp.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/renesas,rzg2l-csi2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/G2L MIPI CSI-2 receiver
> +
> +maintainers:
> +  - Lad Prabhakar <[e-mail]>
> +
> +description:
> +  The RZ/G2L CSI-2 receiver device provides MIPI CSI-2 capabilities for the
> +  Renesas RZ/G2L family of devices. MIPI CSI-2 is part of the CRU block which
> +  is used in conjunction with the Image Processing module, which provides the
> +  video capture capabilities.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - renesas,r9a07g044-csi2     # RZ/G2{L,LC}
> +          - const: renesas,rzg2l-csi2

As per Rob's comment on the CRU bindings, you can remove oneOf:

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    items:
> +      - const: csi2_link

Can this be just

  interrupt-names:
    const: csi2_link
?

(I've run dt_binding_check and it does not complain)

> +
> +  clocks:
> +    items:
> +      - description: Internal clock for connecting CRU and MIPI
> +      - description: CRU Main clock
> +      - description: CPU Register access clock
> +
> +  clock-names:
> +    items:
> +      - const: sysclk
> +      - const: vclk
> +      - const: pclk
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    items:
> +      - description: CRU_CMN_RSTB reset terminal
> +
> +  reset-names:
> +    items:
> +      - const: cmn-rstb

Here and above, is items: needed for a single entry ?
(again, dt_binding_check does not complain if I remove it)

> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port node, single endpoint describing the CSI-2 transmitter.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +                items:
> +                  maximum: 4
> +
> +            required:
> +              - clock-lanes
> +              - data-lanes
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          Output port node, Image Processing block connected to the CSI-2 receiver.

Isn't the next processing block the CRU ? IOW, isn't this driver the
CSI-2 receiver ?

> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - resets
> +  - reset-names
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/r9a07g044-cpg.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    csi20: csi2@10830400 {
> +            compatible = "renesas,r9a07g044-csi2", "renesas,rzg2l-csi2";
> +            reg = <0x10830400 0xfc00>;
> +            interrupts = <GIC_SPI 166 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&cpg CPG_MOD R9A07G044_CRU_SYSCLK>,
> +                     <&cpg CPG_MOD R9A07G044_CRU_VCLK>,
> +                     <&cpg CPG_MOD R9A07G044_CRU_PCLK>;
> +            clock-names = "sysclk", "vclk", "pclk";
> +            power-domains = <&cpg>;
> +            resets = <&cpg R9A07G044_CRU_CMN_RSTB>;
> +            reset-names = "cmn-rstb";
> +
> +            ports {
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +
> +                    port@0 {
> +                            reg = <0>;
> +
> +                            csi2_in: endpoint {
> +                                    clock-lanes = <0>;
> +                                    data-lanes = <1 2>;
> +                                    remote-endpoint = <&ov5645_ep>;
> +                            };
> +                    };
> +
> +                    port@1 {
> +                            #address-cells = <1>;
> +                            #size-cells = <0>;
> +
> +                            reg = <1>;
> +
> +                            csi2cru: endpoint@0 {
> +                                    reg = <0>;
> +                                    remote-endpoint = <&crucsi2>;
> +                            };
> +                    };
> +            };
> +    };
> --
> 2.17.1
>

Prabhakar March 18, 2022, 4:58 p.m. UTC | #5

Hi Jacopo,

Thank you for the review.

On Tue, Feb 15, 2022 at 1:00 PM Jacopo Mondi <[e-mail]> wrote:
>
> Hi Prabhakar
>
> On Fri, Jan 21, 2022 at 01:05:40AM +0000, Lad Prabhakar wrote:
> > Document the CSI-2 block which is part of CRU found in Renesas
> > RZ/G2L SoC.
> >
> > Signed-off-by: Lad Prabhakar <[e-mail]>
> > ---
> > Hi Geert/All,
> >
> > vclk and pclk clocks are shared with CRU both CSI and CRU driver are using
> > pm_runtime. pclk clock is necessary for register access where as vclk clock
> > is only used for calculations. So would you suggest passing vclk as part of
> > clocks (as currently implemented) or pass the vclk clock rate as a dt property.
> >
> > Cheers,
> > Prabhakar
> >
> > v1->v2
> > * New patch
> > ---
> >  .../bindings/media/renesas,rzg2l-csi2.yaml    | 151 ++++++++++++++++++
> >  1 file changed, 151 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml b/Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml
> > new file mode 100644
> > index 000000000000..bf907768a157
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml
> > @@ -0,0 +1,151 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (C) 2022 Renesas Electronics Corp.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/renesas,rzg2l-csi2.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas RZ/G2L MIPI CSI-2 receiver
> > +
> > +maintainers:
> > +  - Lad Prabhakar <[e-mail]>
> > +
> > +description:
> > +  The RZ/G2L CSI-2 receiver device provides MIPI CSI-2 capabilities for the
> > +  Renesas RZ/G2L family of devices. MIPI CSI-2 is part of the CRU block which
> > +  is used in conjunction with the Image Processing module, which provides the
> > +  video capture capabilities.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - renesas,r9a07g044-csi2     # RZ/G2{L,LC}
> > +          - const: renesas,rzg2l-csi2
>
> As per Rob's comment on the CRU bindings, you can remove oneOf:
>
I would like to keep it, as there will be RZ/V2L and RZ/G2UL entries
following as soon once as this gets merged in. So just want to avoid
the change hunk later.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: csi2_link
>
> Can this be just
>
>   interrupt-names:
>     const: csi2_link
> ?
>
Agreed.

> (I've run dt_binding_check and it does not complain)
>
> > +
> > +  clocks:
> > +    items:
> > +      - description: Internal clock for connecting CRU and MIPI
> > +      - description: CRU Main clock
> > +      - description: CPU Register access clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: sysclk
> > +      - const: vclk
> > +      - const: pclk
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    items:
> > +      - description: CRU_CMN_RSTB reset terminal
> > +
> > +  reset-names:
> > +    items:
> > +      - const: cmn-rstb
>
> Here and above, is items: needed for a single entry ?
> (again, dt_binding_check does not complain if I remove it)
>
Agreed.

> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > +        unevaluatedProperties: false
> > +        description:
> > +          Input port node, single endpoint describing the CSI-2 transmitter.
> > +
> > +        properties:
> > +          endpoint:
> > +            $ref: video-interfaces.yaml#
> > +            unevaluatedProperties: false
> > +
> > +            properties:
> > +              data-lanes:
> > +                minItems: 1
> > +                maxItems: 4
> > +                items:
> > +                  maximum: 4
> > +
> > +            required:
> > +              - clock-lanes
> > +              - data-lanes
> > +
> > +      port@1:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description:
> > +          Output port node, Image Processing block connected to the CSI-2 receiver.
>
> Isn't the next processing block the CRU ? IOW, isn't this driver the
> CSI-2 receiver ?
>
On RZ/G2L CRU consists of CSI + image processing block (as seen in
[0]). As requested by Laurent in previous version I split up the
driver for CSI. So instead of mentioning CRU I have mentioned it as
"Image Processing block".

[0] https://renesas.info/wiki/File:CRU.png

Cheers,
Prabhakar

> > +
> > +    required:
> > +      - port@0
> > +      - port@1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +  - power-domains
> > +  - resets
> > +  - reset-names
> > +  - ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/r9a07g044-cpg.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    csi20: csi2@10830400 {
> > +            compatible = "renesas,r9a07g044-csi2", "renesas,rzg2l-csi2";
> > +            reg = <0x10830400 0xfc00>;
> > +            interrupts = <GIC_SPI 166 IRQ_TYPE_LEVEL_HIGH>;
> > +            clocks = <&cpg CPG_MOD R9A07G044_CRU_SYSCLK>,
> > +                     <&cpg CPG_MOD R9A07G044_CRU_VCLK>,
> > +                     <&cpg CPG_MOD R9A07G044_CRU_PCLK>;
> > +            clock-names = "sysclk", "vclk", "pclk";
> > +            power-domains = <&cpg>;
> > +            resets = <&cpg R9A07G044_CRU_CMN_RSTB>;
> > +            reset-names = "cmn-rstb";
> > +
> > +            ports {
> > +                    #address-cells = <1>;
> > +                    #size-cells = <0>;
> > +
> > +                    port@0 {
> > +                            reg = <0>;
> > +
> > +                            csi2_in: endpoint {
> > +                                    clock-lanes = <0>;
> > +                                    data-lanes = <1 2>;
> > +                                    remote-endpoint = <&ov5645_ep>;
> > +                            };
> > +                    };
> > +
> > +                    port@1 {
> > +                            #address-cells = <1>;
> > +                            #size-cells = <0>;
> > +
> > +                            reg = <1>;
> > +
> > +                            csi2cru: endpoint@0 {
> > +                                    reg = <0>;
> > +                                    remote-endpoint = <&crucsi2>;
> > +                            };
> > +                    };
> > +            };
> > +    };
> > --
> > 2.17.1
> >

Patch

diff --git a/Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml b/Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml
new file mode 100644
index 000000000000..bf907768a157
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/renesas,rzg2l-csi2.yaml
@@ -0,0 +1,151 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (C) 2022 Renesas Electronics Corp.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/renesas,rzg2l-csi2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/G2L MIPI CSI-2 receiver
+
+maintainers:
+  - Lad Prabhakar <[e-mail]>
+
+description:
+  The RZ/G2L CSI-2 receiver device provides MIPI CSI-2 capabilities for the
+  Renesas RZ/G2L family of devices. MIPI CSI-2 is part of the CRU block which
+  is used in conjunction with the Image Processing module, which provides the
+  video capture capabilities.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - renesas,r9a07g044-csi2     # RZ/G2{L,LC}
+          - const: renesas,rzg2l-csi2
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    items:
+      - const: csi2_link
+
+  clocks:
+    items:
+      - description: Internal clock for connecting CRU and MIPI
+      - description: CRU Main clock
+      - description: CPU Register access clock
+
+  clock-names:
+    items:
+      - const: sysclk
+      - const: vclk
+      - const: pclk
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    items:
+      - description: CRU_CMN_RSTB reset terminal
+
+  reset-names:
+    items:
+      - const: cmn-rstb
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port node, single endpoint describing the CSI-2 transmitter.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+                items:
+                  maximum: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Output port node, Image Processing block connected to the CSI-2 receiver.
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - power-domains
+  - resets
+  - reset-names
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/r9a07g044-cpg.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    csi20: csi2@10830400 {
+            compatible = "renesas,r9a07g044-csi2", "renesas,rzg2l-csi2";
+            reg = <0x10830400 0xfc00>;
+            interrupts = <GIC_SPI 166 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&cpg CPG_MOD R9A07G044_CRU_SYSCLK>,
+                     <&cpg CPG_MOD R9A07G044_CRU_VCLK>,
+                     <&cpg CPG_MOD R9A07G044_CRU_PCLK>;
+            clock-names = "sysclk", "vclk", "pclk";
+            power-domains = <&cpg>;
+            resets = <&cpg R9A07G044_CRU_CMN_RSTB>;
+            reset-names = "cmn-rstb";
+
+            ports {
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+
+                    port@0 {
+                            reg = <0>;
+
+                            csi2_in: endpoint {
+                                    clock-lanes = <0>;
+                                    data-lanes = <1 2>;
+                                    remote-endpoint = <&ov5645_ep>;
+                            };
+                    };
+
+                    port@1 {
+                            #address-cells = <1>;
+                            #size-cells = <0>;
+
+                            reg = <1>;
+
+                            csi2cru: endpoint@0 {
+                                    reg = <0>;
+                                    remote-endpoint = <&crucsi2>;
+                            };
+                    };
+            };
+    };