[PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Michael Turquette]

This is an alternative solution to Lee's "clk: Provide support for
always-on clocks" series[0].

The first two patches introduce run-time checks to ensure that clock
consumer drivers are respecting the clk.h api. The former patch checks
for prepare and enable imbalances. The latter checks for calls to
clk_put without first disabling and unpreparing the clk.

The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which
prepares and enables a clk at registration-time. The reference counts
(prepare & enable) are transferred to the first clock consumer driver
that clk_get's the clk with this flag set AND calls clk_prepare or
clk_enable.

The net result is that a clock with this flag set will be enabled at
boot and neither the clk_disable_unused garbage collector or the
"sibling clock disables a shared parent" scenario will cause the flagged
clock to be disabled. The first driver to come along and explicitly
claim, prepare and enable this clock will inherit those reference
counts. No change to clock consumer drivers is required for this to
work. Please continue to use the clk.h api properly.

In time this approach can probably replace the CLK_IGNORE_UNUSED flag
and hopefully reduce the number of users of the clk_ignore_unused boot
parameter.

Finally, a quick note on comparing this series to Lee's. I went with the
simplest approach to solve a real problem: preventing critical clocks
from being spuriously disabled at boot, or before a their parent clock
becomes accidentally disabled by a sibling.

All of the other kitchen sink stuff (DT binding, passing the flag back
to the framework when the clock consumer driver calls clk_put) was left
out because I do not see a real use case for it. If one can demonstrate
a real use case (and not a hypothetical one) then this patch series can
be expanded further.

[0] http://lkml.kernel.org/r/<1437570255-21049-1-git-send-email-lee.jones@linaro.org>

Michael Turquette (3):
  clk: per-user clk prepare & enable ref counts
  clk: clk_put WARNs if user has not disabled clk
  clk: introduce CLK_ENABLE_HAND_OFF flag

 drivers/clk/clk.c            | 79 +++++++++++++++++++++++++++++++++++++++++---
 include/linux/clk-provider.h |  3 ++
 2 files changed, 78 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH RFC RFT 1/3] clk: per-user clk prepare & enable ref counts

[Michael Turquette]

This patch adds prepare and enable reference counts for the per-user
handles that clock consumers have for a clock node. This patch warns if
an imbalance occurs while trying to disable or unprepare a clock and
aborts, leaving the hardware unaffected.

Signed-off-by: Michael Turquette <mturquette@baylibre.com>
---
 drivers/clk/clk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 898052e..72feee9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -84,6 +84,8 @@ struct clk {
 	unsigned long min_rate;
 	unsigned long max_rate;
 	struct hlist_node clks_node;
+	unsigned int enable_count;
+	unsigned int prepare_count;
 };
 
 /***           locking             ***/
@@ -600,6 +602,9 @@ void clk_unprepare(struct clk *clk)
 		return;
 
 	clk_prepare_lock();
+	if (WARN_ON(clk->prepare_count == 0))
+		return;
+	clk->prepare_count--;
 	clk_core_unprepare(clk->core);
 	clk_prepare_unlock();
 }
@@ -657,6 +662,7 @@ int clk_prepare(struct clk *clk)
 		return 0;
 
 	clk_prepare_lock();
+	clk->prepare_count++;
 	ret = clk_core_prepare(clk->core);
 	clk_prepare_unlock();
 
@@ -707,6 +713,9 @@ void clk_disable(struct clk *clk)
 		return;
 
 	flags = clk_enable_lock();
+	if (WARN_ON(clk->enable_count == 0))
+		return;
+	clk->enable_count--;
 	clk_core_disable(clk->core);
 	clk_enable_unlock(flags);
 }
@@ -769,6 +778,7 @@ int clk_enable(struct clk *clk)
 		return 0;
 
 	flags = clk_enable_lock();
+	clk->enable_count++;
 	ret = clk_core_enable(clk->core);
 	clk_enable_unlock(flags);
 
-- 
1.9.1

[PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Michael Turquette]

>From the clk_put kerneldoc in include/linux/clk.h:

"""
Note: drivers must ensure that all clk_enable calls made on this clock
source are balanced by clk_disable calls prior to calling this function.
"""

The common clock framework implementation of the clk.h api has per-user
reference counts for calls to clk_prepare and clk_disable. As such it
can enforce the requirement to properly call clk_disable and
clk_unprepare before calling clk_put.

Because this requirement is probably violated in many places, this patch
starts with a simple warning. Once offending code has been fixed this
check could additionally release the reference counts automatically.

Signed-off-by: Michael Turquette <mturquette@baylibre.com>
---
 drivers/clk/clk.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 72feee9..6ec0f77 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk)
 	    clk->max_rate < clk->core->req_rate)
 		clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
 
+	/*
+	 * before calling clk_put, all calls to clk_prepare and clk_enable from
+	 * a given user must be balanced with calls to clk_disable and
+	 * clk_unprepare by that same user
+	 */
+	WARN_ON(clk->prepare_count);
+	WARN_ON(clk->enable_count);
+
 	owner = clk->core->owner;
 	kref_put(&clk->core->ref, __clk_release);
 
-- 
1.9.1

[PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Michael Turquette]

Some clocks are critical to system operation (e.g. cpu, memory, etc) and
should not be gated until a driver that knows best claims such a clock
and expressly gates that clock through the normal clk.h api.

The typical way to handle this is for the clk driver or some other early
code to call clk_prepare_enable on this important clock as soon as it is
registered and before the clk_disable_unused garbage collector kicks in.

This patch introduces a formal way to handle this scenario that is
provided by the clk framework. Clk driver authors can set the
CLK_ENABLE_HAND_OFF flag in their clk data, which will cause the clk to
be enabled in clk_register(). Then when the first clk consumer driver
comes along and calls clk_get() & clk_prepare_enable(), the reference
counts taken during clk registration are transfered (or handed off) to
the clk consumer.

At this point handling the clk is the same as any other clock which as
not set the new CLK_ENABLE_HAND_OFF flag. In fact no changes to any
clock consumer driver are needed for this to work.

Signed-off-by: Michael Turquette <mturquette@baylibre.com>
---
 drivers/clk/clk.c            | 61 +++++++++++++++++++++++++++++++++++++++++---
 include/linux/clk-provider.h |  3 +++
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 6ec0f77..a3fdeab 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -59,6 +59,8 @@ struct clk_core {
 	unsigned long		flags;
 	unsigned int		enable_count;
 	unsigned int		prepare_count;
+	bool			need_handoff_enable;
+	bool			need_handoff_prepare;
 	unsigned long		min_rate;
 	unsigned long		max_rate;
 	unsigned long		accuracy;
@@ -656,16 +658,31 @@ static int clk_core_prepare(struct clk_core *core)
  */
 int clk_prepare(struct clk *clk)
 {
-	int ret;
+	int ret = 0;
 
 	if (!clk)
 		return 0;
 
 	clk_prepare_lock();
 	clk->prepare_count++;
+
+	/*
+	 * setting CLK_ENABLE_HAND_OFF flag triggers this conditional
+	 *
+	 * need_handoff_prepare implies this clk was already prepared by
+	 * __clk_init. now we have a proper user, so unset the flag in our
+	 * internal bookkeeping. See CLK_ENABLE_HAND_OFF flag in clk-provider.h
+	 * for details.
+	 */
+	if (clk->core->need_handoff_prepare) {
+		clk->core->need_handoff_prepare = false;
+		goto out;
+	}
+
 	ret = clk_core_prepare(clk->core);
-	clk_prepare_unlock();
 
+out:
+	clk_prepare_unlock();
 	return ret;
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
@@ -772,16 +789,31 @@ static int clk_core_enable(struct clk_core *core)
 int clk_enable(struct clk *clk)
 {
 	unsigned long flags;
-	int ret;
+	int ret = 0;
 
 	if (!clk)
 		return 0;
 
 	flags = clk_enable_lock();
 	clk->enable_count++;
+
+	/*
+	 * setting CLK_ENABLE_HAND_OFF flag triggers this conditional
+	 *
+	 * need_handoff_enable implies this clk was already enabled by
+	 * __clk_init. now we have a proper user, so unset the flag in our
+	 * internal bookkeeping. See CLK_ENABLE_HAND_OFF flag in clk-provider.h
+	 * for details.
+	 */
+	if (clk->core->need_handoff_enable) {
+		clk->core->need_handoff_enable = false;
+		goto out;
+	}
+
 	ret = clk_core_enable(clk->core);
-	clk_enable_unlock(flags);
 
+out:
+	clk_enable_unlock(flags);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(clk_enable);
@@ -2447,6 +2479,27 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
 	if (core->ops->init)
 		core->ops->init(core->hw);
 
+	/*
+	 * enable clocks with the CLK_ENABLE_HAND_OFF flag set
+	 *
+	 * This flag causes the framework to enable the clock at registration
+	 * time, which is sometimes necessary for clocks that would cause a
+	 * system crash when gated (e.g. cpu, memory, etc). The prepare_count
+	 * is migrated over to the first clk consumer to call clk_prepare().
+	 * Similarly the clk's enable_count is migrated to the first consumer
+	 * to call clk_enable().
+	 */
+	if (core->flags & CLK_ENABLE_HAND_OFF) {
+		core->need_handoff_prepare = true;
+		core->need_handoff_enable = true;
+		ret = clk_core_prepare(core);
+		if (ret)
+			goto out;
+		clk_core_enable(core);
+		if (ret)
+			goto out;
+	}
+
 	kref_init(&core->ref);
 out:
 	clk_prepare_unlock();
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 06a56e5..0230900 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -31,6 +31,9 @@
 #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
 #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
 #define CLK_RECALC_NEW_RATES	BIT(9) /* recalc rates after notifications */
+#define CLK_ENABLE_HAND_OFF	BIT(10) /* enable clock when registered.
+					   hand-off enable_count & prepare_count
+					   to first consumer that enables clk */
 
 struct clk;
 struct clk_hw;
-- 
1.9.1

Re: [PATCH RFC RFT 1/3] clk: per-user clk prepare & enable ref counts

[Maxime Coquelin]

Hi Mike,

On 08/07/2015 09:09 PM, Michael Turquette wrote:
> This patch adds prepare and enable reference counts for the per-user
> handles that clock consumers have for a clock node. This patch warns if
> an imbalance occurs while trying to disable or unprepare a clock and
> aborts, leaving the hardware unaffected.
>
> Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> ---
>   drivers/clk/clk.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 898052e..72feee9 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -84,6 +84,8 @@ struct clk {
>   	unsigned long min_rate;
>   	unsigned long max_rate;
>   	struct hlist_node clks_node;
> +	unsigned int enable_count;
> +	unsigned int prepare_count;
>   };
>   
>   /***           locking             ***/
> @@ -600,6 +602,9 @@ void clk_unprepare(struct clk *clk)
>   		return;
>   
>   	clk_prepare_lock();
> +	if (WARN_ON(clk->prepare_count == 0))
Isn't clk_prepare_unlock()call missing here before return?
> +		return;
> +	clk->prepare_count--;
>   	clk_core_unprepare(clk->core);
>   	clk_prepare_unlock();
>   }
> @@ -657,6 +662,7 @@ int clk_prepare(struct clk *clk)
>   		return 0;
>   
>   	clk_prepare_lock();
> +	clk->prepare_count++;
>   	ret = clk_core_prepare(clk->core);
>   	clk_prepare_unlock();
>   
> @@ -707,6 +713,9 @@ void clk_disable(struct clk *clk)
>   		return;
>   
>   	flags = clk_enable_lock();
> +	if (WARN_ON(clk->enable_count == 0))
Ditto.
> +		return;
> +	clk->enable_count--;
>   	clk_core_disable(clk->core);
>   	clk_enable_unlock(flags);
>   }

Regards,
Maxime

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Lee Jones]

On Fri, 07 Aug 2015, Michael Turquette wrote:

> Some clocks are critical to system operation (e.g. cpu, memory, etc) and
> should not be gated until a driver that knows best claims such a clock
> and expressly gates that clock through the normal clk.h api.
> 
> The typical way to handle this is for the clk driver or some other early
> code to call clk_prepare_enable on this important clock as soon as it is
> registered and before the clk_disable_unused garbage collector kicks in.
> 
> This patch introduces a formal way to handle this scenario that is
> provided by the clk framework. Clk driver authors can set the
> CLK_ENABLE_HAND_OFF flag in their clk data, which will cause the clk to
> be enabled in clk_register(). 

Doesn't this patch put as right back at square one?  We still require
each of the clock providers to know which of its clocks are critical
on any given platform.  Only this time we're setting a flag as opposed
to actually enabling the clock.  The code for doing so will still be
little per-vendor hand-rolled chunks scattered all over the subsystem.

Mitigating this was the whole point of my critical clocks set.  It
appears we're not solving the problem here at all.

