2022-09-05 23:11:42

by Prabhakar Mahadev Lad

[permalink] [raw]
Subject: [PATCH v2 2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

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

Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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

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 <[email protected]>
+
+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>;
+ };
+ };
+ };
+ };
--
2.25.1


2022-09-08 11:56:09

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

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 <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>

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

2022-09-21 12:48:19

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

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 <[email protected]>
> > Reviewed-by: Laurent Pinchart <[email protected]>
>
> 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 ?

> > +

--
Regards,

Laurent Pinchart

2022-09-21 16:35:02

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

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 <[email protected]>
>>> Reviewed-by: Laurent Pinchart <[email protected]>
>>
>> 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

2022-09-21 18:00:27

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

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 <[email protected]>
> >>> Reviewed-by: Laurent Pinchart <[email protected]>
> >>
> >> 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 ?

--
Regards,

Laurent Pinchart

2022-09-21 19:18:04

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

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/[email protected]/
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/YYFCaHI%[email protected]/
https://lore.kernel.org/all/[email protected]/

Best regards,
Krzysztof

2022-09-22 14:06:23

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

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/[email protected]/
> https://lore.kernel.org/all/[email protected]/
> https://lore.kernel.org/all/YYFCaHI%[email protected]/
> https://lore.kernel.org/all/[email protected]/

--
Regards,

Laurent Pinchart

2022-09-30 11:28:56

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

Hi Laurent and Krzysztof,

On Thu, Sep 22, 2022 at 2:46 PM Laurent Pinchart
<[email protected]> 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

2022-09-30 12:49:29

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

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

2022-09-30 21:27:13

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] media: dt-bindings: Document Renesas RZ/G2L CRU block

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 !

--
Regards,

Laurent Pinchart