[RFC PATCH 0/2] Propose critical clocks

[RFC PATCH 0/2] Propose critical clocks

[Marco Felsch]

Hi,

this proposal is to mark clocks as critical. It is somehow inspired by
the regulator-always-on property. Since sometimes we can end in circular
dependcies if we wanna solve the dependcies for a specific clock
provider.

The property is generic so it can be used by every hw clock provider. So
it can be seen as generic implementation to [1].

[1] https://lore.kernel.org/linux-clk/20220913092136.1706263-1-peng.fan@oss.nxp.com/

Marco Felsch (2):
  clk: add support for critical always-on clocks
  arm64: dts: imx8mm-evk: mark 32k pmic clock as always-on

 arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi |  1 +
 drivers/clk/clk.c                             | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 1/2] clk: add support for critical always-on clocks

[Marco Felsch]

Add support to mark specific clocks as always-on (critical), like the
regulator-alawys-on property. So the platform integrators can specify
the property on a per device basis by specifying it within the firmware
and not only within the driver. Unlike the regulator framework the clock
framework uses a 1:n matching, which means 1 firmware node can provide
up to n clock providers. Therefore the binding uses a string-array so we
can specify n clock providers as critical.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/clk/clk.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7fc191c15507..c3651bf96f90 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3825,6 +3825,22 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
 	return 0;
 }
 
+static void clk_core_check_critical(struct clk_core *core)
+{
+	struct fwnode_handle *fwnode = of_fwnode_handle(core->of_node);
+	const char *prop = "clocks-always-on";
+	int ret;
+
+	/* Very early added clocks don't have a fwnode */
+	if (!fwnode || !fwnode_property_present(fwnode, prop))
+		return;
+
+	/* Mark clock as critical if listed within the clocks-always-on array */
+	ret = fwnode_property_match_string(fwnode, prop, core->name);
+	if (!ret)
+		core->flags |= CLK_IS_CRITICAL;
+}
+
 static int clk_core_populate_parent_map(struct clk_core *core,
 					const struct clk_init_data *init)
 {
@@ -3946,6 +3962,8 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 	core->min_rate = 0;
 	core->max_rate = ULONG_MAX;
 
+	clk_core_check_critical(core);
+
 	ret = clk_core_populate_parent_map(core, init);
 	if (ret)
 		goto fail_parents;
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 2/2] arm64: dts: imx8mm-evk: mark 32k pmic clock as always-on

[Marco Felsch]

This clock is critical for the system since it supplies the 32k SoC
clock. Unfortunately the imx8mm.dtsi uses a fixed clock provider for the
32k SoC clock and not this one. If it would use this clock we would add
a cycle-dependency since the pmic driver depends on the i2c driver which
depends on the clock driver. Therefore use the new "clocks-always-on"
macro to mark the clock as critical, so it is never turned off by the
kernel.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
index 7d6317d95b13..0e950ef61900 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
@@ -195,6 +195,7 @@ pmic@4b {
 		#clock-cells = <0>;
 		clocks = <&osc_32k 0>;
 		clock-output-names = "clk-32k-out";
+		clocks-always-on = "clk-32k-out";
 
 		regulators {
 			buck1_reg: BUCK1 {
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Marco Felsch]

+Cc Peng (OSS)

On 22-09-13, Marco Felsch wrote:
> Hi,
> 
> this proposal is to mark clocks as critical. It is somehow inspired by
> the regulator-always-on property. Since sometimes we can end in circular
> dependcies if we wanna solve the dependcies for a specific clock
> provider.
> 
> The property is generic so it can be used by every hw clock provider. So
> it can be seen as generic implementation to [1].
> 
> [1] https://lore.kernel.org/linux-clk/20220913092136.1706263-1-peng.fan@oss.nxp.com/
> 
> Marco Felsch (2):
>   clk: add support for critical always-on clocks
>   arm64: dts: imx8mm-evk: mark 32k pmic clock as always-on
> 
>  arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi |  1 +
>  drivers/clk/clk.c                             | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> -- 
> 2.30.2
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Krzysztof Kozlowski]

On 13/09/2022 12:21, Marco Felsch wrote:
> Hi,
> 
> this proposal is to mark clocks as critical. It is somehow inspired by
> the regulator-always-on property. Since sometimes we can end in circular
> dependcies if we wanna solve the dependcies for a specific clock
> provider.
> 
> The property is generic so it can be used by every hw clock provider. So
> it can be seen as generic implementation to [1].

Missing devicetree list (so no testing), missing bindings. Please follow
Linux process, run checkpatch and CC necessary people an dlists pointed
out by get_maintainers.pl.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Marco Felsch]