> Then when the first clk consumer driver
> comes along and calls clk_get() & clk_prepare_enable(), the reference
> counts taken during clk registration are transfered (or handed off) to
> the clk consumer.
> 
> At this point handling the clk is the same as any other clock which as
> not set the new CLK_ENABLE_HAND_OFF flag. In fact no changes to any
> clock consumer driver are needed for this to work.
> 
> Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> ---
>  drivers/clk/clk.c            | 61 +++++++++++++++++++++++++++++++++++++++++---
>  include/linux/clk-provider.h |  3 +++
>  2 files changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 6ec0f77..a3fdeab 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -59,6 +59,8 @@ struct clk_core {
>  	unsigned long		flags;
>  	unsigned int		enable_count;
>  	unsigned int		prepare_count;
> +	bool			need_handoff_enable;
> +	bool			need_handoff_prepare;
>  	unsigned long		min_rate;
>  	unsigned long		max_rate;
>  	unsigned long		accuracy;
> @@ -656,16 +658,31 @@ static int clk_core_prepare(struct clk_core *core)
>   */
>  int clk_prepare(struct clk *clk)
>  {
> -	int ret;
> +	int ret = 0;
>  
>  	if (!clk)
>  		return 0;
>  
>  	clk_prepare_lock();
>  	clk->prepare_count++;
> +
> +	/*
> +	 * setting CLK_ENABLE_HAND_OFF flag triggers this conditional
> +	 *
> +	 * need_handoff_prepare implies this clk was already prepared by
> +	 * __clk_init. now we have a proper user, so unset the flag in our
> +	 * internal bookkeeping. See CLK_ENABLE_HAND_OFF flag in clk-provider.h
> +	 * for details.
> +	 */
> +	if (clk->core->need_handoff_prepare) {
> +		clk->core->need_handoff_prepare = false;
> +		goto out;
> +	}
> +
>  	ret = clk_core_prepare(clk->core);
> -	clk_prepare_unlock();
>  
> +out:
> +	clk_prepare_unlock();
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(clk_prepare);
> @@ -772,16 +789,31 @@ static int clk_core_enable(struct clk_core *core)
>  int clk_enable(struct clk *clk)
>  {
>  	unsigned long flags;
> -	int ret;
> +	int ret = 0;
>  
>  	if (!clk)
>  		return 0;
>  
>  	flags = clk_enable_lock();
>  	clk->enable_count++;

This insinuates that we now have two users.  Same goes for the prepare
count.

What happens during disable() and unprepare()?

> +	/*
> +	 * setting CLK_ENABLE_HAND_OFF flag triggers this conditional
> +	 *
> +	 * need_handoff_enable implies this clk was already enabled by
> +	 * __clk_init. now we have a proper user, so unset the flag in our
> +	 * internal bookkeeping. See CLK_ENABLE_HAND_OFF flag in clk-provider.h
> +	 * for details.
> +	 */
> +	if (clk->core->need_handoff_enable) {
> +		clk->core->need_handoff_enable = false;
> +		goto out;
> +	}
> +
>  	ret = clk_core_enable(clk->core);
> -	clk_enable_unlock(flags);
>  
> +out:
> +	clk_enable_unlock(flags);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(clk_enable);
> @@ -2447,6 +2479,27 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
>  	if (core->ops->init)
>  		core->ops->init(core->hw);
>  
> +	/*
> +	 * enable clocks with the CLK_ENABLE_HAND_OFF flag set
> +	 *
> +	 * This flag causes the framework to enable the clock at registration
> +	 * time, which is sometimes necessary for clocks that would cause a
> +	 * system crash when gated (e.g. cpu, memory, etc). The prepare_count
> +	 * is migrated over to the first clk consumer to call clk_prepare().
> +	 * Similarly the clk's enable_count is migrated to the first consumer
> +	 * to call clk_enable().
> +	 */
> +	if (core->flags & CLK_ENABLE_HAND_OFF) {
> +		core->need_handoff_prepare = true;
> +		core->need_handoff_enable = true;
> +		ret = clk_core_prepare(core);
> +		if (ret)
> +			goto out;
> +		clk_core_enable(core);
> +		if (ret)
> +			goto out;
> +	}
> +
>  	kref_init(&core->ref);
>  out:
>  	clk_prepare_unlock();
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 06a56e5..0230900 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -31,6 +31,9 @@
>  #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
>  #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
>  #define CLK_RECALC_NEW_RATES	BIT(9) /* recalc rates after notifications */
> +#define CLK_ENABLE_HAND_OFF	BIT(10) /* enable clock when registered.
> +					   hand-off enable_count & prepare_count
> +					   to first consumer that enables clk */
>  
>  struct clk;
>  struct clk_hw;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Lee Jones]

On Fri, 07 Aug 2015, Michael Turquette wrote:
> This is an alternative solution to Lee's "clk: Provide support for
> always-on clocks" series[0].
 
So after all the hours of work that's been put in and all of the
versions that have been authored, you're just going to write your own
solution anyway, without any further discussion?

That's one way to make your contributors feel valued.

> The first two patches introduce run-time checks to ensure that clock
> consumer drivers are respecting the clk.h api. The former patch checks
> for prepare and enable imbalances. The latter checks for calls to
> clk_put without first disabling and unpreparing the clk.

Are these real issues that people are having trouble with, or just
hypothetical ones?  I can understand the need for them if they're
causing people some pain, but if they are unnecessarily hardening the
API, then it makes it difficult for proper issues (such as critical
clocks) to be solved easily.

> The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which
> prepares and enables a clk at registration-time. The reference counts
> (prepare & enable) are transferred to the first clock consumer driver
> that clk_get's the clk with this flag set AND calls clk_prepare or
> clk_enable.
> 
> The net result is that a clock with this flag set will be enabled at
> boot and neither the clk_disable_unused garbage collector or the
> "sibling clock disables a shared parent" scenario will cause the flagged
> clock to be disabled.

I'm not sure if the third patch actually solves the _real_ issue you
allude to here.  The original critical (then called always-on) clock
patch-set solved it just fine.  However others were concerned about
how you would then turn the clock off if some knowledgeable consumer
(who knew the full consequences of their actions) came along and had a
good reason to gate it.  That, the turning off the clock if still
desired is what the third patch _actually_ solves.

Now we are back where we started however, and still need to figure out
a generic method to mark the clocks (either by setting a FLAG or
actually calling enable()) as critical.

> The first driver to come along and explicitly
> claim, prepare and enable this clock will inherit those reference
> counts. No change to clock consumer drivers is required for this to
> work.

Do you mean it won't break anything?  I think to make use of the
functionality each of the providers still have to figure out which of
their clocks are critical (which may change from platform to platform)
then mark them as such before registration.

> Please continue to use the clk.h api properly.
> 
> In time this approach can probably replace the CLK_IGNORE_UNUSED flag
> and hopefully reduce the number of users of the clk_ignore_unused boot
> parameter.
> 
> Finally, a quick note on comparing this series to Lee's. I went with the
> simplest approach to solve a real problem: preventing critical clocks
> from being spuriously disabled at boot, or before a their parent clock
> becomes accidentally disabled by a sibling.

This wasn't the problem.  Platforms are already doing this.  The _real_
problem was doing it in a generic way, so vendors didn't have to roll
their own 'gather' and 'mark' code, which unfortunately they still have
to do with this solution.

> All of the other kitchen sink stuff (DT binding, 

The DT binding will still be required, at least for ST, as they have
gone for the more Linuxy approach of having a generic set of clock
drivers and provide all of the platform specific information in
platform code (i.e. DT).  So to identify which clocks are critical on
each platform the DT will need to be interrogated in some way.

> passing the flag back to the framework when the clock consumer
> driver calls clk_put) was left

I'm not sure what this is.

> out because I do not see a real use case for it. If one can demonstrate
> a real use case (and not a hypothetical one) then this patch series can
> be expanded further.
> 
> [0] http://lkml.kernel.org/r/<1437570255-21049-1-git-send-email-lee.jones@linaro.org>
> 
> Michael Turquette (3):
>   clk: per-user clk prepare & enable ref counts
>   clk: clk_put WARNs if user has not disabled clk
>   clk: introduce CLK_ENABLE_HAND_OFF flag
> 
>  drivers/clk/clk.c            | 79 +++++++++++++++++++++++++++++++++++++++++---
>  include/linux/clk-provider.h |  3 ++
>  2 files changed, 78 insertions(+), 4 deletions(-)
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 1/3] clk: per-user clk prepare & enable ref counts

[Michael Turquette]

Quoting Maxime Coquelin (2015-08-10 06:47:51)
> Hi Mike,
> 
> On 08/07/2015 09:09 PM, Michael Turquette wrote:
> > This patch adds prepare and enable reference counts for the per-user
> > handles that clock consumers have for a clock node. This patch warns if
> > an imbalance occurs while trying to disable or unprepare a clock and
> > aborts, leaving the hardware unaffected.
> >
> > Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> > ---
> >   drivers/clk/clk.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 898052e..72feee9 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -84,6 +84,8 @@ struct clk {
> >       unsigned long min_rate;
> >       unsigned long max_rate;
> >       struct hlist_node clks_node;
> > +     unsigned int enable_count;
> > +     unsigned int prepare_count;
> >   };
> >   
> >   /***           locking             ***/
> > @@ -600,6 +602,9 @@ void clk_unprepare(struct clk *clk)
> >               return;
> >   
> >       clk_prepare_lock();
> > +     if (WARN_ON(clk->prepare_count == 0))
> Isn't clk_prepare_unlock()call missing here before return?

Doh! Good catch.

Thanks,
Mike

> > +             return;
> > +     clk->prepare_count--;
> >       clk_core_unprepare(clk->core);
> >       clk_prepare_unlock();
> >   }
> > @@ -657,6 +662,7 @@ int clk_prepare(struct clk *clk)
> >               return 0;
> >   
> >       clk_prepare_lock();
> > +     clk->prepare_count++;
> >       ret = clk_core_prepare(clk->core);
> >       clk_prepare_unlock();
> >   
> > @@ -707,6 +713,9 @@ void clk_disable(struct clk *clk)
> >               return;
> >   
> >       flags = clk_enable_lock();
> > +     if (WARN_ON(clk->enable_count == 0))
> Ditto.
> > +             return;
> > +     clk->enable_count--;
> >       clk_core_disable(clk->core);
> >       clk_enable_unlock(flags);
> >   }
> 
> Regards,
> Maxime
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Lee Jones]

On Mon, 10 Aug 2015, Michael Turquette wrote:
> Quoting Lee Jones (2015-08-10 07:48:11)
> > On Fri, 07 Aug 2015, Michael Turquette wrote:
> > 
> > > Some clocks are critical to system operation (e.g. cpu, memory, etc) and
> > > should not be gated until a driver that knows best claims such a clock
> > > and expressly gates that clock through the normal clk.h api.
> > > 
> > > The typical way to handle this is for the clk driver or some other early
> > > code to call clk_prepare_enable on this important clock as soon as it is
> > > registered and before the clk_disable_unused garbage collector kicks in.
> > > 
> > > This patch introduces a formal way to handle this scenario that is
> > > provided by the clk framework. Clk driver authors can set the
> > > CLK_ENABLE_HAND_OFF flag in their clk data, which will cause the clk to
> > > be enabled in clk_register(). 
> > 
> > Doesn't this patch put as right back at square one?  We still require
> > each of the clock providers to know which of its clocks are critical
> > on any given platform.  Only this time we're setting a flag as opposed
> > to actually enabling the clock.  The code for doing so will still be
> > little per-vendor hand-rolled chunks scattered all over the subsystem.
> 
> This is conceptually analogous to what your "ARM: sti: stih410-clocks:
> Identify critical clocks" patch does. In that patch you mark-up the
> critical clocks within the *provider* node.
> 
> My patch puts that data in the clock *provider* driver, where it
> belongs.
> 
> ST's driver is an unfortunate case. All of the clock data was shoved
> into DT before we had a clue that doing so is a terrible idea.
> Thankfully such platforms are a minority. For the sane clock driver,
> with static clock data in the kernel source, it is trivial to set the
> CLK_ENABLE_HAND_OFF flag for a given clock.
> 
> All that hand-wavey crap about "little per-vendor hand-rolled chunks
> scattered all over the system" is FUD and a waste of everyone's time.

Given most of the "ST's driver is an unfortunate case ..." paragraph
above is mostly FUD, I guess it's a suitable tactic to use here.

Actually, what I said wasn't designed to be FUD, it was based on my
understanding due to "hacking on a platform that uses Device Tree"
(... as a suitable means to pass platform specific driver data, which
is what it was designed to do).

> Mitigating this was the whole point of my critical clocks set.  It
> > appears we're not solving the problem here at all.
> 
> This series is solving the following problems:
> 
> 1) enabling specified clocks at boot
> 2) preventing those clocks from being gated by clk_disable_unused

The original patch-set did this just fine.

> 3) gracefully handing off the reference counts to clock consumer drivers

This wasn't required at the time I authored my set.  Do bear in mind
that my set was written before you informed us of your per-user
"vaporware".  With that knowledge I would have authored a different
approach.

> 4) not bastardizing the clk.h api or otherwise making an ugly mess of
> things

My set was based on a method which you'd already reviewed, accepted
and applied multiple times.  The API which provided the ability to
over-ride a critical clock's always-on behaviour was only in first
version.  I took your point about not clk_disable_critical()ing before
clk_enable_critical()ing and was going to do something about it.

> If you mean to say, "this patch doesn't let me toss this data in
> Devicetree, a data orifice that is used by only a fraction of Linux
> kernel users" then you would be right.

A fraction of Linux kernel users, yes, but the majority (all?) of
the Clock Framework users do use DT.

What I mean to say is; this solution isn't as generic as I initially
planned.  Yes, my first submission only supported DT, but I planned
on a second patch-set.  The subsequent set would have supported all
the other drivers which have all their platform data in the driver
too.  This solution however, leads ST out in the cold and "back to
square one".

> Here is a diff of how to do this with a sane clock driver:

sane; arguable, FUD.

