[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

Signed-off-by: Lee Jones <redacted>
---
 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

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

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

Hi Lee,

On Mon, Jan 18, 2016 at 3:28 PM, Lee Jones [off-list ref] wrote:

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

On Mon, 18 Jan 2016, Geert Uytterhoeven wrote:

On Mon, Jan 18, 2016 at 3:28 PM, Lee Jones [off-list ref] wrote:

quoted

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

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.

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:

quoted

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160201/7cc643aa/attachment.sig>

On Mon, 01 Feb 2016, Maxime Ripard wrote:

On Wed, Jan 27, 2016 at 11:51:45PM +0000, Andr? Przywara wrote:

quoted

Hi,

On 18/01/16 14:28, Lee Jones wrote:

quoted

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

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:

quoted

Hi,

On 18/01/16 14:28, Lee Jones wrote:

quoted

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

On Tue, 02 Feb 2016, Andre Przywara wrote:

Hi Maxime,

On 01/02/16 06:32, Maxime Ripard wrote:

quoted

Hi Andre,

On Wed, Jan 27, 2016 at 11:51:45PM +0000, Andr? Przywara wrote:

quoted

Hi,

On 18/01/16 14:28, Lee Jones wrote:

quoted

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

Quoting Lee Jones (2016-02-01 00:22:45)

On Mon, 01 Feb 2016, Maxime Ripard wrote:

quoted

On Wed, Jan 27, 2016 at 11:51:45PM +0000, Andr? Przywara wrote:

quoted

Hi,

On 18/01/16 14:28, Lee Jones wrote:

quoted

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.

quoted

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.

Handoff clocks solved both the handoff clock problem (which Rhyland's
thread[0] brings back up) and the critical clock problem entirely except
for a single corner case: if a *critical* clk is enabled at boot via the
HANDOFF flag and then a driver get's that clk and enables/disables it.
Then we lose the refcount and the critical clock (which must never be
gated) will be gated.

That is the executive summary of my discussion with Lee last year. I'm
fine to merge both critical clocks and handoff clocks into the clk core
to cover all corner cases.

I'll post a series combining both sets of patches later today for
review.

quoted

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

Because I was super busy. I'm still super busy but now I will ignore
other important things and catch up on clk land. ;-)

[0] https://lkml.org/lkml/2016/2/9/817

Regards,
Mike

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

Quoting Lee Jones (2016-01-18 06:28:49)

I moved the above code into __clk_init where we do similar sort of clk
init-y type things. Modified patch below.

Best regards,
Mike

From 83b4f533b581a386341cd0cf9dbce92286e41e75 Mon Sep 17 00:00:00 2001

From: Lee Jones <redacted>
Date: Mon, 18 Jan 2016 14:28:49 +0000
Subject: [PATCH 1/6] 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 <redacted>
Signed-off-by: Michael Turquette <redacted>
---
 drivers/clk/clk.c            | 5 +++++
 include/linux/clk-provider.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b4db67a..993f775 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c

@@ -2484,6 +2484,11 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
 	if (core->ops->init)
 		core->ops->init(core->hw);
 
+	if (core->flags & CLK_IS_CRITICAL) {
+		clk_core_prepare(core);
+		clk_core_enable(core);
+	}
+
 	kref_init(&core->ref);
 out:
 	clk_prepare_unlock();

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 1143e38..1d986ea 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -32,6 +32,7 @@
 #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
 #define CLK_RECALC_NEW_RATES	BIT(9) /* recalc rates after notifications */
 #define CLK_SET_RATE_UNGATE	BIT(10) /* clock needs to run to set rate */
+#define CLK_IS_CRITICAL		BIT(11) /* do not gate, ever */
 
 struct clk;
 struct clk_hw;

-- 
2.1.4

Quoting Lee Jones (2016-01-18 06:28:50)

Signed-off-by: Lee Jones <redacted>

Looks good to me.

Regards,
Mike

Quoting Lee Jones (2016-01-18 06:28:51)

Added kerneldoc here outlining who should use this (legacy DT bindings
and no one else).

