[PATCH v42 0/6] critical clocks and handoff clocks

[PATCH v42 0/6] critical clocks and handoff clocks

[Michael Turquette]

This series combines Lee's critical clock patches[0] plus some small
fixes and changes with a repost of my handoff clock patches[1]. Both
features are enabled by setting new flags on static clock data within
clock provider drivers.

In addition, there is a new function for setting the CLK_IS_CRITICAL
flag based on the presence of the clock-critical Device Tree property.
This DT property is not a part of the common-clock binding and must be
enabled on a per-binding basis (including adding that property to the
machine-specific binding description). Likewise the new function,
of_clk_detect_critical() must be called by either the clock provider
driver's probe or OF_CLK_DECLARE setup function.

[0] https://lkml.org/lkml/2016/1/18/272
[1] http://www.gossamer-threads.com/lists/linux/kernel/2234337

Lee Jones (3):
  clk: Allow clocks to be marked as CRITICAL
  clk: WARN_ON about to disable a critical clock
  clk: Provide OF helper to mark clocks as CRITICAL

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            | 127 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/clk-provider.h |  12 +++-
 2 files changed, 134 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH v42 1/6] clk: Allow clocks to be marked as CRITICAL

[Michael Turquette]

From: Lee Jones <lee.jones@linaro.org>

Critical clocks are those which must not be gated, else undefined
or catastrophic failure would occur.  Here we have chosen to
ensure the prepare/enable counts are correctly incremented, so as
not to confuse users with enabled clocks with no visible users.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Michael Turquette <mturquette@baylibre.com>
---
Changes in v42:
  * Moved code from clk_register into __clk_init

 drivers/clk/clk.c            | 5 +++++
 include/linux/clk-provider.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b4db67a..993f775 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2484,6 +2484,11 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
 	if (core->ops->init)
 		core->ops->init(core->hw);
 
+	if (core->flags & CLK_IS_CRITICAL) {
+		clk_core_prepare(core);
+		clk_core_enable(core);
+	}
+
 	kref_init(&core->ref);
 out:
 	clk_prepare_unlock();
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 1143e38..1d986ea 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -32,6 +32,7 @@
 #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_SET_RATE_UNGATE	BIT(10) /* clock needs to run to set rate */
+#define CLK_IS_CRITICAL		BIT(11) /* do not gate, ever */
 
 struct clk;
 struct clk_hw;
-- 
2.1.4

Re: [PATCH v42 1/6] clk: Allow clocks to be marked as CRITICAL

[Lee Jones]

On Thu, 11 Feb 2016, Michael Turquette wrote:

> From: Lee Jones <lee.jones@linaro.org>
> 
> Critical clocks are those which must not be gated, else undefined
> or catastrophic failure would occur.  Here we have chosen to
> ensure the prepare/enable counts are correctly incremented, so as
> not to confuse users with enabled clocks with no visible users.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> ---
> Changes in v42:
>   * Moved code from clk_register into __clk_init

I'm happy with this change.  Thanks Mike!

>  drivers/clk/clk.c            | 5 +++++
>  include/linux/clk-provider.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b4db67a..993f775 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2484,6 +2484,11 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
>  	if (core->ops->init)
>  		core->ops->init(core->hw);
>  
> +	if (core->flags & CLK_IS_CRITICAL) {
> +		clk_core_prepare(core);
> +		clk_core_enable(core);
> +	}
> +
>  	kref_init(&core->ref);
>  out:
>  	clk_prepare_unlock();
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 1143e38..1d986ea 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -32,6 +32,7 @@
>  #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_SET_RATE_UNGATE	BIT(10) /* clock needs to run to set rate */
> +#define CLK_IS_CRITICAL		BIT(11) /* do not gate, ever */
>  
>  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 v42 1/6] clk: Allow clocks to be marked as CRITICAL

[Stephen Boyd]

On 02/11, Michael Turquette wrote:
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b4db67a..993f775 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2484,6 +2484,11 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
>  	if (core->ops->init)
>  		core->ops->init(core->hw);
>  
> +	if (core->flags & CLK_IS_CRITICAL) {
> +		clk_core_prepare(core);
> +		clk_core_enable(core);
> +	}