> diff --git a/drivers/clk/qcom/gcc-apq8084.c b/drivers/clk/qcom/gcc-apq8084.c
> index 3563019..d2f5e5a 100644
> --- a/drivers/clk/qcom/gcc-apq8084.c
> +++ b/drivers/clk/qcom/gcc-apq8084.c
> @@ -1450,23 +1450,23 @@ static struct clk_branch gcc_blsp1_qup1_spi_apps_clk = {
>  static struct clk_branch gcc_blsp1_qup2_i2c_apps_clk = {
>         .halt_reg = 0x06c8,
>         .clkr = {
>                 .enable_reg = 0x06c8,
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_blsp1_qup2_i2c_apps_clk",
>                         .parent_names = (const char *[]){
>                                 "blsp1_qup2_i2c_apps_clk_src",
>                         },
>                         .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
> +                       .flags = CLK_SET_RATE_PARENT | CLK_ENABLE_HAND_OFF,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
>  };

Fair enough.  Obviously for anyone using Device Tree, this solution
makes it pretty difficult to partake.

> The real problem that has been informing all of your design decisions is
> that you are hacking on a platform that uses Devicetree as a data-driven
> interface to the kernel, which it most certainly is not.  We're are not
> tossing SoC RTL in there after all, nor replicating the register map
> from a reference manual. The data that your clock provider uses to build
> up its contribution to the system-wide clock tree belongs in your Linux
> device driver.
> 
> Devicetree is most useful in creating connections between providers and
> consumers of these resources, not in defining the resource itself.

We'll have to agree to disagree here I think.  Perhaps I am lacking
some information (which is likely), but I still don't see why the
Clock Framework is 'special'.  For every other subsystem, platform
data is described in the platform data area (LINUX/arch/) and passed
into drivers.  In the Clock Framework however, you encourage shoving
large, platform specific data structures right into drivers.

> > > Then when the first clk consumer driver
> > > comes along and calls clk_get() & clk_prepare_enable(), the reference
> > > counts taken during clk registration are transfered (or handed off) to
> > > the clk consumer.
> > > 
> > > At this point handling the clk is the same as any other clock which as
> > > not set the new CLK_ENABLE_HAND_OFF flag. In fact no changes to any
> > > clock consumer driver are needed for this to work.
> > > 
> > > Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> > > ---
> > >  drivers/clk/clk.c            | 61 +++++++++++++++++++++++++++++++++++++++++---
> > >  include/linux/clk-provider.h |  3 +++
> > >  2 files changed, 60 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 6ec0f77..a3fdeab 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -59,6 +59,8 @@ struct clk_core {
> > >       unsigned long           flags;
> > >       unsigned int            enable_count;
> > >       unsigned int            prepare_count;
> > > +     bool                    need_handoff_enable;
> > > +     bool                    need_handoff_prepare;
> > >       unsigned long           min_rate;
> > >       unsigned long           max_rate;
> > >       unsigned long           accuracy;
> > > @@ -656,16 +658,31 @@ static int clk_core_prepare(struct clk_core *core)
> > >   */
> > >  int clk_prepare(struct clk *clk)
> > >  {
> > > -     int ret;
> > > +     int ret = 0;
> > >  
> > >       if (!clk)
> > >               return 0;
> > >  
> > >       clk_prepare_lock();
> > >       clk->prepare_count++;
> > > +
> > > +     /*
> > > +      * setting CLK_ENABLE_HAND_OFF flag triggers this conditional
> > > +      *
> > > +      * need_handoff_prepare implies this clk was already prepared by
> > > +      * __clk_init. now we have a proper user, so unset the flag in our
> > > +      * internal bookkeeping. See CLK_ENABLE_HAND_OFF flag in clk-provider.h
> > > +      * for details.
> > > +      */
> > > +     if (clk->core->need_handoff_prepare) {
> > > +             clk->core->need_handoff_prepare = false;
> > > +             goto out;
> > > +     }
> > > +
> > >       ret = clk_core_prepare(clk->core);
> > > -     clk_prepare_unlock();
> > >  
> > > +out:
> > > +     clk_prepare_unlock();
> > >       return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_prepare);
> > > @@ -772,16 +789,31 @@ static int clk_core_enable(struct clk_core *core)
> > >  int clk_enable(struct clk *clk)
> > >  {
> > >       unsigned long flags;
> > > -     int ret;
> > > +     int ret = 0;
> > >  
> > >       if (!clk)
> > >               return 0;
> > >  
> > >       flags = clk_enable_lock();
> > >       clk->enable_count++;
> > 
> > This insinuates that we now have two users.  Same goes for the prepare
> > count.
> 
> Wrong. After this statement struct clk.enable_count will be 1. Remember
> that we have a struct clk.enable_count as well as a struct
> clk_core.enable_count.
> 
> The former is set here in clk_enable() which is ONLY called by clock
> consumer drivers (though the clk.h api). The latter is used within the
> framework during __clk_init() with a call to clk_core_enable() if the
> CLK_ENABLE_HAND_OFF flag is detected.

Okay, thanks for the explanation.

> > What happens during disable() and unprepare()?
> 
> The reference counts go to zero. As I stated in my cover letter, I'll
> need to see evidence of a real use case where the "leave the clock on on
> when I call clk_disable, clk_unprepare and clk_put" behavior is
> warranted.

I can't say for sure (get-out clause), but I doubt we'd need that, as
this would only be required if a knowledgeable consumer existed
i.e. one which actually wanted to the disable critical clock.  On ST's
platforms I don't think there is a use-case for these clocks to ever
be gated, as the platform would be unrecoverable and require a reboot.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Lee Jones]

On Mon, 10 Aug 2015, Michael Turquette wrote:
> Quoting Lee Jones (2015-08-10 08:36:38)
> > On Fri, 07 Aug 2015, Michael Turquette wrote:
> > > This is an alternative solution to Lee's "clk: Provide support for
> > > always-on clocks" series[0].
> >  
> > So after all the hours of work that's been put in and all of the
> > versions that have been authored, you're just going to write your own
> > solution anyway, without any further discussion?
> 
> This certainly seems like "further discussion" to me. In fact the
> capital C in RFC pretty much announces that this is a big, wide open
> discussion.
> 
> If I had ninja-merged the patches then I would understand your
> frustration, but that clearly is not the case.

This doesn't have anything to do with merging code.

When contributors submit patches to subsystem I maintain, I provide
reviews, advice and guidance.  If I think an approach is wrong, I
state why I think that and provide ideas for an alternative approach
if required.  If instead of providing feedback I just replied with a
patch-set containing my own alternative method, I would expect the
original submitter to be justifiably cross.

> > That's one way to make your contributors feel valued.
> 
> Lee, your contributions are very much valued. You and I both know that
> there are times when a concept is far better communicated with code than
> with prose. This is one of those times. The DT-centric solution that
> requires a clock consumer driver to call clk_disable() before calling
> clk_enable() (an obvious violation of the clk.h api) is just plain wrong
> and this implementation hopes to get it back on the right course.

See my previous email about what I think about "bastardizing the clk.h
api".

> > > The first two patches introduce run-time checks to ensure that clock
> > > consumer drivers are respecting the clk.h api. The former patch checks
> > > for prepare and enable imbalances. The latter checks for calls to
> > > clk_put without first disabling and unpreparing the clk.
> > 
> > Are these real issues that people are having trouble with, or just
> > hypothetical ones?  I can understand the need for them if they're
> > causing people some pain, but if they are unnecessarily hardening the
> > API, then it makes it difficult for proper issues (such as critical
> > clocks) to be solved easily.
> 
> They are deeply related. Per-user accounting gives us a graceful method
> to implement hand-off, which is the real feature that is needed here.

The hand-off feature was mentioned a week ago, as part of an 8 month
discussion.  This only aids knowledgeable consumers to turn of a
critical clock after the introduction of the API tightening which
happens in _this_ set.  So I disagree that this is the "real feature
that is needed here".

The _real_ feature that's required is a generic way to keep critical
clocks enabled and a way for them to be gated if another driver knows
best.  Granted this method is pretty clean for drivers which have all
of their platform data in driver code, but it's still not ideal.

> > > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which
> > > prepares and enables a clk at registration-time. The reference counts
> > > (prepare & enable) are transferred to the first clock consumer driver
> > > that clk_get's the clk with this flag set AND calls clk_prepare or
> > > clk_enable.
> > > 
> > > The net result is that a clock with this flag set will be enabled at
> > > boot and neither the clk_disable_unused garbage collector or the
> > > "sibling clock disables a shared parent" scenario will cause the flagged
> > > clock to be disabled.
> > 
> > I'm not sure if the third patch actually solves the _real_ issue you
> > allude to here.  The original critical (then called always-on) clock
> > patch-set solved it just fine.  However others were concerned about
> > how you would then turn the clock off if some knowledgeable consumer
> > (who knew the full consequences of their actions) came along and had a
> > good reason to gate it.  That, the turning off the clock if still
> > desired is what the third patch _actually_ solves.
> > 
> > Now we are back where we started however, and still need to figure out
> > a generic method to mark the clocks (either by setting a FLAG or
> > actually calling enable()) as critical.
> 
> The third patch:
> 
> 1) implements an always-on behavior
> 2) allows knowledgeable drivers to gate the clock
> 3) marks such clocks with a flag
> 
> What is missing?

The difficuly is marking the clock in the first place.  We already
have drivers doing 1), just not in a generic way.  This solution is
more generic (as was mine), but it's still not completely generic.

2) is a new requirement that would have required a little more
thought.  My solution to this was only in V1/RFC and admitted would
have required a little more discussion.

3) isn't really a 'thing'.

> > > The first driver to come along and explicitly
> > > claim, prepare and enable this clock will inherit those reference
> > > counts. No change to clock consumer drivers is required for this to
> > > work.
> > 
> > Do you mean it won't break anything?  I think to make use of the
> > functionality each of the providers still have to figure out which of
> > their clocks are critical (which may change from platform to platform)
> > then mark them as such before registration.
> 
> What I meant was: "does not require any new api such as
> clk_critical_enable() or clk_critical_disable()". There is zero change
> to clk.h as a part of this series, which is a good thing.

Okay, fine.

> > > Please continue to use the clk.h api properly.
> > > 
> > > In time this approach can probably replace the CLK_IGNORE_UNUSED flag
> > > and hopefully reduce the number of users of the clk_ignore_unused boot
> > > parameter.
> > > 
> > > Finally, a quick note on comparing this series to Lee's. I went with the
> > > simplest approach to solve a real problem: preventing critical clocks
> > > from being spuriously disabled at boot, or before a their parent clock
> > > becomes accidentally disabled by a sibling.
> > 
> > This wasn't the problem.  Platforms are already doing this.  The _real_
> 
> Yes, this is a very real problem. In fact it is two very real problems:
> 
> 1) Platforms are open-coding this "always-on" behavior by calling
> clk_prepare_enable a bunch in their clock provider driver. I'm still OK
> with that, but this flag provides a coherent way to do it. And,
> 
> 2) Drivers that open-code calls to clk_prepare_enable in their clock
> provider driver have no way to allow clock knowledgeable consumer
> drivers to gate those clocks later on.
> 
> > problem was doing it in a generic way, so vendors didn't have to roll
> > their own 'gather' and 'mark' code, which unfortunately they still have
> > to do with this solution.
> 
> There is no gather. The data for clocks belongs in most drivers, just
> not yours. Marking is as simple as setting a flag (which many drives
> already do). Please see my response to you in patch #3 for an example.

I have also replied in #3.  No need for me to labour the points here
too.

> > > All of the other kitchen sink stuff (DT binding, 
> > 
> > The DT binding will still be required, at least for ST, as they have
> > gone for the more Linuxy approach of having a generic set of clock
> > drivers and provide all of the platform specific information in
> > platform code (i.e. DT).  So to identify which clocks are critical on
> > each platform the DT will need to be interrogated in some way.
> 
> At the risk of instigating a serious religious conflict, I humbly
> disagree that using Devicetree as a data-driven interface for device
> drivers is the "more Linuxy approach". I think that the Devicetree
> people would disagree with you too.
> 
> The more Linuxy approach is for device drivers to contain all of the
> information they need to function.  Devicetree does a stellar job of
> linking up providers and consumers of these resources, which is outside
> the purview of individual drivers.

I agree to disagree.

> > > passing the flag back to the framework when the clock consumer
> > > driver calls clk_put) was left
> > 
> > I'm not sure what this is.
> > 
> > > out because I do not see a real use case for it. If one can demonstrate
> > > a real use case (and not a hypothetical one) then this patch series can
> > > be expanded further.
> > > 
> > > [0] http://lkml.kernel.org/r/<1437570255-21049-1-git-send-email-lee.jones@linaro.org>
> > > 
> > > Michael Turquette (3):
> > >   clk: per-user clk prepare & enable ref counts
> > >   clk: clk_put WARNs if user has not disabled clk
> > >   clk: introduce CLK_ENABLE_HAND_OFF flag
> > > 
> > >  drivers/clk/clk.c            | 79 +++++++++++++++++++++++++++++++++++++++++---
> > >  include/linux/clk-provider.h |  3 ++
> > >  2 files changed, 78 insertions(+), 4 deletions(-)
> > > 
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Geert Uytterhoeven]

Hi Mike,

On Fri, Aug 7, 2015 at 9:09 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> This is an alternative solution to Lee's "clk: Provide support for
> always-on clocks" series[0].
>
> The first two patches introduce run-time checks to ensure that clock
> consumer drivers are respecting the clk.h api. The former patch checks
> for prepare and enable imbalances. The latter checks for calls to
> clk_put without first disabling and unpreparing the clk.
>
> The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which
> prepares and enables a clk at registration-time. The reference counts
> (prepare & enable) are transferred to the first clock consumer driver
> that clk_get's the clk with this flag set AND calls clk_prepare or
> clk_enable.
>
> The net result is that a clock with this flag set will be enabled at
> boot and neither the clk_disable_unused garbage collector or the
> "sibling clock disables a shared parent" scenario will cause the flagged
> clock to be disabled. The first driver to come along and explicitly
> claim, prepare and enable this clock will inherit those reference
> counts. No change to clock consumer drivers is required for this to
> work. Please continue to use the clk.h api properly.
>
> In time this approach can probably replace the CLK_IGNORE_UNUSED flag
> and hopefully reduce the number of users of the clk_ignore_unused boot
> parameter.

Thanks for your series!

I gave it a try on r8a7791/koelsch, where I replaced the hack from
"[PATCH/RFC 1/5] clk: shmobile: mstp: Never disable INTC-SYS"
(http://www.spinics.net/lists/linux-sh/msg41107.html) by setting

        init.flags |= CLK_ENABLE_HAND_OFF

in cpg_mstp_clock_register() for "intc-sys".

The end result is fine (the "intc-sys" clock is never disabled), but I get
a few annoying lockdep splats like below (one for the "intc-sys" clock,
and one more for each parent up to the root clock):

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at drivers/clk/clk.c:745 clk_core_enable+0x6c/0xdc()
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.2.0-rc6-koelsch-04462-g27bac5e25174da01-dirty #1507
Hardware name: Generic R8A7791 (Flattened Device Tree)
Backtrace:
[<c00138b4>] (dump_backtrace) from [<c0013aac>] (show_stack+0x18/0x1c)
 r6:c05c249d r5:00000009 r4:00000000 r3:00200000
[<c0013a94>] (show_stack) from [<c045fea4>] (dump_stack+0x78/0x94)
[<c045fe2c>] (dump_stack) from [<c002ba18>] (warn_slowpath_common+0x90/0xbc)
 r4:00000000 r3:00000000
[<c002b988>] (warn_slowpath_common) from [<c002bae8>]
(warn_slowpath_null+0x24/0x2c)
 r8:eec10d00 r7:00000001 r6:eec0f8c0 r5:00000000 r4:eec0f8c0
[<c002bac4>] (warn_slowpath_null) from [<c0344fbc>] (clk_core_enable+0x6c/0xdc)
[<c0344f50>] (clk_core_enable) from [<c0346e44>] (clk_register+0x504/0x62c)
 r5:00000000 r4:eec0f8c0
[<c0346940>] (clk_register) from [<c0625cd8>] (cpg_mstp_clocks_init+0x240/0x310)
 r10:c05c2f26 r9:00000008 r8:eec10d00 r7:c0ff3755 r6:eec0f7c0 r5:ef1df1cc
 r4:eec09080
[<c0625a98>] (cpg_mstp_clocks_init) from [<c0624f7c>] (of_clk_init+0xe0/0x188)
 r10:00000002 r9:eec09100 r8:eec09108 r7:00000001 r6:eec09140 r5:c0643f48
 r4:00000000
[<c0624e9c>] (of_clk_init) from [<c060ea20>] (rcar_gen2_timer_init+0x108/0x120)
 r10:c0633ae4 r9:c0644400 r8:ffffffff r7:00000000 r6:ef7fca00 r5:f0006000
 r4:00989680
[<c060e918>] (rcar_gen2_timer_init) from [<c0609334>] (time_init+0x24/0x38)
 r5:c067c000 r4:00000000
[<c0609310>] (time_init) from [<c0606bb0>] (start_kernel+0x268/0x378)
[<c0606948>] (start_kernel) from [<40008090>] (0x40008090)
 r10:00000000 r9:413fc0f2 r8:40007000 r7:c0647fe8 r6:c0633ae0 r5:c0644480
 r4:c067c394
---[ end trace cb88537fdc8fa200 ]---

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Maxime Coquelin]

Hi Mike,

On 08/11/2015 10:43 AM, Lee Jones wrote:
> On Mon, 10 Aug 2015, Michael Turquette wrote:
>
>>
>>
>> ST's driver is an unfortunate case. All of the clock data was shoved
>> into DT before we had a clue that doing so is a terrible idea.

I tend to agree, and wouldn't do it this way if we could rewrite the 
history.
But now, we have to support it.

How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi 
platform?

Could we imagine having a kind of "clocks-enable-hand-off" property we 
could use in our clock controller DT node?

Regards,
Maxime

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Geert Uytterhoeven]

Hi Maxime,

On Tue, Aug 11, 2015 at 12:02 PM, Maxime Coquelin
<maxime.coquelin@st.com> wrote:
> How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi
> platform?

Add the flag to the relevant clocks in the C code, e.g. in
clk_register_flexgen():

        if (!strcmp(name, "clk-icn-cpu"))
                init.flags |= CLK_ENABLE_HAND_OFF;

> Could we imagine having a kind of "clocks-enable-hand-off" property we could
> use in our clock controller DT node?