On 22-09-13, Krzysztof Kozlowski wrote:
> On 13/09/2022 12:21, Marco Felsch wrote:
> > Hi,
> > 
> > this proposal is to mark clocks as critical. It is somehow inspired by
> > the regulator-always-on property. Since sometimes we can end in circular
> > dependcies if we wanna solve the dependcies for a specific clock
> > provider.
> > 
> > The property is generic so it can be used by every hw clock provider. So
> > it can be seen as generic implementation to [1].
> 
> Missing devicetree list (so no testing), missing bindings. Please follow
> Linux process, run checkpatch and CC necessary people an dlists pointed
> out by get_maintainers.pl.

I just pushd the _proposal_ as reaction of adding a NXP specific
binding. If the clk maintainer(s) would agree to this this proposal I
would update the patchset with bindings and _all_ mail-addresses.

Regards,
  Marco

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Peng Fan]

On 9/13/2022 6:21 PM, Marco Felsch wrote:
> Hi,
> 
> this proposal is to mark clocks as critical. It is somehow inspired by
> the regulator-always-on property. Since sometimes we can end in circular
> dependcies if we wanna solve the dependcies for a specific clock
> provider.
> 
> The property is generic so it can be used by every hw clock provider. So
> it can be seen as generic implementation to [1].

Thanks for working on a generic solution, I think your proposal could 
also help [1] and try to resolve same issue as [2].


[1] 
https://lore.kernel.org/all/CAJ+vNU1Za2CPGVX3q4HKufsxbL5zRrk1B5CWFpKritetrTs4dA@mail.gmail.com/
[2] 
https://lore.kernel.org/all/20220517235919.200375-1-marex@denx.de/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564

Thanks,
Peng.


> 
> [1] https://lore.kernel.org/linux-clk/20220913092136.1706263-1-peng.fan@oss.nxp.com/
> 
> Marco Felsch (2):
>    clk: add support for critical always-on clocks
>    arm64: dts: imx8mm-evk: mark 32k pmic clock as always-on
> 
>   arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi |  1 +
>   drivers/clk/clk.c                             | 18 ++++++++++++++++++
>   2 files changed, 19 insertions(+)
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Marco Felsch]

Hi Peng,

On 22-09-13, Peng Fan wrote:
> On 9/13/2022 6:21 PM, Marco Felsch wrote:
> > Hi,
> > 
> > this proposal is to mark clocks as critical. It is somehow inspired by
> > the regulator-always-on property. Since sometimes we can end in circular
> > dependcies if we wanna solve the dependcies for a specific clock
> > provider.
> > 
> > The property is generic so it can be used by every hw clock provider. So
> > it can be seen as generic implementation to [1].
> 
> Thanks for working on a generic solution, I think your proposal could also
> help [1] and try to resolve same issue as [2].

Didn't noticed that there was already a on going discussion on this
topic. Maybe a combination of my proposal and [2] is the solution, but I
have no idea if I my solution can work on a ID based array. Let's see
what the maintainers say.

