Re: [PATCH v2] clk: qcom: smd: Disable unused clocks

[Konrad Dybcio]

On 4.10.2023 14:10, Stephan Gerhold wrote:
> At the moment, clk-smd-rpm forces all clocks on at probe time (for
> "handoff"). However, it does not make the clk core aware of that.
> 
> This means that the clocks stay enabled forever if they are not used
> by anything. We can easily disable them again after bootup has been
> completed, by making the clk core aware of the state. This is
> implemented by returning the current state of the clock in
> is_prepared().
> 
> Checking the SPMI clock registers reveals that this allows the RPM to
> disable unused BB/RF clocks. This reduces the power consumption quite
> significantly and is also needed to allow entering low-power states.
> 
> As of commit d6edc31f3a68 ("clk: qcom: smd-rpm: Separate out
> interconnect bus clocks") the interconnect-related clocks are no longer
> managed/exposed by clk-smd-rpm. Also the BI_TCXO_AO clock is now
> critical (and never disabled).
> 
> There is still a slight chance that this change will break boot on some
> devices. However, this will be most likely caused by actual mistakes in
> the device tree (where required clocks were not actually specified).
Precisely this, and solely as a consequence of the interconnect driver
not covering all the required clocks (usually named GCC_SOME_NOC_XYZ_CLK,
but there's quite a lot more).

For platforms without an interconnect driver, breaking stuff this **MOST
LIKELY** means that Linux uses some hw that isn't voted for (e.g. missing
crypto clock under scm or something).

For those with an interconnect driver, this will uncover issues that were
previously hidden because of the smd-rpm interconnect being essentially
broken for most of its existence. I can smell 660 breaking from however
many miles you are away from me, but it's "good", as we were relying on
(board specific) magic..

I've been carrying an equivalent patch in my tree for over half a year now
and IIRC 8996 was mostly fine. It's also a good idea to test suspend
(echo mem > /sys/power/state) and wakeup.

For reasons that I don't fully recall, I do have both .is_prepared and
.is_enabled though..

Konrad