[PATCH v6 0/4] clk: Provide support for always-on clocks

[PATCH v6 0/4] 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 always-on clocks to be supplied
which must be kept ungated.  Each clock mentioned in the newly
introduced 'clock-always-on' 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 clk_disable_unused()
and disable_clock() procedures.

Changelog:
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 (4):
  ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
  ARM: sti: stih410-clocks: Identify critical clocks as always-on
  clk: Provide always-on clock support
  clk: dt: Introduce binding for always-on clock support

 .../devicetree/bindings/clock/clock-bindings.txt   | 38 +++++++++++++++++++
 arch/arm/boot/dts/stih410-clock.dtsi               | 10 +++++
 drivers/clk/clk-conf.c                             | 43 +++++++++++++++++++++-
 include/dt-bindings/clock/stih407-clks.h           |  4 ++
 4 files changed, 94 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH v6 1/4] 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 v6 2/4] ARM: sti: stih410-clocks: Identify critical clocks as always-on

[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..e1dd9b2 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";
+				clock-always-on = <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";
+
+				clock-always-on = <CLK_IC_LMI0>;
 			};
 		};
 
@@ -142,6 +145,7 @@
 				clocks = <&clk_sysin>;
 
 				clock-output-names = "clk-s-c0-pll0-odf-0";
+				clock-always-on = <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";
+
+				clock-always-on = <CLK_ICN_CPU>,
+						  <CLK_TX_ICN_DMU>,
+						  <CLK_EXT2F_A9>,
+						  <CLK_ICN_LMI>,
+						  <CLK_ICN_SBC>;
 			};
 		};
 
-- 
1.9.1

[PATCH v6 3/4] clk: Provide always-on 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 always-on clocks to be supplied
which must be kept ungated.  Each clock mentioned in the newly
introduced 'clock-always-on' 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 clk_disable_unused()
and disable_clock() procedures.

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

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index aad4796..a07aced 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -116,6 +116,43 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 	return 0;
 }
 
+static int __set_clk_always_on(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, "clock-always-on", 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);
+		}
+
+		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 always-on %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 +176,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_clk_always_on(node, clk_supplier);
 }
 EXPORT_SYMBOL_GPL(of_clk_set_defaults);
-- 
1.9.1

Re: [PATCH v6 3/4] clk: Provide always-on clock support

[Stephen Boyd]

On 04/07, Lee Jones wrote:
> 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 always-on clocks to be supplied
> which must be kept ungated.  Each clock mentioned in the newly
> introduced 'clock-always-on' 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 clk_disable_unused()
> and disable_clock() procedures.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/clk/clk-conf.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> index aad4796..a07aced 100644
> --- a/drivers/clk/clk-conf.c
> +++ b/drivers/clk/clk-conf.c
> @@ -116,6 +116,43 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
>  	return 0;
>  }
>  
> +static int __set_clk_always_on(struct device_node *node, bool clk_supplier)
> +{
> +	struct of_phandle_args clkspec;
> +	struct clk *clk;
> +	struct property	*prop;
> +	const __be32 *cur;
> +	uint32_t index;

u32 please

> +	int ret;
> +
> +	if (!clk_supplier)
> +		return 0;
> +
> +	of_property_for_each_u32(node, "clock-always-on", 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);
> +		}
> +
> +		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;
> +		}

I guess we don't have a clk_put() here because we want to keep the
refcount high forever? We should at least put a comment here
explaining why we left out clk_put() if so.

> +
> +		pr_debug("Setting clock as always-on %pC\n", clk);
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * of_clk_set_defaults() - parse and set assigned clocks configuration
>   * @node: device node to apply clock settings for

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Lee Jones]

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

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index 06fc6d5..daf3323 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -44,6 +44,44 @@ For example:
   clocks by index. The names should reflect the clock output signal
   names for the device.
 
+clock-always-on:    Some hardware contains bunches of clocks which 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, the common clk framework will attempt to
+		    disable them and 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 always-on 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 an always-on clock.
+
+For example:
+
+    oscillator {
+        #clock-cells = <1>;
+        clock-output-names = "ckil", "ckih";
+        clock-always-on = <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 always-on clocks, so the framework 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

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2404 bytes --]

Hi Lee,

On Tue, Apr 07, 2015 at 07:43:59PM +0100, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt   | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index 06fc6d5..daf3323 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -44,6 +44,44 @@ For example:
>    clocks by index. The names should reflect the clock output signal
>    names for the device.
>  
> +clock-always-on:    Some hardware contains bunches of clocks which 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, the common clk framework will attempt to
> +		    disable them and 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 always-on using this property
> +		    from inside a clocksource's node.

Isn't "clocksource" here way too similar to, I don't know, the
clocksource framework? Wouln't clock provider be better?

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

Disregarding what's stated here, I'm pretty sure that this will
actually happen. Where do you place the cursor?

Should we create a new driver for our RAM controller, or do we want to
use clock-always-on?

Do we really want to enforce this if we ever gain a driver that would
actually be able to manage its clock (like do we want the CPU clock to
never *ever* be gated just because we don't have a cpuidle/hotplug
driver yet?)

Have you seen the numerous NAK on such approach Mike did?

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Lee Jones]

On Tue, 07 Apr 2015, Maxime Ripard wrote:
> On Tue, Apr 07, 2015 at 07:43:59PM +0100, Lee Jones wrote:
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../devicetree/bindings/clock/clock-bindings.txt   | 38 ++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > index 06fc6d5..daf3323 100644
> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > @@ -44,6 +44,44 @@ For example:
> >    clocks by index. The names should reflect the clock output signal
> >    names for the device.
> >  
> > +clock-always-on:    Some hardware contains bunches of clocks which 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, the common clk framework will attempt to
> > +		    disable them and 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 always-on using this property
> > +		    from inside a clocksource's node.
> 
> Isn't "clocksource" here way too similar to, I don't know, the
> clocksource framework? Wouln't clock provider be better?

Yes, I think you're right.  Stephen also mentioned it.

> > +
> > +		    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.
> 
> Disregarding what's stated here, I'm pretty sure that this will
> actually happen. Where do you place the cursor?

That's up to Mike.

> Should we create a new driver for our RAM controller, or do we want to
> use clock-always-on?

I would say that if all the driver did was to enable clocks, then you
should use this instead.  This binding was designed specifically for
that purpose.

However, if the aforementioned driver clock can be safely gated, then
it should not be an always-on clock.

> Do we really want to enforce this if we ever gain a driver that would
> actually be able to manage its clock (like do we want the CPU clock to
> never *ever* be gated just because we don't have a cpuidle/hotplug
> driver yet?)

As I've just mentioned, if a clock 'can' be turned off, this binding
should never be used.  Situations where using always-on as a stop-gap
due to a lack of current functionality is what the paragraph above is
trying to mitigate.

> Have you seen the numerous NAK on such approach Mike did?

I haven't, but the folks reviewing previous versions have.  Do you
have something specific in mind that you'd like to bring to my
attention?

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

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3208 bytes --]

On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > +
> > > +		    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.
> > 
> > Disregarding what's stated here, I'm pretty sure that this will
> > actually happen. Where do you place the cursor?
> 
> That's up to Mike.

Except that Mike won't review any of the DT changes, so he won't be
able to refrain users from using it. Let alone out-of-tree DTs using a
mainline kernel.

> > Should we create a new driver for our RAM controller, or do we want to
> > use clock-always-on?
> 
> I would say that if all the driver did was to enable clocks, then you
> should use this instead.  This binding was designed specifically for
> that purpose.
> 
> However, if the aforementioned driver clock can be safely gated, then
> it should not be an always-on clock.

Yeah, of course, I understand the original intent of it, but that
argument, which might very well be true at one point in time, might
not be true anymore two or three releases later.

And that driver might actually rely on the fact that the clock is shut
down, which won't be the case.

Introducing a DT interface solely by refering to the current state of
a driver is a bit odd.

> > Do we really want to enforce this if we ever gain a driver that would
> > actually be able to manage its clock (like do we want the CPU clock to
> > never *ever* be gated just because we don't have a cpuidle/hotplug
> > driver yet?)
> 
> As I've just mentioned, if a clock 'can' be turned off, this binding
> should never be used. Situations where using always-on as a stop-gap
> due to a lack of current functionality is what the paragraph above is
> trying to mitigate.

But it's not really what this property is about. What this property
describes is that these clocks should never be gated. Any point in
time during the life of the system AND with in any kernel version.

> > Have you seen the numerous NAK on such approach Mike did?
> 
> I haven't, but the folks reviewing previous versions have.  Do you
> have something specific in mind that you'd like to bring to my
> attention?

Unfortunately, I haven't been able to dig out such mails. But it's why
we ended up with clock protection code in various clock drivers
including:

AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504

Which is much more flexible, since you won't have to modify the DT to
change which clocks are to be left enabled, as well as way easier to
debug if you ever have to remove that property from the DT.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Lee Jones]

On Wed, 08 Apr 2015, Maxime Ripard wrote:

> On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > +
> > > > +		    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.
> > > 
> > > Disregarding what's stated here, I'm pretty sure that this will
> > > actually happen. Where do you place the cursor?
> > 
> > That's up to Mike.
> 
> Except that Mike won't review any of the DT changes, so he won't be
> able to refrain users from using it. Let alone out-of-tree DTs using a
> mainline kernel.

Ideally Mike should be Cc'ed on patches using clock bindings, but if
he isn't the DT guys are smart enough to either make the right
decisions themselves (Rob has Acked these bindings already, so will be
on the lookout for misuse, I'm sure), or ask for Mike's help.

> > > Should we create a new driver for our RAM controller, or do we want to
> > > use clock-always-on?
> > 
> > I would say that if all the driver did was to enable clocks, then you
> > should use this instead.  This binding was designed specifically for
> > that purpose.
> > 
> > However, if the aforementioned driver clock can be safely gated, then
> > it should not be an always-on clock.
> 
> Yeah, of course, I understand the original intent of it, but that
> argument, which might very well be true at one point in time, might
> not be true anymore two or three releases later.

Why?  The H/W isn't going to change in two or three releases.  The
clocks designated as 'always-on' will have to be on forever, or
synonymously, 'always'.

> And that driver might actually rely on the fact that the clock is shut
> down, which won't be the case.

I think you are missing the point of this binding.  The driver can
never rely on that in this use-case.  If the clock is off, there is no
device driver, period. 

> Introducing a DT interface solely by refering to the current state of
> a driver is a bit odd.

I'm not sure I get your point.  This binding has nothing to do with
drivers.

> > > Do we really want to enforce this if we ever gain a driver that would
> > > actually be able to manage its clock (like do we want the CPU clock to
> > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > driver yet?)
> > 
> > As I've just mentioned, if a clock 'can' be turned off, this binding
> > should never be used. Situations where using always-on as a stop-gap
> > due to a lack of current functionality is what the paragraph above is
> > trying to mitigate.
> 
> But it's not really what this property is about. What this property
> describes is that these clocks should never be gated. Any point in
> time during the life of the system AND with in any kernel version.

You got it, that's correct -- these clocks should never be gated.

So why would that ever change?  If that is likely (or even possible)
to change in the future then this binding should not be used.

To reiterate; this binding should be used on ungatable clocks only.
Non-negotiable, non-changeable either by the introduction of new
functionality/support or kernel version.

> > > Have you seen the numerous NAK on such approach Mike did?
> > 
> > I haven't, but the folks reviewing previous versions have.  Do you
> > have something specific in mind that you'd like to bring to my
> > attention?
> 
> Unfortunately, I haven't been able to dig out such mails. But it's why
> we ended up with clock protection code in various clock drivers
> including:
> 
> AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> 
> Which is much more flexible, since you won't have to modify the DT to
> change which clocks are to be left enabled, as well as way easier to
> debug if you ever have to remove that property from the DT.

You're right, you don't have the change the DT in these cases.  You
have to write new C code, which is _less_ flexible.

So all these platforms are adding their own hand-rolled version of
this binding, adding more duplication and cruft to the kernel.
Instead they can use this 'always-on' and we can consolidate and strip
it all out.

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

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6549 bytes --]

On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> On Wed, 08 Apr 2015, Maxime Ripard wrote:
> 
> > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > +
> > > > > +		    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.
> > > > 
> > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > actually happen. Where do you place the cursor?
> > > 
> > > That's up to Mike.
> > 
> > Except that Mike won't review any of the DT changes, so he won't be
> > able to refrain users from using it. Let alone out-of-tree DTs using a
> > mainline kernel.
> 
> Ideally Mike should be Cc'ed on patches using clock bindings, but if
> he isn't the DT guys are smart enough to either make the right
> decisions themselves (Rob has Acked these bindings already, so will be
> on the lookout for misuse, I'm sure), or ask for Mike's help.

Yeah, right, as if this strategy really worked in the past....

Do we really want to look at even the DT bindings that have actually
been reviewed by maintainers that got merged?

They don't have time for that, which is totally fine, but we really
should bury our head in the sand by actually thinking they will review
every single DT-related patch.

Using that as an argument is just plain denial of what really happened
for the past 4 years.

> > > > Should we create a new driver for our RAM controller, or do we want to
> > > > use clock-always-on?
> > > 
> > > I would say that if all the driver did was to enable clocks, then you
> > > should use this instead.  This binding was designed specifically for
> > > that purpose.
> > > 
> > > However, if the aforementioned driver clock can be safely gated, then
> > > it should not be an always-on clock.
> > 
> > Yeah, of course, I understand the original intent of it, but that
> > argument, which might very well be true at one point in time, might
> > not be true anymore two or three releases later.
> 
> Why?  The H/W isn't going to change in two or three releases.  The
> clocks designated as 'always-on' will have to be on forever, or
> synonymously, 'always'.
>
> > And that driver might actually rely on the fact that the clock is shut
> > down, which won't be the case.
> 
> I think you are missing the point of this binding.  The driver can
> never rely on that in this use-case.  If the clock is off, there is no
> device driver, period. 

Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
PM guys will be happy to hear that.

And they are not device drivers, are not mandatory in the system, and
it's usually a good thing to keep the CPU running whenever you don't
have such drivers.

> > Introducing a DT interface solely by refering to the current state of
> > a driver is a bit odd.
> 
> I'm not sure I get your point.  This binding has nothing to do with
> drivers.

It's all about drivers. Or rather all about missing drivers.

> > > > Do we really want to enforce this if we ever gain a driver that would
> > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > driver yet?)
> > > 
> > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > should never be used. Situations where using always-on as a stop-gap
> > > due to a lack of current functionality is what the paragraph above is
> > > trying to mitigate.
> > 
> > But it's not really what this property is about. What this property
> > describes is that these clocks should never be gated. Any point in
> > time during the life of the system AND with in any kernel version.
> 
> You got it, that's correct -- these clocks should never be gated.
> 
> So why would that ever change?  If that is likely (or even possible)
> to change in the future then this binding should not be used.
>
> To reiterate; this binding should be used on ungatable clocks only.
> Non-negotiable, non-changeable either by the introduction of new
> functionality/support or kernel version.

I'm pretty sure that if that patch gets merged, by the end of the
year, there will be "incorrect" users by your standards.

If you introduce a feature, you should expect people to use
it.  If not, what's the point?

> > > > Have you seen the numerous NAK on such approach Mike did?
> > > 
> > > I haven't, but the folks reviewing previous versions have.  Do you
> > > have something specific in mind that you'd like to bring to my
> > > attention?
> > 
> > Unfortunately, I haven't been able to dig out such mails. But it's why
> > we ended up with clock protection code in various clock drivers
> > including:
> > 
> > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > 
> > Which is much more flexible, since you won't have to modify the DT to
> > change which clocks are to be left enabled, as well as way easier to
> > debug if you ever have to remove that property from the DT.
> 
> You're right, you don't have the change the DT in these cases.  You
> have to write new C code, which is _less_ flexible.

I'm sorry to learn that you never heard of that stable-DT thing.

And a bit sorry to see that a maintainer is really seeing C as not
flexible.

> So all these platforms are adding their own hand-rolled version of
> this binding, adding more duplication and cruft to the kernel.
> Instead they can use this 'always-on' and we can consolidate and strip
> it all out.

Except that all these platforms are actually not implementing a
binding, ie not an interface with the DT they are bound to. Each and
every of these platforms can change that list whenever they wish, just
by sending a single one-liner patch (just like the DT, really.).

Which is not something that you can achieve with a DT binding.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Lee Jones]

On Wed, 08 Apr 2015, Maxime Ripard wrote:

> On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > 
> > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > > +
> > > > > > +		    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.
> > > > > 
> > > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > > actually happen. Where do you place the cursor?
> > > > 
> > > > That's up to Mike.
> > > 
> > > Except that Mike won't review any of the DT changes, so he won't be
> > > able to refrain users from using it. Let alone out-of-tree DTs using a
> > > mainline kernel.
> > 
> > Ideally Mike should be Cc'ed on patches using clock bindings, but if
> > he isn't the DT guys are smart enough to either make the right
> > decisions themselves (Rob has Acked these bindings already, so will be
> > on the lookout for misuse, I'm sure), or ask for Mike's help.
> 
> Yeah, right, as if this strategy really worked in the past....
> 
> Do we really want to look at even the DT bindings that have actually
> been reviewed by maintainers that got merged?
> 
> They don't have time for that, which is totally fine, but we really
> should bury our head in the sand by actually thinking they will review
> every single DT-related patch.
> 
> Using that as an argument is just plain denial of what really happened
> for the past 4 years.

I agree that it's a problem, but this is a process problem and has
nothing to do with this set.  If you have a problem with the current
process and have a better alternative, submit your thoughts to the DT
list.  Rejecting all new bindings because you are frightened that they
will be used in a manner that they were not intended is not the way to
go though.

> > > > > Should we create a new driver for our RAM controller, or do we want to
> > > > > use clock-always-on?
> > > > 
> > > > I would say that if all the driver did was to enable clocks, then you
> > > > should use this instead.  This binding was designed specifically for
> > > > that purpose.
> > > > 
> > > > However, if the aforementioned driver clock can be safely gated, then
> > > > it should not be an always-on clock.
> > > 
> > > Yeah, of course, I understand the original intent of it, but that
> > > argument, which might very well be true at one point in time, might
> > > not be true anymore two or three releases later.
> > 
> > Why?  The H/W isn't going to change in two or three releases.  The
> > clocks designated as 'always-on' will have to be on forever, or
> > synonymously, 'always'.
> >
> > > And that driver might actually rely on the fact that the clock is shut
> > > down, which won't be the case.
> > 
> > I think you are missing the point of this binding.  The driver can
> > never rely on that in this use-case.  If the clock is off, there is no
> > device driver, period. 
> 
> Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
> PM guys will be happy to hear that.
> 
> And they are not device drivers, are not mandatory in the system, and
> it's usually a good thing to keep the CPU running whenever you don't
> have such drivers.
> 
> > > Introducing a DT interface solely by refering to the current state of
> > > a driver is a bit odd.
> > 
> > I'm not sure I get your point.  This binding has nothing to do with
> > drivers.
> 
> It's all about drivers. Or rather all about missing drivers.

I think you are going to have to be more forthcoming with your issues
with this binding, because I'm struggling to understand what your
problem with it is.  You have already pointed me to vendors which have
a genuine/valid need for it.  But instead you'd prefer they hand-roll
their own implementations over multiple lines of C code (each).

IMHO, that's madness.

> > > > > Do we really want to enforce this if we ever gain a driver that would
> > > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > > driver yet?)
> > > > 
> > > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > > should never be used. Situations where using always-on as a stop-gap
> > > > due to a lack of current functionality is what the paragraph above is
> > > > trying to mitigate.
> > > 
> > > But it's not really what this property is about. What this property
> > > describes is that these clocks should never be gated. Any point in
> > > time during the life of the system AND with in any kernel version.
> > 
> > You got it, that's correct -- these clocks should never be gated.
> > 
> > So why would that ever change?  If that is likely (or even possible)
> > to change in the future then this binding should not be used.
> >
> > To reiterate; this binding should be used on ungatable clocks only.
> > Non-negotiable, non-changeable either by the introduction of new
> > functionality/support or kernel version.
> 
> I'm pretty sure that if that patch gets merged, by the end of the
> year, there will be "incorrect" users by your standards.

