On Wed 10 Nov 06:09 PST 2021, Stephan Gerhold wrote: > On Wed, Nov 10, 2021 at 09:15:11PM +0800, Shawn Guo wrote: > > On Tue, Nov 09, 2021 at 11:26:21AM +0100, Stephan Gerhold wrote: > > > 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 :( > > > > Sorry, I was waiting for Stephen to reply and eventually decided to > shift focus to other things first. :) > > The whole low-power topic is kind of frustrating on older platforms > because they currently still lack almost everything that is necessary to > reach those low power states. Even things that you already consider > natural for newer platforms (such as interconnect) are still very much > work in progress on all older ones. > > > > 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. IMHO, properly reporting enable > > state to framework is definitely the right thing to do, and should have > > been done from day one. > > > > ... And therefore I think we should be careful with such changes, > especially if they would prevent devices from booting completely. > Unfortunately the users trying to make use of old platforms are also > often the ones who might not be aware that they suddenly need > "clk_ignore_unused" just to boot a system that was previously working > (mostly) fine, except for the whole low-power topic. > > I fully agree with you that disabling the unused clocks here is the > right thing to do, but I think we should try to carefully flag the most > important clocks in the driver to avoid causing too many regressions. > I don't fancy the idea of forcing everyone to run with specific kernel command line parameters - in particular not as a means to avoid "regressions". I think the only way around this problem is to figure out how to move the clk disablement to sync_state - probably per clock driver. Regards, Bjorn