+static inline int of_clk_mark_if_critical(struct device_node *np,
+                                         int index, unsigned long *flags)

Moved this to clk.c, uninlined it and unstaticized it. Renamed beastly
function name to of_clk_detect_critical.

Changed property name to clock-critical to better match its siblings.

Modified patch below.

Best regards,
Mike

From 42df0a24d3c0bfd321e867e4113214160f17fadc Mon Sep 17 00:00:00 2001

From: Lee Jones <redacted>
Date: Mon, 18 Jan 2016 14:28:51 +0000
Subject: [PATCH 3/6] 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 <redacted>
Signed-off-by: Michael Turquette <redacted>
---
 drivers/clk/clk.c            | 35 +++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  8 +++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 39f9527..5181a15 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c

@@ -3202,6 +3202,41 @@ static int parent_ready(struct device_node *np)
 }
 
 /**
+ * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree
+ * @np: Device node pointer associated with clock provider
+ * @index: clock index
+ * @flags: pointer to clk_core->flags
+ *
+ * Detects if the clock-critical property exists and, if so, sets the
+ * corresponding CLK_IS_CRITICAL flag.
+ *
+ * Do not use this function. It exists only for legacy Device Tree
+ * bindings, such as the one-clock-per-node style that are outdated.
+ * Those bindings typically put all clock data into .dts and the Linux
+ * driver has no clock data, thus making it impossible to set this flag
+ * correctly from the driver. Only those drivers may call
+ * of_clk_detect_critical from their setup functions.
+ *
+ * Return: error code or zero on success
+ */
+int of_clk_mark_if_critical(struct device_node *np,
+					  int index, unsigned long *flags)
+{
+	struct property *prop;
+	const __be32 *cur;
+	uint32_t idx;
+
+	if (!np || !flags)
+		return -EINVAL;
+
+	of_property_for_each_u32(np, "clock-critical", prop, cur, idx)
+		if (index == idx)
+			*flags |= CLK_IS_CRITICAL;
+
+	return 0;
+}
+
+/**
  * of_clk_init() - Scan and init clock providers from the DT
  * @matches: array of compatible values and init functions for providers.
  *

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 1d986ea..d15d3cc 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -705,7 +705,8 @@ int of_clk_get_parent_count(struct device_node *np);
 int of_clk_parent_fill(struct device_node *np, const char **parents,
 		       unsigned int size);
 const char *of_clk_get_parent_name(struct device_node *np, int index);
-
+int of_clk_mark_if_critical(struct device_node *np, int index,
+			    unsigned long *flags);
 void of_clk_init(const struct of_device_id *matches);
 
 #else /* !CONFIG_OF */

@@ -742,6 +743,11 @@ static inline const char *of_clk_get_parent_name(struct device_node *np,
 {
 	return NULL;
 }
+static inline int of_clk_mark_if_critical(struct device_node *np, int index,
+					  unsigned long *flags)
+{
+	return 0;
+}
 static inline void of_clk_init(const struct of_device_id *matches) {}
 #endif /* CONFIG_OF */
 

-- 
2.1.4

Hi Lee,

Quoting Lee Jones (2016-01-18 06:28:48)

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.

Thanks for getting this going again. I've made tiny modifications to
your patches and reposted in this thread (w/ attribution to you of
course). Please let me know what you think.

Regarding Architecting The Perfect Solution, my take away from our
face-to-face discussion wass that handoff clocks covered every need of
critical (always on) clocks with a single exception, and that exception
is enough to merit supporting both.

The one area where we disagree is support for the DT property. You need
this because at least one of the platforms you care about use an
unfortunate, legacy clock binding style that came from a time before we
knew any better.

I definitely will never add a critical-clock property to the common
clock binding, but I cannot leave you without a solution either. I've
added kerneldoc around the function that sets the critical clock flag
making it clear that it should only be called from the setup functions
for clocks using the legacy binding style. It won't be called from
clk_register, __clk_init, or otherwise, but on a per-driver basis. This
removes the need for drivers to open code a solution where they match on
clk string name and add the flag or something super gross like that.

I will also repost my 3 handoff patches, rebased on top of your 3
critical clock patches. I'm sure they'll be pals and get along just
great.

Best regards,
Mike

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

On 11.02.2016 01:43, Michael Turquette wrote:

Quoting Lee Jones (2016-01-18 06:28:50)

quoted

Signed-off-by: Lee Jones <redacted>

Looks good to me.

Regards,
Mike

quoted

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

I have a question regarding this patch, which is mainline meanwhile [1]:

Having the following clock configuration:

                                         |--> child clk '1' (crit)
clk source --> parent clk 'A' (crit) -->|
                                         |--> child clk '2'


Clock '2' might be used, or not. It might be disabled or not. It doesn't 
matter. Clock '1' is not allowed to be disabled. Therefore its marked as 
critical.

Parent clock 'A' is marked as critical because its not allowed to be 
disabled, even if the enable_count of all child clocks is 0. To avoid 
that by disabling parent clock 'A' the child clock '1' is disabled, too, 
whats not allowed as its marked as critical.


Now, child clock '2' is used and enabled & disabled continuously by a 
(SPI) driver. What is ok. But:

Disabling child clock '2' results in the attempt to disable parent clock 
'A', too, which has correct enable_count 1 (from enabling the child 
'2'). What results

a) in the WARN_ON output

and

b) enable_count of 'A' never decreases to 0. Being off by one after the 
WARN_ON


It sounds like both is wrong for a configuration like above.

Opinions or proposal how to fix/change this?


Best regards

Dirk

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/clk.c?id=2e20fbf592621b2c2aeddd82e0fa3dad053cce03

On Tue, 27 Jun 2017, Dirk Behme wrote:

On 11.02.2016 01:43, Michael Turquette wrote:

quoted

Quoting Lee Jones (2016-01-18 06:28:50)

quoted

Signed-off-by: Lee Jones <redacted>

Looks good to me.

Regards,
Mike

quoted

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

I have a question regarding this patch, which is mainline meanwhile [1]:

Having the following clock configuration:

                                        |--> child clk '1' (crit)
clk source --> parent clk 'A' (crit) -->|
                                        |--> child clk '2'


Clock '2' might be used, or not. It might be disabled or not. It doesn't
matter. Clock '1' is not allowed to be disabled. Therefore its marked as
critical.

Parent clock 'A' is marked as critical because its not allowed to be
disabled, even if the enable_count of all child clocks is 0. To avoid that
by disabling parent clock 'A' the child clock '1' is disabled, too, whats
not allowed as its marked as critical.


Now, child clock '2' is used and enabled & disabled continuously by a (SPI)
driver. What is ok. But:

Disabling child clock '2' results in the attempt to disable parent clock
'A', too, which has correct enable_count 1 (from enabling the child '2').
What results

a) in the WARN_ON output

and

b) enable_count of 'A' never decreases to 0. Being off by one after the
WARN_ON


It sounds like both is wrong for a configuration like above.

Clock A still has one user, Clock 1.

Why is that wrong?

Opinions or proposal how to fix/change this?


Best regards

Dirk

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/clk.c?id=2e20fbf592621b2c2aeddd82e0fa3dad053cce03

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

On 03.07.2017 13:53, Lee Jones wrote:

On Tue, 27 Jun 2017, Dirk Behme wrote:

quoted

On 11.02.2016 01:43, Michael Turquette wrote:

quoted

Quoting Lee Jones (2016-01-18 06:28:50)

quoted

Signed-off-by: Lee Jones <redacted>

Looks good to me.

Regards,
Mike

quoted

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

I have a question regarding this patch, which is mainline meanwhile [1]:

Having the following clock configuration:

                                         |--> child clk '1' (crit)
clk source --> parent clk 'A' (crit) -->|
                                         |--> child clk '2'


Clock '2' might be used, or not. It might be disabled or not. It doesn't
matter. Clock '1' is not allowed to be disabled. Therefore its marked as
critical.

Parent clock 'A' is marked as critical because its not allowed to be
disabled, even if the enable_count of all child clocks is 0. To avoid that
by disabling parent clock 'A' the child clock '1' is disabled, too, whats
not allowed as its marked as critical.


Now, child clock '2' is used and enabled & disabled continuously by a (SPI)
driver. What is ok. But:

Disabling child clock '2' results in the attempt to disable parent clock
'A', too, which has correct enable_count 1 (from enabling the child '2').
What results

a) in the WARN_ON output

and

b) enable_count of 'A' never decreases to 0. Being off by one after the
WARN_ON


It sounds like both is wrong for a configuration like above.

Clock A still has one user, Clock 1.

Why is that wrong?

Because clock 1 is not a (Linux kernel clock framework) used clock. Its 
enable count is 0. So from Linux kernel (clock framework) point of view 
clock 1 is unused.

The increase/decrease of enable count of parent clock A is only driven 
by the Linux kernel usage of clock 2.

Best regards

Dirk

quoted

Opinions or proposal how to fix/change this?


Best regards

Dirk

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/clk.c?id=2e20fbf592621b2c2aeddd82e0fa3dad053cce03

On Mon, 03 Jul 2017, Dirk Behme wrote:

On 03.07.2017 13:53, Lee Jones wrote:

quoted

On Tue, 27 Jun 2017, Dirk Behme wrote:

quoted

On 11.02.2016 01:43, Michael Turquette wrote:

quoted

Quoting Lee Jones (2016-01-18 06:28:50)

quoted

Signed-off-by: Lee Jones <redacted>

Looks good to me.

Regards,
Mike

quoted

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

I have a question regarding this patch, which is mainline meanwhile [1]:

Having the following clock configuration:

                                         |--> child clk '1' (crit)
clk source --> parent clk 'A' (crit) -->|
                                         |--> child clk '2'


Clock '2' might be used, or not. It might be disabled or not. It doesn't
matter. Clock '1' is not allowed to be disabled. Therefore its marked as
critical.

Parent clock 'A' is marked as critical because its not allowed to be
disabled, even if the enable_count of all child clocks is 0. To avoid that
by disabling parent clock 'A' the child clock '1' is disabled, too, whats
not allowed as its marked as critical.


Now, child clock '2' is used and enabled & disabled continuously by a (SPI)
driver. What is ok. But:

Disabling child clock '2' results in the attempt to disable parent clock
'A', too, which has correct enable_count 1 (from enabling the child '2').
What results

a) in the WARN_ON output

and

b) enable_count of 'A' never decreases to 0. Being off by one after the
WARN_ON


It sounds like both is wrong for a configuration like above.

Clock A still has one user, Clock 1.

Why is that wrong?

Because clock 1 is not a (Linux kernel clock framework) used clock. Its
enable count is 0. So from Linux kernel (clock framework) point of view
clock 1 is unused.

All critical clocks are 'used'.  That's the point of critical clocks.

The increase/decrease of enable count of parent clock A is only driven by
the Linux kernel usage of clock 2.

quoted

quoted

Opinions or proposal how to fix/change this?


Best regards

Dirk

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/clk.c?id=2e20fbf592621b2c2aeddd82e0fa3dad053cce03

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

On 03.07.2017 16:25, Lee Jones wrote:

On Mon, 03 Jul 2017, Dirk Behme wrote:

quoted

On 03.07.2017 13:53, Lee Jones wrote:

quoted

On Tue, 27 Jun 2017, Dirk Behme wrote:

quoted

On 11.02.2016 01:43, Michael Turquette wrote:

quoted

Quoting Lee Jones (2016-01-18 06:28:50)

quoted

Signed-off-by: Lee Jones <redacted>

Looks good to me.

Regards,
Mike

quoted

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

I have a question regarding this patch, which is mainline meanwhile [1]:

Having the following clock configuration:

                                          |--> child clk '1' (crit)
clk source --> parent clk 'A' (crit) -->|
                                          |--> child clk '2'


Clock '2' might be used, or not. It might be disabled or not. It doesn't
matter. Clock '1' is not allowed to be disabled. Therefore its marked as
critical.

Parent clock 'A' is marked as critical because its not allowed to be
disabled, even if the enable_count of all child clocks is 0. To avoid that
by disabling parent clock 'A' the child clock '1' is disabled, too, whats
not allowed as its marked as critical.


Now, child clock '2' is used and enabled & disabled continuously by a (SPI)
driver. What is ok. But:

Disabling child clock '2' results in the attempt to disable parent clock
'A', too, which has correct enable_count 1 (from enabling the child '2').
What results

a) in the WARN_ON output

and

b) enable_count of 'A' never decreases to 0. Being off by one after the
WARN_ON


It sounds like both is wrong for a configuration like above.

Clock A still has one user, Clock 1.

Why is that wrong?

Because clock 1 is not a (Linux kernel clock framework) used clock. Its
enable count is 0. So from Linux kernel (clock framework) point of view
clock 1 is unused.

All critical clocks are 'used'.  That's the point of critical clocks.

Could you translate 'used' to enable_count? Whats valid enable_count 
numbers for critical clocks?

Best regards

Dirk

quoted

The increase/decrease of enable count of parent clock A is only driven by
the Linux kernel usage of clock 2.

quoted

quoted

Opinions or proposal how to fix/change this?


Best regards

Dirk

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/clk.c?id=2e20fbf592621b2c2aeddd82e0fa3dad053cce03

On Mon, 03 Jul 2017, Dirk Behme wrote:

On 03.07.2017 16:25, Lee Jones wrote:

quoted

On Mon, 03 Jul 2017, Dirk Behme wrote:

quoted

On 03.07.2017 13:53, Lee Jones wrote:

quoted

On Tue, 27 Jun 2017, Dirk Behme wrote:

quoted

On 11.02.2016 01:43, Michael Turquette wrote:

quoted

Quoting Lee Jones (2016-01-18 06:28:50)

quoted

Signed-off-by: Lee Jones <redacted>

Looks good to me.

Regards,
Mike

quoted

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

I have a question regarding this patch, which is mainline meanwhile [1]:

Having the following clock configuration:

                                          |--> child clk '1' (crit)
clk source --> parent clk 'A' (crit) -->|
                                          |--> child clk '2'


Clock '2' might be used, or not. It might be disabled or not. It doesn't
matter. Clock '1' is not allowed to be disabled. Therefore its marked as
critical.

Parent clock 'A' is marked as critical because its not allowed to be
disabled, even if the enable_count of all child clocks is 0. To avoid that
by disabling parent clock 'A' the child clock '1' is disabled, too, whats
not allowed as its marked as critical.


Now, child clock '2' is used and enabled & disabled continuously by a (SPI)
driver. What is ok. But:

Disabling child clock '2' results in the attempt to disable parent clock
'A', too, which has correct enable_count 1 (from enabling the child '2').
What results

a) in the WARN_ON output

and

b) enable_count of 'A' never decreases to 0. Being off by one after the
WARN_ON


It sounds like both is wrong for a configuration like above.

Clock A still has one user, Clock 1.

Why is that wrong?

Because clock 1 is not a (Linux kernel clock framework) used clock. Its
enable count is 0. So from Linux kernel (clock framework) point of view
clock 1 is unused.

All critical clocks are 'used'.  That's the point of critical clocks.

Could you translate 'used' to enable_count? Whats valid enable_count numbers
for critical clocks?

'used' == 'currently in use' == 'enabled'

Here's the piece of the puzzle you're probably missing:

	if (core->flags & CLK_IS_CRITICAL) {
		clk_core_prepare(core);
		clk_core_enable(core);
	}

Any clock that is identified as critical is prepared and enabled.

Thus your use_count of 1 is actually correct.

quoted

quoted

The increase/decrease of enable count of parent clock A is only driven by
the Linux kernel usage of clock 2.

quoted

quoted

Opinions or proposal how to fix/change this?


Best regards

Dirk

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/clk.c?id=2e20fbf592621b2c2aeddd82e0fa3dad053cce03

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