LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3 0/4] clk: st: New always-on clock domain
@ 2015-02-24 17:33 Lee Jones
  2015-02-24 17:33 ` [PATCH v3 1/4] ARM: sti: stih407-family: Supply defines for CLOCKGEN A0 Lee Jones
                   ` (6 more replies)
  0 siblings, 7 replies; 21+ messages in thread
From: Lee Jones @ 2015-02-24 17:33 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: lee.jones, kernel, mturquette, sboyd, devicetree

v2 => v3:
  - Ensure DT actually reflects h/w
    - i.e. Nodes should not contain a mishmash of different IP
      blocks, but should identify related h/w.  In the current
      example we use interconnects
  - Change naming from clkdomain to clk-always-on
  - Place "do not abuse" warning in documentation

v1 => v2:
  - Turned the ST specific driver into a generic one
  
Hardware can have a bunch of clocks which must not 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 turn them off and the hardware will
subsequently die.  The only way to recover from this failure is to
restart.
  
To avoid either of these two scenarios from catastrophically
disabling the running system we have implemented a clock domain
where clocks are consumed and references are taken, thus preventing
them from being shut down by the framework.

*** BLURB HERE ***

Lee Jones (4):
  ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
  ARM: sti: stih407-family: Add platform interconnects to always-on clk
    domain
  clk: Provide an always-on clock domain framework
  clk: dt: Introduce always-on clock domain documentation

 .../devicetree/bindings/clock/clk-always-on.txt    | 35 ++++++++++++
 arch/arm/boot/dts/stih407-family.dtsi              | 15 ++++++
 drivers/clk/Makefile                               |  1 +
 drivers/clk/clk-always-on.c                        | 62 ++++++++++++++++++++++
 include/dt-bindings/clock/stih407-clks.h           |  4 ++
 5 files changed, 117 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/clk-always-on.txt
 create mode 100644 drivers/clk/clk-always-on.c

