linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
@ 2021-11-09  2:25 Shawn Guo
  2021-11-09  2:25 ` [PATCH 1/3] clk: qcom: smd-rpm: Mark clock enabled in clk_smd_rpm_handoff() Shawn Guo
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Shawn Guo @ 2021-11-09  2:25 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Loic Poulain, linux-arm-msm, linux-clk,
	linux-kernel, Shawn Guo

Currently the enable state of smd-rpm clocks are not properly reported
back to framework due to missing .is_enabled and .is_prepared hooks.
This causes a couple of issues.

- All those unused clocks are not voted for off, because framework has
  no knowledge that they are unused.  It becomes a problem for vlow
  power mode support, as we do not have every single RPM clock claimed
  and voted for off by client devices, and rely on clock framework to
  disable those unused RPM clocks.

- The clk_summary in debugfs doesn't show a correct enable state for
  RPM clocks.


Shawn Guo (3):
  clk: qcom: smd-rpm: Mark clock enabled in clk_smd_rpm_handoff()
  clk: qcom: smd-rpm: Add .is_enabled hook
  clk: qcom: smd-rpm: Add .is_prepared hook

 drivers/clk/qcom/clk-smd-rpm.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] clk: qcom: smd-rpm: Mark clock enabled in clk_smd_rpm_handoff()
  2021-11-09  2:25 [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Shawn Guo
@ 2021-11-09  2:25 ` Shawn Guo
  2021-11-09  2:25 ` [PATCH 2/3] clk: qcom: smd-rpm: Add .is_enabled hook Shawn Guo
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Shawn Guo @ 2021-11-09  2:25 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Loic Poulain, linux-arm-msm, linux-clk,
	linux-kernel, Shawn Guo

The result of clock handoff is that the clock is voted by APSS and
enabled by RPM.  So it should be marked as enabled.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 26c70c2fbd26..6695a43a3d73 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -178,6 +178,8 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r)
 	if (ret)
 		return ret;
 
+	r->enabled = true;
+
 	return 0;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] clk: qcom: smd-rpm: Add .is_enabled hook
  2021-11-09  2:25 [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Shawn Guo
  2021-11-09  2:25 ` [PATCH 1/3] clk: qcom: smd-rpm: Mark clock enabled in clk_smd_rpm_handoff() Shawn Guo