You can imagine doing "flex_flags |= CLK_ENABLE_HAND_OFF" in
st_of_flexgen_setup(), depending on the presence of such a property.

However, not disabling clocks is a software policy, not a hardware description,
so IMHO it doesn't belong in DT.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Maxime Coquelin]

Hi Geert,

On 08/11/2015 12:11 PM, Geert Uytterhoeven wrote:
> Hi Maxime,
>
> On Tue, Aug 11, 2015 at 12:02 PM, Maxime Coquelin
> <maxime.coquelin@st.com> wrote:
>> How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi
>> platform?
> Add the flag to the relevant clocks in the C code, e.g. in
> clk_register_flexgen():
>
>          if (!strcmp(name, "clk-icn-cpu"))
>                  init.flags |= CLK_ENABLE_HAND_OFF;
The main problem I see with this proposal
>
>> Could we imagine having a kind of "clocks-enable-hand-off" property we could
>> use in our clock controller DT node?
> You can imagine doing "flex_flags |= CLK_ENABLE_HAND_OFF" in
> st_of_flexgen_setup(), depending on the presence of such a property.
Exactly, this is what I was thinking about.

>
> However, not disabling clocks is a software policy, not a hardware description,
> so IMHO it doesn't belong in DT.
>
I disagree here because if these clocks get gated the system is dead, so 
I wouldn't call this a SW Policy.
Moreover, I don't see how this property is different from 
assigned-clock-parents and assigned-clock-rates properties, which have 
been accepted.

Thanks,
Maxime

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Maxime Coquelin]

Sorry Geert, my reply has been cut:

On 08/11/2015 01:36 PM, Maxime Coquelin wrote:
> Hi Geert,
>
> On 08/11/2015 12:11 PM, Geert Uytterhoeven wrote:
>> Hi Maxime,
>>
>> On Tue, Aug 11, 2015 at 12:02 PM, Maxime Coquelin
>> <maxime.coquelin@st.com> wrote:
>>> How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi
>>> platform?
>> Add the flag to the relevant clocks in the C code, e.g. in
>> clk_register_flexgen():
>>
>>          if (!strcmp(name, "clk-icn-cpu"))
>>                  init.flags |= CLK_ENABLE_HAND_OFF;

The main problem I see with this proposal is that clk_register_flexgen() 
is called for several SoCs (STiH407/410/418...).
Each of these SoCs have this clock, but maybe STiH407 will need the 
flag, but not STiH410 and STiH418.
So I think the best place to set this information is in DT, where the 
differentiation is made between the SoCs.

Kind regards,
Maxime

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Geert Uytterhoeven]

Hi Maxime,

On Tue, Aug 11, 2015 at 1:41 PM, Maxime Coquelin <maxime.coquelin@st.com> wrote:
> On 08/11/2015 01:36 PM, Maxime Coquelin wrote:
>> On 08/11/2015 12:11 PM, Geert Uytterhoeven wrote:
>>> On Tue, Aug 11, 2015 at 12:02 PM, Maxime Coquelin
>>> <maxime.coquelin@st.com> wrote:
>>>> How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi
>>>> platform?
>>>
>>> Add the flag to the relevant clocks in the C code, e.g. in
>>> clk_register_flexgen():
>>>
>>>          if (!strcmp(name, "clk-icn-cpu"))
>>>                  init.flags |= CLK_ENABLE_HAND_OFF;
>
> The main problem I see with this proposal is that clk_register_flexgen() is
> called for several SoCs (STiH407/410/418...).
> Each of these SoCs have this clock, but maybe STiH407 will need the flag,
> but not STiH410 and STiH418.
> So I think the best place to set this information is in DT, where the
> differentiation is made between the SoCs.

If (of_machine_is_compatible("st,stih410")) ...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Maxime Coquelin]

On 08/11/2015 01:49 PM, Geert Uytterhoeven wrote:
> Hi Maxime,
>
> On Tue, Aug 11, 2015 at 1:41 PM, Maxime Coquelin <maxime.coquelin@st.com> wrote:
>> On 08/11/2015 01:36 PM, Maxime Coquelin wrote:
>>> On 08/11/2015 12:11 PM, Geert Uytterhoeven wrote:
>>>> On Tue, Aug 11, 2015 at 12:02 PM, Maxime Coquelin
>>>> <maxime.coquelin@st.com> wrote:
>>>>> How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi
>>>>> platform?
>>>> Add the flag to the relevant clocks in the C code, e.g. in
>>>> clk_register_flexgen():
>>>>
>>>>           if (!strcmp(name, "clk-icn-cpu"))
>>>>                   init.flags |= CLK_ENABLE_HAND_OFF;
>> The main problem I see with this proposal is that clk_register_flexgen() is
>> called for several SoCs (STiH407/410/418...).
>> Each of these SoCs have this clock, but maybe STiH407 will need the flag,
>> but not STiH410 and STiH418.
>> So I think the best place to set this information is in DT, where the
>> differentiation is made between the SoCs.
> If (of_machine_is_compatible("st,stih410")) ...
>
It works, but is it really what we want?
Each time we will add a new soc, we will have to patch this SoC agnostic 
function?
With the number of SoCs and the number of clocks, it will be a nightmare 
to maintain and debug, no?

Regards,
Maxime

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Lee Jones]

On Tue, 11 Aug 2015, Geert Uytterhoeven wrote:

> Hi Maxime,
> 
> On Tue, Aug 11, 2015 at 1:41 PM, Maxime Coquelin <maxime.coquelin@st.com> wrote:
> > On 08/11/2015 01:36 PM, Maxime Coquelin wrote:
> >> On 08/11/2015 12:11 PM, Geert Uytterhoeven wrote:
> >>> On Tue, Aug 11, 2015 at 12:02 PM, Maxime Coquelin
> >>> <maxime.coquelin@st.com> wrote:
> >>>> How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi
> >>>> platform?
> >>>
> >>> Add the flag to the relevant clocks in the C code, e.g. in
> >>> clk_register_flexgen():
> >>>
> >>>          if (!strcmp(name, "clk-icn-cpu"))
> >>>                  init.flags |= CLK_ENABLE_HAND_OFF;
> >
> > The main problem I see with this proposal is that clk_register_flexgen() is
> > called for several SoCs (STiH407/410/418...).
> > Each of these SoCs have this clock, but maybe STiH407 will need the flag,
> > but not STiH410 and STiH418.
> > So I think the best place to set this information is in DT, where the
> > differentiation is made between the SoCs.
> 
> If (of_machine_is_compatible("st,stih410")) ...

This is getting very messy.

Ideally we'd like to keep platform code out of device drivers.
Critical clock description belongs in DT for our use-case.  We can
write code to extract the information from there and set the flag is
Mike's solution is deemed appropriate.

With regards to your "Software Policy Vs Hardware Description"
comment; we already have 10's of "Software Policy" bindings which do
not describe hardware in the purest sense; frequency specifications,
line/voltage levels, GPIO configuration, the list goes on.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Geert Uytterhoeven]

On Tue, Aug 11, 2015 at 2:03 PM, Maxime Coquelin <maxime.coquelin@st.com> wrote:
> On 08/11/2015 01:49 PM, Geert Uytterhoeven wrote:
>> On Tue, Aug 11, 2015 at 1:41 PM, Maxime Coquelin <maxime.coquelin@st.com>
>> wrote:
>>> On 08/11/2015 01:36 PM, Maxime Coquelin wrote:
>>>> On 08/11/2015 12:11 PM, Geert Uytterhoeven wrote:
>>>>> On Tue, Aug 11, 2015 at 12:02 PM, Maxime Coquelin
>>>>> <maxime.coquelin@st.com> wrote:
>>>>>> How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi
>>>>>> platform?
>>>>>
>>>>> Add the flag to the relevant clocks in the C code, e.g. in
>>>>> clk_register_flexgen():
>>>>>
>>>>>           if (!strcmp(name, "clk-icn-cpu"))
>>>>>                   init.flags |= CLK_ENABLE_HAND_OFF;
>>>
>>> The main problem I see with this proposal is that clk_register_flexgen()
>>> is
>>> called for several SoCs (STiH407/410/418...).
>>> Each of these SoCs have this clock, but maybe STiH407 will need the flag,
>>> but not STiH410 and STiH418.
>>> So I think the best place to set this information is in DT, where the
>>> differentiation is made between the SoCs.
>>
>> If (of_machine_is_compatible("st,stih410")) ...
>>
> It works, but is it really what we want?
> Each time we will add a new soc, we will have to patch this SoC agnostic
> function?
> With the number of SoCs and the number of clocks, it will be a nightmare to
> maintain and debug, no?

One day[*], when you will discover the presence of the small security-related
control processor, you will finally understand why it can disable and enable
e.g. your main CPU clock, and may want to write a driver for it. Then you can
just remove the CLK_ENABLE_HAND_OFF flags.

[*] Perhaps this day has already happened, but obviously you're not allowed
    to discuss this on a public mailing list ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Michael Turquette]

Quoting Maxime Coquelin (2015-08-11 03:02:23)
> Hi Mike,
> 
> On 08/11/2015 10:43 AM, Lee Jones wrote:
> > On Mon, 10 Aug 2015, Michael Turquette wrote:
> >
> >>
> >>
> >> ST's driver is an unfortunate case. All of the clock data was shoved
> >> into DT before we had a clue that doing so is a terrible idea.
> 
> I tend to agree, and wouldn't do it this way if we could rewrite the 
> history.
> But now, we have to support it.
> 
> How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi 
> platform?
> 
> Could we imagine having a kind of "clocks-enable-hand-off" property we 
> could use in our clock controller DT node?

Maxime,

Yes. I'm sure that the ST binding isn't the only one that needs
something like this. Furthermore I am sure that there are interesting
users like the FPGA people that would love to dynamically set this flag
from DT based on their hardware description.

So the question is, what does it look like? We've already discussed
doing a clk-conf.c approach, but that is really meant for consumers of a
clock to set their default parameters. I don't think that is the right
way here.

Probably we should list the hand-off clocks directly in the
clock-provider node itself. We can design it as a list (for
clock-controller nodes that expose multiple clocks). In practice for the
st,flexgen binding it will always be a list with one element in it.

In my email to Lee a few minutes ago I asked if ST actually needs to
turn on gated clocks, or if the goal is to prevent already-on clocks
(enabled by default out of reset, or bootloader) from being gated? I
guess that the goal is the latter since we've been discussing "critical"
clocks that will crash the system if disabled.

Regards,
Mike

> 
> Regards,
> Maxime
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Michael Turquette]

Quoting Geert Uytterhoeven (2015-08-11 03:11:05)
> Hi Maxime,
> 
> On Tue, Aug 11, 2015 at 12:02 PM, Maxime Coquelin
> <maxime.coquelin@st.com> wrote:
> > How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi
> > platform?
> 
> Add the flag to the relevant clocks in the C code, e.g. in
> clk_register_flexgen():
> 
>         if (!strcmp(name, "clk-icn-cpu"))
>                 init.flags |= CLK_ENABLE_HAND_OFF;
> 
> > Could we imagine having a kind of "clocks-enable-hand-off" property we could
> > use in our clock controller DT node?
> 
> You can imagine doing "flex_flags |= CLK_ENABLE_HAND_OFF" in
> st_of_flexgen_setup(), depending on the presence of such a property.

This is precisely what Lee is trying to avoid. The would constitute a
hand-rolled, open-code, gather-and-mark exercise that drivers would have
to re-invent each time. (rough paraphrase of what Lee said)

I think that we can come up with a reasonable DT wrapper around the
flag. I will be ecstatic if we can agree that the meaning of the flag
can be tweaked just a bit to mean, "prevent this critical clock from
being disabled, as it was enabled out of reset or by the bootloader,
until a driver claims it and calls clk_prepare_enable".

Then everyone should be happy.

Regards,
Mike

> 
> However, not disabling clocks is a software policy, not a hardware description,
> so IMHO it doesn't belong in DT.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Geert Uytterhoeven]

Hi Mike,

On Tue, Aug 11, 2015 at 6:41 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> Quoting Geert Uytterhoeven (2015-08-11 02:20:12)
>> On Fri, Aug 7, 2015 at 9:09 PM, Michael Turquette
>> <mturquette@baylibre.com> wrote:
>> > This is an alternative solution to Lee's "clk: Provide support for
>> > always-on clocks" series[0].

>> I gave it a try on r8a7791/koelsch, where I replaced the hack from
>> "[PATCH/RFC 1/5] clk: shmobile: mstp: Never disable INTC-SYS"
>> (http://www.spinics.net/lists/linux-sh/msg41107.html) by setting
>>
>>         init.flags |= CLK_ENABLE_HAND_OFF
>>
>> in cpg_mstp_clock_register() for "intc-sys".
>>
>> The end result is fine (the "intc-sys" clock is never disabled), but I get
>> a few annoying lockdep splats like below (one for the "intc-sys" clock,
>> and one more for each parent up to the root clock):
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 0 at drivers/clk/clk.c:745 clk_core_enable+0x6c/0xdc()

> Thanks much for testing! I forgot to hold the enable lock in __clk_init
> (we already hold the prepare lock). Can you tell me if this diff fixes
> it?

Yes it does. Thanks for the quick fix!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Lee Jones]

On Tue, 11 Aug 2015, Michael Turquette wrote:

> Quoting Geert Uytterhoeven (2015-08-11 03:11:05)
> > Hi Maxime,
> > 
> > On Tue, Aug 11, 2015 at 12:02 PM, Maxime Coquelin
> > <maxime.coquelin@st.com> wrote:
> > > How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi
> > > platform?
> > 
> > Add the flag to the relevant clocks in the C code, e.g. in
> > clk_register_flexgen():
> > 
> >         if (!strcmp(name, "clk-icn-cpu"))
> >                 init.flags |= CLK_ENABLE_HAND_OFF;
> > 
> > > Could we imagine having a kind of "clocks-enable-hand-off" property we could
> > > use in our clock controller DT node?
> > 
> > You can imagine doing "flex_flags |= CLK_ENABLE_HAND_OFF" in
> > st_of_flexgen_setup(), depending on the presence of such a property.
> 
> This is precisely what Lee is trying to avoid. The would constitute a
> hand-rolled, open-code, gather-and-mark exercise that drivers would have
> to re-invent each time. (rough paraphrase of what Lee said)

Thanks.

> I think that we can come up with a reasonable DT wrapper around the
> flag. I will be ecstatic if we can agree that the meaning of the flag
> can be tweaked just a bit to mean, "prevent this critical clock from
> being disabled, as it was enabled out of reset or by the bootloader,
> until a driver claims it and calls clk_prepare_enable".

Easy, how about:

'prevent_this_critical_clock_from_being_disabled_as_it_was_enabled_out_of_reset_or_by_the_bootloader_until_a_driver_claims_it_and_calls_clk_prepare_enable'

Or

I could come up with something else?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Lee Jones]

On Tue, 11 Aug 2015, Michael Turquette wrote:

