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

[Marek Vasut <marex@denx.de>]

On 2/21/22 01:58, Marek Vasut wrote:
> On 2/17/22 23:23, Stephen Boyd wrote:
>> Quoting Marek Vasut (2022-02-15 00:44:11)
>>> Some platforms require clock 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-clock property of a clock 
>>> controller
>>> allows listing clock which must never be turned off.
>>>
>>> The implementation here is similar to "protected-clock", except 
>>> protected
>>> clock property is currently driver specific. This patch attempts to make
>>> a generic implementation of "critical-clock" instead.
>>>
>>> Unlike "assigned-clocks", the "critical-clock" must be parsed much 
>>> earlier
>>> in __clk_register() to assign CLK_IS_CRITICAL flag to clk_init_data 
>>> .flags
>>> field. The parsing code obviously need to be cleaned up and factor 
>>> out into
>>> separate function.
>>>
>>> The new match_clkspec() callback is used to determine whether struct 
>>> clk_hw
>>> that is currently being registered matches the clock specifier in the DT
>>> "critical-clock" property, and if so, then the CLK_IS_CRITICAL is 
>>> added to
>>> these newly registered clock. This callback is currently driver 
>>> specific,
>>> although I suspect a common and/or generic version of the callback could
>>> be added. Also, this new callback could possibly be used to replace 
>>> (*get)
>>> argument of of_clk_add_hw_provider() later on too.
>>
>> I don't see any mention of of_clk_detect_critical() here. We don't want
>> to enshrine the critical clk flag in DT. There was a bunch of discussion
>> about this on the mailing list years ago and the end result was this
>> instantly deprecated function to set the flag based on a DT property.
>> That thread isn't mentioned here either.
> 
> I wasn't aware of clock-critical DT prop, but it seems deprecated and 
> not generic enough anyway.
> 
>> 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 ?