| Message ID | 20220913102141.971148-1-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Propose critical clocks | expand |
+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 > > >
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 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 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(+) >
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(+) > > >
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 > > >
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/
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/
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/ > >
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/ >
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/