Re: [PATCH 1/4] dt-bindings: clock: renesas: Document RZ/V2H(P) SoC CPG driver

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Tue, Jun 4, 2024 at 4:50 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> Please drop "driver" from the one-line summary.
>
OK, I will drop it.

> On Fri, May 24, 2024 at 10:29 AM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Document the device tree bindings of the Renesas RZ/V2H(P) SoC
>
> s/of/for/
>
OK.

> > Clock Pulse Generator (CPG).
> >
> > CPG block handles the below operations:
> > - Handles the generation and control of clock signals for the IP modules
>
> Please drop "Handles the"
>
OK.

> > - The generation and control of resets
>
> Please drop "The".
>
OK.

> > - Control over booting
> > - Low power consumption and the power supply domains
>
> Please drop "the".
>
OK.

> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
> > @@ -0,0 +1,78 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/renesas,rzv2h-cpg.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas RZ/V2H(P) Clock Pulse Generator (CPG)
> > +
> > +maintainers:
> > +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > +
> > +description: |
> > +  On Renesas RZ/V2H(P) SoC's, the CPG (Clock Pulse Generator) handles the generation
>
> SoCs
>
OK.

> > +  and control of clock signals for the IP modules, the generation and control of resets,
> > +  and control over booting, low power consumption and the power supply domains.
>
> Please drop "the".
>
OK.

> > +
> > +properties:
> > +  compatible:
> > +    const: renesas,r9a09g057-cpg
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    description:
> > +      Clock source to CPG can be either from external clock input (EXCLK) or
> > +      crystal oscillator (XIN/XOUT).
> > +    const: extal
>
> According to Figure 4.4-1 ("CPG Functional Block Diagram"), there are 3
> (RTC, audio, main).
>
Agreed, I will add the below:
- QEXTAL
- RTXIN
- AUDIO_EXTAL
- AUDIO_CLKB
- AUDIO_CLKC

> > +
> > +  '#clock-cells':
> > +    description: |
> > +      - For CPG core clocks, the two clock specifier cells must be "CPG_CORE"
> > +        and a core clock reference, as defined in
> > +        <dt-bindings/clock/r9a09g057-cpg.h>,
> > +      - For module clocks, the two clock specifier cells must be "CPG_MOD" and
> > +        a module number, as defined in <dt-bindings/clock/r9a09g057-cpg.h>.
> > +    const: 2
>
> I understand this will be changed to 1, the clock number?
>
I'll keep this '2'. I will introduce core clocks (clocks which cannot
be controlled by  CLKON_m register) for example,
- SYS_0_PCLK
- CA55_0_CORE_CLK[x]
- IOTOP_0_SHCLK.

> > +  '#power-domain-cells':
> > +    description:
> > +      SoC devices that are part of the CPG/Module Standby Mode Clock Domain and
> > +      can be power-managed through Module Standby should refer to the CPG device
> > +      node in their "power-domains" property, as documented by the generic PM
> > +      Domain bindings in Documentation/devicetree/bindings/power/power-domain.yaml.
> > +      The power domain specifiers defined in <dt-bindings/clock/r9a09g057-cpg.h> could
> > +      be used to reference individual CPG power domains.
>
> The latter suggests "const: 1".
> But the example below uses zero, as does the code?
>
This should be '0' indeed.

> > +
> > +  '#reset-cells':
> > +    description:
> > +      The single reset specifier cell must be the module number, as defined in
>
> reset number (or index).
>
OK.

Cheers,
Prabhakar