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

From: Konrad Dybcio
Date: Tue Oct 10 2023 - 16:45:31 EST




On 10/9/23 22:15, Stephan Gerhold wrote:
On Fri, Oct 06, 2023 at 11:08:39PM +0200, Konrad Dybcio wrote:
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.


I didn't notice any problems on 8916 and 8909 either. :-)

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


clk-smd-rpm doesn't have any .enable()/.disable() ops (only .prepare()
and .unprepare()) so I don't think is_enabled is needed. For the unused
clock cleanup in drivers/clk/clk.c (clk_disable_unused()) we just care
about the clk_unprepare_unused_subtree() part. That part is run when the
clock reports true in .is_prepared(). The equivalent for .is_enabled()
would just be a no-op because there are no .enable()/.disable() ops.
Oh I found out why :D

"""
The RPM clock enabling state can be found with 'enabled' in struct
clk_smd_rpm. Add .is_enabled hook so that clk_summary in debugfs
can a correct enabling state for RPM clocks.
"""

Konrad