-- 
1.9.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v3 1/4] ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
  2015-02-24 17:33 [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
@ 2015-02-24 17:33 ` Lee Jones
  2015-02-24 17:33 ` [PATCH v3 2/4] ARM: sti: stih407-family: Add platform interconnects to always-on clk domain Lee Jones
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Lee Jones @ 2015-02-24 17:33 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: lee.jones, kernel, mturquette, sboyd, devicetree

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v3 2/4] ARM: sti: stih407-family: Add platform interconnects to always-on clk domain
  2015-02-24 17:33 [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
  2015-02-24 17:33 ` [PATCH v3 1/4] ARM: sti: stih407-family: Supply defines for CLOCKGEN A0 Lee Jones
@ 2015-02-24 17:33 ` Lee Jones
  2015-02-24 17:33 ` [PATCH v3 3/4] clk: Provide an always-on clock domain framework Lee Jones
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Lee Jones @ 2015-02-24 17:33 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: lee.jones, kernel, mturquette, sboyd, devicetree

Certain clocks should not be turned of by clk_disable_unused.  Until
now we have been using the kernel command-line of the same name to
prevent common clk from turning off all clocks without a reference,
as this would ensure hardware lockup.  This patch lists each
platform interconnect which needs to stay on to prevent the
aforementioned issue from arising.

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

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index c06a546..35c74c6 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -34,6 +34,21 @@
 		reg = <0x08761000 0x1000>, <0x08760100 0x100>;
 	};
 
+	/*
+	 * ICNs are not visible/controllable in Linux, but references to their
+	 * clocks must be obtained and retained or the platform will become
+	 * irrecoverably unresponsive.
+	 */
+	icn {
+		compatible = "clk-always-on";
+		clocks = <&clk_s_c0_flexgen CLK_ICN_SBC>,
+			 <&clk_s_c0_flexgen CLK_ICN_CPU>,
+			 <&clk_s_c0_flexgen CLK_ICN_LMI>,
+			 <&clk_s_c0_flexgen CLK_TX_ICN_DMU>,
+			 <&clk_s_a0_flexgen CLK_IC_LMI0>,
+			 <&clk_s_c0_flexgen CLK_EXT2F_A9>;
+	};
+
 	scu@08760000 {
 		compatible = "arm,cortex-a9-scu";
 		reg = <0x08760000 0x1000>;
-- 
1.9.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v3 3/4] clk: Provide an always-on clock domain framework
  2015-02-24 17:33 [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
  2015-02-24 17:33 ` [PATCH v3 1/4] ARM: sti: stih407-family: Supply defines for CLOCKGEN A0 Lee Jones
  2015-02-24 17:33 ` [PATCH v3 2/4] ARM: sti: stih407-family: Add platform interconnects to always-on clk domain Lee Jones
@ 2015-02-24 17:33 ` Lee Jones
  2015-02-24 17:33 ` [PATCH v3 4/4] clk: dt: Introduce always-on clock domain documentation Lee Jones
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Lee Jones @ 2015-02-24 17:33 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: lee.jones, kernel, mturquette, sboyd, devicetree

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 in order to
prevent the common clk framework from trying to turn them off during
the clk_disabled_unused() procedure.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/clk/Makefile        |  1 +
 drivers/clk/clk-always-on.c | 62 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)
 create mode 100644 drivers/clk/clk-always-on.c

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index d478ceb..0d42541 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_HAVE_CLK)		+= clk-devres.o
 obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
 obj-$(CONFIG_COMMON_CLK)	+= clk.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-divider.o
+obj-$(CONFIG_COMMON_CLK)	+= clk-always-on.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-factor.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-rate.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-gate.o
diff --git a/drivers/clk/clk-always-on.c b/drivers/clk/clk-always-on.c
new file mode 100644
index 0000000..f3d90ac
--- /dev/null
+++ b/drivers/clk/clk-always-on.c
@@ -0,0 +1,62 @@
+/*
+ * Always-on Clock Domain
+ *
+ * Copyright (C) 2015 STMicroelectronics – All Rights Reserved
+ *
+ * Author: Lee Jones <lee.jones@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+static void ao_clock_domain_hog_clock(struct platform_device *pdev, int index)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct clk *clk;
+	int ret;
+
+	clk = of_clk_get(np, index);
+	if (IS_ERR(clk)) {
+		dev_warn(&pdev->dev, "Failed get clock %s[%d]: %li\n",
+			 np->full_name, index, PTR_ERR(clk));
+		return;
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		dev_warn(&pdev->dev, "Failed to enable clock: %s\n", __clk_get_name(clk));
+}
+
+static int ao_clock_domain_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int nclks, i;
+
+	nclks = of_count_phandle_with_args(np, "clocks", "#clock-cells");
+
+	for (i = 0; i < nclks; i++)
+		ao_clock_domain_hog_clock(pdev, i);
+
+	return 0;
+}
+
+static const struct of_device_id ao_clock_domain_match[] = {
+	{ .compatible = "clk-always-on" },
+	{ },
+};
+
+static struct platform_driver ao_clock_domain_driver = {
+	.probe = ao_clock_domain_probe,
+	.driver = {
+		.name = "clk-always-on",
+		.of_match_table = ao_clock_domain_match,
+	},
+};
+module_platform_driver(ao_clock_domain_driver);
-- 
1.9.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v3 4/4] clk: dt: Introduce always-on clock domain documentation
  2015-02-24 17:33 [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
                   ` (2 preceding siblings ...)
  2015-02-24 17:33 ` [PATCH v3 3/4] clk: Provide an always-on clock domain framework Lee Jones
@ 2015-02-24 17:33 ` Lee Jones
  2015-02-24 17:42 ` [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Lee Jones @ 2015-02-24 17:33 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: lee.jones, kernel, mturquette, sboyd, devicetree

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 .../devicetree/bindings/clock/clk-always-on.txt    | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/clk-always-on.txt

diff --git a/Documentation/devicetree/bindings/clock/clk-always-on.txt b/Documentation/devicetree/bindings/clock/clk-always-on.txt
new file mode 100644
index 0000000..d61eaca
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/clk-always-on.txt
@@ -0,0 +1,35 @@
+Always-on Clock Domain
+
+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 the hardware can fail
+irrecoverably.  Usually the only way to recover from these failures is
+to restart.
+
+To avoid either of these two scenarios from catastrophically disabling
+an otherwise perfectly healthy running system, an always-on clock
+domain has been provided.  In this implementation clocks are consumed
+and references are taken, thus preventing them from being shut down by
+the framework.
+
+This binding 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 clk_disable_unused.
+
+We use the generic clock bindings found in:
+  Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : Must be "clk-always-on"
+
+Example:
+
+icn {
+	compatible = "clk-always-on";
+	clocks = <&clk_s_c0_flexgen CLK_ICN_SBC>,
+		 <&clk_s_c0_flexgen CLK_ICN_LMI>,
+		 <&clk_s_c0_flexgen CLK_ICN_CPU>,
+		 <&clk_s_c0_flexgen CLK_TX_ICN_DMU>,
+		 <&clk_s_a0_flexgen CLK_IC_LMI0>,
+};
-- 
1.9.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-02-24 17:33 [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
                   ` (3 preceding siblings ...)
  2015-02-24 17:33 ` [PATCH v3 4/4] clk: dt: Introduce always-on clock domain documentation Lee Jones
@ 2015-02-24 17:42 ` Lee Jones
  2015-02-27 21:37 ` Robert Jarzmik
  2015-03-04 12:00 ` Lee Jones
  6 siblings, 0 replies; 21+ messages in thread
From: Lee Jones @ 2015-02-24 17:42 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel; +Cc: kernel, mturquette, sboyd, devicetree

On Tue, 24 Feb 2015, Lee Jones wrote:

> v2 => v3:
>   - Ensure DT actually reflects h/w
>     - i.e. Nodes should not contain a mishmash of different IP
>       blocks, but should identify related h/w.  In the current
>       example we use interconnects
>   - Change naming from clkdomain to clk-always-on
>   - Place "do not abuse" warning in documentation
> 
> v1 => v2:
>   - Turned the ST specific driver into a generic one
>   
> Hardware can have a bunch of clocks which must not 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 turn them off and the hardware will
> subsequently die.  The only way to recover from this failure is to
> restart.
>   
> To avoid either of these two scenarios from catastrophically
> disabling the running system we have implemented a clock domain
> where clocks are consumed and references are taken, thus preventing
> them from being shut down by the framework.
> 
> *** BLURB HERE ***

*grumble*, that's annoying, as I removed this.

I guess Git caches the patches before sending them.

> Lee Jones (4):
>   ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
>   ARM: sti: stih407-family: Add platform interconnects to always-on clk
>     domain
>   clk: Provide an always-on clock domain framework
>   clk: dt: Introduce always-on clock domain documentation
> 
>  .../devicetree/bindings/clock/clk-always-on.txt    | 35 ++++++++++++
>  arch/arm/boot/dts/stih407-family.dtsi              | 15 ++++++
>  drivers/clk/Makefile                               |  1 +
>  drivers/clk/clk-always-on.c                        | 62 ++++++++++++++++++++++
>  include/dt-bindings/clock/stih407-clks.h           |  4 ++
>  5 files changed, 117 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/clk-always-on.txt
>  create mode 100644 drivers/clk/clk-always-on.c
> 

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-02-24 17:33 [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
                   ` (4 preceding siblings ...)
  2015-02-24 17:42 ` [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
@ 2015-02-27 21:37 ` Robert Jarzmik
  2015-02-27 21:49   ` Lee Jones
  2015-03-04 12:00 ` Lee Jones
  6 siblings, 1 reply; 21+ messages in thread
From: Robert Jarzmik @ 2015-02-27 21:37 UTC (permalink / raw)
  To: Lee Jones
  Cc: linux-arm-kernel, linux-kernel, mturquette, sboyd, kernel, devicetree

Lee Jones <lee.jones@linaro.org> writes:

> v2 => v3:
>   - Ensure DT actually reflects h/w
>     - i.e. Nodes should not contain a mishmash of different IP
>       blocks, but should identify related h/w.  In the current
>       example we use interconnects
>   - Change naming from clkdomain to clk-always-on
>   - Place "do not abuse" warning in documentation
>
> v1 => v2:
>   - Turned the ST specific driver into a generic one
>   
> Hardware can have a bunch of clocks which must not 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 turn them off and the hardware will
> subsequently die.  The only way to recover from this failure is to
> restart.
>   
> To avoid either of these two scenarios from catastrophically
> disabling the running system we have implemented a clock domain
> where clocks are consumed and references are taken, thus preventing
> them from being shut down by the framework.

Hi Lee,

I wonder why there is a need for a new clock when CLK_IGNORE_UNUSED does
exist. What is the usecase that is covered by this patchset which is not used by
CLK_IGNORE_UNUSED clock flag ?

And if that reason exists, I'd like to find it in the commit message.

Cheers.

--
Robert

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-02-27 21:37 ` Robert Jarzmik
@ 2015-02-27 21:49   ` Lee Jones
  2015-02-27 23:38     ` Robert Jarzmik
  0 siblings, 1 reply; 21+ messages in thread
From: Lee Jones @ 2015-02-27 21:49 UTC (permalink / raw)
  To: Robert Jarzmik
  Cc: linux-arm-kernel, linux-kernel, mturquette, sboyd, kernel, devicetree

On Fri, 27 Feb 2015, Robert Jarzmik wrote:

> Lee Jones <lee.jones@linaro.org> writes:
> 
> > v2 => v3:
> >   - Ensure DT actually reflects h/w
> >     - i.e. Nodes should not contain a mishmash of different IP
> >       blocks, but should identify related h/w.  In the current
> >       example we use interconnects
> >   - Change naming from clkdomain to clk-always-on
> >   - Place "do not abuse" warning in documentation
> >
> > v1 => v2:
> >   - Turned the ST specific driver into a generic one
> >   
> > Hardware can have a bunch of clocks which must not 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 turn them off and the hardware will
> > subsequently die.  The only way to recover from this failure is to
> > restart.
> >   
> > To avoid either of these two scenarios from catastrophically
> > disabling the running system we have implemented a clock domain
> > where clocks are consumed and references are taken, thus preventing
> > them from being shut down by the framework.
> 
> Hi Lee,
> 
> I wonder why there is a need for a new clock when CLK_IGNORE_UNUSED does
> exist. What is the usecase that is covered by this patchset which is not used by
> CLK_IGNORE_UNUSED clock flag ?
> 
> And if that reason exists, I'd like to find it in the commit message.

The problem is applying that flag in a generic way.

However, I guess you haven't seen this [1] yet?

[1] https://lkml.org/lkml/2015/2/27/548

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-02-27 21:49   ` Lee Jones
@ 2015-02-27 23:38     ` Robert Jarzmik
  2015-03-02  8:30       ` Lee Jones
  0 siblings, 1 reply; 21+ messages in thread
From: Robert Jarzmik @ 2015-02-27 23:38 UTC (permalink / raw)
  To: Lee Jones
  Cc: linux-arm-kernel, linux-kernel, mturquette, sboyd, kernel, devicetree

Lee Jones <lee.jones@linaro.org> writes:

>> I wonder why there is a need for a new clock when CLK_IGNORE_UNUSED does
>> exist. What is the usecase that is covered by this patchset which is not used by
>> CLK_IGNORE_UNUSED clock flag ?
>> 
>> And if that reason exists, I'd like to find it in the commit message.
>
> The problem is applying that flag in a generic way.
>
> However, I guess you haven't seen this [1] yet?
>
> [1] https://lkml.org/lkml/2015/2/27/548
I have.

And yet :
 1) This won't go in a _commit_ message (as opposed to cover-letter). Moreover
    it doesn't specify which usecase is not covered by CLK_IGNORE_UNUSED, it
    says, up to my understanding, that is it another way to have to
    CLK_IGNORE_UNUSED flag applied.

 2) I still fail to see why this is necessary
    IOW why declaring the mandatory always-on clocks with the proper flag should
    be augmented with a new clock list. Isn't the existing flag the generic way
    ?

I might not understand the real motivation behind that of course, that's why I'm
asking.

Cheers.

-- 
Robert

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-02-27 23:38     ` Robert Jarzmik
@ 2015-03-02  8:30       ` Lee Jones
  2015-03-02 11:29         ` Robert Jarzmik
  0 siblings, 1 reply; 21+ messages in thread
From: Lee Jones @ 2015-03-02  8:30 UTC (permalink / raw)
  To: Robert Jarzmik
  Cc: linux-arm-kernel, linux-kernel, mturquette, sboyd, kernel, devicetree

On Sat, 28 Feb 2015, Robert Jarzmik wrote:

> Lee Jones <lee.jones@linaro.org> writes:
> 
> >> I wonder why there is a need for a new clock when CLK_IGNORE_UNUSED does
> >> exist. What is the usecase that is covered by this patchset which is not used by
> >> CLK_IGNORE_UNUSED clock flag ?
> >> 
> >> And if that reason exists, I'd like to find it in the commit message.
> >
> > The problem is applying that flag in a generic way.
> >
> > However, I guess you haven't seen this [1] yet?
> >
> > [1] https://lkml.org/lkml/2015/2/27/548
> I have.
> 
> And yet :
>  1) This won't go in a _commit_ message (as opposed to cover-letter). Moreover

Did you read rest of the set, or just the cover-letter?  I referenced
0/0 because it is the thread parent and from there you can drill down
into the commits where I believe there is adequate explanation in
each.  If you could be more specific and tell me which commit you
think requires more explanation, I'd be happy to take a look.

>     it doesn't specify which usecase is not covered by CLK_IGNORE_UNUSED, it
>     says, up to my understanding, that is it another way to have to
>     CLK_IGNORE_UNUSED flag applied.

Well that is exactly what we're doing.  Is there an issue with that?

This is a way to do it at a platform level.  It means we can support
multiple platforms where clocksources have been switched around
without writing new driver code in drivers/clk/st.

If you have something else in mind, let me know.

>  2) I still fail to see why this is necessary
>     IOW why declaring the mandatory always-on clocks with the proper flag should
>     be augmented with a new clock list. Isn't the existing flag the generic way
>     ?

I'm not sure what you mean by this, would you be able to expland a
little?

> I might not understand the real motivation behind that of course, that's why I'm
> asking.

Please bear in mind that we don't supply our clocks statically.  All
of the information is extracted from DT, so if the always-on
information does reside in there, where do you propose it comes from?

We could just write this code inside our own driver and apply the
CLK_IGNORE_UNUSED at a local level, but that's not the generic
solution I am searching for.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-03-02  8:30       ` Lee Jones
@ 2015-03-02 11:29         ` Robert Jarzmik
  2015-03-02 11:37           ` Lee Jones
  0 siblings, 1 reply; 21+ messages in thread
From: Robert Jarzmik @ 2015-03-02 11:29 UTC (permalink / raw)
  To: Lee Jones
  Cc: linux-arm-kernel, linux-kernel, mturquette, sboyd, kernel, devicetree

Lee Jones <lee.jones@linaro.org> writes:

> On Sat, 28 Feb 2015, Robert Jarzmik wrote:
>
>> Lee Jones <lee.jones@linaro.org> writes:
>>     it doesn't specify which usecase is not covered by CLK_IGNORE_UNUSED, it
>>     says, up to my understanding, that is it another way to have to
>>     CLK_IGNORE_UNUSED flag applied.
>
> Well that is exactly what we're doing.  Is there an issue with that?
>
> This is a way to do it at a platform level.  It means we can support
> multiple platforms where clocksources have been switched around
> without writing new driver code in drivers/clk/st.
>
> If you have something else in mind, let me know.
>
>>  2) I still fail to see why this is necessary
>>     IOW why declaring the mandatory always-on clocks with the proper flag should
>>     be augmented with a new clock list. Isn't the existing flag the generic way
>>     ?
>
> I'm not sure what you mean by this, would you be able to expland a
> little?
>
>> I might not understand the real motivation behind that of course, that's why I'm
>> asking.
>
> Please bear in mind that we don't supply our clocks statically.  All
> of the information is extracted from DT, so if the always-on
> information does reside in there, where do you propose it comes from?

I thought the standard clock binding provided a way to set this flag. Now I
crosschecked the binding, it doesn't ...

My point was I didn't want the flag to be settable from 2 different places,
where consistency was to be kept across different device-tree leafs.

> We could just write this code inside our own driver and apply the
> CLK_IGNORE_UNUSED at a local level, but that's not the generic
> solution I am searching for.

All right, I'm convinced now I undertand the flag was not settable from
devicetree binding before this patchset.

You can add to patch 3/4 :
Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>

-- 
Robert

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-03-02 11:29         ` Robert Jarzmik
@ 2015-03-02 11:37           ` Lee Jones
  0 siblings, 0 replies; 21+ messages in thread
From: Lee Jones @ 2015-03-02 11:37 UTC (permalink / raw)
  To: Robert Jarzmik
  Cc: linux-arm-kernel, linux-kernel, mturquette, sboyd, kernel, devicetree

On Mon, 02 Mar 2015, Robert Jarzmik wrote:

> Lee Jones <lee.jones@linaro.org> writes:
> 
> > On Sat, 28 Feb 2015, Robert Jarzmik wrote:
> >
> >> Lee Jones <lee.jones@linaro.org> writes:
> >>     it doesn't specify which usecase is not covered by CLK_IGNORE_UNUSED, it
> >>     says, up to my understanding, that is it another way to have to
> >>     CLK_IGNORE_UNUSED flag applied.
> >
> > Well that is exactly what we're doing.  Is there an issue with that?
> >
> > This is a way to do it at a platform level.  It means we can support
> > multiple platforms where clocksources have been switched around
> > without writing new driver code in drivers/clk/st.
> >
> > If you have something else in mind, let me know.
> >
> >>  2) I still fail to see why this is necessary
> >>     IOW why declaring the mandatory always-on clocks with the proper flag should
> >>     be augmented with a new clock list. Isn't the existing flag the generic way
> >>     ?
> >
> > I'm not sure what you mean by this, would you be able to expland a
> > little?
> >
> >> I might not understand the real motivation behind that of course, that's why I'm
> >> asking.
> >
> > Please bear in mind that we don't supply our clocks statically.  All
> > of the information is extracted from DT, so if the always-on
> > information does reside in there, where do you propose it comes from?
> 
> I thought the standard clock binding provided a way to set this flag. Now I
> crosschecked the binding, it doesn't ...
> 
> My point was I didn't want the flag to be settable from 2 different places,
> where consistency was to be kept across different device-tree leafs.
> 
> > We could just write this code inside our own driver and apply the
> > CLK_IGNORE_UNUSED at a local level, but that's not the generic
> > solution I am searching for.
> 
> All right, I'm convinced now I undertand the flag was not settable from
> devicetree binding before this patchset.
> 
> You can add to patch 3/4 :
> Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>

Until told otherwise, I'm going to apply this onto the other
patchset.  This one has already been NACKed, due to DT push-back.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-02-24 17:33 [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
                   ` (5 preceding siblings ...)
  2015-02-27 21:37 ` Robert Jarzmik
@ 2015-03-04 12:00 ` Lee Jones
  2015-03-06 19:08   ` Mike Turquette
  6 siblings, 1 reply; 21+ messages in thread
From: Lee Jones @ 2015-03-04 12:00 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel; +Cc: kernel, mturquette, sboyd, devicetree

Mike,

Do you want me to resend this set with Robert's Reviewed-by applied,
or are you happy to apply it yourself?

> v2 => v3:
>   - Ensure DT actually reflects h/w
>     - i.e. Nodes should not contain a mishmash of different IP
>       blocks, but should identify related h/w.  In the current
>       example we use interconnects
>   - Change naming from clkdomain to clk-always-on
>   - Place "do not abuse" warning in documentation
> 
> v1 => v2:
>   - Turned the ST specific driver into a generic one
>   
> Hardware can have a bunch of clocks which must not 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 turn them off and the hardware will
> subsequently die.  The only way to recover from this failure is to
> restart.
>   
> To avoid either of these two scenarios from catastrophically
> disabling the running system we have implemented a clock domain
> where clocks are consumed and references are taken, thus preventing
> them from being shut down by the framework.
> 
> *** BLURB HERE ***
> 
> Lee Jones (4):
>   ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
>   ARM: sti: stih407-family: Add platform interconnects to always-on clk
>     domain
>   clk: Provide an always-on clock domain framework
>   clk: dt: Introduce always-on clock domain documentation
> 
>  .../devicetree/bindings/clock/clk-always-on.txt    | 35 ++++++++++++
>  arch/arm/boot/dts/stih407-family.dtsi              | 15 ++++++
>  drivers/clk/Makefile                               |  1 +
>  drivers/clk/clk-always-on.c                        | 62 ++++++++++++++++++++++
>  include/dt-bindings/clock/stih407-clks.h           |  4 ++
>  5 files changed, 117 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/clk-always-on.txt
>  create mode 100644 drivers/clk/clk-always-on.c
> 

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-03-04 12:00 ` Lee Jones
@ 2015-03-06 19:08   ` Mike Turquette
  2015-03-09  9:28     ` Lee Jones
  0 siblings, 1 reply; 21+ messages in thread
From: Mike Turquette @ 2015-03-06 19:08 UTC (permalink / raw)
  To: Lee Jones, linux-arm-kernel, linux-kernel; +Cc: kernel, sboyd, devicetree

Quoting Lee Jones (2015-03-04 04:00:03)
> Mike,
> 
> Do you want me to resend this set with Robert's Reviewed-by applied,
> or are you happy to apply it yourself?

No need for the resend. I am hoping for a final review from a DT human.

This approach looks fine to me. In practice I think it is restricted to
hardware blocks that don't exist in DT yet (e.g. no driver, in the case
of your interconnect) and that restriction is probably for the best.

Regards,
Mike

> 
> > v2 => v3:
> >   - Ensure DT actually reflects h/w
> >     - i.e. Nodes should not contain a mishmash of different IP
> >       blocks, but should identify related h/w.  In the current
> >       example we use interconnects
> >   - Change naming from clkdomain to clk-always-on
> >   - Place "do not abuse" warning in documentation
> > 
> > v1 => v2:
> >   - Turned the ST specific driver into a generic one
> >   
> > Hardware can have a bunch of clocks which must not 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 turn them off and the hardware will
> > subsequently die.  The only way to recover from this failure is to
> > restart.
> >   
> > To avoid either of these two scenarios from catastrophically
> > disabling the running system we have implemented a clock domain
> > where clocks are consumed and references are taken, thus preventing
> > them from being shut down by the framework.
> > 
> > *** BLURB HERE ***
> > 
> > Lee Jones (4):
> >   ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
> >   ARM: sti: stih407-family: Add platform interconnects to always-on clk
> >     domain
> >   clk: Provide an always-on clock domain framework
> >   clk: dt: Introduce always-on clock domain documentation
> > 
> >  .../devicetree/bindings/clock/clk-always-on.txt    | 35 ++++++++++++
> >  arch/arm/boot/dts/stih407-family.dtsi              | 15 ++++++
> >  drivers/clk/Makefile                               |  1 +
> >  drivers/clk/clk-always-on.c                        | 62 ++++++++++++++++++++++
> >  include/dt-bindings/clock/stih407-clks.h           |  4 ++
> >  5 files changed, 117 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/clk-always-on.txt
> >  create mode 100644 drivers/clk/clk-always-on.c
> > 
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-03-06 19:08   ` Mike Turquette
@ 2015-03-09  9:28     ` Lee Jones
  2015-03-25  4:11       ` Geert Uytterhoeven
  0 siblings, 1 reply; 21+ messages in thread
From: Lee Jones @ 2015-03-09  9:28 UTC (permalink / raw)
  To: Mike Turquette; +Cc: linux-arm-kernel, linux-kernel, kernel, sboyd, devicetree

On Fri, 06 Mar 2015, Mike Turquette wrote:

> Quoting Lee Jones (2015-03-04 04:00:03)
> > Mike,
> > 
> > Do you want me to resend this set with Robert's Reviewed-by applied,
> > or are you happy to apply it yourself?
> 
> No need for the resend. I am hoping for a final review from a DT human.
> 
> This approach looks fine to me. In practice I think it is restricted to
> hardware blocks that don't exist in DT yet (e.g. no driver, in the case
> of your interconnect) and that restriction is probably for the best.

Agreed.

> > > v2 => v3:
> > >   - Ensure DT actually reflects h/w
> > >     - i.e. Nodes should not contain a mishmash of different IP
> > >       blocks, but should identify related h/w.  In the current
> > >       example we use interconnects
> > >   - Change naming from clkdomain to clk-always-on
> > >   - Place "do not abuse" warning in documentation
> > > 
> > > v1 => v2:
> > >   - Turned the ST specific driver into a generic one
> > >   
> > > Hardware can have a bunch of clocks which must not 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 turn them off and the hardware will
> > > subsequently die.  The only way to recover from this failure is to
> > > restart.
> > >   
> > > To avoid either of these two scenarios from catastrophically
> > > disabling the running system we have implemented a clock domain
> > > where clocks are consumed and references are taken, thus preventing
> > > them from being shut down by the framework.
> > > 
> > > *** BLURB HERE ***
> > > 
> > > Lee Jones (4):
> > >   ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
> > >   ARM: sti: stih407-family: Add platform interconnects to always-on clk
> > >     domain
> > >   clk: Provide an always-on clock domain framework
> > >   clk: dt: Introduce always-on clock domain documentation
> > > 
> > >  .../devicetree/bindings/clock/clk-always-on.txt    | 35 ++++++++++++
> > >  arch/arm/boot/dts/stih407-family.dtsi              | 15 ++++++
> > >  drivers/clk/Makefile                               |  1 +
> > >  drivers/clk/clk-always-on.c                        | 62 ++++++++++++++++++++++
> > >  include/dt-bindings/clock/stih407-clks.h           |  4 ++
> > >  5 files changed, 117 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/clock/clk-always-on.txt
> > >  create mode 100644 drivers/clk/clk-always-on.c
> > > 
> > 

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-03-09  9:28     ` Lee Jones
@ 2015-03-25  4:11       ` Geert Uytterhoeven
  2015-03-26 13:51         ` Lee Jones
  0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2015-03-25  4:11 UTC (permalink / raw)
  To: Lee Jones
  Cc: Mike Turquette, linux-arm-kernel, linux-kernel, kernel,
	Stephen Boyd, devicetree

