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

[Konrad Dybcio <konrad.dybcio@linaro.org>]

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