Message ID | 20240610233221.242749-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | Add CPG support for RZ/V2H(P) SoC | expand |
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
Hi Krzysztof, Thank you for the review. On Tue, Jun 11, 2024 at 8:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > 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. > OK. > > + 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? > It will be used for blocks using core clocks. > > + - 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. > Are you suggesting not to differentiate between core/mod clocks. They are differentiated because the MOD clocks can turned ON/OFF but where as with the core clocks we cannot turn them ON/OF so the driver needs to know this, hence two specifiers are used. > > + 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. > OK. > > + 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. > OK. > > + compatible = "renesas,r9a09g057-cpg"; > > Use 4 spaces for example indentation. > Sure, I will update it. Cheers, Prabhakar
Hi Geert, On Tue, Jun 11, 2024 at 12:32 AM Prabhakar <prabhakar.csengg@gmail.com> 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> > --- > Hi Geert, > > WRT XIN_{RTCCLK/AUDCLK/MAINCLK)clks going to CPG I have created an > internal request for clarification if the clocks are inputs to CPG > or to respective clocks. As the board schematic doesnt have any of > these. For now I have just kept qextal clk as input to CPG. > I have got the feedback from the manual team. The XIN_* clocks will be renamed as below (and the block diagram will be updated), XIN_MAINCLK -> QXCLK XIN_RTCCLK -> RTX_XCLK XIN_AUDCLK -> AUDIO_XCLK From section 4.2.1.7 Functional Block diagram (page 359) we have the below, RTXIN ----------------> PFC ------> RTX_XCLK --------> CPG QEXTAL--------------> PFC ------> QXCLK -------------> CPG AUDIO_EXTAL-----> PFC ------> AUDIO_XCLK ----> CPG How should we model this now, please provide your feedback? Cheers, Prabhakar
On 13/06/2024 11:53, Lad, Prabhakar wrote: > Hi Krzysztof, > > Thank you for the review. > > On Tue, Jun 11, 2024 at 8:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> 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. >> > OK. > >>> + 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? >> > It will be used for blocks using core clocks. > >>> + - 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. >> > Are you suggesting not to differentiate between core/mod clocks. They > are differentiated because the MOD clocks can turned ON/OFF but where > as with the core clocks we cannot turn them ON/OF so the driver needs > to know this, hence two specifiers are used. Every driver knows it... I am really, what is the problem here? Are you saying the drivers create some unknown clocks? Anyway, that's not an argument for bindings. Fix your driver design. Best regards, Krzysztof
Hi Krzysztof, On Thu, Jun 13, 2024 at 2:57 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 13/06/2024 11:53, Lad, Prabhakar wrote: > > On Tue, Jun 11, 2024 at 8:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > >> 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> > >>> + '#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? > >> > > It will be used for blocks using core clocks. > > > >>> + - 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. > >> > > Are you suggesting not to differentiate between core/mod clocks. They > > are differentiated because the MOD clocks can turned ON/OFF but where > > as with the core clocks we cannot turn them ON/OF so the driver needs > > to know this, hence two specifiers are used. > > Every driver knows it... I am really, what is the problem here? Are you > saying the drivers create some unknown clocks? The driver knows for sure which clocks are module clocks, and thus can be used for power management. To simplify the driver, two separate numbers spaces are used: 1. Core clock numbers come from IDs in the DT binding headers, 2. Module clock numbers come straight[1] from the hardware docs. As the latter are fixed, merging them into a single number space in a future-proof way is hard[2], the bindings use 2 clock cells. Alternatively, a unified number space using IDs in the DT binding headers could be used, as you suggest. [1] "straight" may be a misnomer here, as the DT writer still has to calculate the number from register index and bit index: n = register index * 16 + bit index i.e. register index 1 and register bit 3 become 19. In the R-Car series, this is handled slightly more elegant (IMHO ;-), and easier to the human eye, by using a sparse number space: n = register index * 100 + bit index i.e. register index 1 and register bit 3 become 103. Which also matches how the bits were named in older SH-Mobile hardware docs. [2] One could use an offset to indicate core or module clocks, but future SoCs in the family may have more clocks. Gr{oetje,eeting}s, Geert
On 26/06/2024 11:35, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Thu, Jun 13, 2024 at 2:57 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On 13/06/2024 11:53, Lad, Prabhakar wrote: >>> On Tue, Jun 11, 2024 at 8:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: >>>> 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> > >>>>> + '#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? >>>> >>> It will be used for blocks using core clocks. >>> >>>>> + - 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. >>>> >>> Are you suggesting not to differentiate between core/mod clocks. They >>> are differentiated because the MOD clocks can turned ON/OFF but where >>> as with the core clocks we cannot turn them ON/OF so the driver needs >>> to know this, hence two specifiers are used. >> >> Every driver knows it... I am really, what is the problem here? Are you >> saying the drivers create some unknown clocks? > > The driver knows for sure which clocks are module clocks, and thus can > be used for power management. To simplify the driver, two separate > numbers spaces are used: > 1. Core clock numbers come from IDs in the DT binding headers, > 2. Module clock numbers come straight[1] from the hardware docs. > As the latter are fixed, merging them into a single number space in > a future-proof way is hard[2], the bindings use 2 clock cells. IIUC, your module clock numbers are not DT ABI and should not be put into the binding headers. I think that's the case currently, right? If above is correct, considering your explanation I am fine. Thanks for the time to make it clear. > > Alternatively, a unified number space using IDs in the DT binding > headers could be used, as you suggest. > > [1] "straight" may be a misnomer here, as the DT writer still has to > calculate the number from register index and bit index: > > n = register index * 16 + bit index > > i.e. register index 1 and register bit 3 become 19. > > In the R-Car series, this is handled slightly more elegant > (IMHO ;-), and easier to the human eye, by using a sparse > number space: > > n = register index * 100 + bit index > > i.e. register index 1 and register bit 3 become 103. > Which also matches how the bits were named in older SH-Mobile > hardware docs. > > [2] One could use an offset to indicate core or module clocks, but > future SoCs in the family may have more clocks. > Best regards, Krzysztof
Hi Krzysztof, On Wed, Jun 26, 2024 at 11:41 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 26/06/2024 11:35, Geert Uytterhoeven wrote: > > On Thu, Jun 13, 2024 at 2:57 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > >> On 13/06/2024 11:53, Lad, Prabhakar wrote: > >>> On Tue, Jun 11, 2024 at 8:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > >>>> 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> > > > >>>>> + '#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? > >>>> > >>> It will be used for blocks using core clocks. > >>> > >>>>> + - 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. > >>>> > >>> Are you suggesting not to differentiate between core/mod clocks. They > >>> are differentiated because the MOD clocks can turned ON/OFF but where > >>> as with the core clocks we cannot turn them ON/OF so the driver needs > >>> to know this, hence two specifiers are used. > >> > >> Every driver knows it... I am really, what is the problem here? Are you > >> saying the drivers create some unknown clocks? > > > > The driver knows for sure which clocks are module clocks, and thus can > > be used for power management. To simplify the driver, two separate > > numbers spaces are used: > > 1. Core clock numbers come from IDs in the DT binding headers, > > 2. Module clock numbers come straight[1] from the hardware docs. > > As the latter are fixed, merging them into a single number space in > > a future-proof way is hard[2], the bindings use 2 clock cells. > > IIUC, your module clock numbers are not DT ABI and should not be put > into the binding headers. I think that's the case currently, right? Exactly: they are hardware ABI, just like e.g. GIC interrupt numbers. > If above is correct, considering your explanation I am fine. Thanks for > the time to make it clear. Thanks! Gr{oetje,eeting}s, Geert
diff --git a/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml b/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml new file mode 100644 index 000000000000..03085308154c --- /dev/null +++ b/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml @@ -0,0 +1,81 @@ +# 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@bp.renesas.com> + +description: | + 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>, + - 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. + 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. + 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 { + compatible = "renesas,r9a09g057-cpg"; + reg = <0x10420000 0x10000>; + clocks = <&extal_clk>; + clock-names = "qextal"; + #clock-cells = <2>; + #power-domain-cells = <0>; + #reset-cells = <1>; + };