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

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

Hi deee Ho Marek,

Long time no chatter :/ Nice to hear from you now!

On 2/15/22 10:44, Marek Vasut wrote:
> 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.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-power@fi.rohmeurope.com
> To: linux-clk@vger.kernel.org
> ---
>   drivers/clk/clk.c            | 41 ++++++++++++++++++++++++++++++++++++
>   include/linux/clk-provider.h |  3 +++
>   2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8de6a22498e70..1e1686fa76e01 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3872,6 +3872,45 @@ static void clk_core_free_parent_map(struct clk_core *core)
>   	kfree(core->parents);
>   }
>   
> +static void
> +__clk_register_critical_clock(struct device_node *np, struct clk_core *core,
> +			      struct clk_hw *hw)
> +{
> +	struct of_phandle_args clkspec;
> +	u32 clksize, clktotal;
> +	int ret, i, index;
> +
> +	if (!np)
> +		return;
> +
> +	if (!core->ops->match_clkspec)
> +		return;
> +
> +	if (of_property_read_u32(np, "#clock-cells", &clksize))
> +		return;
> +
> +	/* Clock node with #clock-cells = <0> uses critical-clocks; */
> +	if (clksize == 0) {
> +		if (of_property_read_bool(np, "critical-clocks") &&
> +		    !core->ops->match_clkspec(hw, &clkspec))

I think this is never true as there is
if (!core->ops->match_clkspec)
	return;

above.

Anyways, seeing you added a dummy bd71837_match_clkspec in a follow-up 
patch for BD71837 - which has only single clock - I wonder if there is a 
way to omit that dummy callback in controllers which really provide only 
one clock? Eg, do you think such a situation could be detected by the 
core already here? Please just go on if that is hard - I was just 
thinking that maybe we could avoid such dummies - or at least provide 
one single dummy helper for that purpose instead of adding one in all 
similar drivers. How do you think?

Other than this - I still do like this idea! Thanks for working to 
implement this!


Best Regards
	-- Matti

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~