@ 2021-11-09  2:25 ` Shawn Guo
  2021-11-09  2:25 ` [PATCH 3/3] clk: qcom: smd-rpm: Add .is_prepared hook Shawn Guo
  2021-11-09 10:26 ` [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Stephan Gerhold
  3 siblings, 0 replies; 13+ messages in thread
From: Shawn Guo @ 2021-11-09  2:25 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Loic Poulain, linux-arm-msm, linux-clk,
	linux-kernel, Shawn Guo

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
show a correct enabling state for RPM clocks.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 6695a43a3d73..f8be58121bd6 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -402,18 +402,27 @@ static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
 	return 0;
 }
 
+static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
+{
+	struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
+
+	return r->enabled;
+}
+
 static const struct clk_ops clk_smd_rpm_ops = {
 	.prepare	= clk_smd_rpm_prepare,
 	.unprepare	= clk_smd_rpm_unprepare,
 	.set_rate	= clk_smd_rpm_set_rate,
 	.round_rate	= clk_smd_rpm_round_rate,
 	.recalc_rate	= clk_smd_rpm_recalc_rate,
+	.is_enabled	= clk_smd_rpm_is_enabled,
 };
 
 static const struct clk_ops clk_smd_rpm_branch_ops = {
 	.prepare	= clk_smd_rpm_prepare,
 	.unprepare	= clk_smd_rpm_unprepare,
 	.recalc_rate	= clk_smd_rpm_recalc_rate,
+	.is_enabled	= clk_smd_rpm_is_enabled,
 };
 
 DEFINE_CLK_SMD_RPM(msm8916, pcnoc_clk, pcnoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 0);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] clk: qcom: smd-rpm: Add .is_prepared hook
  2021-11-09  2:25 [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Shawn Guo
  2021-11-09  2:25 ` [PATCH 1/3] clk: qcom: smd-rpm: Mark clock enabled in clk_smd_rpm_handoff() Shawn Guo
  2021-11-09  2:25 ` [PATCH 2/3] clk: qcom: smd-rpm: Add .is_enabled hook Shawn Guo
@ 2021-11-09  2:25 ` Shawn Guo
  2021-11-09 10:26 ` [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Stephan Gerhold
  3 siblings, 0 replies; 13+ messages in thread
From: Shawn Guo @ 2021-11-09  2:25 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Loic Poulain, linux-arm-msm, linux-clk,
	linux-kernel, Shawn Guo

The RPM clocks are enabled/disabled through clk framework prepare/unprepare
hooks.  Without .is_prepared hook, those unused RPM clocks will not be
disabled by core function clk_unprepare_unused_subtree(), because
clk_core_is_prepared() always returns 0.

Add .is_prepared hook to clk_ops and return the clock prepare (enable)
state, so that those unused RPM clocks can be disabled by clk framework.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index f8be58121bd6..c8f058720e66 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -409,6 +409,13 @@ static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
 	return r->enabled;
 }
 
+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 const struct clk_ops clk_smd_rpm_ops = {
 	.prepare	= clk_smd_rpm_prepare,
 	.unprepare	= clk_smd_rpm_unprepare,
@@ -416,6 +423,7 @@ static const struct clk_ops clk_smd_rpm_ops = {
 	.round_rate	= clk_smd_rpm_round_rate,
 	.recalc_rate	= clk_smd_rpm_recalc_rate,
 	.is_enabled	= clk_smd_rpm_is_enabled,
+	.is_prepared	= clk_smd_rpm_is_prepared,
 };
 
 static const struct clk_ops clk_smd_rpm_branch_ops = {
@@ -423,6 +431,7 @@ static const struct clk_ops clk_smd_rpm_branch_ops = {
 	.unprepare	= clk_smd_rpm_unprepare,
 	.recalc_rate	= clk_smd_rpm_recalc_rate,
 	.is_enabled	= clk_smd_rpm_is_enabled,
+	.is_prepared	= clk_smd_rpm_is_prepared,
 };
 
 DEFINE_CLK_SMD_RPM(msm8916, pcnoc_clk, pcnoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 0);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
  2021-11-09  2:25 [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Shawn Guo
                   ` (2 preceding siblings ...)
  2021-11-09  2:25 ` [PATCH 3/3] clk: qcom: smd-rpm: Add .is_prepared hook Shawn Guo
@ 2021-11-09 10:26 ` Stephan Gerhold
  2021-11-09 15:56   ` Bjorn Andersson
                     ` (2 more replies)
  3 siblings, 3 replies; 13+ messages in thread
From: Stephan Gerhold @ 2021-11-09 10:26 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Stephen Boyd, Bjorn Andersson, Loic Poulain, linux-arm-msm,
	linux-clk, linux-kernel

Hi Shawn,

On Tue, Nov 09, 2021 at 10:25:55AM +0800, Shawn Guo wrote:
> Currently the enable state of smd-rpm clocks are not properly reported
> back to framework due to missing .is_enabled and .is_prepared hooks.
> This causes a couple of issues.
> 
> - All those unused clocks are not voted for off, because framework has
>   no knowledge that they are unused.  It becomes a problem for vlow
>   power mode support, as we do not have every single RPM clock claimed
>   and voted for off by client devices, and rely on clock framework to
>   disable those unused RPM clocks.
> 

I posted a similar patch a bit more than a year ago [1]. Back then one
of the concerns was that we might disable critical clocks just because
they have no driver using it actively. For example, not all of the
platforms using clk-smd-rpm already have an interconnect driver.
Disabling the interconnect related clocks will almost certainly make the
device lock up completely. (I tried it back then, it definitely does...)

I proposed adding CLK_IGNORE_UNUSED for the interconnect related clocks
back then [2] which would allow disabling most of the clocks at least.
Stephen Boyd had an alternative proposal to instead move the
interconnect related clocks completely out of clk-smd-rpm [3].
But I'm still unsure how this would work in a backwards compatible way. [4]

