| Message ID | 20231004-clk-qcom-smd-rpm-unused-v2-1-9a5281f324dc@kernkonzept.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] clk: qcom: smd: Disable unused clocks | expand |
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
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
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
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
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 --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++) {
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,