diff mbox series

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

Message ID 20231004-clk-qcom-smd-rpm-unused-v2-1-9a5281f324dc@kernkonzept.com
State New
Headers
Series [v2] clk: qcom: smd: Disable unused clocks |

Commit Message

Stephan Gerhold Oct. 4, 2023, 12:10 p.m. UTC 
  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).

Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
---
Changes in v2:
- Rebase on latest qcom/for-next, update commit message with other recent
  related changes
- Link to v1: https://lore.kernel.org/linux-arm-msm/20200817140908.185976-1-stephan@gerhold.net/
---
Keeping all unused clocks on makes it very easy to forget to enable
actually required clocks. [1] is one example of that, where the crypto
engine worked fine without any clocks. IMHO we should try to get this
change in sooner than later to avoid introducing more new mistakes.

[1]: https://lore.kernel.org/linux-arm-msm/ZGdLCdSof027mk5u@gerhold.net/
---
 drivers/clk/qcom/clk-smd-rpm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)


---
base-commit: 870b5222204243ee6dbeada9ad1b90cda9ecb4da
change-id: 20231004-clk-qcom-smd-rpm-unused-b79d00122811

Best regards,

Comments

Konrad Dybcio Oct. 6, 2023, 9:08 p.m. UTC | #1 
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
Stephan Gerhold Oct. 9, 2023, 8:15 p.m. UTC | #2 
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.

Thanks,
Stephan
Konrad Dybcio Oct. 10, 2023, 8:45 p.m. UTC | #3 
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
Stephan Gerhold Oct. 10, 2023, 9:21 p.m. UTC | #4 
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. :)

Thanks,
Stephan
Konrad Dybcio Oct. 10, 2023, 9:22 p.m. UTC | #5 
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
diff mbox series

Patch

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 0191fc0dd7da..eba650ad7291 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -335,6 +335,13 @@  static void clk_smd_rpm_unprepare(struct clk_hw *hw)
 	mutex_unlock(&rpm_smd_clk_lock);
 }
 
+static int clk_smd_rpm_is_prepared(struct clk_hw *hw)
+{
+	struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
+
+	return r->enabled;
+}
+
 static int clk_smd_rpm_set_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long parent_rate)
 {
@@ -431,6 +438,7 @@  static int clk_smd_rpm_enable_scaling(void)
 static const struct clk_ops clk_smd_rpm_ops = {
 	.prepare	= clk_smd_rpm_prepare,
 	.unprepare	= clk_smd_rpm_unprepare,
+	.is_prepared	= clk_smd_rpm_is_prepared,
 	.set_rate	= clk_smd_rpm_set_rate,
 	.round_rate	= clk_smd_rpm_round_rate,
 	.recalc_rate	= clk_smd_rpm_recalc_rate,
@@ -439,6 +447,7 @@  static const struct clk_ops clk_smd_rpm_ops = {
 static const struct clk_ops clk_smd_rpm_branch_ops = {
 	.prepare	= clk_smd_rpm_prepare,
 	.unprepare	= clk_smd_rpm_unprepare,
+	.is_prepared	= clk_smd_rpm_is_prepared,
 	.recalc_rate	= clk_smd_rpm_recalc_rate,
 };
 
@@ -1279,6 +1288,9 @@  static int rpm_smd_clk_probe(struct platform_device *pdev)
 		ret = clk_smd_rpm_handoff(rpm_smd_clks[i]);
 		if (ret)
 			goto err;
+
+		/* During handoff we force all clocks on */
+		rpm_smd_clks[i]->enabled = true;
 	}
 
 	for (i = 0; i < desc->num_icc_clks; i++) {