> [1] https://lore.kernel.org/all/CAJ+vNU1Za2CPGVX3q4HKufsxbL5zRrk1B5CWFpKritetrTs4dA@mail.gmail.com/
> [2] https://lore.kernel.org/all/20220517235919.200375-1-marex@denx.de/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564
> 
> Thanks,
> Peng.
> 
> 
> > 
> > [1] https://lore.kernel.org/linux-clk/20220913092136.1706263-1-peng.fan@oss.nxp.com/
> > 
> > Marco Felsch (2):
> >    clk: add support for critical always-on clocks
> >    arm64: dts: imx8mm-evk: mark 32k pmic clock as always-on
> > 
> >   arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi |  1 +
> >   drivers/clk/clk.c                             | 18 ++++++++++++++++++
> >   2 files changed, 19 insertions(+)
> > 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Marco Felsch]

Hi Stephen, Michael,

I know it is a busy time right now, but maybe you have a few minutes for
this RFC. I know it is incomplete, but the interessting part is there
and it would fix a real issue we encountered on the imx8mm-evk's.

Regards,
  Marco

On 22-09-13, Marco Felsch wrote:
> Hi,
> 
> this proposal is to mark clocks as critical. It is somehow inspired by
> the regulator-always-on property. Since sometimes we can end in circular
> dependcies if we wanna solve the dependcies for a specific clock
> provider.
> 
> The property is generic so it can be used by every hw clock provider. So
> it can be seen as generic implementation to [1].
> 
> [1] https://lore.kernel.org/linux-clk/20220913092136.1706263-1-peng.fan@oss.nxp.com/
> 
> Marco Felsch (2):
>   clk: add support for critical always-on clocks
>   arm64: dts: imx8mm-evk: mark 32k pmic clock as always-on
> 
>  arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi |  1 +
>  drivers/clk/clk.c                             | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> -- 
> 2.30.2
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Stephen Boyd]

Quoting Marco Felsch (2022-10-05 01:23:48)
> Hi Stephen, Michael,
> 
> I know it is a busy time right now, but maybe you have a few minutes for
> this RFC. I know it is incomplete, but the interessting part is there
> and it would fix a real issue we encountered on the imx8mm-evk's.
> 

There's another approach by Marek[1]. Can you work together on a
solution? I think we should step away from trying to make the critical
flag work during clk registration, and turn on the clk during provider
registration instead. That hopefully makes it simpler. We can keep the
clk flag of course, so that the clk can't be turned off, but otherwise
we shouldn't need to make registration path check for the property.

[1] https://lore.kernel.org/all/20220924174517.458657-1-marex@denx.de/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Marek Vasut]

On 10/6/22 01:06, Stephen Boyd wrote:
> Quoting Marco Felsch (2022-10-05 01:23:48)
>> Hi Stephen, Michael,
>>
>> I know it is a busy time right now, but maybe you have a few minutes for
>> this RFC. I know it is incomplete, but the interessting part is there
>> and it would fix a real issue we encountered on the imx8mm-evk's.

The i.MX8M hang when using 32kHz supplied by PMIC is solved by modeling 
the clock in DT correctly, see:

https://lore.kernel.org/all/20220924174603.458956-1-marex@denx.de/

> There's another approach by Marek[1]. Can you work together on a
> solution? I think we should step away from trying to make the critical
> flag work during clk registration, and turn on the clk during provider
> registration instead.

So that would work like the qualcomm-specific 'protected-clock' property?

I really want to avoid such clock-driver specific hacks which are poorly 
or inconsistently supported. This critical-clock should be a generic 
solution and that should be in clock core.

> That hopefully makes it simpler. We can keep the
> clk flag of course, so that the clk can't be turned off, but otherwise
> we shouldn't need to make registration path check for the property.
> 
> [1] https://lore.kernel.org/all/20220924174517.458657-1-marex@denx.de/


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Marco Felsch]

Hi Marek,

sorry for the delay.

On 22-10-06, Marek Vasut wrote:
> On 10/6/22 01:06, Stephen Boyd wrote:
> > Quoting Marco Felsch (2022-10-05 01:23:48)
> > > Hi Stephen, Michael,
> > > 
> > > I know it is a busy time right now, but maybe you have a few minutes for
> > > this RFC. I know it is incomplete, but the interessting part is there
> > > and it would fix a real issue we encountered on the imx8mm-evk's.
> 
> The i.MX8M hang when using 32kHz supplied by PMIC is solved by modeling the
> clock in DT correctly, see:
> 
> https://lore.kernel.org/all/20220924174603.458956-1-marex@denx.de/