Since your patches are more or less identical I'm afraid the same
concerns still need to be solved somehow. :)

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/20200817140908.185976-1-stephan@gerhold.net/
[2]: https://lore.kernel.org/linux-arm-msm/20200818080738.GA46574@gerhold.net/
[3]: https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
[4]: https://lore.kernel.org/linux-arm-msm/20200821064857.GA905@gerhold.net/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
  2021-11-09 10:26 ` [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Stephan Gerhold
@ 2021-11-09 15:56   ` Bjorn Andersson
  2021-11-10 13:15   ` Shawn Guo
  2021-12-09  9:10   ` Stephen Boyd
  2 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2021-11-09 15:56 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Shawn Guo, Stephen Boyd, Loic Poulain, linux-arm-msm, linux-clk,
	linux-kernel

On Tue 09 Nov 02:26 PST 2021, Stephan Gerhold wrote:

> Hi Shawn,
> 
> On Tue, Nov 09, 2021 at 10:25:55AM +0800, Shawn Guo wrote:
> > Currently the enable state of smd-rpm clocks are not properly reported
> > back to framework due to missing .is_enabled and .is_prepared hooks.
> > This causes a couple of issues.
> > 
> > - All those unused clocks are not voted for off, because framework has
> >   no knowledge that they are unused.  It becomes a problem for vlow
> >   power mode support, as we do not have every single RPM clock claimed
> >   and voted for off by client devices, and rely on clock framework to
> >   disable those unused RPM clocks.
> > 
> 
> I posted a similar patch a bit more than a year ago [1]. Back then one
> of the concerns was that we might disable critical clocks just because
> they have no driver using it actively. For example, not all of the
> platforms using clk-smd-rpm already have an interconnect driver.
> Disabling the interconnect related clocks will almost certainly make the
> device lock up completely. (I tried it back then, it definitely does...)
> 
> I proposed adding CLK_IGNORE_UNUSED for the interconnect related clocks
> back then [2] which would allow disabling most of the clocks at least.
> Stephen Boyd had an alternative proposal to instead move the
> interconnect related clocks completely out of clk-smd-rpm [3].
> But I'm still unsure how this would work in a backwards compatible way. [4]
> 

With the introduction of QoS the interconnect drivers need to be mmio
devices, and plural, while in order to talk to the RPM we need something
on the rpmsg bus.

So I don't think Stephen's proposal will work, unless we like in the
RPMh case comes up with an equivalent of the bcm-voter (which just moved
the clocks from one clock driver to a "clock" driver).


On the other hand, if clocks and the clk-smd-rpm driver in particular
moves to sync_state then this wouldn't be a problem...

Regards,
Bjorn

> Since your patches are more or less identical I'm afraid the same
> concerns still need to be solved somehow. :)
> 
> Thanks,
> Stephan
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20200817140908.185976-1-stephan@gerhold.net/
> [2]: https://lore.kernel.org/linux-arm-msm/20200818080738.GA46574@gerhold.net/
> [3]: https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
> [4]: https://lore.kernel.org/linux-arm-msm/20200821064857.GA905@gerhold.net/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
  2021-11-09 10:26 ` [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Stephan Gerhold
  2021-11-09 15:56   ` Bjorn Andersson
@ 2021-11-10 13:15   ` Shawn Guo
  2021-11-10 13:48     ` Bjorn Andersson
  2021-11-10 14:09     ` Stephan Gerhold
  2021-12-09  9:10   ` Stephen Boyd
  2 siblings, 2 replies; 13+ messages in thread
From: Shawn Guo @ 2021-11-10 13:15 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Stephen Boyd, Bjorn Andersson, Loic Poulain, linux-arm-msm,
	linux-clk, linux-kernel

Hi Stephan,

On Tue, Nov 09, 2021 at 11:26:21AM +0100, Stephan Gerhold wrote:
> Hi Shawn,
> 
> On Tue, Nov 09, 2021 at 10:25:55AM +0800, Shawn Guo wrote:
> > Currently the enable state of smd-rpm clocks are not properly reported
> > back to framework due to missing .is_enabled and .is_prepared hooks.
> > This causes a couple of issues.
> > 
> > - All those unused clocks are not voted for off, because framework has
> >   no knowledge that they are unused.  It becomes a problem for vlow
> >   power mode support, as we do not have every single RPM clock claimed
> >   and voted for off by client devices, and rely on clock framework to
> >   disable those unused RPM clocks.
> > 
> 
> I posted a similar patch a bit more than a year ago [1].

