[PATCH v4] clk: Introduce 'always-on-clocks' property

[PATCH v4] clk: Introduce 'always-on-clocks' property

[Marek Vasut]

Some platforms require select clock to be always running, e.g. because
those clock supply vital devices which are not otherwise attached to
the system and thus do not have a matching DT node and clock consumer.

An example is a system where the SoC serves as a crystal oscillator
replacement for a programmable logic device. The "always-on-clocks"
property of a clock controller allows listing clock which must never
be turned off.

Clock listed in the "always-on-clocks" property may have other consumers
in DT, listing the clock in "always-on-clocks" only assures those clock
are never turned off, and none of these optional additional consumers
can turn the clock off either. This is achieved by adding CLK_IS_CRITICAL
flag to these critical clock.

This flag has thus far been added to select clock by hard-coding it in
various clock drivers, this patch provides generic DT interface to add
the flag to arbitrary clock that may be critical.

The implementation is modeled after "protected-clocks", except the protected
clock property is currently driver specific. This patch attempts to provide
a generic implementation of "always-on-clocks" instead.

Unlike "assigned-clocks", the "always-on-clocks" must be parsed much earlier
in __clk_register() to assign CLK_IS_CRITICAL flag to clk_init_data .flags
field.

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
"always-on-clocks" property, and if so, then the CLK_IS_CRITICAL is added to
these newly registered clock. This callback can only be driver specific.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: devicetree@vger.kernel.org
To: linux-clk@vger.kernel.org
---
V2: - Warn in case critical-clock field cannot be parsed and skip those clock
    - Use match_clkspec() only for non-zero clock-cells controllers
    - Pull the critical-clock code into __clk_register_critical_clock()
    - Update commit message
V3: - Pick np from clk_core->of_node
V4: - Rename DT property critical-clocks to always-on-clocks
---
 drivers/clk/clk.c            | 44 ++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  3 +++
 2 files changed, 47 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b70769d0db99f..6b07f1a086277 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3900,6 +3900,48 @@ static void clk_core_free_parent_map(struct clk_core *core)
 	kfree(core->parents);
 }
 