What do we do if this is an orphan clk? From what I can tell
we're not going to increment the ref count on the parents that
may or may not appear at some later time when this flag is set.
Furthermore, do we want to propagate the CLK_IS_CRITICAL flag up
to all the parent clocks so that the warning mechanism spits out
errors for parent clocks? I suppose that may not be very useful
assuming refcounts are correct, but it may be useful to know
which clocks are critical and which ones aren't during debug.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v42 1/6] clk: Allow clocks to be marked as CRITICAL

[Stephen Boyd]

On 02/15, Michael Turquette wrote:
> Quoting Stephen Boyd (2016-02-12 17:14:03)
> > On 02/11, Michael Turquette wrote:
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index b4db67a..993f775 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -2484,6 +2484,11 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
> > >       if (core->ops->init)
> > >               core->ops->init(core->hw);
> > >  
> > > +     if (core->flags & CLK_IS_CRITICAL) {
> > > +             clk_core_prepare(core);
> > > +             clk_core_enable(core);
> > > +     }
> > 
> > What do we do if this is an orphan clk? From what I can tell
> > we're not going to increment the ref count on the parents that
> > may or may not appear at some later time when this flag is set.
> 
> I don't see how this is any different than any other orphan clock.
> __clk_set_parent_before and __clk_set_parent_after should still handle
> migration and propagation of the {prepare,enable}_count when it is
> finally re-parented.

>From what I can see we don't call __clk_set_parent_before() or
__clk_set_parent_after() when we're reparenting orphans to
registered clks. We just call clk_core_reparent() that does
mostly a list manipulation and recalc down the tree (BTW we
probably shouldn't recalc at all if a clk is still orphaned
because the rate is totally bogus).

> 
> (as an aside, that code conditionally calls clk_prepare AND clk_enable
> based solely on the prepare refcount, which seems weird to me...)

Yeah I think I was questioning why we need to call clk_enable()
there too so we should probably revisit this topic in another
thread.

> 
> > Furthermore, do we want to propagate the CLK_IS_CRITICAL flag up
> > to all the parent clocks so that the warning mechanism spits out
> > errors for parent clocks? I suppose that may not be very useful
> > assuming refcounts are correct, but it may be useful to know
> > which clocks are critical and which ones aren't during debug.
> 
> No, propagating flags is a bad idea. Existing prepare/enable ref counts
> should do the job for us.
> 
> Regarding debug, propagating the flag will hurt debug-ability. We
> already expose the clk_core->flags in sysfs, and debuggers can grep for
> the CRITICAL flag there to find any spuriously enabled clocks.
> 

Awesome.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v42 2/6] clk: WARN_ON about to disable a critical clock

[Michael Turquette]

From: Lee Jones <lee.jones@linaro.org>

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Michael Turquette <mturquette@baylibre.com>
---
 drivers/clk/clk.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 993f775..39f9527 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -575,6 +575,9 @@ static void clk_core_unprepare(struct clk_core *core)
 	if (WARN_ON(core->prepare_count == 0))
 		return;
 
+	if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL))
+		return;
+
 	if (--core->prepare_count > 0)
 		return;
 
@@ -680,6 +683,9 @@ static void clk_core_disable(struct clk_core *core)
 	if (WARN_ON(core->enable_count == 0))
 		return;
 
+	if (WARN_ON(core->enable_count == 1 && core->flags & CLK_IS_CRITICAL))
+		return;
+
 	if (--core->enable_count > 0)
 		return;
 
-- 
2.1.4

Re: [PATCH v42 2/6] clk: WARN_ON about to disable a critical clock

[Stephen Boyd]

On 02/11, Michael Turquette wrote:
> From: Lee Jones <lee.jones@linaro.org>
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v42 3/6] clk: Provide OF helper to mark clocks as CRITICAL

[Michael Turquette]

From: Lee Jones <lee.jones@linaro.org>

This call matches clocks which have been marked as critical in DT
and sets the appropriate flag.  These flags can then be used to
mark the clock core flags appropriately prior to registration.

Legacy bindings requiring this feature must add the clock-critical
property to their binding descriptions, as it is not a part of
common-clock binding.

