[v2,2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

Commit Message

Prabhakar Sept. 5, 2022, 11:04 p.m. UTC

Document the CRU block found on Renesas RZ/G2L (and alike) SoCs.

Signed-off-by: Lad Prabhakar <[e-mail]>
Reviewed-by: Laurent Pinchart <[e-mail]>
---
v1 -> v2
* Dropped media prefix from subject
* Dropped oneOf from compatible
* Used 4 spaces for indentation in example node
* Marked port0/1 as required
* Updated example node
* Included RB tag from Laurent

RFC v2 -> v1
* Dropped endpoint stuff from port1 as suggested by Rob
* Updated description for endpoint

RFC v1 -> RFC v2
* Dropped CSI
---
 .../bindings/media/renesas,rzg2l-cru.yaml     | 157 ++++++++++++++++++
 1 file changed, 157 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/renesas,rzg2l-cru.yaml

Comments

Krzysztof Kozlowski Sept. 8, 2022, 11:40 a.m. UTC | #1

On 06/09/2022 01:04, Lad Prabhakar wrote:
> Document the CRU block found on Renesas RZ/G2L (and alike) SoCs.
> 
> Signed-off-by: Lad Prabhakar <[e-mail]>
> Reviewed-by: Laurent Pinchart <[e-mail]>

Thank you for your patch. There is something to discuss/improve.

> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - renesas,r9a07g044-cru       # RZ/G2{L,LC}
> +          - renesas,r9a07g054-cru       # RZ/V2L
> +      - const: renesas,rzg2l-cru
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 3
> +
> +  interrupt-names:
> +    items:
> +      - const: image_conv
> +      - const: image_conv_err
> +      - const: axi_mst_err
> +
> +  clocks:
> +    items:
> +      - description: CRU Main clock
> +      - description: CPU Register access clock
> +      - description: CRU image transfer clock
> +
> +  clock-names:
> +    items:
> +      - const: vclk
> +      - const: pclk
> +      - const: aclk

Drop the "clk" suffixes. Remaining names could be made a bit more readable.

> +
Best regards,
Krzysztof

Laurent Pinchart Sept. 21, 2022, 12:43 p.m. UTC | #2

On Thu, Sep 08, 2022 at 01:40:39PM +0200, Krzysztof Kozlowski wrote:
> On 06/09/2022 01:04, Lad Prabhakar wrote:
> > Document the CRU block found on Renesas RZ/G2L (and alike) SoCs.
> > 
> > Signed-off-by: Lad Prabhakar <[e-mail]>
> > Reviewed-by: Laurent Pinchart <[e-mail]>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - renesas,r9a07g044-cru       # RZ/G2{L,LC}
> > +          - renesas,r9a07g054-cru       # RZ/V2L
> > +      - const: renesas,rzg2l-cru
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 3
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: image_conv
> > +      - const: image_conv_err
> > +      - const: axi_mst_err
> > +
> > +  clocks:
> > +    items:
> > +      - description: CRU Main clock
> > +      - description: CPU Register access clock
> > +      - description: CRU image transfer clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: vclk
> > +      - const: pclk
> > +      - const: aclk
> 
> Drop the "clk" suffixes. Remaining names could be made a bit more readable.

These names come from the documentation, isn't it better to match the
datasheet ?

> > +

Krzysztof Kozlowski Sept. 21, 2022, 3:51 p.m. UTC | #3

On 21/09/2022 14:43, Laurent Pinchart wrote:
> On Thu, Sep 08, 2022 at 01:40:39PM +0200, Krzysztof Kozlowski wrote:
>> On 06/09/2022 01:04, Lad Prabhakar wrote:
>>> Document the CRU block found on Renesas RZ/G2L (and alike) SoCs.
>>>
>>> Signed-off-by: Lad Prabhakar <[e-mail]>
>>> Reviewed-by: Laurent Pinchart <[e-mail]>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - renesas,r9a07g044-cru       # RZ/G2{L,LC}
>>> +          - renesas,r9a07g054-cru       # RZ/V2L
>>> +      - const: renesas,rzg2l-cru
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 3
>>> +
>>> +  interrupt-names:
>>> +    items:
>>> +      - const: image_conv
>>> +      - const: image_conv_err
>>> +      - const: axi_mst_err
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description: CRU Main clock
>>> +      - description: CPU Register access clock
>>> +      - description: CRU image transfer clock
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: vclk
>>> +      - const: pclk
>>> +      - const: aclk
>>
>> Drop the "clk" suffixes. Remaining names could be made a bit more readable.
> 
> These names come from the documentation, isn't it better to match the
> datasheet ?

If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the
reason to use it. :)

