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 2 of 2 — ← Prev page 1 [2]

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

[Multipart message — attachments visible in raw view] — view raw

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

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

Maxime

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

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

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

[Multipart message — attachments visible in raw view] — view raw

Hi Mike,

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

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

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

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

Maxime

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

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

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

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

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

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

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

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

Hi Mike,

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

Regards,
Maxime
--
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]

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

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

Doh! Good catch.

Thanks,
Mike

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

I'm not sure what this is.

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

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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]

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

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

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

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

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

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

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

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

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

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

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

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

3) isn't really a 'thing'.

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

Okay, fine.

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

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

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

I agree to disagree.

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

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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]

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

Hi Mike,

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

Thanks for your series!

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

        init.flags |= CLK_ENABLE_HAND_OFF

in cpg_mstp_clock_register() for "intc-sys".

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

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

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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]

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

Hi Mike,

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

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

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

Yes it does. Thanks for the quick fix!

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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]

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

[Multipart message — attachments visible in raw view] — view raw

Hi Mike,

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

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

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

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

Maxime

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

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web