Cc: devicetree@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Michael Turquette <mturquette@baylibre.com>
---
Changed in vWhatever:
  * Added kerneldoc
  * Moved to clk.c, removed static inline
  * s/of_clk_mark_if_critical/of_clk_detect_critical/
  * s/critical-clock/clock-critical/

 drivers/clk/clk.c            | 35 +++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  8 +++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 39f9527..5181a15 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3202,6 +3202,41 @@ static int parent_ready(struct device_node *np)
 }
 
 /**
+ * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree
+ * @np: Device node pointer associated with clock provider
+ * @index: clock index
+ * @flags: pointer to clk_core->flags
+ *
+ * Detects if the clock-critical property exists and, if so, sets the
+ * corresponding CLK_IS_CRITICAL flag.
+ *
+ * Do not use this function. It exists only for legacy Device Tree
+ * bindings, such as the one-clock-per-node style that are outdated.
+ * Those bindings typically put all clock data into .dts and the Linux
+ * driver has no clock data, thus making it impossible to set this flag
+ * correctly from the driver. Only those drivers may call
+ * of_clk_detect_critical from their setup functions.
+ *
+ * Return: error code or zero on success
+ */
+int of_clk_mark_if_critical(struct device_node *np,
+					  int index, unsigned long *flags)
+{
+	struct property *prop;
+	const __be32 *cur;
+	uint32_t idx;
+
+	if (!np || !flags)
+		return -EINVAL;
+
+	of_property_for_each_u32(np, "clock-critical", prop, cur, idx)
+		if (index == idx)
+			*flags |= CLK_IS_CRITICAL;
+
+	return 0;
+}
+
+/**
  * of_clk_init() - Scan and init clock providers from the DT
  * @matches: array of compatible values and init functions for providers.
  *
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 1d986ea..d15d3cc 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -705,7 +705,8 @@ int of_clk_get_parent_count(struct device_node *np);
 int of_clk_parent_fill(struct device_node *np, const char **parents,
 		       unsigned int size);
 const char *of_clk_get_parent_name(struct device_node *np, int index);
-
+int of_clk_mark_if_critical(struct device_node *np, int index,
+			    unsigned long *flags);
 void of_clk_init(const struct of_device_id *matches);
 
 #else /* !CONFIG_OF */