> Quoting Maxime Coquelin (2015-08-11 03:02:23)
> > Hi Mike,
> > 
> > On 08/11/2015 10:43 AM, Lee Jones wrote:
> > > On Mon, 10 Aug 2015, Michael Turquette wrote:
> > >
> > >>
> > >>
> > >> ST's driver is an unfortunate case. All of the clock data was shoved
> > >> into DT before we had a clue that doing so is a terrible idea.
> > 
> > I tend to agree, and wouldn't do it this way if we could rewrite the 
> > history.
> > But now, we have to support it.
> > 
> > How can we pass CLK_ENABLE_HAND_OFF flag to a specific clock on STi 
> > platform?
> > 
> > Could we imagine having a kind of "clocks-enable-hand-off" property we 
> > could use in our clock controller DT node?
> 
> Maxime,
> 
> Yes. I'm sure that the ST binding isn't the only one that needs
> something like this. Furthermore I am sure that there are interesting
> users like the FPGA people that would love to dynamically set this flag
> from DT based on their hardware description.
> 
> So the question is, what does it look like? We've already discussed
> doing a clk-conf.c approach, but that is really meant for consumers of a
> clock to set their default parameters. I don't think that is the right
> way here.
> 
> Probably we should list the hand-off clocks directly in the
> clock-provider node itself. We can design it as a list (for
> clock-controller nodes that expose multiple clocks). In practice for the
> st,flexgen binding it will always be a list with one element in it.
> 
> In my email to Lee a few minutes ago I asked if ST actually needs to
> turn on gated clocks, or if the goal is to prevent already-on clocks
> (enabled by default out of reset, or bootloader) from being gated? I
> guess that the goal is the latter since we've been discussing "critical"
> clocks that will crash the system if disabled.

The latter is correct.  Clocks are on at boot-up.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Lee Jones]

On Tue, 11 Aug 2015, Michael Turquette wrote:

> Quoting Lee Jones (2015-08-11 01:43:29)
> > On Mon, 10 Aug 2015, Michael Turquette wrote:
> > > Quoting Lee Jones (2015-08-10 07:48:11)
> > > > On Fri, 07 Aug 2015, Michael Turquette wrote:
> > > This series is solving the following problems:
> > > 
> > > 1) enabling specified clocks at boot
> > > 2) preventing those clocks from being gated by clk_disable_unused
> > 
> > The original patch-set did this just fine.
> 
> There is a very real difference between the implementations.
> 
> The original patch made it easy to call clk_prepare_enable on a clock
> from some place other than a Linux device driver (e.g. DT).
> 
> The hand-off semantic establishes an expectation that a driver will come
> along and claim ownership of the clk using standard Linux apis; we're
> just preserving the enabled state of the clock until that time.
> 
> I had a chat with Stephen Boyd about this yesterday and we discussed
> taking it even further: do not explicitly enable the clock, but instead
> simply refrain from disabling a clock that is both ON and has this flag
> set.

Doing so will prevent clk_disable_unused() from gating it, but if we
don't take a reference sibling clocks will be able to disable the
parent which will be fatal.

> It sounds like that would that work for ST, yes? Are you interested in
> using a flag (or a DT property) to enable an otherwise-gated clock, or
> simply insuring that bootloader-enabled and reset-enabled clocks are not
> spuriously turned off?

Clocks are ungated by the bootloader.

> > > If you mean to say, "this patch doesn't let me toss this data in
> > > Devicetree, a data orifice that is used by only a fraction of Linux
> > > kernel users" then you would be right.
> > 
> > A fraction of Linux kernel users, yes, but the majority (all?) of
> > the Clock Framework users do use DT.
> 
> At last count we had 5 architectures using ccf, I haven't counted in a
> while. x86 definitely does not use Devicetree. I have no clue if MIPS
> does. PowerPC and ARM-ish both do.

I believe that most of your users are ARM-ish.

> > > diff --git a/drivers/clk/qcom/gcc-apq8084.c b/drivers/clk/qcom/gcc-apq8084.c
> > > index 3563019..d2f5e5a 100644
> > > --- a/drivers/clk/qcom/gcc-apq8084.c
> > > +++ b/drivers/clk/qcom/gcc-apq8084.c
> > > @@ -1450,23 +1450,23 @@ static struct clk_branch gcc_blsp1_qup1_spi_apps_clk = {
> > >  static struct clk_branch gcc_blsp1_qup2_i2c_apps_clk = {
> > >         .halt_reg = 0x06c8,
> > >         .clkr = {
> > >                 .enable_reg = 0x06c8,
> > >                 .enable_mask = BIT(0),
> > >                 .hw.init = &(struct clk_init_data){
> > >                         .name = "gcc_blsp1_qup2_i2c_apps_clk",
> > >                         .parent_names = (const char *[]){
> > >                                 "blsp1_qup2_i2c_apps_clk_src",
> > >                         },
> > >                         .num_parents = 1,
> > > -                       .flags = CLK_SET_RATE_PARENT,
> > > +                       .flags = CLK_SET_RATE_PARENT | CLK_ENABLE_HAND_OFF,
> > >                         .ops = &clk_branch2_ops,
> > >                 },
> > >         },
> > >  };
> > 
> > Fair enough.  Obviously for anyone using Device Tree, this solution
> > makes it pretty difficult to partake.
> 
> QCOM is using Devicetree. I've covered how to make a clock-controller
> style binding before using QCOM's driver & binding as examples. Take a
> look here if you have some spare time:
> 
> http://lkml.kernel.org/r/<20150416192014.19585.9663@quantum>

Spare time, what's that?

> > > > What happens during disable() and unprepare()?
> > > 
> > > The reference counts go to zero. As I stated in my cover letter, I'll
> > > need to see evidence of a real use case where the "leave the clock on on
> > > when I call clk_disable, clk_unprepare and clk_put" behavior is
> > > warranted.
> > 
> > I can't say for sure (get-out clause), but I doubt we'd need that, as
> > this would only be required if a knowledgeable consumer existed
> > i.e. one which actually wanted to the disable critical clock.  On ST's
> > platforms I don't think there is a use-case for these clocks to ever
> > be gated, as the platform would be unrecoverable and require a reboot.
> 
> That's great. I suspected that behavior was not necessary at all.
> 
> Let's zero in on the technical concerns here:
> 
> 1) ST's flexgen binding should not get screwed over. So we'll need a DT
> wrapper around the flag

Great.

> 2) I would love feedback on whether you expect the flag/property to
> enable a disabled clock or if you merely want to keep an already-enabled
> clock from being disabled

For us, we only need the clock not to be turned off, either by
clk_disable_unused() or by drivers using critical clock siblings, but
as I'm striving for a generic approach, it would be hypocritical of me
to encourage not to cover all bases with this solution.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Geert Uytterhoeven]

On Tue, Aug 11, 2015 at 8:17 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> I think that we can come up with a reasonable DT wrapper around the
>> flag. I will be ecstatic if we can agree that the meaning of the flag
>> can be tweaked just a bit to mean, "prevent this critical clock from
>> being disabled, as it was enabled out of reset or by the bootloader,
>> until a driver claims it and calls clk_prepare_enable".
>
> Easy, how about:
>
> 'prevent_this_critical_clock_from_being_disabled_as_it_was_enabled_out_of_reset_or_by_the_bootloader_until_a_driver_claims_it_and_calls_clk_prepare_enable'

To make it less Linux-centric:

"Prevent this critical clock from being disabled implicitly by the OS, as it
 was enabled out of reset or by the bootloader, until it's explicitly managed
 by a driver."

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Lee Jones]

On Wed, 12 Aug 2015, Geert Uytterhoeven wrote:

> On Tue, Aug 11, 2015 at 8:17 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> I think that we can come up with a reasonable DT wrapper around the
> >> flag. I will be ecstatic if we can agree that the meaning of the flag
> >> can be tweaked just a bit to mean, "prevent this critical clock from
> >> being disabled, as it was enabled out of reset or by the bootloader,
> >> until a driver claims it and calls clk_prepare_enable".
> >
> > Easy, how about:
> >
> > 'prevent_this_critical_clock_from_being_disabled_as_it_was_enabled_out_of_reset_or_by_the_bootloader_until_a_driver_claims_it_and_calls_clk_prepare_enable'
> 
> To make it less Linux-centric:
> 
> "Prevent this critical clock from being disabled implicitly by the OS, as it
>  was enabled out of reset or by the bootloader, until it's explicitly managed
>  by a driver."

Hmm... I think you missed the giggles. :)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

Hi Mike,

On Fri, Aug 07, 2015 at 12:09:27PM -0700, Michael Turquette wrote:
> All of the other kitchen sink stuff (DT binding, passing the flag back
> to the framework when the clock consumer driver calls clk_put) was left
> out because I do not see a real use case for it. If one can demonstrate
> a real use case (and not a hypothetical one) then this patch series can
> be expanded further.

I think there is a very trivial use case for passing back the
reference to the framework, if during the probed, we have something
like:

clk = clk_get()
clk_prepare_enable(clk)
foo_framework_register()

if foo_framework_register fails, the sensible thing to do would be to
call clk_disable_unprepare. If the clock was a critical clock, you
just gated it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

On Tue, Aug 11, 2015 at 11:58:27AM -0700, Michael Turquette wrote:
> For example the whole big messy fuss over the DT bindings for the
> simple-fb driver could have been avoided if this feature had existed
> then.

Not really, there was additional issues that would have prevented to
use that in simplefb too. For example the fact that you wouldn't care
about the clock at all if simplefb was not enabled in the kernel. Or
that you would like to protect the pixel clock (or its parent) from
having its rate changed.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]

Hi Mike,

On Fri, Aug 07, 2015 at 12:09:30PM -0700, Michael Turquette wrote:
> Some clocks are critical to system operation (e.g. cpu, memory, etc) and
> should not be gated until a driver that knows best claims such a clock
> and expressly gates that clock through the normal clk.h api.
> 
> The typical way to handle this is for the clk driver or some other early
> code to call clk_prepare_enable on this important clock as soon as it is
> registered and before the clk_disable_unused garbage collector kicks in.
> 
> This patch introduces a formal way to handle this scenario that is
> provided by the clk framework. Clk driver authors can set the
> CLK_ENABLE_HAND_OFF flag in their clk data, which will cause the clk to
> be enabled in clk_register(). Then when the first clk consumer driver
> comes along and calls clk_get() & clk_prepare_enable(), the reference
> counts taken during clk registration are transfered (or handed off) to
> the clk consumer.
> 
> At this point handling the clk is the same as any other clock which as
> not set the new CLK_ENABLE_HAND_OFF flag. In fact no changes to any
> clock consumer driver are needed for this to work.

This looks fine, the only thing I'm not really fond of is the name of
the flag itself (and it's usually a good thing when we come to that
kind of bikeshedding).

In my mind, the fact that we hand off the clock reference is a direct
result to the clock being critical (or whatever name we want to call
it). The hand off is a side effect, but the real information we want
to carry is that it should not be gated.

And then the framework will know what behaviour it want to have based
on that information.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

On Tue, Aug 18, 2015 at 09:33:56AM -0700, Michael Turquette wrote:
> Quoting Maxime Ripard (2015-08-18 08:52:03)
> > On Tue, Aug 11, 2015 at 11:58:27AM -0700, Michael Turquette wrote:
> > > For example the whole big messy fuss over the DT bindings for the
> > > simple-fb driver could have been avoided if this feature had existed
> > > then.
> > 
> > Not really, there was additional issues that would have prevented to
> > use that in simplefb too. For example the fact that you wouldn't care
> > about the clock at all if simplefb was not enabled in the kernel. Or
> > that you would like to protect the pixel clock (or its parent) from
> > having its rate changed.
> 
> You're right, there may be more issues involved here. But as I recall
> there was interest in using simplefb to "hand-off" to a loadable module
> later on, and that was really what I was referring to.
> 
> In that case we could "skip" simplefb having to claim and enable clocks.
> The loadable module that is the "rich" driver could just claim them as
> usual and the reference counts would be handed over at that time.

Yep, but the issue would be broader but just the clocks, but yeah,
you're right :)

> But I don't mean to dig up past flamebait ;-)

Yep, it's probably better not to :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1821 bytes --]

On Tue, Aug 18, 2015 at 09:43:56AM -0700, Michael Turquette wrote:
> Quoting Maxime Ripard (2015-08-18 08:45:52)
> > Hi Mike,
> > 
> > On Fri, Aug 07, 2015 at 12:09:27PM -0700, Michael Turquette wrote:
> > > All of the other kitchen sink stuff (DT binding, passing the flag back
> > > to the framework when the clock consumer driver calls clk_put) was left
> > > out because I do not see a real use case for it. If one can demonstrate
> > > a real use case (and not a hypothetical one) then this patch series can
> > > be expanded further.
> > 
> > I think there is a very trivial use case for passing back the
> > reference to the framework, if during the probed, we have something
> > like:
> > 
> > clk = clk_get()
> > clk_prepare_enable(clk)
> > foo_framework_register()
> > 
> > if foo_framework_register fails, the sensible thing to do would be to
> > call clk_disable_unprepare. If the clock was a critical clock, you
> > just gated it.
> 
> Hmm, a good point. Creating the "pass the reference back" call is not
> hard technically. But how to keep from abusing it? E.g. I do not want
> that call to become an alternative to correct use of clk_enable.
> 
> Maybe I'll need a Coccinelle script or just some regular sed to
> occasionally search for new users of this api and audit them?
> 
> I was hoping to not add any new consumer api at all :-/

I don't think there's any abuse that can be done with the current API,
nor do I think you need to have new functions either.

If the clock is critical, when the customer calls
clk_unprepare_disable on it, simply take back the reference you gave
in the framework, and you're done. Or am I missing something?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

On Tue, Aug 18, 2015 at 09:39:31AM -0700, Michael Turquette wrote:
> > In my mind, the fact that we hand off the clock reference is a direct
> > result to the clock being critical (or whatever name we want to call
> > it). The hand off is a side effect, but the real information we want
> > to carry is that it should not be gated.
> 
> I chose the "hand-off" name because I want to set an expectation to
> users of this feature. That expectation is that some day they will have
> a Linux device driver that claims and manages this "critical clock".
> Clearly this is not always the case. Many clocks using this feature will
> never have a driver that "owns" them.
> 
> But I wanted to avoid any kind of "always on" or "easy hack to avoid
> writing proper driver code" naming convention that encourages bad
> behavior down the line.
> 
> Also, the hand-off thing is sort of a big deal. If driver writers only
> thought of this as an "alway on" mechanism then subtle bugs might creep
> in where drivers are getting and disabling a clock that the author
> incorrectly thought would always be enabled. So I'd like the name to
> reflect that somehow.
> 
> As always I am open to suggestions.

For the record, I think always-on would be just as bad, since it has
the same issue of describing the behaviour instead of describing what
the clock is.

I would think critical is better, and if you feel there's some
unexpected behaviour, we can always add some comment / documentation
for that (heresy, I know ;))

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Lee Jones]

On Tue, 25 Aug 2015, Michael Turquette wrote:

