[PATCH v7 0/5] clk: Provide support for always-on clocks

[PATCH v7 0/5] clk: Provide support for always-on clocks

[Lee Jones]

Lots of platforms contain clocks which if turned off would prove fatal.
The only way to recover from these catastrophic failures is to restart
the board(s).  Now, when a clock provider is registered with the
framework it is possible for a list of critical clocks to be supplied
which must be kept ungated.  Each clock mentioned in the newly
introduced 'critical-clock' property will be clk_prepare_enable()d
where the normal references will be taken.  This will prevent the
common clk framework from attempting to gate them during the normal
clk_disable_unused() and disable_clock() procedures.

Note that it will still be possible for knowledgeable drivers to turn
these clocks off using clk_{enable,disable}_critical() calls.

Changelog:
v6 => v7:
  - Introduce API to enable and disable critical clocks
  - Rename 'always-on-clock' to 'critical-clock'

v5 => v6:
  - Use of_clk_get_from_provider() instead of of_clk_get_by_clkspec()
  - Explicitly describe expected DT values
  - Be pedantic with regards to printk() format specifiers

vX => v5:
  Implementations have changed drastically between versions, thus I
  would like for this set to be thought of independently from its
  predecessors.  The only reason for identifying as 'v5' is ease of
  differentiation on the list, which stems from the confusion caused
  by submitting 'v4' as a separate entity.

Lee Jones (5):
  ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
  ARM: sti: stih410-clocks: Identify critical clocks
  clk: Supply the critical clock {init, enable, disable} framework
  clk: Provide critical clock support
  clk: dt: Introduce binding for critical clock support

 .../devicetree/bindings/clock/clock-bindings.txt   | 39 +++++++++++++++++++
 arch/arm/boot/dts/stih410-clock.dtsi               | 10 +++++
 drivers/clk/clk-conf.c                             | 45 +++++++++++++++++++++-
 drivers/clk/clk.c                                  | 45 ++++++++++++++++++++++
 include/dt-bindings/clock/stih407-clks.h           |  4 ++
 include/linux/clk-provider.h                       |  2 +
 include/linux/clk.h                                | 30 +++++++++++++++
 7 files changed, 174 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH v7 1/5] ARM: sti: stih407-family: Supply defines for CLOCKGEN A0

[Lee Jones]

There are 2 LMI clocks generated by CLOCKGEN A0.  We wish to control
them individually and need to use these indexes to do so.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 include/dt-bindings/clock/stih407-clks.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/dt-bindings/clock/stih407-clks.h b/include/dt-bindings/clock/stih407-clks.h
index 7af2b71..082edd9 100644
--- a/include/dt-bindings/clock/stih407-clks.h
+++ b/include/dt-bindings/clock/stih407-clks.h
@@ -5,6 +5,10 @@
 #ifndef _DT_BINDINGS_CLK_STIH407
 #define _DT_BINDINGS_CLK_STIH407
 
+/* CLOCKGEN A0 */
+#define CLK_IC_LMI0		0
+#define CLK_IC_LMI1		1
+
 /* CLOCKGEN C0 */
 #define CLK_ICN_GPU		0
 #define CLK_FDMA		1
-- 
1.9.1

[PATCH v7 2/5] ARM: sti: stih410-clocks: Identify critical clocks

[Lee Jones]

Lots of platforms contain clocks which if turned off would prove fatal.
The only way to recover is to restart the board(s).  This driver takes
references to clocks which are required to be always-on.  The Common
Clk Framework will then take references to them.  This way they will
not be turned off during the clk_disabled_unused() procedure.

In this patch we are identifying clocks, which if gated would render
the STiH410 development board unserviceable.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih410-clock.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/stih410-clock.dtsi b/arch/arm/boot/dts/stih410-clock.dtsi
index 6b5803a..0a72c56 100644
--- a/arch/arm/boot/dts/stih410-clock.dtsi
+++ b/arch/arm/boot/dts/stih410-clock.dtsi
@@ -103,6 +103,7 @@
 				clocks = <&clk_sysin>;
 
 				clock-output-names = "clk-s-a0-pll-ofd-0";
+				critical-clock = <0>; /* clk-s-a0-pll-ofd-0 */
 			};
 
 			clk_s_a0_flexgen: clk-s-a0-flexgen {
@@ -115,6 +116,8 @@
 
 				clock-output-names = "clk-ic-lmi0",
 						     "clk-ic-lmi1";
+
+				critical-clock = <CLK_IC_LMI0>;
 			};
 		};
 
@@ -142,6 +145,7 @@
 				clocks = <&clk_sysin>;
 
 				clock-output-names = "clk-s-c0-pll0-odf-0";
+				critical-clock = <0>; /* clk-s-c0-pll0-odf-0 */
 			};
 
 			clk_s_c0_pll1: clk-s-c0-pll1 {
@@ -204,6 +208,12 @@
 						     "clk-clust-hades",
 						     "clk-hwpe-hades",
 						     "clk-fc-hades";
+
+				critical-clock = <CLK_ICN_CPU>,
+						 <CLK_TX_ICN_DMU>,
+						 <CLK_EXT2F_A9>,
+						 <CLK_ICN_LMI>,
+						 <CLK_ICN_SBC>;
 			};
 		};
 
-- 
1.9.1

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones]

These new API calls will firstly provide a mechanisms to tag a clock as
critical and secondly allow any knowledgeable driver to (un)gate clocks,
even if they are marked as critical.

Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 61c3fc5..486b1da 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -	Provides 'play' over critical clocks.  A clock can be
+ *			marked as critical, meaning that it should not be
+ *			disabled.  However, if a driver which is aware of the
+ *			critical behaviour wants to control it, it can do so
+ *			using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled	Is clock critical?  Once set, doesn't change
+ * @leave_on	Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+	bool enabled;
+	bool leave_on;
+};
+
 struct clk_core {
 	const char		*name;
 	const struct clk_ops	*ops;
@@ -75,6 +90,7 @@ struct clk_core {
 	struct dentry		*dentry;
 #endif
 	struct kref		ref;
+	struct critical		critical;
 };
 
 struct clk {
@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
 	if (WARN_ON(clk->enable_count == 0))
 		return;
 
+	/* Refuse to turn off a critical clock */
+	if (clk->enable_count == 1 && clk->critical.leave_on)
+		return;
+
 	if (--clk->enable_count > 0)
 		return;
 
@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 
+void clk_disable_critical(struct clk *clk)
+{
+	clk->core->critical.leave_on = false;
+	clk_disable(clk);
+}
+EXPORT_SYMBOL_GPL(clk_disable_critical);
+
 static int clk_core_enable(struct clk_core *clk)
 {
 	int ret = 0;
@@ -1100,6 +1127,15 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_enable);
 
+int clk_enable_critical(struct clk *clk)
+{
+	if (clk->core->critical.enabled)
+		clk->core->critical.leave_on = true;
+
+	return clk_enable(clk);
+}
+EXPORT_SYMBOL_GPL(clk_enable_critical);
+
 static unsigned long clk_core_round_rate_nolock(struct clk_core *clk,
 						unsigned long rate,
 						unsigned long min_rate,
@@ -2482,6 +2518,15 @@ fail_out:
 }
 EXPORT_SYMBOL_GPL(clk_register);
 
+void clk_init_critical(struct clk *clk)
+{
+	struct critical *critical = &clk->core->critical;
+
+	critical->enabled = true;
+	critical->leave_on = true;
+}
+EXPORT_SYMBOL_GPL(clk_init_critical);
+
 /*
  * Free memory allocated for a clock.
  * Caller must hold prepare_lock.
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5591ea7..15ef8c9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -563,6 +563,8 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
 void clk_unregister(struct clk *clk);
 void devm_clk_unregister(struct device *dev, struct clk *clk);
 
+void clk_init_critical(struct clk *clk);
+
 /* helper functions */
 const char *__clk_get_name(struct clk *clk);
 struct clk_hw *__clk_get_hw(struct clk *clk);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 8381bbf..9807f3b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -231,6 +231,19 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
 int clk_enable(struct clk *clk);
 
 /**
+ * clk_enable_critical - inform the system when the clock source should be
+ * 			 running, even if clock is critical.
+ * @clk: clock source
+ *
+ * If the clock can not be enabled/disabled, this should return success.
+ *
+ * May be called from atomic contexts.
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_enable_critical(struct clk *clk);
+
+/**
  * clk_disable - inform the system when the clock source is no longer required.
  * @clk: clock source
  *
@@ -247,6 +260,23 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_disable_critical - inform the system when the clock source is no
+ * 			  longer required, even if clock is critical.
+ * @clk: clock source
+ *
+ * Inform the system that a clock source is no longer required by
+ * a driver and may be shut down.
+ *
+ * May be called from atomic contexts.
+ *
+ * Implementation detail: if the clock source is shared between
+ * multiple drivers, clk_enable_critical() calls must be balanced
+ * by the same number of clk_disable_critical() calls for the clock
+ * source to be disabled.
+ */
+void clk_disable_critical(struct clk *clk);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
  * @clk: clock source
-- 
1.9.1

[PATCH v7 4/5] clk: Provide critical clock support

[Lee Jones]

Lots of platforms contain clocks which if turned off would prove fatal.
The only way to recover from these catastrophic failures is to restart
the board(s).  Now, when a clock provider is registered with the
framework it is possible for a list of critical clocks to be supplied
which must be kept ungated.  Each clock mentioned in the newly
introduced 'critical-clock' property will be clk_prepare_enable()d
where the normal references will be taken.  This will prevent the
common clk framework from attempting to gate them during the normal
clk_disable_unused() and disable_clock() procedures.

Note that it will still be possible for knowledgeable drivers to turn
these clocks off using clk_{enable,disable}_critical() calls.

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

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index aad4796..f83c1c2 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -116,6 +116,45 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 	return 0;
 }
 
+static int __set_critical_clocks(struct device_node *node, bool clk_supplier)
+{
+	struct of_phandle_args clkspec;
+	struct clk *clk;
+	struct property	*prop;
+	const __be32 *cur;
+	uint32_t index;
+	int ret;
+
+	if (!clk_supplier)
+		return 0;
+
+	of_property_for_each_u32(node, "critical-clock", prop, cur, index) {
+		clkspec.np = node;
+		clkspec.args_count = 1;
+		clkspec.args[0] = index;
+
+		clk = of_clk_get_from_provider(&clkspec);
+		if (IS_ERR(clk)) {
+			pr_err("clk: couldn't get clock %u for %s\n",
+				index, node->full_name);
+			return PTR_ERR(clk);
+		}
+
+		clk_init_critical(clk);
+
+		ret = clk_prepare_enable(clk);
+		if (ret) {
+			pr_err("Failed to enable clock %u for %s: %d\n",
+			       index, node->full_name, ret);
+			return ret;
+		}
+
+		pr_debug("Setting clock as critical %pC\n", clk);
+	}
+
+	return 0;
+}
+
 /**
  * of_clk_set_defaults() - parse and set assigned clocks configuration
  * @node: device node to apply clock settings for
@@ -139,6 +178,10 @@ int of_clk_set_defaults(struct device_node *node, bool clk_supplier)
 	if (rc < 0)
 		return rc;
 
-	return __set_clk_rates(node, clk_supplier);
+	rc = __set_clk_rates(node, clk_supplier);
+	if (rc < 0)
+		return rc;
+
+	return __set_critical_clocks(node, clk_supplier);
 }
 EXPORT_SYMBOL_GPL(of_clk_set_defaults);
-- 
1.9.1

[PATCH v7 5/5] clk: dt: Introduce binding for critical clock support

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 .../devicetree/bindings/clock/clock-bindings.txt   | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index 06fc6d5..4137034 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -44,6 +44,45 @@ For example:
   clocks by index. The names should reflect the clock output signal
   names for the device.
 
+critical-clock:	Some hardware contains bunches of clocks which, in normal
+		circumstances, must never be turned off.  If drivers a) fail to
+		obtain a reference to any of these or b) give up a previously
+		obtained reference during suspend, it is possible that some
+		Operating Systems might attempt to disable them to save power.
+		If this happens a platform can fail irrecoverably as a result.
+		Usually the only way to recover from these failures is to
+		reboot.
+
+		To avoid either of these two scenarios from catastrophically
+		disabling an otherwise perfectly healthy running system,
+		clocks can be identified as 'critical' using this property from
+		inside a clocksource's node.
+
+		This property is not to be abused.  It is only to be used to
+		protect platforms from being crippled by gated clocks, NOT as a
+		convenience function to avoid using the framework correctly
+		inside device drivers.
+
+		Expected values are hardware clock indices.  If the
+		clock-indices property (see below) is used, then supplied
+		values must correspond to one of the listed identifiers.
+		Using the clock-indices example below, hardware clock <2>
+		is missing, therefore it is considered invalid to then
+		list clock <2> as a critical clock.
+
+For example:
+
+    oscillator {
+	#clock-cells = <1>;
+	clock-output-names = "ckil", "ckih";
+	critical-clock = <0>, <1>;
+    };
+
+- this node defines a device with two clock outputs, just as in the
+  example above.  The only difference being that 'ckil' and 'ckih'
+  are now identified as an critical clocks, so an OS will know to
+  never attempt to gate them.
+
 clock-indices:	   If the identifying number for the clocks in the node
 		   is not linear from zero, then this allows the mapping of
 		   identifiers into the clock-output-names array.
-- 
1.9.1

[PATCH v7 5/5] clk: dt: Introduce binding for critical clock support

[Maxime Ripard]

Hi Lee,

On Wed, Jul 22, 2015 at 02:04:15PM +0100, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt   | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index 06fc6d5..4137034 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -44,6 +44,45 @@ For example:
>    clocks by index. The names should reflect the clock output signal
>    names for the device.
>  
> +critical-clock:	Some hardware contains bunches of clocks which, in normal
> +		circumstances, must never be turned off.  If drivers a) fail to
> +		obtain a reference to any of these or b) give up a previously
> +		obtained reference during suspend, it is possible that some
> +		Operating Systems might attempt to disable them to save power.
> +		If this happens a platform can fail irrecoverably as a result.
> +		Usually the only way to recover from these failures is to
> +		reboot.
> +
> +		To avoid either of these two scenarios from catastrophically
> +		disabling an otherwise perfectly healthy running system,
> +		clocks can be identified as 'critical' using this property from
> +		inside a clocksource's node.
> +
> +		This property is not to be abused.  It is only to be used to
> +		protect platforms from being crippled by gated clocks, NOT as a
> +		convenience function to avoid using the framework correctly
> +		inside device drivers.
> +
> +		Expected values are hardware clock indices.  If the
> +		clock-indices property (see below) is used, then supplied
> +		values must correspond to one of the listed identifiers.
> +		Using the clock-indices example below, hardware clock <2>
> +		is missing, therefore it is considered invalid to then
> +		list clock <2> as a critical clock.

I think we should also consider having it simply as a boolean. Using
indices for clocks that don't have any (for example because it only
provides a single clock) seem to not really make much sense.

Also, since you can have a bunch of them, using critical-clocks seem
more appropriate.

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/20150727/752ad1cd/attachment.sig>

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Maxime Ripard]

On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> These new API calls will firstly provide a mechanisms to tag a clock as
> critical and secondly allow any knowledgeable driver to (un)gate clocks,
> even if they are marked as critical.
> 
> Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk-provider.h |  2 ++
>  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 61c3fc5..486b1da 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
>  
>  /***    private data structures    ***/
>  
> +/**
> + * struct critical -	Provides 'play' over critical clocks.  A clock can be
> + *			marked as critical, meaning that it should not be
> + *			disabled.  However, if a driver which is aware of the
> + *			critical behaviour wants to control it, it can do so
> + *			using clk_enable_critical() and clk_disable_critical().
> + *
> + * @enabled	Is clock critical?  Once set, doesn't change
> + * @leave_on	Self explanatory.  Can be disabled by knowledgeable drivers
> + */
> +struct critical {
> +	bool enabled;
> +	bool leave_on;
> +};
> +
>  struct clk_core {
>  	const char		*name;
>  	const struct clk_ops	*ops;
> @@ -75,6 +90,7 @@ struct clk_core {
>  	struct dentry		*dentry;
>  #endif
>  	struct kref		ref;
> +	struct critical		critical;
>  };
>  
>  struct clk {
> @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
>  	if (WARN_ON(clk->enable_count == 0))
>  		return;
>  
> +	/* Refuse to turn off a critical clock */
> +	if (clk->enable_count == 1 && clk->critical.leave_on)
> +		return;
> +

I think it should be handled by a separate counting. Otherwise, if you
have two users that marked the clock as critical, and then one of them
disable it...

>  	if (--clk->enable_count > 0)
>  		return;
>  
> @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_disable);
>  
> +void clk_disable_critical(struct clk *clk)
> +{
> +	clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

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/20150727/ee6145cd/attachment.sig>

[PATCH v7 5/5] clk: dt: Introduce binding for critical clock support

[Lee Jones]

On Mon, 27 Jul 2015, Maxime Ripard wrote:

> Hi Lee,
> 
> On Wed, Jul 22, 2015 at 02:04:15PM +0100, Lee Jones wrote:
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../devicetree/bindings/clock/clock-bindings.txt   | 39 ++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > index 06fc6d5..4137034 100644
> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > @@ -44,6 +44,45 @@ For example:
> >    clocks by index. The names should reflect the clock output signal
> >    names for the device.
> >  
> > +critical-clock:	Some hardware contains bunches of clocks which, in normal
> > +		circumstances, must never be turned off.  If drivers a) fail to
> > +		obtain a reference to any of these or b) give up a previously
> > +		obtained reference during suspend, it is possible that some
> > +		Operating Systems might attempt to disable them to save power.
> > +		If this happens a platform can fail irrecoverably as a result.
> > +		Usually the only way to recover from these failures is to
> > +		reboot.
> > +
> > +		To avoid either of these two scenarios from catastrophically
> > +		disabling an otherwise perfectly healthy running system,
> > +		clocks can be identified as 'critical' using this property from
> > +		inside a clocksource's node.
> > +
> > +		This property is not to be abused.  It is only to be used to
> > +		protect platforms from being crippled by gated clocks, NOT as a
> > +		convenience function to avoid using the framework correctly
> > +		inside device drivers.
> > +
> > +		Expected values are hardware clock indices.  If the
> > +		clock-indices property (see below) is used, then supplied
> > +		values must correspond to one of the listed identifiers.
> > +		Using the clock-indices example below, hardware clock <2>
> > +		is missing, therefore it is considered invalid to then
> > +		list clock <2> as a critical clock.
> 
> I think we should also consider having it simply as a boolean. Using
> indices for clocks that don't have any (for example because it only
> provides a single clock) seem to not really make much sense.

