[PATCH] clk: export function clk_disable_unused()

[PATCH] clk: export function clk_disable_unused()

[Jim Quinlan]

For Broadcom STB chips, clocks may come up after resume in an "on"
state, so calling clk_disable_unused() again will turn off unused
clocks.  This commit exports clk_disable_unused() so it may be
called in such cases.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/clk/clk.c   | 2 +-
 include/linux/clk.h | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 459ce9d..4469f8b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -549,7 +549,7 @@ static int __init clk_ignore_unused_setup(char *__unused)
 }
 __setup("clk_ignore_unused", clk_ignore_unused_setup);
 
-static int clk_disable_unused(void)
+int clk_disable_unused(void)
 {
 	struct clk_core *clk;
 
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 68c16a6..c03822c 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -265,6 +265,14 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_disable_unused - turns off unused clocks.
+ *
+ * This is already called as a late initcall, but may be
+ * called again in special cases such as resuming from S3.
+ */
+int clk_disable_unused(void);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
  * @clk: clock source
-- 
1.9.0.138.g2de3478

Re: [PATCH] clk: export function clk_disable_unused()

[Michael Turquette]

Quoting Jim Quinlan (2015-05-15 15:22:36)
> For Broadcom STB chips, clocks may come up after resume in an "on"
> state, so calling clk_disable_unused() again will turn off unused
> clocks.  This commit exports clk_disable_unused() so it may be
> called in such cases.

Jim,

Thanks for the patch.

I think a more general solution to the problem might be needed. E.g. how
do we solve for the opposite case where after a low-power suspend/resume
we turn on clocks that were enabled by drivers before suspending, but
are disabled out of reset/post-transition?

Additionally, it would be helpful to see your driver changes and how
exactly you plan to use this newly exported function.

I've Cc'd Grygorii Strashko from TI. I faintly recall that he had an
out-of-tree solution to add some .suspend/.resume callbacks to struct
clk_ops. I didn't like that solution at the time either, but maybe we
can figure out a more natural way to handle these cases.

(Grygorii: sorry in advance if this is a case of mistaken identity. I
could not find the code in question from my email archive)

Regards,
Mike

> =

> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/clk/clk.c   | 2 +-
>  include/linux/clk.h | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> =

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 459ce9d..4469f8b 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -549,7 +549,7 @@ static int __init clk_ignore_unused_setup(char *__unu=
sed)
>  }
>  __setup("clk_ignore_unused", clk_ignore_unused_setup);
>  =