> Quoting Maxime Ripard (2015-08-20 08:15:10)
> > On Tue, Aug 18, 2015 at 09:43:56AM -0700, Michael Turquette wrote:
> > > Quoting Maxime Ripard (2015-08-18 08:45:52)
> > > > Hi Mike,
> > > > 
> > > > On Fri, Aug 07, 2015 at 12:09:27PM -0700, Michael Turquette wrote:
> > > > > All of the other kitchen sink stuff (DT binding, passing the flag back
> > > > > to the framework when the clock consumer driver calls clk_put) was left
> > > > > out because I do not see a real use case for it. If one can demonstrate
> > > > > a real use case (and not a hypothetical one) then this patch series can
> > > > > be expanded further.
> > > > 
> > > > I think there is a very trivial use case for passing back the
> > > > reference to the framework, if during the probed, we have something
> > > > like:
> > > > 
> > > > clk = clk_get()
> > > > clk_prepare_enable(clk)
> > > > foo_framework_register()
> > > > 
> > > > if foo_framework_register fails, the sensible thing to do would be to
> > > > call clk_disable_unprepare. If the clock was a critical clock, you
> > > > just gated it.
> > > 
> > > Hmm, a good point. Creating the "pass the reference back" call is not
> > > hard technically. But how to keep from abusing it? E.g. I do not want
> > > that call to become an alternative to correct use of clk_enable.
> > > 
> > > Maybe I'll need a Coccinelle script or just some regular sed to
> > > occasionally search for new users of this api and audit them?
> > > 
> > > I was hoping to not add any new consumer api at all :-/
> > 
> > I don't think there's any abuse that can be done with the current API,
> > nor do I think you need to have new functions either.
> > 
> > If the clock is critical, when the customer calls
> > clk_unprepare_disable on it, simply take back the reference you gave
> > in the framework, and you're done. Or am I missing something?
> 
> Maybe I am the one missing something? My goal was to allow the consumer
> driver to gate the critical clock. So we need clk_disable_unused to
> actually disable the clock for that to work.
> 
> I think you are suggesting that clk_disable_unused should *not* disable
> the clock if it is critical. Can you confirm that?

My take is that a critical clock should only be disabled when a
knowledgeable driver wants to gate it for a specific purpose [probably
using clk_disable()].  Once the aforementioned driver no longer has a
use for the clock [whether that happens with clk_unprepare_disable()
or clk_put() ...] the clock should be ungated and be provided with
critical status once more.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Maxime Coquelin]

Hi Lee,

On 08/26/2015 08:54 AM, Lee Jones wrote:
> On Tue, 25 Aug 2015, Michael Turquette wrote:
>
>
>> Maybe I am the one missing something? My goal was to allow the consumer
>> driver to gate the critical clock. So we need clk_disable_unused to
>> actually disable the clock for that to work.
>>
>> I think you are suggesting that clk_disable_unused should *not* disable
>> the clock if it is critical. Can you confirm that?
> My take is that a critical clock should only be disabled when a
> knowledgeable driver wants to gate it for a specific purpose [probably
> using clk_disable()].  Once the aforementioned driver no longer has a
> use for the clock [whether that happens with clk_unprepare_disable()
> or clk_put() ...] the clock should be ungated and be provided with
> critical status once more.
>
How do you differentiate between a knowledgeable and non-knowledgeable 
driver?
Let's take the example of the clock used by the i2c on STi SoCs.
This clock is used by i2c, and is also critical to the system, but only 
i2c takes it.

At first transfer, the i2c will enable the clock and then disables it.

What we would expect here is that the clk_disable does not gate the 
clock, even if only user since the hand-off flag has been set.
Else, system will freeze.

Maxime

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Lee Jones]

On Wed, 26 Aug 2015, Maxime Coquelin wrote:

> Hi Lee,
> 
> On 08/26/2015 08:54 AM, Lee Jones wrote:
> >On Tue, 25 Aug 2015, Michael Turquette wrote:
> >
> >
> >>Maybe I am the one missing something? My goal was to allow the consumer
> >>driver to gate the critical clock. So we need clk_disable_unused to
> >>actually disable the clock for that to work.
> >>
> >>I think you are suggesting that clk_disable_unused should *not* disable
> >>the clock if it is critical. Can you confirm that?
> >My take is that a critical clock should only be disabled when a
> >knowledgeable driver wants to gate it for a specific purpose [probably
> >using clk_disable()].  Once the aforementioned driver no longer has a
> >use for the clock [whether that happens with clk_unprepare_disable()
> >or clk_put() ...] the clock should be ungated and be provided with
> >critical status once more.
> >
> How do you differentiate between a knowledgeable and
> non-knowledgeable driver?
> Let's take the example of the clock used by the i2c on STi SoCs.
> This clock is used by i2c, and is also critical to the system, but
> only i2c takes it.
> 
> At first transfer, the i2c will enable the clock and then disables it.
> 
> What we would expect here is that the clk_disable does not gate the
> clock, even if only user since the hand-off flag has been set.
> Else, system will freeze.

The I2C driver in this instance is not a knowledgeable driver and
should not be taking a reference to a critical clock.

In the example you provide, the real issue is that the I2C driver uses
one of the critical clock's siblings.  Without this framework, if it
gives up the reference to its own clock and there are no users of any
sibling clocks, the parent is gated.  This has the unfortunate effect
of gating the entire family, critical clock included.

These sorts of issues are precisely what we're trying to fix here.

For clarification, a knowledgeable driver is one that requests an
actual (not a sibling of) critical clock.  It's knowledgeable in the
fact that it knows what gating the clock will do to the system, but it
"knows best" that this is actually fine.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Maxime Coquelin]

On 08/26/2015 11:09 AM, Lee Jones wrote:
> On Wed, 26 Aug 2015, Maxime Coquelin wrote:
>
>> Hi Lee,
>>
>> On 08/26/2015 08:54 AM, Lee Jones wrote:
>>> On Tue, 25 Aug 2015, Michael Turquette wrote:
>>>
>>>
>>>> Maybe I am the one missing something? My goal was to allow the consumer
>>>> driver to gate the critical clock. So we need clk_disable_unused to
>>>> actually disable the clock for that to work.
>>>>
>>>> I think you are suggesting that clk_disable_unused should *not* disable
>>>> the clock if it is critical. Can you confirm that?
>>> My take is that a critical clock should only be disabled when a
>>> knowledgeable driver wants to gate it for a specific purpose [probably
>>> using clk_disable()].  Once the aforementioned driver no longer has a
>>> use for the clock [whether that happens with clk_unprepare_disable()
>>> or clk_put() ...] the clock should be ungated and be provided with
>>> critical status once more.
>>>
>> How do you differentiate between a knowledgeable and
>> non-knowledgeable driver?
>> Let's take the example of the clock used by the i2c on STi SoCs.
>> This clock is used by i2c, and is also critical to the system, but
>> only i2c takes it.
>>
>> At first transfer, the i2c will enable the clock and then disables it.
>>
>> What we would expect here is that the clk_disable does not gate the
>> clock, even if only user since the hand-off flag has been set.
>> Else, system will freeze.
> The I2C driver in this instance is not a knowledgeable driver and
> should not be taking a reference to a critical clock.
This is the case:
         i2c@9840000 {
             ...
             clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
             clock-names = "ssc";
             ...
         }

CLK_EXT2F_A9 is a critical clock I think.
Indeed, this clock corresponds to output 13 of clockgen c0.
This ouput has several clock names in the datasheet, but is in reality 
the same clock from HW point of view (i.e. "same wire"):

- CLK_ICN_REG
- CLK_TRACE_A9
- CLK_PTI_STM
- CLK_EXT2F_A9

I'm pretty sure CLK_ICN_REG is a critical clock.

Try to gate it without gating its parent, and see if system is still alive.


>
> In the example you provide, the real issue is that the I2C driver uses
> one of the critical clock's siblings.  Without this framework, if it
> gives up the reference to its own clock and there are no users of any
> sibling clocks, the parent is gated.  This has the unfortunate effect
> of gating the entire family, critical clock included.
I don't see why a clock used by i2c could not be a critical clock, if it 
is used by other parts of the system that cannot be represented as 
drivers, and rely on the clock to be always on.

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Lee Jones]

Mike, Maxime, Maxime,

On Wed, 26 Aug 2015, Maxime Coquelin wrote:
> On 08/26/2015 11:09 AM, Lee Jones wrote:
> >On Wed, 26 Aug 2015, Maxime Coquelin wrote:
> >>On 08/26/2015 08:54 AM, Lee Jones wrote:
> >>>On Tue, 25 Aug 2015, Michael Turquette wrote:
> >>>
> >>>>Maybe I am the one missing something? My goal was to allow the consumer
> >>>>driver to gate the critical clock. So we need clk_disable_unused to
> >>>>actually disable the clock for that to work.
> >>>>
> >>>>I think you are suggesting that clk_disable_unused should *not* disable
> >>>>the clock if it is critical. Can you confirm that?
> >>>My take is that a critical clock should only be disabled when a
> >>>knowledgeable driver wants to gate it for a specific purpose [probably
> >>>using clk_disable()].  Once the aforementioned driver no longer has a
> >>>use for the clock [whether that happens with clk_unprepare_disable()
> >>>or clk_put() ...] the clock should be ungated and be provided with
> >>>critical status once more.
> >>>
> >>How do you differentiate between a knowledgeable and
> >>non-knowledgeable driver?
> >>Let's take the example of the clock used by the i2c on STi SoCs.
> >>This clock is used by i2c, and is also critical to the system, but
> >>only i2c takes it.
> >>
> >>At first transfer, the i2c will enable the clock and then disables it.
> >>
> >>What we would expect here is that the clk_disable does not gate the
> >>clock, even if only user since the hand-off flag has been set.
> >>Else, system will freeze.
> >The I2C driver in this instance is not a knowledgeable driver and
> >should not be taking a reference to a critical clock.
> This is the case:
>         i2c@9840000 {
>             ...
>             clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
>             clock-names = "ssc";
>             ...
>         }
> 
> CLK_EXT2F_A9 is a critical clock I think.
> Indeed, this clock corresponds to output 13 of clockgen c0.
> This ouput has several clock names in the datasheet, but is in
> reality the same clock from HW point of view (i.e. "same wire"):
> 
> - CLK_ICN_REG
> - CLK_TRACE_A9
> - CLK_PTI_STM
> - CLK_EXT2F_A9
> 
> I'm pretty sure CLK_ICN_REG is a critical clock.
> 
> Try to gate it without gating its parent, and see if system is still alive.
> 
> >In the example you provide, the real issue is that the I2C driver uses
> >one of the critical clock's siblings.  Without this framework, if it
> >gives up the reference to its own clock and there are no users of any
> >sibling clocks, the parent is gated.  This has the unfortunate effect
> >of gating the entire family, critical clock included.
> 
> I don't see why a clock used by i2c could not be a critical clock,
> if it is used by other parts of the system that cannot be
> represented as drivers, and rely on the clock to be always on.

This is actually a great point and one that slipped my mind recently.
Handing-off a critical clock to the first requester will break our
platform.  It's part of the reason I set-up a special API.

To summarise:

In the beginning we were faced with an issue where unclaimed, but
still required clocks were being gated on start-up.  This was due to
the 'disable-unused' functionality used for power saving.  This was
tackled in one of two ways; either turn it off completely using the
'clk_ignore_unused' kernel command line parameter or on a per-clock
bases using a flag in C code.

Even with 'clk_ignore_unused' provided, drivers were able to gate
clocks critical to the running of the system by either gating their
siblings or the critical clock itself if it was shared with other
users.  It was this issue that prompted the creation of this set's
predecessor.

Although the original set worked, there were two shortcomings.
Firstly, it created a imbalance in the internal framework reference
counting.  Something that wasn't an issue at the time, but would
become an issue once Mike had authored and submitted his per-clock
reference counting patch set.  It also didn't allow "knowledgeable"
drivers (ones which knew the risks of gating a critical clock, but
knew better, and that it was okay to do so) to adopt the clock in
order to disable it.

So now we have this new set, where the priorities seem to have been
reversed.  It solves the issue of clock adoption by knowledgeable
drivers, but it suffers from the same symptoms as the ones which
prompted this functionality in the first place.  If, let's call it
an "uninformed" driver requests a critical clock using the current
API, the critical clock will be handed over, then the uninformed
driver is free to gate and ungate it as it sees fit.  The issue is
that the first call to clk_disable() will bork the running platform
irrecoverably.

We've already made it quite clear that we shouldn't be coding for
hypothetical situations.  So why do we even have this hand-off
feature?  I'm not aware of any knowledgeable drivers which do think
it's a good idea to gate a critical clock.  Are there any?

The hand-off feature was only mentioned because we were marking clocks
as critical in DT.  And due to the fact that DTBs sometimes get
separated (out dated) from the kernel, we needed this as a fall-back
plan to gate clocks previously thought to be critical at a later date.
However, this implementation doesn't even have DT support.  So to fix
this problem you could just un-flag the clock as critical, no?

>From a personal PoV, this set has all the features we don't need and
none of the ones we do.

I would like to suggest once more that if you wanted to keep this
adoption/hand-off feature that we do so in a cleaner (i.e. have proper
functions that deal with this stuff as opposed to shoehorning extra
code into existing functions) and more deliberate (i.e. insist that a
driver identify itself as 'knowledgeable', rather than 'uninformed' by
way of a specific call, clk_get_critical() for instance) way.

How do you propose we move forward?  Would be be okay with me having
another stab at this?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]

On Wed, Aug 26, 2015 at 07:54:23AM +0100, Lee Jones wrote:
> On Tue, 25 Aug 2015, Michael Turquette wrote:
> 
> > Quoting Maxime Ripard (2015-08-20 08:15:10)
> > > On Tue, Aug 18, 2015 at 09:43:56AM -0700, Michael Turquette wrote:
> > > > Quoting Maxime Ripard (2015-08-18 08:45:52)
> > > > > Hi Mike,
> > > > > 
> > > > > On Fri, Aug 07, 2015 at 12:09:27PM -0700, Michael Turquette wrote:
> > > > > > All of the other kitchen sink stuff (DT binding, passing the flag back
> > > > > > to the framework when the clock consumer driver calls clk_put) was left
> > > > > > out because I do not see a real use case for it. If one can demonstrate
> > > > > > a real use case (and not a hypothetical one) then this patch series can
> > > > > > be expanded further.
> > > > > 
> > > > > I think there is a very trivial use case for passing back the
> > > > > reference to the framework, if during the probed, we have something
> > > > > like:
> > > > > 
> > > > > clk = clk_get()
> > > > > clk_prepare_enable(clk)
> > > > > foo_framework_register()
> > > > > 
> > > > > if foo_framework_register fails, the sensible thing to do would be to
> > > > > call clk_disable_unprepare. If the clock was a critical clock, you
> > > > > just gated it.
> > > > 
> > > > Hmm, a good point. Creating the "pass the reference back" call is not
> > > > hard technically. But how to keep from abusing it? E.g. I do not want
> > > > that call to become an alternative to correct use of clk_enable.
> > > > 
> > > > Maybe I'll need a Coccinelle script or just some regular sed to
> > > > occasionally search for new users of this api and audit them?
> > > > 
> > > > I was hoping to not add any new consumer api at all :-/
> > > 
> > > I don't think there's any abuse that can be done with the current API,
> > > nor do I think you need to have new functions either.
> > > 
> > > If the clock is critical, when the customer calls
> > > clk_unprepare_disable on it, simply take back the reference you gave
> > > in the framework, and you're done. Or am I missing something?
> > 
> > Maybe I am the one missing something? My goal was to allow the consumer
> > driver to gate the critical clock. So we need clk_disable_unused to
> > actually disable the clock for that to work.
> > 
> > I think you are suggesting that clk_disable_unused should *not* disable
> > the clock if it is critical. Can you confirm that?
> 
> My take is that a critical clock should only be disabled when a
> knowledgeable driver wants to gate it for a specific purpose [probably
> using clk_disable()].  Once the aforementioned driver no longer has a
> use for the clock [whether that happens with clk_unprepare_disable()
> or clk_put() ...] the clock should be ungated and be provided with
> critical status once more.

