Re: [PATCH 2/3] clk: Introduce 'critical-clocks' property

[Marek Vasut <marex@denx.de>]

On 3/16/22 12:30, Marek Vasut wrote:
> On 3/16/22 00:52, Stephen Boyd wrote:
>> Quoting Marek Vasut (2022-03-12 02:26:17)
>>> On 3/12/22 06:04, Stephen Boyd wrote:
>>>> Quoting Marek Vasut (2022-03-09 12:54:35)
>>>>> On 2/21/22 01:58, Marek Vasut wrote:
>>>>>> On 2/17/22 23:23, Stephen Boyd wrote:
>>>>>>
>>>>>>> I see that there isn't any more 'clock-critical' in the kernel's 
>>>>>>> dts so
>>>>>>> I wonder if we would be able to get rid of that function or at least
>>>>>>> hollow it out and see if anyone complains. Either way, what is the
>>>>>>> actual problem trying to be solved? If the crystal oscillator 
>>>>>>> isn't used
>>>>>>> anywhere in the kernel why are we registering it with the clk 
>>>>>>> framework?
>>>>>>
>>>>>> The problem is the other way around -- the SoC clock IPs often have a
>>>>>> couple of general purpose clock routed to various SoC IO pins, those
>>>>>> clock can be used for any purpose, and those are already 
>>>>>> registered with
>>>>>> kernel clock framework. Some devices save on BoM and use those 
>>>>>> general
>>>>>> purpose clock to supply clock networks which are otherwise not
>>>>>> interacting with the kernel, like some CPLD for example. Since 
>>>>>> from the
>>>>>> kernel point of view, those clock are unused, the kernel can turn 
>>>>>> those
>>>>>> clock OFF and that will make the entire device fail.
>>>>>>
>>>>>> So this critical-clocks property permits marking clock which must not
>>>>>> ever be turned OFF accordingly.
>>>>>
>>>>> How can we proceed here ?
>>>>
>>>> Why are we registering the clks with the framework on device that are
>>>> saving on BoM and using them outside of the kernel. What is the use of
>>>> kernel memory for struct clk_core that aren't ever used?
>>>
>>> Those clock may be used to supply a device in DT on another hardware
>>> using the same SoC.
>>>
>>> Take e.g. this random git grep result:
>>>
>>> arch/arm/boot/dts/imx7d-remarkable2.dts
>>> / {
>>>     wifi_pwrseq {
>>>       ...
>>>       clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
>>>       ...
>>>     };
>>> };
>>>
>>> This IMX7D_CLKO2_ROOT_DIV is one such general purpose clock output. In
>>> the aforementioned case, it is used to supply 32 kHz clock to a WiFi
>>> chip, i.e. it has a consumer in DT. These clock are registered by the
>>> platform clock driver:
>>>
>>> drivers/clk/imx/clk-imx7d.c
>>>
>>> But those clock can also be used to supply e.g. CPLD which has no other
>>> connection to the SoC but the clock. That is where it needs this
>>> critical-clocks property. Because then there is no consumer in DT. So
>>> the kernel will now think the clock are not used and will turn them off
>>> after boot, thus e.g. crashing such platform.
>>>
>>> So in the later case, the DT would contain the following to avoid the 
>>> crash:
>>> &clks {
>>>     critical-clocks = <IMX7D_CLKO2_ROOT_DIV>;
>>> };
>>
>> Got it. Why, in the latter case, would we register the clk with the clk
>> framework?
> 
> Because those clock may be both critical and have other consumers which 
> can be fully described in DT, i.e. a combination of the two 
> aforementioned use cases.
> 
> The CLK_IS_CRITICAL flag does not imply the clock can only supply single 
> device, rather the CLK_IS_CRITICAL flag indicates the clock must not 
> ever be turned off. The clock can still supply multiple devices, some of 
> them described in DT, some of them not.
> 
> If you were to unregister the clock from clock framework if they are 
> critical, you wouldn't be able to handle the aforementioned use case.
> 
>> I can see that they're "critical" in the sense that there's
>> no consumer node in DT and we want to make sure that nothing turns it
>> off.
> 
> There may be other consumers in DT, we _only_ want to make sure the 
> clock are never turned off, ever.
> 
> The "no consumers in DT" and "never turn clock off" are orthogonal.
> 
>> But it's also wasteful to even register the clk with the kernel
>> because no device is using it. It feels like we need a property like
>> 'clock-dont-register' which is very simiilar to 'protected-clocks'.
>> There's already a binding for 'protected-clocks' so maybe that should be
>> reused and the definition of what the property means can be flexible to
>> handle the various use cases. The cases would be first this one here
>> where a clock doesn't matter because nobody uses it and second how it is
>> used on qualcomm SoCs where they have blocked access to certain clk
>> registers in the firmware so that the system crashes if we try to
>> read/write those clk registers.
>>
>> The dt-binding can be reworded as "the OS shouldn't use these clks" and
>> then the implementation can skip registering those clks with the
>> framework.
> 
> See above, I don't think not registering the critical clock is the right 
> approach.

It has been another month and half, I got no further feedback here. I 
sent V2 with further updated commit message, got no feedback either. I 
re-sent V2 and got no feedback either.

How can we proceed ?