Then how would you distinguish between the clocks if the provider
provides more than a single clock?

> Also, since you can have a bunch of them, using critical-clocks seem
> more appropriate.

I can change the name to critical-clocks, no problem.

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

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones]

On Mon, 27 Jul 2015, Maxime Ripard wrote:

> On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > These new API calls will firstly provide a mechanisms to tag a clock as
> > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > even if they are marked as critical.
> > 
> > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/clk-provider.h |  2 ++
> >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> >  3 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 61c3fc5..486b1da 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> >  
> >  /***    private data structures    ***/
> >  
> > +/**
> > + * struct critical -	Provides 'play' over critical clocks.  A clock can be
> > + *			marked as critical, meaning that it should not be
> > + *			disabled.  However, if a driver which is aware of the
> > + *			critical behaviour wants to control it, it can do so
> > + *			using clk_enable_critical() and clk_disable_critical().
> > + *
> > + * @enabled	Is clock critical?  Once set, doesn't change
> > + * @leave_on	Self explanatory.  Can be disabled by knowledgeable drivers
> > + */
> > +struct critical {
> > +	bool enabled;
> > +	bool leave_on;
> > +};
> > +
> >  struct clk_core {
> >  	const char		*name;
> >  	const struct clk_ops	*ops;
> > @@ -75,6 +90,7 @@ struct clk_core {
> >  	struct dentry		*dentry;
> >  #endif
> >  	struct kref		ref;
> > +	struct critical		critical;
> >  };
> >  
> >  struct clk {
> > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> >  	if (WARN_ON(clk->enable_count == 0))
> >  		return;
> >  
> > +	/* Refuse to turn off a critical clock */
> > +	if (clk->enable_count == 1 && clk->critical.leave_on)
> > +		return;
> > +
> 
> I think it should be handled by a separate counting. Otherwise, if you
> have two users that marked the clock as critical, and then one of them
> disable it...
> 
> >  	if (--clk->enable_count > 0)
> >  		return;
> >  
> > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_disable);
> >  
> > +void clk_disable_critical(struct clk *clk)
> > +{
> > +	clk->core->critical.leave_on = false;
> 
> .. you just lost the fact that it was critical in the first place.

I thought about both of these points, which is why I came up with this
strategy.

Any device which uses the *_critical() API should a) have knowledge of
what happens when a particular critical clock is gated and b) have
thought about the consequences.  I don't think we can use reference
counting, because we'd need as many critical clock owners as there are
critical clocks.  Cast your mind back to the reasons for this critical
clock API.  One of the most important intentions of this API is the
requirement mitigation for each of the critical clocks to have an owner
(driver).

With regards to your second point, that's what 'critical.enabled'
is for.  Take a look at clk_enable_critical().

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

[PATCH v7 5/5] clk: dt: Introduce binding for critical clock support

[Maxime Ripard]

On Mon, Jul 27, 2015 at 08:31:49AM +0100, Lee Jones wrote:
> On Mon, 27 Jul 2015, Maxime Ripard wrote:
> 
> > Hi Lee,
> > 
> > On Wed, Jul 22, 2015 at 02:04:15PM +0100, Lee Jones wrote:
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  .../devicetree/bindings/clock/clock-bindings.txt   | 39 ++++++++++++++++++++++
> > >  1 file changed, 39 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > index 06fc6d5..4137034 100644
> > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > @@ -44,6 +44,45 @@ For example:
> > >    clocks by index. The names should reflect the clock output signal
> > >    names for the device.
> > >  
> > > +critical-clock:	Some hardware contains bunches of clocks which, in normal
> > > +		circumstances, must never be turned off.  If drivers a) fail to
> > > +		obtain a reference to any of these or b) give up a previously
> > > +		obtained reference during suspend, it is possible that some
> > > +		Operating Systems might attempt to disable them to save power.
> > > +		If this happens a platform can fail irrecoverably as a result.
> > > +		Usually the only way to recover from these failures is to
> > > +		reboot.
> > > +
> > > +		To avoid either of these two scenarios from catastrophically
> > > +		disabling an otherwise perfectly healthy running system,
> > > +		clocks can be identified as 'critical' using this property from
> > > +		inside a clocksource's node.
> > > +
> > > +		This property is not to be abused.  It is only to be used to
> > > +		protect platforms from being crippled by gated clocks, NOT as a
> > > +		convenience function to avoid using the framework correctly
> > > +		inside device drivers.
> > > +
> > > +		Expected values are hardware clock indices.  If the
> > > +		clock-indices property (see below) is used, then supplied
> > > +		values must correspond to one of the listed identifiers.
> > > +		Using the clock-indices example below, hardware clock <2>
> > > +		is missing, therefore it is considered invalid to then
> > > +		list clock <2> as a critical clock.
> > 
> > I think we should also consider having it simply as a boolean. Using
> > indices for clocks that don't have any (for example because it only
> > provides a single clock) seem to not really make much sense.
> 
> Then how would you distinguish between the clocks if the provider
> provides more than a single clock?

What I had in mind was that, you would have three cases:

  - critical-clocks is not there: no clocks are made critical

  - critical-clocks is there, but doesn't have any values: all the
    clocks provided are marked critical

  - critical-clocks is there and it has a list of values: only the
    clocks listed are marked critical.

Does that make sense to you?

Thanks,
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/20150728/1de9ebb8/attachment-0001.sig>

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Maxime Ripard]

On Mon, Jul 27, 2015 at 09:53:38AM +0100, Lee Jones wrote:
> On Mon, 27 Jul 2015, Maxime Ripard wrote:
> 
> > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > even if they are marked as critical.
> > > 
> > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/clk-provider.h |  2 ++
> > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > >  3 files changed, 77 insertions(+)
> > > 
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 61c3fc5..486b1da 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > >  
> > >  /***    private data structures    ***/
> > >  
> > > +/**
> > > + * struct critical -	Provides 'play' over critical clocks.  A clock can be
> > > + *			marked as critical, meaning that it should not be
> > > + *			disabled.  However, if a driver which is aware of the
> > > + *			critical behaviour wants to control it, it can do so
> > > + *			using clk_enable_critical() and clk_disable_critical().
> > > + *
> > > + * @enabled	Is clock critical?  Once set, doesn't change
> > > + * @leave_on	Self explanatory.  Can be disabled by knowledgeable drivers
> > > + */
> > > +struct critical {
> > > +	bool enabled;
> > > +	bool leave_on;
> > > +};
> > > +
> > >  struct clk_core {
> > >  	const char		*name;
> > >  	const struct clk_ops	*ops;
> > > @@ -75,6 +90,7 @@ struct clk_core {
> > >  	struct dentry		*dentry;
> > >  #endif
> > >  	struct kref		ref;
> > > +	struct critical		critical;
> > >  };
> > >  
> > >  struct clk {
> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > >  	if (WARN_ON(clk->enable_count == 0))
> > >  		return;
> > >  
> > > +	/* Refuse to turn off a critical clock */
> > > +	if (clk->enable_count == 1 && clk->critical.leave_on)
> > > +		return;
> > > +
> > 
> > I think it should be handled by a separate counting. Otherwise, if you
> > have two users that marked the clock as critical, and then one of them
> > disable it...
> > 
> > >  	if (--clk->enable_count > 0)
> > >  		return;
> > >  
> > > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_disable);
> > >  
> > > +void clk_disable_critical(struct clk *clk)
> > > +{
> > > +	clk->core->critical.leave_on = false;
> > 
> > .. you just lost the fact that it was critical in the first place.
> 
> I thought about both of these points, which is why I came up with this
> strategy.
> 
> Any device which uses the *_critical() API should a) have knowledge of
> what happens when a particular critical clock is gated and b) have
> thought about the consequences.

Indeed.

> I don't think we can use reference counting, because we'd need as
> many critical clock owners as there are critical clocks.

Which we can have if we replace the call to clk_prepare_enable you add
in your fourth patch in __set_critical_clocks.

> Cast your mind back to the reasons for this critical clock API.  One
> of the most important intentions of this API is the requirement
> mitigation for each of the critical clocks to have an owner
> (driver).
> 
> With regards to your second point, that's what 'critical.enabled'
> is for.  Take a look at clk_enable_critical().

I don't think this addresses the issue, if you just throw more
customers at it, the issue remain with your implementation.

If you have three customers that used the critical API, and if on of
these calls clk_disable_critical, you're losing leave_on.

Which means that if there's one of the two users left that calls
clk_disable on it, the clock will actually be disabled, which is
clearly not what we want to do, as we have still a user that want the
clock to be enabled.

It would be much more robust to have another count for the critical
stuff, initialised to one by the __set_critical_clocks function.

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/20150728/f38b6aaf/attachment-0001.sig>

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones]

On Tue, 28 Jul 2015, Maxime Ripard wrote:

> On Mon, Jul 27, 2015 at 09:53:38AM +0100, Lee Jones wrote:
> > On Mon, 27 Jul 2015, Maxime Ripard wrote:
> > 
> > > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > > even if they are marked as critical.
> > > > 
> > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/clk-provider.h |  2 ++
> > > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > > >  3 files changed, 77 insertions(+)
> > > > 
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index 61c3fc5..486b1da 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > > >  
> > > >  /***    private data structures    ***/
> > > >  
> > > > +/**
> > > > + * struct critical -	Provides 'play' over critical clocks.  A clock can be
> > > > + *			marked as critical, meaning that it should not be
> > > > + *			disabled.  However, if a driver which is aware of the
> > > > + *			critical behaviour wants to control it, it can do so
> > > > + *			using clk_enable_critical() and clk_disable_critical().
> > > > + *
> > > > + * @enabled	Is clock critical?  Once set, doesn't change
> > > > + * @leave_on	Self explanatory.  Can be disabled by knowledgeable drivers
> > > > + */
> > > > +struct critical {
> > > > +	bool enabled;
> > > > +	bool leave_on;
> > > > +};
> > > > +
> > > >  struct clk_core {
> > > >  	const char		*name;
> > > >  	const struct clk_ops	*ops;
> > > > @@ -75,6 +90,7 @@ struct clk_core {
> > > >  	struct dentry		*dentry;
> > > >  #endif
> > > >  	struct kref		ref;
> > > > +	struct critical		critical;
> > > >  };
> > > >  
> > > >  struct clk {
> > > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > > >  	if (WARN_ON(clk->enable_count == 0))
> > > >  		return;
> > > >  
> > > > +	/* Refuse to turn off a critical clock */
> > > > +	if (clk->enable_count == 1 && clk->critical.leave_on)
> > > > +		return;
> > > > +
> > > 
> > > I think it should be handled by a separate counting. Otherwise, if you
> > > have two users that marked the clock as critical, and then one of them
> > > disable it...
> > > 
> > > >  	if (--clk->enable_count > 0)
> > > >  		return;
> > > >  
> > > > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(clk_disable);
> > > >  
> > > > +void clk_disable_critical(struct clk *clk)
> > > > +{
> > > > +	clk->core->critical.leave_on = false;
> > > 
> > > .. you just lost the fact that it was critical in the first place.
> > 
> > I thought about both of these points, which is why I came up with this
> > strategy.
> > 
> > Any device which uses the *_critical() API should a) have knowledge of
> > what happens when a particular critical clock is gated and b) have
> > thought about the consequences.
> 
> Indeed.
> 
> > I don't think we can use reference counting, because we'd need as
> > many critical clock owners as there are critical clocks.
> 
> Which we can have if we replace the call to clk_prepare_enable you add
> in your fourth patch in __set_critical_clocks.

What should it be replaced with?

> > Cast your mind back to the reasons for this critical clock API.  One
> > of the most important intentions of this API is the requirement
> > mitigation for each of the critical clocks to have an owner
> > (driver).
> > 
> > With regards to your second point, that's what 'critical.enabled'
> > is for.  Take a look at clk_enable_critical().
> 
> I don't think this addresses the issue, if you just throw more
> customers at it, the issue remain with your implementation.
> 
> If you have three customers that used the critical API, and if on of
> these calls clk_disable_critical, you're losing leave_on.

That's the idea.  See my point above, the one you replied "Indeed"
to.  So when a driver uses clk_disable_critical() it's saying, "I know
why this clock is a critical clock, and I know that nothing terrible
will happen if I disable it, as I have that covered".  So then if it's
not the last user to call clk_disable(), the last one out the door
will be allowed to finally gate the clock, regardless whether it's
critical aware or not.

Then, when we come to enable the clock again, the critical aware user
then re-marks the clock as leave_on, so not critical un-aware user can
take the final reference and disable the clock.

> Which means that if there's one of the two users left that calls
> clk_disable on it, the clock will actually be disabled, which is
> clearly not what we want to do, as we have still a user that want the
> clock to be enabled.

That's not what happens (at least it shouldn't if I've coded it up
right).  The API _still_ requires all of the users to give-up their
reference.

> It would be much more robust to have another count for the critical
> stuff, initialised to one by the __set_critical_clocks function.

If I understand you correctly, we already have a count.  We use the
original reference count.  No need for one of our own.

Using your RAM Clock (Clock 4) as an example
--------------------------------------------

Early start-up:
  Clock 4 is marked as critical and a reference is taken (ref == 1)

Driver probe:
  SPI enables Clock 4 (ref == 2)
  I2C enables Clock 4 (ref == 3)

Suspend (without RAM driver's permission):
  SPI disables Clock 4 (ref == 2)
  I2C disables Clock 4 (ref == 1)
  /*
   * Clock won't be gated because:
   *   .leave_on is True - can't dec final reference
   */

Suspend (with RAM driver's permission):
  /* Order is unimportant */
  SPI disables Clock 4 (ref == 2)
  RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0)
  I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */
  /*
   * Clock will be gated because:
   *   .leave_on is False, so (ref == 0)
   */

Resume:
  /* Order is unimportant */
  SPI enables Clock 4 (ref == 1)
  RAM enables Clock 4 and re-enables .leave_on (ref == 2)
  I2C enables Clock 4 (ref == 3)

Hopefully that clears things up.

Please tell me if the code doesn't reflect this strategy, or if you
can see anything wrong with how it operates.

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

[PATCH v7 0/5] clk: Provide support for always-on clocks

[Michael Turquette]

Quoting Lee Jones (2015-07-22 06:04:10)
> Lots of platforms contain clocks which if turned off would prove fatal.
> The only way to recover from these catastrophic failures is to restart
> the board(s).  Now, when a clock provider is registered with the
> framework it is possible for a list of critical clocks to be supplied
> which must be kept ungated.  Each clock mentioned in the newly
> introduced 'critical-clock' property will be clk_prepare_enable()d
> where the normal references will be taken.  This will prevent the
> common clk framework from attempting to gate them during the normal
> clk_disable_unused() and disable_clock() procedures.
> 
> Note that it will still be possible for knowledgeable drivers to turn
> these clocks off using clk_{enable,disable}_critical() calls.

Hi Lee,

Thanks for submitting the series.

It has been a little while since v6. It would be helpful to remind me of
why a new api is necessary, versus using the existing clk_prepare_unused
call at the time that a clock is enabled.

I'm looking over the patches in detail now, but one question stands out:
besides the DT use case, would a Linux device driver ever call
clk_enable_critical instead of using clk_enable? If so, why?

Thanks,
Mike

> 
> Changelog:
> v6 => v7:
>   - Introduce API to enable and disable critical clocks
>   - Rename 'always-on-clock' to 'critical-clock'
> 
> v5 => v6:
>   - Use of_clk_get_from_provider() instead of of_clk_get_by_clkspec()
>   - Explicitly describe expected DT values
>   - Be pedantic with regards to printk() format specifiers
> 
> vX => v5:
>   Implementations have changed drastically between versions, thus I
>   would like for this set to be thought of independently from its
>   predecessors.  The only reason for identifying as 'v5' is ease of
>   differentiation on the list, which stems from the confusion caused
>   by submitting 'v4' as a separate entity.
> 
> Lee Jones (5):
>   ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
>   ARM: sti: stih410-clocks: Identify critical clocks
>   clk: Supply the critical clock {init, enable, disable} framework
>   clk: Provide critical clock support
>   clk: dt: Introduce binding for critical clock support
> 
>  .../devicetree/bindings/clock/clock-bindings.txt   | 39 +++++++++++++++++++
>  arch/arm/boot/dts/stih410-clock.dtsi               | 10 +++++
>  drivers/clk/clk-conf.c                             | 45 +++++++++++++++++++++-
>  drivers/clk/clk.c                                  | 45 ++++++++++++++++++++++
>  include/dt-bindings/clock/stih407-clks.h           |  4 ++
>  include/linux/clk-provider.h                       |  2 +
>  include/linux/clk.h                                | 30 +++++++++++++++
>  7 files changed, 174 insertions(+), 1 deletion(-)
> 
> -- 
> 1.9.1
> 

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Michael Turquette]