The "clk" is redundant even if the hardware engineer thought different.

The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma").

Best regards,
Krzysztof

Laurent Pinchart Sept. 21, 2022, 5:29 p.m. UTC | #4

On Wed, Sep 21, 2022 at 05:51:29PM +0200, Krzysztof Kozlowski wrote:
> On 21/09/2022 14:43, Laurent Pinchart wrote:
> > On Thu, Sep 08, 2022 at 01:40:39PM +0200, Krzysztof Kozlowski wrote:
> >> On 06/09/2022 01:04, Lad Prabhakar wrote:
> >>> Document the CRU block found on Renesas RZ/G2L (and alike) SoCs.
> >>>
> >>> Signed-off-by: Lad Prabhakar <[e-mail]>
> >>> Reviewed-by: Laurent Pinchart <[e-mail]>
> >>
> >> Thank you for your patch. There is something to discuss/improve.
> >>
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - enum:
> >>> +          - renesas,r9a07g044-cru       # RZ/G2{L,LC}
> >>> +          - renesas,r9a07g054-cru       # RZ/V2L
> >>> +      - const: renesas,rzg2l-cru
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 3
> >>> +
> >>> +  interrupt-names:
> >>> +    items:
> >>> +      - const: image_conv
> >>> +      - const: image_conv_err
> >>> +      - const: axi_mst_err
> >>> +
> >>> +  clocks:
> >>> +    items:
> >>> +      - description: CRU Main clock
> >>> +      - description: CPU Register access clock
> >>> +      - description: CRU image transfer clock
> >>> +
> >>> +  clock-names:
> >>> +    items:
> >>> +      - const: vclk
> >>> +      - const: pclk
> >>> +      - const: aclk
> >>
> >> Drop the "clk" suffixes. Remaining names could be made a bit more readable.
> > 
> > These names come from the documentation, isn't it better to match the
> > datasheet ?
> 
> If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the
> reason to use it. :)
> 
> The "clk" is redundant even if the hardware engineer thought different.
> 
> The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma").

I'd argue that naming clocks "v", "p" and "a" would be less readable and
more confusing. Is this a new rule ?

Krzysztof Kozlowski Sept. 21, 2022, 6:58 p.m. UTC | #5

On 21/09/2022 19:29, Laurent Pinchart wrote:
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: vclk
>>>>> +      - const: pclk
>>>>> +      - const: aclk
>>>>
>>>> Drop the "clk" suffixes. Remaining names could be made a bit more readable.
>>>
>>> These names come from the documentation, isn't it better to match the
>>> datasheet ?
>>
>> If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the
>> reason to use it. :)
>>
>> The "clk" is redundant even if the hardware engineer thought different.
>>
>> The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma").
> 
> I'd argue that naming clocks "v", "p" and "a" would be less readable and
> more confusing. Is this a new rule ?

Not really, but also it's only a style issue.

Indeed "v" and "p" are not much better... but still "vclk" does not
bring any additional information over "v". It's redundant.

You can also drop entire entry - unless it helps in particular
implementation.

https://lore.kernel.org/all/[e-mail]/
https://lore.kernel.org/all/[e-mail]/
https://lore.kernel.org/all/[e-mail]/
https://lore.kernel.org/all/[e-mail]/

Best regards,
Krzysztof

Laurent Pinchart Sept. 22, 2022, 1:46 p.m. UTC | #6

On Wed, Sep 21, 2022 at 08:58:26PM +0200, Krzysztof Kozlowski wrote:
> On 21/09/2022 19:29, Laurent Pinchart wrote:
> >>>>> +  clock-names:
> >>>>> +    items:
> >>>>> +      - const: vclk
> >>>>> +      - const: pclk
> >>>>> +      - const: aclk
> >>>>
> >>>> Drop the "clk" suffixes. Remaining names could be made a bit more readable.
> >>>
> >>> These names come from the documentation, isn't it better to match the
> >>> datasheet ?
> >>
> >> If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the
> >> reason to use it. :)
> >>
> >> The "clk" is redundant even if the hardware engineer thought different.
> >>
> >> The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma").
> > 
> > I'd argue that naming clocks "v", "p" and "a" would be less readable and
> > more confusing. Is this a new rule ?
> 
> Not really, but also it's only a style issue.
> 
> Indeed "v" and "p" are not much better... but still "vclk" does not
> bring any additional information over "v". It's redundant.
> 
> You can also drop entire entry - unless it helps in particular
> implementation.

