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

Krzysztof Kozlowski
2024-06-11T07:02:32Z

On 11/06/2024 01:32, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Document the device tree bindings for the Renesas RZ/V2H(P) SoC
> Clock Pulse Generator (CPG).
>
> CPG block handles the below operations:
> - Generation and control of clock signals for the IP modules
> - Generation and control of resets
> - Control over booting
> - Low power consumption and power supply domains
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Since this is not a finished work (RFC), only limited review follows.

> +$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@bp.renesas.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> + On Renesas RZ/V2H(P) SoCs, the CPG (Clock Pulse Generator) handles generation
> + and control of clock signals for the IP modules, generation and control of resets,
> + and control over booting, low power consumption and power supply domains.
> +
> +properties:
> + compatible:
> + const: renesas,r9a09g057-cpg
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> + clock-names:
> + description:
> + Clock source to CPG on QEXTAL pin.
> + const: qextal
> +
> + '#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>,

So second cell is not used?

> + - For module clocks, the two clock specifier cells must be "CPG_MOD" and
> + a module number. The module number is calculated as the CLKON register
> + offset index multiplied by 16, plus the actual bit in the register
> + used to turn the CLK ON. For example, for CGC_GIC_0_GICCLK, the
> + calculation is (1 * 16 + 3) = 19.

You should not have different values. Make it const: 1 and just use IDs.

> + const: 2
> +
> + '#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.

Drop description, it's redundant.

> + const: 0
> +
> + '#reset-cells':
> + description:
> + The single reset specifier cell must be the reset number. The reset number
> + is calculated as the reset register offset index multiplied by 16, plus the
> + actual bit in the register used to reset the specific IP block. For example,
> + for SYS_0_PRESETN, the calculation is (3 * 16 + 0) = 48.
> + const: 1
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - '#clock-cells'
> + - '#power-domain-cells'
> + - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + cpg: clock-controller@10420000 {

Drop unused label.

> + compatible = "renesas,r9a09g057-cpg";

Use 4 spaces for example indentation.
> + };

Best regards,
Krzysztof