@@ -742,6 +743,11 @@ static inline const char *of_clk_get_parent_name(struct device_node *np,
 {
 	return NULL;
 }
+static inline int of_clk_mark_if_critical(struct device_node *np, int index,
+					  unsigned long *flags)
+{
+	return 0;
+}
 static inline void of_clk_init(const struct of_device_id *matches) {}
 #endif /* CONFIG_OF */
 
-- 
2.1.4

Re: [PATCH v42 3/6] clk: Provide OF helper to mark clocks as CRITICAL

[Lee Jones]

On Thu, 11 Feb 2016, Michael Turquette wrote:

> From: Lee Jones <lee.jones@linaro.org>
> 
> This call matches clocks which have been marked as critical in DT
> and sets the appropriate flag.  These flags can then be used to
> mark the clock core flags appropriately prior to registration.
> 
> Legacy bindings requiring this feature must add the clock-critical
> property to their binding descriptions, as it is not a part of
> common-clock binding.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> ---
> Changed in vWhatever:
>   * Added kerneldoc
>   * Moved to clk.c, removed static inline
>   * s/of_clk_mark_if_critical/of_clk_detect_critical/

Looks like you didn't fold this change in.

Only the kerneldoc is correct.

>   * s/critical-clock/clock-critical/
> 
>  drivers/clk/clk.c            | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/clk-provider.h |  8 +++++++-
>  2 files changed, 42 insertions(+), 1 deletion(-)

Appart from the small aforementioned issue, the changes look good to
me.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 39f9527..5181a15 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3202,6 +3202,41 @@ static int parent_ready(struct device_node *np)
>  }
>  
>  /**
> + * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree
> + * @np: Device node pointer associated with clock provider
> + * @index: clock index
> + * @flags: pointer to clk_core->flags
> + *
> + * Detects if the clock-critical property exists and, if so, sets the
> + * corresponding CLK_IS_CRITICAL flag.
> + *
> + * Do not use this function. It exists only for legacy Device Tree
> + * bindings, such as the one-clock-per-node style that are outdated.
> + * Those bindings typically put all clock data into .dts and the Linux
> + * driver has no clock data, thus making it impossible to set this flag
> + * correctly from the driver. Only those drivers may call
> + * of_clk_detect_critical from their setup functions.
> + *
> + * Return: error code or zero on success
> + */
> +int of_clk_mark_if_critical(struct device_node *np,
> +					  int index, unsigned long *flags)
> +{
> +	struct property *prop;
> +	const __be32 *cur;
> +	uint32_t idx;
> +
> +	if (!np || !flags)
> +		return -EINVAL;
> +
> +	of_property_for_each_u32(np, "clock-critical", prop, cur, idx)
> +		if (index == idx)
> +			*flags |= CLK_IS_CRITICAL;
> +
> +	return 0;
> +}
> +
> +/**
>   * of_clk_init() - Scan and init clock providers from the DT
>   * @matches: array of compatible values and init functions for providers.
>   *
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 1d986ea..d15d3cc 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -705,7 +705,8 @@ int of_clk_get_parent_count(struct device_node *np);
>  int of_clk_parent_fill(struct device_node *np, const char **parents,
>  		       unsigned int size);
>  const char *of_clk_get_parent_name(struct device_node *np, int index);
> -
> +int of_clk_mark_if_critical(struct device_node *np, int index,
> +			    unsigned long *flags);
>  void of_clk_init(const struct of_device_id *matches);
>  
>  #else /* !CONFIG_OF */
> @@ -742,6 +743,11 @@ static inline const char *of_clk_get_parent_name(struct device_node *np,
>  {
>  	return NULL;
>  }
> +static inline int of_clk_mark_if_critical(struct device_node *np, int index,
> +					  unsigned long *flags)
> +{
> +	return 0;
> +}
>  static inline void of_clk_init(const struct of_device_id *matches) {}
>  #endif /* CONFIG_OF */
>  

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

Re: [PATCH v42 3/6] clk: Provide OF helper to mark clocks as CRITICAL

[Lee Jones]

On Wed, 30 Mar 2016, Michael Turquette wrote:

> Quoting Lee Jones (2016-02-12 00:02:54)
> > On Thu, 11 Feb 2016, Michael Turquette wrote:
> > 
> > > From: Lee Jones <lee.jones@linaro.org>
> > > 
> > > This call matches clocks which have been marked as critical in DT
> > > and sets the appropriate flag.  These flags can then be used to
> > > mark the clock core flags appropriately prior to registration.
> > > 
> > > Legacy bindings requiring this feature must add the clock-critical
> > > property to their binding descriptions, as it is not a part of
> > > common-clock binding.
> > > 
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> > > ---
> > > Changed in vWhatever:
> > >   * Added kerneldoc
> > >   * Moved to clk.c, removed static inline
> > >   * s/of_clk_mark_if_critical/of_clk_detect_critical/
> > 
> > Looks like you didn't fold this change in.
> > 
> > Only the kerneldoc is correct.
> 
> Thanks for the catch. I've fixed it and applied patches 1-3 to clk-next.
> Patches 4-6 are turning up some bugs in other code that need to be fixed
> before they are applied.

Thanks Mike. \o/

> > >   * s/critical-clock/clock-critical/
> > > 
> > >  drivers/clk/clk.c            | 35 +++++++++++++++++++++++++++++++++++
> > >  include/linux/clk-provider.h |  8 +++++++-
> > >  2 files changed, 42 insertions(+), 1 deletion(-)
> > 
> > Appart from the small aforementioned issue, the changes look good to
> > me.
> > 
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 39f9527..5181a15 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -3202,6 +3202,41 @@ static int parent_ready(struct device_node *np)
> > >  }
> > >  
> > >  /**
> > > + * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree
> > > + * @np: Device node pointer associated with clock provider
> > > + * @index: clock index
> > > + * @flags: pointer to clk_core->flags
> > > + *
> > > + * Detects if the clock-critical property exists and, if so, sets the
> > > + * corresponding CLK_IS_CRITICAL flag.
> > > + *
> > > + * Do not use this function. It exists only for legacy Device Tree
> > > + * bindings, such as the one-clock-per-node style that are outdated.
> > > + * Those bindings typically put all clock data into .dts and the Linux
> > > + * driver has no clock data, thus making it impossible to set this flag
> > > + * correctly from the driver. Only those drivers may call
> > > + * of_clk_detect_critical from their setup functions.
> > > + *
> > > + * Return: error code or zero on success
> > > + */
> > > +int of_clk_mark_if_critical(struct device_node *np,
> > > +                                       int index, unsigned long *flags)
> > > +{
> > > +     struct property *prop;
> > > +     const __be32 *cur;
> > > +     uint32_t idx;
> > > +
> > > +     if (!np || !flags)
> > > +             return -EINVAL;
> > > +
> > > +     of_property_for_each_u32(np, "clock-critical", prop, cur, idx)
> > > +             if (index == idx)
> > > +                     *flags |= CLK_IS_CRITICAL;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +/**
> > >   * of_clk_init() - Scan and init clock providers from the DT
> > >   * @matches: array of compatible values and init functions for providers.
> > >   *
> > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > index 1d986ea..d15d3cc 100644
> > > --- a/include/linux/clk-provider.h
> > > +++ b/include/linux/clk-provider.h
> > > @@ -705,7 +705,8 @@ int of_clk_get_parent_count(struct device_node *np);
> > >  int of_clk_parent_fill(struct device_node *np, const char **parents,
> > >                      unsigned int size);
> > >  const char *of_clk_get_parent_name(struct device_node *np, int index);
> > > -
> > > +int of_clk_mark_if_critical(struct device_node *np, int index,
> > > +                         unsigned long *flags);
> > >  void of_clk_init(const struct of_device_id *matches);
> > >  
> > >  #else /* !CONFIG_OF */
> > > @@ -742,6 +743,11 @@ static inline const char *of_clk_get_parent_name(struct device_node *np,
> > >  {
> > >       return NULL;
> > >  }
> > > +static inline int of_clk_mark_if_critical(struct device_node *np, int index,
> > > +                                       unsigned long *flags)
> > > +{
> > > +     return 0;
> > > +}
> > >  static inline void of_clk_init(const struct of_device_id *matches) {}
> > >  #endif /* CONFIG_OF */
> > >  
> > 

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

Re: [PATCH v42 3/6] clk: Provide OF helper to mark clocks as CRITICAL

[Stephen Boyd]

On 02/11, Michael Turquette wrote:
> +int of_clk_mark_if_critical(struct device_node *np,
> +					  int index, unsigned long *flags)
> +{
> +	struct property *prop;
> +	const __be32 *cur;
> +	uint32_t idx;
> +
> +	if (!np || !flags)
> +		return -EINVAL;
> +
> +	of_property_for_each_u32(np, "clock-critical", prop, cur, idx)
> +		if (index == idx)
> +			*flags |= CLK_IS_CRITICAL;
> +
> +	return 0;
> +}

I hope we don't have to export this to modules...

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v42 4/6] 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>
---
Changed in v2ish:
  * Fixed locking bug (Thanks Maxime!)

 drivers/clk/clk.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5181a15..01183e3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -85,6 +85,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             ***/
@@ -609,7 +611,11 @@ void clk_unprepare(struct clk *clk)
 		return;
 
 	clk_prepare_lock();
+	if (WARN_ON(clk->prepare_count == 0))
+		goto out;
+	clk->prepare_count--;
 	clk_core_unprepare(clk->core);
+out:
 	clk_prepare_unlock();
 }
 EXPORT_SYMBOL_GPL(clk_unprepare);
