dt-bindings: clk: Introduce 'critical-clocks' property
Subsystems (2)
PATCH v3 1/2 dt-bindings: clk: Introduce 'critical-clocks' property
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".
To: [email protected]
---
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(+)
Documentation/devicetree/bindings/clock/clock-bindings.txt +16 -0
@@ -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
Signature · 1 line
2.35.1
PATCH v3 2/2 clk: Introduce 'critical-clocks' property
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.
To: [email protected]
---
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(+)
drivers/clk/clk.c +44 -0
@@ -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;
include/linux/clk-provider.h +3 -0
@@ -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); };/**
Signature · 1 line
2.35.1
Stephen Boyd
maintainerRe: [PATCH v3 2/2] clk: Introduce 'critical-clocks' property
Quoting Marek Vasut (2022-05-17 16:59:19)
Quoted 19 lines
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_CRITICALflag 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.
Quoted 7 lines
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 assignCLK_IS_CRITICALflag toclk_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.
Quoted 7 lines
The new
match_clkspec()callback is used to determine whether structclk_hwthat is currently being registered matches the clock specifier in the DT "critical-clocks" property, and if so, then theCLK_IS_CRITICALis added to these newly registered clock. This callback can only be driver specific.Signed-off-by Marek Vasut <[email protected]>
Rob Herring
maintainerQuoted 22 lines · 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 <[email protected]>Cc Andrew Morton <[email protected]>Cc Michael Turquette <[email protected]>Cc Rob Herring <[email protected]>Cc Stephen Boyd <[email protected]>
Quoted 7 lines
To: [email protected]
---
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
Stephen Boyd
maintainerQuoting Marek Vasut (2022-05-17 16:59:18)
Quoted 12 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt +4
-0@@ -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
Documentation/devicetree/bindings/clock/clock-bindings.txt a shared clock is forbidden. · line 185
Quoted 10 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt +10
-0+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.
Quoted 18 lines · Stephen Boyd
Quoting Marek Vasut (2022-05-17 16:59:18)
Quoted 17 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt +9
-0@@ -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 {
Quoted 8 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt a shared clock is forbidden. · line 185
Quoted 5 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt +5
-0+ 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.
Quoted 3 lines
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 ?
Stephen Boyd
maintainerQuoting Marek Vasut (2022-06-15 14:55:17)
Quoted 28 lines · Stephen Boyd
Quoted 18 lines
Quoting Marek Vasut (2022-05-17 16:59:18)
Quoted 17 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt +9
-0@@ -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 {Quoted 8 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt a shared clock is forbidden. · line 185
Quoted 5 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt +5
-0+ 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?
Quoted 2 lines
It means those clock must not be turned off, but there is no consumer described in DT.
So it means "always on".
Quoted 8 lines
Quoted 4 lines
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.
Quoted 39 lines · Stephen Boyd
Quoting Marek Vasut (2022-06-15 14:55:17)
Quoted 28 lines · Stephen Boyd
Quoted 18 lines
Quoting Marek Vasut (2022-05-17 16:59:18)
Quoted 17 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt +9
-0@@ -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 {Quoted 8 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt a shared clock is forbidden. · line 185
Quoted 5 lines
Documentation/devicetree/bindings/clock/clock-bindings.txt +5
-0+ 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?
Quoted 2 lines
It means those clock must not be turned off, but there is no consumer described in DT.
So it means "always on".
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 3 lines
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_PROTECTEDflag. 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.
Stephen Boyd
maintainerQuoting Marek Vasut (2022-06-15 16:42:25)
Quoted 17 lines · Stephen Boyd
Quoted 16 lines
Quoting Marek Vasut (2022-06-15 14:55:17)
Quoted 3 lines
It means those clock must not be turned off, but there is no consumer described in DT.
So it means "always on".
Quoted 8 lines
Quoted 4 lines
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 ?
Quoted 5 lines
Quoted 3 lines
protected-clocks is based on assigned-clocks. There isn't a
CLK_IS_PROTECTEDflag. I'm not following your argument at all here, sorry.critical-clock property name is based on protected-clock property name.
Got that.
Quoted 4 lines
There is also no
CLK_IS_ALWAYS_ONflag , but there isCLK_IS_CRITICALflag . Sure, there is noCLK_IS_PROTECTEDflag because the protected clock is implemented only by a single driver (qualcomm).
Alright, thanks for clarifying the argument.
Quoted 4 lines
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.