Ouch, that's unfortunate!  If your patch landed, I wouldn't have had to
spend such a long time to figure out why my platform fails to reach vlow
power mode :(

> Back then one
> of the concerns was that we might disable critical clocks just because
> they have no driver using it actively. For example, not all of the
> platforms using clk-smd-rpm already have an interconnect driver.
> Disabling the interconnect related clocks will almost certainly make the
> device lock up completely. (I tried it back then, it definitely does...)
> 
> I proposed adding CLK_IGNORE_UNUSED for the interconnect related clocks
> back then [2] which would allow disabling most of the clocks at least.
> Stephen Boyd had an alternative proposal to instead move the
> interconnect related clocks completely out of clk-smd-rpm [3].
> But I'm still unsure how this would work in a backwards compatible way. [4]
> 
> Since your patches are more or less identical I'm afraid the same
> concerns still need to be solved somehow. :)

I do not really understand why smd-rpm clock driver needs to be a special
case.  This is a very common issue, mostly in device early support phase
where not all clock consumer drivers are ready.  Flag CLK_IGNORE_UNUSED
and kernel cmdline 'clk_ignore_unused' are created just for that.  Those
"broken" platforms should be booted with 'clk_ignore_unused' until they
have related consumer drivers in place.  IMHO, properly reporting enable
state to framework is definitely the right thing to do, and should have
been done from day one.

Shawn

> [1]: https://lore.kernel.org/linux-arm-msm/20200817140908.185976-1-stephan@gerhold.net/
> [2]: https://lore.kernel.org/linux-arm-msm/20200818080738.GA46574@gerhold.net/
> [3]: https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
> [4]: https://lore.kernel.org/linux-arm-msm/20200821064857.GA905@gerhold.net/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
  2021-11-10 13:15   ` Shawn Guo
@ 2021-11-10 13:48     ` Bjorn Andersson
  2021-11-11  9:39       ` Shawn Guo
  2021-11-10 14:09     ` Stephan Gerhold
  1 sibling, 1 reply; 13+ messages in thread
From: Bjorn Andersson @ 2021-11-10 13:48 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Stephan Gerhold, Stephen Boyd, Loic Poulain, linux-arm-msm,
	linux-clk, linux-kernel

On Wed 10 Nov 05:15 PST 2021, Shawn Guo wrote:

> Hi Stephan,
> 
> On Tue, Nov 09, 2021 at 11:26:21AM +0100, Stephan Gerhold wrote:
> > Hi Shawn,
> > 
> > On Tue, Nov 09, 2021 at 10:25:55AM +0800, Shawn Guo wrote:
> > > Currently the enable state of smd-rpm clocks are not properly reported
> > > back to framework due to missing .is_enabled and .is_prepared hooks.
> > > This causes a couple of issues.
> > > 
> > > - All those unused clocks are not voted for off, because framework has
> > >   no knowledge that they are unused.  It becomes a problem for vlow
> > >   power mode support, as we do not have every single RPM clock claimed
> > >   and voted for off by client devices, and rely on clock framework to
> > >   disable those unused RPM clocks.
> > > 
> > 
> > I posted a similar patch a bit more than a year ago [1].
> 
> Ouch, that's unfortunate!  If your patch landed, I wouldn't have had to
> spend such a long time to figure out why my platform fails to reach vlow
> power mode :(
> 
> > Back then one
> > of the concerns was that we might disable critical clocks just because
> > they have no driver using it actively. For example, not all of the
> > platforms using clk-smd-rpm already have an interconnect driver.
> > Disabling the interconnect related clocks will almost certainly make the
> > device lock up completely. (I tried it back then, it definitely does...)
> > 
> > I proposed adding CLK_IGNORE_UNUSED for the interconnect related clocks
> > back then [2] which would allow disabling most of the clocks at least.
> > Stephen Boyd had an alternative proposal to instead move the
> > interconnect related clocks completely out of clk-smd-rpm [3].
> > But I'm still unsure how this would work in a backwards compatible way. [4]
> > 
> > Since your patches are more or less identical I'm afraid the same
> > concerns still need to be solved somehow. :)
> 
> I do not really understand why smd-rpm clock driver needs to be a special
> case.  This is a very common issue, mostly in device early support phase
> where not all clock consumer drivers are ready.  Flag CLK_IGNORE_UNUSED
> and kernel cmdline 'clk_ignore_unused' are created just for that.  Those
> "broken" platforms should be booted with 'clk_ignore_unused' until they
> have related consumer drivers in place.

