Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework

[Shawn Guo <shawn.guo@linaro.org>]

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.  IMHO, properly reporting enable
state to framework is definitely the right thing to do, and should have
been done from day one.

Shawn

> [1]: https://lore.kernel.org/linux-arm-msm/20200817140908.185976-1-stephan@gerhold.net/
> [2]: https://lore.kernel.org/linux-arm-msm/20200818080738.GA46574@gerhold.net/
> [3]: https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
> [4]: https://lore.kernel.org/linux-arm-msm/20200821064857.GA905@gerhold.net/