There are multiple clocks, so I think names are better than indices. If
you really insist we could name them "v", "p" and "a", but I think the
clk suffix here improves readability. If those clocks were named
"videoclk", "pixelclk" and "axiclk" I'd be fine dropping the suffix, but
that's not the case here.

> https://lore.kernel.org/all/[e-mail]/
> https://lore.kernel.org/all/[e-mail]/
> https://lore.kernel.org/all/[e-mail]/
> https://lore.kernel.org/all/[e-mail]/

Prabhakar Sept. 30, 2022, 10:49 a.m. UTC | #7

Hi Laurent and Krzysztof,

On Thu, Sep 22, 2022 at 2:46 PM Laurent Pinchart
<[e-mail]> wrote:
>
> On Wed, Sep 21, 2022 at 08:58:26PM +0200, Krzysztof Kozlowski wrote:
> > On 21/09/2022 19:29, Laurent Pinchart wrote:
> > >>>>> +  clock-names:
> > >>>>> +    items:
> > >>>>> +      - const: vclk
> > >>>>> +      - const: pclk
> > >>>>> +      - const: aclk
> > >>>>
> > >>>> Drop the "clk" suffixes. Remaining names could be made a bit more readable.
> > >>>
> > >>> These names come from the documentation, isn't it better to match the
> > >>> datasheet ?
> > >>
> > >> If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the
> > >> reason to use it. :)
> > >>
> > >> The "clk" is redundant even if the hardware engineer thought different.
> > >>
> > >> The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma").
> > >
> > > I'd argue that naming clocks "v", "p" and "a" would be less readable and
> > > more confusing. Is this a new rule ?
> >
> > Not really, but also it's only a style issue.
> >
> > Indeed "v" and "p" are not much better... but still "vclk" does not
> > bring any additional information over "v". It's redundant.
> >
> > You can also drop entire entry - unless it helps in particular
> > implementation.
>
> There are multiple clocks, so I think names are better than indices. If
> you really insist we could name them "v", "p" and "a", but I think the
> clk suffix here improves readability. If those clocks were named
> "videoclk", "pixelclk" and "axiclk" I'd be fine dropping the suffix, but
> that's not the case here.
>
I have got the below details from the HW  team:

CRU_SYSCLK -> System clock for CSI-2 DPHY
CRU_VCLK -> video clock
CRU_PCLK -> APB clock
CRU_ACLK -> AXI clock

So I'll rename the clocks to below respectively:

+  clock-names:
+    items:
+      - const: system
+      - const: video
+      - const: apb
+      - const: axi

Does the above sound good?

Cheers,
Prabhakar

Krzysztof Kozlowski Sept. 30, 2022, 12:07 p.m. UTC | #8

On 30/09/2022 12:49, Lad, Prabhakar wrote:
> I have got the below details from the HW  team:
> 
> CRU_SYSCLK -> System clock for CSI-2 DPHY
> CRU_VCLK -> video clock
> CRU_PCLK -> APB clock
> CRU_ACLK -> AXI clock
> 
> So I'll rename the clocks to below respectively:
> 
> +  clock-names:
> +    items:
> +      - const: system
> +      - const: video
> +      - const: apb
> +      - const: axi
> 
> Does the above sound good?

For me sounds awesome! Thank you.

Best regards,
Krzysztof

Laurent Pinchart Sept. 30, 2022, 9:05 p.m. UTC | #9

Hi Prabhakar,