Afaict we still have the problem that if the interconnect driver is
compiled as a module, or for other reasons doesn't probe until after
late_initcall() clk-smd-rpm will turn off these clocks and we never will
get a chance to probe the interconnect provider.

I believe the way to handle that is to rely on sync_state, but there
seems to be a lot of corner cases here.

But with that in place, I agree that we should handle this temporarily
during bringup by the use of clk_ignore_unused.

> IMHO, properly reporting enable state to framework is definitely the
> right thing to do, and should have been done from day one.
> 

I always thought is_enabled() should reflect the hardware state - in
particular for clk_summary. The particular concern being that by
initializing the is_enabled() state to either true or false, we're
making an assumption about the hardware state. And if something where to
do if (enabled) disable (or if (disabled) enable), we might skip a
critical operation just because we tricked the logic.

So, do you need it for anything other than clk_disable_unused()?

I have a clock in the MDP with similar issue, where we don't have
is_enabled() but I need it to be disabled by clk_disable_unused(),
because the next iteration turns off the parent and locks up the still
"active" rcg.
So far I've not received any feedback on this though...
https://lore.kernel.org/all/20210707043859.195870-1-bjorn.andersson@linaro.org/

With this approach we don't make any assumptions about the hardware
state, beyond the fact that we will issue a disable in
clk_disable_unused() if no one has yet enabled the clock - which at
worst turns off a clock that's already is off.

Regards,
Bjorn

> Shawn
> 
> > [1]: https://lore.kernel.org/linux-arm-msm/20200817140908.185976-1-stephan@gerhold.net/
> > [2]: https://lore.kernel.org/linux-arm-msm/20200818080738.GA46574@gerhold.net/
> > [3]: https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
> > [4]: https://lore.kernel.org/linux-arm-msm/20200821064857.GA905@gerhold.net/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
  2021-11-10 13:15   ` Shawn Guo
  2021-11-10 13:48     ` Bjorn Andersson
@ 2021-11-10 14:09     ` Stephan Gerhold
  2021-11-10 14:58       ` Bjorn Andersson
  1 sibling, 1 reply; 13+ messages in thread
From: Stephan Gerhold @ 2021-11-10 14:09 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Stephen Boyd, Bjorn Andersson, Loic Poulain, linux-arm-msm,
	linux-clk, linux-kernel

On Wed, Nov 10, 2021 at 09:15:11PM +0800, Shawn Guo wrote:
> On Tue, Nov 09, 2021 at 11:26:21AM +0100, Stephan Gerhold wrote:
> > On Tue, Nov 09, 2021 at 10:25:55AM +0800, Shawn Guo wrote:
> > > Currently the enable state of smd-rpm clocks are not properly reported
> > > back to framework due to missing .is_enabled and .is_prepared hooks.
> > > This causes a couple of issues.
> > > 
> > > - All those unused clocks are not voted for off, because framework has
> > >   no knowledge that they are unused.  It becomes a problem for vlow
> > >   power mode support, as we do not have every single RPM clock claimed
> > >   and voted for off by client devices, and rely on clock framework to
> > >   disable those unused RPM clocks.
> > > 
> > 
> > I posted a similar patch a bit more than a year ago [1].
> 
> Ouch, that's unfortunate!  If your patch landed, I wouldn't have had to
> spend such a long time to figure out why my platform fails to reach vlow
> power mode :(
> 

Sorry, I was waiting for Stephen to reply and eventually decided to
shift focus to other things first. :)

The whole low-power topic is kind of frustrating on older platforms
because they currently still lack almost everything that is necessary to
reach those low power states. Even things that you already consider
natural for newer platforms (such as interconnect) are still very much
work in progress on all older ones.