Thanks for this link, but I have a few doubts about your modeling. As
you already noted in the above link, the 32K osc is critical for some
reason not listed in the RM.

My doubts about your modeling are:
 - The snvs-rtc driver will be required no matter if it is used or not
 - According the i.MX8MM RM rev.3 11/2020: The SNVS is supplied by clock
   gate 71 (CCM_CCGR71). So this would be the clock provider for the
   SNVS modul. This clock gate is enabled by the ROM loader and the
   clock driver have no support for it yet. As soon as the clock driver
   have support for it your modeling will break since the clock
   gate gets turned off since you specify an other clock source.

With my solution the modeling is still correct and the user is not
enforced to enable driver for an _maybe_ unused modul. What do you
think?

Regards,
  Marco

> > There's another approach by Marek[1]. Can you work together on a
> > solution? I think we should step away from trying to make the critical
> > flag work during clk registration, and turn on the clk during provider
> > registration instead.
> 
> So that would work like the qualcomm-specific 'protected-clock' property?
> 
> I really want to avoid such clock-driver specific hacks which are poorly or
> inconsistently supported. This critical-clock should be a generic solution
> and that should be in clock core.
> 
> > That hopefully makes it simpler. We can keep the
> > clk flag of course, so that the clk can't be turned off, but otherwise
> > we shouldn't need to make registration path check for the property.
> > 
> > [1] https://lore.kernel.org/all/20220924174517.458657-1-marex@denx.de/
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Marco Felsch]

Hi Stephen,

sorry for the delay.

On 22-10-05, Stephen Boyd wrote:
> Quoting Marco Felsch (2022-10-05 01:23:48)
> > Hi Stephen, Michael,
> > 
> > I know it is a busy time right now, but maybe you have a few minutes for
> > this RFC. I know it is incomplete, but the interessting part is there
> > and it would fix a real issue we encountered on the imx8mm-evk's.
> > 
> 
> There's another approach by Marek[1]. Can you work together on a
> solution? I think we should step away from trying to make the critical
> flag work during clk registration, and turn on the clk during provider
> registration instead. That hopefully makes it simpler. We can keep the
> clk flag of course, so that the clk can't be turned off, but otherwise
> we shouldn't need to make registration path check for the property.

Can you please explain your idea a bit more in detail so I can follow
you. The whole idea of this patchset is to enable a clock and never turn
it off. According the clk-provider.h comment this is the exact use-case
for the CLK_IS_CRITICAL flag. For static clock provider tree's like
soc-clock tree's this can be done by the driver by setting the
CLK_IS_CRITICAL flag within the struct clk_init_data. Now the question
is how I can add such a handling to "dynamic" clock providers which are
added by system-designs e.g. an i2c-clock provider. Of course each I2C
clock provider driver can check the flag but I wanted to make it common
to all.

Regards,
  Marco


> 
> [1] https://lore.kernel.org/all/20220924174517.458657-1-marex@denx.de/
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/2] Propose critical clocks

[Stephen Boyd]

Quoting Marco Felsch (2023-01-17 09:55:53)
> Hi Stephen,
> 
> sorry for the delay.

Sorry for my delay as well. I dread this topic!