Hi Lee,

On Mon, Mar 9, 2015 at 10:28 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 06 Mar 2015, Mike Turquette wrote:
>> Quoting Lee Jones (2015-03-04 04:00:03)
>> > Mike,
>> >
>> > Do you want me to resend this set with Robert's Reviewed-by applied,
>> > or are you happy to apply it yourself?
>>
>> No need for the resend. I am hoping for a final review from a DT human.
>>
>> This approach looks fine to me. In practice I think it is restricted to
>> hardware blocks that don't exist in DT yet (e.g. no driver, in the case
>> of your interconnect) and that restriction is probably for the best.
>
> Agreed.

I think this restriction should be documented in the DT binding more clearly,
as adding a "clk-always-on" node prohibits you from handling the clock
correctly in
the future.

Still, for simple devices where you don't have a driver, but have "predictable"
bindings (e.g. a bus like "simple-pm-bus"), I think it's better to add
a device node
for that simple device now, incl. a reference to the clock, and have a simple
driver that binds to the device, or platform code that looks for a
compatible node,
and enables the clock. That way you don't have to make any chances to the DTS
later, when you'll have a real driver.

>> > > v2 => v3:
>> > >   - Ensure DT actually reflects h/w
>> > >     - i.e. Nodes should not contain a mishmash of different IP
>> > >       blocks, but should identify related h/w.  In the current
>> > >       example we use interconnects
>> > >   - Change naming from clkdomain to clk-always-on
>> > >   - Place "do not abuse" warning in documentation
>> > >
>> > > v1 => v2:
>> > >   - Turned the ST specific driver into a generic one
>> > >
>> > > Hardware can have a bunch of clocks which must not 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 turn them off and the hardware will
>> > > subsequently die.  The only way to recover from this failure is to
>> > > restart.
>> > >
>> > > To avoid either of these two scenarios from catastrophically
>> > > disabling the running system we have implemented a clock domain
>> > > where clocks are consumed and references are taken, thus preventing
>> > > them from being shut down by the framework.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-03-25  4:11       ` Geert Uytterhoeven
@ 2015-03-26 13:51         ` Lee Jones
  2015-03-26 16:55           ` Geert Uytterhoeven
  0 siblings, 1 reply; 21+ messages in thread
From: Lee Jones @ 2015-03-26 13:51 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mike Turquette, linux-arm-kernel, linux-kernel, kernel,
	Stephen Boyd, devicetree

On Wed, 25 Mar 2015, Geert Uytterhoeven wrote:

> Hi Lee,
> 
> On Mon, Mar 9, 2015 at 10:28 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Fri, 06 Mar 2015, Mike Turquette wrote:
> >> Quoting Lee Jones (2015-03-04 04:00:03)
> >> > Mike,
> >> >
> >> > Do you want me to resend this set with Robert's Reviewed-by applied,
> >> > or are you happy to apply it yourself?
> >>
> >> No need for the resend. I am hoping for a final review from a DT human.
> >>
> >> This approach looks fine to me. In practice I think it is restricted to
> >> hardware blocks that don't exist in DT yet (e.g. no driver, in the case
> >> of your interconnect) and that restriction is probably for the best.
> >
> > Agreed.
> 
> I think this restriction should be documented in the DT binding more clearly,
> as adding a "clk-always-on" node prohibits you from handling the clock
> correctly in
> the future.

Would you mind taking the time to explain what you think those
limitations are?

> Still, for simple devices where you don't have a driver, but have "predictable"
> bindings (e.g. a bus like "simple-pm-bus"), I think it's better to add
> a device node
> for that simple device now, incl. a reference to the clock, and have a simple
> driver that binds to the device, or platform code that looks for a
> compatible node,
> and enables the clock. That way you don't have to make any chances to the DTS
> later, when you'll have a real driver.
> 
> >> > > v2 => v3:
> >> > >   - Ensure DT actually reflects h/w
> >> > >     - i.e. Nodes should not contain a mishmash of different IP
> >> > >       blocks, but should identify related h/w.  In the current
> >> > >       example we use interconnects
> >> > >   - Change naming from clkdomain to clk-always-on
> >> > >   - Place "do not abuse" warning in documentation
> >> > >
> >> > > v1 => v2:
> >> > >   - Turned the ST specific driver into a generic one
> >> > >
> >> > > Hardware can have a bunch of clocks which must not 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 turn them off and the hardware will
> >> > > subsequently die.  The only way to recover from this failure is to
> >> > > restart.
> >> > >
> >> > > To avoid either of these two scenarios from catastrophically
> >> > > disabling the running system we have implemented a clock domain
> >> > > where clocks are consumed and references are taken, thus preventing
> >> > > them from being shut down by the framework.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-03-26 13:51         ` Lee Jones
@ 2015-03-26 16:55           ` Geert Uytterhoeven
  2015-03-26 19:38             ` Lee Jones
  0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2015-03-26 16:55 UTC (permalink / raw)
  To: Lee Jones
  Cc: Mike Turquette, linux-arm-kernel, linux-kernel, kernel,
	Stephen Boyd, devicetree

Hi Lee,

On Thu, Mar 26, 2015 at 2:51 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Wed, 25 Mar 2015, Geert Uytterhoeven wrote:
>> On Mon, Mar 9, 2015 at 10:28 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > On Fri, 06 Mar 2015, Mike Turquette wrote:
>> >> This approach looks fine to me. In practice I think it is restricted to
>> >> hardware blocks that don't exist in DT yet (e.g. no driver, in the case
>> >> of your interconnect) and that restriction is probably for the best.
>> >
>> > Agreed.
>>
>> I think this restriction should be documented in the DT binding more clearly,
>> as adding a "clk-always-on" node prohibits you from handling the clock
>> correctly in
>> the future.
>
> Would you mind taking the time to explain what you think those
> limitations are?

If you add a "clk-always-on" node, the clock will always on using that DT.
That will still be true later, when you get a better understanding of the
hardware, and might discover you're gonna need a driver for the currently
hidden core component that's driven by the clock, and may want to manage
that clock.

When that happens, you're gonna need a DT update to make use of the
newly introduced driver and the features it provides (e.g. better power
management).

On the other hand, if you would describe the actual hardware topology in DT,
the device node for the future driver would already be there, and no DT update
is needed (assuming you can guess right w.r.t. its bindings; usually these are
transparent buses, for which bindings are fairly generic, cfr. e.g.
"simple-pm-bus").