It's possible to abuse any binding.  I don't see why you are so
offended of this one in particular.

> If you introduce a feature, you should expect people to use
> it.  If not, what's the point?

By your own admission, there are genuine users for this binding and I
expect people to use it.

> > > > > Have you seen the numerous NAK on such approach Mike did?
> > > > 
> > > > I haven't, but the folks reviewing previous versions have.  Do you
> > > > have something specific in mind that you'd like to bring to my
> > > > attention?
> > > 
> > > Unfortunately, I haven't been able to dig out such mails. But it's why
> > > we ended up with clock protection code in various clock drivers
> > > including:
> > > 
> > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > > 
> > > Which is much more flexible, since you won't have to modify the DT to
> > > change which clocks are to be left enabled, as well as way easier to
> > > debug if you ever have to remove that property from the DT.
> > 
> > You're right, you don't have the change the DT in these cases.  You
> > have to write new C code, which is _less_ flexible.
> 
> I'm sorry to learn that you never heard of that stable-DT thing.
> 
> And a bit sorry to see that a maintainer is really seeing C as not
> flexible.

You're putting words in my mouth.  I didn't say C was not flexible.

I'm referencing the original DT pros i.e. it is possible to supply a
different configuration without the need to compile the kernel.
That's certainly true in this case.  We can provide a clk-provider and
tag it as always-on, all without re-compile.

> > So all these platforms are adding their own hand-rolled version of
> > this binding, adding more duplication and cruft to the kernel.
> > Instead they can use this 'always-on' and we can consolidate and strip
> > it all out.
> 
> Except that all these platforms are actually not implementing a
> binding, ie not an interface with the DT they are bound to. Each and
> every of these platforms can change that list whenever they wish, just
> by sending a single one-liner patch (just like the DT, really.).
> 
> Which is not something that you can achieve with a DT binding.

Once again, can you give me more information about why you have such a
problem with this binding.  I wish for it to be stable/ABI, I wish for
it never to be removed, I envisage it will always be needed, so what's
the problem? 

Do you have a vested interest that I am missing?

Perhaps an example of possible calamity will help convince me that
you're not completely wrong and blowing everything out of proportion
for no good reason.

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

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 11227 bytes --]

On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote:
> On Wed, 08 Apr 2015, Maxime Ripard wrote:
> 
> > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > 
> > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > > > +
> > > > > > > +		    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.
> > > > > > 
> > > > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > > > actually happen. Where do you place the cursor?
> > > > > 
> > > > > That's up to Mike.
> > > > 
> > > > Except that Mike won't review any of the DT changes, so he won't be
> > > > able to refrain users from using it. Let alone out-of-tree DTs using a
> > > > mainline kernel.
> > > 
> > > Ideally Mike should be Cc'ed on patches using clock bindings, but if
> > > he isn't the DT guys are smart enough to either make the right
> > > decisions themselves (Rob has Acked these bindings already, so will be
> > > on the lookout for misuse, I'm sure), or ask for Mike's help.
> > 
> > Yeah, right, as if this strategy really worked in the past....
> > 
> > Do we really want to look at even the DT bindings that have actually
> > been reviewed by maintainers that got merged?
> > 
> > They don't have time for that, which is totally fine, but we really
> > should bury our head in the sand by actually thinking they will review
> > every single DT-related patch.
> > 
> > Using that as an argument is just plain denial of what really happened
> > for the past 4 years.
> 
> I agree that it's a problem, but this is a process problem and has
> nothing to do with this set.  If you have a problem with the current
> process and have a better alternative, submit your thoughts to the DT
> list.  Rejecting all new bindings because you are frightened that they
> will be used in a manner that they were not intended is not the way to
> go though.

I'm not saying that this binding should not go in because of a process
issue.

I'm saying that discarding arguments against your binding by adding
restrictions that cannot be enforced is not reasonable.

> > > > > > Should we create a new driver for our RAM controller, or do we want to
> > > > > > use clock-always-on?
> > > > > 
> > > > > I would say that if all the driver did was to enable clocks, then you
> > > > > should use this instead.  This binding was designed specifically for
> > > > > that purpose.
> > > > > 
> > > > > However, if the aforementioned driver clock can be safely gated, then
> > > > > it should not be an always-on clock.
> > > > 
> > > > Yeah, of course, I understand the original intent of it, but that
> > > > argument, which might very well be true at one point in time, might
> > > > not be true anymore two or three releases later.
> > > 
> > > Why?  The H/W isn't going to change in two or three releases.  The
> > > clocks designated as 'always-on' will have to be on forever, or
> > > synonymously, 'always'.
> > >
> > > > And that driver might actually rely on the fact that the clock is shut
> > > > down, which won't be the case.
> > > 
> > > I think you are missing the point of this binding.  The driver can
> > > never rely on that in this use-case.  If the clock is off, there is no
> > > device driver, period. 
> > 
> > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
> > PM guys will be happy to hear that.
> > 
> > And they are not device drivers, are not mandatory in the system, and
> > it's usually a good thing to keep the CPU running whenever you don't
> > have such drivers.
> > 
> > > > Introducing a DT interface solely by refering to the current state of
> > > > a driver is a bit odd.
> > > 
> > > I'm not sure I get your point.  This binding has nothing to do with
> > > drivers.
> > 
> > It's all about drivers. Or rather all about missing drivers.
> 
> I think you are going to have to be more forthcoming with your issues
> with this binding, because I'm struggling to understand what your
> problem with it is.  You have already pointed me to vendors which have
> a genuine/valid need for it.  But instead you'd prefer they hand-roll
> their own implementations over multiple lines of C code (each).

I told you already.

If you have that property, there's absolutely no way to do any kind of
clock management in the future.

It might be fine for your use case, but see my point about the
unreasonable restriction. People are going to use it for clocks that
just don't have a driver *yet*, and when that driver will be merged we
will end up with a driver that (for example) makes the assumption that
the clock has been shut down to reset the IP, that might or might not
be the case, depending on how old the DT is exactly.

This will be even a bigger madness if you ask me.

> > > > > > Do we really want to enforce this if we ever gain a driver that would
> > > > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > > > driver yet?)
> > > > > 
> > > > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > > > should never be used. Situations where using always-on as a stop-gap
> > > > > due to a lack of current functionality is what the paragraph above is
> > > > > trying to mitigate.
> > > > 
> > > > But it's not really what this property is about. What this property
> > > > describes is that these clocks should never be gated. Any point in
> > > > time during the life of the system AND with in any kernel version.
> > > 
> > > You got it, that's correct -- these clocks should never be gated.
> > > 
> > > So why would that ever change?  If that is likely (or even possible)
> > > to change in the future then this binding should not be used.
> > >
> > > To reiterate; this binding should be used on ungatable clocks only.
> > > Non-negotiable, non-changeable either by the introduction of new
> > > functionality/support or kernel version.
> > 
> > I'm pretty sure that if that patch gets merged, by the end of the
> > year, there will be "incorrect" users by your standards.
> 
> It's possible to abuse any binding.  I don't see why you are so
> offended of this one in particular.

I'm not offended, I just tried to push the same kind of patches two
years ago, with Mike pushing back, and actually came to see that he
was right a few monthes down the road.

And yeah, your point that any binding can be abused is true. This one
is only so easy to abuse it's not even funny.

> > If you introduce a feature, you should expect people to use
> > it.  If not, what's the point?
> 
> By your own admission, there are genuine users for this binding and I
> expect people to use it.

The only thing that we really disagree upon is that whether that
restriction will really be followed. You expect people to, I
don't. It's the fundamental disagreement we have, that really prevent
any purely technical discussion.

Maybe we can try to address that before moving forward?

> > > > > > Have you seen the numerous NAK on such approach Mike did?
> > > > > 
> > > > > I haven't, but the folks reviewing previous versions have.  Do you
> > > > > have something specific in mind that you'd like to bring to my
> > > > > attention?
> > > > 
> > > > Unfortunately, I haven't been able to dig out such mails. But it's why
> > > > we ended up with clock protection code in various clock drivers
> > > > including:
> > > > 
> > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > > > 
> > > > Which is much more flexible, since you won't have to modify the DT to
> > > > change which clocks are to be left enabled, as well as way easier to
> > > > debug if you ever have to remove that property from the DT.
> > > 
> > > You're right, you don't have the change the DT in these cases.  You
> > > have to write new C code, which is _less_ flexible.
> > 
> > I'm sorry to learn that you never heard of that stable-DT thing.
> > 
> > And a bit sorry to see that a maintainer is really seeing C as not
> > flexible.
> 
> You're putting words in my mouth.  I didn't say C was not flexible.
> 
> I'm referencing the original DT pros i.e. it is possible to supply a
> different configuration without the need to compile the kernel.
> That's certainly true in this case.  We can provide a clk-provider and
> tag it as always-on, all without re-compile.
> 
> > > So all these platforms are adding their own hand-rolled version of
> > > this binding, adding more duplication and cruft to the kernel.
> > > Instead they can use this 'always-on' and we can consolidate and strip
> > > it all out.
> > 
> > Except that all these platforms are actually not implementing a
> > binding, ie not an interface with the DT they are bound to. Each and
> > every of these platforms can change that list whenever they wish, just
> > by sending a single one-liner patch (just like the DT, really.).
> > 
> > Which is not something that you can achieve with a DT binding.
> 
> Once again, can you give me more information about why you have such a
> problem with this binding.  I wish for it to be stable/ABI, I wish for
> it never to be removed, I envisage it will always be needed, so what's
> the problem? 
> 
> Do you have a vested interest that I am missing?
> 
> Perhaps an example of possible calamity will help convince me that
> you're not completely wrong and blowing everything out of proportion
> for no good reason.

Let's say you've introduced such a clock in kernel 4.0 for the memory
clock.

At some point down the road, you create a ddrfreq driver (if that ever
exists). You have a new driver, which will manage the clock.

In that driver, for some reason, you have to shutdown the clock to
reset the DDR controller. Of course that also means that you will be
removing the clk-always-on property from your DT.

You will have in your driver something like:

/* Reset our controller */
clk_disable(clk);
clk_enable(clk):

And then, you expect your controller to be in its out-of-reset
state. Which will be the case with a new DT, and not with the old one,
probably creating all kind of very entertaining issues to debug.

All of this wouldn't be the case if you had this inside the kernel,
since (hopefully) the kernel is consistent with itself.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Lee Jones]

On Wed, 22 Apr 2015, Maxime Ripard wrote:

> On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote:
> > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > 
> > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > 
> > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > > > > +
> > > > > > > > +		    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.
> > > > > > > 
> > > > > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > > > > actually happen. Where do you place the cursor?
> > > > > > 
> > > > > > That's up to Mike.
> > > > > 
> > > > > Except that Mike won't review any of the DT changes, so he won't be
> > > > > able to refrain users from using it. Let alone out-of-tree DTs using a
> > > > > mainline kernel.
> > > > 
> > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if
> > > > he isn't the DT guys are smart enough to either make the right
> > > > decisions themselves (Rob has Acked these bindings already, so will be
> > > > on the lookout for misuse, I'm sure), or ask for Mike's help.
> > > 
> > > Yeah, right, as if this strategy really worked in the past....
> > > 
> > > Do we really want to look at even the DT bindings that have actually
> > > been reviewed by maintainers that got merged?
> > > 
> > > They don't have time for that, which is totally fine, but we really
> > > should bury our head in the sand by actually thinking they will review
> > > every single DT-related patch.
> > > 
> > > Using that as an argument is just plain denial of what really happened
> > > for the past 4 years.
> > 
> > I agree that it's a problem, but this is a process problem and has
> > nothing to do with this set.  If you have a problem with the current
> > process and have a better alternative, submit your thoughts to the DT
> > list.  Rejecting all new bindings because you are frightened that they
> > will be used in a manner that they were not intended is not the way to
> > go though.
> 
> I'm not saying that this binding should not go in because of a process
> issue.
> 
> I'm saying that discarding arguments against your binding by adding
> restrictions that cannot be enforced is not reasonable.

I'm open to constructive suggestions/alternatives.

Hand rolling this stuff in C per vendor is not of of them.

> > > > > > > Should we create a new driver for our RAM controller, or do we want to
> > > > > > > use clock-always-on?
> > > > > > 
> > > > > > I would say that if all the driver did was to enable clocks, then you
> > > > > > should use this instead.  This binding was designed specifically for
> > > > > > that purpose.
> > > > > > 
> > > > > > However, if the aforementioned driver clock can be safely gated, then
> > > > > > it should not be an always-on clock.
> > > > > 
> > > > > Yeah, of course, I understand the original intent of it, but that
> > > > > argument, which might very well be true at one point in time, might
> > > > > not be true anymore two or three releases later.
> > > > 
> > > > Why?  The H/W isn't going to change in two or three releases.  The
> > > > clocks designated as 'always-on' will have to be on forever, or
> > > > synonymously, 'always'.
> > > >
> > > > > And that driver might actually rely on the fact that the clock is shut
> > > > > down, which won't be the case.
> > > > 
> > > > I think you are missing the point of this binding.  The driver can
> > > > never rely on that in this use-case.  If the clock is off, there is no
> > > > device driver, period. 
> > > 
> > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
> > > PM guys will be happy to hear that.
> > > 
> > > And they are not device drivers, are not mandatory in the system, and
> > > it's usually a good thing to keep the CPU running whenever you don't
> > > have such drivers.
> > > 
> > > > > Introducing a DT interface solely by refering to the current state of
> > > > > a driver is a bit odd.
> > > > 
> > > > I'm not sure I get your point.  This binding has nothing to do with
> > > > drivers.
> > > 
> > > It's all about drivers. Or rather all about missing drivers.
> > 
> > I think you are going to have to be more forthcoming with your issues
> > with this binding, because I'm struggling to understand what your
> > problem with it is.  You have already pointed me to vendors which have
> > a genuine/valid need for it.  But instead you'd prefer they hand-roll
> > their own implementations over multiple lines of C code (each).
> 
> I told you already.
> 
> If you have that property, there's absolutely no way to do any kind of
> clock management in the future.
> 
> It might be fine for your use case, but see my point about the
> unreasonable restriction. People are going to use it for clocks that
> just don't have a driver *yet*, and when that driver will be merged we
> will end up with a driver that (for example) makes the assumption that
> the clock has been shut down to reset the IP, that might or might not
> be the case, depending on how old the DT is exactly.

There is a need for this binding, but as you say, it must not be
abused.  So how to we get people not to use it willy-nilly?

IMO, if people choose to ignore the stark warning in the documentation
then that's they're lookout.  I guess you'd like to wrap them in more
cotton wool than I would.  That's fine too, but how.

> This will be even a bigger madness if you ask me.
> 
> > > > > > > Do we really want to enforce this if we ever gain a driver that would
> > > > > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > > > > driver yet?)
> > > > > > 
> > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > > > > should never be used. Situations where using always-on as a stop-gap
> > > > > > due to a lack of current functionality is what the paragraph above is
> > > > > > trying to mitigate.
> > > > > 
> > > > > But it's not really what this property is about. What this property
> > > > > describes is that these clocks should never be gated. Any point in
> > > > > time during the life of the system AND with in any kernel version.
> > > > 
> > > > You got it, that's correct -- these clocks should never be gated.
> > > > 
> > > > So why would that ever change?  If that is likely (or even possible)
> > > > to change in the future then this binding should not be used.
> > > >
> > > > To reiterate; this binding should be used on ungatable clocks only.
> > > > Non-negotiable, non-changeable either by the introduction of new
> > > > functionality/support or kernel version.
> > > 
> > > I'm pretty sure that if that patch gets merged, by the end of the
> > > year, there will be "incorrect" users by your standards.
> > 
> > It's possible to abuse any binding.  I don't see why you are so
> > offended of this one in particular.
> 
> I'm not offended, I just tried to push the same kind of patches two
> years ago, with Mike pushing back, and actually came to see that he
> was right a few monthes down the road.

Well this was suggested by Mike.  I even have his Ack already.  So I
guess he too has changed the error of his ways. :)

> And yeah, your point that any binding can be abused is true. This one
> is only so easy to abuse it's not even funny.
> 
> > > If you introduce a feature, you should expect people to use
> > > it.  If not, what's the point?
> > 
> > By your own admission, there are genuine users for this binding and I
> > expect people to use it.
> 
> The only thing that we really disagree upon is that whether that
> restriction will really be followed. You expect people to, I
> don't. It's the fundamental disagreement we have, that really prevent
> any purely technical discussion.
> 
> Maybe we can try to address that before moving forward?

I'd like that.

If a firm warning isn't good enough, then what will be?

