Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <a6f0c714-37c7-4df6-96d6-e070a1c75fc3@linaro.org>
Date:   Tue, 10 Oct 2023 23:22:47 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] clk: qcom: smd: Disable unused clocks
Content-Language: en-US
To:     Stephan Gerhold <stephan@gerhold.net>
Cc:     Stephan Gerhold <stephan.gerhold@kernkonzept.com>,
        Bjorn Andersson <andersson@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>, linux-arm-msm@vger.kernel.org,
        linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20231004-clk-qcom-smd-rpm-unused-v2-1-9a5281f324dc@kernkonzept.com>
 <bc8fa799-aa64-4b69-97ce-8f1872c8eb11@linaro.org>
 <ZSRfc_w19h-55Bib@gerhold.net>
 <da02414c-a151-464b-8976-d353c6da7b8e@linaro.org>
 <ZSXAY5mbXB7Gbz2x@gerhold.net>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <ZSXAY5mbXB7Gbz2x@gerhold.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/10/23 23:21, Stephan Gerhold wrote:
> On Tue, Oct 10, 2023 at 10:45:15PM +0200, Konrad Dybcio wrote:
>> 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.
>> """
>>
> 
> I see, thanks! I think you should see at least the "prepared" state with
> this patch. I'm not entirely convinced we should implement .is_enabled()
> if we don't actually do anything on .enable()/.disable().
> 
> Anyway, given that the debugfs state is not directly related to the main
> objective of disabling unused clocks I think that would be better
> discussed in a separate patch later. :)
Agreed

Konrad