[2/3] clk: Introduce 'critical-clocks' property

Commit Message

Marek Vasut Feb. 15, 2022, 8:44 a.m. UTC

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 <[e-mail]>
Cc: Matti Vaittinen <[e-mail]>
Cc: Michael Turquette <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
To: [e-mail]
---
 drivers/clk/clk.c            | 41 ++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  3 +++
 2 files changed, 44 insertions(+)

Comments

kernel test robot Feb. 15, 2022, 11:23 a.m. UTC | #1

Hi Marek,

I love your patch! Perhaps something to improve:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on v5.17-rc4 next-20220214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Marek-Vasut/dt-bindings-clk-Introduce-critical-clocks-property/20220215-164757
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: nds32-allnoconfig (https://download.01.org/0day-ci/archive/20220215/[e-mail]/config)
compiler: nds32le-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/aded04bc3dec13df3f940621d94d84e32ff8a5ea
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Marek-Vasut/dt-bindings-clk-Introduce-critical-clocks-property/20220215-164757
        git checkout aded04bc3dec13df3f940621d94d84e32ff8a5ea
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nds32 SHELL=/bin/bash drivers/clk/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

   drivers/clk/clk.c: In function '__clk_register_critical_clock':
>> drivers/clk/clk.c:3881:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
    3881 |         int ret, i, index;
         |             ^~~


vim +/ret +3881 drivers/clk/clk.c

  3874	
  3875	static void
  3876	__clk_register_critical_clock(struct device_node *np, struct clk_core *core,
  3877				      struct clk_hw *hw)
  3878	{
  3879		struct of_phandle_args clkspec;
  3880		u32 clksize, clktotal;
> 3881		int ret, i, index;
  3882	
  3883		if (!np)
  3884			return;
  3885	
  3886		if (!core->ops->match_clkspec)
  3887			return;
  3888	
  3889		if (of_property_read_u32(np, "#clock-cells", &clksize))
  3890			return;
  3891	
  3892		/* Clock node with #clock-cells = <0> uses critical-clocks; */
  3893		if (clksize == 0) {
  3894			if (of_property_read_bool(np, "critical-clocks") &&
  3895			    !core->ops->match_clkspec(hw, &clkspec))
  3896				core->flags |= CLK_IS_CRITICAL;
  3897			return;
  3898		}
  3899	
  3900		clkspec.np = np;
  3901		clktotal = of_property_count_u32_elems(np, "critical-clocks");
  3902		clktotal /= clksize;
  3903		for (index = 0; index < clktotal; index++) {
  3904			for (i = 0; i < clksize; i++) {
  3905				ret = of_property_read_u32_index(np, "critical-clocks",
  3906								 (index * clksize) + i,
  3907								 &(clkspec.args[i]));
  3908			}
  3909			if (!core->ops->match_clkspec(hw, &clkspec))
  3910				core->flags |= CLK_IS_CRITICAL;
  3911		}
  3912	}
  3913	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

kernel test robot Feb. 15, 2022, 1:57 p.m. UTC | #2

Hi Marek,

I love your patch! Perhaps something to improve:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on v5.17-rc4 next-20220215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Marek-Vasut/dt-bindings-clk-Introduce-critical-clocks-property/20220215-164757
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: x86_64-randconfig-a002-20220214 (https://download.01.org/0day-ci/archive/20220215/[e-mail]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 37f422f4ac31c8b8041c6b62065263314282dab6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/aded04bc3dec13df3f940621d94d84e32ff8a5ea
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Marek-Vasut/dt-bindings-clk-Introduce-critical-clocks-property/20220215-164757
        git checkout aded04bc3dec13df3f940621d94d84e32ff8a5ea
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/clk/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> drivers/clk/clk.c:3881:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
           int ret, i, index;
               ^
   1 warning generated.


vim +/ret +3881 drivers/clk/clk.c

  3874	
  3875	static void
  3876	__clk_register_critical_clock(struct device_node *np, struct clk_core *core,
  3877				      struct clk_hw *hw)
  3878	{
  3879		struct of_phandle_args clkspec;
  3880		u32 clksize, clktotal;
> 3881		int ret, i, index;
  3882	
  3883		if (!np)
  3884			return;
  3885	
  3886		if (!core->ops->match_clkspec)
  3887			return;
  3888	
  3889		if (of_property_read_u32(np, "#clock-cells", &clksize))
  3890			return;
  3891	
  3892		/* Clock node with #clock-cells = <0> uses critical-clocks; */
  3893		if (clksize == 0) {
  3894			if (of_property_read_bool(np, "critical-clocks") &&
  3895			    !core->ops->match_clkspec(hw, &clkspec))
  3896				core->flags |= CLK_IS_CRITICAL;
  3897			return;
  3898		}
  3899	
  3900		clkspec.np = np;
  3901		clktotal = of_property_count_u32_elems(np, "critical-clocks");
  3902		clktotal /= clksize;
  3903		for (index = 0; index < clktotal; index++) {
  3904			for (i = 0; i < clksize; i++) {
  3905				ret = of_property_read_u32_index(np, "critical-clocks",
  3906								 (index * clksize) + i,
  3907								 &(clkspec.args[i]));
  3908			}
  3909			if (!core->ops->match_clkspec(hw, &clkspec))
  3910				core->flags |= CLK_IS_CRITICAL;
  3911		}
  3912	}
  3913	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Vaittinen, Matti Feb. 16, 2022, 12:06 p.m. UTC | #3

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 <[e-mail]>
> Cc: Matti Vaittinen <[e-mail]>
> Cc: Michael Turquette <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> To: [e-mail]
> ---
>   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

Marek Vasut Feb. 16, 2022, 4:52 p.m. UTC | #4

On 2/16/22 13:06, Vaittinen, Matti wrote:

Hi,

[...]

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

If the driver implements match_clkspec() callback, then the callback 
gets used here to determine whether the clock match this clkspec.

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

Yes, I think we can omit the match_clkspec call for clock controllers 
with clock-cells == 0 altogether.

Vaittinen, Matti Feb. 17, 2022, 5:01 a.m. UTC | #5

On 2/16/22 18:52, Marek Vasut wrote:
> On 2/16/22 13:06, Vaittinen, Matti wrote:
> 
> Hi,
> 
> [...]
> 
>>> 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.
> 
> If the driver implements match_clkspec() callback, then the callback 
> gets used here to determine whether the clock match this clkspec.

/me feels _utterly_ stupid.

Of course :) I somehow completely misread the code. Sorry for the noise!

>> 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?
> 
> Yes, I think we can omit the match_clkspec call for clock controllers 
> with clock-cells == 0 altogether.

That would mean you could probably drop the bd718x7 driver patch, right?


Best Regards
	-- Matti

Marek Vasut Feb. 17, 2022, 1:43 p.m. UTC | #6

[...]

>>> 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?
>>
>> Yes, I think we can omit the match_clkspec call for clock controllers
>> with clock-cells == 0 altogether.
> 
> That would mean you could probably drop the bd718x7 driver patch, right?

Yes

I'm just waiting for feedback from Stephen on 2/3, then I'll send V2.

Stephen Boyd Feb. 17, 2022, 10:23 p.m. UTC | #7

Quoting Marek Vasut (2022-02-15 00:44:11)
> 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.

I don't see any mention of of_clk_detect_critical() here. We don't want
to enshrine the critical clk flag in DT. There was a bunch of discussion
about this on the mailing list years ago and the end result was this
instantly deprecated function to set the flag based on a DT property.
That thread isn't mentioned here either.

I see that there isn't any more 'clock-critical' in the kernel's dts so
I wonder if we would be able to get rid of that function or at least
hollow it out and see if anyone complains. Either way, what is the
actual problem trying to be solved? If the crystal oscillator isn't used
anywhere in the kernel why are we registering it with the clk framework?

Marek Vasut Feb. 21, 2022, 12:58 a.m. UTC | #8

On 2/17/22 23:23, Stephen Boyd wrote:
> Quoting Marek Vasut (2022-02-15 00:44:11)
>> 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.
> 
> I don't see any mention of of_clk_detect_critical() here. We don't want
> to enshrine the critical clk flag in DT. There was a bunch of discussion
> about this on the mailing list years ago and the end result was this
> instantly deprecated function to set the flag based on a DT property.
> That thread isn't mentioned here either.

I wasn't aware of clock-critical DT prop, but it seems deprecated and 
not generic enough anyway.

> I see that there isn't any more 'clock-critical' in the kernel's dts so
> I wonder if we would be able to get rid of that function or at least
> hollow it out and see if anyone complains. Either way, what is the
> actual problem trying to be solved? If the crystal oscillator isn't used
> anywhere in the kernel why are we registering it with the clk framework?

The problem is the other way around -- the SoC clock IPs often have a 
couple of general purpose clock routed to various SoC IO pins, those 
clock can be used for any purpose, and those are already registered with 
kernel clock framework. Some devices save on BoM and use those general 
purpose clock to supply clock networks which are otherwise not 
interacting with the kernel, like some CPLD for example. Since from the 
kernel point of view, those clock are unused, the kernel can turn those 
clock OFF and that will make the entire device fail.

So this critical-clocks property permits marking clock which must not 
ever be turned OFF accordingly.

[...]

Marek Vasut March 9, 2022, 8:54 p.m. UTC | #9

On 2/21/22 01:58, Marek Vasut wrote:
> On 2/17/22 23:23, Stephen Boyd wrote:
>> Quoting Marek Vasut (2022-02-15 00:44:11)
>>> 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.
>>
>> I don't see any mention of of_clk_detect_critical() here. We don't want
>> to enshrine the critical clk flag in DT. There was a bunch of discussion
>> about this on the mailing list years ago and the end result was this
>> instantly deprecated function to set the flag based on a DT property.
>> That thread isn't mentioned here either.
> 
> I wasn't aware of clock-critical DT prop, but it seems deprecated and 
> not generic enough anyway.
> 
>> I see that there isn't any more 'clock-critical' in the kernel's dts so
>> I wonder if we would be able to get rid of that function or at least
>> hollow it out and see if anyone complains. Either way, what is the
>> actual problem trying to be solved? If the crystal oscillator isn't used
>> anywhere in the kernel why are we registering it with the clk framework?
> 
> The problem is the other way around -- the SoC clock IPs often have a 
> couple of general purpose clock routed to various SoC IO pins, those 
> clock can be used for any purpose, and those are already registered with 
> kernel clock framework. Some devices save on BoM and use those general 
> purpose clock to supply clock networks which are otherwise not 
> interacting with the kernel, like some CPLD for example. Since from the 
> kernel point of view, those clock are unused, the kernel can turn those 
> clock OFF and that will make the entire device fail.
> 
> So this critical-clocks property permits marking clock which must not 
> ever be turned OFF accordingly.

How can we proceed here ?

Stephen Boyd March 12, 2022, 5:04 a.m. UTC | #10

Quoting Marek Vasut (2022-03-09 12:54:35)
> On 2/21/22 01:58, Marek Vasut wrote:
> > On 2/17/22 23:23, Stephen Boyd wrote:
> > 
> >> I see that there isn't any more 'clock-critical' in the kernel's dts so
> >> I wonder if we would be able to get rid of that function or at least
> >> hollow it out and see if anyone complains. Either way, what is the
> >> actual problem trying to be solved? If the crystal oscillator isn't used
> >> anywhere in the kernel why are we registering it with the clk framework?
> > 
> > The problem is the other way around -- the SoC clock IPs often have a 
> > couple of general purpose clock routed to various SoC IO pins, those 
> > clock can be used for any purpose, and those are already registered with 
> > kernel clock framework. Some devices save on BoM and use those general 
> > purpose clock to supply clock networks which are otherwise not 
> > interacting with the kernel, like some CPLD for example. Since from the 
> > kernel point of view, those clock are unused, the kernel can turn those 
> > clock OFF and that will make the entire device fail.
> > 
> > So this critical-clocks property permits marking clock which must not 
> > ever be turned OFF accordingly.
> 
> How can we proceed here ?

Why are we registering the clks with the framework on device that are
saving on BoM and using them outside of the kernel. What is the use of
kernel memory for struct clk_core that aren't ever used?

Marek Vasut March 12, 2022, 10:26 a.m. UTC | #11

On 3/12/22 06:04, Stephen Boyd wrote:
> Quoting Marek Vasut (2022-03-09 12:54:35)
>> On 2/21/22 01:58, Marek Vasut wrote:
>>> On 2/17/22 23:23, Stephen Boyd wrote:
>>>
>>>> I see that there isn't any more 'clock-critical' in the kernel's dts so
>>>> I wonder if we would be able to get rid of that function or at least
>>>> hollow it out and see if anyone complains. Either way, what is the
>>>> actual problem trying to be solved? If the crystal oscillator isn't used
>>>> anywhere in the kernel why are we registering it with the clk framework?
>>>
>>> The problem is the other way around -- the SoC clock IPs often have a
>>> couple of general purpose clock routed to various SoC IO pins, those
>>> clock can be used for any purpose, and those are already registered with
>>> kernel clock framework. Some devices save on BoM and use those general
>>> purpose clock to supply clock networks which are otherwise not
>>> interacting with the kernel, like some CPLD for example. Since from the
>>> kernel point of view, those clock are unused, the kernel can turn those
>>> clock OFF and that will make the entire device fail.
>>>
>>> So this critical-clocks property permits marking clock which must not
>>> ever be turned OFF accordingly.
>>
>> How can we proceed here ?
> 
> Why are we registering the clks with the framework on device that are
> saving on BoM and using them outside of the kernel. What is the use of
> kernel memory for struct clk_core that aren't ever used?

Those clock may be used to supply a device in DT on another hardware 
using the same SoC.

Take e.g. this random git grep result:

arch/arm/boot/dts/imx7d-remarkable2.dts
/ {
   wifi_pwrseq {
     ...
     clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
     ...
   };
};

This IMX7D_CLKO2_ROOT_DIV is one such general purpose clock output. In 
the aforementioned case, it is used to supply 32 kHz clock to a WiFi 
chip, i.e. it has a consumer in DT. These clock are registered by the 
platform clock driver:

drivers/clk/imx/clk-imx7d.c

But those clock can also be used to supply e.g. CPLD which has no other 
connection to the SoC but the clock. That is where it needs this 
critical-clocks property. Because then there is no consumer in DT. So 
the kernel will now think the clock are not used and will turn them off 
after boot, thus e.g. crashing such platform.

So in the later case, the DT would contain the following to avoid the crash:
&clks {
   critical-clocks = <IMX7D_CLKO2_ROOT_DIV>;
};

Stephen Boyd March 15, 2022, 11:52 p.m. UTC | #12

Quoting Marek Vasut (2022-03-12 02:26:17)
> On 3/12/22 06:04, Stephen Boyd wrote:
> > Quoting Marek Vasut (2022-03-09 12:54:35)
> >> On 2/21/22 01:58, Marek Vasut wrote:
> >>> On 2/17/22 23:23, Stephen Boyd wrote:
> >>>
> >>>> I see that there isn't any more 'clock-critical' in the kernel's dts so
> >>>> I wonder if we would be able to get rid of that function or at least
> >>>> hollow it out and see if anyone complains. Either way, what is the
> >>>> actual problem trying to be solved? If the crystal oscillator isn't used
> >>>> anywhere in the kernel why are we registering it with the clk framework?
> >>>
> >>> The problem is the other way around -- the SoC clock IPs often have a
> >>> couple of general purpose clock routed to various SoC IO pins, those
> >>> clock can be used for any purpose, and those are already registered with
> >>> kernel clock framework. Some devices save on BoM and use those general
> >>> purpose clock to supply clock networks which are otherwise not
> >>> interacting with the kernel, like some CPLD for example. Since from the
> >>> kernel point of view, those clock are unused, the kernel can turn those
> >>> clock OFF and that will make the entire device fail.
> >>>
> >>> So this critical-clocks property permits marking clock which must not
> >>> ever be turned OFF accordingly.
> >>
> >> How can we proceed here ?
> > 
> > Why are we registering the clks with the framework on device that are
> > saving on BoM and using them outside of the kernel. What is the use of
> > kernel memory for struct clk_core that aren't ever used?
> 
> Those clock may be used to supply a device in DT on another hardware 
> using the same SoC.
> 
> Take e.g. this random git grep result:
> 
> arch/arm/boot/dts/imx7d-remarkable2.dts
> / {
>    wifi_pwrseq {
>      ...
>      clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
>      ...
>    };
> };
> 
> This IMX7D_CLKO2_ROOT_DIV is one such general purpose clock output. In 
> the aforementioned case, it is used to supply 32 kHz clock to a WiFi 
> chip, i.e. it has a consumer in DT. These clock are registered by the 
> platform clock driver:
> 
> drivers/clk/imx/clk-imx7d.c
> 
> But those clock can also be used to supply e.g. CPLD which has no other 
> connection to the SoC but the clock. That is where it needs this 
> critical-clocks property. Because then there is no consumer in DT. So 
> the kernel will now think the clock are not used and will turn them off 
> after boot, thus e.g. crashing such platform.
> 
> So in the later case, the DT would contain the following to avoid the crash:
> &clks {
>    critical-clocks = <IMX7D_CLKO2_ROOT_DIV>;
> };

Got it. Why, in the latter case, would we register the clk with the clk
framework? I can see that they're "critical" in the sense that there's
no consumer node in DT and we want to make sure that nothing turns it
off. But it's also wasteful to even register the clk with the kernel
because no device is using it. It feels like we need a property like
'clock-dont-register' which is very simiilar to 'protected-clocks'.
There's already a binding for 'protected-clocks' so maybe that should be
reused and the definition of what the property means can be flexible to
handle the various use cases. The cases would be first this one here
where a clock doesn't matter because nobody uses it and second how it is
used on qualcomm SoCs where they have blocked access to certain clk
registers in the firmware so that the system crashes if we try to
read/write those clk registers.

The dt-binding can be reworded as "the OS shouldn't use these clks" and
then the implementation can skip registering those clks with the
framework.

Marek Vasut March 16, 2022, 11:30 a.m. UTC | #13

On 3/16/22 00:52, Stephen Boyd wrote:
> Quoting Marek Vasut (2022-03-12 02:26:17)
>> On 3/12/22 06:04, Stephen Boyd wrote:
>>> Quoting Marek Vasut (2022-03-09 12:54:35)
>>>> On 2/21/22 01:58, Marek Vasut wrote:
>>>>> On 2/17/22 23:23, Stephen Boyd wrote:
>>>>>
>>>>>> I see that there isn't any more 'clock-critical' in the kernel's dts so
>>>>>> I wonder if we would be able to get rid of that function or at least
>>>>>> hollow it out and see if anyone complains. Either way, what is the
>>>>>> actual problem trying to be solved? If the crystal oscillator isn't used
>>>>>> anywhere in the kernel why are we registering it with the clk framework?
>>>>>
>>>>> The problem is the other way around -- the SoC clock IPs often have a
>>>>> couple of general purpose clock routed to various SoC IO pins, those
>>>>> clock can be used for any purpose, and those are already registered with
>>>>> kernel clock framework. Some devices save on BoM and use those general
>>>>> purpose clock to supply clock networks which are otherwise not
>>>>> interacting with the kernel, like some CPLD for example. Since from the
>>>>> kernel point of view, those clock are unused, the kernel can turn those
>>>>> clock OFF and that will make the entire device fail.
>>>>>
>>>>> So this critical-clocks property permits marking clock which must not
>>>>> ever be turned OFF accordingly.
>>>>
>>>> How can we proceed here ?
>>>
>>> Why are we registering the clks with the framework on device that are
>>> saving on BoM and using them outside of the kernel. What is the use of
>>> kernel memory for struct clk_core that aren't ever used?
>>
>> Those clock may be used to supply a device in DT on another hardware
>> using the same SoC.
>>
>> Take e.g. this random git grep result:
>>
>> arch/arm/boot/dts/imx7d-remarkable2.dts
>> / {
>>     wifi_pwrseq {
>>       ...
>>       clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
>>       ...
>>     };
>> };
>>
>> This IMX7D_CLKO2_ROOT_DIV is one such general purpose clock output. In
>> the aforementioned case, it is used to supply 32 kHz clock to a WiFi
>> chip, i.e. it has a consumer in DT. These clock are registered by the
>> platform clock driver:
>>
>> drivers/clk/imx/clk-imx7d.c
>>
>> But those clock can also be used to supply e.g. CPLD which has no other
>> connection to the SoC but the clock. That is where it needs this
>> critical-clocks property. Because then there is no consumer in DT. So
>> the kernel will now think the clock are not used and will turn them off
>> after boot, thus e.g. crashing such platform.
>>
>> So in the later case, the DT would contain the following to avoid the crash:
>> &clks {
>>     critical-clocks = <IMX7D_CLKO2_ROOT_DIV>;
>> };
> 
> Got it. Why, in the latter case, would we register the clk with the clk
> framework?

Because those clock may be both critical and have other consumers which 
can be fully described in DT, i.e. a combination of the two 
aforementioned use cases.

The CLK_IS_CRITICAL flag does not imply the clock can only supply single 
device, rather the CLK_IS_CRITICAL flag indicates the clock must not 
ever be turned off. The clock can still supply multiple devices, some of 
them described in DT, some of them not.

If you were to unregister the clock from clock framework if they are 
critical, you wouldn't be able to handle the aforementioned use case.

> I can see that they're "critical" in the sense that there's
> no consumer node in DT and we want to make sure that nothing turns it
> off.

There may be other consumers in DT, we _only_ want to make sure the 
clock are never turned off, ever.

The "no consumers in DT" and "never turn clock off" are orthogonal.

> But it's also wasteful to even register the clk with the kernel
> because no device is using it. It feels like we need a property like
> 'clock-dont-register' which is very simiilar to 'protected-clocks'.
> There's already a binding for 'protected-clocks' so maybe that should be
> reused and the definition of what the property means can be flexible to
> handle the various use cases. The cases would be first this one here
> where a clock doesn't matter because nobody uses it and second how it is
> used on qualcomm SoCs where they have blocked access to certain clk
> registers in the firmware so that the system crashes if we try to
> read/write those clk registers.
> 
> The dt-binding can be reworded as "the OS shouldn't use these clks" and
> then the implementation can skip registering those clks with the
> framework.

See above, I don't think not registering the critical clock is the right 
approach.

Marek Vasut May 3, 2022, 7:17 p.m. UTC | #14

On 3/16/22 12:30, Marek Vasut wrote:
> On 3/16/22 00:52, Stephen Boyd wrote:
>> Quoting Marek Vasut (2022-03-12 02:26:17)
>>> On 3/12/22 06:04, Stephen Boyd wrote:
>>>> Quoting Marek Vasut (2022-03-09 12:54:35)
>>>>> On 2/21/22 01:58, Marek Vasut wrote:
>>>>>> On 2/17/22 23:23, Stephen Boyd wrote:
>>>>>>
>>>>>>> I see that there isn't any more 'clock-critical' in the kernel's 
>>>>>>> dts so
>>>>>>> I wonder if we would be able to get rid of that function or at least
>>>>>>> hollow it out and see if anyone complains. Either way, what is the
>>>>>>> actual problem trying to be solved? If the crystal oscillator 
>>>>>>> isn't used
>>>>>>> anywhere in the kernel why are we registering it with the clk 
>>>>>>> framework?
>>>>>>
>>>>>> The problem is the other way around -- the SoC clock IPs often have a
>>>>>> couple of general purpose clock routed to various SoC IO pins, those
>>>>>> clock can be used for any purpose, and those are already 
>>>>>> registered with
>>>>>> kernel clock framework. Some devices save on BoM and use those 
>>>>>> general
>>>>>> purpose clock to supply clock networks which are otherwise not
>>>>>> interacting with the kernel, like some CPLD for example. Since 
>>>>>> from the
>>>>>> kernel point of view, those clock are unused, the kernel can turn 
>>>>>> those
>>>>>> clock OFF and that will make the entire device fail.
>>>>>>
>>>>>> So this critical-clocks property permits marking clock which must not
>>>>>> ever be turned OFF accordingly.
>>>>>
>>>>> How can we proceed here ?
>>>>
>>>> Why are we registering the clks with the framework on device that are
>>>> saving on BoM and using them outside of the kernel. What is the use of
>>>> kernel memory for struct clk_core that aren't ever used?
>>>
>>> Those clock may be used to supply a device in DT on another hardware
>>> using the same SoC.
>>>
>>> Take e.g. this random git grep result:
>>>
>>> arch/arm/boot/dts/imx7d-remarkable2.dts
>>> / {
>>>     wifi_pwrseq {
>>>       ...
>>>       clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
>>>       ...
>>>     };
>>> };
>>>
>>> This IMX7D_CLKO2_ROOT_DIV is one such general purpose clock output. In
>>> the aforementioned case, it is used to supply 32 kHz clock to a WiFi
>>> chip, i.e. it has a consumer in DT. These clock are registered by the
>>> platform clock driver:
>>>
>>> drivers/clk/imx/clk-imx7d.c
>>>
>>> But those clock can also be used to supply e.g. CPLD which has no other
>>> connection to the SoC but the clock. That is where it needs this
>>> critical-clocks property. Because then there is no consumer in DT. So
>>> the kernel will now think the clock are not used and will turn them off
>>> after boot, thus e.g. crashing such platform.
>>>
>>> So in the later case, the DT would contain the following to avoid the 
>>> crash:
>>> &clks {
>>>     critical-clocks = <IMX7D_CLKO2_ROOT_DIV>;
>>> };
>>
>> Got it. Why, in the latter case, would we register the clk with the clk
>> framework?
> 
> Because those clock may be both critical and have other consumers which 
> can be fully described in DT, i.e. a combination of the two 
> aforementioned use cases.
> 
> The CLK_IS_CRITICAL flag does not imply the clock can only supply single 
> device, rather the CLK_IS_CRITICAL flag indicates the clock must not 
> ever be turned off. The clock can still supply multiple devices, some of 
> them described in DT, some of them not.
> 
> If you were to unregister the clock from clock framework if they are 
> critical, you wouldn't be able to handle the aforementioned use case.
> 
>> I can see that they're "critical" in the sense that there's
>> no consumer node in DT and we want to make sure that nothing turns it
>> off.
> 
> There may be other consumers in DT, we _only_ want to make sure the 
> clock are never turned off, ever.
> 
> The "no consumers in DT" and "never turn clock off" are orthogonal.
> 
>> But it's also wasteful to even register the clk with the kernel
>> because no device is using it. It feels like we need a property like
>> 'clock-dont-register' which is very simiilar to 'protected-clocks'.
>> There's already a binding for 'protected-clocks' so maybe that should be
>> reused and the definition of what the property means can be flexible to
>> handle the various use cases. The cases would be first this one here
>> where a clock doesn't matter because nobody uses it and second how it is
>> used on qualcomm SoCs where they have blocked access to certain clk
>> registers in the firmware so that the system crashes if we try to
>> read/write those clk registers.
>>
>> The dt-binding can be reworded as "the OS shouldn't use these clks" and
>> then the implementation can skip registering those clks with the
>> framework.
> 
> See above, I don't think not registering the critical clock is the right 
> approach.

It has been another month and half, I got no further feedback here. I 
sent V2 with further updated commit message, got no feedback either. I 
re-sent V2 and got no feedback either.

How can we proceed ?

Patch

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))
+			core->flags |= CLK_IS_CRITICAL;
+		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 (!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)
 {
@@ -3916,6 +3955,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(np, 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 2faa6f7aa8a87..8bc0eedfeb2fd 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);
 };
 
 /**