> > Back then one
> > of the concerns was that we might disable critical clocks just because
> > they have no driver using it actively. For example, not all of the
> > platforms using clk-smd-rpm already have an interconnect driver.
> > Disabling the interconnect related clocks will almost certainly make the
> > device lock up completely. (I tried it back then, it definitely does...)
> > 
> > I proposed adding CLK_IGNORE_UNUSED for the interconnect related clocks
> > back then [2] which would allow disabling most of the clocks at least.
> > Stephen Boyd had an alternative proposal to instead move the
> > interconnect related clocks completely out of clk-smd-rpm [3].
> > But I'm still unsure how this would work in a backwards compatible way. [4]
> > 
> > Since your patches are more or less identical I'm afraid the same
> > concerns still need to be solved somehow. :)
> 
> I do not really understand why smd-rpm clock driver needs to be a special
> case.  This is a very common issue, mostly in device early support phase
> where not all clock consumer drivers are ready.  Flag CLK_IGNORE_UNUSED
> and kernel cmdline 'clk_ignore_unused' are created just for that.  Those
> "broken" platforms should be booted with 'clk_ignore_unused' until they
> have related consumer drivers in place.  IMHO, properly reporting enable
> state to framework is definitely the right thing to do, and should have
> been done from day one.
> 

... And therefore I think we should be careful with such changes,
especially if they would prevent devices from booting completely.
Unfortunately the users trying to make use of old platforms are also
often the ones who might not be aware that they suddenly need
"clk_ignore_unused" just to boot a system that was previously working
(mostly) fine, except for the whole low-power topic.

I fully agree with you that disabling the unused clocks here is the
right thing to do, but I think we should try to carefully flag the most
important clocks in the driver to avoid causing too many regressions.

Thanks,
Stephan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
  2021-11-10 14:09     ` Stephan Gerhold
@ 2021-11-10 14:58       ` Bjorn Andersson
  0 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2021-11-10 14:58 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Shawn Guo, Stephen Boyd, Loic Poulain, linux-arm-msm, linux-clk,
	linux-kernel

On Wed 10 Nov 06:09 PST 2021, Stephan Gerhold wrote:

> On Wed, Nov 10, 2021 at 09:15:11PM +0800, Shawn Guo wrote:
> > On Tue, Nov 09, 2021 at 11:26:21AM +0100, Stephan Gerhold wrote:
> > > On Tue, Nov 09, 2021 at 10:25:55AM +0800, Shawn Guo wrote:
> > > > Currently the enable state of smd-rpm clocks are not properly reported
> > > > back to framework due to missing .is_enabled and .is_prepared hooks.
> > > > This causes a couple of issues.
> > > > 
> > > > - All those unused clocks are not voted for off, because framework has
> > > >   no knowledge that they are unused.  It becomes a problem for vlow
> > > >   power mode support, as we do not have every single RPM clock claimed
> > > >   and voted for off by client devices, and rely on clock framework to
> > > >   disable those unused RPM clocks.
> > > > 
> > > 
> > > I posted a similar patch a bit more than a year ago [1].
> > 
> > Ouch, that's unfortunate!  If your patch landed, I wouldn't have had to
> > spend such a long time to figure out why my platform fails to reach vlow
> > power mode :(
> > 
> 
> Sorry, I was waiting for Stephen to reply and eventually decided to
> shift focus to other things first. :)
> 
> The whole low-power topic is kind of frustrating on older platforms
> because they currently still lack almost everything that is necessary to
> reach those low power states. Even things that you already consider
> natural for newer platforms (such as interconnect) are still very much
> work in progress on all older ones.
> 
> > > Back then one
> > > of the concerns was that we might disable critical clocks just because
> > > they have no driver using it actively. For example, not all of the
> > > platforms using clk-smd-rpm already have an interconnect driver.
> > > Disabling the interconnect related clocks will almost certainly make the
> > > device lock up completely. (I tried it back then, it definitely does...)
> > > 
> > > I proposed adding CLK_IGNORE_UNUSED for the interconnect related clocks
> > > back then [2] which would allow disabling most of the clocks at least.
> > > Stephen Boyd had an alternative proposal to instead move the
> > > interconnect related clocks completely out of clk-smd-rpm [3].
> > > But I'm still unsure how this would work in a backwards compatible way. [4]
> > > 
> > > Since your patches are more or less identical I'm afraid the same
> > > concerns still need to be solved somehow. :)
> > 
> > I do not really understand why smd-rpm clock driver needs to be a special
> > case.  This is a very common issue, mostly in device early support phase
> > where not all clock consumer drivers are ready.  Flag CLK_IGNORE_UNUSED
> > and kernel cmdline 'clk_ignore_unused' are created just for that.  Those
> > "broken" platforms should be booted with 'clk_ignore_unused' until they
> > have related consumer drivers in place.  IMHO, properly reporting enable
> > state to framework is definitely the right thing to do, and should have
> > been done from day one.
> > 
> 
> ... And therefore I think we should be careful with such changes,
> especially if they would prevent devices from booting completely.
> Unfortunately the users trying to make use of old platforms are also
> often the ones who might not be aware that they suddenly need
> "clk_ignore_unused" just to boot a system that was previously working
> (mostly) fine, except for the whole low-power topic.
> 
> I fully agree with you that disabling the unused clocks here is the
> right thing to do, but I think we should try to carefully flag the most
> important clocks in the driver to avoid causing too many regressions.
> 