@@ -666,6 +672,7 @@ int clk_prepare(struct clk *clk)
 		return 0;
 
 	clk_prepare_lock();
+	clk->prepare_count++;
 	ret = clk_core_prepare(clk->core);
 	clk_prepare_unlock();
 
@@ -719,7 +726,11 @@ void clk_disable(struct clk *clk)
 		return;
 
 	flags = clk_enable_lock();
+	if (WARN_ON(clk->enable_count == 0))
+		goto out;
+	clk->enable_count--;
 	clk_core_disable(clk->core);
+out:
 	clk_enable_unlock(flags);
 }
 EXPORT_SYMBOL_GPL(clk_disable);
@@ -781,6 +792,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);
 
-- 
2.1.4

Re: [PATCH v42 4/6] clk: per-user clk prepare & enable ref counts

[Stephen Boyd]

On 02/11, 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>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v42 5/6] 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>
---
Changed in v2ish:
  * s/WARN_ON/WARN_ON_ONCE/ to reduce noise for pm-clocks

 drivers/clk/clk.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 01183e3..d81f970 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2824,6 +2824,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_ONCE(clk->prepare_count);
+	WARN_ON_ONCE(clk->enable_count);
+
 	owner = clk->core->owner;
 	kref_put(&clk->core->ref, __clk_release);
 
