[PATCH v3 1/2] dt-bindings: clk: Introduce 'critical-clocks' property

[PATCH v3 1/2] dt-bindings: clk: Introduce 'critical-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 "critical-clocks"
property of a clock controller allows listing clock which must never
be turned off.

Clock listed in the "critical-clocks" property may have other consumers
in DT, listing the clock in "critical-clocks" only assures those clock
are never turned off, and none of these optional additional consumers
can turn the clock off either.

The implementation is modeled after "protected-clocks".

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: Update the commit message to clarify the behavior
V3: s@Some platforms require clock@Some platforms require some clocks@
---
 .../devicetree/bindings/clock/clock-bindings.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index f2ea53832ac63..d7f7afe2cbd0c 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -169,6 +169,22 @@ a shared clock is forbidden.
 Configuration of common clocks, which affect multiple consumer devices can
 be similarly specified in the clock provider node.
 
+==Critical clocks==
+
+Some platforms require some clocks 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-clocks property of a clock
+controller allows listing clock which must never be turned off.
+
+   clock-controller@a000f000 {
+        compatible = "vendor,clk95;
+        reg = <0xa000f000 0x1000>
+        #clocks-cells = <1>;
+        ...
+        critical-clocks = <UART3_CLK>, <SPI5_CLK>;
+   };
+
 ==Protected clocks==
 
 Some platforms or firmwares may not fully expose all the clocks to the OS, such
-- 
2.35.1

[PATCH v3 2/2] clk: Introduce 'critical-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 "critical-clocks"
property of a clock controller allows listing clock which must never
be turned off.

Clock listed in the "critical-clocks" property may have other consumers
in DT, listing the clock in "critical-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 "critical-clocks" instead.

Unlike "assigned-clocks", the "critical-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
"critical-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
---
 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 f00d4c1158d72..e04bca5e1ff30 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3902,6 +3902,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 critical-clocks; */