I don't fancy the idea of forcing everyone to run with specific kernel
command line parameters - in particular not as a means to avoid
"regressions".

I think the only way around this problem is to figure out how to move
the clk disablement to sync_state - probably per clock driver.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
  2021-11-10 13:48     ` Bjorn Andersson
@ 2021-11-11  9:39       ` Shawn Guo
  2021-12-06 15:42         ` Bjorn Andersson
  0 siblings, 1 reply; 13+ messages in thread
From: Shawn Guo @ 2021-11-11  9:39 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Stephan Gerhold, Stephen Boyd, Loic Poulain, linux-arm-msm,
	linux-clk, linux-kernel

On Wed, Nov 10, 2021 at 05:48:10AM -0800, Bjorn Andersson wrote:
> > IMHO, properly reporting enable state to framework is definitely the
> > right thing to do, and should have been done from day one.
> > 
> 
> I always thought is_enabled() should reflect the hardware state - in
> particular for clk_summary. The particular concern being that by
> initializing the is_enabled() state to either true or false, we're
> making an assumption about the hardware state. And if something where to
> do if (enabled) disable (or if (disabled) enable), we might skip a
> critical operation just because we tricked the logic.

That's probably why clk_smd_rpm_handoff() is called.  As there is no way
to query RPM for resource state, we send enable request for all RPM
clocks to get hardware and software state in sync.

> So, do you need it for anything other than clk_disable_unused()?

Not critical, but I need it for debugfs clk_summary as well.

Shawn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
  2021-11-11  9:39       ` Shawn Guo
