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@xxxxxxx>
> Cc: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx
> Cc: linux-power@xxxxxxxxxxxxxxxxx
> To: linux-clk@xxxxxxxxxxxxxxx
> ---
> 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 ~~