> > > > > > > Have you seen the numerous NAK on such approach Mike did?
> > > > > > 
> > > > > > I haven't, but the folks reviewing previous versions have.  Do you
> > > > > > have something specific in mind that you'd like to bring to my
> > > > > > attention?
> > > > > 
> > > > > Unfortunately, I haven't been able to dig out such mails. But it's why
> > > > > we ended up with clock protection code in various clock drivers
> > > > > including:
> > > > > 
> > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > > > > 
> > > > > Which is much more flexible, since you won't have to modify the DT to
> > > > > change which clocks are to be left enabled, as well as way easier to
> > > > > debug if you ever have to remove that property from the DT.
> > > > 
> > > > You're right, you don't have the change the DT in these cases.  You
> > > > have to write new C code, which is _less_ flexible.
> > > 
> > > I'm sorry to learn that you never heard of that stable-DT thing.
> > > 
> > > And a bit sorry to see that a maintainer is really seeing C as not
> > > flexible.
> > 
> > You're putting words in my mouth.  I didn't say C was not flexible.
> > 
> > I'm referencing the original DT pros i.e. it is possible to supply a
> > different configuration without the need to compile the kernel.
> > That's certainly true in this case.  We can provide a clk-provider and
> > tag it as always-on, all without re-compile.
> > 
> > > > So all these platforms are adding their own hand-rolled version of
> > > > this binding, adding more duplication and cruft to the kernel.
> > > > Instead they can use this 'always-on' and we can consolidate and strip
> > > > it all out.
> > > 
> > > Except that all these platforms are actually not implementing a
> > > binding, ie not an interface with the DT they are bound to. Each and
> > > every of these platforms can change that list whenever they wish, just
> > > by sending a single one-liner patch (just like the DT, really.).
> > > 
> > > Which is not something that you can achieve with a DT binding.
> > 
> > Once again, can you give me more information about why you have such a
> > problem with this binding.  I wish for it to be stable/ABI, I wish for
> > it never to be removed, I envisage it will always be needed, so what's
> > the problem? 
> > 
> > Do you have a vested interest that I am missing?
> > 
> > Perhaps an example of possible calamity will help convince me that
> > you're not completely wrong and blowing everything out of proportion
> > for no good reason.
> 
> Let's say you've introduced such a clock in kernel 4.0 for the memory
> clock.
> 
> At some point down the road, you create a ddrfreq driver (if that ever
> exists). You have a new driver, which will manage the clock.
> 
> In that driver, for some reason, you have to shutdown the clock to
> reset the DDR controller. Of course that also means that you will be
> removing the clk-always-on property from your DT.
> 
> You will have in your driver something like:
> 
> /* Reset our controller */
> clk_disable(clk);
> clk_enable(clk):
> 
> And then, you expect your controller to be in its out-of-reset
> state. Which will be the case with a new DT, and not with the old one,
> probably creating all kind of very entertaining issues to debug.
> 
> All of this wouldn't be the case if you had this inside the kernel,
> since (hopefully) the kernel is consistent with itself.

Surely you must have realised already that DTBs are more tightly
coupled to kernel versions than we would have initially liked?  It's
naive to assume that old DTBs will 'just-work' with newer kernels.

Wrong decisions related to DT are being made daily.  Adding mistakenly
or foolhardily adding 'clk-always-on' to a DTS is not going to be the
sole cause of breakage somewhere down the line.

Pushing back on the acceptance of this binding based on idealistic,
possibly already out-of-date premise is just frustrating.

This useful binding should be accepted and people should not abuse
it.  If they do and the vendor Maintainer's review and accept then
they have no foundation for recourse.

Would you prefer it if I made the warning starker?

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

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Geert Uytterhoeven]

Hi Lee,

On Wed, Apr 29, 2015 at 4:17 PM, Lee Jones <lee.jones@linaro.org> wrote:
> This useful binding should be accepted and people should not abuse
> it.  If they do and the vendor Maintainer's review and accept then
> they have no foundation for recourse.
>
> Would you prefer it if I made the warning starker?

Perhaps you can add a check for "clock-always-on" to scripts/checkpatch.pl,
to make people aware they should think twice?

Nah, people don't read documentation, don't run checkpatch, ignore compiler
warnings, etc...

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Lee Jones]

On Wed, 29 Apr 2015, Geert Uytterhoeven wrote:
> On Wed, Apr 29, 2015 at 4:17 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > This useful binding should be accepted and people should not abuse
> > it.  If they do and the vendor Maintainer's review and accept then
> > they have no foundation for recourse.
> >
> > Would you prefer it if I made the warning starker?
> 
> Perhaps you can add a check for "clock-always-on" to scripts/checkpatch.pl,
> to make people aware they should think twice?

That's a really nice idea!

Maxime, would that make you happ(y|ier)?

> Nah, people don't read documentation, don't run checkpatch, ignore compiler
> warnings, etc...

:)

You can only wrap people in so much cotton wool.  If they then decide
to ignore all the warnings and jump, woe betide them.

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

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

On Wed, Apr 29, 2015 at 04:11:47PM +0100, Lee Jones wrote:
> On Wed, 29 Apr 2015, Geert Uytterhoeven wrote:
> > On Wed, Apr 29, 2015 at 4:17 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > > This useful binding should be accepted and people should not abuse
> > > it.  If they do and the vendor Maintainer's review and accept then
> > > they have no foundation for recourse.
> > >
> > > Would you prefer it if I made the warning starker?
> > 
> > Perhaps you can add a check for "clock-always-on" to scripts/checkpatch.pl,
> > to make people aware they should think twice?
> 
> That's a really nice idea!
> 
> Maxime, would that make you happ(y|ier)?

Do whatever you want.

This binding is going to be merged no matter what, so my opinion has
really little value here.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Sascha Hauer]

On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote:
> On Wed, 22 Apr 2015, Maxime Ripard wrote:
> 
> > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote:
> > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > 
> > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > > 
> > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > > > > > +
> > > > > > > > > +		    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.
> > > > > > > > 
> > > > > > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > > > > > actually happen. Where do you place the cursor?
> > > > > > > 
> > > > > > > That's up to Mike.
> > > > > > 
> > > > > > Except that Mike won't review any of the DT changes, so he won't be
> > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a
> > > > > > mainline kernel.
> > > > > 
> > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if
> > > > > he isn't the DT guys are smart enough to either make the right
> > > > > decisions themselves (Rob has Acked these bindings already, so will be
> > > > > on the lookout for misuse, I'm sure), or ask for Mike's help.
> > > > 
> > > > Yeah, right, as if this strategy really worked in the past....
> > > > 
> > > > Do we really want to look at even the DT bindings that have actually
> > > > been reviewed by maintainers that got merged?
> > > > 
> > > > They don't have time for that, which is totally fine, but we really
> > > > should bury our head in the sand by actually thinking they will review
> > > > every single DT-related patch.
> > > > 
> > > > Using that as an argument is just plain denial of what really happened
> > > > for the past 4 years.
> > > 
> > > I agree that it's a problem, but this is a process problem and has
> > > nothing to do with this set.  If you have a problem with the current
> > > process and have a better alternative, submit your thoughts to the DT
> > > list.  Rejecting all new bindings because you are frightened that they
> > > will be used in a manner that they were not intended is not the way to
> > > go though.
> > 
> > I'm not saying that this binding should not go in because of a process
> > issue.
> > 
> > I'm saying that discarding arguments against your binding by adding
> > restrictions that cannot be enforced is not reasonable.
> 
> I'm open to constructive suggestions/alternatives.
> 
> Hand rolling this stuff in C per vendor is not of of them.
> 
> > > > > > > > Should we create a new driver for our RAM controller, or do we want to
> > > > > > > > use clock-always-on?
> > > > > > > 
> > > > > > > I would say that if all the driver did was to enable clocks, then you
> > > > > > > should use this instead.  This binding was designed specifically for
> > > > > > > that purpose.
> > > > > > > 
> > > > > > > However, if the aforementioned driver clock can be safely gated, then
> > > > > > > it should not be an always-on clock.
> > > > > > 
> > > > > > Yeah, of course, I understand the original intent of it, but that
> > > > > > argument, which might very well be true at one point in time, might
> > > > > > not be true anymore two or three releases later.
> > > > > 
> > > > > Why?  The H/W isn't going to change in two or three releases.  The
> > > > > clocks designated as 'always-on' will have to be on forever, or
> > > > > synonymously, 'always'.
> > > > >
> > > > > > And that driver might actually rely on the fact that the clock is shut
> > > > > > down, which won't be the case.
> > > > > 
> > > > > I think you are missing the point of this binding.  The driver can
> > > > > never rely on that in this use-case.  If the clock is off, there is no
> > > > > device driver, period. 
> > > > 
> > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
> > > > PM guys will be happy to hear that.
> > > > 
> > > > And they are not device drivers, are not mandatory in the system, and
> > > > it's usually a good thing to keep the CPU running whenever you don't
> > > > have such drivers.
> > > > 
> > > > > > Introducing a DT interface solely by refering to the current state of
> > > > > > a driver is a bit odd.
> > > > > 
> > > > > I'm not sure I get your point.  This binding has nothing to do with
> > > > > drivers.
> > > > 
> > > > It's all about drivers. Or rather all about missing drivers.
> > > 
> > > I think you are going to have to be more forthcoming with your issues
> > > with this binding, because I'm struggling to understand what your
> > > problem with it is.  You have already pointed me to vendors which have
> > > a genuine/valid need for it.  But instead you'd prefer they hand-roll
> > > their own implementations over multiple lines of C code (each).
> > 
> > I told you already.
> > 
> > If you have that property, there's absolutely no way to do any kind of
> > clock management in the future.
> > 
> > It might be fine for your use case, but see my point about the
> > unreasonable restriction. People are going to use it for clocks that
> > just don't have a driver *yet*, and when that driver will be merged we
> > will end up with a driver that (for example) makes the assumption that
> > the clock has been shut down to reset the IP, that might or might not
> > be the case, depending on how old the DT is exactly.
> 
> There is a need for this binding, but as you say, it must not be
> abused.  So how to we get people not to use it willy-nilly?
> 
> IMO, if people choose to ignore the stark warning in the documentation
> then that's they're lookout.  I guess you'd like to wrap them in more
> cotton wool than I would.  That's fine too, but how.
> 
> > This will be even a bigger madness if you ask me.
> > 
> > > > > > > > Do we really want to enforce this if we ever gain a driver that would
> > > > > > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > > > > > driver yet?)
> > > > > > > 
> > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > > > > > should never be used. Situations where using always-on as a stop-gap
> > > > > > > due to a lack of current functionality is what the paragraph above is
> > > > > > > trying to mitigate.
> > > > > > 
> > > > > > But it's not really what this property is about. What this property
> > > > > > describes is that these clocks should never be gated. Any point in
> > > > > > time during the life of the system AND with in any kernel version.
> > > > > 
> > > > > You got it, that's correct -- these clocks should never be gated.
> > > > > 
> > > > > So why would that ever change?  If that is likely (or even possible)
> > > > > to change in the future then this binding should not be used.
> > > > >
> > > > > To reiterate; this binding should be used on ungatable clocks only.
> > > > > Non-negotiable, non-changeable either by the introduction of new
> > > > > functionality/support or kernel version.
> > > > 
> > > > I'm pretty sure that if that patch gets merged, by the end of the
> > > > year, there will be "incorrect" users by your standards.
> > > 
> > > It's possible to abuse any binding.  I don't see why you are so
> > > offended of this one in particular.
> > 
> > I'm not offended, I just tried to push the same kind of patches two
> > years ago, with Mike pushing back, and actually came to see that he
> > was right a few monthes down the road.
> 
> Well this was suggested by Mike.  I even have his Ack already.  So I
> guess he too has changed the error of his ways. :)
> 
> > And yeah, your point that any binding can be abused is true. This one
> > is only so easy to abuse it's not even funny.
> > 
> > > > If you introduce a feature, you should expect people to use
> > > > it.  If not, what's the point?
> > > 
> > > By your own admission, there are genuine users for this binding and I
> > > expect people to use it.
> > 
> > The only thing that we really disagree upon is that whether that
> > restriction will really be followed. You expect people to, I
> > don't. It's the fundamental disagreement we have, that really prevent
> > any purely technical discussion.
> > 
> > Maybe we can try to address that before moving forward?
> 
> I'd like that.
> 
> If a firm warning isn't good enough, then what will be?
> 
> > > > > > > > Have you seen the numerous NAK on such approach Mike did?
> > > > > > > 
> > > > > > > I haven't, but the folks reviewing previous versions have.  Do you
> > > > > > > have something specific in mind that you'd like to bring to my
> > > > > > > attention?
> > > > > > 
> > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why
> > > > > > we ended up with clock protection code in various clock drivers
> > > > > > including:
> > > > > > 
> > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > > > > > 
> > > > > > Which is much more flexible, since you won't have to modify the DT to
> > > > > > change which clocks are to be left enabled, as well as way easier to
> > > > > > debug if you ever have to remove that property from the DT.
> > > > > 
> > > > > You're right, you don't have the change the DT in these cases.  You
> > > > > have to write new C code, which is _less_ flexible.
> > > > 
> > > > I'm sorry to learn that you never heard of that stable-DT thing.
> > > > 
> > > > And a bit sorry to see that a maintainer is really seeing C as not
> > > > flexible.
> > > 
> > > You're putting words in my mouth.  I didn't say C was not flexible.
> > > 
> > > I'm referencing the original DT pros i.e. it is possible to supply a
> > > different configuration without the need to compile the kernel.
> > > That's certainly true in this case.  We can provide a clk-provider and
> > > tag it as always-on, all without re-compile.
> > > 
> > > > > So all these platforms are adding their own hand-rolled version of
> > > > > this binding, adding more duplication and cruft to the kernel.
> > > > > Instead they can use this 'always-on' and we can consolidate and strip
> > > > > it all out.
> > > > 
> > > > Except that all these platforms are actually not implementing a
> > > > binding, ie not an interface with the DT they are bound to. Each and
> > > > every of these platforms can change that list whenever they wish, just
> > > > by sending a single one-liner patch (just like the DT, really.).
> > > > 
> > > > Which is not something that you can achieve with a DT binding.
> > > 
> > > Once again, can you give me more information about why you have such a
> > > problem with this binding.  I wish for it to be stable/ABI, I wish for
> > > it never to be removed, I envisage it will always be needed, so what's
> > > the problem? 
> > > 
> > > Do you have a vested interest that I am missing?
> > > 
> > > Perhaps an example of possible calamity will help convince me that
> > > you're not completely wrong and blowing everything out of proportion
> > > for no good reason.
> > 
> > Let's say you've introduced such a clock in kernel 4.0 for the memory
> > clock.
> > 
> > At some point down the road, you create a ddrfreq driver (if that ever
> > exists). You have a new driver, which will manage the clock.
> > 
> > In that driver, for some reason, you have to shutdown the clock to
> > reset the DDR controller. Of course that also means that you will be
> > removing the clk-always-on property from your DT.
> > 
> > You will have in your driver something like:
> > 
> > /* Reset our controller */
> > clk_disable(clk);
> > clk_enable(clk):
> > 
> > And then, you expect your controller to be in its out-of-reset
> > state. Which will be the case with a new DT, and not with the old one,
> > probably creating all kind of very entertaining issues to debug.
> > 
> > All of this wouldn't be the case if you had this inside the kernel,
> > since (hopefully) the kernel is consistent with itself.
> 
> Surely you must have realised already that DTBs are more tightly
> coupled to kernel versions than we would have initially liked?  It's
> naive to assume that old DTBs will 'just-work' with newer kernels.
> 
> Wrong decisions related to DT are being made daily.  Adding mistakenly
> or foolhardily adding 'clk-always-on' to a DTS is not going to be the
> sole cause of breakage somewhere down the line.

It may not be the sole cause of breakage, but one we can avoid.

> 
> Pushing back on the acceptance of this binding based on idealistic,
> possibly already out-of-date premise is just frustrating.
> 
> This useful binding should be accepted and people should not abuse
> it.  If they do and the vendor Maintainer's review and accept then
> they have no foundation for recourse.

Maxime gave a good example for a clock which looks like a candidate for
clk-always-on but needs to be turned off in a later kernel revision.
Could you elaborate why you still want to have the clk-always-on in the
device tree instead of in the kernel where it can be removed when
necessary? What's your problem with enabling the critical clocks using
clk_prepare_enable like other SoCs already do?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Lee Jones]

On Wed, 29 Apr 2015, Sascha Hauer wrote:

> On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote:
> > On Wed, 22 Apr 2015, Maxime Ripard wrote:
> > 
> > > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote:
> > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > 
> > > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > > > 
> > > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > > > > > > +
> > > > > > > > > > +		    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.
> > > > > > > > > 
> > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > > > > > > actually happen. Where do you place the cursor?
> > > > > > > > 
> > > > > > > > That's up to Mike.
> > > > > > > 
> > > > > > > Except that Mike won't review any of the DT changes, so he won't be
> > > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a
> > > > > > > mainline kernel.
> > > > > > 
> > > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if
> > > > > > he isn't the DT guys are smart enough to either make the right
> > > > > > decisions themselves (Rob has Acked these bindings already, so will be
> > > > > > on the lookout for misuse, I'm sure), or ask for Mike's help.
> > > > > 
> > > > > Yeah, right, as if this strategy really worked in the past....
> > > > > 
> > > > > Do we really want to look at even the DT bindings that have actually
> > > > > been reviewed by maintainers that got merged?
> > > > > 
> > > > > They don't have time for that, which is totally fine, but we really
> > > > > should bury our head in the sand by actually thinking they will review
> > > > > every single DT-related patch.
> > > > > 
> > > > > Using that as an argument is just plain denial of what really happened
> > > > > for the past 4 years.
> > > > 
> > > > I agree that it's a problem, but this is a process problem and has
> > > > nothing to do with this set.  If you have a problem with the current
> > > > process and have a better alternative, submit your thoughts to the DT
> > > > list.  Rejecting all new bindings because you are frightened that they
> > > > will be used in a manner that they were not intended is not the way to
> > > > go though.
> > > 
> > > I'm not saying that this binding should not go in because of a process
> > > issue.
> > > 
> > > I'm saying that discarding arguments against your binding by adding
> > > restrictions that cannot be enforced is not reasonable.
> > 
> > I'm open to constructive suggestions/alternatives.
> > 
> > Hand rolling this stuff in C per vendor is not of of them.
> > 
> > > > > > > > > Should we create a new driver for our RAM controller, or do we want to
> > > > > > > > > use clock-always-on?
> > > > > > > > 
> > > > > > > > I would say that if all the driver did was to enable clocks, then you
> > > > > > > > should use this instead.  This binding was designed specifically for
> > > > > > > > that purpose.
> > > > > > > > 
> > > > > > > > However, if the aforementioned driver clock can be safely gated, then
> > > > > > > > it should not be an always-on clock.
> > > > > > > 
> > > > > > > Yeah, of course, I understand the original intent of it, but that
> > > > > > > argument, which might very well be true at one point in time, might
> > > > > > > not be true anymore two or three releases later.
> > > > > > 
> > > > > > Why?  The H/W isn't going to change in two or three releases.  The
> > > > > > clocks designated as 'always-on' will have to be on forever, or
> > > > > > synonymously, 'always'.
> > > > > >
> > > > > > > And that driver might actually rely on the fact that the clock is shut
> > > > > > > down, which won't be the case.
> > > > > > 
> > > > > > I think you are missing the point of this binding.  The driver can
> > > > > > never rely on that in this use-case.  If the clock is off, there is no
> > > > > > device driver, period. 
> > > > > 
> > > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
> > > > > PM guys will be happy to hear that.
> > > > > 
> > > > > And they are not device drivers, are not mandatory in the system, and
> > > > > it's usually a good thing to keep the CPU running whenever you don't
> > > > > have such drivers.
> > > > > 
> > > > > > > Introducing a DT interface solely by refering to the current state of
> > > > > > > a driver is a bit odd.
> > > > > > 
> > > > > > I'm not sure I get your point.  This binding has nothing to do with
> > > > > > drivers.
> > > > > 
> > > > > It's all about drivers. Or rather all about missing drivers.
> > > > 
> > > > I think you are going to have to be more forthcoming with your issues
> > > > with this binding, because I'm struggling to understand what your
> > > > problem with it is.  You have already pointed me to vendors which have
> > > > a genuine/valid need for it.  But instead you'd prefer they hand-roll
> > > > their own implementations over multiple lines of C code (each).
> > > 
> > > I told you already.
> > > 
> > > If you have that property, there's absolutely no way to do any kind of
> > > clock management in the future.
> > > 
> > > It might be fine for your use case, but see my point about the
> > > unreasonable restriction. People are going to use it for clocks that
> > > just don't have a driver *yet*, and when that driver will be merged we
> > > will end up with a driver that (for example) makes the assumption that
> > > the clock has been shut down to reset the IP, that might or might not
> > > be the case, depending on how old the DT is exactly.
> > 
> > There is a need for this binding, but as you say, it must not be
> > abused.  So how to we get people not to use it willy-nilly?
> > 
> > IMO, if people choose to ignore the stark warning in the documentation
> > then that's they're lookout.  I guess you'd like to wrap them in more
> > cotton wool than I would.  That's fine too, but how.
> > 
> > > This will be even a bigger madness if you ask me.
> > > 
> > > > > > > > > Do we really want to enforce this if we ever gain a driver that would
> > > > > > > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > > > > > > driver yet?)
> > > > > > > > 
> > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > > > > > > should never be used. Situations where using always-on as a stop-gap
> > > > > > > > due to a lack of current functionality is what the paragraph above is
> > > > > > > > trying to mitigate.
> > > > > > > 
> > > > > > > But it's not really what this property is about. What this property
> > > > > > > describes is that these clocks should never be gated. Any point in
> > > > > > > time during the life of the system AND with in any kernel version.
> > > > > > 
> > > > > > You got it, that's correct -- these clocks should never be gated.
> > > > > > 
> > > > > > So why would that ever change?  If that is likely (or even possible)
> > > > > > to change in the future then this binding should not be used.
> > > > > >
> > > > > > To reiterate; this binding should be used on ungatable clocks only.
> > > > > > Non-negotiable, non-changeable either by the introduction of new
> > > > > > functionality/support or kernel version.
> > > > > 
> > > > > I'm pretty sure that if that patch gets merged, by the end of the
> > > > > year, there will be "incorrect" users by your standards.
> > > > 
> > > > It's possible to abuse any binding.  I don't see why you are so
> > > > offended of this one in particular.
> > > 
> > > I'm not offended, I just tried to push the same kind of patches two
> > > years ago, with Mike pushing back, and actually came to see that he
> > > was right a few monthes down the road.
> > 
> > Well this was suggested by Mike.  I even have his Ack already.  So I
> > guess he too has changed the error of his ways. :)
> > 
> > > And yeah, your point that any binding can be abused is true. This one
> > > is only so easy to abuse it's not even funny.
> > > 
> > > > > If you introduce a feature, you should expect people to use
> > > > > it.  If not, what's the point?
> > > > 
> > > > By your own admission, there are genuine users for this binding and I
> > > > expect people to use it.
> > > 
> > > The only thing that we really disagree upon is that whether that
> > > restriction will really be followed. You expect people to, I
> > > don't. It's the fundamental disagreement we have, that really prevent
> > > any purely technical discussion.
> > > 
> > > Maybe we can try to address that before moving forward?
> > 
> > I'd like that.
> > 
> > If a firm warning isn't good enough, then what will be?
> > 
> > > > > > > > > Have you seen the numerous NAK on such approach Mike did?
> > > > > > > > 
> > > > > > > > I haven't, but the folks reviewing previous versions have.  Do you
> > > > > > > > have something specific in mind that you'd like to bring to my
> > > > > > > > attention?
> > > > > > > 
> > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why
> > > > > > > we ended up with clock protection code in various clock drivers
> > > > > > > including:
> > > > > > > 
> > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > > > > > > 
> > > > > > > Which is much more flexible, since you won't have to modify the DT to
> > > > > > > change which clocks are to be left enabled, as well as way easier to
> > > > > > > debug if you ever have to remove that property from the DT.
> > > > > > 
> > > > > > You're right, you don't have the change the DT in these cases.  You
> > > > > > have to write new C code, which is _less_ flexible.
> > > > > 
> > > > > I'm sorry to learn that you never heard of that stable-DT thing.
> > > > > 
> > > > > And a bit sorry to see that a maintainer is really seeing C as not
> > > > > flexible.
> > > > 
> > > > You're putting words in my mouth.  I didn't say C was not flexible.
> > > > 
> > > > I'm referencing the original DT pros i.e. it is possible to supply a
> > > > different configuration without the need to compile the kernel.
> > > > That's certainly true in this case.  We can provide a clk-provider and
> > > > tag it as always-on, all without re-compile.
> > > > 
> > > > > > So all these platforms are adding their own hand-rolled version of
> > > > > > this binding, adding more duplication and cruft to the kernel.
> > > > > > Instead they can use this 'always-on' and we can consolidate and strip
> > > > > > it all out.
> > > > > 
> > > > > Except that all these platforms are actually not implementing a
> > > > > binding, ie not an interface with the DT they are bound to. Each and
> > > > > every of these platforms can change that list whenever they wish, just
> > > > > by sending a single one-liner patch (just like the DT, really.).
> > > > > 
> > > > > Which is not something that you can achieve with a DT binding.
> > > > 
> > > > Once again, can you give me more information about why you have such a
> > > > problem with this binding.  I wish for it to be stable/ABI, I wish for
> > > > it never to be removed, I envisage it will always be needed, so what's
> > > > the problem? 
> > > > 
> > > > Do you have a vested interest that I am missing?
> > > > 
> > > > Perhaps an example of possible calamity will help convince me that
> > > > you're not completely wrong and blowing everything out of proportion
> > > > for no good reason.
> > > 
> > > Let's say you've introduced such a clock in kernel 4.0 for the memory
> > > clock.
> > > 
> > > At some point down the road, you create a ddrfreq driver (if that ever
> > > exists). You have a new driver, which will manage the clock.
> > > 
> > > In that driver, for some reason, you have to shutdown the clock to
> > > reset the DDR controller. Of course that also means that you will be
> > > removing the clk-always-on property from your DT.
> > > 
> > > You will have in your driver something like:
> > > 
> > > /* Reset our controller */
> > > clk_disable(clk);
> > > clk_enable(clk):
> > > 
> > > And then, you expect your controller to be in its out-of-reset
> > > state. Which will be the case with a new DT, and not with the old one,
> > > probably creating all kind of very entertaining issues to debug.
> > > 
> > > All of this wouldn't be the case if you had this inside the kernel,
> > > since (hopefully) the kernel is consistent with itself.
> > 
> > Surely you must have realised already that DTBs are more tightly
> > coupled to kernel versions than we would have initially liked?  It's
> > naive to assume that old DTBs will 'just-work' with newer kernels.
> > 
> > Wrong decisions related to DT are being made daily.  Adding mistakenly
> > or foolhardily adding 'clk-always-on' to a DTS is not going to be the
> > sole cause of breakage somewhere down the line.
> 
> It may not be the sole cause of breakage, but one we can avoid.
> 
> > 
> > Pushing back on the acceptance of this binding based on idealistic,
> > possibly already out-of-date premise is just frustrating.
> > 
> > This useful binding should be accepted and people should not abuse
> > it.  If they do and the vendor Maintainer's review and accept then
> > they have no foundation for recourse.
> 
> Maxime gave a good example for a clock which looks like a candidate for
> clk-always-on but needs to be turned off in a later kernel revision.

In the example Maxime gave, I'm struggling to understand how you might
enable such a framework whist still using an old DT.  If you want to
make use of new features/drivers/enablement/power-saving you must be
in a position to edit your platform code/Device Tree.

> Could you elaborate why you still want to have the clk-always-on in the
> device tree instead of in the kernel where it can be removed when
> necessary? What's your problem with enabling the critical clocks using
> clk_prepare_enable like other SoCs already do?

I've explained what my issues are already.  I'm not a fan of
hand-rolling and duplicating code which can be consolidated and make
generic.  Call me old fashioned. :)
 
-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Michael Turquette]

Quoting Lee Jones (2015-04-29 09:07:13)
> On Wed, 29 Apr 2015, Sascha Hauer wrote:
> 
> > On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote:
> > > On Wed, 22 Apr 2015, Maxime Ripard wrote:
> > > 
> > > > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote:
> > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > > 
> > > > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> > > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > > > > 
> > > > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > > > > > > > +
> > > > > > > > > > > +               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.
> > > > > > > > > > 
> > > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > > > > > > > actually happen. Where do you place the cursor?
> > > > > > > > > 
> > > > > > > > > That's up to Mike.
> > > > > > > > 
> > > > > > > > Except that Mike won't review any of the DT changes, so he won't be
> > > > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a
> > > > > > > > mainline kernel.
> > > > > > > 
> > > > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if
> > > > > > > he isn't the DT guys are smart enough to either make the right
> > > > > > > decisions themselves (Rob has Acked these bindings already, so will be
> > > > > > > on the lookout for misuse, I'm sure), or ask for Mike's help.
> > > > > > 
> > > > > > Yeah, right, as if this strategy really worked in the past....
> > > > > > 
> > > > > > Do we really want to look at even the DT bindings that have actually
> > > > > > been reviewed by maintainers that got merged?
> > > > > > 
> > > > > > They don't have time for that, which is totally fine, but we really
> > > > > > should bury our head in the sand by actually thinking they will review
> > > > > > every single DT-related patch.
> > > > > > 
> > > > > > Using that as an argument is just plain denial of what really happened
> > > > > > for the past 4 years.
> > > > > 
> > > > > I agree that it's a problem, but this is a process problem and has
> > > > > nothing to do with this set.  If you have a problem with the current
> > > > > process and have a better alternative, submit your thoughts to the DT
> > > > > list.  Rejecting all new bindings because you are frightened that they
> > > > > will be used in a manner that they were not intended is not the way to
> > > > > go though.
> > > > 
> > > > I'm not saying that this binding should not go in because of a process
> > > > issue.
> > > > 
> > > > I'm saying that discarding arguments against your binding by adding
> > > > restrictions that cannot be enforced is not reasonable.
> > > 
> > > I'm open to constructive suggestions/alternatives.
> > > 
> > > Hand rolling this stuff in C per vendor is not of of them.
> > > 
> > > > > > > > > > Should we create a new driver for our RAM controller, or do we want to
> > > > > > > > > > use clock-always-on?
> > > > > > > > > 
> > > > > > > > > I would say that if all the driver did was to enable clocks, then you
> > > > > > > > > should use this instead.  This binding was designed specifically for
> > > > > > > > > that purpose.
> > > > > > > > > 
> > > > > > > > > However, if the aforementioned driver clock can be safely gated, then
> > > > > > > > > it should not be an always-on clock.
> > > > > > > > 
> > > > > > > > Yeah, of course, I understand the original intent of it, but that
> > > > > > > > argument, which might very well be true at one point in time, might
> > > > > > > > not be true anymore two or three releases later.
> > > > > > > 
> > > > > > > Why?  The H/W isn't going to change in two or three releases.  The
> > > > > > > clocks designated as 'always-on' will have to be on forever, or
> > > > > > > synonymously, 'always'.
> > > > > > >
> > > > > > > > And that driver might actually rely on the fact that the clock is shut
> > > > > > > > down, which won't be the case.
> > > > > > > 
> > > > > > > I think you are missing the point of this binding.  The driver can
> > > > > > > never rely on that in this use-case.  If the clock is off, there is no
> > > > > > > device driver, period. 
> > > > > > 
> > > > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
> > > > > > PM guys will be happy to hear that.
> > > > > > 
> > > > > > And they are not device drivers, are not mandatory in the system, and
> > > > > > it's usually a good thing to keep the CPU running whenever you don't
> > > > > > have such drivers.
> > > > > > 
> > > > > > > > Introducing a DT interface solely by refering to the current state of
> > > > > > > > a driver is a bit odd.
> > > > > > > 
> > > > > > > I'm not sure I get your point.  This binding has nothing to do with
> > > > > > > drivers.
> > > > > > 
> > > > > > It's all about drivers. Or rather all about missing drivers.
> > > > > 
> > > > > I think you are going to have to be more forthcoming with your issues
> > > > > with this binding, because I'm struggling to understand what your
> > > > > problem with it is.  You have already pointed me to vendors which have
> > > > > a genuine/valid need for it.  But instead you'd prefer they hand-roll
> > > > > their own implementations over multiple lines of C code (each).
> > > > 
> > > > I told you already.
> > > > 
> > > > If you have that property, there's absolutely no way to do any kind of
> > > > clock management in the future.
> > > > 
> > > > It might be fine for your use case, but see my point about the
> > > > unreasonable restriction. People are going to use it for clocks that
> > > > just don't have a driver *yet*, and when that driver will be merged we
> > > > will end up with a driver that (for example) makes the assumption that
> > > > the clock has been shut down to reset the IP, that might or might not
> > > > be the case, depending on how old the DT is exactly.
> > > 
> > > There is a need for this binding, but as you say, it must not be
> > > abused.  So how to we get people not to use it willy-nilly?
> > > 
> > > IMO, if people choose to ignore the stark warning in the documentation
> > > then that's they're lookout.  I guess you'd like to wrap them in more
> > > cotton wool than I would.  That's fine too, but how.
> > > 
> > > > This will be even a bigger madness if you ask me.
> > > > 
> > > > > > > > > > Do we really want to enforce this if we ever gain a driver that would
> > > > > > > > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > > > > > > > driver yet?)
> > > > > > > > > 
> > > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > > > > > > > should never be used. Situations where using always-on as a stop-gap
> > > > > > > > > due to a lack of current functionality is what the paragraph above is
> > > > > > > > > trying to mitigate.
> > > > > > > > 
> > > > > > > > But it's not really what this property is about. What this property
> > > > > > > > describes is that these clocks should never be gated. Any point in
> > > > > > > > time during the life of the system AND with in any kernel version.
> > > > > > > 
> > > > > > > You got it, that's correct -- these clocks should never be gated.
> > > > > > > 
> > > > > > > So why would that ever change?  If that is likely (or even possible)
> > > > > > > to change in the future then this binding should not be used.
> > > > > > >
> > > > > > > To reiterate; this binding should be used on ungatable clocks only.
> > > > > > > Non-negotiable, non-changeable either by the introduction of new
> > > > > > > functionality/support or kernel version.
> > > > > > 
> > > > > > I'm pretty sure that if that patch gets merged, by the end of the
> > > > > > year, there will be "incorrect" users by your standards.
> > > > > 
> > > > > It's possible to abuse any binding.  I don't see why you are so
> > > > > offended of this one in particular.
> > > > 
> > > > I'm not offended, I just tried to push the same kind of patches two
> > > > years ago, with Mike pushing back, and actually came to see that he
> > > > was right a few monthes down the road.
> > > 
> > > Well this was suggested by Mike.  I even have his Ack already.  So I
> > > guess he too has changed the error of his ways. :)
> > > 
> > > > And yeah, your point that any binding can be abused is true. This one
> > > > is only so easy to abuse it's not even funny.
> > > > 
> > > > > > If you introduce a feature, you should expect people to use
> > > > > > it.  If not, what's the point?
> > > > > 
> > > > > By your own admission, there are genuine users for this binding and I
> > > > > expect people to use it.
> > > > 
> > > > The only thing that we really disagree upon is that whether that
> > > > restriction will really be followed. You expect people to, I
> > > > don't. It's the fundamental disagreement we have, that really prevent
> > > > any purely technical discussion.
> > > > 
> > > > Maybe we can try to address that before moving forward?
> > > 
> > > I'd like that.
> > > 
> > > If a firm warning isn't good enough, then what will be?
> > > 
> > > > > > > > > > Have you seen the numerous NAK on such approach Mike did?
> > > > > > > > > 
> > > > > > > > > I haven't, but the folks reviewing previous versions have.  Do you
> > > > > > > > > have something specific in mind that you'd like to bring to my
> > > > > > > > > attention?
> > > > > > > > 
> > > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why
> > > > > > > > we ended up with clock protection code in various clock drivers
> > > > > > > > including:
> > > > > > > > 
> > > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > > > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > > > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > > > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > > > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > > > > > > > 
> > > > > > > > Which is much more flexible, since you won't have to modify the DT to
> > > > > > > > change which clocks are to be left enabled, as well as way easier to
> > > > > > > > debug if you ever have to remove that property from the DT.
> > > > > > > 
> > > > > > > You're right, you don't have the change the DT in these cases.  You
> > > > > > > have to write new C code, which is _less_ flexible.
> > > > > > 
> > > > > > I'm sorry to learn that you never heard of that stable-DT thing.
> > > > > > 
> > > > > > And a bit sorry to see that a maintainer is really seeing C as not
> > > > > > flexible.
> > > > > 
> > > > > You're putting words in my mouth.  I didn't say C was not flexible.
> > > > > 
> > > > > I'm referencing the original DT pros i.e. it is possible to supply a
> > > > > different configuration without the need to compile the kernel.
> > > > > That's certainly true in this case.  We can provide a clk-provider and
> > > > > tag it as always-on, all without re-compile.
> > > > > 
> > > > > > > So all these platforms are adding their own hand-rolled version of
> > > > > > > this binding, adding more duplication and cruft to the kernel.
> > > > > > > Instead they can use this 'always-on' and we can consolidate and strip
> > > > > > > it all out.
> > > > > > 
> > > > > > Except that all these platforms are actually not implementing a
> > > > > > binding, ie not an interface with the DT they are bound to. Each and
> > > > > > every of these platforms can change that list whenever they wish, just
> > > > > > by sending a single one-liner patch (just like the DT, really.).
> > > > > > 
> > > > > > Which is not something that you can achieve with a DT binding.
> > > > > 
> > > > > Once again, can you give me more information about why you have such a
> > > > > problem with this binding.  I wish for it to be stable/ABI, I wish for
> > > > > it never to be removed, I envisage it will always be needed, so what's
> > > > > the problem? 
> > > > > 
> > > > > Do you have a vested interest that I am missing?
> > > > > 
> > > > > Perhaps an example of possible calamity will help convince me that
> > > > > you're not completely wrong and blowing everything out of proportion
> > > > > for no good reason.
> > > > 
> > > > Let's say you've introduced such a clock in kernel 4.0 for the memory
> > > > clock.
> > > > 
> > > > At some point down the road, you create a ddrfreq driver (if that ever
> > > > exists). You have a new driver, which will manage the clock.
> > > > 
> > > > In that driver, for some reason, you have to shutdown the clock to
> > > > reset the DDR controller. Of course that also means that you will be
> > > > removing the clk-always-on property from your DT.
> > > > 
> > > > You will have in your driver something like:
> > > > 
> > > > /* Reset our controller */
> > > > clk_disable(clk);
> > > > clk_enable(clk):
> > > > 
> > > > And then, you expect your controller to be in its out-of-reset
> > > > state. Which will be the case with a new DT, and not with the old one,
> > > > probably creating all kind of very entertaining issues to debug.
> > > > 
> > > > All of this wouldn't be the case if you had this inside the kernel,
> > > > since (hopefully) the kernel is consistent with itself.
> > > 
> > > Surely you must have realised already that DTBs are more tightly
> > > coupled to kernel versions than we would have initially liked?  It's
> > > naive to assume that old DTBs will 'just-work' with newer kernels.
> > > 
> > > Wrong decisions related to DT are being made daily.  Adding mistakenly
> > > or foolhardily adding 'clk-always-on' to a DTS is not going to be the
> > > sole cause of breakage somewhere down the line.
> > 
> > It may not be the sole cause of breakage, but one we can avoid.
> > 
> > > 
> > > Pushing back on the acceptance of this binding based on idealistic,
> > > possibly already out-of-date premise is just frustrating.
> > > 
> > > This useful binding should be accepted and people should not abuse
> > > it.  If they do and the vendor Maintainer's review and accept then
> > > they have no foundation for recourse.
> > 
> > Maxime gave a good example for a clock which looks like a candidate for
> > clk-always-on but needs to be turned off in a later kernel revision.
> 
> In the example Maxime gave, I'm struggling to understand how you might
> enable such a framework whist still using an old DT.  If you want to
> make use of new features/drivers/enablement/power-saving you must be
> in a position to edit your platform code/Device Tree.