> 
> On 22-10-05, Stephen Boyd wrote:
> > Quoting Marco Felsch (2022-10-05 01:23:48)
> > > Hi Stephen, Michael,
> > > 
> > > I know it is a busy time right now, but maybe you have a few minutes for
> > > this RFC. I know it is incomplete, but the interessting part is there
> > > and it would fix a real issue we encountered on the imx8mm-evk's.
> > > 
> > 
> > There's another approach by Marek[1]. Can you work together on a
> > solution? I think we should step away from trying to make the critical
> > flag work during clk registration, and turn on the clk during provider
> > registration instead. That hopefully makes it simpler. We can keep the
> > clk flag of course, so that the clk can't be turned off, but otherwise
> > we shouldn't need to make registration path check for the property.
> 
> Can you please explain your idea a bit more in detail so I can follow
> you. The whole idea of this patchset is to enable a clock and never turn
> it off. According the clk-provider.h comment this is the exact use-case
> for the CLK_IS_CRITICAL flag. For static clock provider tree's like
> soc-clock tree's this can be done by the driver by setting the
> CLK_IS_CRITICAL flag within the struct clk_init_data. Now the question
> is how I can add such a handling to "dynamic" clock providers which are
> added by system-designs e.g. an i2c-clock provider. Of course each I2C
> clock provider driver can check the flag but I wanted to make it common
> to all.
> 

A long time ago we had a large debate about putting critical clock flag
into devicetree. During that time, we wanted an 'always-on' sort of
binding[1] that told the clk framework to turn on the clk and leave it
on forever. In fact, there was a binding and everything[2] but it didn't
get merged. I don't want to drag Maxime into this thread again, but we
need a summary of these old threads to make sure we're not falling into
the traps they describe[3].

I admit I don't want to spend my time re-reading these huge threads, but
I may have to because I don't recall all the details anymore about why
we were so opposed to critical clocks or always on clocks as a DT
binding. I think it was because we were concerned about abuse by DT
authors and getting stuck using old DTBs with newer kernels that have
drivers that want to start gating clks. I'm not sure that is a real
concern anymore though. If you have a driver that starts getting clks
that have been marked as always-on in the DT you would probably remove
them from the always-on list at the same time as adding the new node
that consumes those clocks.

Furthermore, back then I recall it was decided that the CLK_IS_CRITICAL
flag should only be set in software, so that it can be removed later on.
The fear was that we may have to live with old DTBs forever, so having a
property that said we must keep some clk enabled forever may run into
some problem. And the argument was that critical clks was a software
design of the Linux kernel, not something that DT cares about, so
putting a hint for that mechanism into the DT was wrong. I don't see how
this argument holds up when you have an external to the SoC clk (i.e.
not a memory controller or CPU clk) that needs to be always on in some
board designs and turned off in other board designs. The SoC clk driver
isn't going to know what to do with the external clk.

This is why I'm leaning towards avoiding the CLK_IS_CRITICAL flag and
implementing an "always-on" binding. The historical baggage with the
critical flag is too large to overcome. Writing that the clk is
always-on in the binding doesn't imply that it is "critical" as strongly
as having the word critical in the property name. It just means that the
clk is always on.

I kept reading the other thread[4] and I see that there was yet another
idea for a binding/feature that said "keep this clock on until a driver
claims it". I suspect we should implement that sort of logic for all
clks, so that we hand off the enable state from boot properly. This gets
into the sync_state stuff from Abel Vesa, and reworking disable unused
so that it doesn't get called early.

I hope this always-on property isn't being used to workaround the
disable unused logic. Instead, it should be used to indicate that some
clk must always be on and that child clks shouldn't be turning it on and
off when they turn themselves on and off. Show a real need for this, Cc
the folks involved in the 8 years ago discussions, and I think it will
work out.

BTW, this is similar to CLK_SET_RATE_PARENT, which we haven't put into
DT as a property. Eventually we modified the clk.h API to have rate
locking so that the flag could be set on clks but the parent rate would
be "locked" ensuring the frequency doesn't change when the child changes
rates. We may need to add a DT binding for rate/parent locking in the
future though if we want to make sure parents don't change or
frequencies don't change for a particular clk that is generated by some
i2c chip or some external to the SoC clk.

[1] https://lore.kernel.org/all/20151001195653.GL7104@lukather/
[2] https://lore.kernel.org/all/1428432239-4114-5-git-send-email-lee.jones@linaro.org/
[3] https://lore.kernel.org/all/20150422093446.GA28007@lukather/
[4] https://lore.kernel.org/all/20150811091146.GB13374@x1/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel