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

Groups > linux.kernel > #1311580 > unrolled thread

[PATCH 0/3] clk: Add support for critical clocks

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] clk: Add support for critical clocks Lee Jones <lee.jones@linaro.org> - 2016-01-18 15:40 +0100
    [PATCH 2/3] clk: WARN_ON about to disable a critical clock Lee Jones <lee.jones@linaro.org> - 2016-01-18 15:40 +0100
    [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL Lee Jones <lee.jones@linaro.org> - 2016-01-18 15:40 +0100
      Re: [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL André Przywara <andre.przywara@arm.com> - 2016-01-28 01:00 +0100
        Re: [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-02-01 07:40 +0100
          Re: [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL Lee Jones <lee.jones@linaro.org> - 2016-02-01 09:30 +0100
          Re: [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL Andre Przywara <andre.przywara@arm.com> - 2016-02-02 14:50 +0100
            Re: [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL Lee Jones <lee.jones@linaro.org> - 2016-02-02 16:10 +0100
    [PATCH 1/3] clk: Allow clocks to be marked as CRITICAL Lee Jones <lee.jones@linaro.org> - 2016-01-18 15:40 +0100
      Re: [PATCH 1/3] clk: Allow clocks to be marked as CRITICAL Geert Uytterhoeven <geert@linux-m68k.org> - 2016-01-18 18:20 +0100
        Re: [PATCH 1/3] clk: Allow clocks to be marked as CRITICAL Lee Jones <lee.jones@linaro.org> - 2016-01-19 09:00 +0100

#1311580 — [PATCH 0/3] clk: Add support for critical clocks

Some platforms contain clocks which if gated, will cause undefined or
catastrophic behaviours.  As such they are not to be turned off, ever.
Many of these such clocks do not have devices, thus device drivers
where clocks may be enabled and references taken to ensure they stay
enabled do not exist.  Therefore, we must handle these such cases in
the core.

This patchset defines an CLK_IS_CRITICAL flag which the core can use
to identify critical clocks and subsequently refuse to gate them.
Once a clock has been recognised as critical, we take extra
references to ensure the continued functionality of the clock
whatever else happens.

Mike,

It's been 17 weeks since our meeting in San Francisco and I'm keen to
move this forward.  As per our meeting, the plan is to separate our
two requirements, as users who require both critical clocks AND the
hand-off feature do not currently exist.  If you'd like to continue
enablement of the hand-off functionality you were interested in, I'll
continue on with critical clocks, as we still need this for our
platform.

I'm hoping this isn't the wrong approach, but if it is, let me know
how it can be improved and I'll re-roll.

Kind regards,
Lee

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

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

-- 
1.9.1

[toc] | [next] | [standalone]

#1311581 — [PATCH 2/3] clk: WARN_ON about to disable a critical clock

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

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

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

#1311585 — [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL

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

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 include/linux/clk-provider.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index ffa0b2e..6f178b7 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -707,6 +707,23 @@ const char *of_clk_get_parent_name(struct device_node *np, int index);
 
 void of_clk_init(const struct of_device_id *matches);
 
+static inline int of_clk_mark_if_critical(struct device_node *np,
+					  int index, unsigned long *flags)
+{
+	struct property *prop;
+	const __be32 *cur;
+	uint32_t idx;
+
+	if (!np || !flags)
+		return -EINVAL;
+
+	of_property_for_each_u32(np, "critical-clock", prop, cur, idx)
+		if (index == idx)
+			*flags |= CLK_IS_CRITICAL;
+
+	return 0;
+}
+
 #else /* !CONFIG_OF */
 
 static inline int of_clk_add_provider(struct device_node *np,
@@ -742,6 +759,11 @@ static inline const char *of_clk_get_parent_name(struct device_node *np,
 {
 	return NULL;
 }
+static inline int of_clk_mark_if_critical(struct device_node *np, int index,
+					  unsigned long *flags)
+{
+	return 0;
+}
 #define of_clk_init(matches) \
 	{ while (0); }
 #endif /* CONFIG_OF */
-- 
1.9.1

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

#1320156 — Re: [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL

Hi,

On 18/01/16 14:28, Lee Jones wrote:
> This call matches clocks which have been marked as critical in DT
> and sets the appropriate flag.  These flags can then be used to
> mark the clock core flags appropriately prior to registration.

I like the idea of having a generic property very much. Also this solves
a problem I have in a very elegant way.

I guess you need to document this in the bindings documentation, I'd
suggest Documentation/devicetree/bindings/clock/clock-bindings.txt.

Also by doing so you should clarify it's exact meaning:
The singular form of "critical-clock" hints as either having a scalar
value only or even being a flag only.
But the code actually reads as it being _a list_ of indices of the
output clocks, so wouldn't "critical-clocks" (plural) be a better name?
This goes along the line of using the plural for the other standard
clock node properties as well.

So is this the intended usage?
some_clk {
	#clock-cells = <1>;
	clock-output-names = "just_led", "cpu";
	critical-clocks = <1>;
	....

to mark the "cpu" clock as critical?
Or matching the clock-indeces property values if that is used?

Also since it is a generic property, isn't there some way of parsing it
and setting the flag automatically for each and every clock provider?
Without driver authors having to explicitly call this function you
provide? The nature of being a generic clock flag makes me think this is
worthwhile.

Cheers,
Andre.

> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  include/linux/clk-provider.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index ffa0b2e..6f178b7 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -707,6 +707,23 @@ const char *of_clk_get_parent_name(struct device_node *np, int index);
>  
>  void of_clk_init(const struct of_device_id *matches);
>  
> +static inline int of_clk_mark_if_critical(struct device_node *np,
> +					  int index, unsigned long *flags)
> +{
> +	struct property *prop;
> +	const __be32 *cur;
> +	uint32_t idx;
> +
> +	if (!np || !flags)
> +		return -EINVAL;
> +
> +	of_property_for_each_u32(np, "critical-clock", prop, cur, idx)
> +		if (index == idx)
> +			*flags |= CLK_IS_CRITICAL;
> +
> +	return 0;
> +}
> +
>  #else /* !CONFIG_OF */
>  
>  static inline int of_clk_add_provider(struct device_node *np,
> @@ -742,6 +759,11 @@ static inline const char *of_clk_get_parent_name(struct device_node *np,
>  {
>  	return NULL;
>  }
> +static inline int of_clk_mark_if_critical(struct device_node *np, int index,
> +					  unsigned long *flags)
> +{
> +	return 0;
> +}
>  #define of_clk_init(matches) \
>  	{ while (0); }
>  #endif /* CONFIG_OF */
> 

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

#1322841 — Re: [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL

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

Hi Andre,

On Wed, Jan 27, 2016 at 11:51:45PM +0000, André Przywara wrote:
> Hi,
> 
> On 18/01/16 14:28, Lee Jones wrote:
> > This call matches clocks which have been marked as critical in DT
> > and sets the appropriate flag.  These flags can then be used to
> > mark the clock core flags appropriately prior to registration.
> 
> I like the idea of having a generic property very much. Also this solves
> a problem I have in a very elegant way.

Not really. It has a significant set of drawbacks that we already
detailed in the initial thread, which are mostly related to the fact
that the clocks are to be left on is something that totally depends on
the software support in the kernel. Some clocks should be reported as
critical because they are simply missing a driver for it, some should
be because the driver for it as not been compiled, some should because
we don't have the proper clocks drivers yet for one of their
downstream clocks.

Basically, it all boils down to this: some clocks should never ever be
shutdown because <hardware reason>, and I believe it's the case Lee is
in. But most of the current code that would use it might, and might
even need at some point to shut down such a clock.

Mike's solution with the flags + handover was solving all this, I'm
not sure why he's not pushed it forward.

Maxime

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

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

#1322896 — Re: [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL

On Mon, 01 Feb 2016, Maxime Ripard wrote:
> On Wed, Jan 27, 2016 at 11:51:45PM +0000, André Przywara wrote:
> > Hi,
> > 
> > On 18/01/16 14:28, Lee Jones wrote:
> > > This call matches clocks which have been marked as critical in DT
> > > and sets the appropriate flag.  These flags can then be used to
> > > mark the clock core flags appropriately prior to registration.
> > 
> > I like the idea of having a generic property very much. Also this solves
> > a problem I have in a very elegant way.
> 
> Not really. It has a significant set of drawbacks that we already
> detailed in the initial thread, which are mostly related to the fact
> that the clocks are to be left on is something that totally depends on
> the software support in the kernel. Some clocks should be reported as
> critical because they are simply missing a driver for it, some should
> be because the driver for it as not been compiled, some should because
> we don't have the proper clocks drivers yet for one of their
> downstream clocks.

Exactly.  This is a not a CLK_DRIVER_NOT_{AUTHORED|UPSTREAM} or
CLK_DRIVER_NOT_ENABLED implementation, it's for CLK_CRITICALs.
Critical clocks must _never_ be turned off, no matter what, else
something really bad will happen.  In our use-case, if the clocks are
turned of, it will be catastrophic to the running system.

> Basically, it all boils down to this: some clocks should never ever be
> shutdown because <hardware reason>, and I believe it's the case Lee is
> in. But most of the current code that would use it might, and might
> even need at some point to shut down such a clock.
> 
> Mike's solution with the flags + handover was solving all this, I'm
> not sure why he's not pushed it forward.

Right, but I think you are missing part of the conversation.  Mike and
I had a face-to-face meeting in San Francisco last year.  The
conclusion was that the CLK_CRITICAL and CLK_HANDOVER solutions should
be separated.  Different handling, different code.  This submission
only solves the former problem.  I believe Mike was going to submit
and follow-up on the CLK_HANDOVER solution separately.

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

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

#1324099 — Re: [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL

Hi Maxime,

On 01/02/16 06:32, Maxime Ripard wrote:
> Hi Andre,
> 
> On Wed, Jan 27, 2016 at 11:51:45PM +0000, André Przywara wrote:
>> Hi,
>>
>> On 18/01/16 14:28, Lee Jones wrote:
>>> This call matches clocks which have been marked as critical in DT
>>> and sets the appropriate flag.  These flags can then be used to
>>> mark the clock core flags appropriately prior to registration.
>>
>> I like the idea of having a generic property very much. Also this solves
>> a problem I have in a very elegant way.
> 
> Not really. It has a significant set of drawbacks that we already
> detailed in the initial thread, which are mostly related to the fact
> that the clocks are to be left on is something that totally depends on
> the software support in the kernel. Some clocks should be reported as
> critical because they are simply missing a driver for it, some should
> be because the driver for it as not been compiled, some should because
> we don't have the proper clocks drivers yet for one of their
> downstream clocks.
> 
> Basically, it all boils down to this: some clocks should never ever be
> shutdown because <hardware reason>, and I believe it's the case Lee is
> in. But most of the current code that would use it might, and might
> even need at some point to shut down such a clock.

I was bascically interested in pushing the critical-clock property into
DT to solve that cumbersome clk-sunxi init scheme - which you have fixed
now in a much better way (thanks for that, btw.)
For that particular case the CPU clock really looks like being actually
critical in the hardware sense - no-one maybe except the mgmt core
should turn the one single CPU clock source off.

So I wonder if we should document this "for hardware reasons only" and
still have that property in DT?
At the weekend I coded something into the generic DT clock code to let
it parse for basically every clock node - without a particular driver
needing to ask for it.
If this sounds useful to you I can post that one.

Cheers,
Andre.
> 
> Mike's solution with the flags + handover was solving all this, I'm
> not sure why he's not pushed it forward.
> 
> Maxime
> 

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

#1324185 — Re: [PATCH 3/3] clk: Provide OF helper to mark clocks as CRITICAL

On Tue, 02 Feb 2016, Andre Przywara wrote:

> Hi Maxime,
> 
> On 01/02/16 06:32, Maxime Ripard wrote:
> > Hi Andre,
> > 
> > On Wed, Jan 27, 2016 at 11:51:45PM +0000, André Przywara wrote:
> >> Hi,
> >>
> >> On 18/01/16 14:28, Lee Jones wrote:
> >>> This call matches clocks which have been marked as critical in DT
> >>> and sets the appropriate flag.  These flags can then be used to
> >>> mark the clock core flags appropriately prior to registration.
> >>
> >> I like the idea of having a generic property very much. Also this solves
> >> a problem I have in a very elegant way.
> > 
> > Not really. It has a significant set of drawbacks that we already
> > detailed in the initial thread, which are mostly related to the fact
> > that the clocks are to be left on is something that totally depends on
> > the software support in the kernel. Some clocks should be reported as
> > critical because they are simply missing a driver for it, some should
> > be because the driver for it as not been compiled, some should because
> > we don't have the proper clocks drivers yet for one of their
> > downstream clocks.
> > 
> > Basically, it all boils down to this: some clocks should never ever be
> > shutdown because <hardware reason>, and I believe it's the case Lee is
> > in. But most of the current code that would use it might, and might
> > even need at some point to shut down such a clock.
> 
> I was bascically interested in pushing the critical-clock property into
> DT to solve that cumbersome clk-sunxi init scheme - which you have fixed
> now in a much better way (thanks for that, btw.)
> For that particular case the CPU clock really looks like being actually
> critical in the hardware sense - no-one maybe except the mgmt core
> should turn the one single CPU clock source off.
> 
> So I wonder if we should document this "for hardware reasons only" and
> still have that property in DT?
> At the weekend I coded something into the generic DT clock code to let
> it parse for basically every clock node - without a particular driver
> needing to ask for it.
> If this sounds useful to you I can post that one.

It sounds very useful.  Very useful indeed.  But then I would say
that, because that's how this all started in the first place: ;)

  https://lkml.org/lkml/2015/7/22/299

I still think it's a pretty elegant method, but it was NACKed by Mike.

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

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

#1311586 — [PATCH 1/3] clk: Allow clocks to be marked as CRITICAL

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

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/clk/clk.c            | 7 ++++++-
 include/linux/clk-provider.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f13c3f4..835cb85 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2576,8 +2576,13 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 	}
 
 	ret = __clk_init(dev, hw->clk);
-	if (!ret)
+	if (!ret) {
+		if (core->flags & CLK_IS_CRITICAL) {
+			clk_core_prepare(core);
+			clk_core_enable(core);
+		}
 		return hw->clk;
+	}
 
 	__clk_free_clk(hw->clk);
 	hw->clk = NULL;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index c56988a..ffa0b2e 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -31,6 +31,7 @@
 #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_IS_CRITICAL		BIT(10) /* do not gate, ever */
 
 struct clk;
 struct clk_hw;
-- 
1.9.1

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

#1311696 — Re: [PATCH 1/3] clk: Allow clocks to be marked as CRITICAL

Hi Lee,

On Mon, Jan 18, 2016 at 3:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -31,6 +31,7 @@
>  #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_IS_CRITICAL                BIT(10) /* do not gate, ever */

10 is already taken, even upstream. Please rebase ;-)

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

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

#1311983 — Re: [PATCH 1/3] clk: Allow clocks to be marked as CRITICAL

On Mon, 18 Jan 2016, Geert Uytterhoeven wrote:
> On Mon, Jan 18, 2016 at 3:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -31,6 +31,7 @@
> >  #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_IS_CRITICAL                BIT(10) /* do not gate, ever */
> 
> 10 is already taken, even upstream. Please rebase ;-)

Thanks for the heads-up.  Will pull Heiko's patch in and rebase.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web