Until you have (a need for) a real driver, you do need some platform code that
makes sure the clock is enabled. When a real driver is introduced, the
platform code has to be updated, but DT doesn't have to change.

(The same is true for devices where the current driver isn't aware of the
 clock, and shouldn't be, but you still need to enable the clock until the
 driver has Runtime PM support (E.g. ARM GIC on shmobile, cfr.
 https://www.marc.info/?l=linux-pm&m=142670617929493&w=3 (good, now
 we have a bidirectional link between these two threads :-) Using a
 "clk-always-on" property there instead of adding a reference to the clock
 in the existing GIC device node would be just lying.)

If we start seeing many users, perhaps we need a general framework where
the platform code can register a list of clocks that must be enabled (properly,
i.e. using clk_prepare_enable())? But I don't think the list should be put
in DT: DT describes hardware, not behavior.

If you don't care about DT stability, and can afford always updating your DT
with your kernel, the above doesn't apply. I heard during the ELC hallways
chats this is actually the case for you?

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-03-26 16:55           ` Geert Uytterhoeven
@ 2015-03-26 19:38             ` Lee Jones
  2015-04-02  8:31               ` Geert Uytterhoeven
  0 siblings, 1 reply; 21+ messages in thread
From: Lee Jones @ 2015-03-26 19:38 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mike Turquette, linux-arm-kernel, linux-kernel, kernel,
	Stephen Boyd, devicetree

On Thu, 26 Mar 2015, Geert Uytterhoeven wrote:

> Hi Lee,
> 
> On Thu, Mar 26, 2015 at 2:51 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Wed, 25 Mar 2015, Geert Uytterhoeven wrote:
> >> On Mon, Mar 9, 2015 at 10:28 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > On Fri, 06 Mar 2015, Mike Turquette wrote:
> >> >> This approach looks fine to me. In practice I think it is restricted to
> >> >> hardware blocks that don't exist in DT yet (e.g. no driver, in the case
> >> >> of your interconnect) and that restriction is probably for the best.
> >> >
> >> > Agreed.
> >>
> >> I think this restriction should be documented in the DT binding more clearly,
> >> as adding a "clk-always-on" node prohibits you from handling the clock
> >> correctly in
> >> the future.
> >
> > Would you mind taking the time to explain what you think those
> > limitations are?
> 
> If you add a "clk-always-on" node, the clock will always on using that DT.
> That will still be true later, when you get a better understanding of the
> hardware, and might discover you're gonna need a driver for the currently
> hidden core component that's driven by the clock, and may want to manage
> that clock.

So I have two points here.

First point; I think you're looking at an older version of my set.
The newer one can be found at [1] and no longer uses 'always-on'
nodes.  Instead the 'clk-always-on' property is applied to the
provider.  See the documentation patch [2] for more details.

Second point; this binding is _not_ to be used as a hack because the
hardware isn't understood.  Genuine uses are for clocks that must not
be turned off ever, else bad things will happen.  If the hardware is
not understood, use 'clk-disable-unused' on the kernel cmdline
instead.

> When that happens, you're gonna need a DT update to make use of the
> newly introduced driver and the features it provides (e.g. better power
> management).

The whole point of this set is to manage clocks that can't be power
managed.  Using the 'clk-always-on' property labels the clock with a
big-red-sign saying "DO NOT TURN ME OFF, OR RISK CATASTROPHIC
FAILURE", not a "TODO: Fix-me when you understand me".

> On the other hand, if you would describe the actual hardware topology in DT,
> the device node for the future driver would already be there, and no DT update
> is needed (assuming you can guess right w.r.t. its bindings; usually these are
> transparent buses, for which bindings are fairly generic, cfr. e.g.
> "simple-pm-bus").

In our case the devices these clocks control will never be added to
Device Tree.  Linux can't see them, they have no registers and they
can't be controlled.  Added a device driver for all such devices would
be foolhardy.

> Until you have (a need for) a real driver, you do need some platform code that
> makes sure the clock is enabled. When a real driver is introduced, the
> platform code has to be updated, but DT doesn't have to change.

No, this binding is _not_ for that use-case.  If your platform is
incomplete you must use 'clk-disable-unused'.  

> (The same is true for devices where the current driver isn't aware of the
>  clock, and shouldn't be, but you still need to enable the clock until the
>  driver has Runtime PM support (E.g. ARM GIC on shmobile, cfr.
>  https://www.marc.info/?l=linux-pm&m=142670617929493&w=3 (good, now
>  we have a bidirectional link between these two threads :-) Using a
>  "clk-always-on" property there instead of adding a reference to the clock
>  in the existing GIC device node would be just lying.)

If this clock should _genuinely_ be always-on, then use my new
binding in the clock controller node and the Clk framework will not
turn it off.

> If we start seeing many users, perhaps we need a general framework where
> the platform code can register a list of clocks that must be enabled (properly,
> i.e. using clk_prepare_enable())? But I don't think the list should be put
> in DT: DT describes hardware, not behavior.

I did this in the very first iteration of this set.  The DT guys
didn't like it, which is why I re-wrote it to look like [1].

> If you don't care about DT stability, and can afford always updating your DT
> with your kernel, the above doesn't apply. I heard during the ELC hallways
> chats this is actually the case for you?

My ears burning eh?  Do tell...

I think you currently misunderstand the use-case for this set.  I hope
my points above will clarify the point somewhat.  Once we write the
bindings and the DT nodes in [1], I don't plan for them to be changed,
thus our DT will stay stable in this regard.

NB: Yes, we are quite fortunate by the fact that most of are bindings
can be considered fairly transient, but that is irrelevant in this
particular case.

[1] https://lkml.org/lkml/2015/2/27/548
[2] https://lkml.org/lkml/2015/2/27/551

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-03-26 19:38             ` Lee Jones
@ 2015-04-02  8:31               ` Geert Uytterhoeven
  2015-04-02 10:48                 ` Lee Jones
  0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2015-04-02  8:31 UTC (permalink / raw)
  To: Lee Jones
  Cc: Mike Turquette, linux-arm-kernel, linux-kernel, kernel,
	Stephen Boyd, devicetree

Hi Lee,

On Thu, Mar 26, 2015 at 8:38 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 26 Mar 2015, Geert Uytterhoeven wrote:
>> On Thu, Mar 26, 2015 at 2:51 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > On Wed, 25 Mar 2015, Geert Uytterhoeven wrote:
>> >> On Mon, Mar 9, 2015 at 10:28 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> > On Fri, 06 Mar 2015, Mike Turquette wrote:
>> >> >> This approach looks fine to me. In practice I think it is restricted to
>> >> >> hardware blocks that don't exist in DT yet (e.g. no driver, in the case
>> >> >> of your interconnect) and that restriction is probably for the best.
>> >> >
>> >> > Agreed.
>> >>
>> >> I think this restriction should be documented in the DT binding more clearly,
>> >> as adding a "clk-always-on" node prohibits you from handling the clock
>> >> correctly in
>> >> the future.
>> >
>> > Would you mind taking the time to explain what you think those
>> > limitations are?
>>
>> If you add a "clk-always-on" node, the clock will always on using that DT.
>> That will still be true later, when you get a better understanding of the
>> hardware, and might discover you're gonna need a driver for the currently
>> hidden core component that's driven by the clock, and may want to manage
>> that clock.
>
> So I have two points here.
>
> First point; I think you're looking at an older version of my set.
> The newer one can be found at [1] and no longer uses 'always-on'
> nodes.  Instead the 'clk-always-on' property is applied to the
> provider.  See the documentation patch [2] for more details.

