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/[email protected]/
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
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
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
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/[email protected]/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564
Thanks,
Peng.
>
> [1] https://lore.kernel.org/linux-clk/[email protected]/
>
> 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(+)
>
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/[email protected]/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564
>
> Thanks,
> Peng.
>
>
> >
> > [1] https://lore.kernel.org/linux-clk/[email protected]/
> >
> > 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(+)
> >
>
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/[email protected]/
>
> 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
>
>
>
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/[email protected]/
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/[email protected]/
> 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/[email protected]/
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/[email protected]/
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/[email protected]/
>
>
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/[email protected]/
>
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/[email protected]/
[3] https://lore.kernel.org/all/20150422093446.GA28007@lukather/
[4] https://lore.kernel.org/all/20150811091146.GB13374@x1/