Hi Lee,

+ linux-clk ml

Quoting Lee Jones (2015-07-22 06:04:13)
> These new API calls will firstly provide a mechanisms to tag a clock as
> critical and secondly allow any knowledgeable driver to (un)gate clocks,
> even if they are marked as critical.
> 
> Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk-provider.h |  2 ++
>  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 61c3fc5..486b1da 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
>  
>  /***    private data structures    ***/
>  
> +/**
> + * struct critical -   Provides 'play' over critical clocks.  A clock can be
> + *                     marked as critical, meaning that it should not be
> + *                     disabled.  However, if a driver which is aware of the
> + *                     critical behaviour wants to control it, it can do so
> + *                     using clk_enable_critical() and clk_disable_critical().
> + *
> + * @enabled    Is clock critical?  Once set, doesn't change
> + * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers

Not self explanatory. I need this explained to me. What does leave_on
do? Better yet, what would happen if leave_on did not exist?

> + */
> +struct critical {
> +       bool enabled;
> +       bool leave_on;
> +};
> +
>  struct clk_core {
>         const char              *name;
>         const struct clk_ops    *ops;
> @@ -75,6 +90,7 @@ struct clk_core {
>         struct dentry           *dentry;
>  #endif
>         struct kref             ref;
> +       struct critical         critical;
>  };
>  
>  struct clk {
> @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
>         if (WARN_ON(clk->enable_count == 0))
>                 return;
>  
> +       /* Refuse to turn off a critical clock */
> +       if (clk->enable_count == 1 && clk->critical.leave_on)
> +               return;

How do we get to this point? clk_enable_critical actually calls
clk_enable, thus incrementing the enable_count. The only time that we
could hit the above case is if,

a) there is an imbalance in clk_enable and clk_disable calls. If this is
the case then the drivers need to be fixed. Or better yet some
infrastructure to catch that, now that we have per-user struct clk
cookies.

b) a driver knowingly calls clk_enable_critical(foo) and then regular,
old clk_disable(foo). But why would a driver do that?

It might be that I am missing the point here, so please feel free to
clue me in.

Regards,
Mike

> +
>         if (--clk->enable_count > 0)
>                 return;
>  
> @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_disable);
>  
> +void clk_disable_critical(struct clk *clk)
> +{
> +       clk->core->critical.leave_on = false;
> +       clk_disable(clk);
> +}
> +EXPORT_SYMBOL_GPL(clk_disable_critical);
> +
>  static int clk_core_enable(struct clk_core *clk)
>  {
>         int ret = 0;
> @@ -1100,6 +1127,15 @@ int clk_enable(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_enable);
>  
> +int clk_enable_critical(struct clk *clk)
> +{
> +       if (clk->core->critical.enabled)
> +               clk->core->critical.leave_on = true;
> +
> +       return clk_enable(clk);
> +}
> +EXPORT_SYMBOL_GPL(clk_enable_critical);
> +
>  static unsigned long clk_core_round_rate_nolock(struct clk_core *clk,
>                                                 unsigned long rate,
>                                                 unsigned long min_rate,
> @@ -2482,6 +2518,15 @@ fail_out:
>  }
>  EXPORT_SYMBOL_GPL(clk_register);
>  
> +void clk_init_critical(struct clk *clk)
> +{
> +       struct critical *critical = &clk->core->critical;
> +
> +       critical->enabled = true;
> +       critical->leave_on = true;
> +}
> +EXPORT_SYMBOL_GPL(clk_init_critical);
> +
>  /*
>   * Free memory allocated for a clock.
>   * Caller must hold prepare_lock.
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 5591ea7..15ef8c9 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -563,6 +563,8 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
>  void clk_unregister(struct clk *clk);
>  void devm_clk_unregister(struct device *dev, struct clk *clk);
>  
> +void clk_init_critical(struct clk *clk);
> +
>  /* helper functions */
>  const char *__clk_get_name(struct clk *clk);
>  struct clk_hw *__clk_get_hw(struct clk *clk);
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 8381bbf..9807f3b 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -231,6 +231,19 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
>  int clk_enable(struct clk *clk);
>  
>  /**
> + * clk_enable_critical - inform the system when the clock source should be
> + *                      running, even if clock is critical.
> + * @clk: clock source
> + *
> + * If the clock can not be enabled/disabled, this should return success.
> + *
> + * May be called from atomic contexts.
> + *
> + * Returns success (0) or negative errno.
> + */
> +int clk_enable_critical(struct clk *clk);
> +
> +/**
>   * clk_disable - inform the system when the clock source is no longer required.
>   * @clk: clock source
>   *
> @@ -247,6 +260,23 @@ int clk_enable(struct clk *clk);
>  void clk_disable(struct clk *clk);
>  
>  /**
> + * clk_disable_critical - inform the system when the clock source is no
> + *                       longer required, even if clock is critical.
> + * @clk: clock source
> + *
> + * Inform the system that a clock source is no longer required by
> + * a driver and may be shut down.
> + *
> + * May be called from atomic contexts.
> + *
> + * Implementation detail: if the clock source is shared between
> + * multiple drivers, clk_enable_critical() calls must be balanced
> + * by the same number of clk_disable_critical() calls for the clock
> + * source to be disabled.
> + */
> +void clk_disable_critical(struct clk *clk);
> +
> +/**
>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>   *               This is only valid once the clock source has been enabled.
>   * @clk: clock source
> -- 
> 1.9.1
> 

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Michael Turquette]

Quoting Lee Jones (2015-07-28 06:00:55)
> On Tue, 28 Jul 2015, Maxime Ripard wrote:
> 
> > On Mon, Jul 27, 2015 at 09:53:38AM +0100, Lee Jones wrote:
> > > On Mon, 27 Jul 2015, Maxime Ripard wrote:
> > > 
> > > > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > > > even if they are marked as critical.
> > > > > 
> > > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/linux/clk-provider.h |  2 ++
> > > > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > > > >  3 files changed, 77 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > index 61c3fc5..486b1da 100644
> > > > > --- a/drivers/clk/clk.c
> > > > > +++ b/drivers/clk/clk.c
> > > > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > > > >  
> > > > >  /***    private data structures    ***/
> > > > >  
> > > > > +/**
> > > > > + * struct critical -   Provides 'play' over critical clocks.  A clock can be
> > > > > + *                     marked as critical, meaning that it should not be
> > > > > + *                     disabled.  However, if a driver which is aware of the
> > > > > + *                     critical behaviour wants to control it, it can do so
> > > > > + *                     using clk_enable_critical() and clk_disable_critical().
> > > > > + *
> > > > > + * @enabled    Is clock critical?  Once set, doesn't change
> > > > > + * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers
> > > > > + */
> > > > > +struct critical {
> > > > > +       bool enabled;
> > > > > +       bool leave_on;
> > > > > +};
> > > > > +
> > > > >  struct clk_core {
> > > > >         const char              *name;
> > > > >         const struct clk_ops    *ops;
> > > > > @@ -75,6 +90,7 @@ struct clk_core {
> > > > >         struct dentry           *dentry;
> > > > >  #endif
> > > > >         struct kref             ref;
> > > > > +       struct critical         critical;
> > > > >  };
> > > > >  
> > > > >  struct clk {
> > > > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > > > >         if (WARN_ON(clk->enable_count == 0))
> > > > >                 return;
> > > > >  
> > > > > +       /* Refuse to turn off a critical clock */
> > > > > +       if (clk->enable_count == 1 && clk->critical.leave_on)
> > > > > +               return;
> > > > > +
> > > > 
> > > > I think it should be handled by a separate counting. Otherwise, if you
> > > > have two users that marked the clock as critical, and then one of them
> > > > disable it...
> > > > 
> > > > >         if (--clk->enable_count > 0)
> > > > >                 return;
> > > > >  
> > > > > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(clk_disable);
> > > > >  
> > > > > +void clk_disable_critical(struct clk *clk)
> > > > > +{
> > > > > +       clk->core->critical.leave_on = false;
> > > > 
> > > > .. you just lost the fact that it was critical in the first place.
> > > 
> > > I thought about both of these points, which is why I came up with this
> > > strategy.
> > > 
> > > Any device which uses the *_critical() API should a) have knowledge of
> > > what happens when a particular critical clock is gated and b) have
> > > thought about the consequences.
> > 
> > Indeed.
> > 
> > > I don't think we can use reference counting, because we'd need as
> > > many critical clock owners as there are critical clocks.
> > 
> > Which we can have if we replace the call to clk_prepare_enable you add
> > in your fourth patch in __set_critical_clocks.
> 
> What should it be replaced with?
> 
> > > Cast your mind back to the reasons for this critical clock API.  One
> > > of the most important intentions of this API is the requirement
> > > mitigation for each of the critical clocks to have an owner
> > > (driver).
> > > 
> > > With regards to your second point, that's what 'critical.enabled'
> > > is for.  Take a look at clk_enable_critical().
> > 
> > I don't think this addresses the issue, if you just throw more
> > customers at it, the issue remain with your implementation.
> > 
> > If you have three customers that used the critical API, and if on of
> > these calls clk_disable_critical, you're losing leave_on.
> 
> That's the idea.  See my point above, the one you replied "Indeed"
> to.  So when a driver uses clk_disable_critical() it's saying, "I know
> why this clock is a critical clock, and I know that nothing terrible
> will happen if I disable it, as I have that covered".  So then if it's
> not the last user to call clk_disable(), the last one out the door
> will be allowed to finally gate the clock, regardless whether it's
> critical aware or not.
> 
> Then, when we come to enable the clock again, the critical aware user
> then re-marks the clock as leave_on, so not critical un-aware user can
> take the final reference and disable the clock.
> 
> > Which means that if there's one of the two users left that calls
> > clk_disable on it, the clock will actually be disabled, which is
> > clearly not what we want to do, as we have still a user that want the
> > clock to be enabled.
> 
> That's not what happens (at least it shouldn't if I've coded it up
> right).  The API _still_ requires all of the users to give-up their
> reference.
> 
> > It would be much more robust to have another count for the critical
> > stuff, initialised to one by the __set_critical_clocks function.
> 
> If I understand you correctly, we already have a count.  We use the
> original reference count.  No need for one of our own.
> 
> Using your RAM Clock (Clock 4) as an example
> --------------------------------------------
> 
> Early start-up:
>   Clock 4 is marked as critical and a reference is taken (ref == 1)
> 
> Driver probe:
>   SPI enables Clock 4 (ref == 2)
>   I2C enables Clock 4 (ref == 3)
> 
> Suspend (without RAM driver's permission):
>   SPI disables Clock 4 (ref == 2)
>   I2C disables Clock 4 (ref == 1)
>   /*
>    * Clock won't be gated because:
>    *   .leave_on is True - can't dec final reference

I am clearly missing the point. The clock won't be gated because the
enable_count is still 1! What does .leave_on do here?

>    */
> 
> Suspend (with RAM driver's permission):
>   /* Order is unimportant */
>   SPI disables Clock 4 (ref == 2)
>   RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0)
>   I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */
>   /*
>    * Clock will be gated because:
>    *   .leave_on is False, so (ref == 0)

Again, .leave_on does nothing new here. We gate the clock because the
reference count is 0.

>    */
> 
> Resume:
>   /* Order is unimportant */
>   SPI enables Clock 4 (ref == 1)
>   RAM enables Clock 4 and re-enables .leave_on (ref == 2)
>   I2C enables Clock 4 (ref == 3)

Same again. As soon as RAM calls clk_enable_critical the ref count goes
up. .leave_on does nothing as far as I can tell. The all works because
of the reference counting, which already exists before this patch
series.

Regards,
Mike

> 
> Hopefully that clears things up.
> 
> Please tell me if the code doesn't reflect this strategy, or if you
> can see anything wrong with how it operates.
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org ? Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Michael Turquette]

Quoting Lee Jones (2015-07-27 01:53:38)
> On Mon, 27 Jul 2015, Maxime Ripard wrote:
> 
> > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > even if they are marked as critical.
> > > 
> > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/clk-provider.h |  2 ++
> > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > >  3 files changed, 77 insertions(+)
> > > 
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 61c3fc5..486b1da 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > >  
> > >  /***    private data structures    ***/
> > >  
> > > +/**
> > > + * struct critical -       Provides 'play' over critical clocks.  A clock can be
> > > + *                 marked as critical, meaning that it should not be
> > > + *                 disabled.  However, if a driver which is aware of the
> > > + *                 critical behaviour wants to control it, it can do so
> > > + *                 using clk_enable_critical() and clk_disable_critical().
> > > + *
> > > + * @enabled        Is clock critical?  Once set, doesn't change
> > > + * @leave_on       Self explanatory.  Can be disabled by knowledgeable drivers
> > > + */
> > > +struct critical {
> > > +   bool enabled;
> > > +   bool leave_on;
> > > +};
> > > +
> > >  struct clk_core {
> > >     const char              *name;
> > >     const struct clk_ops    *ops;
> > > @@ -75,6 +90,7 @@ struct clk_core {
> > >     struct dentry           *dentry;
> > >  #endif
> > >     struct kref             ref;
> > > +   struct critical         critical;
> > >  };
> > >  
> > >  struct clk {
> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > >     if (WARN_ON(clk->enable_count == 0))
> > >             return;
> > >  
> > > +   /* Refuse to turn off a critical clock */
> > > +   if (clk->enable_count == 1 && clk->critical.leave_on)
> > > +           return;
> > > +
> > 
> > I think it should be handled by a separate counting. Otherwise, if you
> > have two users that marked the clock as critical, and then one of them
> > disable it...
> > 
> > >     if (--clk->enable_count > 0)
> > >             return;
> > >  
> > > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_disable);
> > >  
> > > +void clk_disable_critical(struct clk *clk)
> > > +{
> > > +   clk->core->critical.leave_on = false;
> > 
> > .. you just lost the fact that it was critical in the first place.
> 
> I thought about both of these points, which is why I came up with this
> strategy.
> 
> Any device which uses the *_critical() API should a) have knowledge of
> what happens when a particular critical clock is gated and b) have
> thought about the consequences.

If this statement above is true then I fail to see the need for a new
api. A driver which has a really great idea of when it is safe or unsafe
to gate a clock should call clk_prepare_enable at probe and then only
call clk_disable_unprepare once it is safe to do so.

The existing bookkeeping in the clock framework will do the rest.

Regards,
Mike

> I don't think we can use reference
> counting, because we'd need as many critical clock owners as there are
> critical clocks.  Cast your mind back to the reasons for this critical
> clock API.  One of the most important intentions of this API is the
> requirement mitigation for each of the critical clocks to have an owner
> (driver).
> 
> With regards to your second point, that's what 'critical.enabled'
> is for.  Take a look at clk_enable_critical().
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org ? Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

[PATCH v7 0/5] clk: Provide support for always-on clocks

[Lee Jones]

On Wed, 29 Jul 2015, Michael Turquette wrote:
> Quoting Lee Jones (2015-07-22 06:04:10)
> > Lots of platforms contain clocks which if turned off would prove fatal.
> > The only way to recover from these catastrophic failures is to restart
> > the board(s).  Now, when a clock provider is registered with the
> > framework it is possible for a list of critical clocks to be supplied
> > which must be kept ungated.  Each clock mentioned in the newly
> > introduced 'critical-clock' property will be clk_prepare_enable()d
> > where the normal references will be taken.  This will prevent the
> > common clk framework from attempting to gate them during the normal
> > clk_disable_unused() and disable_clock() procedures.
> > 
> > Note that it will still be possible for knowledgeable drivers to turn
> > these clocks off using clk_{enable,disable}_critical() calls.
> 
> Hi Lee,
> 
> Thanks for submitting the series.
> 
> It has been a little while since v6. It would be helpful to remind me of
> why a new api is necessary, versus using the existing clk_prepare_unused
> call at the time that a clock is enabled.
> 
> I'm looking over the patches in detail now, but one question stands out:
> besides the DT use case, would a Linux device driver ever call
> clk_enable_critical instead of using clk_enable? If so, why?

Looks like a have a lot of emails to reply to.  Let's start here and
work our way through them, there are good answers to all of your
reasonable questions.

This set only supports the DT way of marking clocks as critical, due
to the fact that that's the way we'll be using the API.  I can spend
more time, once we have the ground work done, writing a similar
generic call (or expand the current one, whatever's deemed the best
approach) which will allow this to be possible from C.

> > Changelog:
> > v6 => v7:
> >   - Introduce API to enable and disable critical clocks
> >   - Rename 'always-on-clock' to 'critical-clock'
> > 
> > v5 => v6:
> >   - Use of_clk_get_from_provider() instead of of_clk_get_by_clkspec()
> >   - Explicitly describe expected DT values
> >   - Be pedantic with regards to printk() format specifiers
> > 
> > vX => v5:
> >   Implementations have changed drastically between versions, thus I
> >   would like for this set to be thought of independently from its
> >   predecessors.  The only reason for identifying as 'v5' is ease of
> >   differentiation on the list, which stems from the confusion caused
> >   by submitting 'v4' as a separate entity.
> > 
> > Lee Jones (5):
> >   ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
> >   ARM: sti: stih410-clocks: Identify critical clocks
> >   clk: Supply the critical clock {init, enable, disable} framework
> >   clk: Provide critical clock support
> >   clk: dt: Introduce binding for critical clock support
> > 
> >  .../devicetree/bindings/clock/clock-bindings.txt   | 39 +++++++++++++++++++
> >  arch/arm/boot/dts/stih410-clock.dtsi               | 10 +++++
> >  drivers/clk/clk-conf.c                             | 45 +++++++++++++++++++++-
> >  drivers/clk/clk.c                                  | 45 ++++++++++++++++++++++
> >  include/dt-bindings/clock/stih407-clks.h           |  4 ++
> >  include/linux/clk-provider.h                       |  2 +
> >  include/linux/clk.h                                | 30 +++++++++++++++
> >  7 files changed, 174 insertions(+), 1 deletion(-)
> > 

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

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones]