Thanks, I was indeed looking at an old version.
Still, that doesn't change that the clock to not be disabled in specified
explicitly from DT.

> Second point; this binding is _not_ to be used as a hack because the
> hardware isn't understood.  Genuine uses are for clocks that must not
> be turned off ever, else bad things will happen.  If the hardware is
> not understood, use 'clk-disable-unused' on the kernel cmdline
> instead.

[...]

>> (The same is true for devices where the current driver isn't aware of the
>>  clock, and shouldn't be, but you still need to enable the clock until the
>>  driver has Runtime PM support (E.g. ARM GIC on shmobile, cfr.
>>  https://www.marc.info/?l=linux-pm&m=142670617929493&w=3 (good, now
>>  we have a bidirectional link between these two threads :-) Using a
>>  "clk-always-on" property there instead of adding a reference to the clock
>>  in the existing GIC device node would be just lying.)
>
> If this clock should _genuinely_ be always-on, then use my new
> binding in the clock controller node and the Clk framework will not
> turn it off.

It's supposed to be on when the application ARM core(s) is/are running.
Many SoCs also have smaller cores (SH, Cortex R or M), intended to
run a real-time OS. If the RT core is in charge, it may decide to shut down
the application ARM core(s), incl. supposedly always-on modules like
the ARM GIC.

I couldn't find a detailed block diagram of the STiH4xx SoCs, but at least
STiH416 has an "ST proprietary multi-compartmental security IP and DRM
processor".

> [1] https://lkml.org/lkml/2015/2/27/548
> [2] https://lkml.org/lkml/2015/2/27/551

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 0/4] clk: st: New always-on clock domain
  2015-04-02  8:31               ` Geert Uytterhoeven
@ 2015-04-02 10:48                 ` Lee Jones
  0 siblings, 0 replies; 21+ messages in thread
From: Lee Jones @ 2015-04-02 10:48 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mike Turquette, linux-arm-kernel, linux-kernel, kernel,
	Stephen Boyd, devicetree

On Thu, 02 Apr 2015, Geert Uytterhoeven wrote:
> On Thu, Mar 26, 2015 at 8:38 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Thu, 26 Mar 2015, Geert Uytterhoeven wrote:
> >> On Thu, Mar 26, 2015 at 2:51 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > On Wed, 25 Mar 2015, Geert Uytterhoeven wrote:
> >> >> On Mon, Mar 9, 2015 at 10:28 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >> >> > On Fri, 06 Mar 2015, Mike Turquette wrote:
> >> >> >> This approach looks fine to me. In practice I think it is restricted to
> >> >> >> hardware blocks that don't exist in DT yet (e.g. no driver, in the case
> >> >> >> of your interconnect) and that restriction is probably for the best.
> >> >> >
> >> >> > Agreed.
> >> >>
> >> >> I think this restriction should be documented in the DT binding more clearly,
> >> >> as adding a "clk-always-on" node prohibits you from handling the clock
> >> >> correctly in
> >> >> the future.
> >> >
> >> > Would you mind taking the time to explain what you think those
> >> > limitations are?
> >>
> >> If you add a "clk-always-on" node, the clock will always on using that DT.
> >> That will still be true later, when you get a better understanding of the
> >> hardware, and might discover you're gonna need a driver for the currently
> >> hidden core component that's driven by the clock, and may want to manage
> >> that clock.
> >
> > So I have two points here.
> >
> > First point; I think you're looking at an older version of my set.
> > The newer one can be found at [1] and no longer uses 'always-on'
> > nodes.  Instead the 'clk-always-on' property is applied to the
> > provider.  See the documentation patch [2] for more details.
> 
> Thanks, I was indeed looking at an old version.
> Still, that doesn't change that the clock to not be disabled in specified
> explicitly from DT.
> 
> > Second point; this binding is _not_ to be used as a hack because the
> > hardware isn't understood.  Genuine uses are for clocks that must not
> > be turned off ever, else bad things will happen.  If the hardware is
> > not understood, use 'clk-disable-unused' on the kernel cmdline
> > instead.

[...]

> > If this clock should _genuinely_ be always-on, then use my new
> > binding in the clock controller node and the Clk framework will not
> > turn it off.
> 
> It's supposed to be on when the application ARM core(s) is/are running.
> Many SoCs also have smaller cores (SH, Cortex R or M), intended to
> run a real-time OS. If the RT core is in charge, it may decide to shut down
> the application ARM core(s), incl. supposedly always-on modules like
> the ARM GIC.
> 
> I couldn't find a detailed block diagram of the STiH4xx SoCs, but at least
> STiH416 has an "ST proprietary multi-compartmental security IP and DRM
> processor".

You might be interested in the latest incarnation of the set.

See if it solves your issues.

https://lkml.org/lkml/2015/4/1/267

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2015-04-02 10:48 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-24 17:33 [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
2015-02-24 17:33 ` [PATCH v3 1/4] ARM: sti: stih407-family: Supply defines for CLOCKGEN A0 Lee Jones
2015-02-24 17:33 ` [PATCH v3 2/4] ARM: sti: stih407-family: Add platform interconnects to always-on clk domain Lee Jones
2015-02-24 17:33 ` [PATCH v3 3/4] clk: Provide an always-on clock domain framework Lee Jones
2015-02-24 17:33 ` [PATCH v3 4/4] clk: dt: Introduce always-on clock domain documentation Lee Jones
2015-02-24 17:42 ` [PATCH v3 0/4] clk: st: New always-on clock domain Lee Jones
2015-02-27 21:37 ` Robert Jarzmik
2015-02-27 21:49   ` Lee Jones
2015-02-27 23:38     ` Robert Jarzmik
2015-03-02  8:30       ` Lee Jones
2015-03-02 11:29         ` Robert Jarzmik
2015-03-02 11:37           ` Lee Jones
2015-03-04 12:00 ` Lee Jones
2015-03-06 19:08   ` Mike Turquette
2015-03-09  9:28     ` Lee Jones
2015-03-25  4:11       ` Geert Uytterhoeven
2015-03-26 13:51         ` Lee Jones
2015-03-26 16:55           ` Geert Uytterhoeven
2015-03-26 19:38             ` Lee Jones
2015-04-02  8:31               ` Geert Uytterhoeven
2015-04-02 10:48                 ` Lee Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).