+	if (clksize == 0) {
+		if (of_property_read_bool(np, "critical-clocks"))
+			core->flags |= CLK_IS_CRITICAL;
+		return;
+	}
+
+	if (!core->ops->match_clkspec)
+		return;
+
+	clkspec.np = np;
+	clktotal = of_property_count_u32_elems(np, "critical-clocks");
+	clktotal /= clksize;
+	for (index = 0; index < clktotal; index++) {
+		for (i = 0; i < clksize; i++) {
+			ret = of_property_read_u32_index(np, "critical-clocks",
+							 (index * clksize) + i,
+							 &(clkspec.args[i]));
+			if (ret) {
+				pr_warn("Skipping critical-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)
 {
@@ -3946,6 +3988,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 c10dc4c659e23..f65f6ef4e9985 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 v3 2/2] clk: Introduce 'critical-clocks' property

[Stephen Boyd]

Quoting Marek Vasut (2022-05-17 16:59:19)
> 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 "critical-clocks"
> property of a clock controller allows listing clock which must never
> be turned off.
> 
> Clock listed in the "critical-clocks" property may have other consumers
> in DT, listing the clock in "critical-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 "critical-clocks" instead.
> 
> Unlike "assigned-clocks", the "critical-clocks" must be parsed much earlier
> in __clk_register() to assign CLK_IS_CRITICAL flag to clk_init_data .flags
> field.

Why? Instead of using the CLK_IS_CRITICAL flag to enable at registration
time for this, why can't we parse the property when a clk provider is
registered and enable those clks manually and then set the
CLK_IS_CRITICAL flag? Ideally we don't implement another clk_op for
this.

> 
> 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-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>

Re: [PATCH v3 1/2] dt-bindings: clk: Introduce 'critical-clocks' property

[Rob Herring]

On Wed, May 18, 2022 at 01:59:18AM +0200, Marek Vasut wrote:
> 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 "critical-clocks"
> property of a clock controller allows listing clock which must never
> be turned off.
> 
> Clock listed in the "critical-clocks" property may have other consumers
> in DT, listing the clock in "critical-clocks" only assures those clock
> are never turned off, and none of these optional additional consumers
> can turn the clock off either.
> 
> The implementation is modeled after "protected-clocks".
> 
> 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: Update the commit message to clarify the behavior
> V3: s@Some platforms require clock@Some platforms require some clocks@
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

This file is removed upstream as it is replaced by the schema in 
dtschema. 

But I'd wait to see what Stephen says. I'm fine with the addition.

Rob

Re: [PATCH v3 1/2] dt-bindings: clk: Introduce 'critical-clocks' property

[Stephen Boyd]

Quoting Marek Vasut (2022-05-17 16:59:18)
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index f2ea53832ac63..d7f7afe2cbd0c 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -169,6 +169,22 @@ a shared clock is forbidden.
>  Configuration of common clocks, which affect multiple consumer devices can
>  be similarly specified in the clock provider node.
>  
> +==Critical clocks==
> +
> +Some platforms require some clocks 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-clocks property of a clock
> +controller allows listing clock which must never be turned off.
> +
> +   clock-controller@a000f000 {
> +        compatible = "vendor,clk95;
> +        reg = <0xa000f000 0x1000>
> +        #clocks-cells = <1>;
> +        ...
> +        critical-clocks = <UART3_CLK>, <SPI5_CLK>;

Historically "critical" is overloaded in the clk framework. We should
avoid using that name. What does "critical" even mean?

Instead I'd prefer "always-on-clocks" here, so we can indicate that
these clks should always be on. It would also parallel the property in
the regulator framework.

Re: [PATCH v3 1/2] dt-bindings: clk: Introduce 'critical-clocks' property

[Marek Vasut]

On 6/15/22 22:10, Stephen Boyd wrote:
> Quoting Marek Vasut (2022-05-17 16:59:18)
>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> index f2ea53832ac63..d7f7afe2cbd0c 100644
>> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> @@ -169,6 +169,22 @@ a shared clock is forbidden.
>>   Configuration of common clocks, which affect multiple consumer devices can
>>   be similarly specified in the clock provider node.
>>   
>> +==Critical clocks==
>> +
>> +Some platforms require some clocks 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-clocks property of a clock
>> +controller allows listing clock which must never be turned off.
>> +
>> +   clock-controller@a000f000 {
>> +        compatible = "vendor,clk95;
>> +        reg = <0xa000f000 0x1000>
>> +        #clocks-cells = <1>;
>> +        ...
>> +        critical-clocks = <UART3_CLK>, <SPI5_CLK>;
> 
> Historically "critical" is overloaded in the clk framework. We should
> avoid using that name. What does "critical" even mean?

It means those clock must not be turned off, but there is no consumer 
described in DT.

> Instead I'd prefer "always-on-clocks" here, so we can indicate that
> these clks should always be on. It would also parallel the property in
> the regulator framework.

This property name is derived from protected-clock which you introduced. 
I think it would be better to stay consistent within the clock framework 
property names ?

Re: [PATCH v3 1/2] dt-bindings: clk: Introduce 'critical-clocks' property

[Stephen Boyd]

Quoting Marek Vasut (2022-06-15 14:55:17)
> On 6/15/22 22:10, Stephen Boyd wrote:
> > Quoting Marek Vasut (2022-05-17 16:59:18)
> >> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> index f2ea53832ac63..d7f7afe2cbd0c 100644
> >> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> @@ -169,6 +169,22 @@ a shared clock is forbidden.
> >>   Configuration of common clocks, which affect multiple consumer devices can
> >>   be similarly specified in the clock provider node.
> >>   
> >> +==Critical clocks==
> >> +
> >> +Some platforms require some clocks 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-clocks property of a clock
> >> +controller allows listing clock which must never be turned off.
> >> +
> >> +   clock-controller@a000f000 {
> >> +        compatible = "vendor,clk95;
> >> +        reg = <0xa000f000 0x1000>
> >> +        #clocks-cells = <1>;
> >> +        ...
> >> +        critical-clocks = <UART3_CLK>, <SPI5_CLK>;
> > 
> > Historically "critical" is overloaded in the clk framework. We should
> > avoid using that name. What does "critical" even mean?
> 
> It means those clock must not be turned off, but there is no consumer 
> described in DT.

So it means "always on".

> 
> > Instead I'd prefer "always-on-clocks" here, so we can indicate that
> > these clks should always be on. It would also parallel the property in
> > the regulator framework.
> 
> This property name is derived from protected-clock which you introduced. 
> I think it would be better to stay consistent within the clock framework 
> property names ?

protected-clocks is based on assigned-clocks. There isn't a
CLK_IS_PROTECTED flag. I'm not following your argument at all here,
sorry.

Re: [PATCH v3 1/2] dt-bindings: clk: Introduce 'critical-clocks' property

[Marek Vasut]

On 6/16/22 00:22, Stephen Boyd wrote:
> Quoting Marek Vasut (2022-06-15 14:55:17)
>> On 6/15/22 22:10, Stephen Boyd wrote:
>>> Quoting Marek Vasut (2022-05-17 16:59:18)
>>>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> index f2ea53832ac63..d7f7afe2cbd0c 100644
>>>> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> @@ -169,6 +169,22 @@ a shared clock is forbidden.
>>>>    Configuration of common clocks, which affect multiple consumer devices can
>>>>    be similarly specified in the clock provider node.
>>>>    
>>>> +==Critical clocks==
>>>> +
>>>> +Some platforms require some clocks 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-clocks property of a clock
>>>> +controller allows listing clock which must never be turned off.
>>>> +
>>>> +   clock-controller@a000f000 {
>>>> +        compatible = "vendor,clk95;
>>>> +        reg = <0xa000f000 0x1000>
>>>> +        #clocks-cells = <1>;
>>>> +        ...
>>>> +        critical-clocks = <UART3_CLK>, <SPI5_CLK>;
>>>
>>> Historically "critical" is overloaded in the clk framework. We should
>>> avoid using that name. What does "critical" even mean?
>>
>> It means those clock must not be turned off, but there is no consumer
>> described in DT.
> 
> So it means "always on".
> 
>>
>>> Instead I'd prefer "always-on-clocks" here, so we can indicate that
>>> these clks should always be on. It would also parallel the property in
>>> the regulator framework.
>>
>> This property name is derived from protected-clock which you introduced.
>> I think it would be better to stay consistent within the clock framework
>> property names ?
> 
> protected-clocks is based on assigned-clocks. There isn't a
> CLK_IS_PROTECTED flag. I'm not following your argument at all here,
> sorry.

critical-clock property name is based on protected-clock property name.

There is also no CLK_IS_ALWAYS_ON flag , but there is CLK_IS_CRITICAL 
flag . Sure, there is no CLK_IS_PROTECTED flag because the protected 
clock is implemented only by a single driver (qualcomm).

I think it makes sense to align the DT property name and the flag name, 
and the critical-clock is aligned with both other DT property names in 
the clock framework and the flag name.

Re: [PATCH v3 1/2] dt-bindings: clk: Introduce 'critical-clocks' property

[Stephen Boyd]

Quoting Marek Vasut (2022-06-15 16:42:25)
> On 6/16/22 00:22, Stephen Boyd wrote:
> > Quoting Marek Vasut (2022-06-15 14:55:17)
> >>
> >> It means those clock must not be turned off, but there is no consumer
> >> described in DT.
> > 
> > So it means "always on".
> > 
> >>
> >>> Instead I'd prefer "always-on-clocks" here, so we can indicate that
> >>> these clks should always be on. It would also parallel the property in
> >>> the regulator framework.
> >>
> >> This property name is derived from protected-clock which you introduced.
> >> I think it would be better to stay consistent within the clock framework
> >> property names ?
> > 
> > protected-clocks is based on assigned-clocks. There isn't a
> > CLK_IS_PROTECTED flag. I'm not following your argument at all here,
> > sorry.
> 
> critical-clock property name is based on protected-clock property name.

Got that.

> 
> There is also no CLK_IS_ALWAYS_ON flag , but there is CLK_IS_CRITICAL 
> flag . Sure, there is no CLK_IS_PROTECTED flag because the protected 
> clock is implemented only by a single driver (qualcomm).

Alright, thanks for clarifying the argument.

> 
> I think it makes sense to align the DT property name and the flag name, 
> and the critical-clock is aligned with both other DT property names in 
> the clock framework and the flag name.

No, it does not make sense to align the CLK_IS_CRITICAL flag with the DT
property name. The property should be named what it is, i.e. always on.
The internal clk framework details could be changed at any time whereas
the DT binding is essentially forever. I agree using a similar name may
help connect the code and the DT, but that is nowhere near as important
as choosing a name that expresses what is happening in a binding that we
need to maintain for the foreseeable future.