On Wed, 29 Jul 2015, Michael Turquette wrote:
> Quoting Lee Jones (2015-07-27 01:53:38)
> > On Mon, 27 Jul 2015, Maxime Ripard wrote:
> > 
> > > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > > even if they are marked as critical.
> > > > 
> > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/clk-provider.h |  2 ++
> > > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > > >  3 files changed, 77 insertions(+)
> > > > 
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index 61c3fc5..486b1da 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > > >  
> > > >  /***    private data structures    ***/
> > > >  
> > > > +/**
> > > > + * struct critical -       Provides 'play' over critical clocks.  A clock can be
> > > > + *                 marked as critical, meaning that it should not be
> > > > + *                 disabled.  However, if a driver which is aware of the
> > > > + *                 critical behaviour wants to control it, it can do so
> > > > + *                 using clk_enable_critical() and clk_disable_critical().
> > > > + *
> > > > + * @enabled        Is clock critical?  Once set, doesn't change
> > > > + * @leave_on       Self explanatory.  Can be disabled by knowledgeable drivers
> > > > + */
> > > > +struct critical {
> > > > +   bool enabled;
> > > > +   bool leave_on;
> > > > +};
> > > > +
> > > >  struct clk_core {
> > > >     const char              *name;
> > > >     const struct clk_ops    *ops;
> > > > @@ -75,6 +90,7 @@ struct clk_core {
> > > >     struct dentry           *dentry;
> > > >  #endif
> > > >     struct kref             ref;
> > > > +   struct critical         critical;
> > > >  };
> > > >  
> > > >  struct clk {
> > > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > > >     if (WARN_ON(clk->enable_count == 0))
> > > >             return;
> > > >  
> > > > +   /* Refuse to turn off a critical clock */
> > > > +   if (clk->enable_count == 1 && clk->critical.leave_on)
> > > > +           return;
> > > > +
> > > 
> > > I think it should be handled by a separate counting. Otherwise, if you
> > > have two users that marked the clock as critical, and then one of them
> > > disable it...
> > > 
> > > >     if (--clk->enable_count > 0)
> > > >             return;
> > > >  
> > > > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(clk_disable);
> > > >  
> > > > +void clk_disable_critical(struct clk *clk)
> > > > +{
> > > > +   clk->core->critical.leave_on = false;
> > > 
> > > .. you just lost the fact that it was critical in the first place.
> > 
> > I thought about both of these points, which is why I came up with this
> > strategy.
> > 
> > Any device which uses the *_critical() API should a) have knowledge of
> > what happens when a particular critical clock is gated and b) have
> > thought about the consequences.
> 
> If this statement above is true then I fail to see the need for a new
> api. A driver which has a really great idea of when it is safe or unsafe
> to gate a clock should call clk_prepare_enable at probe and then only
> call clk_disable_unprepare once it is safe to do so.
> 
> The existing bookkeeping in the clock framework will do the rest.

I think you are viewing this particular API back-to-front.  The idea
is to mark all of the critical clocks at start-up by taking a
reference.  Then, if there are no knowledgable drivers who wish to
turn the clock off, the CCF will leave the clock ungated becuase the
reference count will always be >0.

The clk_{disable,enable}_critical() calls are to be used by
knowledgable drivers to say "[disable] I know what I'm doing and it's
okay for this clock to be turned off" and "[enable] right I'm done
with this clock now, let's turn it back on and mark it back as
critical, so no one else can turn it off".

To put things simply, the knowledgable driver will _not_ be enabling
the clock in the first place.  The first interaction it has with it is
to gate it.  Then, once it's done, it will enable it again and mark it
back up as critical.

Still confused ... let's go on another Q in one of your other emails
for another way of putting it.

> > I don't think we can use reference
> > counting, because we'd need as many critical clock owners as there are
> > critical clocks.  Cast your mind back to the reasons for this critical
> > clock API.  One of the most important intentions of this API is the
> > requirement mitigation for each of the critical clocks to have an owner
> > (driver).
> > 
> > With regards to your second point, that's what 'critical.enabled'
> > is for.  Take a look at clk_enable_critical().
> > 

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

[PATCH v7 5/5] clk: dt: Introduce binding for critical clock support

[Lee Jones]

On Tue, 28 Jul 2015, Maxime Ripard wrote:

> On Mon, Jul 27, 2015 at 08:31:49AM +0100, Lee Jones wrote:
> > On Mon, 27 Jul 2015, Maxime Ripard wrote:
> > 
> > > Hi Lee,
> > > 
> > > On Wed, Jul 22, 2015 at 02:04:15PM +0100, Lee Jones wrote:
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  .../devicetree/bindings/clock/clock-bindings.txt   | 39 ++++++++++++++++++++++
> > > >  1 file changed, 39 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > index 06fc6d5..4137034 100644
> > > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > @@ -44,6 +44,45 @@ For example:
> > > >    clocks by index. The names should reflect the clock output signal
> > > >    names for the device.
> > > >  
> > > > +critical-clock:	Some hardware contains bunches of clocks which, in normal
> > > > +		circumstances, must never be turned off.  If drivers a) fail to
> > > > +		obtain a reference to any of these or b) give up a previously
> > > > +		obtained reference during suspend, it is possible that some
> > > > +		Operating Systems might attempt to disable them to save power.
> > > > +		If this happens a platform can fail irrecoverably as a result.
> > > > +		Usually the only way to recover from these failures is to
> > > > +		reboot.
> > > > +
> > > > +		To avoid either of these two scenarios from catastrophically
> > > > +		disabling an otherwise perfectly healthy running system,
> > > > +		clocks can be identified as 'critical' using this property from
> > > > +		inside a clocksource's node.
> > > > +
> > > > +		This property is not to be abused.  It is only to be used to
> > > > +		protect platforms from being crippled by gated clocks, NOT as a
> > > > +		convenience function to avoid using the framework correctly
> > > > +		inside device drivers.
> > > > +
> > > > +		Expected values are hardware clock indices.  If the
> > > > +		clock-indices property (see below) is used, then supplied
> > > > +		values must correspond to one of the listed identifiers.
> > > > +		Using the clock-indices example below, hardware clock <2>
> > > > +		is missing, therefore it is considered invalid to then
> > > > +		list clock <2> as a critical clock.
> > > 
> > > I think we should also consider having it simply as a boolean. Using
> > > indices for clocks that don't have any (for example because it only
> > > provides a single clock) seem to not really make much sense.
> > 
> > Then how would you distinguish between the clocks if the provider
> > provides more than a single clock?
> 
> What I had in mind was that, you would have three cases:
> 
>   - critical-clocks is not there: no clocks are made critical
> 
>   - critical-clocks is there, but doesn't have any values: all the
>     clocks provided are marked critical
> 
>   - critical-clocks is there and it has a list of values: only the
>     clocks listed are marked critical.
> 
> Does that make sense to you?

Yep, sounds good.

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

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones]

On Wed, 29 Jul 2015, Michael Turquette wrote:
> Quoting Lee Jones (2015-07-28 06:00:55)
> > On Tue, 28 Jul 2015, Maxime Ripard wrote:
> > 
> > > On Mon, Jul 27, 2015 at 09:53:38AM +0100, Lee Jones wrote:
> > > > On Mon, 27 Jul 2015, Maxime Ripard wrote:
> > > > 
> > > > > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > > > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > > > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > > > > even if they are marked as critical.
> > > > > > 
> > > > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > ---
> > > > > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/linux/clk-provider.h |  2 ++
> > > > > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > > > > >  3 files changed, 77 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > > index 61c3fc5..486b1da 100644
> > > > > > --- a/drivers/clk/clk.c
> > > > > > +++ b/drivers/clk/clk.c
> > > > > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > > > > >  
> > > > > >  /***    private data structures    ***/
> > > > > >  
> > > > > > +/**
> > > > > > + * struct critical -   Provides 'play' over critical clocks.  A clock can be
> > > > > > + *                     marked as critical, meaning that it should not be
> > > > > > + *                     disabled.  However, if a driver which is aware of the
> > > > > > + *                     critical behaviour wants to control it, it can do so
> > > > > > + *                     using clk_enable_critical() and clk_disable_critical().
> > > > > > + *
> > > > > > + * @enabled    Is clock critical?  Once set, doesn't change
> > > > > > + * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers
> > > > > > + */
> > > > > > +struct critical {
> > > > > > +       bool enabled;
> > > > > > +       bool leave_on;
> > > > > > +};
> > > > > > +
> > > > > >  struct clk_core {
> > > > > >         const char              *name;
> > > > > >         const struct clk_ops    *ops;
> > > > > > @@ -75,6 +90,7 @@ struct clk_core {
> > > > > >         struct dentry           *dentry;
> > > > > >  #endif
> > > > > >         struct kref             ref;
> > > > > > +       struct critical         critical;
> > > > > >  };
> > > > > >  
> > > > > >  struct clk {
> > > > > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > > > > >         if (WARN_ON(clk->enable_count == 0))
> > > > > >                 return;
> > > > > >  
> > > > > > +       /* Refuse to turn off a critical clock */
> > > > > > +       if (clk->enable_count == 1 && clk->critical.leave_on)
> > > > > > +               return;
> > > > > > +
> > > > > 
> > > > > I think it should be handled by a separate counting. Otherwise, if you
> > > > > have two users that marked the clock as critical, and then one of them
> > > > > disable it...
> > > > > 
> > > > > >         if (--clk->enable_count > 0)
> > > > > >                 return;
> > > > > >  
> > > > > > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(clk_disable);
> > > > > >  
> > > > > > +void clk_disable_critical(struct clk *clk)
> > > > > > +{
> > > > > > +       clk->core->critical.leave_on = false;
> > > > > 
> > > > > .. you just lost the fact that it was critical in the first place.
> > > > 
> > > > I thought about both of these points, which is why I came up with this
> > > > strategy.
> > > > 
> > > > Any device which uses the *_critical() API should a) have knowledge of
> > > > what happens when a particular critical clock is gated and b) have
> > > > thought about the consequences.
> > > 
> > > Indeed.
> > > 
> > > > I don't think we can use reference counting, because we'd need as
> > > > many critical clock owners as there are critical clocks.
> > > 
> > > Which we can have if we replace the call to clk_prepare_enable you add
> > > in your fourth patch in __set_critical_clocks.
> > 
> > What should it be replaced with?
> > 
> > > > Cast your mind back to the reasons for this critical clock API.  One
> > > > of the most important intentions of this API is the requirement
> > > > mitigation for each of the critical clocks to have an owner
> > > > (driver).
> > > > 
> > > > With regards to your second point, that's what 'critical.enabled'
> > > > is for.  Take a look at clk_enable_critical().
> > > 
> > > I don't think this addresses the issue, if you just throw more
> > > customers at it, the issue remain with your implementation.
> > > 
> > > If you have three customers that used the critical API, and if on of
> > > these calls clk_disable_critical, you're losing leave_on.
> > 
> > That's the idea.  See my point above, the one you replied "Indeed"
> > to.  So when a driver uses clk_disable_critical() it's saying, "I know
> > why this clock is a critical clock, and I know that nothing terrible
> > will happen if I disable it, as I have that covered".  So then if it's
> > not the last user to call clk_disable(), the last one out the door
> > will be allowed to finally gate the clock, regardless whether it's
> > critical aware or not.
> > 
> > Then, when we come to enable the clock again, the critical aware user
> > then re-marks the clock as leave_on, so not critical un-aware user can
> > take the final reference and disable the clock.
> > 
> > > Which means that if there's one of the two users left that calls
> > > clk_disable on it, the clock will actually be disabled, which is
> > > clearly not what we want to do, as we have still a user that want the
> > > clock to be enabled.
> > 
> > That's not what happens (at least it shouldn't if I've coded it up
> > right).  The API _still_ requires all of the users to give-up their
> > reference.
> > 
> > > It would be much more robust to have another count for the critical
> > > stuff, initialised to one by the __set_critical_clocks function.
> > 
> > If I understand you correctly, we already have a count.  We use the
> > original reference count.  No need for one of our own.
> > 
> > Using your RAM Clock (Clock 4) as an example
> > --------------------------------------------
> > 
> > Early start-up:
> >   Clock 4 is marked as critical and a reference is taken (ref == 1)
> > 
> > Driver probe:
> >   SPI enables Clock 4 (ref == 2)
> >   I2C enables Clock 4 (ref == 3)
> > 
> > Suspend (without RAM driver's permission):
> >   SPI disables Clock 4 (ref == 2)
> >   I2C disables Clock 4 (ref == 1)
> >   /*
> >    * Clock won't be gated because:
> >    *   .leave_on is True - can't dec final reference
> 
> I am clearly missing the point. The clock won't be gated because the
> enable_count is still 1! What does .leave_on do here?

The point of _this_ (the extended) part of the API is so that the
clock _can_ be turned off.  Without the possibility to disable
.leave_on and the logic with accompanies it (i.e.
clk_disable_critical()) the clock will _never_ be gated.

> >    */
> > 
> > Suspend (with RAM driver's permission):
> >   /* Order is unimportant */
> >   SPI disables Clock 4 (ref == 2)
> >   RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0)
> >   I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */
> >   /*
> >    * Clock will be gated because:
> >    *   .leave_on is False, so (ref == 0)
> 
> Again, .leave_on does nothing new here. We gate the clock because the
> reference count is 0.

It's the fact that .leave_on has been disabled in
clk_disable_critical() that allows the final reference to be taken.

> >    */
> > 
> > Resume:
> >   /* Order is unimportant */
> >   SPI enables Clock 4 (ref == 1)
> >   RAM enables Clock 4 and re-enables .leave_on (ref == 2)
> >   I2C enables Clock 4 (ref == 3)
> 
> Same again. As soon as RAM calls clk_enable_critical the ref count goes
> up. .leave_on does nothing as far as I can tell. The all works because
> of the reference counting, which already exists before this patch
> series.

So fundamentally you're right in what you say.  All you really need to
disable a critical clock is write a knowledgeable driver, which is
intentionally unbalanced i.e. just calls clk_disable().  All this
extended API really does is makes the process more official and
ensures that an unintentionally unbalanced driver doesn't bugger up
the running platform.  We could also add a new WARN() to say that said
driver is unbalanced, as it just tried to turn off a critical clock.

What do you think is best?

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

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones]

On Wed, 29 Jul 2015, Michael Turquette wrote:

> Hi Lee,
> 
> + linux-clk ml
> 
> Quoting Lee Jones (2015-07-22 06:04:13)
> > These new API calls will firstly provide a mechanisms to tag a clock as
> > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > even if they are marked as critical.
> > 
> > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/clk-provider.h |  2 ++
> >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> >  3 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 61c3fc5..486b1da 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> >  
> >  /***    private data structures    ***/
> >  
> > +/**
> > + * struct critical -   Provides 'play' over critical clocks.  A clock can be
> > + *                     marked as critical, meaning that it should not be
> > + *                     disabled.  However, if a driver which is aware of the
> > + *                     critical behaviour wants to control it, it can do so
> > + *                     using clk_enable_critical() and clk_disable_critical().
> > + *
> > + * @enabled    Is clock critical?  Once set, doesn't change
> > + * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers
> 
> Not self explanatory. I need this explained to me. What does leave_on
> do? Better yet, what would happen if leave_on did not exist?
> 
> > + */
> > +struct critical {
> > +       bool enabled;
> > +       bool leave_on;
> > +};
> > +
> >  struct clk_core {
> >         const char              *name;
> >         const struct clk_ops    *ops;
> > @@ -75,6 +90,7 @@ struct clk_core {
> >         struct dentry           *dentry;
> >  #endif
> >         struct kref             ref;
> > +       struct critical         critical;
> >  };
> >  
> >  struct clk {
> > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> >         if (WARN_ON(clk->enable_count == 0))
> >                 return;
> >  
> > +       /* Refuse to turn off a critical clock */
> > +       if (clk->enable_count == 1 && clk->critical.leave_on)
> > +               return;
> 
> How do we get to this point? clk_enable_critical actually calls
> clk_enable, thus incrementing the enable_count. The only time that we
> could hit the above case is if,
> 
> a) there is an imbalance in clk_enable and clk_disable calls. If this is
> the case then the drivers need to be fixed. Or better yet some
> infrastructure to catch that, now that we have per-user struct clk
> cookies.
> 
> b) a driver knowingly calls clk_enable_critical(foo) and then regular,
> old clk_disable(foo). But why would a driver do that?
> 
> It might be that I am missing the point here, so please feel free to
> clue me in.

This check behaves in a very similar to the WARN() above.  It's more
of a fail-safe.  If all drivers are behaving properly, then it
shouldn't ever be true.  If they're not, it prevents an incorrectly
written driver from irrecoverably crippling the system.

As I said in the other mail.  We can do without these 3 new wrappers.
We _could_ just write a driver which only calls clk_enable() _after_
it calls clk_disable(), a kind of intentional unbalance and it would
do that same thing.  However, what we're trying to do here is provide
a proper API, so we can see at first glance what the 'knowledgeable'
driver is trying to do and not have someone attempt to submit a 'fix'
which calls clk_enable() or something.