Agreed.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3046 bytes --]

Hi Mike,

On Tue, Aug 25, 2015 at 02:50:51PM -0700, Michael Turquette wrote:
> Quoting Maxime Ripard (2015-08-20 08:15:10)
> > On Tue, Aug 18, 2015 at 09:43:56AM -0700, Michael Turquette wrote:
> > > Quoting Maxime Ripard (2015-08-18 08:45:52)
> > > > Hi Mike,
> > > > 
> > > > On Fri, Aug 07, 2015 at 12:09:27PM -0700, Michael Turquette wrote:
> > > > > All of the other kitchen sink stuff (DT binding, passing the flag back
> > > > > to the framework when the clock consumer driver calls clk_put) was left
> > > > > out because I do not see a real use case for it. If one can demonstrate
> > > > > a real use case (and not a hypothetical one) then this patch series can
> > > > > be expanded further.
> > > > 
> > > > I think there is a very trivial use case for passing back the
> > > > reference to the framework, if during the probed, we have something
> > > > like:
> > > > 
> > > > clk = clk_get()
> > > > clk_prepare_enable(clk)
> > > > foo_framework_register()
> > > > 
> > > > if foo_framework_register fails, the sensible thing to do would be to
> > > > call clk_disable_unprepare. If the clock was a critical clock, you
> > > > just gated it.
> > > 
> > > Hmm, a good point. Creating the "pass the reference back" call is not
> > > hard technically. But how to keep from abusing it? E.g. I do not want
> > > that call to become an alternative to correct use of clk_enable.
> > > 
> > > Maybe I'll need a Coccinelle script or just some regular sed to
> > > occasionally search for new users of this api and audit them?
> > > 
> > > I was hoping to not add any new consumer api at all :-/
> > 
> > I don't think there's any abuse that can be done with the current API,
> > nor do I think you need to have new functions either.
> > 
> > If the clock is critical, when the customer calls
> > clk_unprepare_disable on it, simply take back the reference you gave
> > in the framework, and you're done. Or am I missing something?
> 
> Maybe I am the one missing something? My goal was to allow the consumer
> driver to gate the critical clock. So we need clk_disable_unused to
> actually disable the clock for that to work.

Yeah, but I guess the consumer driver clock gating is not the default
mode of operations.

Under normal circumstances, it should just always leave the clock
enabled, all the time.

> I think you are suggesting that clk_disable_unused should *not* disable
> the clock if it is critical. Can you confirm that?

By default, yes.

Now, we also have the knowledgeable driver case wanting to force the
clock gating. I think it's an orthogonal issue, we might have the same
use case for non-critical clocks, and since it's hard to get that done
with the current API, and that we don't really know what a
knowledgeable driver will look like at this point, maybe we can just
delay this entirely until we actually have one in front of us?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Geert Uytterhoeven]

Hi Mike,

On Fri, Aug 7, 2015 at 9:09 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> From the clk_put kerneldoc in include/linux/clk.h:
>
> """
> Note: drivers must ensure that all clk_enable calls made on this clock
> source are balanced by clk_disable calls prior to calling this function.
> """
>
> The common clock framework implementation of the clk.h api has per-user
> reference counts for calls to clk_prepare and clk_disable. As such it
> can enforce the requirement to properly call clk_disable and
> clk_unprepare before calling clk_put.
>
> Because this requirement is probably violated in many places, this patch
> starts with a simple warning. Once offending code has been fixed this
> check could additionally release the reference counts automatically.
>
> Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> ---
>  drivers/clk/clk.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 72feee9..6ec0f77 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk)
>             clk->max_rate < clk->core->req_rate)
>                 clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
>
> +       /*
> +        * before calling clk_put, all calls to clk_prepare and clk_enable from
> +        * a given user must be balanced with calls to clk_disable and
> +        * clk_unprepare by that same user
> +        */
> +       WARN_ON(clk->prepare_count);
> +       WARN_ON(clk->enable_count);

These two WARN_ON()s are triggered a lot when using a legacy clock domain,
and CONFIG_PM=n. Indeed, without Runtime PM, the idea is that the module clocks
get enabled unconditionally, which violates the assumptions above.

Cfr. the CONFIG_PM=n version of pm_clk_notify() in
drivers/base/power/clock_ops.c, which calls enable_clock():

    /**
     * enable_clock - Enable a device clock.
     * @dev: Device whose clock is to be enabled.
     * @con_id: Connection ID of the clock.
     */
    static void enable_clock(struct device *dev, const char *con_id)
    {
            struct clk *clk;

            clk = clk_get(dev, con_id);
            if (!IS_ERR(clk)) {
                    clk_prepare_enable(clk);
                    clk_put(clk);
                    dev_info(dev, "Runtime PM disabled, clock forced on.\n");
            }
    }

I think this affects shmobile, keystone, davinci, omap1, and legacy sh.

Sorry for not noticing before, we usually build with CONFIG_PM=y.
One more reason for making CONFIG_PM=y mandatory on SoCs with clock domains?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4028 bytes --]

On Wed, Sep 30, 2015 at 05:36:49AM -0700, Michael Turquette wrote:
> Quoting Maxime Ripard (2015-08-28 20:55:57)
> > Hi Mike,
> > 
> > On Tue, Aug 25, 2015 at 02:50:51PM -0700, Michael Turquette wrote:
> > > Quoting Maxime Ripard (2015-08-20 08:15:10)
> > > > On Tue, Aug 18, 2015 at 09:43:56AM -0700, Michael Turquette wrote:
> > > > > Quoting Maxime Ripard (2015-08-18 08:45:52)
> > > > > > Hi Mike,
> > > > > > 
> > > > > > On Fri, Aug 07, 2015 at 12:09:27PM -0700, Michael Turquette wrote:
> > > > > > > All of the other kitchen sink stuff (DT binding, passing the flag back
> > > > > > > to the framework when the clock consumer driver calls clk_put) was left
> > > > > > > out because I do not see a real use case for it. If one can demonstrate
> > > > > > > a real use case (and not a hypothetical one) then this patch series can
> > > > > > > be expanded further.
> > > > > > 
> > > > > > I think there is a very trivial use case for passing back the
> > > > > > reference to the framework, if during the probed, we have something
> > > > > > like:
> > > > > > 
> > > > > > clk = clk_get()
> > > > > > clk_prepare_enable(clk)
> > > > > > foo_framework_register()
> > > > > > 
> > > > > > if foo_framework_register fails, the sensible thing to do would be to
> > > > > > call clk_disable_unprepare. If the clock was a critical clock, you
> > > > > > just gated it.
> > > > > 
> > > > > Hmm, a good point. Creating the "pass the reference back" call is not
> > > > > hard technically. But how to keep from abusing it? E.g. I do not want
> > > > > that call to become an alternative to correct use of clk_enable.
> > > > > 
> > > > > Maybe I'll need a Coccinelle script or just some regular sed to
> > > > > occasionally search for new users of this api and audit them?
> > > > > 
> > > > > I was hoping to not add any new consumer api at all :-/
> > > > 
> > > > I don't think there's any abuse that can be done with the current API,
> > > > nor do I think you need to have new functions either.
> > > > 
> > > > If the clock is critical, when the customer calls
> > > > clk_unprepare_disable on it, simply take back the reference you gave
> > > > in the framework, and you're done. Or am I missing something?
> > > 
> > > Maybe I am the one missing something? My goal was to allow the consumer
> > > driver to gate the critical clock. So we need clk_disable_unused to
> > > actually disable the clock for that to work.
> > 
> > Yeah, but I guess the consumer driver clock gating is not the default
> > mode of operations.
> > 
> > Under normal circumstances, it should just always leave the clock
> > enabled, all the time.
> > 
> > > I think you are suggesting that clk_disable_unused should *not* disable
> > > the clock if it is critical. Can you confirm that?
> > 
> > By default, yes.
> > 
> > Now, we also have the knowledgeable driver case wanting to force the
> > clock gating. I think it's an orthogonal issue, we might have the same
> > use case for non-critical clocks, and since it's hard to get that done
> > with the current API, and that we don't really know what a
> > knowledgeable driver will look like at this point, maybe we can just
> > delay this entirely until we actually have one in front of us?
> 
> Yes. I discussed this face to face with Lee last week at Linaro Connect.
> I proposed the following:
> 
> 1) support an always-on clk, which is enabled by the framework at
> registration-time and can never be claimed and gated
> 
> 2) support a hand-off clk, which is enabled by the framework at
> registration-time and whose reference counts are handed off to the first
> driver to get, prepare and enable this clk
> 
> 3) forget about knowledgeable drivers because nobody needs this (yet)
> 
> Lee was happy with this. Does it sound OK to you?

Yep, it sounds good!

See you in Dublin,

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 05:40:00AM -0700, Michael Turquette wrote:
> Why not keep the reference to the struct clk after get'ing it the first
> time?

Yes, that's exactly what you're supposed to do in this case.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Geert Uytterhoeven]

Hi Mike, Russell,

