Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1203004 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off Michael Turquette <mturquette@baylibre.com> - 2015-08-07 21:20 +0200
    [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk Michael Turquette <mturquette@baylibre.com> - 2015-08-07 21:20 +0200
    [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Michael Turquette <mturquette@baylibre.com> - 2015-08-07 21:20 +0200
      Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Lee Jones <lee.jones@linaro.org> - 2015-08-10 16:50 +0200
        Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Lee Jones <lee.jones@linaro.org> - 2015-08-11 10:50 +0200
          Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Maxime Coquelin <maxime.coquelin@st.com> - 2015-08-11 12:10 +0200
            Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Geert Uytterhoeven <geert@linux-m68k.org> - 2015-08-11 12:20 +0200
              Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Maxime Coquelin <maxime.coquelin@st.com> - 2015-08-11 13:40 +0200
                Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Geert Uytterhoeven <geert@linux-m68k.org> - 2015-08-11 13:50 +0200
                  Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Lee Jones <lee.jones@linaro.org> - 2015-08-11 14:10 +0200
                  Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Maxime Coquelin <maxime.coquelin@st.com> - 2015-08-11 14:10 +0200
                    Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Geert Uytterhoeven <geert@linux-m68k.org> - 2015-08-11 14:40 +0200
                Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Maxime Coquelin <maxime.coquelin@st.com> - 2015-08-11 13:50 +0200
              Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Michael Turquette <mturquette@baylibre.com> - 2015-08-11 19:10 +0200
                Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Lee Jones <lee.jones@linaro.org> - 2015-08-11 20:20 +0200
                  Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Geert Uytterhoeven <geert@linux-m68k.org> - 2015-08-12 09:30 +0200
                    Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Lee Jones <lee.jones@linaro.org> - 2015-08-12 10:00 +0200
            Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Michael Turquette <mturquette@baylibre.com> - 2015-08-11 19:10 +0200
              Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Lee Jones <lee.jones@linaro.org> - 2015-08-11 20:30 +0200
          Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Lee Jones <lee.jones@linaro.org> - 2015-08-11 20:40 +0200
            Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Maxime Ripard <maxime.ripard@free-electrons.com> - 2015-08-18 18:00 +0200
      Re: [PATCH RFC RFT 3/3] clk: introduce CLK_ENABLE_HAND_OFF flag Maxime Ripard <maxime.ripard@free-electrons.com> - 2015-08-18 18:00 +0200
    [PATCH RFC RFT 1/3] clk: per-user clk prepare & enable ref counts Michael Turquette <mturquette@baylibre.com> - 2015-08-07 21:20 +0200
      Re: [PATCH RFC RFT 1/3] clk: per-user clk prepare & enable ref counts Maxime Coquelin <maxime.coquelin@st.com> - 2015-08-10 15:50 +0200
        Re: [PATCH RFC RFT 1/3] clk: per-user clk prepare & enable ref counts Michael Turquette <mturquette@baylibre.com> - 2015-08-10 21:40 +0200
    Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Lee Jones <lee.jones@linaro.org> - 2015-08-10 17:40 +0200
      Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Lee Jones <lee.jones@linaro.org> - 2015-08-11 11:20 +0200
    Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Geert Uytterhoeven <geert@linux-m68k.org> - 2015-08-11 11:30 +0200
      Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Geert Uytterhoeven <geert@linux-m68k.org> - 2015-08-11 19:50 +0200
    Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Maxime Ripard <maxime.ripard@free-electrons.com> - 2015-08-18 17:50 +0200

Page 1 of 2  [1] 2  Next page →

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

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

--
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/

[toc] | [next] | [standalone]

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

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

--
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/

[toc] | [prev] | [next] | [standalone]

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

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

--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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

--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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/
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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/
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

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

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
--
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/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web