> > +
> >         if (--clk->enable_count > 0)
> >                 return;
> >  
> > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_disable);
> >  
> > +void clk_disable_critical(struct clk *clk)
> > +{
> > +       clk->core->critical.leave_on = false;
> > +       clk_disable(clk);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_disable_critical);
> > +
> >  static int clk_core_enable(struct clk_core *clk)
> >  {
> >         int ret = 0;
> > @@ -1100,6 +1127,15 @@ int clk_enable(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_enable);
> >  
> > +int clk_enable_critical(struct clk *clk)
> > +{
> > +       if (clk->core->critical.enabled)
> > +               clk->core->critical.leave_on = true;
> > +
> > +       return clk_enable(clk);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_enable_critical);
> > +
> >  static unsigned long clk_core_round_rate_nolock(struct clk_core *clk,
> >                                                 unsigned long rate,
> >                                                 unsigned long min_rate,
> > @@ -2482,6 +2518,15 @@ fail_out:
> >  }
> >  EXPORT_SYMBOL_GPL(clk_register);
> >  
> > +void clk_init_critical(struct clk *clk)
> > +{
> > +       struct critical *critical = &clk->core->critical;
> > +
> > +       critical->enabled = true;
> > +       critical->leave_on = true;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_init_critical);
> > +
> >  /*
> >   * Free memory allocated for a clock.
> >   * Caller must hold prepare_lock.
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 5591ea7..15ef8c9 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -563,6 +563,8 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
> >  void clk_unregister(struct clk *clk);
> >  void devm_clk_unregister(struct device *dev, struct clk *clk);
> >  
> > +void clk_init_critical(struct clk *clk);
> > +
> >  /* helper functions */
> >  const char *__clk_get_name(struct clk *clk);
> >  struct clk_hw *__clk_get_hw(struct clk *clk);
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index 8381bbf..9807f3b 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -231,6 +231,19 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
> >  int clk_enable(struct clk *clk);
> >  
> >  /**
> > + * clk_enable_critical - inform the system when the clock source should be
> > + *                      running, even if clock is critical.
> > + * @clk: clock source
> > + *
> > + * If the clock can not be enabled/disabled, this should return success.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Returns success (0) or negative errno.
> > + */
> > +int clk_enable_critical(struct clk *clk);
> > +
> > +/**
> >   * clk_disable - inform the system when the clock source is no longer required.
> >   * @clk: clock source
> >   *
> > @@ -247,6 +260,23 @@ int clk_enable(struct clk *clk);
> >  void clk_disable(struct clk *clk);
> >  
> >  /**
> > + * clk_disable_critical - inform the system when the clock source is no
> > + *                       longer required, even if clock is critical.
> > + * @clk: clock source
> > + *
> > + * Inform the system that a clock source is no longer required by
> > + * a driver and may be shut down.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Implementation detail: if the clock source is shared between
> > + * multiple drivers, clk_enable_critical() calls must be balanced
> > + * by the same number of clk_disable_critical() calls for the clock
> > + * source to be disabled.
> > + */
> > +void clk_disable_critical(struct clk *clk);
> > +
> > +/**
> >   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
> >   *               This is only valid once the clock source has been enabled.
> >   * @clk: clock source

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

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Michael Turquette]

Quoting Lee Jones (2015-07-30 02:50:14)
> On Wed, 29 Jul 2015, Michael Turquette wrote:
> > Quoting Lee Jones (2015-07-28 06:00:55)
> > > On Tue, 28 Jul 2015, Maxime Ripard wrote:
> > > 
> > > > On Mon, Jul 27, 2015 at 09:53:38AM +0100, Lee Jones wrote:
> > > > > On Mon, 27 Jul 2015, Maxime Ripard wrote:
> > > > > 
> > > > > > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > > > > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > > > > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > > > > > even if they are marked as critical.
> > > > > > > 
> > > > > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > ---
> > > > > > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  include/linux/clk-provider.h |  2 ++
> > > > > > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > > > > > >  3 files changed, 77 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > > > index 61c3fc5..486b1da 100644
> > > > > > > --- a/drivers/clk/clk.c
> > > > > > > +++ b/drivers/clk/clk.c
> > > > > > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > > > > > >  
> > > > > > >  /***    private data structures    ***/
> > > > > > >  
> > > > > > > +/**
> > > > > > > + * struct critical -   Provides 'play' over critical clocks.  A clock can be
> > > > > > > + *                     marked as critical, meaning that it should not be
> > > > > > > + *                     disabled.  However, if a driver which is aware of the
> > > > > > > + *                     critical behaviour wants to control it, it can do so
> > > > > > > + *                     using clk_enable_critical() and clk_disable_critical().
> > > > > > > + *
> > > > > > > + * @enabled    Is clock critical?  Once set, doesn't change
> > > > > > > + * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers
> > > > > > > + */
> > > > > > > +struct critical {
> > > > > > > +       bool enabled;
> > > > > > > +       bool leave_on;
> > > > > > > +};
> > > > > > > +
> > > > > > >  struct clk_core {
> > > > > > >         const char              *name;
> > > > > > >         const struct clk_ops    *ops;
> > > > > > > @@ -75,6 +90,7 @@ struct clk_core {
> > > > > > >         struct dentry           *dentry;
> > > > > > >  #endif
> > > > > > >         struct kref             ref;
> > > > > > > +       struct critical         critical;
> > > > > > >  };
> > > > > > >  
> > > > > > >  struct clk {
> > > > > > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > > > > > >         if (WARN_ON(clk->enable_count == 0))
> > > > > > >                 return;
> > > > > > >  
> > > > > > > +       /* Refuse to turn off a critical clock */
> > > > > > > +       if (clk->enable_count == 1 && clk->critical.leave_on)
> > > > > > > +               return;
> > > > > > > +
> > > > > > 
> > > > > > I think it should be handled by a separate counting. Otherwise, if you
> > > > > > have two users that marked the clock as critical, and then one of them
> > > > > > disable it...
> > > > > > 
> > > > > > >         if (--clk->enable_count > 0)
> > > > > > >                 return;
> > > > > > >  
> > > > > > > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL_GPL(clk_disable);
> > > > > > >  
> > > > > > > +void clk_disable_critical(struct clk *clk)
> > > > > > > +{
> > > > > > > +       clk->core->critical.leave_on = false;
> > > > > > 
> > > > > > .. you just lost the fact that it was critical in the first place.
> > > > > 
> > > > > I thought about both of these points, which is why I came up with this
> > > > > strategy.
> > > > > 
> > > > > Any device which uses the *_critical() API should a) have knowledge of
> > > > > what happens when a particular critical clock is gated and b) have
> > > > > thought about the consequences.
> > > > 
> > > > Indeed.
> > > > 
> > > > > I don't think we can use reference counting, because we'd need as
> > > > > many critical clock owners as there are critical clocks.
> > > > 
> > > > Which we can have if we replace the call to clk_prepare_enable you add
> > > > in your fourth patch in __set_critical_clocks.
> > > 
> > > What should it be replaced with?
> > > 
> > > > > Cast your mind back to the reasons for this critical clock API.  One
> > > > > of the most important intentions of this API is the requirement
> > > > > mitigation for each of the critical clocks to have an owner
> > > > > (driver).
> > > > > 
> > > > > With regards to your second point, that's what 'critical.enabled'
> > > > > is for.  Take a look at clk_enable_critical().
> > > > 
> > > > I don't think this addresses the issue, if you just throw more
> > > > customers at it, the issue remain with your implementation.
> > > > 
> > > > If you have three customers that used the critical API, and if on of
> > > > these calls clk_disable_critical, you're losing leave_on.
> > > 
> > > That's the idea.  See my point above, the one you replied "Indeed"
> > > to.  So when a driver uses clk_disable_critical() it's saying, "I know
> > > why this clock is a critical clock, and I know that nothing terrible
> > > will happen if I disable it, as I have that covered".  So then if it's
> > > not the last user to call clk_disable(), the last one out the door
> > > will be allowed to finally gate the clock, regardless whether it's
> > > critical aware or not.
> > > 
> > > Then, when we come to enable the clock again, the critical aware user
> > > then re-marks the clock as leave_on, so not critical un-aware user can
> > > take the final reference and disable the clock.
> > > 
> > > > Which means that if there's one of the two users left that calls
> > > > clk_disable on it, the clock will actually be disabled, which is
> > > > clearly not what we want to do, as we have still a user that want the
> > > > clock to be enabled.
> > > 
> > > That's not what happens (at least it shouldn't if I've coded it up
> > > right).  The API _still_ requires all of the users to give-up their
> > > reference.
> > > 
> > > > It would be much more robust to have another count for the critical
> > > > stuff, initialised to one by the __set_critical_clocks function.
> > > 
> > > If I understand you correctly, we already have a count.  We use the
> > > original reference count.  No need for one of our own.
> > > 
> > > Using your RAM Clock (Clock 4) as an example
> > > --------------------------------------------
> > > 
> > > Early start-up:
> > >   Clock 4 is marked as critical and a reference is taken (ref == 1)
> > > 
> > > Driver probe:
> > >   SPI enables Clock 4 (ref == 2)
> > >   I2C enables Clock 4 (ref == 3)
> > > 
> > > Suspend (without RAM driver's permission):
> > >   SPI disables Clock 4 (ref == 2)
> > >   I2C disables Clock 4 (ref == 1)
> > >   /*
> > >    * Clock won't be gated because:
> > >    *   .leave_on is True - can't dec final reference
> > 
> > I am clearly missing the point. The clock won't be gated because the
> > enable_count is still 1! What does .leave_on do here?
> 
> The point of _this_ (the extended) part of the API is so that the
> clock _can_ be turned off.  Without the possibility to disable
> .leave_on and the logic with accompanies it (i.e.
> clk_disable_critical()) the clock will _never_ be gated.
> 
> > >    */
> > > 
> > > Suspend (with RAM driver's permission):
> > >   /* Order is unimportant */
> > >   SPI disables Clock 4 (ref == 2)
> > >   RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0)
> > >   I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */
> > >   /*
> > >    * Clock will be gated because:
> > >    *   .leave_on is False, so (ref == 0)
> > 
> > Again, .leave_on does nothing new here. We gate the clock because the
> > reference count is 0.
> 
> It's the fact that .leave_on has been disabled in
> clk_disable_critical() that allows the final reference to be taken.
> 
> > >    */
> > > 
> > > Resume:
> > >   /* Order is unimportant */
> > >   SPI enables Clock 4 (ref == 1)
> > >   RAM enables Clock 4 and re-enables .leave_on (ref == 2)
> > >   I2C enables Clock 4 (ref == 3)
> > 
> > Same again. As soon as RAM calls clk_enable_critical the ref count goes
> > up. .leave_on does nothing as far as I can tell. The all works because
> > of the reference counting, which already exists before this patch
> > series.
> 
> So fundamentally you're right in what you say.  All you really need to
> disable a critical clock is write a knowledgeable driver, which is
> intentionally unbalanced i.e. just calls clk_disable().  All this

OK, the line above is helpful. What you really want is a formalized
hand-off mechanism, whereby the clock is enabled at registration-time
and it cannot be turned off until the right driver claims it and decides
turning it off is OK (with a priori knowledge that the clock is already
enabled).

Note that I don't think this implementation can really work in the near
future. Today we do not catch unbalanced calls to clk_enable and
clk_disable, but I have a patch that catches this and WARNs loudly in my
private tree. More on that in the next stanza.

What I don't understand is if there is ever a case for a clock consumer
driver to ever call clk_enable_critical... I do not think there is. What
you're trying to protect against is having the clock disabled BEFORE
that "knowledgeable driver" has a chance to enable it.

Let me know if I've got that right. The only user of this function in
your series is the clk-conf.c stuff, which matches my summary above.

> extended API really does is makes the process more official and
> ensures that an unintentionally unbalanced driver doesn't bugger up
> the running platform.  We could also add a new WARN() to say that said
> driver is unbalanced, as it just tried to turn off a critical clock.

As I mentioned up above I am working on this right now. Our per-user
struct clk stuff makes it trivial to track prepare_count and
enable_count values on a per-user basis. Consequently a naive approach
that simply calls clk_disable an extra time will not work once this code
is merged. This is because the struct clk used in clk-conf.c and in your
knowledgeable driver will be two distinct instances.

Regards,
Mike

> 
> What do you think is best?
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org ? Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Michael Turquette]

Quoting Lee Jones (2015-07-30 02:21:39)
> On Wed, 29 Jul 2015, Michael Turquette wrote:
> > Quoting Lee Jones (2015-07-27 01:53:38)
> > > On Mon, 27 Jul 2015, Maxime Ripard wrote:
> > > 
> > > > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > > > even if they are marked as critical.
> > > > > 
> > > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/linux/clk-provider.h |  2 ++
> > > > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > > > >  3 files changed, 77 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > index 61c3fc5..486b1da 100644
> > > > > --- a/drivers/clk/clk.c
> > > > > +++ b/drivers/clk/clk.c
> > > > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > > > >  
> > > > >  /***    private data structures    ***/
> > > > >  
> > > > > +/**
> > > > > + * struct critical -       Provides 'play' over critical clocks.  A clock can be
> > > > > + *                 marked as critical, meaning that it should not be
> > > > > + *                 disabled.  However, if a driver which is aware of the
> > > > > + *                 critical behaviour wants to control it, it can do so
> > > > > + *                 using clk_enable_critical() and clk_disable_critical().
> > > > > + *
> > > > > + * @enabled        Is clock critical?  Once set, doesn't change
> > > > > + * @leave_on       Self explanatory.  Can be disabled by knowledgeable drivers
> > > > > + */
> > > > > +struct critical {
> > > > > +   bool enabled;
> > > > > +   bool leave_on;
> > > > > +};
> > > > > +
> > > > >  struct clk_core {
> > > > >     const char              *name;
> > > > >     const struct clk_ops    *ops;
> > > > > @@ -75,6 +90,7 @@ struct clk_core {
> > > > >     struct dentry           *dentry;
> > > > >  #endif
> > > > >     struct kref             ref;
> > > > > +   struct critical         critical;
> > > > >  };
> > > > >  
> > > > >  struct clk {
> > > > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > > > >     if (WARN_ON(clk->enable_count == 0))
> > > > >             return;
> > > > >  
> > > > > +   /* Refuse to turn off a critical clock */
> > > > > +   if (clk->enable_count == 1 && clk->critical.leave_on)
> > > > > +           return;
> > > > > +
> > > > 
> > > > I think it should be handled by a separate counting. Otherwise, if you
> > > > have two users that marked the clock as critical, and then one of them
> > > > disable it...
> > > > 
> > > > >     if (--clk->enable_count > 0)
> > > > >             return;
> > > > >  
> > > > > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(clk_disable);
> > > > >  
> > > > > +void clk_disable_critical(struct clk *clk)
> > > > > +{
> > > > > +   clk->core->critical.leave_on = false;
> > > > 
> > > > .. you just lost the fact that it was critical in the first place.
> > > 
> > > I thought about both of these points, which is why I came up with this
> > > strategy.
> > > 
> > > Any device which uses the *_critical() API should a) have knowledge of
> > > what happens when a particular critical clock is gated and b) have
> > > thought about the consequences.
> > 
> > If this statement above is true then I fail to see the need for a new
> > api. A driver which has a really great idea of when it is safe or unsafe
> > to gate a clock should call clk_prepare_enable at probe and then only
> > call clk_disable_unprepare once it is safe to do so.
> > 
> > The existing bookkeeping in the clock framework will do the rest.
> 
> I think you are viewing this particular API back-to-front.  The idea
> is to mark all of the critical clocks at start-up by taking a
> reference.  Then, if there are no knowledgable drivers who wish to
> turn the clock off, the CCF will leave the clock ungated becuase the
> reference count will always be >0.

Right. So I'll ask the same question here that I asked in the other
patch: is there ever a case where a clock consumer driver would want to
call clk_enable_critical? It seems to me that in you usage of it, that
call would only ever be made by the core framework code (e.g. clk-conf.c
or perhaps somewhere in drivers/clk/clk.c).

> 
> The clk_{disable,enable}_critical() calls are to be used by
> knowledgable drivers to say "[disable] I know what I'm doing and it's
> okay for this clock to be turned off" and "[enable] right I'm done
> with this clock now, let's turn it back on and mark it back as
> critical, so no one else can turn it off".

OK, so this almost answers my question above. You have a driver that may
finish using a clock for a while (ie, rmmod knowledgeable_driver), and
you want it (critically) enabled again. Is this a real use case? Who
would come along and disable this clock later on? If the driver is to be
re-loaded then I would suggest never unloading it in the first place.

(Oh and bear in mind when answering my question above that imbalanced
clk_enable/clk_disable calls will not succeed thanks to the vaporware
patch that I have in my local tree)

If you have a second knowledgeable_driver_2 that shares that clock and
can be trusted to manage it (critically) then I would need to see that
example, as it doesn't feel like a real use to me.

> 
> To put things simply, the knowledgable driver will _not_ be enabling
> the clock in the first place.  The first interaction it has with it is
> to gate it.  Then, once it's done, it will enable it again and mark it
> back up as critical.a

I like the first part. Makes sense and fills a real need. I am fully
on-board with a provider-handoff-to-consumer solution. It is all the
weird stuff that comes after it that I'm unsure of.

Regards,
Mike

> 
> Still confused ... let's go on another Q in one of your other emails
> for another way of putting it.
> 
> > > I don't think we can use reference
> > > counting, because we'd need as many critical clock owners as there are
> > > critical clocks.  Cast your mind back to the reasons for this critical
> > > clock API.  One of the most important intentions of this API is the
> > > requirement mitigation for each of the critical clocks to have an owner
> > > (driver).
> > > 
> > > With regards to your second point, that's what 'critical.enabled'
> > > is for.  Take a look at clk_enable_critical().
> > > 
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org ? Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Michael Turquette]

Quoting Lee Jones (2015-07-30 04:17:47)
> On Wed, 29 Jul 2015, Michael Turquette wrote:
> 
> > Hi Lee,
> > 
> > + linux-clk ml
> > 
> > Quoting Lee Jones (2015-07-22 06:04:13)
> > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > even if they are marked as critical.
> > > 
> > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/clk-provider.h |  2 ++
> > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > >  3 files changed, 77 insertions(+)
> > > 
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 61c3fc5..486b1da 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > >  
> > >  /***    private data structures    ***/
> > >  
> > > +/**
> > > + * struct critical -   Provides 'play' over critical clocks.  A clock can be
> > > + *                     marked as critical, meaning that it should not be
> > > + *                     disabled.  However, if a driver which is aware of the
> > > + *                     critical behaviour wants to control it, it can do so
> > > + *                     using clk_enable_critical() and clk_disable_critical().
> > > + *
> > > + * @enabled    Is clock critical?  Once set, doesn't change
> > > + * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers
> > 
> > Not self explanatory. I need this explained to me. What does leave_on
> > do? Better yet, what would happen if leave_on did not exist?
> > 
> > > + */
> > > +struct critical {
> > > +       bool enabled;
> > > +       bool leave_on;
> > > +};
> > > +
> > >  struct clk_core {
> > >         const char              *name;
> > >         const struct clk_ops    *ops;
> > > @@ -75,6 +90,7 @@ struct clk_core {
> > >         struct dentry           *dentry;
> > >  #endif
> > >         struct kref             ref;
> > > +       struct critical         critical;
> > >  };
> > >  
> > >  struct clk {
> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > >         if (WARN_ON(clk->enable_count == 0))
> > >                 return;
> > >  
> > > +       /* Refuse to turn off a critical clock */
> > > +       if (clk->enable_count == 1 && clk->critical.leave_on)
> > > +               return;
> > 
> > How do we get to this point? clk_enable_critical actually calls
> > clk_enable, thus incrementing the enable_count. The only time that we
> > could hit the above case is if,
> > 
> > a) there is an imbalance in clk_enable and clk_disable calls. If this is
> > the case then the drivers need to be fixed. Or better yet some
> > infrastructure to catch that, now that we have per-user struct clk
> > cookies.
> > 
> > b) a driver knowingly calls clk_enable_critical(foo) and then regular,
> > old clk_disable(foo). But why would a driver do that?
> > 
> > It might be that I am missing the point here, so please feel free to
> > clue me in.
> 
> This check behaves in a very similar to the WARN() above.  It's more
> of a fail-safe.  If all drivers are behaving properly, then it
> shouldn't ever be true.  If they're not, it prevents an incorrectly
> written driver from irrecoverably crippling the system.

Then this check should be replaced with a generic approach that refuses
to honor imbalances anyways. Below are two patches that probably resolve
the issue of badly behaving drivers that cause enable imbalances.

> 
> As I said in the other mail.  We can do without these 3 new wrappers.
> We _could_ just write a driver which only calls clk_enable() _after_
> it calls clk_disable(), a kind of intentional unbalance and it would
> do that same thing.

This naive approach will not work with per-user imbalance tracking.

> However, what we're trying to do here is provide
> a proper API, so we can see at first glance what the 'knowledgeable'
> driver is trying to do and not have someone attempt to submit a 'fix'
> which calls clk_enable() or something.

We'll need some type of api for sure for the handoff.

Regards,
Mike



>From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001
From: Michael Turquette <mturquette@baylibre.com>
Date: Wed, 29 Jul 2015 18:22:45 -0700
Subject: [PATCH 1/2] 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

>From ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001
From: Michael Turquette <mturquette@baylibre.com>
Date: Thu, 30 Jul 2015 12:52:26 -0700
Subject: [PATCH 2/2] clk: clk_put WARNs if user has not disabled clk

>From the clk_put kerneldoc in include/linux/clk.h:

"""
Note: drivers must ensure that all clk_enable calls made on this clock
source are balanced by clk_disable calls prior to calling this function.
"""

The common clock framework implementation of the clk.h api has per-user
reference counts for calls to clk_prepare and clk_disable. As such it
can enforce the requirement to properly call clk_disable and
clk_unprepare before calling clk_put.

Because this requirement is probably violated in many places, this patch
starts with a simple warning. Once offending code has been fixed this
check could additionally release the reference counts automatically.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 72feee9..6ec0f77 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk)
 	    clk->max_rate < clk->core->req_rate)
 		clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
 
