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

[Krzysztof Kozlowski <krzk@kernel.org>]

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