@ 2021-12-06 15:42         ` Bjorn Andersson
  0 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2021-12-06 15:42 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Stephan Gerhold, Stephen Boyd, Loic Poulain, linux-arm-msm,
	linux-clk, linux-kernel

On Thu 11 Nov 03:39 CST 2021, Shawn Guo wrote:

> On Wed, Nov 10, 2021 at 05:48:10AM -0800, Bjorn Andersson wrote:
> > > IMHO, properly reporting enable state to framework is definitely the
> > > right thing to do, and should have been done from day one.
> > > 
> > 
> > I always thought is_enabled() should reflect the hardware state - in
> > particular for clk_summary. The particular concern being that by
> > initializing the is_enabled() state to either true or false, we're
> > making an assumption about the hardware state. And if something where to
> > do if (enabled) disable (or if (disabled) enable), we might skip a
> > critical operation just because we tricked the logic.
> 
> That's probably why clk_smd_rpm_handoff() is called.  As there is no way
> to query RPM for resource state, we send enable request for all RPM
> clocks to get hardware and software state in sync.
> 

clk_smd_rpm_handoff() will ensure that all SMD clocks are enabled, and
at max speed during rpm_smd_clk_probe(). Once clients starts actually
voting for rates that will change.

(Un)fortunately as we don't provide an implementation of is_enabled()
clk_disable_unused() won't try to turn them off. This similar to a
problem I have elsewhere, for which I proposed:
https://lore.kernel.org/linux-arm-msm/20211203035436.3505743-1-bjorn.andersson@linaro.org/

We should at some point introduce this for the SMD clocks as well.


However, we have two problems:
1) Compiling e.g. the interconnect provider as a module would mean that
clk_disable_unused() kicks in before the client has had a chance to vote
for the clock.

2) One client may enable the clock during its probe and then disable it.
Being the last active user the clock framework happily turns off the
clock.


For both of these cases, we need to ensure that the clocks aren't
disabled until sync_state() kicks in.

Regards,
Bjorn

> > So, do you need it for anything other than clk_disable_unused()?
> 
> Not critical, but I need it for debugfs clk_summary as well.
> 
> Shawn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
  2021-11-09 10:26 ` [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Stephan Gerhold
  2021-11-09 15:56   ` Bjorn Andersson
  2021-11-10 13:15   ` Shawn Guo
@ 2021-12-09  9:10   ` Stephen Boyd
  2 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2021-12-09  9:10 UTC (permalink / raw)
  To: Shawn Guo, Stephan Gerhold
  Cc: Bjorn Andersson, Loic Poulain, linux-arm-msm, linux-clk, linux-kernel

Quoting Stephan Gerhold (2021-11-09 02:26:21)
> Hi Shawn,
> 
> On Tue, Nov 09, 2021 at 10:25:55AM +0800, Shawn Guo wrote:
> > Currently the enable state of smd-rpm clocks are not properly reported
> > back to framework due to missing .is_enabled and .is_prepared hooks.
> > This causes a couple of issues.
> > 
> > - All those unused clocks are not voted for off, because framework has
> >   no knowledge that they are unused.  It becomes a problem for vlow
> >   power mode support, as we do not have every single RPM clock claimed
> >   and voted for off by client devices, and rely on clock framework to
> >   disable those unused RPM clocks.
> > 
> 
> I posted a similar patch a bit more than a year ago [1]. Back then one
> of the concerns was that we might disable critical clocks just because
> they have no driver using it actively. For example, not all of the
> platforms using clk-smd-rpm already have an interconnect driver.
> Disabling the interconnect related clocks will almost certainly make the
> device lock up completely. (I tried it back then, it definitely does...)
> 
> I proposed adding CLK_IGNORE_UNUSED for the interconnect related clocks
> back then [2] which would allow disabling most of the clocks at least.
> Stephen Boyd had an alternative proposal to instead move the
> interconnect related clocks completely out of clk-smd-rpm [3].
> But I'm still unsure how this would work in a backwards compatible way. [4]

We should stop adding to the pile of smd-rpm clks that are clearly
interconnects. I'm ready to stop accepting patches like 78b727d02815
("clk: qcom: smd-rpm: Add QCM2290 RPM clock support").

Someone needs to put in the work to make an interconnect provider that
directly talks to the rpm, without going through the clk framework just
because the rpm talks in kHz for these resources. These clk have no
parent and are essentially a proxy for some firmware interface to the
rpm but we put it behind the clk framework for reasons I don't know why.

I honestly don't understand the backwards incompatibility argument for
this either. If we're adding more SoC support for this driver we need to
stop and figure out a better approach. Make a new interconnect driver,
plug it in via DT, wait a release cycle, and finally dump the smd-rpm
clk node from older platforms that were using the clk framework. At
least for new SoCs this problem doesn't exist.

There's that one graphics clk (RPM_SMD_GFX3D_CLK_SRC) but I don't see it
used anywhere. So it's not really important? Maybe we need to set some
bandwidth in the graphics clk driver? I dunno.

> 
> Since your patches are more or less identical I'm afraid the same
> concerns still need to be solved somehow. :)
> 
> Thanks,
> Stephan
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20200817140908.185976-1-stephan@gerhold.net/
> [2]: https://lore.kernel.org/linux-arm-msm/20200818080738.GA46574@gerhold.net/
> [3]: https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
> [4]: https://lore.kernel.org/linux-arm-msm/20200821064857.GA905@gerhold.net/

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-12-09  9:10 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-09  2:25 [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Shawn Guo
2021-11-09  2:25 ` [PATCH 1/3] clk: qcom: smd-rpm: Mark clock enabled in clk_smd_rpm_handoff() Shawn Guo
2021-11-09  2:25 ` [PATCH 2/3] clk: qcom: smd-rpm: Add .is_enabled hook Shawn Guo
2021-11-09  2:25 ` [PATCH 3/3] clk: qcom: smd-rpm: Add .is_prepared hook Shawn Guo
2021-11-09 10:26 ` [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework Stephan Gerhold
2021-11-09 15:56   ` Bjorn Andersson
2021-11-10 13:15   ` Shawn Guo
2021-11-10 13:48     ` Bjorn Andersson
2021-11-11  9:39       ` Shawn Guo
2021-12-06 15:42         ` Bjorn Andersson
2021-11-10 14:09     ` Stephan Gerhold
2021-11-10 14:58       ` Bjorn Andersson
2021-12-09  9:10   ` Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).