+	/*
+	 * before calling clk_put, all calls to clk_prepare and clk_enable from
+	 * a given user must be balanced with calls to clk_disable and
+	 * clk_unprepare by that same user
+	 */
+	WARN_ON(clk->prepare_count);
+	WARN_ON(clk->enable_count);
+
 	owner = clk->core->owner;
 	kref_put(&clk->core->ref, __clk_release);
 
-- 
1.9.1

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Maxime Ripard]

On Tue, Jul 28, 2015 at 02:00:55PM +0100, Lee Jones wrote:
> > > I don't think we can use reference counting, because we'd need as
> > > many critical clock owners as there are critical clocks.
> > 
> > Which we can have if we replace the call to clk_prepare_enable you add
> > in your fourth patch in __set_critical_clocks.
> 
> What should it be replaced with?

clk->critical_count++
clk_prepare_enable

?

> 
> > > Cast your mind back to the reasons for this critical clock API.  One
> > > of the most important intentions of this API is the requirement
> > > mitigation for each of the critical clocks to have an owner
> > > (driver).
> > > 
> > > With regards to your second point, that's what 'critical.enabled'
> > > is for.  Take a look at clk_enable_critical().
> > 
> > I don't think this addresses the issue, if you just throw more
> > customers at it, the issue remain with your implementation.
> > 
> > If you have three customers that used the critical API, and if on of
> > these calls clk_disable_critical, you're losing leave_on.
> 
> That's the idea.  See my point above, the one you replied "Indeed"
> to.  So when a driver uses clk_disable_critical() it's saying, "I know
> why this clock is a critical clock, and I know that nothing terrible
> will happen if I disable it, as I have that covered".

We do agree on the semantic of clk_disable_critical :)

> So then if it's not the last user to call clk_disable(), the last
> one out the door will be allowed to finally gate the clock,
> regardless whether it's critical aware or not.

That's right, but what I mean would be a case where you have two users
that are aware that it is a critical clock (A and B), and one which is
not (C).

If I understood correctly your code, if A calls clk_disable_critical,
leave_on is set to false. That means that now, if C calls clk_disable
on that clock, it will actually be shut down, while B still considers
it a critical clock.

> Then, when we come to enable the clock again, the critical aware user
> then re-marks the clock as leave_on, so not critical un-aware user can
> take the final reference and disable the clock.
> 
> > Which means that if there's one of the two users left that calls
> > clk_disable on it, the clock will actually be disabled, which is
> > clearly not what we want to do, as we have still a user that want the
> > clock to be enabled.
> 
> That's not what happens (at least it shouldn't if I've coded it up
> right).  The API _still_ requires all of the users to give-up their
> reference.

Ah, right. So I guess it all boils down to the discussion you're
having with Mike regarding whether critical users should expect to
already have a reference taken or calling clk_prepare / clk_enable
themselves.

> 
> > It would be much more robust to have another count for the critical
> > stuff, initialised to one by the __set_critical_clocks function.
> 
> If I understand you correctly, we already have a count.  We use the
> original reference count.  No need for one of our own.
> 
> Using your RAM Clock (Clock 4) as an example
> --------------------------------------------
> 
> Early start-up:
>   Clock 4 is marked as critical and a reference is taken (ref == 1)
> 
> Driver probe:
>   SPI enables Clock 4 (ref == 2)
>   I2C enables Clock 4 (ref == 3)
> 
> Suspend (without RAM driver's permission):
>   SPI disables Clock 4 (ref == 2)
>   I2C disables Clock 4 (ref == 1)
>   /*
>    * Clock won't be gated because:
>    *   .leave_on is True - can't dec final reference
>    */
> 
> Suspend (with RAM driver's permission):
>   /* Order is unimportant */
>   SPI disables Clock 4 (ref == 2)
>   RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0)
>   I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */
>   /*
>    * Clock will be gated because:
>    *   .leave_on is False, so (ref == 0)
>    */
> 
> Resume:
>   /* Order is unimportant */
>   SPI enables Clock 4 (ref == 1)
>   RAM enables Clock 4 and re-enables .leave_on (ref == 2)
>   I2C enables Clock 4 (ref == 3)
> 
> Hopefully that clears things up.

It does indeed. I simply forgot to take into account the fact that it
would still need the reference to be set to 0. My bad.

Still, If we take the same kind of scenario:

Early start-up:
  Clock 4 is marked as critical and a reference is taken (ref == 1, leave_on = true)

Driver probe:
  SPI enables Clock 4 (ref == 2)
  I2C enables Clock 4 (ref == 3)
  RAM enables Clock 4 (ref == 4, leave_on = true )
  CPUIdle enables Clock 4 (ref == 5, leave_on = true )

Suspend (with CPUIdle and RAM permissions):
  /* Order is unimportant */
  SPI disables Clock 4 (ref == 4)
  CPUIdle disables Clock 4 (ref == 3, leave_on = false ) 
  RAM disables Clock 4 (ref == 2, leave_on = false )
  I2C disables Clock 4 (ref == 1)

And even though the clock will still be running when CPUIdle calls
clk_disable_critical because of the enable_count, the status of
leave_on is off, as the RAM driver still considers it to be left on
(ie, hasn't called clk_disable_critical yet).

Or at least, that's what I understood of it.

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/20150731/780c6dca/attachment.sig>

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Maxime Ripard]

On Thu, Jul 30, 2015 at 03:47:20PM -0700, Michael Turquette wrote:
> > > >    */
> > > > 
> > > > Resume:
> > > >   /* Order is unimportant */
> > > >   SPI enables Clock 4 (ref == 1)
> > > >   RAM enables Clock 4 and re-enables .leave_on (ref == 2)
> > > >   I2C enables Clock 4 (ref == 3)
> > > 
> > > Same again. As soon as RAM calls clk_enable_critical the ref count goes
> > > up. .leave_on does nothing as far as I can tell. The all works because
> > > of the reference counting, which already exists before this patch
> > > series.
> > 
> > So fundamentally you're right in what you say.  All you really need to
> > disable a critical clock is write a knowledgeable driver, which is
> > intentionally unbalanced i.e. just calls clk_disable().  All this
> 
> OK, the line above is helpful. What you really want is a formalized
> hand-off mechanism, whereby the clock is enabled at registration-time
> and it cannot be turned off until the right driver claims it and decides
> turning it off is OK (with a priori knowledge that the clock is already
> enabled).

There's two things that should be covered, and are related, yet can be
done in two steps:

  - Have a way to, no matter what (which configuration we have, if we
    have multiple users or not that might reparent or disable their
    clocks, etc.), make sure that a clock will always be running by
    default. This is done through the call in clk-conf, and we
    identify such clocks using critical-clocks in the DT.

  - Now, even though that information is true, some driver who are
    aware of that fact might want to disable those critical
    clocks. This is what the clk_disable_critical and
    clk_enable_critical functions are here for.

> Note that I don't think this implementation can really work in the near
> future. Today we do not catch unbalanced calls to clk_enable and
> clk_disable, but I have a patch that catches this and WARNs loudly in my
> private tree. More on that in the next stanza.
> 
> What I don't understand is if there is ever a case for a clock consumer
> driver to ever call clk_enable_critical... I do not think there is. What
> you're trying to protect against is having the clock disabled BEFORE
> that "knowledgeable driver" has a chance to enable it.

It's really about what we want the API to look like in the second
case.

Do we want such drivers to still call clk_prepare_enable? Some other
function? Should they assume that the clock has already been enabled,
or do we want a handover, or a force disable, or whatever.

I guess we should discuss those questions, before starting to think
about how to implement it.

IMHO, I think that the existing way of doing should be used, or at
least with the same mindset to avoid confusion, errors, and
misinformed reviews.

So I'd expect the drivers to do something like:

probe:
  clk_get
  clk_critical_enable

remove / probe error path:
  clk_critical_disable
  clk_put

and use the clk_critical_enable and clk_critical_disable whenever
needed, and thing would just work as intended.

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/20150731/9e5c7098/attachment.sig>

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones]

On Fri, 31 Jul 2015, Maxime Ripard wrote:

> On Thu, Jul 30, 2015 at 03:47:20PM -0700, Michael Turquette wrote:
> > > > >    */
> > > > > 
> > > > > Resume:
> > > > >   /* Order is unimportant */
> > > > >   SPI enables Clock 4 (ref == 1)
> > > > >   RAM enables Clock 4 and re-enables .leave_on (ref == 2)
> > > > >   I2C enables Clock 4 (ref == 3)
> > > > 
> > > > Same again. As soon as RAM calls clk_enable_critical the ref count goes
> > > > up. .leave_on does nothing as far as I can tell. The all works because
> > > > of the reference counting, which already exists before this patch
> > > > series.
> > > 
> > > So fundamentally you're right in what you say.  All you really need to
> > > disable a critical clock is write a knowledgeable driver, which is
> > > intentionally unbalanced i.e. just calls clk_disable().  All this
> > 
> > OK, the line above is helpful. What you really want is a formalized
> > hand-off mechanism, whereby the clock is enabled at registration-time
> > and it cannot be turned off until the right driver claims it and decides
> > turning it off is OK (with a priori knowledge that the clock is already
> > enabled).
> 
> There's two things that should be covered, and are related, yet can be
> done in two steps:
> 
>   - Have a way to, no matter what (which configuration we have, if we
>     have multiple users or not that might reparent or disable their
>     clocks, etc.), make sure that a clock will always be running by
>     default. This is done through the call in clk-conf, and we
>     identify such clocks using critical-clocks in the DT.
> 
>   - Now, even though that information is true, some driver who are
>     aware of that fact might want to disable those critical
>     clocks. This is what the clk_disable_critical and
>     clk_enable_critical functions are here for.

+1

> > Note that I don't think this implementation can really work in the near
> > future. Today we do not catch unbalanced calls to clk_enable and
> > clk_disable, but I have a patch that catches this and WARNs loudly in my
> > private tree. More on that in the next stanza.
> > 
> > What I don't understand is if there is ever a case for a clock consumer
> > driver to ever call clk_enable_critical... I do not think there is. What
> > you're trying to protect against is having the clock disabled BEFORE
> > that "knowledgeable driver" has a chance to enable it.

In my mind and in this implementation  clk_disable_critical() will be
used _first_ by the knowledgeable driver, then when the knowledgeable
driver has finished whatever it was doing (shutting down banks of RAM
etc...), it should then call clk_enable_critical() to reset the clock
state back to the way it found it i.e. enabled and marked as critical.

> It's really about what we want the API to look like in the second
> case.
> 
> Do we want such drivers to still call clk_prepare_enable? Some other
> function? Should they assume that the clock has already been enabled,
> or do we want a handover, or a force disable, or whatever.
> 
> I guess we should discuss those questions, before starting to think
> about how to implement it.
> 
> IMHO, I think that the existing way of doing should be used, or at
> least with the same mindset to avoid confusion, errors, and
> misinformed reviews.
> 
> So I'd expect the drivers to do something like:
> 
> probe:
>   clk_get
>   clk_critical_enable

Well becuase the clock has been marked as critical, a reference has
already been taken, so even if there are 0 users the clock now has 2
references attributed to it.

> remove / probe error path:
>   clk_critical_disable
>   clk_put

I think we should assume that the clock is already running in the
knowledgeable driver:

start-up:
  __set_critical_clocks()

probe:
  clk_get()

suspend (or whatever reason the driver wishes to disable the clk):
  clk_disable_critical()

resume (or whatever ...):
  clk_enable_critical()

remove:
  clk_put() /* Or just rely on devm_*() */ 

> and use the clk_critical_enable and clk_critical_disable whenever
> needed, and thing would just work as intended.

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

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones]

On Fri, 31 Jul 2015, Maxime Ripard wrote:

> On Tue, Jul 28, 2015 at 02:00:55PM +0100, Lee Jones wrote:
> > > > I don't think we can use reference counting, because we'd need as
> > > > many critical clock owners as there are critical clocks.
> > > 
> > > Which we can have if we replace the call to clk_prepare_enable you add
> > > in your fourth patch in __set_critical_clocks.
> > 
> > What should it be replaced with?
> 
> clk->critical_count++
> clk_prepare_enable
> 
> ?

Ah, so not replace it then.  Just add a reference counter.

I'm with you, that's fine.

> > > > Cast your mind back to the reasons for this critical clock API.  One
> > > > of the most important intentions of this API is the requirement
> > > > mitigation for each of the critical clocks to have an owner
> > > > (driver).
> > > > 
> > > > With regards to your second point, that's what 'critical.enabled'
> > > > is for.  Take a look at clk_enable_critical().
> > > 
> > > I don't think this addresses the issue, if you just throw more
> > > customers at it, the issue remain with your implementation.
> > > 
> > > If you have three customers that used the critical API, and if on of
> > > these calls clk_disable_critical, you're losing leave_on.
> > 
> > That's the idea.  See my point above, the one you replied "Indeed"
> > to.  So when a driver uses clk_disable_critical() it's saying, "I know
> > why this clock is a critical clock, and I know that nothing terrible
> > will happen if I disable it, as I have that covered".
> 
> We do agree on the semantic of clk_disable_critical :)
> 
> > So then if it's not the last user to call clk_disable(), the last
> > one out the door will be allowed to finally gate the clock,
> > regardless whether it's critical aware or not.
> 
> That's right, but what I mean would be a case where you have two users
> that are aware that it is a critical clock (A and B), and one which is
> not (C).
> 
> If I understood correctly your code, if A calls clk_disable_critical,
> leave_on is set to false. That means that now, if C calls clk_disable
> on that clock, it will actually be shut down, while B still considers
> it a critical clock.