Not true. The DT sticklers out there will point out that DT should
describe the hardware. We have clock bindings for this. Whether or not
Linux controls a given clk node for now or later on is irrelevant to the
correctness of the binding and the dts/dtb.

You could deploy a fully realized dtb on a factory image for a device,
but later update the device in the field by replacing two loadable
kernel modules:

1) the clock provider driver
2) the clock consumer driver

This requires no change to DT and would allow for a clock consumer
driver to ignore aggressive clock gating in the factory image (clock
provider driver calls clk_get, clk_prepare_enable) and then later on
receive and update to your kernel modules that enabled aggressive power
management (clock provider only registers the clk, clock consumer driver
calls clk_get, clk_prepare_enable).

> 
> > Could you elaborate why you still want to have the clk-always-on in the
> > device tree instead of in the kernel where it can be removed when
> > necessary? What's your problem with enabling the critical clocks using
> > clk_prepare_enable like other SoCs already do?
> 
> I've explained what my issues are already.  I'm not a fan of
> hand-rolling and duplicating code which can be consolidated and make
> generic.  Call me old fashioned. :)

I agree with Lee that the open code stuff isn't great, but I'm less and
less convinced that the DT method is the way to go. Maxime has done a
good job of reminding me why I always pushed back on this type of
approach in the past.

Having a clock provider driver call clk_get, clk_prepare_enable on a clk
is just fine. I think what needs to be done is to look at the platforms
that open code this and find out how to replace this with some common
code that any clock provider driver can call. Perhaps we can:

1) use the struct clk_ops.init callback (which is used very little) and
pass in a generic function to handle this case, or

2) we can create a new per-clk flag which is used by __clk_init to call
clk_prepare_enable, or

3) we can add a new generic function like clk_register which sets any
specified defaults (clk_set_defaults), but using C code and not DT

I would need to look at the drivers that open code their
clk_prepare_enable calls for non-Linux devices and see what similarities
exist. But clearly the DT element of Lee's approach is causing some push
back, so we should consider if there is a less controversial way to do
this (and a way that benefits non-DT platforms as well).

I do think Lee's idea of consolidating around a single solution to a
common problem is a great idea, but maybe not by using Devicetree.

Regards,
Mike

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

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2948 bytes --]

On Wed, Apr 29, 2015 at 04:05:49PM -0700, Michael Turquette wrote:
> > > Could you elaborate why you still want to have the clk-always-on in the
> > > device tree instead of in the kernel where it can be removed when
> > > necessary? What's your problem with enabling the critical clocks using
> > > clk_prepare_enable like other SoCs already do?
> > 
> > I've explained what my issues are already.  I'm not a fan of
> > hand-rolling and duplicating code which can be consolidated and make
> > generic.  Call me old fashioned. :)
> 
> I agree with Lee that the open code stuff isn't great, but I'm less and
> less convinced that the DT method is the way to go. Maxime has done a
> good job of reminding me why I always pushed back on this type of
> approach in the past.
> 
> Having a clock provider driver call clk_get, clk_prepare_enable on a clk
> is just fine. I think what needs to be done is to look at the platforms
> that open code this and find out how to replace this with some common
> code that any clock provider driver can call. Perhaps we can:
> 
> 1) use the struct clk_ops.init callback (which is used very little) and
> pass in a generic function to handle this case, or

I'm not sure that would work with clocks that provide several outputs,
and need only one (or at least not all of them) to be enabled.

Or we would need to pass some arguments to this function when we
register our clock.

> 2) we can create a new per-clk flag which is used by __clk_init to call
> clk_prepare_enable, or

That would work. It's what people usually expect from
CLK_IGNORE_UNUSED, and it's the first thing they try, so it would be
the easiest I guess.

> 3) we can add a new generic function like clk_register which sets any
> specified defaults (clk_set_defaults), but using C code and not DT

What kind of defaults are we talking about here? Just the clock state
or do you include boundaries, rate, etc. in it?

> I would need to look at the drivers that open code their
> clk_prepare_enable calls for non-Linux devices and see what similarities
> exist.

What really worked for us is to call clk_prepare_enable straight after
the call to clk_register.

Having a single place where we enable all the clocks causes a few
issues, mostly because we're not even sure the clock has been
registered at this time, and we do have to consider the clock name as
an ABI, which caused some issues for us in the past.

> But clearly the DT element of Lee's approach is causing some push
> back, so we should consider if there is a less controversial way to do
> this (and a way that benefits non-DT platforms as well).
> 
> I do think Lee's idea of consolidating around a single solution to a
> common problem is a great idea, but maybe not by using Devicetree.

Agreed.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 15686 bytes --]

On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote:
> On Wed, 22 Apr 2015, Maxime Ripard wrote:
> 
> > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote:
> > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > 
> > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > > 
> > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > > > > > +
> > > > > > > > > +		    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.
> > > > > > > > 
> > > > > > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > > > > > actually happen. Where do you place the cursor?
> > > > > > > 
> > > > > > > That's up to Mike.
> > > > > > 
> > > > > > Except that Mike won't review any of the DT changes, so he won't be
> > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a
> > > > > > mainline kernel.
> > > > > 
> > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if
> > > > > he isn't the DT guys are smart enough to either make the right
> > > > > decisions themselves (Rob has Acked these bindings already, so will be
> > > > > on the lookout for misuse, I'm sure), or ask for Mike's help.
> > > > 
> > > > Yeah, right, as if this strategy really worked in the past....
> > > > 
> > > > Do we really want to look at even the DT bindings that have actually
> > > > been reviewed by maintainers that got merged?
> > > > 
> > > > They don't have time for that, which is totally fine, but we really
> > > > should bury our head in the sand by actually thinking they will review
> > > > every single DT-related patch.
> > > > 
> > > > Using that as an argument is just plain denial of what really happened
> > > > for the past 4 years.
> > > 
> > > I agree that it's a problem, but this is a process problem and has
> > > nothing to do with this set.  If you have a problem with the current
> > > process and have a better alternative, submit your thoughts to the DT
> > > list.  Rejecting all new bindings because you are frightened that they
> > > will be used in a manner that they were not intended is not the way to
> > > go though.
> > 
> > I'm not saying that this binding should not go in because of a process
> > issue.
> > 
> > I'm saying that discarding arguments against your binding by adding
> > restrictions that cannot be enforced is not reasonable.
> 
> I'm open to constructive suggestions/alternatives.
> 
> Hand rolling this stuff in C per vendor is not of of them.

I'm sorry, but ruling out alternatives that work for everyone (and
actually work better) just because you don't want to edit a C file is
not really constructive either.

> > > > > > > > Should we create a new driver for our RAM controller, or do we want to
> > > > > > > > use clock-always-on?
> > > > > > > 
> > > > > > > I would say that if all the driver did was to enable clocks, then you
> > > > > > > should use this instead.  This binding was designed specifically for
> > > > > > > that purpose.
> > > > > > > 
> > > > > > > However, if the aforementioned driver clock can be safely gated, then
> > > > > > > it should not be an always-on clock.
> > > > > > 
> > > > > > Yeah, of course, I understand the original intent of it, but that
> > > > > > argument, which might very well be true at one point in time, might
> > > > > > not be true anymore two or three releases later.
> > > > > 
> > > > > Why?  The H/W isn't going to change in two or three releases.  The
> > > > > clocks designated as 'always-on' will have to be on forever, or
> > > > > synonymously, 'always'.
> > > > >
> > > > > > And that driver might actually rely on the fact that the clock is shut
> > > > > > down, which won't be the case.
> > > > > 
> > > > > I think you are missing the point of this binding.  The driver can
> > > > > never rely on that in this use-case.  If the clock is off, there is no
> > > > > device driver, period. 
> > > > 
> > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
> > > > PM guys will be happy to hear that.
> > > > 
> > > > And they are not device drivers, are not mandatory in the system, and
> > > > it's usually a good thing to keep the CPU running whenever you don't
> > > > have such drivers.
> > > > 
> > > > > > Introducing a DT interface solely by refering to the current state of
> > > > > > a driver is a bit odd.
> > > > > 
> > > > > I'm not sure I get your point.  This binding has nothing to do with
> > > > > drivers.
> > > > 
> > > > It's all about drivers. Or rather all about missing drivers.
> > > 
> > > I think you are going to have to be more forthcoming with your issues
> > > with this binding, because I'm struggling to understand what your
> > > problem with it is.  You have already pointed me to vendors which have
> > > a genuine/valid need for it.  But instead you'd prefer they hand-roll
> > > their own implementations over multiple lines of C code (each).
> > 
> > I told you already.
> > 
> > If you have that property, there's absolutely no way to do any kind of
> > clock management in the future.
> > 
> > It might be fine for your use case, but see my point about the
> > unreasonable restriction. People are going to use it for clocks that
> > just don't have a driver *yet*, and when that driver will be merged we
> > will end up with a driver that (for example) makes the assumption that
> > the clock has been shut down to reset the IP, that might or might not
> > be the case, depending on how old the DT is exactly.
> 
> There is a need for this binding,

That's your opinion. Several people already disagreed on this.

Now, what we probably need is a generic way to flag the clocks as
supposed to be enabled. The fact that this information should be in
the DT is a different story.

> but as you say, it must not be abused.  So how to we get people not
> to use it willy-nilly?
> 
> IMO, if people choose to ignore the stark warning in the documentation
> then that's they're lookout.  I guess you'd like to wrap them in more
> cotton wool than I would.  That's fine too, but how.

You've chosen to ignore all our warnings. Fine. Assume that other
people will ignore yours.

This is not about whatever you put in the documentation or checkpatch,
there will be people and/or maintainers that will go against that.

The only way to prevent any abuse of the binding, is not to have any
binding, really.

> > This will be even a bigger madness if you ask me.
> > 
> > > > > > > > Do we really want to enforce this if we ever gain a driver that would
> > > > > > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > > > > > driver yet?)
> > > > > > > 
> > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > > > > > should never be used. Situations where using always-on as a stop-gap
> > > > > > > due to a lack of current functionality is what the paragraph above is
> > > > > > > trying to mitigate.
> > > > > > 
> > > > > > But it's not really what this property is about. What this property
> > > > > > describes is that these clocks should never be gated. Any point in
> > > > > > time during the life of the system AND with in any kernel version.
> > > > > 
> > > > > You got it, that's correct -- these clocks should never be gated.
> > > > > 
> > > > > So why would that ever change?  If that is likely (or even possible)
> > > > > to change in the future then this binding should not be used.
> > > > >
> > > > > To reiterate; this binding should be used on ungatable clocks only.
> > > > > Non-negotiable, non-changeable either by the introduction of new
> > > > > functionality/support or kernel version.
> > > > 
> > > > I'm pretty sure that if that patch gets merged, by the end of the
> > > > year, there will be "incorrect" users by your standards.
> > > 
> > > It's possible to abuse any binding.  I don't see why you are so
> > > offended of this one in particular.
> > 
> > I'm not offended, I just tried to push the same kind of patches two
> > years ago, with Mike pushing back, and actually came to see that he
> > was right a few monthes down the road.
> 
> Well this was suggested by Mike.  I even have his Ack already.  So I
> guess he too has changed the error of his ways. :)

I wonder why it's not even merged yet then if you have the maintainers
Acked-by, and want to ignore any other review.

> > And yeah, your point that any binding can be abused is true. This one
> > is only so easy to abuse it's not even funny.
> > 
> > > > If you introduce a feature, you should expect people to use
> > > > it.  If not, what's the point?
> > > 
> > > By your own admission, there are genuine users for this binding and I
> > > expect people to use it.
> > 
> > The only thing that we really disagree upon is that whether that
> > restriction will really be followed. You expect people to, I
> > don't. It's the fundamental disagreement we have, that really prevent
> > any purely technical discussion.
> > 
> > Maybe we can try to address that before moving forward?
> 
> I'd like that.
> 
> If a firm warning isn't good enough, then what will be?
> 
> > > > > > > > Have you seen the numerous NAK on such approach Mike did?
> > > > > > > 
> > > > > > > I haven't, but the folks reviewing previous versions have.  Do you
> > > > > > > have something specific in mind that you'd like to bring to my
> > > > > > > attention?
> > > > > > 
> > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why
> > > > > > we ended up with clock protection code in various clock drivers
> > > > > > including:
> > > > > > 
> > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > > > > > 
> > > > > > Which is much more flexible, since you won't have to modify the DT to
> > > > > > change which clocks are to be left enabled, as well as way easier to
> > > > > > debug if you ever have to remove that property from the DT.
> > > > > 
> > > > > You're right, you don't have the change the DT in these cases.  You
> > > > > have to write new C code, which is _less_ flexible.
> > > > 
> > > > I'm sorry to learn that you never heard of that stable-DT thing.
> > > > 
> > > > And a bit sorry to see that a maintainer is really seeing C as not
> > > > flexible.
> > > 
> > > You're putting words in my mouth.  I didn't say C was not flexible.
> > > 
> > > I'm referencing the original DT pros i.e. it is possible to supply a
> > > different configuration without the need to compile the kernel.
> > > That's certainly true in this case.  We can provide a clk-provider and
> > > tag it as always-on, all without re-compile.
> > > 
> > > > > So all these platforms are adding their own hand-rolled version of
> > > > > this binding, adding more duplication and cruft to the kernel.
> > > > > Instead they can use this 'always-on' and we can consolidate and strip
> > > > > it all out.
> > > > 
> > > > Except that all these platforms are actually not implementing a
> > > > binding, ie not an interface with the DT they are bound to. Each and
> > > > every of these platforms can change that list whenever they wish, just
> > > > by sending a single one-liner patch (just like the DT, really.).
> > > > 
> > > > Which is not something that you can achieve with a DT binding.
> > > 
> > > Once again, can you give me more information about why you have such a
> > > problem with this binding.  I wish for it to be stable/ABI, I wish for
> > > it never to be removed, I envisage it will always be needed, so what's
> > > the problem? 
> > > 
> > > Do you have a vested interest that I am missing?
> > > 
> > > Perhaps an example of possible calamity will help convince me that
> > > you're not completely wrong and blowing everything out of proportion
> > > for no good reason.
> > 
> > Let's say you've introduced such a clock in kernel 4.0 for the memory
> > clock.
> > 
> > At some point down the road, you create a ddrfreq driver (if that ever
> > exists). You have a new driver, which will manage the clock.
> > 
> > In that driver, for some reason, you have to shutdown the clock to
> > reset the DDR controller. Of course that also means that you will be
> > removing the clk-always-on property from your DT.
> > 
> > You will have in your driver something like:
> > 
> > /* Reset our controller */
> > clk_disable(clk);
> > clk_enable(clk):
> > 
> > And then, you expect your controller to be in its out-of-reset
> > state. Which will be the case with a new DT, and not with the old one,
> > probably creating all kind of very entertaining issues to debug.
> > 
> > All of this wouldn't be the case if you had this inside the kernel,
> > since (hopefully) the kernel is consistent with itself.
> 
> Surely you must have realised already that DTBs are more tightly
> coupled to kernel versions than we would have initially liked?

Just to get things straight. I'm *not* one proponent of the "DT as an
ABI" rule.

BUT, if you're designing a generic property, then you're also
designing it for platforms that have chose to have that stability. And
we do have some of those in the tree.

And of course, that's assuming that the ABI stability is never going
to be a thing (which might or might not happen).

> It's naive to assume that old DTBs will 'just-work' with newer
> kernels.

Like I said, it does work on some mainline ARM platforms.

> Wrong decisions related to DT are being made daily. Adding
> mistakenly or foolhardily adding 'clk-always-on' to a DTS is not
> going to be the sole cause of breakage somewhere down the line.

I'm not sure that saying that accepting bindings because some other
bindings are just as bad is a good argument, especially on bindings
that do impact all the platforms.

> Pushing back on the acceptance of this binding based on idealistic,
> possibly already out-of-date premise is just frustrating.

Don't worry, it's just as frustrating on the other end. Showing you
exactly why it's going to be an issue with you simply ignoring by
saying something close to "not my use case" is just as frustrating.

> This useful binding should be accepted and people should not abuse
> it.  If they do and the vendor Maintainer's review and accept then
> they have no foundation for recourse.

And in the end, there will be more and more bloated and / or poor code
in the kernel, hurting it as a whole.

> Would you prefer it if I made the warning starker?

The only kind of warning that would be noticed by anyone is a runtime
warning. I'm not sure this is a reasonable option :)

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Lee Jones]

On Wed, 29 Apr 2015, Maxime Ripard wrote:

> On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote:
> > On Wed, 22 Apr 2015, Maxime Ripard wrote:
> > 
> > > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote:
> > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > 
> > > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > > > 
> > > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > > > > > > +
> > > > > > > > > > +		    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.
> > > > > > > > > 
> > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > > > > > > actually happen. Where do you place the cursor?
> > > > > > > > 
> > > > > > > > That's up to Mike.
> > > > > > > 
> > > > > > > Except that Mike won't review any of the DT changes, so he won't be
> > > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a
> > > > > > > mainline kernel.
> > > > > > 
> > > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if
> > > > > > he isn't the DT guys are smart enough to either make the right
> > > > > > decisions themselves (Rob has Acked these bindings already, so will be
> > > > > > on the lookout for misuse, I'm sure), or ask for Mike's help.
> > > > > 
> > > > > Yeah, right, as if this strategy really worked in the past....
> > > > > 
> > > > > Do we really want to look at even the DT bindings that have actually
> > > > > been reviewed by maintainers that got merged?
> > > > > 
> > > > > They don't have time for that, which is totally fine, but we really
> > > > > should bury our head in the sand by actually thinking they will review
> > > > > every single DT-related patch.
> > > > > 
> > > > > Using that as an argument is just plain denial of what really happened
> > > > > for the past 4 years.
> > > > 
> > > > I agree that it's a problem, but this is a process problem and has
> > > > nothing to do with this set.  If you have a problem with the current
> > > > process and have a better alternative, submit your thoughts to the DT
> > > > list.  Rejecting all new bindings because you are frightened that they
> > > > will be used in a manner that they were not intended is not the way to
> > > > go though.
> > > 
> > > I'm not saying that this binding should not go in because of a process
> > > issue.
> > > 
> > > I'm saying that discarding arguments against your binding by adding
> > > restrictions that cannot be enforced is not reasonable.
> > 
> > I'm open to constructive suggestions/alternatives.
> > 
> > Hand rolling this stuff in C per vendor is not of of them.
> 
> I'm sorry, but ruling out alternatives that work for everyone (and
> actually work better) just because you don't want to edit a C file is
> not really constructive either.
> 
> > > > > > > > > Should we create a new driver for our RAM controller, or do we want to
> > > > > > > > > use clock-always-on?
> > > > > > > > 
> > > > > > > > I would say that if all the driver did was to enable clocks, then you
> > > > > > > > should use this instead.  This binding was designed specifically for
> > > > > > > > that purpose.
> > > > > > > > 
> > > > > > > > However, if the aforementioned driver clock can be safely gated, then
> > > > > > > > it should not be an always-on clock.
> > > > > > > 
> > > > > > > Yeah, of course, I understand the original intent of it, but that
> > > > > > > argument, which might very well be true at one point in time, might
> > > > > > > not be true anymore two or three releases later.
> > > > > > 
> > > > > > Why?  The H/W isn't going to change in two or three releases.  The
> > > > > > clocks designated as 'always-on' will have to be on forever, or
> > > > > > synonymously, 'always'.
> > > > > >
> > > > > > > And that driver might actually rely on the fact that the clock is shut
> > > > > > > down, which won't be the case.
> > > > > > 
> > > > > > I think you are missing the point of this binding.  The driver can
> > > > > > never rely on that in this use-case.  If the clock is off, there is no
> > > > > > device driver, period. 
> > > > > 
> > > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
> > > > > PM guys will be happy to hear that.
> > > > > 
> > > > > And they are not device drivers, are not mandatory in the system, and
> > > > > it's usually a good thing to keep the CPU running whenever you don't
> > > > > have such drivers.
> > > > > 
> > > > > > > Introducing a DT interface solely by refering to the current state of
> > > > > > > a driver is a bit odd.
> > > > > > 
> > > > > > I'm not sure I get your point.  This binding has nothing to do with
> > > > > > drivers.
> > > > > 
> > > > > It's all about drivers. Or rather all about missing drivers.
> > > > 
> > > > I think you are going to have to be more forthcoming with your issues
> > > > with this binding, because I'm struggling to understand what your
> > > > problem with it is.  You have already pointed me to vendors which have
> > > > a genuine/valid need for it.  But instead you'd prefer they hand-roll
> > > > their own implementations over multiple lines of C code (each).
> > > 
> > > I told you already.
> > > 
> > > If you have that property, there's absolutely no way to do any kind of
> > > clock management in the future.
> > > 
> > > It might be fine for your use case, but see my point about the
> > > unreasonable restriction. People are going to use it for clocks that
> > > just don't have a driver *yet*, and when that driver will be merged we
> > > will end up with a driver that (for example) makes the assumption that
> > > the clock has been shut down to reset the IP, that might or might not
> > > be the case, depending on how old the DT is exactly.
> > 
> > There is a need for this binding,
> 
> That's your opinion. Several people already disagreed on this.
> 
> Now, what we probably need is a generic way to flag the clocks as
> supposed to be enabled. The fact that this information should be in
> the DT is a different story.
> 
> > but as you say, it must not be abused.  So how to we get people not
> > to use it willy-nilly?
> > 
> > IMO, if people choose to ignore the stark warning in the documentation
> > then that's they're lookout.  I guess you'd like to wrap them in more
> > cotton wool than I would.  That's fine too, but how.
> 
> You've chosen to ignore all our warnings. Fine. Assume that other
> people will ignore yours.
> 
> This is not about whatever you put in the documentation or checkpatch,
> there will be people and/or maintainers that will go against that.
> 
> The only way to prevent any abuse of the binding, is not to have any
> binding, really.
> 
> > > This will be even a bigger madness if you ask me.
> > > 
> > > > > > > > > Do we really want to enforce this if we ever gain a driver that would
> > > > > > > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > > > > > > driver yet?)
> > > > > > > > 
> > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > > > > > > should never be used. Situations where using always-on as a stop-gap
> > > > > > > > due to a lack of current functionality is what the paragraph above is
> > > > > > > > trying to mitigate.
> > > > > > > 
> > > > > > > But it's not really what this property is about. What this property
> > > > > > > describes is that these clocks should never be gated. Any point in
> > > > > > > time during the life of the system AND with in any kernel version.
> > > > > > 
> > > > > > You got it, that's correct -- these clocks should never be gated.
> > > > > > 
> > > > > > So why would that ever change?  If that is likely (or even possible)
> > > > > > to change in the future then this binding should not be used.
> > > > > >
> > > > > > To reiterate; this binding should be used on ungatable clocks only.
> > > > > > Non-negotiable, non-changeable either by the introduction of new
> > > > > > functionality/support or kernel version.
> > > > > 
> > > > > I'm pretty sure that if that patch gets merged, by the end of the
> > > > > year, there will be "incorrect" users by your standards.
> > > > 
> > > > It's possible to abuse any binding.  I don't see why you are so
> > > > offended of this one in particular.
> > > 
> > > I'm not offended, I just tried to push the same kind of patches two
> > > years ago, with Mike pushing back, and actually came to see that he
> > > was right a few monthes down the road.
> > 
> > Well this was suggested by Mike.  I even have his Ack already.  So I
> > guess he too has changed the error of his ways. :)
> 
> I wonder why it's not even merged yet then if you have the maintainers
> Acked-by, and want to ignore any other review.
> 
> > > And yeah, your point that any binding can be abused is true. This one
> > > is only so easy to abuse it's not even funny.
> > > 
> > > > > If you introduce a feature, you should expect people to use
> > > > > it.  If not, what's the point?
> > > > 
> > > > By your own admission, there are genuine users for this binding and I
> > > > expect people to use it.
> > > 
> > > The only thing that we really disagree upon is that whether that
> > > restriction will really be followed. You expect people to, I
> > > don't. It's the fundamental disagreement we have, that really prevent
> > > any purely technical discussion.
> > > 
> > > Maybe we can try to address that before moving forward?
> > 
> > I'd like that.
> > 
> > If a firm warning isn't good enough, then what will be?
> > 
> > > > > > > > > Have you seen the numerous NAK on such approach Mike did?
> > > > > > > > 
> > > > > > > > I haven't, but the folks reviewing previous versions have.  Do you
> > > > > > > > have something specific in mind that you'd like to bring to my
> > > > > > > > attention?
> > > > > > > 
> > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why
> > > > > > > we ended up with clock protection code in various clock drivers
> > > > > > > including:
> > > > > > > 
> > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > > > > > > 
> > > > > > > Which is much more flexible, since you won't have to modify the DT to
> > > > > > > change which clocks are to be left enabled, as well as way easier to
> > > > > > > debug if you ever have to remove that property from the DT.
> > > > > > 
> > > > > > You're right, you don't have the change the DT in these cases.  You
> > > > > > have to write new C code, which is _less_ flexible.
> > > > > 
> > > > > I'm sorry to learn that you never heard of that stable-DT thing.
> > > > > 
> > > > > And a bit sorry to see that a maintainer is really seeing C as not
> > > > > flexible.
> > > > 
> > > > You're putting words in my mouth.  I didn't say C was not flexible.
> > > > 
> > > > I'm referencing the original DT pros i.e. it is possible to supply a
> > > > different configuration without the need to compile the kernel.
> > > > That's certainly true in this case.  We can provide a clk-provider and
> > > > tag it as always-on, all without re-compile.
> > > > 
> > > > > > So all these platforms are adding their own hand-rolled version of
> > > > > > this binding, adding more duplication and cruft to the kernel.
> > > > > > Instead they can use this 'always-on' and we can consolidate and strip
> > > > > > it all out.
> > > > > 
> > > > > Except that all these platforms are actually not implementing a
> > > > > binding, ie not an interface with the DT they are bound to. Each and
> > > > > every of these platforms can change that list whenever they wish, just
> > > > > by sending a single one-liner patch (just like the DT, really.).
> > > > > 
> > > > > Which is not something that you can achieve with a DT binding.
> > > > 
> > > > Once again, can you give me more information about why you have such a
> > > > problem with this binding.  I wish for it to be stable/ABI, I wish for
> > > > it never to be removed, I envisage it will always be needed, so what's
> > > > the problem? 
> > > > 
> > > > Do you have a vested interest that I am missing?
> > > > 
> > > > Perhaps an example of possible calamity will help convince me that
> > > > you're not completely wrong and blowing everything out of proportion
> > > > for no good reason.
> > > 
> > > Let's say you've introduced such a clock in kernel 4.0 for the memory
> > > clock.
> > > 
> > > At some point down the road, you create a ddrfreq driver (if that ever
> > > exists). You have a new driver, which will manage the clock.
> > > 
> > > In that driver, for some reason, you have to shutdown the clock to
> > > reset the DDR controller. Of course that also means that you will be
> > > removing the clk-always-on property from your DT.
> > > 
> > > You will have in your driver something like:
> > > 
> > > /* Reset our controller */
> > > clk_disable(clk);
> > > clk_enable(clk):
> > > 
> > > And then, you expect your controller to be in its out-of-reset
> > > state. Which will be the case with a new DT, and not with the old one,
> > > probably creating all kind of very entertaining issues to debug.
> > > 
> > > All of this wouldn't be the case if you had this inside the kernel,
> > > since (hopefully) the kernel is consistent with itself.
> > 
> > Surely you must have realised already that DTBs are more tightly
> > coupled to kernel versions than we would have initially liked?
> 
> Just to get things straight. I'm *not* one proponent of the "DT as an
> ABI" rule.
> 
> BUT, if you're designing a generic property, then you're also
> designing it for platforms that have chose to have that stability. And
> we do have some of those in the tree.
> 
> And of course, that's assuming that the ABI stability is never going
> to be a thing (which might or might not happen).
> 
> > It's naive to assume that old DTBs will 'just-work' with newer
> > kernels.
> 
> Like I said, it does work on some mainline ARM platforms.
> 
> > Wrong decisions related to DT are being made daily. Adding
> > mistakenly or foolhardily adding 'clk-always-on' to a DTS is not
> > going to be the sole cause of breakage somewhere down the line.
> 
> I'm not sure that saying that accepting bindings because some other
> bindings are just as bad is a good argument, especially on bindings
> that do impact all the platforms.
> 
> > Pushing back on the acceptance of this binding based on idealistic,
> > possibly already out-of-date premise is just frustrating.
> 
> Don't worry, it's just as frustrating on the other end. Showing you
> exactly why it's going to be an issue with you simply ignoring by
> saying something close to "not my use case" is just as frustrating.
> 
> > This useful binding should be accepted and people should not abuse
> > it.  If they do and the vendor Maintainer's review and accept then
> > they have no foundation for recourse.
> 
> And in the end, there will be more and more bloated and / or poor code
> in the kernel, hurting it as a whole.
> 
> > Would you prefer it if I made the warning starker?
> 
> The only kind of warning that would be noticed by anyone is a runtime
> warning. I'm not sure this is a reasonable option :)

Does Sascha's antidote patch change your opinion?  We can use DT to
declare critical clocks, and in the rare case of the introduction of a
new DDRFreq-like feature, which doesn't adapt the DT will still be
able to unlock the criticalness of the clock and use it as expected?

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

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Sascha Hauer]