On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> Quoting Geert Uytterhoeven (2015-09-30 08:38:46)
>> On Fri, Aug 7, 2015 at 9:09 PM, Michael Turquette
>> <mturquette@baylibre.com> wrote:
>> > From the clk_put kerneldoc in include/linux/clk.h:
>> >
>> > """
>> > Note: drivers must ensure that all clk_enable calls made on this clock
>> > source are balanced by clk_disable calls prior to calling this function.
>> > """
>> >
>> > The common clock framework implementation of the clk.h api has per-user
>> > reference counts for calls to clk_prepare and clk_disable. As such it
>> > can enforce the requirement to properly call clk_disable and
>> > clk_unprepare before calling clk_put.
>> >
>> > Because this requirement is probably violated in many places, this patch
>> > starts with a simple warning. Once offending code has been fixed this
>> > check could additionally release the reference counts automatically.
>> >
>> > Signed-off-by: Michael Turquette <mturquette@baylibre.com>
>> > ---
>> >  drivers/clk/clk.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> > index 72feee9..6ec0f77 100644
>> > --- a/drivers/clk/clk.c
>> > +++ b/drivers/clk/clk.c
>> > @@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk)
>> >             clk->max_rate < clk->core->req_rate)
>> >                 clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
>> >
>> > +       /*
>> > +        * before calling clk_put, all calls to clk_prepare and clk_enable from
>> > +        * a given user must be balanced with calls to clk_disable and
>> > +        * clk_unprepare by that same user
>> > +        */
>> > +       WARN_ON(clk->prepare_count);
>> > +       WARN_ON(clk->enable_count);
>>
>> These two WARN_ON()s are triggered a lot when using a legacy clock domain,
>> and CONFIG_PM=n. Indeed, without Runtime PM, the idea is that the module clocks
>> get enabled unconditionally, which violates the assumptions above.
>>
>> Cfr. the CONFIG_PM=n version of pm_clk_notify() in
>> drivers/base/power/clock_ops.c, which calls enable_clock():
>>
>>     /**
>>      * enable_clock - Enable a device clock.
>>      * @dev: Device whose clock is to be enabled.
>>      * @con_id: Connection ID of the clock.
>>      */
>>     static void enable_clock(struct device *dev, const char *con_id)
>>     {
>>             struct clk *clk;
>>
>>             clk = clk_get(dev, con_id);
>>             if (!IS_ERR(clk)) {
>>                     clk_prepare_enable(clk);
>>                     clk_put(clk);
>
> This is a violation of the clkdev api as defined in include/linux/clk.h:
>
>         /**
>          * clk_put|------ "free" the clock source
>          * @clk: clock source
>          *
>          * Note: drivers must ensure that all clk_enable calls made on this
>          * clock source are balanced by clk_disable calls prior to calling
>          * this function.

I know.

> So the WARN is doing its job and letting us know about incorrect use of
> the API.
>
>>                     dev_info(dev, "Runtime PM disabled, clock forced on.\n");
>>             }
>>     }
>>
>> I think this affects shmobile, keystone, davinci, omap1, and legacy sh.
>
> Why not keep the reference to the struct clk after get'ing it the first
> time?

And store it where?

dev_pm_get_subsys_data() also depends on CONFIG_PM=y.

Note that there can be multiple clocks.

>> Sorry for not noticing before, we usually build with CONFIG_PM=y.
>> One more reason for making CONFIG_PM=y mandatory on SoCs with clock domains?

On ARM/shmobile, we only use it for the CONFIG_PM=n case, cfr.
drivers/sh/pm_runtime.c. In the CONFIG_PM=y case, we use DT and genpd.

For keystone, davinci, omap1, and legacy sh it's different, though.

With the advent of hardware Power and Clock Domains, keeping support for
CONFIG_PM=n alive is getting harder and harder...

> I don't know about that, but it seems like a reason to fix the clkdev
> usage in the clock domain code.

This is the legacy clock domain code. The way forward is genpd ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Russell King - ARM Linux]

On Wed, Oct 21, 2015 at 11:50:07AM +0200, Geert Uytterhoeven wrote:
> Hi Mike, Russell,
> 
> On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette
> <mturquette@baylibre.com> wrote:
> > Why not keep the reference to the struct clk after get'ing it the first
> > time?
> 
> And store it where?

Not my problem :)

Users are supposed to hold on to the reference obtained via clk_get()
while they're making use of the clock: in some implementations, this
increments the module use count if the clock driver is a module, and
may have other effects too.

Dropping that while you're still requiring the clock to be enabled is
unsafe: if it is provided by a module, then removing and reinserting
the module may very well change the enabled state of the clock, it
most certainly will disrupt the enable count.

It's always been this way, right from the outset, and when I've seen
people doing this bollocks, I've always pointed out that it's wrong.
Generally, people will fix it once they become aware of it, so it's
really that people just don't like reading and conforming to published
API requirements.

I think the root cause is that people just don't like reading what
other people write in terms of documentation, and they prefer to go
off and do their own thing, provided it works for them.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Michael Turquette]

Quoting Russell King - ARM Linux (2015-10-21 03:59:32)
> On Wed, Oct 21, 2015 at 11:50:07AM +0200, Geert Uytterhoeven wrote:
> > Hi Mike, Russell,
> > 
> > On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette
> > <mturquette@baylibre.com> wrote:
> > > Why not keep the reference to the struct clk after get'ing it the first
> > > time?
> > 
> > And store it where?
> 
> Not my problem :)
> 
> Users are supposed to hold on to the reference obtained via clk_get()
> while they're making use of the clock: in some implementations, this
> increments the module use count if the clock driver is a module, and
> may have other effects too.
> 
> Dropping that while you're still requiring the clock to be enabled is
> unsafe: if it is provided by a module, then removing and reinserting
> the module may very well change the enabled state of the clock, it
> most certainly will disrupt the enable count.
> 
> It's always been this way, right from the outset, and when I've seen
> people doing this bollocks, I've always pointed out that it's wrong.
> Generally, people will fix it once they become aware of it, so it's
> really that people just don't like reading and conforming to published
> API requirements.
> 
> I think the root cause is that people just don't like reading what
> other people write in terms of documentation, and they prefer to go
> off and do their own thing, provided it works for them.

Right, so in other words this problem must be solved by the caller of
clk_get, as it should be. I have never much looked at the pm clk code in
question, but I gave it a quick look and came up with some example code
that does not compile, in an effort to be as helpful as possible.

Basically I added a flex array to struct pm_clk_notifier_block, so that
now there are two flex arrays which is stupid. But I am too lazy to
replace the nbclk->clks thing with a malloc after walking all of the
clk_id's to figure out the number of clocks. Or we could just add
.num_clk to the struct, fix up all 4 users of it and drop the NULL
sentinel used the .clk_id's... Hmm that would have been better.

Anyways here is the ugly, non-compiling code. I'll take another look at
it in one year if no one else beats me to it.

Regards,
Mike



diff --git a/arch/arm/mach-davinci/pm_domain.c b/arch/arm/mach-davinci/pm_domain.c
index 78eac2c..b46e5ce 100644
--- a/arch/arm/mach-davinci/pm_domain.c
+++ b/arch/arm/mach-davinci/pm_domain.c
@@ -24,6 +24,7 @@ static struct dev_pm_domain davinci_pm_domain = {
 static struct pm_clk_notifier_block platform_bus_notifier = {
 	.pm_domain = &davinci_pm_domain,
 	.con_ids = { "fck", "master", "slave", NULL },
+	.clks = { ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN) },
 };
 
 static int __init davinci_pm_runtime_init(void)
diff --git a/arch/arm/mach-keystone/pm_domain.c b/arch/arm/mach-keystone/pm_domain.c
index e283939..d21c18b 100644
--- a/arch/arm/mach-keystone/pm_domain.c
+++ b/arch/arm/mach-keystone/pm_domain.c
@@ -27,6 +27,7 @@ static struct dev_pm_domain keystone_pm_domain = {
 
 static struct pm_clk_notifier_block platform_domain_notifier = {
 	.pm_domain = &keystone_pm_domain,
+	.clks = { ERR_PTR(-EAGAIN) },
 };
 
 static const struct of_device_id of_keystone_table[] = {
diff --git a/arch/arm/mach-omap1/pm_bus.c b/arch/arm/mach-omap1/pm_bus.c
index 667c163..5506453 100644
--- a/arch/arm/mach-omap1/pm_bus.c
+++ b/arch/arm/mach-omap1/pm_bus.c
@@ -31,6 +31,7 @@ static struct dev_pm_domain default_pm_domain = {
 static struct pm_clk_notifier_block platform_bus_notifier = {
 	.pm_domain = &default_pm_domain,
 	.con_ids = { "ick", "fck", NULL, },
+	.clks = { ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN) },
 };
 
 static int __init omap1_pm_runtime_init(void)
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 652b5a3..26f0dcf 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -407,40 +407,6 @@ int pm_clk_runtime_resume(struct device *dev)
 #else /* !CONFIG_PM */
 
 /**
- * enable_clock - Enable a device clock.
- * @dev: Device whose clock is to be enabled.
- * @con_id: Connection ID of the clock.
- */
-static void enable_clock(struct device *dev, const char *con_id)
-{
-	struct clk *clk;
-
-	clk = clk_get(dev, con_id);
-	if (!IS_ERR(clk)) {
-		clk_prepare_enable(clk);
-		clk_put(clk);
-		dev_info(dev, "Runtime PM disabled, clock forced on.\n");
-	}
-}
-
-/**
- * disable_clock - Disable a device clock.
- * @dev: Device whose clock is to be disabled.
- * @con_id: Connection ID of the clock.
- */
-static void disable_clock(struct device *dev, const char *con_id)
-{
-	struct clk *clk;
-
-	clk = clk_get(dev, con_id);
-	if (!IS_ERR(clk)) {
-		clk_disable_unprepare(clk);
-		clk_put(clk);
-		dev_info(dev, "Runtime PM disabled, clock forced off.\n");
-	}
-}
-
-/**
  * pm_clk_notify - Notify routine for device addition and removal.
  * @nb: Notifier block object this function is a member of.
  * @action: Operation being carried out by the caller.
@@ -465,18 +431,45 @@ static int pm_clk_notify(struct notifier_block *nb,
 	switch (action) {
 	case BUS_NOTIFY_BIND_DRIVER:
 		if (clknb->con_ids[0]) {
-			for (con_id = clknb->con_ids; *con_id; con_id++)
-				enable_clock(dev, *con_id);
+			int i;
+			for (con_id = clknb->con_ids, i = 0; *con_id;
+						con_id++, i++) {
+				if (clknb->clks[i] == ERR_PTR(-EAGAIN))
+					clks[i] = clk_get(dev, *con_id);
+				if (!IS_ERR(clknb->clks[i])) {
+					clk_prepare_enable(clk);
+					dev_info(dev, "Runtime PM disabled, clock forced on.\n");
+				}
 		} else {
-			enable_clock(dev, NULL);
+			if (clknb->clks[0] == ERR_PTR(-EAGAIN))
+				clks[0] = clk_get(dev, NULL);
+			if (!IS_ERR(clknb->clks[0])) {
+				clk_prepare_enable(clk);
+				dev_info(dev, "Runtime PM disabled, clock forced on.\n");
+			}
 		}
 		break;
 	case BUS_NOTIFY_UNBOUND_DRIVER:
+		/*
+		 * FIXME
+		 * We never call clk_put. This should be done with
+		 * pm_clk_remove_notifier, which doesn't exist but probably
+		 * should
+		 */
 		if (clknb->con_ids[0]) {
-			for (con_id = clknb->con_ids; *con_id; con_id++)
-				disable_clock(dev, *con_id);
+			int i;
+			for (con_id = clknb->con_ids, i = 0; *con_id;
+					con_id++, i++) {
+				if (!IS_ERR(clknb->clks[i])) {
+					clk_disable_unprepare(clknb->clks[i]);
+					dev_info(dev, "Runtime PM disabled, clock forced off.\n");
+				}
+			}
 		} else {
-			disable_clock(dev, NULL);
+			if (!IS_ERR(clknb->clks[0])) {
+				clk_disable_unprepare(clknb->clks[i]);
+				dev_info(dev, "Runtime PM disabled, clock forced off.\n");
+			}
 		}
 		break;
 	}
diff --git a/drivers/sh/pm_runtime.c b/drivers/sh/pm_runtime.c
index 25abd4e..08754a4 100644
--- a/drivers/sh/pm_runtime.c
+++ b/drivers/sh/pm_runtime.c
@@ -30,6 +30,7 @@ static struct dev_pm_domain default_pm_domain = {
 static struct pm_clk_notifier_block platform_bus_notifier = {
 	.pm_domain = &default_pm_domain,
 	.con_ids = { NULL, },
+	.clks = { ERR_PTR(-EAGAIN) },
 };
 
 static int __init sh_pm_runtime_init(void)
diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h
index 25266c6..45e58fe 100644
--- a/include/linux/pm_clock.h
+++ b/include/linux/pm_clock.h
@@ -16,6 +16,7 @@ struct pm_clk_notifier_block {
 	struct notifier_block nb;
 	struct dev_pm_domain *pm_domain;
 	char *con_ids[];
+	struct clk *clks[];
 };
 
 struct clk;

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Geert Uytterhoeven]

Hi Mike,

On Wed, Oct 21, 2015 at 5:50 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> Quoting Russell King - ARM Linux (2015-10-21 03:59:32)
>> On Wed, Oct 21, 2015 at 11:50:07AM +0200, Geert Uytterhoeven wrote:
>> > On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette
>> > <mturquette@baylibre.com> wrote:
>> > > Why not keep the reference to the struct clk after get'ing it the first
>> > > time?
>> >
>> > And store it where?
>>
>> Not my problem :)
>>
>> Users are supposed to hold on to the reference obtained via clk_get()
>> while they're making use of the clock: in some implementations, this
>> increments the module use count if the clock driver is a module, and
>> may have other effects too.
>>
>> Dropping that while you're still requiring the clock to be enabled is
>> unsafe: if it is provided by a module, then removing and reinserting
>> the module may very well change the enabled state of the clock, it
>> most certainly will disrupt the enable count.
>>
>> It's always been this way, right from the outset, and when I've seen
>> people doing this bollocks, I've always pointed out that it's wrong.
>> Generally, people will fix it once they become aware of it, so it's
>> really that people just don't like reading and conforming to published
>> API requirements.
>>
>> I think the root cause is that people just don't like reading what
>> other people write in terms of documentation, and they prefer to go
>> off and do their own thing, provided it works for them.
>
> Right, so in other words this problem must be solved by the caller of
> clk_get, as it should be. I have never much looked at the pm clk code in
> question, but I gave it a quick look and came up with some example code
> that does not compile, in an effort to be as helpful as possible.
>
> Basically I added a flex array to struct pm_clk_notifier_block, so that
> now there are two flex arrays which is stupid. But I am too lazy to
> replace the nbclk->clks thing with a malloc after walking all of the
> clk_id's to figure out the number of clocks. Or we could just add
> .num_clk to the struct, fix up all 4 users of it and drop the NULL
> sentinel used the .clk_id's... Hmm that would have been better.

Thanks for trying.

> index 25266c6..45e58fe 100644
> --- a/include/linux/pm_clock.h
> +++ b/include/linux/pm_clock.h
> @@ -16,6 +16,7 @@ struct pm_clk_notifier_block {
>         struct notifier_block nb;
>         struct dev_pm_domain *pm_domain;
>         char *con_ids[];
> +       struct clk *clks[];
>  };
>
>  struct clk;

Unfortunately that won't work: while the .con_ids[] array is per-platform,
the .clks[] array should be per-device. I.e. it should be tied to the struct
device, not to the struct pm_clk_notifier_block.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Heiko Stübner]

Hi Mike,

Am Freitag, 7. August 2015, 12:09:27 schrieb Michael Turquette:
> This is an alternative solution to Lee's "clk: Provide support for
> always-on clocks" series[0].
> 
> The first two patches introduce run-time checks to ensure that clock
> consumer drivers are respecting the clk.h api. The former patch checks
> for prepare and enable imbalances. The latter checks for calls to
> clk_put without first disabling and unpreparing the clk.
> 
> The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which
> prepares and enables a clk at registration-time. The reference counts
> (prepare & enable) are transferred to the first clock consumer driver
> that clk_get's the clk with this flag set AND calls clk_prepare or
> clk_enable.
> 
> The net result is that a clock with this flag set will be enabled at
> boot and neither the clk_disable_unused garbage collector or the
> "sibling clock disables a shared parent" scenario will cause the flagged
> clock to be disabled. The first driver to come along and explicitly
> claim, prepare and enable this clock will inherit those reference
> counts. No change to clock consumer drivers is required for this to
> work. Please continue to use the clk.h api properly.

just out of curiosity, did this move anywhere yet? (Last message from october 
1st it seems)

It looks like it is needed to fix the orphan-deferral I need on Rockchip that 
breaks sunxi in its current state.


Thanks
Heiko

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Michael Turquette]

Heiko Stübner wrote:
> Hi Mike,
> 
> Am Freitag, 7. August 2015, 12:09:27 schrieb Michael Turquette:
> > This is an alternative solution to Lee's "clk: Provide support for
> > always-on clocks" series[0].
> > 
> > The first two patches introduce run-time checks to ensure that clock
> > consumer drivers are respecting the clk.h api. The former patch checks
> > for prepare and enable imbalances. The latter checks for calls to
> > clk_put without first disabling and unpreparing the clk.
> > 
> > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which
> > prepares and enables a clk at registration-time. The reference counts
> > (prepare & enable) are transferred to the first clock consumer driver
> > that clk_get's the clk with this flag set AND calls clk_prepare or
> > clk_enable.
> > 
> > The net result is that a clock with this flag set will be enabled at
> > boot and neither the clk_disable_unused garbage collector or the
> > "sibling clock disables a shared parent" scenario will cause the flagged
> > clock to be disabled. The first driver to come along and explicitly
> > claim, prepare and enable this clock will inherit those reference
> > counts. No change to clock consumer drivers is required for this to
> > work. Please continue to use the clk.h api properly.
> 
> just out of curiosity, did this move anywhere yet? (Last message from october 
> 1st it seems)
> 
> It looks like it is needed to fix the orphan-deferral I need on Rockchip that 
> breaks sunxi in its current state.

Yes, I'm preparing another version. Sorry for high latency, but I've been
traveling for more than 2 months non-stop.

Regards,
Mike

> 
> 
> Thanks
> Heiko

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

[Michael Turquette]

Quoting Michael Turquette (2015-12-04 16:46:28)
> Heiko Stübner wrote:
> > Hi Mike,
> > 
> > Am Freitag, 7. August 2015, 12:09:27 schrieb Michael Turquette:
> > > This is an alternative solution to Lee's "clk: Provide support for
> > > always-on clocks" series[0].
> > > 
> > > The first two patches introduce run-time checks to ensure that clock
> > > consumer drivers are respecting the clk.h api. The former patch checks
> > > for prepare and enable imbalances. The latter checks for calls to
> > > clk_put without first disabling and unpreparing the clk.
> > > 
> > > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which
> > > prepares and enables a clk at registration-time. The reference counts
> > > (prepare & enable) are transferred to the first clock consumer driver
> > > that clk_get's the clk with this flag set AND calls clk_prepare or
> > > clk_enable.
> > > 
> > > The net result is that a clock with this flag set will be enabled at
> > > boot and neither the clk_disable_unused garbage collector or the
> > > "sibling clock disables a shared parent" scenario will cause the flagged
> > > clock to be disabled. The first driver to come along and explicitly
> > > claim, prepare and enable this clock will inherit those reference
> > > counts. No change to clock consumer drivers is required for this to
> > > work. Please continue to use the clk.h api properly.
> > 
> > just out of curiosity, did this move anywhere yet? (Last message from october 
> > 1st it seems)
> > 
> > It looks like it is needed to fix the orphan-deferral I need on Rockchip that 
> > breaks sunxi in its current state.
> 
> Yes, I'm preparing another version. Sorry for high latency, but I've been
> traveling for more than 2 months non-stop.

Done. See:

http://lkml.kernel.org/r/<1455225554-13267-1-git-send-email-mturquette@baylibre.com>

Regards,
Mike

> 
> Regards,
> Mike
> 
> > 
> > 
> > Thanks
> > Heiko
> 
>