Hmm... I'd have to think about this.

How would you mark a clock as critical twice?

> > Then, when we come to enable the clock again, the critical aware user
> > then re-marks the clock as leave_on, so not critical un-aware user can
> > take the final reference and disable the clock.
> > 
> > > Which means that if there's one of the two users left that calls
> > > clk_disable on it, the clock will actually be disabled, which is
> > > clearly not what we want to do, as we have still a user that want the
> > > clock to be enabled.
> > 
> > That's not what happens (at least it shouldn't if I've coded it up
> > right).  The API _still_ requires all of the users to give-up their
> > reference.
> 
> Ah, right. So I guess it all boils down to the discussion you're
> having with Mike regarding whether critical users should expect to
> already have a reference taken or calling clk_prepare / clk_enable
> themselves.

Then we'd need a clk_prepare_enable_critical() :)

... which would be aware of the original reference (taken by
__set_critical_clocks).  However, if a second knowledgeable driver
were to call it, then how would it know that whether the original
reference was still present or not?  I guess that's where your
critical clock reference comes in.  If it's the first critical user,
it would decrement the original reference, it it's a subsequent user,
then it won't

> > > It would be much more robust to have another count for the critical
> > > stuff, initialised to one by the __set_critical_clocks function.
> > 
> > If I understand you correctly, we already have a count.  We use the
> > original reference count.  No need for one of our own.
> > 
> > Using your RAM Clock (Clock 4) as an example
> > --------------------------------------------
> > 
> > Early start-up:
> >   Clock 4 is marked as critical and a reference is taken (ref == 1)
> > 
> > Driver probe:
> >   SPI enables Clock 4 (ref == 2)
> >   I2C enables Clock 4 (ref == 3)
> > 
> > Suspend (without RAM driver's permission):
> >   SPI disables Clock 4 (ref == 2)
> >   I2C disables Clock 4 (ref == 1)
> >   /*
> >    * Clock won't be gated because:
> >    *   .leave_on is True - can't dec final reference
> >    */
> > 
> > Suspend (with RAM driver's permission):
> >   /* Order is unimportant */
> >   SPI disables Clock 4 (ref == 2)
> >   RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0)
> >   I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */
> >   /*
> >    * Clock will be gated because:
> >    *   .leave_on is False, so (ref == 0)
> >    */
> > 
> > Resume:
> >   /* Order is unimportant */
> >   SPI enables Clock 4 (ref == 1)
> >   RAM enables Clock 4 and re-enables .leave_on (ref == 2)
> >   I2C enables Clock 4 (ref == 3)
> > 
> > Hopefully that clears things up.
> 
> It does indeed. I simply forgot to take into account the fact that it
> would still need the reference to be set to 0. My bad.
> 
> Still, If we take the same kind of scenario:
> 
> Early start-up:
>   Clock 4 is marked as critical and a reference is taken (ref == 1, leave_on = true)
> 
> Driver probe:
>   SPI enables Clock 4 (ref == 2)
>   I2C enables Clock 4 (ref == 3)
>   RAM enables Clock 4 (ref == 4, leave_on = true )
>   CPUIdle enables Clock 4 (ref == 5, leave_on = true )
> 
> Suspend (with CPUIdle and RAM permissions):
>   /* Order is unimportant */
>   SPI disables Clock 4 (ref == 4)
>   CPUIdle disables Clock 4 (ref == 3, leave_on = false ) 
>   RAM disables Clock 4 (ref == 2, leave_on = false )
>   I2C disables Clock 4 (ref == 1)
> 
> And even though the clock will still be running when CPUIdle calls
> clk_disable_critical because of the enable_count, the status of
> leave_on is off, as the RAM driver still considers it to be left on
> (ie, hasn't called clk_disable_critical yet).
> 
> Or at least, that's what I understood of it.

Right, I understood this problem when you suggested that two critical
clock users could be using the same clock.  Other than having them
call clock_prepare_enable[_critical](), I'm not sure if that's
possible.

As I mentioned above, we can handle this with reference counting and
I'm happy to code that up.

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

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones]

On Thu, 30 Jul 2015, Michael Turquette wrote:

> Quoting Lee Jones (2015-07-30 02:21:39)
> > On Wed, 29 Jul 2015, Michael Turquette wrote:
> > > Quoting Lee Jones (2015-07-27 01:53:38)
> > > > On Mon, 27 Jul 2015, Maxime Ripard wrote:
> > > > 
> > > > > On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> > > > > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > > > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > > > > even if they are marked as critical.
> > > > > > 
> > > > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > ---
> > > > > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/linux/clk-provider.h |  2 ++
> > > > > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > > > > >  3 files changed, 77 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > > index 61c3fc5..486b1da 100644
> > > > > > --- a/drivers/clk/clk.c
> > > > > > +++ b/drivers/clk/clk.c
> > > > > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > > > > >  
> > > > > >  /***    private data structures    ***/
> > > > > >  
> > > > > > +/**
> > > > > > + * struct critical -       Provides 'play' over critical clocks.  A clock can be
> > > > > > + *                 marked as critical, meaning that it should not be
> > > > > > + *                 disabled.  However, if a driver which is aware of the
> > > > > > + *                 critical behaviour wants to control it, it can do so
> > > > > > + *                 using clk_enable_critical() and clk_disable_critical().
> > > > > > + *
> > > > > > + * @enabled        Is clock critical?  Once set, doesn't change
> > > > > > + * @leave_on       Self explanatory.  Can be disabled by knowledgeable drivers
> > > > > > + */
> > > > > > +struct critical {
> > > > > > +   bool enabled;
> > > > > > +   bool leave_on;
> > > > > > +};
> > > > > > +
> > > > > >  struct clk_core {
> > > > > >     const char              *name;
> > > > > >     const struct clk_ops    *ops;
> > > > > > @@ -75,6 +90,7 @@ struct clk_core {
> > > > > >     struct dentry           *dentry;
> > > > > >  #endif
> > > > > >     struct kref             ref;
> > > > > > +   struct critical         critical;
> > > > > >  };
> > > > > >  
> > > > > >  struct clk {
> > > > > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > > > > >     if (WARN_ON(clk->enable_count == 0))
> > > > > >             return;
> > > > > >  
> > > > > > +   /* Refuse to turn off a critical clock */
> > > > > > +   if (clk->enable_count == 1 && clk->critical.leave_on)
> > > > > > +           return;
> > > > > > +
> > > > > 
> > > > > I think it should be handled by a separate counting. Otherwise, if you
> > > > > have two users that marked the clock as critical, and then one of them
> > > > > disable it...
> > > > > 
> > > > > >     if (--clk->enable_count > 0)
> > > > > >             return;
> > > > > >  
> > > > > > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(clk_disable);
> > > > > >  
> > > > > > +void clk_disable_critical(struct clk *clk)
> > > > > > +{
> > > > > > +   clk->core->critical.leave_on = false;
> > > > > 
> > > > > .. you just lost the fact that it was critical in the first place.
> > > > 
> > > > I thought about both of these points, which is why I came up with this
> > > > strategy.
> > > > 
> > > > Any device which uses the *_critical() API should a) have knowledge of
> > > > what happens when a particular critical clock is gated and b) have
> > > > thought about the consequences.
> > > 
> > > If this statement above is true then I fail to see the need for a new
> > > api. A driver which has a really great idea of when it is safe or unsafe
> > > to gate a clock should call clk_prepare_enable at probe and then only
> > > call clk_disable_unprepare once it is safe to do so.
> > > 
> > > The existing bookkeeping in the clock framework will do the rest.
> > 
> > I think you are viewing this particular API back-to-front.  The idea
> > is to mark all of the critical clocks at start-up by taking a
> > reference.  Then, if there are no knowledgable drivers who wish to
> > turn the clock off, the CCF will leave the clock ungated becuase the
> > reference count will always be >0.
> 
> Right. So I'll ask the same question here that I asked in the other
> patch: is there ever a case where a clock consumer driver would want to
> call clk_enable_critical? It seems to me that in you usage of it, that
> call would only ever be made by the core framework code (e.g. clk-conf.c
> or perhaps somewhere in drivers/clk/clk.c).

Yes, _after_ it has called clk_disable_critical(), when it has
finished fiddling with it.  clk_enable_critical() simply resets the
clock back to an enabled/critical state (how the knowledgeable driver
found it).

> > The clk_{disable,enable}_critical() calls are to be used by
> > knowledgable drivers to say "[disable] I know what I'm doing and it's
> > okay for this clock to be turned off" and "[enable] right I'm done
> > with this clock now, let's turn it back on and mark it back as
> > critical, so no one else can turn it off".
> 
> OK, so this almost answers my question above. You have a driver that may
> finish using a clock for a while (ie, rmmod knowledgeable_driver), and
> you want it (critically) enabled again. Is this a real use case? Who
> would come along and disable this clock later on? If the driver is to be
> re-loaded then I would suggest never unloading it in the first place.

This has nothing to do with modules.  I believe the knowledgeable
consumer should only gate the clock (steal a reference) when the clock
is actually gated.  The rest of the time the framework will have it
marked as critical "do not turn me off".

> (Oh and bear in mind when answering my question above that imbalanced
> clk_enable/clk_disable calls will not succeed thanks to the vaporware
> patch that I have in my local tree)

They won't be imbalanced, because clk_enable() would have been called
first during start-up (__set_critical_clocks()).

> If you have a second knowledgeable_driver_2 that shares that clock and
> can be trusted to manage it (critically) then I would need to see that
> example, as it doesn't feel like a real use to me.

Nor me, that's why this impementation doesn't handle that use-case,
however Maxime thinks it is one, so we can solve that with reference
counting.

> > To put things simply, the knowledgable driver will _not_ be enabling
> > the clock in the first place.  The first interaction it has with it is
> > to gate it.  Then, once it's done, it will enable it again and mark it
> > back up as critical.a
> 
> I like the first part. Makes sense and fills a real need. I am fully
> on-board with a provider-handoff-to-consumer solution. It is all the
> weird stuff that comes after it that I'm unsure of.

I don't think there should be hand-off.  I think the
{enable,disable}_critical() should "momentarily" take the last
reference, then put everything back as it found it when it's finished
disabling the clock.

> > Still confused ... let's go on another Q in one of your other emails
> > for another way of putting it.
> > 
> > > > I don't think we can use reference
> > > > counting, because we'd need as many critical clock owners as there are
> > > > critical clocks.  Cast your mind back to the reasons for this critical
> > > > clock API.  One of the most important intentions of this API is the
> > > > requirement mitigation for each of the critical clocks to have an owner
> > > > (driver).
> > > > 
> > > > With regards to your second point, that's what 'critical.enabled'
> > > > is for.  Take a look at clk_enable_critical().
> > > > 
> > 

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

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones]

On Thu, 30 Jul 2015, Michael Turquette wrote:

> Quoting Lee Jones (2015-07-30 04:17:47)
> > On Wed, 29 Jul 2015, Michael Turquette wrote:
> > 
> > > Hi Lee,
> > > 
> > > + linux-clk ml
> > > 
> > > Quoting Lee Jones (2015-07-22 06:04:13)
> > > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > > even if they are marked as critical.
> > > > 
> > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/clk-provider.h |  2 ++
> > > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > > >  3 files changed, 77 insertions(+)
> > > > 
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index 61c3fc5..486b1da 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > > >  
> > > >  /***    private data structures    ***/
> > > >  
> > > > +/**
> > > > + * struct critical -   Provides 'play' over critical clocks.  A clock can be
> > > > + *                     marked as critical, meaning that it should not be
> > > > + *                     disabled.  However, if a driver which is aware of the
> > > > + *                     critical behaviour wants to control it, it can do so
> > > > + *                     using clk_enable_critical() and clk_disable_critical().
> > > > + *
> > > > + * @enabled    Is clock critical?  Once set, doesn't change
> > > > + * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers
> > > 
> > > Not self explanatory. I need this explained to me. What does leave_on
> > > do? Better yet, what would happen if leave_on did not exist?
> > > 
> > > > + */
> > > > +struct critical {
> > > > +       bool enabled;
> > > > +       bool leave_on;
> > > > +};
> > > > +
> > > >  struct clk_core {
> > > >         const char              *name;
> > > >         const struct clk_ops    *ops;
> > > > @@ -75,6 +90,7 @@ struct clk_core {
> > > >         struct dentry           *dentry;
> > > >  #endif
> > > >         struct kref             ref;
> > > > +       struct critical         critical;
> > > >  };
> > > >  
> > > >  struct clk {
> > > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > > >         if (WARN_ON(clk->enable_count == 0))
> > > >                 return;
> > > >  
> > > > +       /* Refuse to turn off a critical clock */
> > > > +       if (clk->enable_count == 1 && clk->critical.leave_on)
> > > > +               return;
> > > 
> > > How do we get to this point? clk_enable_critical actually calls
> > > clk_enable, thus incrementing the enable_count. The only time that we
> > > could hit the above case is if,
> > > 
> > > a) there is an imbalance in clk_enable and clk_disable calls. If this is
> > > the case then the drivers need to be fixed. Or better yet some
> > > infrastructure to catch that, now that we have per-user struct clk
> > > cookies.
> > > 
> > > b) a driver knowingly calls clk_enable_critical(foo) and then regular,
> > > old clk_disable(foo). But why would a driver do that?
> > > 
> > > It might be that I am missing the point here, so please feel free to
> > > clue me in.
> > 
> > This check behaves in a very similar to the WARN() above.  It's more
> > of a fail-safe.  If all drivers are behaving properly, then it
> > shouldn't ever be true.  If they're not, it prevents an incorrectly
> > written driver from irrecoverably crippling the system.
> 
> Then this check should be replaced with a generic approach that refuses
> to honor imbalances anyways. Below are two patches that probably resolve
> the issue of badly behaving drivers that cause enable imbalances.

Your patch should make the requirement for this check moot, so it can
probably be removed.

> > As I said in the other mail.  We can do without these 3 new wrappers.
> > We _could_ just write a driver which only calls clk_enable() _after_
> > it calls clk_disable(), a kind of intentional unbalance and it would
> > do that same thing.
> 
> This naive approach will not work with per-user imbalance tracking.

Steady on.  I said we "_could_", that that I think it's a good idea.

I think it's a bad idea, which is why I wrote this set. ;)

> > However, what we're trying to do here is provide
> > a proper API, so we can see at first glance what the 'knowledgeable'
> > driver is trying to do and not have someone attempt to submit a 'fix'
> > which calls clk_enable() or something.
> 
> We'll need some type of api for sure for the handoff.

This set will not trigger your new checks.  The clocks will be in
perfect ballance becuase a reference will be taken at start-up.

Again:

start-up:
  clk_prepare_enable()

knowlegable_driver_probe:
  clk_get()

knowlegable_driver_gate_clk:
  clk_disable_critical()

knowlegable_driver_ungate_clk:
  clk_enable_critical()

knowlegable_driver_remove:
  clk_put()

> From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001
> From: Michael Turquette <mturquette@baylibre.com>
> Date: Wed, 29 Jul 2015 18:22:45 -0700
> Subject: [PATCH 1/2] 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);
>  

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

[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Michael Turquette]

Quoting Lee Jones (2015-07-31 02:02:19)
> On Thu, 30 Jul 2015, Michael Turquette wrote:
> 
> > Quoting Lee Jones (2015-07-30 04:17:47)
> > > On Wed, 29 Jul 2015, Michael Turquette wrote:
> > > 
> > > > Hi Lee,
> > > > 
> > > > + linux-clk ml
> > > > 
> > > > Quoting Lee Jones (2015-07-22 06:04:13)
> > > > > These new API calls will firstly provide a mechanisms to tag a clock as
> > > > > critical and secondly allow any knowledgeable driver to (un)gate clocks,
> > > > > even if they are marked as critical.
> > > > > 
> > > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/linux/clk-provider.h |  2 ++
> > > > >  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
> > > > >  3 files changed, 77 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > index 61c3fc5..486b1da 100644
> > > > > --- a/drivers/clk/clk.c
> > > > > +++ b/drivers/clk/clk.c
> > > > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
> > > > >  
> > > > >  /***    private data structures    ***/
> > > > >  
> > > > > +/**
> > > > > + * struct critical -   Provides 'play' over critical clocks.  A clock can be
> > > > > + *                     marked as critical, meaning that it should not be
> > > > > + *                     disabled.  However, if a driver which is aware of the
> > > > > + *                     critical behaviour wants to control it, it can do so
> > > > > + *                     using clk_enable_critical() and clk_disable_critical().
> > > > > + *
> > > > > + * @enabled    Is clock critical?  Once set, doesn't change
> > > > > + * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers
> > > > 
> > > > Not self explanatory. I need this explained to me. What does leave_on
> > > > do? Better yet, what would happen if leave_on did not exist?
> > > > 
> > > > > + */
> > > > > +struct critical {
> > > > > +       bool enabled;
> > > > > +       bool leave_on;
> > > > > +};
> > > > > +
> > > > >  struct clk_core {
> > > > >         const char              *name;
> > > > >         const struct clk_ops    *ops;
> > > > > @@ -75,6 +90,7 @@ struct clk_core {
> > > > >         struct dentry           *dentry;
> > > > >  #endif
> > > > >         struct kref             ref;
> > > > > +       struct critical         critical;
> > > > >  };
> > > > >  
> > > > >  struct clk {
> > > > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
> > > > >         if (WARN_ON(clk->enable_count == 0))
> > > > >                 return;
> > > > >  
> > > > > +       /* Refuse to turn off a critical clock */
> > > > > +       if (clk->enable_count == 1 && clk->critical.leave_on)
> > > > > +               return;
> > > > 
> > > > How do we get to this point? clk_enable_critical actually calls
> > > > clk_enable, thus incrementing the enable_count. The only time that we
> > > > could hit the above case is if,
> > > > 
> > > > a) there is an imbalance in clk_enable and clk_disable calls. If this is
> > > > the case then the drivers need to be fixed. Or better yet some
> > > > infrastructure to catch that, now that we have per-user struct clk
> > > > cookies.
> > > > 
> > > > b) a driver knowingly calls clk_enable_critical(foo) and then regular,
> > > > old clk_disable(foo). But why would a driver do that?
> > > > 
> > > > It might be that I am missing the point here, so please feel free to
> > > > clue me in.
> > > 
> > > This check behaves in a very similar to the WARN() above.  It's more
> > > of a fail-safe.  If all drivers are behaving properly, then it
> > > shouldn't ever be true.  If they're not, it prevents an incorrectly
> > > written driver from irrecoverably crippling the system.
> > 
> > Then this check should be replaced with a generic approach that refuses
> > to honor imbalances anyways. Below are two patches that probably resolve
> > the issue of badly behaving drivers that cause enable imbalances.
> 
> Your patch should make the requirement for this check moot, so it can
> probably be removed.
> 
> > > As I said in the other mail.  We can do without these 3 new wrappers.
> > > We _could_ just write a driver which only calls clk_enable() _after_
> > > it calls clk_disable(), a kind of intentional unbalance and it would
> > > do that same thing.
> > 
> > This naive approach will not work with per-user imbalance tracking.
> 
> Steady on.  I said we "_could_", that that I think it's a good idea.
> 
> I think it's a bad idea, which is why I wrote this set. ;)
> 
> > > However, what we're trying to do here is provide
> > > a proper API, so we can see at first glance what the 'knowledgeable'
> > > driver is trying to do and not have someone attempt to submit a 'fix'
> > > which calls clk_enable() or something.
> > 
> > We'll need some type of api for sure for the handoff.
> 
> This set will not trigger your new checks.  The clocks will be in
> perfect ballance becuase a reference will be taken at start-up.
> 
> Again:
> 
> start-up:
>   clk_prepare_enable()
> 
> knowlegable_driver_probe:
>   clk_get()
> 
> knowlegable_driver_gate_clk:
>   clk_disable_critical()

