Re: [PATCH v3 1/2] dt-bindings: clk: Introduce 'critical-clocks' property

[Marek Vasut <marex@denx.de>]

On 6/16/22 00:22, Stephen Boyd wrote:
> Quoting Marek Vasut (2022-06-15 14:55:17)
>> On 6/15/22 22:10, Stephen Boyd wrote:
>>> Quoting Marek Vasut (2022-05-17 16:59:18)
>>>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> index f2ea53832ac63..d7f7afe2cbd0c 100644
>>>> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> @@ -169,6 +169,22 @@ a shared clock is forbidden.
>>>>    Configuration of common clocks, which affect multiple consumer devices can
>>>>    be similarly specified in the clock provider node.
>>>>    
>>>> +==Critical clocks==
>>>> +
>>>> +Some platforms require some clocks to be always running, e.g. because those
>>>> +clock supply devices which are not otherwise attached to the system. One
>>>> +example is a system where the SoC serves as a crystal oscillator replacement
>>>> +for a programmable logic device. The critical-clocks property of a clock
>>>> +controller allows listing clock which must never be turned off.
>>>> +
>>>> +   clock-controller@a000f000 {
>>>> +        compatible = "vendor,clk95;
>>>> +        reg = <0xa000f000 0x1000>
>>>> +        #clocks-cells = <1>;
>>>> +        ...
>>>> +        critical-clocks = <UART3_CLK>, <SPI5_CLK>;
>>>
>>> Historically "critical" is overloaded in the clk framework. We should
>>> avoid using that name. What does "critical" even mean?
>>
>> It means those clock must not be turned off, but there is no consumer
>> described in DT.
> 
> So it means "always on".
> 
>>
>>> Instead I'd prefer "always-on-clocks" here, so we can indicate that
>>> these clks should always be on. It would also parallel the property in
>>> the regulator framework.
>>
>> This property name is derived from protected-clock which you introduced.
>> I think it would be better to stay consistent within the clock framework
>> property names ?
> 
> protected-clocks is based on assigned-clocks. There isn't a
> CLK_IS_PROTECTED flag. I'm not following your argument at all here,
> sorry.

critical-clock property name is based on protected-clock property name.

There is also no CLK_IS_ALWAYS_ON flag , but there is CLK_IS_CRITICAL 
flag . Sure, there is no CLK_IS_PROTECTED flag because the protected 
clock is implemented only by a single driver (qualcomm).

I think it makes sense to align the DT property name and the flag name, 
and the critical-clock is aligned with both other DT property names in 
the clock framework and the flag name.