On Thu, Apr 30, 2015 at 10:57:22AM +0100, Lee Jones wrote:
> On Wed, 29 Apr 2015, Maxime Ripard wrote:
> 
> > On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote:
> > > On Wed, 22 Apr 2015, Maxime Ripard wrote:
> > > 
> > > > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote:
> > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > > 
> > > > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> > > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > > > > 
> > > > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > > > > > > > +
> > > > > > > > > > > +		    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.
> > > > > > > > > > 
> > > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > > > > > > > actually happen. Where do you place the cursor?
> > > > > > > > > 
> > > > > > > > > That's up to Mike.
> > > > > > > > 
> > > > > > > > Except that Mike won't review any of the DT changes, so he won't be
> > > > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a
> > > > > > > > mainline kernel.
> > > > > > > 
> > > > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if
> > > > > > > he isn't the DT guys are smart enough to either make the right
> > > > > > > decisions themselves (Rob has Acked these bindings already, so will be
> > > > > > > on the lookout for misuse, I'm sure), or ask for Mike's help.
> > > > > > 
> > > > > > Yeah, right, as if this strategy really worked in the past....
> > > > > > 
> > > > > > Do we really want to look at even the DT bindings that have actually
> > > > > > been reviewed by maintainers that got merged?
> > > > > > 
> > > > > > They don't have time for that, which is totally fine, but we really
> > > > > > should bury our head in the sand by actually thinking they will review
> > > > > > every single DT-related patch.
> > > > > > 
> > > > > > Using that as an argument is just plain denial of what really happened
> > > > > > for the past 4 years.
> > > > > 
> > > > > I agree that it's a problem, but this is a process problem and has
> > > > > nothing to do with this set.  If you have a problem with the current
> > > > > process and have a better alternative, submit your thoughts to the DT
> > > > > list.  Rejecting all new bindings because you are frightened that they
> > > > > will be used in a manner that they were not intended is not the way to
> > > > > go though.
> > > > 
> > > > I'm not saying that this binding should not go in because of a process
> > > > issue.
> > > > 
> > > > I'm saying that discarding arguments against your binding by adding
> > > > restrictions that cannot be enforced is not reasonable.
> > > 
> > > I'm open to constructive suggestions/alternatives.
> > > 
> > > Hand rolling this stuff in C per vendor is not of of them.
> > 
> > I'm sorry, but ruling out alternatives that work for everyone (and
> > actually work better) just because you don't want to edit a C file is
> > not really constructive either.
> > 
> > > > > > > > > > Should we create a new driver for our RAM controller, or do we want to
> > > > > > > > > > use clock-always-on?
> > > > > > > > > 
> > > > > > > > > I would say that if all the driver did was to enable clocks, then you
> > > > > > > > > should use this instead.  This binding was designed specifically for
> > > > > > > > > that purpose.
> > > > > > > > > 
> > > > > > > > > However, if the aforementioned driver clock can be safely gated, then
> > > > > > > > > it should not be an always-on clock.
> > > > > > > > 
> > > > > > > > Yeah, of course, I understand the original intent of it, but that
> > > > > > > > argument, which might very well be true at one point in time, might
> > > > > > > > not be true anymore two or three releases later.
> > > > > > > 
> > > > > > > Why?  The H/W isn't going to change in two or three releases.  The
> > > > > > > clocks designated as 'always-on' will have to be on forever, or
> > > > > > > synonymously, 'always'.
> > > > > > >
> > > > > > > > And that driver might actually rely on the fact that the clock is shut
> > > > > > > > down, which won't be the case.
> > > > > > > 
> > > > > > > I think you are missing the point of this binding.  The driver can
> > > > > > > never rely on that in this use-case.  If the clock is off, there is no
> > > > > > > device driver, period. 
> > > > > > 
> > > > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
> > > > > > PM guys will be happy to hear that.
> > > > > > 
> > > > > > And they are not device drivers, are not mandatory in the system, and
> > > > > > it's usually a good thing to keep the CPU running whenever you don't
> > > > > > have such drivers.
> > > > > > 
> > > > > > > > Introducing a DT interface solely by refering to the current state of
> > > > > > > > a driver is a bit odd.
> > > > > > > 
> > > > > > > I'm not sure I get your point.  This binding has nothing to do with
> > > > > > > drivers.
> > > > > > 
> > > > > > It's all about drivers. Or rather all about missing drivers.
> > > > > 
> > > > > I think you are going to have to be more forthcoming with your issues
> > > > > with this binding, because I'm struggling to understand what your
> > > > > problem with it is.  You have already pointed me to vendors which have
> > > > > a genuine/valid need for it.  But instead you'd prefer they hand-roll
> > > > > their own implementations over multiple lines of C code (each).
> > > > 
> > > > I told you already.
> > > > 
> > > > If you have that property, there's absolutely no way to do any kind of
> > > > clock management in the future.
> > > > 
> > > > It might be fine for your use case, but see my point about the
> > > > unreasonable restriction. People are going to use it for clocks that
> > > > just don't have a driver *yet*, and when that driver will be merged we
> > > > will end up with a driver that (for example) makes the assumption that
> > > > the clock has been shut down to reset the IP, that might or might not
> > > > be the case, depending on how old the DT is exactly.
> > > 
> > > There is a need for this binding,
> > 
> > That's your opinion. Several people already disagreed on this.
> > 
> > Now, what we probably need is a generic way to flag the clocks as
> > supposed to be enabled. The fact that this information should be in
> > the DT is a different story.
> > 
> > > but as you say, it must not be abused.  So how to we get people not
> > > to use it willy-nilly?
> > > 
> > > IMO, if people choose to ignore the stark warning in the documentation
> > > then that's they're lookout.  I guess you'd like to wrap them in more
> > > cotton wool than I would.  That's fine too, but how.
> > 
> > You've chosen to ignore all our warnings. Fine. Assume that other
> > people will ignore yours.
> > 
> > This is not about whatever you put in the documentation or checkpatch,
> > there will be people and/or maintainers that will go against that.
> > 
> > The only way to prevent any abuse of the binding, is not to have any
> > binding, really.
> > 
> > > > This will be even a bigger madness if you ask me.
> > > > 
> > > > > > > > > > Do we really want to enforce this if we ever gain a driver that would
> > > > > > > > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > > > > > > > driver yet?)
> > > > > > > > > 
> > > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > > > > > > > should never be used. Situations where using always-on as a stop-gap
> > > > > > > > > due to a lack of current functionality is what the paragraph above is
> > > > > > > > > trying to mitigate.
> > > > > > > > 
> > > > > > > > But it's not really what this property is about. What this property
> > > > > > > > describes is that these clocks should never be gated. Any point in
> > > > > > > > time during the life of the system AND with in any kernel version.
> > > > > > > 
> > > > > > > You got it, that's correct -- these clocks should never be gated.
> > > > > > > 
> > > > > > > So why would that ever change?  If that is likely (or even possible)
> > > > > > > to change in the future then this binding should not be used.
> > > > > > >
> > > > > > > To reiterate; this binding should be used on ungatable clocks only.
> > > > > > > Non-negotiable, non-changeable either by the introduction of new
> > > > > > > functionality/support or kernel version.
> > > > > > 
> > > > > > I'm pretty sure that if that patch gets merged, by the end of the
> > > > > > year, there will be "incorrect" users by your standards.
> > > > > 
> > > > > It's possible to abuse any binding.  I don't see why you are so
> > > > > offended of this one in particular.
> > > > 
> > > > I'm not offended, I just tried to push the same kind of patches two
> > > > years ago, with Mike pushing back, and actually came to see that he
> > > > was right a few monthes down the road.
> > > 
> > > Well this was suggested by Mike.  I even have his Ack already.  So I
> > > guess he too has changed the error of his ways. :)
> > 
> > I wonder why it's not even merged yet then if you have the maintainers
> > Acked-by, and want to ignore any other review.
> > 
> > > > And yeah, your point that any binding can be abused is true. This one
> > > > is only so easy to abuse it's not even funny.
> > > > 
> > > > > > If you introduce a feature, you should expect people to use
> > > > > > it.  If not, what's the point?
> > > > > 
> > > > > By your own admission, there are genuine users for this binding and I
> > > > > expect people to use it.
> > > > 
> > > > The only thing that we really disagree upon is that whether that
> > > > restriction will really be followed. You expect people to, I
> > > > don't. It's the fundamental disagreement we have, that really prevent
> > > > any purely technical discussion.
> > > > 
> > > > Maybe we can try to address that before moving forward?
> > > 
> > > I'd like that.
> > > 
> > > If a firm warning isn't good enough, then what will be?
> > > 
> > > > > > > > > > Have you seen the numerous NAK on such approach Mike did?
> > > > > > > > > 
> > > > > > > > > I haven't, but the folks reviewing previous versions have.  Do you
> > > > > > > > > have something specific in mind that you'd like to bring to my
> > > > > > > > > attention?
> > > > > > > > 
> > > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why
> > > > > > > > we ended up with clock protection code in various clock drivers
> > > > > > > > including:
> > > > > > > > 
> > > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > > > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > > > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > > > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > > > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > > > > > > > 
> > > > > > > > Which is much more flexible, since you won't have to modify the DT to
> > > > > > > > change which clocks are to be left enabled, as well as way easier to
> > > > > > > > debug if you ever have to remove that property from the DT.
> > > > > > > 
> > > > > > > You're right, you don't have the change the DT in these cases.  You
> > > > > > > have to write new C code, which is _less_ flexible.
> > > > > > 
> > > > > > I'm sorry to learn that you never heard of that stable-DT thing.
> > > > > > 
> > > > > > And a bit sorry to see that a maintainer is really seeing C as not
> > > > > > flexible.
> > > > > 
> > > > > You're putting words in my mouth.  I didn't say C was not flexible.
> > > > > 
> > > > > I'm referencing the original DT pros i.e. it is possible to supply a
> > > > > different configuration without the need to compile the kernel.
> > > > > That's certainly true in this case.  We can provide a clk-provider and
> > > > > tag it as always-on, all without re-compile.
> > > > > 
> > > > > > > So all these platforms are adding their own hand-rolled version of
> > > > > > > this binding, adding more duplication and cruft to the kernel.
> > > > > > > Instead they can use this 'always-on' and we can consolidate and strip
> > > > > > > it all out.
> > > > > > 
> > > > > > Except that all these platforms are actually not implementing a
> > > > > > binding, ie not an interface with the DT they are bound to. Each and
> > > > > > every of these platforms can change that list whenever they wish, just
> > > > > > by sending a single one-liner patch (just like the DT, really.).
> > > > > > 
> > > > > > Which is not something that you can achieve with a DT binding.
> > > > > 
> > > > > Once again, can you give me more information about why you have such a
> > > > > problem with this binding.  I wish for it to be stable/ABI, I wish for
> > > > > it never to be removed, I envisage it will always be needed, so what's
> > > > > the problem? 
> > > > > 
> > > > > Do you have a vested interest that I am missing?
> > > > > 
> > > > > Perhaps an example of possible calamity will help convince me that
> > > > > you're not completely wrong and blowing everything out of proportion
> > > > > for no good reason.
> > > > 
> > > > Let's say you've introduced such a clock in kernel 4.0 for the memory
> > > > clock.
> > > > 
> > > > At some point down the road, you create a ddrfreq driver (if that ever
> > > > exists). You have a new driver, which will manage the clock.
> > > > 
> > > > In that driver, for some reason, you have to shutdown the clock to
> > > > reset the DDR controller. Of course that also means that you will be
> > > > removing the clk-always-on property from your DT.
> > > > 
> > > > You will have in your driver something like:
> > > > 
> > > > /* Reset our controller */
> > > > clk_disable(clk);
> > > > clk_enable(clk):
> > > > 
> > > > And then, you expect your controller to be in its out-of-reset
> > > > state. Which will be the case with a new DT, and not with the old one,
> > > > probably creating all kind of very entertaining issues to debug.
> > > > 
> > > > All of this wouldn't be the case if you had this inside the kernel,
> > > > since (hopefully) the kernel is consistent with itself.
> > > 
> > > Surely you must have realised already that DTBs are more tightly
> > > coupled to kernel versions than we would have initially liked?
> > 
> > Just to get things straight. I'm *not* one proponent of the "DT as an
> > ABI" rule.
> > 
> > BUT, if you're designing a generic property, then you're also
> > designing it for platforms that have chose to have that stability. And
> > we do have some of those in the tree.
> > 
> > And of course, that's assuming that the ABI stability is never going
> > to be a thing (which might or might not happen).
> > 
> > > It's naive to assume that old DTBs will 'just-work' with newer
> > > kernels.
> > 
> > Like I said, it does work on some mainline ARM platforms.
> > 
> > > Wrong decisions related to DT are being made daily. Adding
> > > mistakenly or foolhardily adding 'clk-always-on' to a DTS is not
> > > going to be the sole cause of breakage somewhere down the line.
> > 
> > I'm not sure that saying that accepting bindings because some other
> > bindings are just as bad is a good argument, especially on bindings
> > that do impact all the platforms.
> > 
> > > Pushing back on the acceptance of this binding based on idealistic,
> > > possibly already out-of-date premise is just frustrating.
> > 
> > Don't worry, it's just as frustrating on the other end. Showing you
> > exactly why it's going to be an issue with you simply ignoring by
> > saying something close to "not my use case" is just as frustrating.
> > 
> > > This useful binding should be accepted and people should not abuse
> > > it.  If they do and the vendor Maintainer's review and accept then
> > > they have no foundation for recourse.
> > 
> > And in the end, there will be more and more bloated and / or poor code
> > in the kernel, hurting it as a whole.
> > 
> > > Would you prefer it if I made the warning starker?
> > 
> > The only kind of warning that would be noticed by anyone is a runtime
> > warning. I'm not sure this is a reasonable option :)
> 
> Does Sascha's antidote patch change your opinion?  We can use DT to
> declare critical clocks, and in the rare case of the introduction of a
> new DDRFreq-like feature, which doesn't adapt the DT will still be
> able to unlock the criticalness of the clock and use it as expected?

Honestly I'm not very fond of declaring these in the device tree, but
naming them 'critical-clocks' rather than 'always-on' seems more
acceptable for me. It leaves a way out for the user to turn the clock
off later as it only means "you may turn them off when you know what you
are doing".

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Lee Jones]

On Fri, 01 May 2015, Sascha Hauer wrote:
> On Thu, Apr 30, 2015 at 10:57:22AM +0100, Lee Jones wrote:
> > On Wed, 29 Apr 2015, Maxime Ripard wrote:
> > 
> > > On Wed, Apr 29, 2015 at 03:17:51PM +0100, Lee Jones wrote:
> > > > On Wed, 22 Apr 2015, Maxime Ripard wrote:
> > > > 
> > > > > On Wed, Apr 08, 2015 at 06:23:44PM +0100, Lee Jones wrote:
> > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > > > 
> > > > > > > On Wed, Apr 08, 2015 at 11:38:32AM +0100, Lee Jones wrote:
> > > > > > > > On Wed, 08 Apr 2015, Maxime Ripard wrote:
> > > > > > > > 
> > > > > > > > > On Wed, Apr 08, 2015 at 09:14:50AM +0100, Lee Jones wrote:
> > > > > > > > > > > > +
> > > > > > > > > > > > +		    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.
> > > > > > > > > > > 
> > > > > > > > > > > Disregarding what's stated here, I'm pretty sure that this will
> > > > > > > > > > > actually happen. Where do you place the cursor?
> > > > > > > > > > 
> > > > > > > > > > That's up to Mike.
> > > > > > > > > 
> > > > > > > > > Except that Mike won't review any of the DT changes, so he won't be
> > > > > > > > > able to refrain users from using it. Let alone out-of-tree DTs using a
> > > > > > > > > mainline kernel.
> > > > > > > > 
> > > > > > > > Ideally Mike should be Cc'ed on patches using clock bindings, but if
> > > > > > > > he isn't the DT guys are smart enough to either make the right
> > > > > > > > decisions themselves (Rob has Acked these bindings already, so will be
> > > > > > > > on the lookout for misuse, I'm sure), or ask for Mike's help.
> > > > > > > 
> > > > > > > Yeah, right, as if this strategy really worked in the past....
> > > > > > > 
> > > > > > > Do we really want to look at even the DT bindings that have actually
> > > > > > > been reviewed by maintainers that got merged?
> > > > > > > 
> > > > > > > They don't have time for that, which is totally fine, but we really
> > > > > > > should bury our head in the sand by actually thinking they will review
> > > > > > > every single DT-related patch.
> > > > > > > 
> > > > > > > Using that as an argument is just plain denial of what really happened
> > > > > > > for the past 4 years.
> > > > > > 
> > > > > > I agree that it's a problem, but this is a process problem and has
> > > > > > nothing to do with this set.  If you have a problem with the current
> > > > > > process and have a better alternative, submit your thoughts to the DT
> > > > > > list.  Rejecting all new bindings because you are frightened that they
> > > > > > will be used in a manner that they were not intended is not the way to
> > > > > > go though.
> > > > > 
> > > > > I'm not saying that this binding should not go in because of a process
> > > > > issue.
> > > > > 
> > > > > I'm saying that discarding arguments against your binding by adding
> > > > > restrictions that cannot be enforced is not reasonable.
> > > > 
> > > > I'm open to constructive suggestions/alternatives.
> > > > 
> > > > Hand rolling this stuff in C per vendor is not of of them.
> > > 
> > > I'm sorry, but ruling out alternatives that work for everyone (and
> > > actually work better) just because you don't want to edit a C file is
> > > not really constructive either.
> > > 
> > > > > > > > > > > Should we create a new driver for our RAM controller, or do we want to
> > > > > > > > > > > use clock-always-on?
> > > > > > > > > > 
> > > > > > > > > > I would say that if all the driver did was to enable clocks, then you
> > > > > > > > > > should use this instead.  This binding was designed specifically for
> > > > > > > > > > that purpose.
> > > > > > > > > > 
> > > > > > > > > > However, if the aforementioned driver clock can be safely gated, then
> > > > > > > > > > it should not be an always-on clock.
> > > > > > > > > 
> > > > > > > > > Yeah, of course, I understand the original intent of it, but that
> > > > > > > > > argument, which might very well be true at one point in time, might
> > > > > > > > > not be true anymore two or three releases later.
> > > > > > > > 
> > > > > > > > Why?  The H/W isn't going to change in two or three releases.  The
> > > > > > > > clocks designated as 'always-on' will have to be on forever, or
> > > > > > > > synonymously, 'always'.
> > > > > > > >
> > > > > > > > > And that driver might actually rely on the fact that the clock is shut
> > > > > > > > > down, which won't be the case.
> > > > > > > > 
> > > > > > > > I think you are missing the point of this binding.  The driver can
> > > > > > > > never rely on that in this use-case.  If the clock is off, there is no
> > > > > > > > device driver, period. 
> > > > > > > 
> > > > > > > Ok. So CPU hotplug or cpuidle is not a thing then? I'm pretty sure the
> > > > > > > PM guys will be happy to hear that.
> > > > > > > 
> > > > > > > And they are not device drivers, are not mandatory in the system, and
> > > > > > > it's usually a good thing to keep the CPU running whenever you don't
> > > > > > > have such drivers.
> > > > > > > 
> > > > > > > > > Introducing a DT interface solely by refering to the current state of
> > > > > > > > > a driver is a bit odd.
> > > > > > > > 
> > > > > > > > I'm not sure I get your point.  This binding has nothing to do with
> > > > > > > > drivers.
> > > > > > > 
> > > > > > > It's all about drivers. Or rather all about missing drivers.
> > > > > > 
> > > > > > I think you are going to have to be more forthcoming with your issues
> > > > > > with this binding, because I'm struggling to understand what your
> > > > > > problem with it is.  You have already pointed me to vendors which have
> > > > > > a genuine/valid need for it.  But instead you'd prefer they hand-roll
> > > > > > their own implementations over multiple lines of C code (each).
> > > > > 
> > > > > I told you already.
> > > > > 
> > > > > If you have that property, there's absolutely no way to do any kind of
> > > > > clock management in the future.
> > > > > 
> > > > > It might be fine for your use case, but see my point about the
> > > > > unreasonable restriction. People are going to use it for clocks that
> > > > > just don't have a driver *yet*, and when that driver will be merged we
> > > > > will end up with a driver that (for example) makes the assumption that
> > > > > the clock has been shut down to reset the IP, that might or might not
> > > > > be the case, depending on how old the DT is exactly.
> > > > 
> > > > There is a need for this binding,
> > > 
> > > That's your opinion. Several people already disagreed on this.
> > > 
> > > Now, what we probably need is a generic way to flag the clocks as
> > > supposed to be enabled. The fact that this information should be in
> > > the DT is a different story.
> > > 
> > > > but as you say, it must not be abused.  So how to we get people not
> > > > to use it willy-nilly?
> > > > 
> > > > IMO, if people choose to ignore the stark warning in the documentation
> > > > then that's they're lookout.  I guess you'd like to wrap them in more
> > > > cotton wool than I would.  That's fine too, but how.
> > > 
> > > You've chosen to ignore all our warnings. Fine. Assume that other
> > > people will ignore yours.
> > > 
> > > This is not about whatever you put in the documentation or checkpatch,
> > > there will be people and/or maintainers that will go against that.
> > > 
> > > The only way to prevent any abuse of the binding, is not to have any
> > > binding, really.
> > > 
> > > > > This will be even a bigger madness if you ask me.
> > > > > 
> > > > > > > > > > > Do we really want to enforce this if we ever gain a driver that would
> > > > > > > > > > > actually be able to manage its clock (like do we want the CPU clock to
> > > > > > > > > > > never *ever* be gated just because we don't have a cpuidle/hotplug
> > > > > > > > > > > driver yet?)
> > > > > > > > > > 
> > > > > > > > > > As I've just mentioned, if a clock 'can' be turned off, this binding
> > > > > > > > > > should never be used. Situations where using always-on as a stop-gap
> > > > > > > > > > due to a lack of current functionality is what the paragraph above is
> > > > > > > > > > trying to mitigate.
> > > > > > > > > 
> > > > > > > > > But it's not really what this property is about. What this property
> > > > > > > > > describes is that these clocks should never be gated. Any point in
> > > > > > > > > time during the life of the system AND with in any kernel version.
> > > > > > > > 
> > > > > > > > You got it, that's correct -- these clocks should never be gated.
> > > > > > > > 
> > > > > > > > So why would that ever change?  If that is likely (or even possible)
> > > > > > > > to change in the future then this binding should not be used.
> > > > > > > >
> > > > > > > > To reiterate; this binding should be used on ungatable clocks only.
> > > > > > > > Non-negotiable, non-changeable either by the introduction of new
> > > > > > > > functionality/support or kernel version.
> > > > > > > 
> > > > > > > I'm pretty sure that if that patch gets merged, by the end of the
> > > > > > > year, there will be "incorrect" users by your standards.
> > > > > > 
> > > > > > It's possible to abuse any binding.  I don't see why you are so
> > > > > > offended of this one in particular.
> > > > > 
> > > > > I'm not offended, I just tried to push the same kind of patches two
> > > > > years ago, with Mike pushing back, and actually came to see that he
> > > > > was right a few monthes down the road.
> > > > 
> > > > Well this was suggested by Mike.  I even have his Ack already.  So I
> > > > guess he too has changed the error of his ways. :)
> > > 
> > > I wonder why it's not even merged yet then if you have the maintainers
> > > Acked-by, and want to ignore any other review.
> > > 
> > > > > And yeah, your point that any binding can be abused is true. This one
> > > > > is only so easy to abuse it's not even funny.
> > > > > 
> > > > > > > If you introduce a feature, you should expect people to use
> > > > > > > it.  If not, what's the point?
> > > > > > 
> > > > > > By your own admission, there are genuine users for this binding and I
> > > > > > expect people to use it.
> > > > > 
> > > > > The only thing that we really disagree upon is that whether that
> > > > > restriction will really be followed. You expect people to, I
> > > > > don't. It's the fundamental disagreement we have, that really prevent
> > > > > any purely technical discussion.
> > > > > 
> > > > > Maybe we can try to address that before moving forward?
> > > > 
> > > > I'd like that.
> > > > 
> > > > If a firm warning isn't good enough, then what will be?
> > > > 
> > > > > > > > > > > Have you seen the numerous NAK on such approach Mike did?
> > > > > > > > > > 
> > > > > > > > > > I haven't, but the folks reviewing previous versions have.  Do you
> > > > > > > > > > have something specific in mind that you'd like to bring to my
> > > > > > > > > > attention?
> > > > > > > > > 
> > > > > > > > > Unfortunately, I haven't been able to dig out such mails. But it's why
> > > > > > > > > we ended up with clock protection code in various clock drivers
> > > > > > > > > including:
> > > > > > > > > 
> > > > > > > > > AT91: http://lxr.free-electrons.com/source/drivers/clk/at91/clk-slow.c#L484
> > > > > > > > > iMX28: http://lxr.free-electrons.com/source/drivers/clk/mxs/clk-imx28.c#L154
> > > > > > > > > Rockchip: http://lxr.free-electrons.com/source/drivers/clk/rockchip/clk.c#L320
> > > > > > > > > sunXi: http://lxr.free-electrons.com/source/drivers/clk/sunxi/clk-sunxi.c#L1183
> > > > > > > > > Zynq: http://lxr.free-electrons.com/source/drivers/clk/zynq/clkc.c#L504
> > > > > > > > > 
> > > > > > > > > Which is much more flexible, since you won't have to modify the DT to
> > > > > > > > > change which clocks are to be left enabled, as well as way easier to
> > > > > > > > > debug if you ever have to remove that property from the DT.
> > > > > > > > 
> > > > > > > > You're right, you don't have the change the DT in these cases.  You
> > > > > > > > have to write new C code, which is _less_ flexible.
> > > > > > > 
> > > > > > > I'm sorry to learn that you never heard of that stable-DT thing.
> > > > > > > 
> > > > > > > And a bit sorry to see that a maintainer is really seeing C as not
> > > > > > > flexible.
> > > > > > 
> > > > > > You're putting words in my mouth.  I didn't say C was not flexible.
> > > > > > 
> > > > > > I'm referencing the original DT pros i.e. it is possible to supply a
> > > > > > different configuration without the need to compile the kernel.
> > > > > > That's certainly true in this case.  We can provide a clk-provider and
> > > > > > tag it as always-on, all without re-compile.
> > > > > > 
> > > > > > > > So all these platforms are adding their own hand-rolled version of
> > > > > > > > this binding, adding more duplication and cruft to the kernel.
> > > > > > > > Instead they can use this 'always-on' and we can consolidate and strip
> > > > > > > > it all out.
> > > > > > > 
> > > > > > > Except that all these platforms are actually not implementing a
> > > > > > > binding, ie not an interface with the DT they are bound to. Each and
> > > > > > > every of these platforms can change that list whenever they wish, just
> > > > > > > by sending a single one-liner patch (just like the DT, really.).
> > > > > > > 
> > > > > > > Which is not something that you can achieve with a DT binding.
> > > > > > 
> > > > > > Once again, can you give me more information about why you have such a
> > > > > > problem with this binding.  I wish for it to be stable/ABI, I wish for
> > > > > > it never to be removed, I envisage it will always be needed, so what's
> > > > > > the problem? 
> > > > > > 
> > > > > > Do you have a vested interest that I am missing?
> > > > > > 
> > > > > > Perhaps an example of possible calamity will help convince me that
> > > > > > you're not completely wrong and blowing everything out of proportion
> > > > > > for no good reason.
> > > > > 
> > > > > Let's say you've introduced such a clock in kernel 4.0 for the memory
> > > > > clock.
> > > > > 
> > > > > At some point down the road, you create a ddrfreq driver (if that ever
> > > > > exists). You have a new driver, which will manage the clock.
> > > > > 
> > > > > In that driver, for some reason, you have to shutdown the clock to
> > > > > reset the DDR controller. Of course that also means that you will be
> > > > > removing the clk-always-on property from your DT.
> > > > > 
> > > > > You will have in your driver something like:
> > > > > 
> > > > > /* Reset our controller */
> > > > > clk_disable(clk);
> > > > > clk_enable(clk):
> > > > > 
> > > > > And then, you expect your controller to be in its out-of-reset
> > > > > state. Which will be the case with a new DT, and not with the old one,
> > > > > probably creating all kind of very entertaining issues to debug.
> > > > > 
> > > > > All of this wouldn't be the case if you had this inside the kernel,
> > > > > since (hopefully) the kernel is consistent with itself.
> > > > 
> > > > Surely you must have realised already that DTBs are more tightly
> > > > coupled to kernel versions than we would have initially liked?
> > > 
> > > Just to get things straight. I'm *not* one proponent of the "DT as an
> > > ABI" rule.
> > > 
> > > BUT, if you're designing a generic property, then you're also
> > > designing it for platforms that have chose to have that stability. And
> > > we do have some of those in the tree.
> > > 
> > > And of course, that's assuming that the ABI stability is never going
> > > to be a thing (which might or might not happen).
> > > 
> > > > It's naive to assume that old DTBs will 'just-work' with newer
> > > > kernels.
> > > 
> > > Like I said, it does work on some mainline ARM platforms.
> > > 
> > > > Wrong decisions related to DT are being made daily. Adding
> > > > mistakenly or foolhardily adding 'clk-always-on' to a DTS is not
> > > > going to be the sole cause of breakage somewhere down the line.
> > > 
> > > I'm not sure that saying that accepting bindings because some other
> > > bindings are just as bad is a good argument, especially on bindings
> > > that do impact all the platforms.
> > > 
> > > > Pushing back on the acceptance of this binding based on idealistic,
> > > > possibly already out-of-date premise is just frustrating.
> > > 
> > > Don't worry, it's just as frustrating on the other end. Showing you
> > > exactly why it's going to be an issue with you simply ignoring by
> > > saying something close to "not my use case" is just as frustrating.
> > > 
> > > > This useful binding should be accepted and people should not abuse
> > > > it.  If they do and the vendor Maintainer's review and accept then
> > > > they have no foundation for recourse.
> > > 
> > > And in the end, there will be more and more bloated and / or poor code
> > > in the kernel, hurting it as a whole.
> > > 
> > > > Would you prefer it if I made the warning starker?
> > > 
> > > The only kind of warning that would be noticed by anyone is a runtime
> > > warning. I'm not sure this is a reasonable option :)
> > 
> > Does Sascha's antidote patch change your opinion?  We can use DT to
> > declare critical clocks, and in the rare case of the introduction of a
> > new DDRFreq-like feature, which doesn't adapt the DT will still be
> > able to unlock the criticalness of the clock and use it as expected?
> 
> Honestly I'm not very fond of declaring these in the device tree, but

