On Wed 10 Nov 05:15 PST 2021, Shawn Guo wrote: > Hi Stephan, > > On Tue, Nov 09, 2021 at 11:26:21AM +0100, Stephan Gerhold wrote: > > Hi Shawn, > > > > On Tue, Nov 09, 2021 at 10:25:55AM +0800, Shawn Guo wrote: > > > Currently the enable state of smd-rpm clocks are not properly reported > > > back to framework due to missing .is_enabled and .is_prepared hooks. > > > This causes a couple of issues. > > > > > > - All those unused clocks are not voted for off, because framework has > > > no knowledge that they are unused. It becomes a problem for vlow > > > power mode support, as we do not have every single RPM clock claimed > > > and voted for off by client devices, and rely on clock framework to > > > disable those unused RPM clocks. > > > > > > > I posted a similar patch a bit more than a year ago [1]. > > Ouch, that's unfortunate! If your patch landed, I wouldn't have had to > spend such a long time to figure out why my platform fails to reach vlow > power mode :( > > > Back then one > > of the concerns was that we might disable critical clocks just because > > they have no driver using it actively. For example, not all of the > > platforms using clk-smd-rpm already have an interconnect driver. > > Disabling the interconnect related clocks will almost certainly make the > > device lock up completely. (I tried it back then, it definitely does...) > > > > I proposed adding CLK_IGNORE_UNUSED for the interconnect related clocks > > back then [2] which would allow disabling most of the clocks at least. > > Stephen Boyd had an alternative proposal to instead move the > > interconnect related clocks completely out of clk-smd-rpm [3]. > > But I'm still unsure how this would work in a backwards compatible way. [4] > > > > Since your patches are more or less identical I'm afraid the same > > concerns still need to be solved somehow. :) > > I do not really understand why smd-rpm clock driver needs to be a special > case. This is a very common issue, mostly in device early support phase > where not all clock consumer drivers are ready. Flag CLK_IGNORE_UNUSED > and kernel cmdline 'clk_ignore_unused' are created just for that. Those > "broken" platforms should be booted with 'clk_ignore_unused' until they > have related consumer drivers in place. Afaict we still have the problem that if the interconnect driver is compiled as a module, or for other reasons doesn't probe until after late_initcall() clk-smd-rpm will turn off these clocks and we never will get a chance to probe the interconnect provider. I believe the way to handle that is to rely on sync_state, but there seems to be a lot of corner cases here. But with that in place, I agree that we should handle this temporarily during bringup by the use of clk_ignore_unused. > IMHO, properly reporting enable state to framework is definitely the > right thing to do, and should have been done from day one. > I always thought is_enabled() should reflect the hardware state - in particular for clk_summary. The particular concern being that by initializing the is_enabled() state to either true or false, we're making an assumption about the hardware state. And if something where to do if (enabled) disable (or if (disabled) enable), we might skip a critical operation just because we tricked the logic. So, do you need it for anything other than clk_disable_unused()? I have a clock in the MDP with similar issue, where we don't have is_enabled() but I need it to be disabled by clk_disable_unused(), because the next iteration turns off the parent and locks up the still "active" rcg. So far I've not received any feedback on this though... https://lore.kernel.org/all/20210707043859.195870-1-bjorn.andersson@xxxxxxxxxx/ With this approach we don't make any assumptions about the hardware state, beyond the fact that we will issue a disable in clk_disable_unused() if no one has yet enabled the clock - which at worst turns off a clock that's already is off. Regards, Bjorn > Shawn > > > [1]: https://lore.kernel.org/linux-arm-msm/20200817140908.185976-1-stephan@xxxxxxxxxxx/ > > [2]: https://lore.kernel.org/linux-arm-msm/20200818080738.GA46574@xxxxxxxxxxx/ > > [3]: https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@xxxxxxxxxxxxxxxxxxxxxxxxxx/ > > [4]: https://lore.kernel.org/linux-arm-msm/20200821064857.GA905@xxxxxxxxxxx/