On Fri, Sep 30, 2022 at 11:49:22AM +0100, Lad, Prabhakar wrote:
> On Thu, Sep 22, 2022 at 2:46 PM Laurent Pinchart wrote:
> > On Wed, Sep 21, 2022 at 08:58:26PM +0200, Krzysztof Kozlowski wrote:
> > > On 21/09/2022 19:29, Laurent Pinchart wrote:
> > > >>>>> +  clock-names:
> > > >>>>> +    items:
> > > >>>>> +      - const: vclk
> > > >>>>> +      - const: pclk
> > > >>>>> +      - const: aclk
> > > >>>>
> > > >>>> Drop the "clk" suffixes. Remaining names could be made a bit more readable.
> > > >>>
> > > >>> These names come from the documentation, isn't it better to match the
> > > >>> datasheet ?
> > > >>
> > > >> If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the
> > > >> reason to use it. :)
> > > >>
> > > >> The "clk" is redundant even if the hardware engineer thought different.
> > > >>
> > > >> The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma").
> > > >
> > > > I'd argue that naming clocks "v", "p" and "a" would be less readable and
> > > > more confusing. Is this a new rule ?
> > >
> > > Not really, but also it's only a style issue.
> > >
> > > Indeed "v" and "p" are not much better... but still "vclk" does not
> > > bring any additional information over "v". It's redundant.
> > >
> > > You can also drop entire entry - unless it helps in particular
> > > implementation.
> >
> > There are multiple clocks, so I think names are better than indices. If
> > you really insist we could name them "v", "p" and "a", but I think the
> > clk suffix here improves readability. If those clocks were named
> > "videoclk", "pixelclk" and "axiclk" I'd be fine dropping the suffix, but
> > that's not the case here.
> 
> I have got the below details from the HW  team:
> 
> CRU_SYSCLK -> System clock for CSI-2 DPHY
> CRU_VCLK -> video clock
> CRU_PCLK -> APB clock
> CRU_ACLK -> AXI clock
> 
> So I'll rename the clocks to below respectively:
> 
> +  clock-names:
> +    items:
> +      - const: system
> +      - const: video
> +      - const: apb
> +      - const: axi
> 
> Does the above sound good?

That's great, thank you !

Patch

diff --git a/Documentation/devicetree/bindings/media/renesas,rzg2l-cru.yaml b/Documentation/devicetree/bindings/media/renesas,rzg2l-cru.yaml
new file mode 100644
index 000000000000..df18aeacfce3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/renesas,rzg2l-cru.yaml
@@ -0,0 +1,157 @@ 
+# 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-cru.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/G2L (and alike SoC's) Camera Data Receiving Unit (CRU) Image processing
+
+maintainers:
+  - Lad Prabhakar <[e-mail]>
+
+description:
+  The CRU image processing module is a data conversion module equipped with pixel
+  color space conversion, LUT, pixel format conversion, etc. An MIPI CSI-2 input and
+  parallel (including ITU-R BT.656) input are provided as the image sensor interface.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,r9a07g044-cru       # RZ/G2{L,LC}
+          - renesas,r9a07g054-cru       # RZ/V2L
+      - const: renesas,rzg2l-cru
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 3
+
+  interrupt-names:
+    items:
+      - const: image_conv
+      - const: image_conv_err
+      - const: axi_mst_err
+
+  clocks:
+    items:
+      - description: CRU Main clock
+      - description: CPU Register access clock
+      - description: CRU image transfer clock
+
+  clock-names:
+    items:
+      - const: vclk
+      - const: pclk
+      - const: aclk
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    items:
+      - description: CRU_PRESETN reset terminal
+      - description: CRU_ARESETN reset terminal
+
+  reset-names:
+    items:
+      - const: presetn
+      - const: aresetn
+
+  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 a parallel input source.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              hsync-active: true
+              vsync-active: true
+              bus-width: true
+              data-shift: true
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Input port node, describing the Image Processing module connected to the
+          CSI-2 receiver.
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - power-domains
+
+additionalProperties: false
+
+examples:
+  # Device node example with CSI-2
+  - |
+    #include <dt-bindings/clock/r9a07g044-cpg.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    cru: video@10830000 {
+        compatible = "renesas,r9a07g044-cru", "renesas,rzg2l-cru";
+        reg = <0x10830000 0x400>;
+        interrupts = <GIC_SPI 167 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "image_conv", "image_conv_err", "axi_mst_err";
+        clocks = <&cpg CPG_MOD R9A07G044_CRU_VCLK>,
+                 <&cpg CPG_MOD R9A07G044_CRU_PCLK>,
+                 <&cpg CPG_MOD R9A07G044_CRU_ACLK>;
+        clock-names = "vclk", "pclk", "aclk";
+        power-domains = <&cpg>;
+        resets = <&cpg R9A07G044_CRU_PRESETN>,
+                 <&cpg R9A07G044_CRU_ARESETN>;
+        reset-names = "presetn", "aresetn";
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                reg = <0>;
+
+                cru_parallel_in: endpoint@0 {
+                    reg = <0>;
+                    remote-endpoint= <&ov5642>;
+                    hsync-active = <1>;
+                    vsync-active = <1>;
+                };
+            };
+
+            port@1 {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                reg = <1>;
+
+                cru_csi_in: endpoint@0 {
+                    reg = <0>;
+                    remote-endpoint= <&csi_cru_in>;
+                };
+            };
+        };
+    };