I know why you guys are saying that, but I'd like you to understand
the reasons for me pushing for this.  Rather than be being deliberately
obtuse, I'm thinking of the mess that not having this stuff in DT will
cause for clock implementations like ours, which describe more of a
framework than a description.

The providers in drivers/clock/st are blissfully ignorant of platform
specifics.  Per-platform configuration is described in DT.  So we'd
have 2 options to use a C-only based API; 1) duplicate platform
information in drivers/clk/st, or 2) supply a vendor specific
st,critical-clocks binding, pull out those references then run them
though the aforementioned framework.  It is my opinion that neither of
those methods are desirable.

As an aside, we also have 9 add_provider() and 9 clock_register()
calls, where we would need to consider calling the new
critical_clock() API from/after.

Please tell me that you understand and agree, or please provide me
with a suggestion to combat the issues I currently face.

> naming them 'critical-clocks' rather than 'always-on' seems more
> acceptable for me. It leaves a way out for the user to turn the clock
> off later as it only means "you may turn them off when you know what you
> are doing".

With the introduction of your latest patch we are no longer in the
peril previously described, thus if a platform does separate their
DT from its associated kernel, it won't be the end of the world (or
cost a couple mWh) if a clock becomes controllable in a subsequent
kernel version.

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

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

On Fri, May 01, 2015 at 07:44:05AM +0100, Lee Jones wrote:
> > > Does Sascha's antidote patch change your opinion?  We can use DT to
> > > declare critical clocks, and in the rare case of the introduction of a
> > > new DDRFreq-like feature, which doesn't adapt the DT will still be
> > > able to unlock the criticalness of the clock and use it as expected?
> > 
> > Honestly I'm not very fond of declaring these in the device tree, but
> 
> I know why you guys are saying that, but I'd like you to understand
> the reasons for me pushing for this.  Rather than be being deliberately
> obtuse, I'm thinking of the mess that not having this stuff in DT will
> cause for clock implementations like ours, which describe more of a
> framework than a description.

The DT should dictate our implementation, not the other way around. I
know that we are pretty bad at doing this, and that there's some clear
abstraction violations already widely used, but really, using this
kind of argument is pretty bad.

The DT can (and is) shared between several OS and bootloaders, what if
the *BSDs or barebox, or whatever, guys come up with the exact same
argument to make a completely different binding?

We'd end up either in a deadlock, or forcing our solution down the
throat to some other system. I'm not sure any of these outcomes is
something we want.

> The providers in drivers/clock/st are blissfully ignorant of platform
> specifics.  Per-platform configuration is described in DT.

Maybe they just need a small amount of education then.

> So we'd have 2 options to use a C-only based API; 1) duplicate
> platform information in drivers/clk/st, or 2) supply a vendor
> specific st,critical-clocks binding, pull out those references then
> run them though the aforementioned framework.  It is my opinion that
> neither of those methods are desirable.

3) have a generic solution for this in the clock framework, like Mike
suggested.

> As an aside, we also have 9 add_provider() and 9 clock_register()
> calls, where we would need to consider calling the new
> critical_clock() API from/after.
> 
> Please tell me that you understand and agree, or please provide me
> with a suggestion to combat the issues I currently face.

I do understand that it is convenient for you, and agree that
duplicating the clock protection code across platforms is not the best
solution.

> > naming them 'critical-clocks' rather than 'always-on' seems more
> > acceptable for me. It leaves a way out for the user to turn the clock
> > off later as it only means "you may turn them off when you know what you
> > are doing".
> 
> With the introduction of your latest patch we are no longer in the
> peril previously described, thus if a platform does separate their
> DT from its associated kernel, it won't be the end of the world (or
> cost a couple mWh) if a clock becomes controllable in a subsequent
> kernel version.

If that critical clock definition comes with a special meaning and a
matching API in the CCF as well (ie, would not be disabled by a
clk_disable_unprepare, but would be by a clk_critical_disable or
something alike), I think we should be fine.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Lee Jones]

On Thu, 07 May 2015, Maxime Ripard wrote:

> On Fri, May 01, 2015 at 07:44:05AM +0100, Lee Jones wrote:
> > > > Does Sascha's antidote patch change your opinion?  We can use DT to
> > > > declare critical clocks, and in the rare case of the introduction of a
> > > > new DDRFreq-like feature, which doesn't adapt the DT will still be
> > > > able to unlock the criticalness of the clock and use it as expected?
> > > 
> > > Honestly I'm not very fond of declaring these in the device tree, but
> > 
> > I know why you guys are saying that, but I'd like you to understand
> > the reasons for me pushing for this.  Rather than be being deliberately
> > obtuse, I'm thinking of the mess that not having this stuff in DT will
> > cause for clock implementations like ours, which describe more of a
> > framework than a description.
> 
> The DT should dictate our implementation, not the other way around. I
> know that we are pretty bad at doing this, and that there's some clear
> abstraction violations already widely used, but really, using this
> kind of argument is pretty bad.

I guess then you haven't correctly understood my argument, as that's
exactly what's happened.  We have a DT implementation which accurately
describes the clock architecture on each of our platforms. The
associated C code in drivers/clk/ is written to extract the
information from it, the hardware description and register the clocks
properly.

What makes you think differently?

> The DT can (and is) shared between several OS and bootloaders, what if
> the *BSDs or barebox, or whatever, guys come up with the exact same
> argument to make a completely different binding?
> 
> We'd end up either in a deadlock, or forcing our solution down the
> throat to some other system. I'm not sure any of these outcomes is
> something we want.

Not sure I understand why this is different from any other binding?

> > The providers in drivers/clock/st are blissfully ignorant of platform
> > specifics.  Per-platform configuration is described in DT.
> 
> Maybe they just need a small amount of education then.

Easy to say (and implement), but that means duplicating the hardware
description in DT, which is not a design win.

> > So we'd have 2 options to use a C-only based API; 1) duplicate
> > platform information in drivers/clk/st, or 2) supply a vendor
> > specific st,critical-clocks binding, pull out those references then
> > run them though the aforementioned framework.  It is my opinion that
> > neither of those methods are desirable.
> 
> 3) have a generic solution for this in the clock framework, like Mike
> suggested.

Did you actually read and understand the points here?  If not, just
say so and I'll figure out a way to explain the issues better.  3) is
not an alternative to 1) and 2).  Instead 1) and 2) imply 3).

I *want* to have a generic solution, and have made several passes at
writing one.  The question here is; what does that look like?  Some
people don't like the idea of having it in DT due to possible abuse of
the property.  But we can't have anything only in C because our clock
implementation (rightly) doesn't know or (shouldn't have to) care
about platform specifics.  Instead all platform description is in DT,
where it should be.  So to specify critical-clocks we need either 1)
or 2) above to pull the info out and send to 3).

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

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4422 bytes --]

On Fri, May 08, 2015 at 08:22:46AM +0100, Lee Jones wrote:
> On Thu, 07 May 2015, Maxime Ripard wrote:
> 
> > On Fri, May 01, 2015 at 07:44:05AM +0100, Lee Jones wrote:
> > > > > Does Sascha's antidote patch change your opinion?  We can use DT to
> > > > > declare critical clocks, and in the rare case of the introduction of a
> > > > > new DDRFreq-like feature, which doesn't adapt the DT will still be
> > > > > able to unlock the criticalness of the clock and use it as expected?
> > > > 
> > > > Honestly I'm not very fond of declaring these in the device tree, but
> > > 
> > > I know why you guys are saying that, but I'd like you to understand
> > > the reasons for me pushing for this.  Rather than be being deliberately
> > > obtuse, I'm thinking of the mess that not having this stuff in DT will
> > > cause for clock implementations like ours, which describe more of a
> > > framework than a description.
> > 
> > The DT should dictate our implementation, not the other way around. I
> > know that we are pretty bad at doing this, and that there's some clear
> > abstraction violations already widely used, but really, using this
> > kind of argument is pretty bad.
> 
> I guess then you haven't correctly understood my argument, as that's
> exactly what's happened.  We have a DT implementation which accurately
> describes the clock architecture on each of our platforms. The
> associated C code in drivers/clk/ is written to extract the
> information from it, the hardware description and register the clocks
> properly.
> 
> What makes you think differently?
> 
> > The DT can (and is) shared between several OS and bootloaders, what if
> > the *BSDs or barebox, or whatever, guys come up with the exact same
> > argument to make a completely different binding?
> > 
> > We'd end up either in a deadlock, or forcing our solution down the
> > throat to some other system. I'm not sure any of these outcomes is
> > something we want.
> 
> Not sure I understand why this is different from any other binding?

The other bindings don't dictate the OS behaviour, this one does.

> > > The providers in drivers/clock/st are blissfully ignorant of platform
> > > specifics.  Per-platform configuration is described in DT.
> > 
> > Maybe they just need a small amount of education then.
> 
> Easy to say (and implement), but that means duplicating the hardware
> description in DT, which is not a design win.

Except that clock-always-on isn't an hardware information, it's what
you expect the OS to do with this clock. The fact that it's a critical
clock, would be way better, as it gives the OS the information that
this clock should be treated with care, and *possibly* never disable
it, but still leaves the option open to do whatever it needs to do
with it if it knows what it's doing.

> > > So we'd have 2 options to use a C-only based API; 1) duplicate
> > > platform information in drivers/clk/st, or 2) supply a vendor
> > > specific st,critical-clocks binding, pull out those references then
> > > run them though the aforementioned framework.  It is my opinion that
> > > neither of those methods are desirable.
> > 
> > 3) have a generic solution for this in the clock framework, like Mike
> > suggested.
> 
> Did you actually read and understand the points here?  If not, just
> say so and I'll figure out a way to explain the issues better.  3) is
> not an alternative to 1) and 2).  Instead 1) and 2) imply 3).

Ok, I misunderstood what you meant then, my bad.

> I *want* to have a generic solution, and have made several passes at
> writing one.  The question here is; what does that look like?  Some
> people don't like the idea of having it in DT due to possible abuse of
> the property.  But we can't have anything only in C because our clock
> implementation (rightly) doesn't know or (shouldn't have to) care
> about platform specifics.

This is exactly the point I was using in my previous argument. You're
using the state of your code and implementation ("our clock
implementation doesn't know about platform specifics") to push for a
DT binding ("I want to use clock-always-on or st,critical-clocks").

And you *can* have such a description in your code. You just don't
want to.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Rob Herring]

On Tue, Apr 7, 2015 at 1:43 PM, Lee Jones <lee.jones@linaro.org> wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Please use get_maintainers.pl in the future.

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  .../devicetree/bindings/clock/clock-bindings.txt   | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index 06fc6d5..daf3323 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -44,6 +44,44 @@ For example:
>    clocks by index. The names should reflect the clock output signal
>    names for the device.
>
> +clock-always-on:    Some hardware contains bunches of clocks which 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, the common clk framework will attempt to
> +                   disable them and 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 always-on 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 an always-on clock.
> +
> +For example:
> +
> +    oscillator {
> +        #clock-cells = <1>;
> +        clock-output-names = "ckil", "ckih";
> +        clock-always-on = <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 always-on clocks, so the framework 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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 4/4] clk: dt: Introduce binding for always-on clock support

[Stephen Boyd]

On 04/07, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---

Can you please add some commit text here? Why did we make this
change?

>  .../devicetree/bindings/clock/clock-bindings.txt   | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index 06fc6d5..daf3323 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -44,6 +44,44 @@ For example:
>    clocks by index. The names should reflect the clock output signal
>    names for the device.
>  
> +clock-always-on:    Some hardware contains bunches of clocks which 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, the common clk framework will attempt to
> +		    disable them and a platform can fail irrecoverably as a
> +		    result.  Usually the only way to recover from these failures
> +		    is to reboot.

Should we even be talking about "the common clk framework" in
this document? Ideally this document/binding isn't Linux
specific. Plus, the framework doesn't disable clocks during
suspend, it disables clocks during late init as long as the clock
isn't enabled by software.

Maybe we can drop this first paragraph and replace the second
with this single sentence:

	Clocks which should never be disabled.

> +
> +		    To avoid either of these two scenarios from catastrophically
> +		    disabling an otherwise perfectly healthy running system,
> +		    clocks can be identified as always-on using this property
> +		    from inside a clocksource's node.

s/clocksource/clock provider/

> +
> +		    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 an always-on clock.
> +
> +For example:
> +
> +    oscillator {
> +        #clock-cells = <1>;
> +        clock-output-names = "ckil", "ckih";
> +        clock-always-on = <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 always-on clocks, so the framework will

s/an//

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 0/4] clk: Provide support for always-on clocks

[Stephen Boyd]

On 04/07, Lee Jones wrote:
> 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 always-on clocks to be supplied
> which must be kept ungated.  Each clock mentioned in the newly
> introduced 'clock-always-on' 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 clk_disable_unused()
> and disable_clock() procedures.

Ah I see now. The disable_clock() part is from the PM core not
the common clock framework. This paragraph could be the commit
text for the binding patch.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project