The call to clk_disable() nested inside clk_disable_critical will fail
with the new checks. This is because the struct clk instance will be
different from one used in your "start-up" section above. clk_get()
creates a unique struct clk every time you call it.

Put another way, a unique user of a clock cannot call clk_disable() when
the per-user enable_count is 0.

Furthermore, there is no way that I will ever be happy with a technique
that requires calling disable prior to an enable within a driver. That
goes against a long-standing api designs and is confusing as hell to
driver authors.

Regards,
Mike

> 
> knowlegable_driver_ungate_clk:
>   clk_enable_critical()
> 
> knowlegable_driver_remove:
>   clk_put()
> 
> > From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001
> > From: Michael Turquette <mturquette@baylibre.com>
> > Date: Wed, 29 Jul 2015 18:22:45 -0700
> > Subject: [PATCH 1/2] 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);
> >  
> 
> -- 
> 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-clk" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 4/5] clk: Provide critical clock support

[Barry Song]

2015-07-22 21:04 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> Lots of platforms contain clocks which if turned off would prove fatal.
> The only way to recover from these catastrophic failures is to restart
> the board(s).  Now, when a clock provider is registered with the
> framework it is possible for a list of critical clocks to be supplied
> which must be kept ungated.  Each clock mentioned in the newly
> introduced 'critical-clock' property will be clk_prepare_enable()d
> where the normal references will be taken.  This will prevent the
> common clk framework from attempting to gate them during the normal
> clk_disable_unused() and disable_clock() procedures.
>
> Note that it will still be possible for knowledgeable drivers to turn
> these clocks off using clk_{enable,disable}_critical() calls.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

hi Lee,
i have another email about this. i am wondering whether
CLK_IGNORE_UNUSE is still useful after your patch. another solution
for your patch might be extending the current CLK_IGNORE_UNUSE to
runtime?


copy the mail here:
currently we can set a CLK_IGNORE_UNUSE flag to  a clock to stop
clk_disable_unused()  from disabling it at the boot stage:

static void clk_disable_unused_subtree(struct clk_core *core)
{
...

if (core->flags & CLK_IGNORE_UNUSED)
goto unlock_out;
}

static int clk_disable_unused(void)
{
...

clk_unprepare_unused_subtree(core);
...
 return 0;
}

late_initcall_sync(clk_disable_unused);

so if there are two clocks A and B, A is the parent of B, and A is
marked as CLK_IGNORE_UNUSED.

in boot stage if there is nobody using A and B, Linux will disable B
due to clk_disable_unused() , but keep A being enabled since A has
CLK_IGNORE_UNUSED.

but in Linux runtime, we might frequently disable /enable B in runtime
power management, this will cause A disabled since Linux will not
check CLK_IGNORE_UNUSED for runtime disabling clk .

 so this makes CLK_IGNORE_UNUSE only work if we don't disable its
sub-clock at runtime. this looks making no sense.

 i am thinking whether we should do some changes to make it have side
affect for runtime clk disable. otherwise, why do we want to stop it
from being disabled during boot stage?

 I am not sure whether i missed something in clk core level support.

-barry

> ---
>  drivers/clk/clk-conf.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> index aad4796..f83c1c2 100644
> --- a/drivers/clk/clk-conf.c
> +++ b/drivers/clk/clk-conf.c
> @@ -116,6 +116,45 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
>         return 0;
>  }
>
> +static int __set_critical_clocks(struct device_node *node, bool clk_supplier)
> +{
> +       struct of_phandle_args clkspec;
> +       struct clk *clk;
> +       struct property *prop;
> +       const __be32 *cur;
> +       uint32_t index;
> +       int ret;
> +
> +       if (!clk_supplier)
> +               return 0;
> +
> +       of_property_for_each_u32(node, "critical-clock", prop, cur, index) {
> +               clkspec.np = node;
> +               clkspec.args_count = 1;
> +               clkspec.args[0] = index;
> +
> +               clk = of_clk_get_from_provider(&clkspec);
> +               if (IS_ERR(clk)) {
> +                       pr_err("clk: couldn't get clock %u for %s\n",
> +                               index, node->full_name);
> +                       return PTR_ERR(clk);
> +               }
> +
> +               clk_init_critical(clk);
> +
> +               ret = clk_prepare_enable(clk);
> +               if (ret) {
> +                       pr_err("Failed to enable clock %u for %s: %d\n",
> +                              index, node->full_name, ret);
> +                       return ret;
> +               }
> +
> +               pr_debug("Setting clock as critical %pC\n", clk);
> +       }
> +
> +       return 0;
> +}
> +
>  /**
>   * of_clk_set_defaults() - parse and set assigned clocks configuration
>   * @node: device node to apply clock settings for
> @@ -139,6 +178,10 @@ int of_clk_set_defaults(struct device_node *node, bool clk_supplier)
>         if (rc < 0)
>                 return rc;
>
> -       return __set_clk_rates(node, clk_supplier);
> +       rc = __set_clk_rates(node, clk_supplier);
> +       if (rc < 0)
> +               return rc;
> +
> +       return __set_critical_clocks(node, clk_supplier);
>  }
>  EXPORT_SYMBOL_GPL(of_clk_set_defaults);
> --
> 1.9.1

[PATCH v7 4/5] clk: Provide critical clock support

[Lee Jones]

On Mon, 17 Aug 2015, Barry Song wrote:

> 2015-07-22 21:04 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> > Lots of platforms contain clocks which if turned off would prove fatal.
> > The only way to recover from these catastrophic failures is to restart
> > the board(s).  Now, when a clock provider is registered with the
> > framework it is possible for a list of critical clocks to be supplied
> > which must be kept ungated.  Each clock mentioned in the newly
> > introduced 'critical-clock' property will be clk_prepare_enable()d
> > where the normal references will be taken.  This will prevent the
> > common clk framework from attempting to gate them during the normal
> > clk_disable_unused() and disable_clock() procedures.
> >
> > Note that it will still be possible for knowledgeable drivers to turn
> > these clocks off using clk_{enable,disable}_critical() calls.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> hi Lee,
> i have another email about this. i am wondering whether
> CLK_IGNORE_UNUSE is still useful after your patch. another solution
> for your patch might be extending the current CLK_IGNORE_UNUSE to
> runtime?
> 
> 
> copy the mail here:
> currently we can set a CLK_IGNORE_UNUSE flag to  a clock to stop
> clk_disable_unused()  from disabling it at the boot stage:
> 
> static void clk_disable_unused_subtree(struct clk_core *core)
> {
> ...
> 
> if (core->flags & CLK_IGNORE_UNUSED)
> goto unlock_out;
> }
> 
> static int clk_disable_unused(void)
> {
> ...
> 
> clk_unprepare_unused_subtree(core);
> ...
>  return 0;
> }
> 
> late_initcall_sync(clk_disable_unused);
> 
> so if there are two clocks A and B, A is the parent of B, and A is
> marked as CLK_IGNORE_UNUSED.
> 
> in boot stage if there is nobody using A and B, Linux will disable B
> due to clk_disable_unused() , but keep A being enabled since A has
> CLK_IGNORE_UNUSED.
> 
> but in Linux runtime, we might frequently disable /enable B in runtime
> power management, this will cause A disabled since Linux will not
> check CLK_IGNORE_UNUSED for runtime disabling clk .
> 
>  so this makes CLK_IGNORE_UNUSE only work if we don't disable its
> sub-clock at runtime. this looks making no sense.
> 
>  i am thinking whether we should do some changes to make it have side
> affect for runtime clk disable. otherwise, why do we want to stop it
> from being disabled during boot stage?

This is one of this problems, along with some others that this set
aims to solve.

Extending CLK_IGNORE_UNUSED is not a good idea.  In fact, if we can
phase it out completely, that will be a good thing.

Since this set Mike has submitted an alternitive solution.

Please see: https://groups.google.com/forum/#!msg/linux.kernel/kX_nWSsWRxU/IZSjhG5Ed4oJ

>  I am not sure whether i missed something in clk core level support.
> 
> -barry
> 
> > ---
> >  drivers/clk/clk-conf.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> > index aad4796..f83c1c2 100644
> > --- a/drivers/clk/clk-conf.c
> > +++ b/drivers/clk/clk-conf.c
> > @@ -116,6 +116,45 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
> >         return 0;
> >  }
> >
> > +static int __set_critical_clocks(struct device_node *node, bool clk_supplier)
> > +{
> > +       struct of_phandle_args clkspec;
> > +       struct clk *clk;
> > +       struct property *prop;
> > +       const __be32 *cur;
> > +       uint32_t index;
> > +       int ret;
> > +
> > +       if (!clk_supplier)
> > +               return 0;
> > +
> > +       of_property_for_each_u32(node, "critical-clock", prop, cur, index) {
> > +               clkspec.np = node;
> > +               clkspec.args_count = 1;
> > +               clkspec.args[0] = index;
> > +
> > +               clk = of_clk_get_from_provider(&clkspec);
> > +               if (IS_ERR(clk)) {
> > +                       pr_err("clk: couldn't get clock %u for %s\n",
> > +                               index, node->full_name);
> > +                       return PTR_ERR(clk);
> > +               }
> > +
> > +               clk_init_critical(clk);
> > +
> > +               ret = clk_prepare_enable(clk);
> > +               if (ret) {
> > +                       pr_err("Failed to enable clock %u for %s: %d\n",
> > +                              index, node->full_name, ret);
> > +                       return ret;
> > +               }
> > +
> > +               pr_debug("Setting clock as critical %pC\n", clk);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  /**
> >   * of_clk_set_defaults() - parse and set assigned clocks configuration
> >   * @node: device node to apply clock settings for
> > @@ -139,6 +178,10 @@ int of_clk_set_defaults(struct device_node *node, bool clk_supplier)
> >         if (rc < 0)
> >                 return rc;
> >
> > -       return __set_clk_rates(node, clk_supplier);
> > +       rc = __set_clk_rates(node, clk_supplier);
> > +       if (rc < 0)
> > +               return rc;
> > +
> > +       return __set_critical_clocks(node, clk_supplier);
> >  }
> >  EXPORT_SYMBOL_GPL(of_clk_set_defaults);

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

[PATCH v7 4/5] clk: Provide critical clock support

[Barry Song]

2015-08-17 15:42 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> On Mon, 17 Aug 2015, Barry Song wrote:
>
>> 2015-07-22 21:04 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
>> > Lots of platforms contain clocks which if turned off would prove fatal.
>> > The only way to recover from these catastrophic failures is to restart
>> > the board(s).  Now, when a clock provider is registered with the
>> > framework it is possible for a list of critical clocks to be supplied
>> > which must be kept ungated.  Each clock mentioned in the newly
>> > introduced 'critical-clock' property will be clk_prepare_enable()d
>> > where the normal references will be taken.  This will prevent the
>> > common clk framework from attempting to gate them during the normal
>> > clk_disable_unused() and disable_clock() procedures.
>> >
>> > Note that it will still be possible for knowledgeable drivers to turn
>> > these clocks off using clk_{enable,disable}_critical() calls.
>> >
>> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>
>> hi Lee,
>> i have another email about this. i am wondering whether
>> CLK_IGNORE_UNUSE is still useful after your patch. another solution
>> for your patch might be extending the current CLK_IGNORE_UNUSE to
>> runtime?
>>
>>
>> copy the mail here:
>> currently we can set a CLK_IGNORE_UNUSE flag to  a clock to stop
>> clk_disable_unused()  from disabling it at the boot stage:
>>
>> static void clk_disable_unused_subtree(struct clk_core *core)
>> {
>> ...
>>
>> if (core->flags & CLK_IGNORE_UNUSED)
>> goto unlock_out;
>> }
>>
>> static int clk_disable_unused(void)
>> {
>> ...
>>
>> clk_unprepare_unused_subtree(core);
>> ...
>>  return 0;
>> }
>>
>> late_initcall_sync(clk_disable_unused);
>>
>> so if there are two clocks A and B, A is the parent of B, and A is
>> marked as CLK_IGNORE_UNUSED.
>>
>> in boot stage if there is nobody using A and B, Linux will disable B
>> due to clk_disable_unused() , but keep A being enabled since A has
>> CLK_IGNORE_UNUSED.
>>
>> but in Linux runtime, we might frequently disable /enable B in runtime
>> power management, this will cause A disabled since Linux will not
>> check CLK_IGNORE_UNUSED for runtime disabling clk .
>>
>>  so this makes CLK_IGNORE_UNUSE only work if we don't disable its
>> sub-clock at runtime. this looks making no sense.
>>
>>  i am thinking whether we should do some changes to make it have side
>> affect for runtime clk disable. otherwise, why do we want to stop it
>> from being disabled during boot stage?
>
> This is one of this problems, along with some others that this set
> aims to solve.
>
> Extending CLK_IGNORE_UNUSED is not a good idea.  In fact, if we can
> phase it out completely, that will be a good thing.

i would agree it is better we can drop CLK_IGNORE_UNUSED since it is
confusing...


>
> Since this set Mike has submitted an alternitive solution.
>
> Please see: https://groups.google.com/forum/#!msg/linux.kernel/kX_nWSsWRxU/IZSjhG5Ed4oJ
>
>>  I am not sure whether i missed something in clk core level support.
>>
>> -barry
>>
>> > ---
>> >  drivers/clk/clk-conf.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 44 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
>> > index aad4796..f83c1c2 100644
>> > --- a/drivers/clk/clk-conf.c
>> > +++ b/drivers/clk/clk-conf.c
>> > @@ -116,6 +116,45 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
>> >         return 0;
>> >  }
>> >
>> > +static int __set_critical_clocks(struct device_node *node, bool clk_supplier)
>> > +{
>> > +       struct of_phandle_args clkspec;
>> > +       struct clk *clk;
>> > +       struct property *prop;
>> > +       const __be32 *cur;
>> > +       uint32_t index;
>> > +       int ret;
>> > +
>> > +       if (!clk_supplier)
>> > +               return 0;
>> > +
>> > +       of_property_for_each_u32(node, "critical-clock", prop, cur, index) {
>> > +               clkspec.np = node;
>> > +               clkspec.args_count = 1;
>> > +               clkspec.args[0] = index;
>> > +
>> > +               clk = of_clk_get_from_provider(&clkspec);
>> > +               if (IS_ERR(clk)) {
>> > +                       pr_err("clk: couldn't get clock %u for %s\n",
>> > +                               index, node->full_name);
>> > +                       return PTR_ERR(clk);
>> > +               }
>> > +
>> > +               clk_init_critical(clk);
>> > +
>> > +               ret = clk_prepare_enable(clk);
>> > +               if (ret) {
>> > +                       pr_err("Failed to enable clock %u for %s: %d\n",
>> > +                              index, node->full_name, ret);
>> > +                       return ret;
>> > +               }
>> > +
>> > +               pr_debug("Setting clock as critical %pC\n", clk);
>> > +       }
>> > +
>> > +       return 0;
>> > +}
>> > +
>> >  /**
>> >   * of_clk_set_defaults() - parse and set assigned clocks configuration
>> >   * @node: device node to apply clock settings for
>> > @@ -139,6 +178,10 @@ int of_clk_set_defaults(struct device_node *node, bool clk_supplier)
>> >         if (rc < 0)
>> >                 return rc;
>> >
>> > -       return __set_clk_rates(node, clk_supplier);
>> > +       rc = __set_clk_rates(node, clk_supplier);
>> > +       if (rc < 0)
>> > +               return rc;
>> > +
>> > +       return __set_critical_clocks(node, clk_supplier);
>> >  }
>> >  EXPORT_SYMBOL_GPL(of_clk_set_defaults);
>
> --

-barry