> -static int clk_disable_unused(void)
> +int clk_disable_unused(void)
>  {
>         struct clk_core *clk;
>  =

> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 68c16a6..c03822c 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -265,6 +265,14 @@ int clk_enable(struct clk *clk);
>  void clk_disable(struct clk *clk);
>  =

>  /**
> + * clk_disable_unused - turns off unused clocks.
> + *
> + * This is already called as a late initcall, but may be
> + * called again in special cases such as resuming from S3.
> + */
> +int clk_disable_unused(void);
> +
> +/**
>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock sour=
ce.
>   *               This is only valid once the clock source has been enabl=
ed.
>   * @clk: clock source
> -- =

> 1.9.0.138.g2de3478
>=20

Re: [PATCH] clk: export function clk_disable_unused()

[Grygorii.Strashko@linaro.org]

Hi Mike,
On 05/23/2015 10:33 PM, Michael Turquette wrote:
> Quoting Jim Quinlan (2015-05-15 15:22:36)
>> For Broadcom STB chips, clocks may come up after resume in an "on"
>> state, so calling clk_disable_unused() again will turn off unused
>> clocks.  This commit exports clk_disable_unused() so it may be
>> called in such cases.
> 
> Jim,
> 
> Thanks for the patch.
> 
> I think a more general solution to the problem might be needed. E.g. how
> do we solve for the opposite case where after a low-power suspend/resume
> we turn on clocks that were enabled by drivers before suspending, but
> are disabled out of reset/post-transition?
> 
> Additionally, it would be helpful to see your driver changes and how
> exactly you plan to use this newly exported function.
> 
> I've Cc'd Grygorii Strashko from TI. I faintly recall that he had an
> out-of-tree solution to add some .suspend/.resume callbacks to struct
> clk_ops. I didn't like that solution at the time either, but maybe we
> can figure out a more natural way to handle these cases.
> 
> (Grygorii: sorry in advance if this is a case of mistaken identity. I
> could not find the code in question from my email archive)

I've copied links here FYI:

[1] http://git.ti.com/ti-linux-kernel/ti-linux-kernel/commit/31bdb0f10c7129a157e238fde12390dae8e08d92
[2-fix] http://git.ti.com/ti-linux-kernel/ti-linux-kernel/commit/bdb1402837ae5232748ce79fc1f286b4c2b0b2ac

It looks like this is a little bit different problem from one side
 - some clocks can be enabled unconditionally by HW or FW on restore/resume.
And, seems, possible way to solve it is to use PM notifiers in
clk driver/core (I hate PM notifiers;), but I don't see another generic way,
because CLKs are not devices):
 PM_POST_SUSPEND:
	if (!hw->clk->enable_count)
		hw->clk->ops->disable(hw);
 - or - 
	clk_disable_unused();
        ^^ then this patch is reasonable :)

>From another side, It might be solved for gate clocks case with mentioned above
.restore_context() callback... theoretically.

Also there are few comments from our internal discussion, FYI:

=== Mike == :
"As per our discussion, your objection was that if we will
- introduce per-clocks  .save_context/restore_context callbacks
- perform clks restore during system suspend resume/hibernation restore
  using depth-first-pre-order tree traversal algorithm

then at some point we might get situation when DPLL's rate has been
changed already, but some Divider clock still configured using default
values. This may lead to the fact that unsafe frequency will be
generated  for the corresponding subtree during some period of time
(potentially short) until context of this Divider clock will be
restored.

proposition was:
- save/restore clock's HW context all at once (PM domains +
clockdomains + clocks registers)"

== Tero Kristo == :
"The problem with per/clock save/restore context is also that it will potentially
 read/write same registers multiple times, where it would be sufficient to combine these
 to a single access. Consider for example omap3, where we have a single FCLKEN / ICLKEN register,
 which contains enable bits for 20+ clocks. This can be optimized to a single access instead of 20+.

 Personally I think the save-restore should be done on lower level, for example by the PRCM drivers
 that are providing the register space for the clocks. This way we can more easily handle all the corner
 cases also, or do a simple save-restore for a range of registers. It might be necessary to do callbacks
 to clock code in some cases, like DPLLs which are rather complex to control.

 I think this sounds pretty similar to what Mike was proposing looking at the comment below."

=== Mike == :
"Correct. My main concern talking to Grygorii at HKG15 was that putting
tree traversal logic into the clock core for doing context
save/restore was fixing a problem in the place and in the wrong way.
The driver that maps onto the range of registers that lose context
should manage this stuff. If that is not working today then it should
be fixed, but the solution is not to put this stuff into the clock
framework core."

-- 
regards,
-grygorii

Re: [PATCH] clk: export function clk_disable_unused()

[Jim Quinlan]

Hi Mike,

We are currently doing [1].  We'd like to change this so that we are
the same as upstream barring a small syscore driver that will call
clk_disable_unused() on S2/S3 resume.  My patch reflects the
suggestion found in [2], which says, "... It could still be generic by
exporting the clk_disable_unused() function to drivers...".

We do not have the general problems you are alluding to.  With few
exceptions, we do not yet use power domains or runtime PM, although of
course this may change.  Our drivers  use either syscore calls or
SIMPLE_DEV_PM_OPS() to turn off/on clocks as needed during S2/S3
suspend/resume.  The problem we have is when the software stack omits
a driver or we have a new HW block which has no driver.  On boot,
clk_disable_unused() turns off the clocks that these missing drivers
would normally use*.  But on resumption of S2/S3, these clocks get
turned on by HW, and all I am asking is that clk_disable_unused() be
exported so a driver may invoke it to do the same thing it does during
boot.

Regards,
Jim

* Why the HW turns these clocks on in the first place is a question I
cannot answer.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/311568.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/311575.html

On Sat, May 23, 2015 at 3:33 PM, Michael Turquette
<mturquette@linaro.org> wrote:
> Quoting Jim Quinlan (2015-05-15 15:22:36)
>> For Broadcom STB chips, clocks may come up after resume in an "on"
>> state, so calling clk_disable_unused() again will turn off unused
>> clocks.  This commit exports clk_disable_unused() so it may be
>> called in such cases.
>
> Jim,
>
> Thanks for the patch.
>
> I think a more general solution to the problem might be needed. E.g. how
> do we solve for the opposite case where after a low-power suspend/resume
> we turn on clocks that were enabled by drivers before suspending, but
> are disabled out of reset/post-transition?
>
> Additionally, it would be helpful to see your driver changes and how
> exactly you plan to use this newly exported function.
>
> I've Cc'd Grygorii Strashko from TI. I faintly recall that he had an
> out-of-tree solution to add some .suspend/.resume callbacks to struct
> clk_ops. I didn't like that solution at the time either, but maybe we
> can figure out a more natural way to handle these cases.
>
> (Grygorii: sorry in advance if this is a case of mistaken identity. I
> could not find the code in question from my email archive)
>
> Regards,
> Mike
>
>>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> ---
>>  drivers/clk/clk.c   | 2 +-
>>  include/linux/clk.h | 8 ++++++++
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 459ce9d..4469f8b 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -549,7 +549,7 @@ static int __init clk_ignore_unused_setup(char *__unused)
>>  }
>>  __setup("clk_ignore_unused", clk_ignore_unused_setup);
>>
>> -static int clk_disable_unused(void)
>> +int clk_disable_unused(void)
>>  {
>>         struct clk_core *clk;
>>
>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>> index 68c16a6..c03822c 100644
>> --- a/include/linux/clk.h
>> +++ b/include/linux/clk.h
>> @@ -265,6 +265,14 @@ int clk_enable(struct clk *clk);
>>  void clk_disable(struct clk *clk);
>>
>>  /**
>> + * clk_disable_unused - turns off unused clocks.
>> + *
>> + * This is already called as a late initcall, but may be
>> + * called again in special cases such as resuming from S3.
>> + */
>> +int clk_disable_unused(void);
>> +
>> +/**
>>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>>   *               This is only valid once the clock source has been enabled.
>>   * @clk: clock source
>> --
>> 1.9.0.138.g2de3478
>>

Re: [PATCH] clk: export function clk_disable_unused()

[Grygorii.Strashko@linaro.org]

On 05/28/2015 05:21 PM, Jim Quinlan wrote:

> We are currently doing [1].  We'd like to change this so that we are
> the same as upstream barring a small syscore driver that will call
> clk_disable_unused() on S2/S3 resume.  My patch reflects the
> suggestion found in [2], which says, "... It could still be generic by
> exporting the clk_disable_unused() function to drivers...".
> 
> We do not have the general problems you are alluding to.  With few
> exceptions, we do not yet use power domains or runtime PM, although of
> course this may change.  Our drivers  use either syscore calls or
> SIMPLE_DEV_PM_OPS() to turn off/on clocks as needed during S2/S3
> suspend/resume.  The problem we have is when the software stack omits
> a driver or we have a new HW block which has no driver.  On boot,
> clk_disable_unused() turns off the clocks that these missing drivers
> would normally use*.  But on resumption of S2/S3, these clocks get
> turned on by HW, and all I am asking is that clk_disable_unused() be
> exported so a driver may invoke it to do the same thing it does during
> boot.

This issue is much more simpler then saving/restoring of clocks state, so
may be it can be solved first.

Taking to the account comments from [2] I think that could be
done inside clk core using PM notifiers, 
and sysfs/module parameter can be used to enable/disable this feature, like:
 PM_POST_SUSPEND:
 PM_POST_HIBERNATION:
 PM_RESTORE_PREPARE:
	if (clk_disable_unused_on_suspend)
		clk_disable_unused();


Mike, What do you think?

> 
> * Why the HW turns these clocks on in the first place is a question I
> cannot answer.
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/311568.html
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/311575.html
> 
> On Sat, May 23, 2015 at 3:33 PM, Michael Turquette
> <mturquette@linaro.org> wrote:
>> Quoting Jim Quinlan (2015-05-15 15:22:36)
>>> For Broadcom STB chips, clocks may come up after resume in an "on"
>>> state, so calling clk_disable_unused() again will turn off unused
>>> clocks.  This commit exports clk_disable_unused() so it may be
>>> called in such cases.
>>
>> Jim,
>>
>> Thanks for the patch.
>>
>> I think a more general solution to the problem might be needed. E.g. how
>> do we solve for the opposite case where after a low-power suspend/resume
>> we turn on clocks that were enabled by drivers before suspending, but
>> are disabled out of reset/post-transition?
>>
>> Additionally, it would be helpful to see your driver changes and how
>> exactly you plan to use this newly exported function.
>>
>> I've Cc'd Grygorii Strashko from TI. I faintly recall that he had an
>> out-of-tree solution to add some .suspend/.resume callbacks to struct
>> clk_ops. I didn't like that solution at the time either, but maybe we
>> can figure out a more natural way to handle these cases.
>>
>> (Grygorii: sorry in advance if this is a case of mistaken identity. I
>> could not find the code in question from my email archive)
>>
>> Regards,
>> Mike



-- 
regards,
-grygorii

Re: [PATCH] clk: export function clk_disable_unused()

[Michael Turquette]

Quoting Grygorii.Strashko@linaro.org (2015-05-28 08:16:58)
> On 05/28/2015 05:21 PM, Jim Quinlan wrote:
> =

> > We are currently doing [1].  We'd like to change this so that we are
> > the same as upstream barring a small syscore driver that will call
> > clk_disable_unused() on S2/S3 resume.  My patch reflects the
> > suggestion found in [2], which says, "... It could still be generic by
> > exporting the clk_disable_unused() function to drivers...".
> > =

> > We do not have the general problems you are alluding to.  With few
> > exceptions, we do not yet use power domains or runtime PM, although of
> > course this may change.  Our drivers  use either syscore calls or
> > SIMPLE_DEV_PM_OPS() to turn off/on clocks as needed during S2/S3
> > suspend/resume.  The problem we have is when the software stack omits
> > a driver or we have a new HW block which has no driver.  On boot,
> > clk_disable_unused() turns off the clocks that these missing drivers
> > would normally use*.  But on resumption of S2/S3, these clocks get
> > turned on by HW, and all I am asking is that clk_disable_unused() be
> > exported so a driver may invoke it to do the same thing it does during
> > boot.
> =

> This issue is much more simpler then saving/restoring of clocks state, so
> may be it can be solved first.
> =

> Taking to the account comments from [2] I think that could be
> done inside clk core using PM notifiers, =

> and sysfs/module parameter can be used to enable/disable this feature, li=
ke:
>  PM_POST_SUSPEND:
>  PM_POST_HIBERNATION:
>  PM_RESTORE_PREPARE:
>         if (clk_disable_unused_on_suspend)
>                 clk_disable_unused();
> =

> =

> Mike, What do you think?

I've chewed on this a bit and I do not think that the framework core
should be doing this suspend/resume stuff. I think that suspend/resume
should be handled by the drivers, as it usually is.

Regarding clk_disable_unused(), I think that function is causing a
variety of problems today and should be revisited. Instead of exposing
it to drivers or calling it as a resume callback from the framework
core, I think we should consider alternatives.

For example, would it be more useful to have a simple interface that
lets a clock driver query the prepare_count and enable_count of a clock,
and compare that to the physical hardware state? Given a list of clocks
that are "owned" by that clock driver, then it can make the right
decision itself, perhaps in a .resume callback. But the logic to do this
could migrate to clock drivers and out of the framework core.

What information would your clock driver need from the framework core to
be able to handle your use cases?

Regards,
Mike

> =

> > =

> > * Why the HW turns these clocks on in the first place is a question I
> > cannot answer.
> > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December=
/311568.html
> > [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December=
/311575.html
> > =

> > On Sat, May 23, 2015 at 3:33 PM, Michael Turquette
> > <mturquette@linaro.org> wrote:
> >> Quoting Jim Quinlan (2015-05-15 15:22:36)
> >>> For Broadcom STB chips, clocks may come up after resume in an "on"
> >>> state, so calling clk_disable_unused() again will turn off unused
> >>> clocks.  This commit exports clk_disable_unused() so it may be
> >>> called in such cases.
> >>
> >> Jim,
> >>
> >> Thanks for the patch.
> >>
> >> I think a more general solution to the problem might be needed. E.g. h=
ow
> >> do we solve for the opposite case where after a low-power suspend/resu=
me
> >> we turn on clocks that were enabled by drivers before suspending, but
> >> are disabled out of reset/post-transition?
> >>
> >> Additionally, it would be helpful to see your driver changes and how
> >> exactly you plan to use this newly exported function.
> >>
> >> I've Cc'd Grygorii Strashko from TI. I faintly recall that he had an
> >> out-of-tree solution to add some .suspend/.resume callbacks to struct
> >> clk_ops. I didn't like that solution at the time either, but maybe we
> >> can figure out a more natural way to handle these cases.
> >>
> >> (Grygorii: sorry in advance if this is a case of mistaken identity. I
> >> could not find the code in question from my email archive)
> >>
> >> Regards,
> >> Mike
> =

> =

> =

> -- =

> regards,
> -grygorii

Re: [PATCH] clk: export function clk_disable_unused()

[Jim Quinlan]

On Thu, Jul 23, 2015 at 2:32 PM, Michael Turquette
<mturquette@linaro.org> wrote:
> Quoting Grygorii.Strashko@linaro.org (2015-05-28 08:16:58)
>> On 05/28/2015 05:21 PM, Jim Quinlan wrote:
>>
>> > We are currently doing [1].  We'd like to change this so that we are
>> > the same as upstream barring a small syscore driver that will call
>> > clk_disable_unused() on S2/S3 resume.  My patch reflects the
>> > suggestion found in [2], which says, "... It could still be generic by
>> > exporting the clk_disable_unused() function to drivers...".
>> >
>> > We do not have the general problems you are alluding to.  With few
>> > exceptions, we do not yet use power domains or runtime PM, although of
>> > course this may change.  Our drivers  use either syscore calls or
>> > SIMPLE_DEV_PM_OPS() to turn off/on clocks as needed during S2/S3
>> > suspend/resume.  The problem we have is when the software stack omits
>> > a driver or we have a new HW block which has no driver.  On boot,
>> > clk_disable_unused() turns off the clocks that these missing drivers
>> > would normally use*.  But on resumption of S2/S3, these clocks get
>> > turned on by HW, and all I am asking is that clk_disable_unused() be
>> > exported so a driver may invoke it to do the same thing it does during
>> > boot.
>>
>> This issue is much more simpler then saving/restoring of clocks state, so
>> may be it can be solved first.
>>
>> Taking to the account comments from [2] I think that could be
>> done inside clk core using PM notifiers,
>> and sysfs/module parameter can be used to enable/disable this feature, like:
>>  PM_POST_SUSPEND:
>>  PM_POST_HIBERNATION:
>>  PM_RESTORE_PREPARE:
>>         if (clk_disable_unused_on_suspend)
>>                 clk_disable_unused();
>>
>>
>> Mike, What do you think?
>
> I've chewed on this a bit and I do not think that the framework core
> should be doing this suspend/resume stuff. I think that suspend/resume
> should be handled by the drivers, as it usually is.
>
> Regarding clk_disable_unused(), I think that function is causing a
> variety of problems today and should be revisited. Instead of exposing
> it to drivers or calling it as a resume callback from the framework
> core, I think we should consider alternatives.
>
> For example, would it be more useful to have a simple interface that
> lets a clock driver query the prepare_count and enable_count of a clock,
> and compare that to the physical hardware state? Given a list of clocks
> that are "owned" by that clock driver, then it can make the right
> decision itself, perhaps in a .resume callback. But the logic to do this
> could migrate to clock drivers and out of the framework core.
>
> What information would your clock driver need from the framework core to
> be able to handle your use cases?
>
> Regards,
> Mike

Hi Mike,
My position is that if clk_disalbe_unused() exists and it is called in
early boot, then it should be available for invoking on S2/S3 resume.
I see you are questioning its existence and role in the first place.
For our situation -- which we don't yet employ PM domains --  I would
also need a way to iterate through all of the root clocks.
Basically, I would just reconstruct an "outside" version of
clk_disable_unused().  The reason it must be exhaustive is that some
clocks are not be governed by a driver at all for reasons out of our
control.

Thanks,
Jim Quinlan
Broadcom

>
>>
>> >
>> > * Why the HW turns these clocks on in the first place is a question I
>> > cannot answer.
>> > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/311568.html
>> > [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/311575.html
>> >
>> > On Sat, May 23, 2015 at 3:33 PM, Michael Turquette
>> > <mturquette@linaro.org> wrote:
>> >> Quoting Jim Quinlan (2015-05-15 15:22:36)
>> >>> For Broadcom STB chips, clocks may come up after resume in an "on"
>> >>> state, so calling clk_disable_unused() again will turn off unused
>> >>> clocks.  This commit exports clk_disable_unused() so it may be
>> >>> called in such cases.
>> >>
>> >> Jim,
>> >>
>> >> Thanks for the patch.
>> >>
>> >> I think a more general solution to the problem might be needed. E.g. how
>> >> do we solve for the opposite case where after a low-power suspend/resume
>> >> we turn on clocks that were enabled by drivers before suspending, but
>> >> are disabled out of reset/post-transition?
>> >>
>> >> Additionally, it would be helpful to see your driver changes and how
>> >> exactly you plan to use this newly exported function.
>> >>
>> >> I've Cc'd Grygorii Strashko from TI. I faintly recall that he had an
>> >> out-of-tree solution to add some .suspend/.resume callbacks to struct
>> >> clk_ops. I didn't like that solution at the time either, but maybe we
>> >> can figure out a more natural way to handle these cases.
>> >>
>> >> (Grygorii: sorry in advance if this is a case of mistaken identity. I
>> >> could not find the code in question from my email archive)
>> >>
>> >> Regards,
>> >> Mike
>>
>>
>>
>> --
>> regards,
>> -grygorii