-- 
2.1.4

Re: [PATCH v42 5/6] clk: clk_put WARNs if user has not disabled clk

[Stephen Boyd]

On 02/11, Michael Turquette 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.

Do we have any fixes for pm code in the works? I'm worried we're
going to be giving a warning and nobody will fix them or has a
plan to fix them.

> 
> Signed-off-by: Michael Turquette <mturquette@baylibre.com>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v42 5/6] clk: clk_put WARNs if user has not disabled clk

[Geert Uytterhoeven]

Hi Stephen, Mike,

On Sat, Feb 13, 2016 at 2:18 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 02/11, Michael Turquette 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.
>
> Do we have any fixes for pm code in the works? I'm worried we're
> going to be giving a warning and nobody will fix them or has a
> plan to fix them.
drivers/base/power/clock_ops.c
AFAIK not.

I've been running with the above patch for several months, and I had
to remove the two clk_put() calls in {en,dis}able_clock() in
drivers/base/power/clock_ops.c to get rid of the warnings when using the
legacy clock domain.

Fixing drivers/base/power/clock_ops.c is non-trivial though, as you need a
place to store the clk's reference obtained in enable_clock(), for later use in
disable_clock().

However, the plan is to make CONFIG_PM=y mandatory for Renesas ARM
SoCs with clock domains, which makes us no longer users of the legacy clock
domain.

Legacy SH and Davinci/Keystone/OMAP1 users may care, though...

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

[PATCH v42 6/6] 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>
---
Changed in v2ish:
  * s/BIT(11)/BIT(12)/

 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 d81f970..5a7776c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -60,6 +60,8 @@ struct clk_core {
 	bool			orphan;
 	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;
@@ -666,16 +668,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);
@@ -786,16 +803,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);
@@ -2507,6 +2539,27 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
 		clk_core_enable(core);
 	}
 
+	/*
+	 * 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 d15d3cc..6e54f41 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -33,6 +33,9 @@
 #define CLK_RECALC_NEW_RATES	BIT(9) /* recalc rates after notifications */
 #define CLK_SET_RATE_UNGATE	BIT(10) /* clock needs to run to set rate */
 #define CLK_IS_CRITICAL		BIT(11) /* do not gate, ever */
+#define CLK_ENABLE_HAND_OFF	BIT(12) /* enable clock when registered.
+					   hand-off enable_count & prepare_count
+					   to first consumer that enables clk */
 
 struct clk;
 struct clk_hw;
-- 
2.1.4

Re: [PATCH v42 0/6] critical clocks and handoff clocks

[Lee Jones]

On Thu, 11 Feb 2016, Michael Turquette wrote:

> This series combines Lee's critical clock patches[0] plus some small
> fixes and changes with a repost of my handoff clock patches[1]. Both
> features are enabled by setting new flags on static clock data within
> clock provider drivers.
> 
> In addition, there is a new function for setting the CLK_IS_CRITICAL
> flag based on the presence of the clock-critical Device Tree property.
> This DT property is not a part of the common-clock binding and must be
> enabled on a per-binding basis (including adding that property to the
> machine-specific binding description). Likewise the new function,
> of_clk_detect_critical() must be called by either the clock provider
> driver's probe or OF_CLK_DECLARE setup function.
> 
> [0] https://lkml.org/lkml/2016/1/18/272
> [1] http://www.gossamer-threads.com/lists/linux/kernel/2234337
> 
> Lee Jones (3):
>   clk: Allow clocks to be marked as CRITICAL
>   clk: WARN_ON about to disable a critical clock
>   clk: Provide OF helper to mark clocks as CRITICAL

One small issue which I've commented on already, but appart from that:

Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>

W00t, finally!  I'm so happy I could kiss you (but I can't due to that
legal injunction you have out on me that says I can't come within 100m
of you). ;)

Let's get this sucker in. :)

> 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            | 127 +++++++++++++++++++++++++++++++++++++++++--
>  include/linux/clk-provider.h |  12 +++-
>  2 files changed, 134 insertions(+), 5 deletions(-)


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