[PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property

[PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property

[Marek Vasut]

NOTE: This is an RFC patch showing how this mechanism might be workable.

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.

Thoughts (on the overall design, not code quality or patch splitting) ?

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
---
 .../bindings/clock/clock-bindings.txt         | 16 ++++++++++++
 drivers/clk/clk-bd718x7.c                     | 15 +++++++++++
 drivers/clk/clk.c                             | 25 +++++++++++++++++++
 include/linux/clk-provider.h                  |  2 ++
 4 files changed, 58 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index f2ea53832ac63..d9a783c35c5a1 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 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.
+
+   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
diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
index a59bc57f13bc4..f40765e2860e4 100644
--- a/drivers/clk/clk-bd718x7.c
+++ b/drivers/clk/clk-bd718x7.c
@@ -70,10 +70,25 @@ static int bd71837_clk_is_enabled(struct clk_hw *hw)
 	return enabled & c->mask;
 }
 
+static int bd71837_match_clkspec(struct clk_hw *hw, struct of_phandle_args *clkspec)
+{
+	struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
+
+	/*
+	 * if (clk_hw == clkspec)
+	 *   return 0;
+	 * else
+	 *   return 1;
+	 */
+
+	return 0;
+}
+
 static const struct clk_ops bd71837_clk_ops = {
 	.prepare = &bd71837_clk_enable,
 	.unprepare = &bd71837_clk_disable,
 	.is_prepared = &bd71837_clk_is_enabled,
+	.match_clkspec = &bd71837_match_clkspec,
 };
 
 static int bd71837_clk_probe(struct platform_device *pdev)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f467d63bbf1ee..fa8e9ea446158 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3849,6 +3849,31 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 	core->max_rate = ULONG_MAX;
 	hw->core = core;
 
+	struct of_phandle_args clkspec;
+	u32 clksize, clktotal;
+	int i, index;
+
+	if (np && core->ops->match_clkspec && !of_property_read_u32(np, "#clock-cells", &clksize)) {
+		if (clksize == 0) {
+			if (of_property_read_bool(np, "critical-clocks"))
+				core->flags |= CLK_IS_CRITICAL;
+			clktotal = 0;
+		} else {
+			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 (!core->ops->match_clkspec(hw, &clkspec))
+					core->flags |= CLK_IS_CRITICAL;
+			}
+		}
+	}
+
 	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 f59c875271a0e..766e93efb23c5 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -205,6 +205,7 @@ 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
  *
  * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
  * implementations to split any work between atomic (enable) and sleepable
@@ -252,6 +253,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.33.0

Re: [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property

[Vaittinen, Matti]

On 11/9/21 00:42, Marek Vasut wrote:
> NOTE: This is an RFC patch showing how this mechanism might be workable.
> 
> 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.

Hm. It slightly bugs me to parse the clock properties form DT in many 
places. I was thinking that perhaps we could parse all the clk 
properties at __clk_register() and store them to be later used when 
of_clk_provider is added. After having a chat with Marek I was kindly 
explained the DT node might not always be present at __clk_register() - 
phase. I don't easily see a better way so I'd better to just learn to 
live with the bugging feeling :)

> 
> 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 do also like the idea of being able to replace the get-callback - and 
a driver specific callback with a generic op. Can the clock-indices be 
somehow used for generic one? Perhaps allowing also adding a driver 
specific callback for cases where generic one can't do the job? (I don't 
have any concrete idea/code how to do that right now though). But what 
ever it is worth, I do like the overall idea. It sounds right to me.

> Thoughts (on the overall design, not code quality or patch splitting) ?
> 
> 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
> ---
>   .../bindings/clock/clock-bindings.txt         | 16 ++++++++++++
>   drivers/clk/clk-bd718x7.c                     | 15 +++++++++++
>   drivers/clk/clk.c                             | 25 +++++++++++++++++++
>   include/linux/clk-provider.h                  |  2 ++
>   4 files changed, 58 insertions(+)
> 

Best Regards
--Matti Vaittinen

Re: [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property

[Rob Herring]

On Mon, Nov 08, 2021 at 11:42:42PM +0100, Marek Vasut wrote:
> NOTE: This is an RFC patch showing how this mechanism might be workable.
> 
> 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.
> 
> Thoughts (on the overall design, not code quality or patch splitting) ?
> 
> 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
> ---
>  .../bindings/clock/clock-bindings.txt         | 16 ++++++++++++
>  drivers/clk/clk-bd718x7.c                     | 15 +++++++++++
>  drivers/clk/clk.c                             | 25 +++++++++++++++++++
>  include/linux/clk-provider.h                  |  2 ++
>  4 files changed, 58 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index f2ea53832ac63..d9a783c35c5a1 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 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.
> +
> +   clock-controller@a000f000 {
> +        compatible = "vendor,clk95;
> +        reg = <0xa000f000 0x1000>
> +        #clocks-cells = <1>;
> +        ...
> +        critical-clocks = <UART3_CLK>, <SPI5_CLK>;

This will need a schema definition in dtschema.

Otherwise, the concept is fine for me. 

Rob

Re: [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property

[Marek Vasut]

On 11/29/21 21:18, Rob Herring wrote:
> On Mon, Nov 08, 2021 at 11:42:42PM +0100, Marek Vasut wrote:
>> NOTE: This is an RFC patch showing how this mechanism might be workable.
>>
>> 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.
>>
>> Thoughts (on the overall design, not code quality or patch splitting) ?
>>
>> 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
>> ---
>>   .../bindings/clock/clock-bindings.txt         | 16 ++++++++++++
>>   drivers/clk/clk-bd718x7.c                     | 15 +++++++++++
>>   drivers/clk/clk.c                             | 25 +++++++++++++++++++
>>   include/linux/clk-provider.h                  |  2 ++
>>   4 files changed, 58 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> index f2ea53832ac63..d9a783c35c5a1 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 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.
>> +
>> +   clock-controller@a000f000 {
>> +        compatible = "vendor,clk95;
>> +        reg = <0xa000f000 0x1000>
>> +        #clocks-cells = <1>;
>> +        ...
>> +        critical-clocks = <UART3_CLK>, <SPI5_CLK>;
> 
> This will need a schema definition in dtschema.
> 
> Otherwise, the concept is fine for me.

I sent out proper patches, also for the schemas.