+static void
+__clk_register_critical_clock(struct clk_core *core, struct clk_hw *hw)
+{
+	struct device_node *np = core->of_node;
+	struct of_phandle_args clkspec;
+	u32 clksize, clktotal;
+	int ret, i, index;
+
+	if (!np)
+		return;
+
+	if (of_property_read_u32(np, "#clock-cells", &clksize))
+		return;
+
+	/* Clock node with #clock-cells = <0> uses always-on-clocks; */
+	if (clksize == 0) {
+		if (of_property_read_bool(np, "always-on-clocks"))
+			core->flags |= CLK_IS_CRITICAL;
+		return;
+	}
+
+	if (!core->ops->match_clkspec)
+		return;
+
+	clkspec.np = np;
+	clktotal = of_property_count_u32_elems(np, "always-on-clocks");
+	clktotal /= clksize;
+	for (index = 0; index < clktotal; index++) {
+		for (i = 0; i < clksize; i++) {
+			ret = of_property_read_u32_index(np, "always-on-clocks",
+							 (index * clksize) + i,
+							 &(clkspec.args[i]));
+			if (ret) {
+				pr_warn("Skipping always-on-clocks index %d (ret=%d)\n",
+					i, ret);
+			}
+		}
+		if (!core->ops->match_clkspec(hw, &clkspec))
+			core->flags |= CLK_IS_CRITICAL;
+	}
+}
+
 static struct clk *
 __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 {
@@ -3944,6 +3986,8 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 	core->min_rate = 0;
 	core->max_rate = ULONG_MAX;
 
+	__clk_register_critical_clock(core, hw);
+
 	ret = clk_core_populate_parent_map(core, init);
 	if (ret)
 		goto fail_parents;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index dec1bcae43790..e02a66dc482be 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -205,6 +205,8 @@ struct clk_duty {
  *		directory is provided as an argument.  Called with
  *		prepare_lock held.  Returns 0 on success, -EERROR otherwise.
  *
+ * @match_clkspec: Check whether clk_hw matches DT clock specifier.
+ *		Returns 0 on success, -EERROR otherwise.
  *
  * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
  * implementations to split any work between atomic (enable) and sleepable
@@ -252,6 +254,7 @@ struct clk_ops {
 	int		(*init)(struct clk_hw *hw);
 	void		(*terminate)(struct clk_hw *hw);
 	void		(*debug_init)(struct clk_hw *hw, struct dentry *dentry);
+	int		(*match_clkspec)(struct clk_hw *hw, struct of_phandle_args *clkspec);
 };
 
 /**
-- 
2.35.1

Re: [PATCH v4] clk: Introduce 'always-on-clocks' property

[Stephen Boyd]

Quoting Marek Vasut (2022-09-24 10:45:17)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b70769d0db99f..6b07f1a086277 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3900,6 +3900,48 @@ static void clk_core_free_parent_map(struct clk_core *core)
>         kfree(core->parents);
>  }
>  
> +static void
> +__clk_register_critical_clock(struct clk_core *core, struct clk_hw *hw)
> +{
> +       struct device_node *np = core->of_node;
> +       struct of_phandle_args clkspec;
> +       u32 clksize, clktotal;
> +       int ret, i, index;
> +
> +       if (!np)
> +               return;
> +
> +       if (of_property_read_u32(np, "#clock-cells", &clksize))
> +               return;
> +
> +       /* Clock node with #clock-cells = <0> uses always-on-clocks; */
> +       if (clksize == 0) {
> +               if (of_property_read_bool(np, "always-on-clocks"))
> +                       core->flags |= CLK_IS_CRITICAL;

Why must we set the CLK_IS_CRITICAL flag like this? Instead, when the
clk provider is registered, parse the node of the provider and get the
clks to call clk_prepare_enable() on. We can set the critical flag or
make a new flag that causes clk_disable_unprepare() to not actually turn
the clk off, if we have some sort of underflow issue with other
consumers. Does that fail somehow?

> +               return;
> +       }
> +
> +       if (!core->ops->match_clkspec)
> +               return;
> +
> +       clkspec.np = np;
> +       clktotal = of_property_count_u32_elems(np, "always-on-clocks");
> +       clktotal /= clksize;
> +       for (index = 0; index < clktotal; index++) {
> +               for (i = 0; i < clksize; i++) {

I'm mainly thinking that we're going to spin on this loop constantly for
any clk providers that have many clks to register, but only a few to
keep always on. It would be best to avoid that and only run through the
DT property once.

> +                       ret = of_property_read_u32_index(np, "always-on-clocks",
> +                                                        (index * clksize) + i,
> +                                                        &(clkspec.args[i]));
> +                       if (ret) {
> +                               pr_warn("Skipping always-on-clocks index %d (ret=%d)\n",
> +                                       i, ret);
> +                       }
> +               }
> +               if (!core->ops->match_clkspec(hw, &clkspec))

This callback is provider specific, and not necessarily clk_hw specific.
For example, the clk_ops could be for a generic gate bit, but the
provider wants to keep that gate always on. To implement such a clk we
would have to copy the generic gate clk_ops and set this match_clkspec
op. I'd like to avoid that if possible. If the clk_op must exist, then
perhaps it should be in clk_init_data, which is sort of the place where
we put provider specific information for a clk, i.e. clk_parent_data.

> +                       core->flags |= CLK_IS_CRITICAL;
> +       }
> +}
> +
>  static struct clk *
>  __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>  {

Re: [PATCH v4] clk: Introduce 'always-on-clocks' property

[Marek Vasut]

On 10/4/22 20:26, Stephen Boyd wrote:
> Quoting Marek Vasut (2022-09-24 10:45:17)
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index b70769d0db99f..6b07f1a086277 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -3900,6 +3900,48 @@ static void clk_core_free_parent_map(struct clk_core *core)
>>          kfree(core->parents);
>>   }
>>   
>> +static void
>> +__clk_register_critical_clock(struct clk_core *core, struct clk_hw *hw)
>> +{
>> +       struct device_node *np = core->of_node;
>> +       struct of_phandle_args clkspec;
>> +       u32 clksize, clktotal;
>> +       int ret, i, index;
>> +
>> +       if (!np)
>> +               return;
>> +
>> +       if (of_property_read_u32(np, "#clock-cells", &clksize))
>> +               return;
>> +
>> +       /* Clock node with #clock-cells = <0> uses always-on-clocks; */
>> +       if (clksize == 0) {
>> +               if (of_property_read_bool(np, "always-on-clocks"))
>> +                       core->flags |= CLK_IS_CRITICAL;
> 
> Why must we set the CLK_IS_CRITICAL flag like this?

I don't quite understand the question here. Clock which shouldn't be 
turned off should have CLK_IS_CRITICAL flag set.

> Instead, when the
> clk provider is registered, parse the node of the provider and get the
> clks to call clk_prepare_enable() on. We can set the critical flag or
> make a new flag that causes clk_disable_unprepare() to not actually turn
> the clk off, if we have some sort of underflow issue with other
> consumers. Does that fail somehow?

Would your proposal be something that would have to be implemented in 
every single clock driver separately ?

>> +               return;
>> +       }
>> +
>> +       if (!core->ops->match_clkspec)
>> +               return;
>> +
>> +       clkspec.np = np;
>> +       clktotal = of_property_count_u32_elems(np, "always-on-clocks");
>> +       clktotal /= clksize;
>> +       for (index = 0; index < clktotal; index++) {
>> +               for (i = 0; i < clksize; i++) {
> 
> I'm mainly thinking that we're going to spin on this loop constantly for
> any clk providers that have many clks to register, but only a few to
> keep always on. It would be best to avoid that and only run through the
> DT property once.

Where could this be implemented in the clock core ?

>> +                       ret = of_property_read_u32_index(np, "always-on-clocks",
>> +                                                        (index * clksize) + i,
>> +                                                        &(clkspec.args[i]));
>> +                       if (ret) {
>> +                               pr_warn("Skipping always-on-clocks index %d (ret=%d)\n",
>> +                                       i, ret);
>> +                       }
>> +               }
>> +               if (!core->ops->match_clkspec(hw, &clkspec))
> 
> This callback is provider specific, and not necessarily clk_hw specific.
> For example, the clk_ops could be for a generic gate bit, but the
> provider wants to keep that gate always on. To implement such a clk we
> would have to copy the generic gate clk_ops and set this match_clkspec
> op. I'd like to avoid that if possible. If the clk_op must exist, then
> perhaps it should be in clk_init_data, which is sort of the place where
> we put provider specific information for a clk, i.e. clk_parent_data.

The core->ops is copied from struct clk_init_data .ops in 
__clk_register() , just before __clk_register_critical_clock() is 
called, so that op is already in clk_init_data.