Subject: [PATCH v2 0/4] clk: st: New clock domain
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.
Lee Jones (4):
ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
ARM: sti: stih407-family: Provide Clock Domain information
clk: Provide an always-on clock domain framework
clk: dt: Introduce always-on clock domain documentation
.../devicetree/bindings/clock/clk-domain.txt | 35 ++++++++++++
arch/arm/boot/dts/stih407-family.dtsi | 13 +++++
drivers/clk/Makefile | 1 +
drivers/clk/clkdomain.c | 63 ++++++++++++++++++++++
include/dt-bindings/clock/stih407-clks.h | 4 ++
5 files changed, 116 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/clk-domain.txt
create mode 100644 drivers/clk/clkdomain.c
--
1.9.1
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 <[email protected]>
---
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
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 clock
which need to stay on to prevent the aforementioned issue from
arising.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/stih407-family.dtsi | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 3e31d32..49af509 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -34,6 +34,19 @@
reg = <0x08761000 0x1000>, <0x08760100 0x100>;
};
+ clk-domain {
+ compatible = "always-on-clk-domain";
+ clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>,
+ <&clk_s_c0_flexgen CLK_COMPO_DVP>,
+ <&clk_s_c0_flexgen CLK_MMC_1>,
+ <&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>,
+ <&clk_m_a9>;
+ };
+
scu@08760000 {
compatible = "arm,cortex-a9-scu";
reg = <0x08760000 0x1000>;
--
1.9.1
Much h/w 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 <[email protected]>
---
drivers/clk/Makefile | 1 +
drivers/clk/clkdomain.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 64 insertions(+)
create mode 100644 drivers/clk/clkdomain.c
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index d5fba5b..d9e2718 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) += clkdomain.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/clkdomain.c b/drivers/clk/clkdomain.c
new file mode 100644
index 0000000..8c83f99
--- /dev/null
+++ b/drivers/clk/clkdomain.c
@@ -0,0 +1,63 @@
+/*
+ * ST Clock Domain
+ *
+ * Copyright (C) 2015 STMicroelectronics – All Rights Reserved
+ *
+ * Author: Lee Jones <[email protected]>
+ *
+ * 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-private.h>
+#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->name);
+}
+
+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 = "always-on-clk-domain" },
+ { },
+};
+
+static struct platform_driver ao_clock_domain_driver = {
+ .probe = ao_clock_domain_probe,
+ .driver = {
+ .name = "always-on-clk-domain",
+ .of_match_table = ao_clock_domain_match,
+ },
+};
+module_platform_driver(ao_clock_domain_driver);
--
1.9.1
Signed-off-by: Lee Jones <[email protected]>
---
.../devicetree/bindings/clock/clk-domain.txt | 35 ++++++++++++++++++++++
1 file changed, 35 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/clk-domain.txt
diff --git a/Documentation/devicetree/bindings/clock/clk-domain.txt b/Documentation/devicetree/bindings/clock/clk-domain.txt
new file mode 100644
index 0000000..b86772f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/clk-domain.txt
@@ -0,0 +1,35 @@
+Always-on Clock Domain
+
+Some hardware is 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, we have
+implemented a clock domain where clocks are consumed and references
+are taken, thus preventing them from being shut down by the
+framework.
+
+We use the generic clock bindings found in:
+ Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : Must be "always-on-clk-domain"
+
+Example:
+
+clk-domain {
+ compatible = "always-on-clk-domain";
+ clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>,
+ <&clk_s_c0_flexgen CLK_COMPO_DVP>,
+ <&clk_s_c0_flexgen CLK_MMC_1>,
+ <&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>,
+ <&clk_m_a9>;
+};
--
1.9.1
On Wed, Feb 18, 2015 at 10:15 AM, Lee Jones <[email protected]> wrote:
> Signed-off-by: Lee Jones <[email protected]>
> ---
> .../devicetree/bindings/clock/clk-domain.txt | 35 ++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/clk-domain.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/clk-domain.txt b/Documentation/devicetree/bindings/clock/clk-domain.txt
> new file mode 100644
> index 0000000..b86772f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/clk-domain.txt
> @@ -0,0 +1,35 @@
> +Always-on Clock Domain
> +
> +Some hardware is 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.
How is (b) not a bug?
While I think we need something here, I worry that this will be abused
to be a list of clocks you have not gotten around to managing. We
cannot be changing the DT every time the kernel starts managing a
clock. I think this should operate more as always on until claimed.
But then you get into drivers having to be aware that the clock
started enabled.
Also, I feel like we are using DT to work around kernel policy (of
turning off clocks). If the policy was to leave on clocks, then we
would be trying to put a list of clocks to disable in DT.
Rob
> +
> +To avoid either of these two scenarios from catastrophically
> +disabling an otherwise perfectly healthy 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.
> +
> +We use the generic clock bindings found in:
> + Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : Must be "always-on-clk-domain"
> +
> +Example:
> +
> +clk-domain {
> + compatible = "always-on-clk-domain";
> + clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>,
> + <&clk_s_c0_flexgen CLK_COMPO_DVP>,
> + <&clk_s_c0_flexgen CLK_MMC_1>,
> + <&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>,
> + <&clk_m_a9>;
> +};
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 18 Feb 2015, Rob Herring wrote:
> On Wed, Feb 18, 2015 at 10:15 AM, Lee Jones <[email protected]> wrote:
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > .../devicetree/bindings/clock/clk-domain.txt | 35 ++++++++++++++++++++++
> > 1 file changed, 35 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/clock/clk-domain.txt
> >
> > diff --git a/Documentation/devicetree/bindings/clock/clk-domain.txt b/Documentation/devicetree/bindings/clock/clk-domain.txt
> > new file mode 100644
> > index 0000000..b86772f5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/clk-domain.txt
> > @@ -0,0 +1,35 @@
> > +Always-on Clock Domain
> > +
> > +Some hardware is 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.
>
> How is (b) not a bug?
Clocks are normally disabled during suspend. When clocks are disabled
they give up their reference. If references reach 0, the clock is
gated. If one of these clocks is gated, the system will never come
out of suspend.
How is it a bug?
> While I think we need something here, I worry that this will be abused
> to be a list of clocks you have not gotten around to managing. We
You can say that about any framework. It's our responsibility to ask
the right questions and to disallow it from being abused. The clocks
I use in the (real-world) example in this set are _really_ always-on.
If any of them are turned off the system will cease to perform in any
meaningful way.
> cannot be changing the DT every time the kernel starts managing a
> clock. I think this should operate more as always on until claimed.
The point of this is that even when these clocks are claimed, there is
an issue that when unclaimed (i.e. during suspend) the clk framework
will attempt to gate them, and when they do *boom*.
> But then you get into drivers having to be aware that the clock
> started enabled.
This has nothing to do with the initial state of the clock. It's
whether the clock is integral (i.e. is part of a vital interconnect)
that's important. For instance, ST's bootloader turns on lots of
clocks which can be safely gated if unused. The clocks we're
registering with this always-on framework cannot.
> Also, I feel like we are using DT to work around kernel policy (of
> turning off clocks). If the policy was to leave on clocks, then we
> would be trying to put a list of clocks to disable in DT.
I'm not sure I understand your point. The current policy is correct
if it's power that you care about, which is invariably the point of
disabling clocks in the first place, right? Also, this has nothing to
do with DT per say. It's just another framework driver.
> > +To avoid either of these two scenarios from catastrophically
> > +disabling an otherwise perfectly healthy 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.
> > +
> > +We use the generic clock bindings found in:
> > + Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +
> > +Required properties:
> > +- compatible : Must be "always-on-clk-domain"
> > +
> > +Example:
> > +
> > +clk-domain {
> > + compatible = "always-on-clk-domain";
> > + clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>,
> > + <&clk_s_c0_flexgen CLK_COMPO_DVP>,
> > + <&clk_s_c0_flexgen CLK_MMC_1>,
> > + <&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>,
> > + <&clk_m_a9>;
> > +};
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Feb 18, 2015 at 11:12 AM, Lee Jones <[email protected]> wrote:
> On Wed, 18 Feb 2015, Rob Herring wrote:
>
>> On Wed, Feb 18, 2015 at 10:15 AM, Lee Jones <[email protected]> wrote:
>> > Signed-off-by: Lee Jones <[email protected]>
>> > ---
>> > .../devicetree/bindings/clock/clk-domain.txt | 35 ++++++++++++++++++++++
>> > 1 file changed, 35 insertions(+)
>> > create mode 100644 Documentation/devicetree/bindings/clock/clk-domain.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/clock/clk-domain.txt b/Documentation/devicetree/bindings/clock/clk-domain.txt
>> > new file mode 100644
>> > index 0000000..b86772f5
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/clock/clk-domain.txt
>> > @@ -0,0 +1,35 @@
>> > +Always-on Clock Domain
>> > +
>> > +Some hardware is 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.
>>
>> How is (b) not a bug?
>
> Clocks are normally disabled during suspend. When clocks are disabled
> they give up their reference. If references reach 0, the clock is
> gated. If one of these clocks is gated, the system will never come
> out of suspend.
>
> How is it a bug?
It a clock needs to be enabled during suspend, then the driver using
it should not disable it. Anyway, suspend is a bit orthogonal to this
issue.
>> While I think we need something here, I worry that this will be abused
>> to be a list of clocks you have not gotten around to managing. We
>
> You can say that about any framework. It's our responsibility to ask
> the right questions and to disallow it from being abused. The clocks
> I use in the (real-world) example in this set are _really_ always-on.
> If any of them are turned off the system will cease to perform in any
> meaningful way.
You cannot tell here up front whether clocks are really always-on. A
reviewer is not going to know, and the submitter may not even have all
the documentation and know the answer. Getting it wrong here means you
have to change the dtb to fix it. Granted, it doesn't really break
things in this case.
>> cannot be changing the DT every time the kernel starts managing a
>> clock. I think this should operate more as always on until claimed.
>
> The point of this is that even when these clocks are claimed, there is
> an issue that when unclaimed (i.e. during suspend) the clk framework
> will attempt to gate them, and when they do *boom*.
>
>> But then you get into drivers having to be aware that the clock
>> started enabled.
>
> This has nothing to do with the initial state of the clock. It's
> whether the clock is integral (i.e. is part of a vital interconnect)
> that's important. For instance, ST's bootloader turns on lots of
> clocks which can be safely gated if unused. The clocks we're
> registering with this always-on framework cannot.
It does because you have to assume either the initial state is wrong
and you need to disable it, or that the initial state is correct and
you need to leave the clock enabled.
There are also other usecases such as simplefb where you want to leave
clocks on until the real fb driver takes over. Consoles have a similar
issue.
Perhaps you need to model your buses more completely? Does
simple-pm-bus help you?
>> Also, I feel like we are using DT to work around kernel policy (of
>> turning off clocks). If the policy was to leave on clocks, then we
>> would be trying to put a list of clocks to disable in DT.
>
> I'm not sure I understand your point. The current policy is correct
> if it's power that you care about, which is invariably the point of
> disabling clocks in the first place, right? Also, this has nothing to
> do with DT per say. It's just another framework driver.
It does have something to do with DT because you are designing a
binding around what the kernel does. Should the kernel assume it can
disable clocks safely? Another OS may do the opposite and assume it
cannot turn-off unused clocks. Then you would have a list of clocks
safe to disable in DT.
This is also completely solvable within the framework driver by
claiming those clocks in the clock controller driver.
Rob
On Wed, 18 Feb 2015, Rob Herring wrote:
> On Wed, Feb 18, 2015 at 11:12 AM, Lee Jones <[email protected]> wrote:
> > On Wed, 18 Feb 2015, Rob Herring wrote:
> >
> >> On Wed, Feb 18, 2015 at 10:15 AM, Lee Jones <[email protected]> wrote:
> >> > Signed-off-by: Lee Jones <[email protected]>
> >> > ---
> >> > .../devicetree/bindings/clock/clk-domain.txt | 35 ++++++++++++++++++++++
> >> > 1 file changed, 35 insertions(+)
> >> > create mode 100644 Documentation/devicetree/bindings/clock/clk-domain.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/clock/clk-domain.txt b/Documentation/devicetree/bindings/clock/clk-domain.txt
> >> > new file mode 100644
> >> > index 0000000..b86772f5
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/clock/clk-domain.txt
> >> > @@ -0,0 +1,35 @@
> >> > +Always-on Clock Domain
> >> > +
> >> > +Some hardware is 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.
> >>
> >> How is (b) not a bug?
> >
> > Clocks are normally disabled during suspend. When clocks are disabled
> > they give up their reference. If references reach 0, the clock is
> > gated. If one of these clocks is gated, the system will never come
> > out of suspend.
> >
> > How is it a bug?
>
> It a clock needs to be enabled during suspend, then the driver using
> it should not disable it. Anyway, suspend is a bit orthogonal to this
> issue.
IMO, it's not the driver's responsibility to know which clock they are
using and whether it's a critical clock or not.
> >> While I think we need something here, I worry that this will be abused
> >> to be a list of clocks you have not gotten around to managing. We
> >
> > You can say that about any framework. It's our responsibility to ask
> > the right questions and to disallow it from being abused. The clocks
> > I use in the (real-world) example in this set are _really_ always-on.
> > If any of them are turned off the system will cease to perform in any
> > meaningful way.
>
> You cannot tell here up front whether clocks are really always-on. A
> reviewer is not going to know, and the submitter may not even have all
> the documentation and know the answer. Getting it wrong here means you
> have to change the dtb to fix it. Granted, it doesn't really break
> things in this case.
We should make it clear in the documentation that this framework
should only be used if the clock is a critical "if it's turned off it
will cripple the system" one.
> >> cannot be changing the DT every time the kernel starts managing a
> >> clock. I think this should operate more as always on until claimed.
> >
> > The point of this is that even when these clocks are claimed, there is
> > an issue that when unclaimed (i.e. during suspend) the clk framework
> > will attempt to gate them, and when they do *boom*.
> >
> >> But then you get into drivers having to be aware that the clock
> >> started enabled.
> >
> > This has nothing to do with the initial state of the clock. It's
> > whether the clock is integral (i.e. is part of a vital interconnect)
> > that's important. For instance, ST's bootloader turns on lots of
> > clocks which can be safely gated if unused. The clocks we're
> > registering with this always-on framework cannot.
>
> It does because you have to assume either the initial state is wrong
> and you need to disable it, or that the initial state is correct and
> you need to leave the clock enabled.
I think the kernel's policy is a good one i.e. wait until all devices
are probed and have had the opportunity to take the clocks they need,
at which point we can usually safely gate the remainder. These types
of clocks are the exception however; hence the need for this driver.
There are other vendors which have similar issues with their h/w.
These are currently using bespoke versions of this implementation, but
IMO a generic solution would be better.
> There are also other usecases such as simplefb where you want to leave
> clocks on until the real fb driver takes over. Consoles have a similar
> issue.
Why wouldn't these devices have taken references by the time
clk_disable_unused() is called?
> Perhaps you need to model your buses more completely?
Would you mind explaining this a little more please?
> Does simple-pm-bus help you?
I have no idea what this is, and I'm struggling to grep for it too?
> >> Also, I feel like we are using DT to work around kernel policy (of
> >> turning off clocks). If the policy was to leave on clocks, then we
> >> would be trying to put a list of clocks to disable in DT.
> >
> > I'm not sure I understand your point. The current policy is correct
> > if it's power that you care about, which is invariably the point of
> > disabling clocks in the first place, right? Also, this has nothing to
> > do with DT per say. It's just another framework driver.
>
> It does have something to do with DT because you are designing a
> binding around what the kernel does. Should the kernel assume it can
> disable clocks safely?
I guess it depends on what you're trying to achieve. Personally I
think the kernel's policy is a good one, especually with regards to
power saving. What are you suggesting? A new policy?
> Another OS may do the opposite and assume it
> cannot turn-off unused clocks. Then you would have a list of clocks
> safe to disable in DT.
Sounds bananas. What's good about that kind of policy? It wouldn't
matter anyway, both of these implementations can live harmoniously in
the same tree.
> This is also completely solvable within the framework driver by
> claiming those clocks in the clock controller driver.
This conversation has now gone full circle. This was an earlier
suggestion, but it was considered hacky, and I'm inclined to agree.
An always-on power domain was deemed to be a much more elegant
solution.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Feb 18, 2015 at 3:54 PM, Lee Jones <[email protected]> wrote:
> On Wed, 18 Feb 2015, Rob Herring wrote:
>
>> On Wed, Feb 18, 2015 at 11:12 AM, Lee Jones <[email protected]> wrote:
>> > On Wed, 18 Feb 2015, Rob Herring wrote:
>> >
>> >> On Wed, Feb 18, 2015 at 10:15 AM, Lee Jones <[email protected]> wrote:
>> >> > Signed-off-by: Lee Jones <[email protected]>
>> >> > ---
>> >> > .../devicetree/bindings/clock/clk-domain.txt | 35 ++++++++++++++++++++++
>> >> > 1 file changed, 35 insertions(+)
>> >> > create mode 100644 Documentation/devicetree/bindings/clock/clk-domain.txt
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/clock/clk-domain.txt b/Documentation/devicetree/bindings/clock/clk-domain.txt
>> >> > new file mode 100644
>> >> > index 0000000..b86772f5
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/clock/clk-domain.txt
>> >> > @@ -0,0 +1,35 @@
>> >> > +Always-on Clock Domain
>> >> > +
>> >> > +Some hardware is 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.
>> >>
>> >> How is (b) not a bug?
>> >
>> > Clocks are normally disabled during suspend. When clocks are disabled
>> > they give up their reference. If references reach 0, the clock is
>> > gated. If one of these clocks is gated, the system will never come
>> > out of suspend.
>> >
>> > How is it a bug?
>>
>> It a clock needs to be enabled during suspend, then the driver using
>> it should not disable it. Anyway, suspend is a bit orthogonal to this
>> issue.
>
> IMO, it's not the driver's responsibility to know which clock they are
> using and whether it's a critical clock or not.
Certainly drivers should not know about clocks outside of their h/w
block, but they absolutely should know if a clock is needed for
wake-up.
>> >> While I think we need something here, I worry that this will be abused
>> >> to be a list of clocks you have not gotten around to managing. We
>> >
>> > You can say that about any framework. It's our responsibility to ask
>> > the right questions and to disallow it from being abused. The clocks
>> > I use in the (real-world) example in this set are _really_ always-on.
>> > If any of them are turned off the system will cease to perform in any
>> > meaningful way.
>>
>> You cannot tell here up front whether clocks are really always-on. A
>> reviewer is not going to know, and the submitter may not even have all
>> the documentation and know the answer. Getting it wrong here means you
>> have to change the dtb to fix it. Granted, it doesn't really break
>> things in this case.
>
> We should make it clear in the documentation that this framework
> should only be used if the clock is a critical "if it's turned off it
> will cripple the system" one.
>
>> >> cannot be changing the DT every time the kernel starts managing a
>> >> clock. I think this should operate more as always on until claimed.
>> >
>> > The point of this is that even when these clocks are claimed, there is
>> > an issue that when unclaimed (i.e. during suspend) the clk framework
>> > will attempt to gate them, and when they do *boom*.
>> >
>> >> But then you get into drivers having to be aware that the clock
>> >> started enabled.
>> >
>> > This has nothing to do with the initial state of the clock. It's
>> > whether the clock is integral (i.e. is part of a vital interconnect)
>> > that's important. For instance, ST's bootloader turns on lots of
>> > clocks which can be safely gated if unused. The clocks we're
>> > registering with this always-on framework cannot.
>>
>> It does because you have to assume either the initial state is wrong
>> and you need to disable it, or that the initial state is correct and
>> you need to leave the clock enabled.
>
> I think the kernel's policy is a good one i.e. wait until all devices
> are probed and have had the opportunity to take the clocks they need,
> at which point we can usually safely gate the remainder. These types
> of clocks are the exception however; hence the need for this driver.
> There are other vendors which have similar issues with their h/w.
> These are currently using bespoke versions of this implementation, but
> IMO a generic solution would be better.
>
>> There are also other usecases such as simplefb where you want to leave
>> clocks on until the real fb driver takes over. Consoles have a similar
>> issue.
>
> Why wouldn't these devices have taken references by the time
> clk_disable_unused() is called?
Not if the drivers are modules.
>> Perhaps you need to model your buses more completely?
>
> Would you mind explaining this a little more please?
>
>> Does simple-pm-bus help you?
>
> I have no idea what this is, and I'm struggling to grep for it too?
http://lwn.net/Articles/632058/
I'm not saying this works as-is for you, but people are starting to
add bus properties to buses.
>> >> Also, I feel like we are using DT to work around kernel policy (of
>> >> turning off clocks). If the policy was to leave on clocks, then we
>> >> would be trying to put a list of clocks to disable in DT.
>> >
>> > I'm not sure I understand your point. The current policy is correct
>> > if it's power that you care about, which is invariably the point of
>> > disabling clocks in the first place, right? Also, this has nothing to
>> > do with DT per say. It's just another framework driver.
>>
>> It does have something to do with DT because you are designing a
>> binding around what the kernel does. Should the kernel assume it can
>> disable clocks safely?
>
> I guess it depends on what you're trying to achieve. Personally I
> think the kernel's policy is a good one, especually with regards to
> power saving. What are you suggesting? A new policy?
No. The binding just has to work no matter what the OS policy.
>> Another OS may do the opposite and assume it
>> cannot turn-off unused clocks. Then you would have a list of clocks
>> safe to disable in DT.
>
> Sounds bananas. What's good about that kind of policy? It wouldn't
> matter anyway, both of these implementations can live harmoniously in
> the same tree.
Your systems won't go *boom*.
>> This is also completely solvable within the framework driver by
>> claiming those clocks in the clock controller driver.
>
> This conversation has now gone full circle. This was an earlier
> suggestion, but it was considered hacky, and I'm inclined to agree.
The clock maintainer doesn't want hacks in the clock framework and the
DT maintainer doesn't want them in DT... We should put them in MFD. ;)
> An always-on power domain was deemed to be a much more elegant
> solution.
Now you are mixing in power domains?
I'm not saying we can't put something in DT. I'm okay with that, but
it needs to handle the case of the clocks do get claimed either after
boot (e.g. by a module) or in later kernels without a dtb change.
Rob
Hi Lee,
On Wed, Feb 18, 2015 at 10:54 PM, Lee Jones <[email protected]> wrote:
>> >> > +Some hardware is 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.
>> >>
>> >> How is (b) not a bug?
>> >
>> > Clocks are normally disabled during suspend. When clocks are disabled
>> > they give up their reference. If references reach 0, the clock is
>> > gated. If one of these clocks is gated, the system will never come
>> > out of suspend.
>> >
>> > How is it a bug?
>>
>> It a clock needs to be enabled during suspend, then the driver using
>> it should not disable it. Anyway, suspend is a bit orthogonal to this
>> issue.
>
> IMO, it's not the driver's responsibility to know which clock they are
> using and whether it's a critical clock or not.
So it's (partly) a platform/bus issue.
>> >> While I think we need something here, I worry that this will be abused
>> >> to be a list of clocks you have not gotten around to managing. We
>> >
>> > You can say that about any framework. It's our responsibility to ask
>> > the right questions and to disallow it from being abused. The clocks
>> > I use in the (real-world) example in this set are _really_ always-on.
>> > If any of them are turned off the system will cease to perform in any
>> > meaningful way.
>>
>> You cannot tell here up front whether clocks are really always-on. A
>> reviewer is not going to know, and the submitter may not even have all
>> the documentation and know the answer. Getting it wrong here means you
>> have to change the dtb to fix it. Granted, it doesn't really break
>> things in this case.
>
> We should make it clear in the documentation that this framework
> should only be used if the clock is a critical "if it's turned off it
> will cripple the system" one.
[...]
> I think the kernel's policy is a good one i.e. wait until all devices
> are probed and have had the opportunity to take the clocks they need,
> at which point we can usually safely gate the remainder. These types
> of clocks are the exception however; hence the need for this driver.
> There are other vendors which have similar issues with their h/w.
> These are currently using bespoke versions of this implementation, but
> IMO a generic solution would be better.
What kind of clocks are these? What do they control?
Memory controllers? Bus controllers?
They must control some device(s), so there should be one or more device
nodes in DT that reference these clocks.
As soon as that information is in DT, support can be added to Linux to
make sure the "critical" clocks stay enabled, either through a real driver,
or through platform code.
>> Does simple-pm-bus help you?
>
> I have no idea what this is, and I'm struggling to grep for it too?
Rob already provided a pointer.
If you have more questions, feel free to ask!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> On Wed, Feb 18, 2015 at 10:54 PM, Lee Jones <[email protected]> wrote:
> >> >> > +Some hardware is 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.
> >> >>
> >> >> How is (b) not a bug?
> >> >
> >> > Clocks are normally disabled during suspend. When clocks are disabled
> >> > they give up their reference. If references reach 0, the clock is
> >> > gated. If one of these clocks is gated, the system will never come
> >> > out of suspend.
> >> >
> >> > How is it a bug?
> >>
> >> It a clock needs to be enabled during suspend, then the driver using
> >> it should not disable it. Anyway, suspend is a bit orthogonal to this
> >> issue.
> >
> > IMO, it's not the driver's responsibility to know which clock they are
> > using and whether it's a critical clock or not.
>
> So it's (partly) a platform/bus issue.
>
> >> >> While I think we need something here, I worry that this will be abused
> >> >> to be a list of clocks you have not gotten around to managing. We
> >> >
> >> > You can say that about any framework. It's our responsibility to ask
> >> > the right questions and to disallow it from being abused. The clocks
> >> > I use in the (real-world) example in this set are _really_ always-on.
> >> > If any of them are turned off the system will cease to perform in any
> >> > meaningful way.
> >>
> >> You cannot tell here up front whether clocks are really always-on. A
> >> reviewer is not going to know, and the submitter may not even have all
> >> the documentation and know the answer. Getting it wrong here means you
> >> have to change the dtb to fix it. Granted, it doesn't really break
> >> things in this case.
> >
> > We should make it clear in the documentation that this framework
> > should only be used if the clock is a critical "if it's turned off it
> > will cripple the system" one.
>
> [...]
>
> > I think the kernel's policy is a good one i.e. wait until all devices
> > are probed and have had the opportunity to take the clocks they need,
> > at which point we can usually safely gate the remainder. These types
> > of clocks are the exception however; hence the need for this driver.
> > There are other vendors which have similar issues with their h/w.
> > These are currently using bespoke versions of this implementation, but
> > IMO a generic solution would be better.
>
> What kind of clocks are these? What do they control?
> Memory controllers? Bus controllers?
>
> They must control some device(s), so there should be one or more device
> nodes in DT that reference these clocks.
> As soon as that information is in DT, support can be added to Linux to
> make sure the "critical" clocks stay enabled, either through a real driver,
> or through platform code.
Some do, some don't. For instance, we have one clock which controls
SPI and I2C that must not be turned off. We discovered this then when
a suspend was attempted and the board refused to resume. This clock
also runs one of the critical interconnects that runs from the a9. It
would be wrong to remove the clk_disable() attempt from the SPI/I2C
drivers because the same IP on another board might be controlled by a
different clock which is able to be gated.
There are also clocks which control other interconnects that are not
connected to any device drivers. If we fail to take references for
them before clk_disable_unused() is called, again the board hangs. We
even lose JTAG support.
> >> Does simple-pm-bus help you?
> >
> > I have no idea what this is, and I'm struggling to grep for it too?
>
> Rob already provided a pointer.
> If you have more questions, feel free to ask!
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On Thu, Feb 19, 2015 at 10:42 AM, Lee Jones <[email protected]> wrote:
>> What kind of clocks are these? What do they control?
>> Memory controllers? Bus controllers?
>>
>> They must control some device(s), so there should be one or more device
>> nodes in DT that reference these clocks.
>> As soon as that information is in DT, support can be added to Linux to
>> make sure the "critical" clocks stay enabled, either through a real driver,
>> or through platform code.
>
> Some do, some don't. For instance, we have one clock which controls
> SPI and I2C that must not be turned off. We discovered this then when
> a suspend was attempted and the board refused to resume. This clock
> also runs one of the critical interconnects that runs from the a9. It
> would be wrong to remove the clk_disable() attempt from the SPI/I2C
> drivers because the same IP on another board might be controlled by a
> different clock which is able to be gated.
>
> There are also clocks which control other interconnects that are not
> connected to any device drivers. If we fail to take references for
> them before clk_disable_unused() is called, again the board hangs. We
> even lose JTAG support.
Interconnects are buses. Can't you represent those buses in the DT
hierarchy, and give them clocks properties?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, 18 Feb 2015, Rob Herring wrote:
> On Wed, Feb 18, 2015 at 3:54 PM, Lee Jones <[email protected]> wrote:
> > On Wed, 18 Feb 2015, Rob Herring wrote:
> >
> >> On Wed, Feb 18, 2015 at 11:12 AM, Lee Jones <[email protected]> wrote:
> >> > On Wed, 18 Feb 2015, Rob Herring wrote:
> >> >
> >> >> On Wed, Feb 18, 2015 at 10:15 AM, Lee Jones <[email protected]> wrote:
> >> >> > Signed-off-by: Lee Jones <[email protected]>
> >> >> > ---
> >> >> > .../devicetree/bindings/clock/clk-domain.txt | 35 ++++++++++++++++++++++
> >> >> > 1 file changed, 35 insertions(+)
> >> >> > create mode 100644 Documentation/devicetree/bindings/clock/clk-domain.txt
> >> >> >
> >> >> > diff --git a/Documentation/devicetree/bindings/clock/clk-domain.txt b/Documentation/devicetree/bindings/clock/clk-domain.txt
> >> >> > new file mode 100644
> >> >> > index 0000000..b86772f5
> >> >> > --- /dev/null
> >> >> > +++ b/Documentation/devicetree/bindings/clock/clk-domain.txt
> >> >> > @@ -0,0 +1,35 @@
> >> >> > +Always-on Clock Domain
> >> >> > +
> >> >> > +Some hardware is 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.
> >> >>
> >> >> How is (b) not a bug?
> >> >
> >> > Clocks are normally disabled during suspend. When clocks are disabled
> >> > they give up their reference. If references reach 0, the clock is
> >> > gated. If one of these clocks is gated, the system will never come
> >> > out of suspend.
> >> >
> >> > How is it a bug?
> >>
> >> It a clock needs to be enabled during suspend, then the driver using
> >> it should not disable it. Anyway, suspend is a bit orthogonal to this
> >> issue.
> >
> > IMO, it's not the driver's responsibility to know which clock they are
> > using and whether it's a critical clock or not.
>
> Certainly drivers should not know about clocks outside of their h/w
> block, but they absolutely should know if a clock is needed for
> wake-up.
>
> >> >> While I think we need something here, I worry that this will be abused
> >> >> to be a list of clocks you have not gotten around to managing. We
> >> >
> >> > You can say that about any framework. It's our responsibility to ask
> >> > the right questions and to disallow it from being abused. The clocks
> >> > I use in the (real-world) example in this set are _really_ always-on.
> >> > If any of them are turned off the system will cease to perform in any
> >> > meaningful way.
> >>
> >> You cannot tell here up front whether clocks are really always-on. A
> >> reviewer is not going to know, and the submitter may not even have all
> >> the documentation and know the answer. Getting it wrong here means you
> >> have to change the dtb to fix it. Granted, it doesn't really break
> >> things in this case.
> >
> > We should make it clear in the documentation that this framework
> > should only be used if the clock is a critical "if it's turned off it
> > will cripple the system" one.
> >
> >> >> cannot be changing the DT every time the kernel starts managing a
> >> >> clock. I think this should operate more as always on until claimed.
> >> >
> >> > The point of this is that even when these clocks are claimed, there is
> >> > an issue that when unclaimed (i.e. during suspend) the clk framework
> >> > will attempt to gate them, and when they do *boom*.
> >> >
> >> >> But then you get into drivers having to be aware that the clock
> >> >> started enabled.
> >> >
> >> > This has nothing to do with the initial state of the clock. It's
> >> > whether the clock is integral (i.e. is part of a vital interconnect)
> >> > that's important. For instance, ST's bootloader turns on lots of
> >> > clocks which can be safely gated if unused. The clocks we're
> >> > registering with this always-on framework cannot.
> >>
> >> It does because you have to assume either the initial state is wrong
> >> and you need to disable it, or that the initial state is correct and
> >> you need to leave the clock enabled.
> >
> > I think the kernel's policy is a good one i.e. wait until all devices
> > are probed and have had the opportunity to take the clocks they need,
> > at which point we can usually safely gate the remainder. These types
> > of clocks are the exception however; hence the need for this driver.
> > There are other vendors which have similar issues with their h/w.
> > These are currently using bespoke versions of this implementation, but
> > IMO a generic solution would be better.
> >
> >> There are also other usecases such as simplefb where you want to leave
> >> clocks on until the real fb driver takes over. Consoles have a similar
> >> issue.
> >
> > Why wouldn't these devices have taken references by the time
> > clk_disable_unused() is called?
>
> Not if the drivers are modules.
Can you rightfully compile the drivers for your monitor and serial
console as modules and expect them to work until you load them?
> >> Perhaps you need to model your buses more completely?
> >
> > Would you mind explaining this a little more please?
> >
> >> Does simple-pm-bus help you?
> >
> > I have no idea what this is, and I'm struggling to grep for it too?
>
> http://lwn.net/Articles/632058/
>
> I'm not saying this works as-is for you, but people are starting to
> add bus properties to buses.
I'm struggling to see how this might help. Which node would you probe
as simple-pm-bus? It sounds like a very bloated and convoluted way
of saying "don't gate these clocks". Besides, isn't this the opposite
what what we're trying to achieve? We don't want to enable any PM on
these clocks, let along runtime PM. FYI, I also looked into genpd,
but came to the same set of conclusions.
> >> >> Also, I feel like we are using DT to work around kernel policy (of
> >> >> turning off clocks). If the policy was to leave on clocks, then we
> >> >> would be trying to put a list of clocks to disable in DT.
> >> >
> >> > I'm not sure I understand your point. The current policy is correct
> >> > if it's power that you care about, which is invariably the point of
> >> > disabling clocks in the first place, right? Also, this has nothing to
> >> > do with DT per say. It's just another framework driver.
> >>
> >> It does have something to do with DT because you are designing a
> >> binding around what the kernel does. Should the kernel assume it can
> >> disable clocks safely?
> >
> > I guess it depends on what you're trying to achieve. Personally I
> > think the kernel's policy is a good one, especually with regards to
> > power saving. What are you suggesting? A new policy?
>
> No. The binding just has to work no matter what the OS policy.
>
> >> Another OS may do the opposite and assume it
> >> cannot turn-off unused clocks. Then you would have a list of clocks
> >> safe to disable in DT.
> >
> > Sounds bananas. What's good about that kind of policy? It wouldn't
> > matter anyway, both of these implementations can live harmoniously in
> > the same tree.
>
> Your systems won't go *boom*.
>
> >> This is also completely solvable within the framework driver by
> >> claiming those clocks in the clock controller driver.
> >
> > This conversation has now gone full circle. This was an earlier
> > suggestion, but it was considered hacky, and I'm inclined to agree.
>
> The clock maintainer doesn't want hacks in the clock framework and the
> DT maintainer doesn't want them in DT... We should put them in MFD. ;)
>
> > An always-on power domain was deemed to be a much more elegant
> > solution.
>
> Now you are mixing in power domains?
>
> I'm not saying we can't put something in DT. I'm okay with that, but
> it needs to handle the case of the clocks do get claimed either after
> boot (e.g. by a module) or in later kernels without a dtb change.
>
> Rob
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> Hi Lee,
>
> On Thu, Feb 19, 2015 at 10:42 AM, Lee Jones <[email protected]> wrote:
> >> What kind of clocks are these? What do they control?
> >> Memory controllers? Bus controllers?
> >>
> >> They must control some device(s), so there should be one or more device
> >> nodes in DT that reference these clocks.
> >> As soon as that information is in DT, support can be added to Linux to
> >> make sure the "critical" clocks stay enabled, either through a real driver,
> >> or through platform code.
> >
> > Some do, some don't. For instance, we have one clock which controls
> > SPI and I2C that must not be turned off. We discovered this then when
> > a suspend was attempted and the board refused to resume. This clock
> > also runs one of the critical interconnects that runs from the a9. It
> > would be wrong to remove the clk_disable() attempt from the SPI/I2C
> > drivers because the same IP on another board might be controlled by a
> > different clock which is able to be gated.
> >
> > There are also clocks which control other interconnects that are not
> > connected to any device drivers. If we fail to take references for
> > them before clk_disable_unused() is called, again the board hangs. We
> > even lose JTAG support.
>
> Interconnects are buses. Can't you represent those buses in the DT
> hierarchy, and give them clocks properties?
So instead of this nice succinct, simple, cover all bases
(interconnects was just an example, there are bound to be others),
generic framework, you are suggesting to write drivers for devices
which other than "don't turn my clocks off", Linux can't actually see
or control?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On Thu, Feb 19, 2015 at 11:11 AM, Lee Jones <[email protected]> wrote:
> On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
>> On Thu, Feb 19, 2015 at 10:42 AM, Lee Jones <[email protected]> wrote:
>> >> What kind of clocks are these? What do they control?
>> >> Memory controllers? Bus controllers?
>> >>
>> >> They must control some device(s), so there should be one or more device
>> >> nodes in DT that reference these clocks.
>> >> As soon as that information is in DT, support can be added to Linux to
>> >> make sure the "critical" clocks stay enabled, either through a real driver,
>> >> or through platform code.
>> >
>> > Some do, some don't. For instance, we have one clock which controls
>> > SPI and I2C that must not be turned off. We discovered this then when
>> > a suspend was attempted and the board refused to resume. This clock
>> > also runs one of the critical interconnects that runs from the a9. It
>> > would be wrong to remove the clk_disable() attempt from the SPI/I2C
>> > drivers because the same IP on another board might be controlled by a
>> > different clock which is able to be gated.
>> >
>> > There are also clocks which control other interconnects that are not
>> > connected to any device drivers. If we fail to take references for
>> > them before clk_disable_unused() is called, again the board hangs. We
>> > even lose JTAG support.
>>
>> Interconnects are buses. Can't you represent those buses in the DT
>> hierarchy, and give them clocks properties?
>
> So instead of this nice succinct, simple, cover all bases
> (interconnects was just an example, there are bound to be others),
> generic framework, you are suggesting to write drivers for devices
> which other than "don't turn my clocks off", Linux can't actually see
> or control?
DT describes the hardware, not behavior.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> Hi Lee,
>
> On Thu, Feb 19, 2015 at 11:11 AM, Lee Jones <[email protected]> wrote:
> > On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> >> On Thu, Feb 19, 2015 at 10:42 AM, Lee Jones <[email protected]> wrote:
> >> >> What kind of clocks are these? What do they control?
> >> >> Memory controllers? Bus controllers?
> >> >>
> >> >> They must control some device(s), so there should be one or more device
> >> >> nodes in DT that reference these clocks.
> >> >> As soon as that information is in DT, support can be added to Linux to
> >> >> make sure the "critical" clocks stay enabled, either through a real driver,
> >> >> or through platform code.
> >> >
> >> > Some do, some don't. For instance, we have one clock which controls
> >> > SPI and I2C that must not be turned off. We discovered this then when
> >> > a suspend was attempted and the board refused to resume. This clock
> >> > also runs one of the critical interconnects that runs from the a9. It
> >> > would be wrong to remove the clk_disable() attempt from the SPI/I2C
> >> > drivers because the same IP on another board might be controlled by a
> >> > different clock which is able to be gated.
> >> >
> >> > There are also clocks which control other interconnects that are not
> >> > connected to any device drivers. If we fail to take references for
> >> > them before clk_disable_unused() is called, again the board hangs. We
> >> > even lose JTAG support.
> >>
> >> Interconnects are buses. Can't you represent those buses in the DT
> >> hierarchy, and give them clocks properties?
> >
> > So instead of this nice succinct, simple, cover all bases
> > (interconnects was just an example, there are bound to be others),
> > generic framework, you are suggesting to write drivers for devices
> > which other than "don't turn my clocks off", Linux can't actually see
> > or control?
>
> DT describes the hardware, not behavior.
Okay so ...
/*
* ICNs are not visible/controllable in Linux, but references to their
* clocks must be obtained and retained or the platform will become
* irrecoverably unresponsive.
*/
interconnects@0 {
compatible = "always-on-clk-domain";
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>;
};
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On Thu, Feb 19, 2015 at 11:28 AM, Lee Jones <[email protected]> wrote:
> On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
>> On Thu, Feb 19, 2015 at 11:11 AM, Lee Jones <[email protected]> wrote:
>> > On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
>> >> On Thu, Feb 19, 2015 at 10:42 AM, Lee Jones <[email protected]> wrote:
>> >> >> What kind of clocks are these? What do they control?
>> >> >> Memory controllers? Bus controllers?
>> >> >>
>> >> >> They must control some device(s), so there should be one or more device
>> >> >> nodes in DT that reference these clocks.
>> >> >> As soon as that information is in DT, support can be added to Linux to
>> >> >> make sure the "critical" clocks stay enabled, either through a real driver,
>> >> >> or through platform code.
>> >> >
>> >> > Some do, some don't. For instance, we have one clock which controls
>> >> > SPI and I2C that must not be turned off. We discovered this then when
>> >> > a suspend was attempted and the board refused to resume. This clock
>> >> > also runs one of the critical interconnects that runs from the a9. It
>> >> > would be wrong to remove the clk_disable() attempt from the SPI/I2C
>> >> > drivers because the same IP on another board might be controlled by a
>> >> > different clock which is able to be gated.
>> >> >
>> >> > There are also clocks which control other interconnects that are not
>> >> > connected to any device drivers. If we fail to take references for
>> >> > them before clk_disable_unused() is called, again the board hangs. We
>> >> > even lose JTAG support.
>> >>
>> >> Interconnects are buses. Can't you represent those buses in the DT
>> >> hierarchy, and give them clocks properties?
>> >
>> > So instead of this nice succinct, simple, cover all bases
>> > (interconnects was just an example, there are bound to be others),
>> > generic framework, you are suggesting to write drivers for devices
>> > which other than "don't turn my clocks off", Linux can't actually see
>> > or control?
>>
>> DT describes the hardware, not behavior.
>
> Okay so ...
>
> /*
> * ICNs are not visible/controllable in Linux, but references to their
> * clocks must be obtained and retained or the platform will become
> * irrecoverably unresponsive.
> */
> interconnects@0 {
> compatible = "always-on-clk-domain";
st,...flexgen...
> 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>;
> };
And then you can have platform code that binds against st,...flexgen...,
and enables all referenced clocks.
Alternatively, if you have power domains, you can add a reference to
the power domain, and let the power domain driver handle it.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> Hi Lee,
>
> On Thu, Feb 19, 2015 at 11:28 AM, Lee Jones <[email protected]> wrote:
> > On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> >> On Thu, Feb 19, 2015 at 11:11 AM, Lee Jones <[email protected]> wrote:
> >> > On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> >> >> On Thu, Feb 19, 2015 at 10:42 AM, Lee Jones <[email protected]> wrote:
> >> >> >> What kind of clocks are these? What do they control?
> >> >> >> Memory controllers? Bus controllers?
> >> >> >>
> >> >> >> They must control some device(s), so there should be one or more device
> >> >> >> nodes in DT that reference these clocks.
> >> >> >> As soon as that information is in DT, support can be added to Linux to
> >> >> >> make sure the "critical" clocks stay enabled, either through a real driver,
> >> >> >> or through platform code.
> >> >> >
> >> >> > Some do, some don't. For instance, we have one clock which controls
> >> >> > SPI and I2C that must not be turned off. We discovered this then when
> >> >> > a suspend was attempted and the board refused to resume. This clock
> >> >> > also runs one of the critical interconnects that runs from the a9. It
> >> >> > would be wrong to remove the clk_disable() attempt from the SPI/I2C
> >> >> > drivers because the same IP on another board might be controlled by a
> >> >> > different clock which is able to be gated.
> >> >> >
> >> >> > There are also clocks which control other interconnects that are not
> >> >> > connected to any device drivers. If we fail to take references for
> >> >> > them before clk_disable_unused() is called, again the board hangs. We
> >> >> > even lose JTAG support.
> >> >>
> >> >> Interconnects are buses. Can't you represent those buses in the DT
> >> >> hierarchy, and give them clocks properties?
> >> >
> >> > So instead of this nice succinct, simple, cover all bases
> >> > (interconnects was just an example, there are bound to be others),
> >> > generic framework, you are suggesting to write drivers for devices
> >> > which other than "don't turn my clocks off", Linux can't actually see
> >> > or control?
> >>
> >> DT describes the hardware, not behavior.
> >
> > Okay so ...
> >
> > /*
> > * ICNs are not visible/controllable in Linux, but references to their
> > * clocks must be obtained and retained or the platform will become
> > * irrecoverably unresponsive.
> > */
> > interconnects@0 {
> > compatible = "always-on-clk-domain";
>
> st,...flexgen...
>
> > 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>;
> > };
>
> And then you can have platform code that binds against st,...flexgen...,
> and enables all referenced clocks.
Flexgen isn't a device, it's a clk source. a) writing a device driver
for a clk source seems wrong b) what if on another platform a
different clock source supplied the clock? Write another driver? And
what if the ICNs are connected to different clock sources? More
drivers? c) all of these drivers will only do one thing -- pull a
reference and keep hold of it. You want 50 drivers (across all
platforms) doing only that? Or, more sanely, do you want this one
generic framework driver doing that?
> Alternatively, if you have power domains, you can add a reference to
> the power domain, and let the power domain driver handle it.
I'm not sure what a power domain driver will do? We need a driver to
_not_ give up references, that is all. :)
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On Thu, Feb 19, 2015 at 11:43 AM, Lee Jones <[email protected]> wrote:
> On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
>> On Thu, Feb 19, 2015 at 11:28 AM, Lee Jones <[email protected]> wrote:
>> > On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
>> >> On Thu, Feb 19, 2015 at 11:11 AM, Lee Jones <[email protected]> wrote:
>> >> > On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
>> >> >> On Thu, Feb 19, 2015 at 10:42 AM, Lee Jones <[email protected]> wrote:
>> >> >> >> What kind of clocks are these? What do they control?
>> >> >> >> Memory controllers? Bus controllers?
>> >> >> >>
>> >> >> >> They must control some device(s), so there should be one or more device
>> >> >> >> nodes in DT that reference these clocks.
>> >> >> >> As soon as that information is in DT, support can be added to Linux to
>> >> >> >> make sure the "critical" clocks stay enabled, either through a real driver,
>> >> >> >> or through platform code.
>> >> >> >
>> >> >> > Some do, some don't. For instance, we have one clock which controls
>> >> >> > SPI and I2C that must not be turned off. We discovered this then when
>> >> >> > a suspend was attempted and the board refused to resume. This clock
>> >> >> > also runs one of the critical interconnects that runs from the a9. It
>> >> >> > would be wrong to remove the clk_disable() attempt from the SPI/I2C
>> >> >> > drivers because the same IP on another board might be controlled by a
>> >> >> > different clock which is able to be gated.
>> >> >> >
>> >> >> > There are also clocks which control other interconnects that are not
>> >> >> > connected to any device drivers. If we fail to take references for
>> >> >> > them before clk_disable_unused() is called, again the board hangs. We
>> >> >> > even lose JTAG support.
>> >> >>
>> >> >> Interconnects are buses. Can't you represent those buses in the DT
>> >> >> hierarchy, and give them clocks properties?
>> >> >
>> >> > So instead of this nice succinct, simple, cover all bases
>> >> > (interconnects was just an example, there are bound to be others),
>> >> > generic framework, you are suggesting to write drivers for devices
>> >> > which other than "don't turn my clocks off", Linux can't actually see
>> >> > or control?
>> >>
>> >> DT describes the hardware, not behavior.
>> >
>> > Okay so ...
>> >
>> > /*
>> > * ICNs are not visible/controllable in Linux, but references to their
>> > * clocks must be obtained and retained or the platform will become
>> > * irrecoverably unresponsive.
>> > */
>> > interconnects@0 {
>> > compatible = "always-on-clk-domain";
>>
>> st,...flexgen...
>>
>> > 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>;
>> > };
>>
>> And then you can have platform code that binds against st,...flexgen...,
>> and enables all referenced clocks.
>
> Flexgen isn't a device, it's a clk source. a) writing a device driver
Sorry, I'm not familiar with ST nomenclature.
So that should become the name of the interconnect/bus.
> for a clk source seems wrong b) what if on another platform a
> different clock source supplied the clock? Write another driver? And
> what if the ICNs are connected to different clock sources? More
> drivers? c) all of these drivers will only do one thing -- pull a
> reference and keep hold of it. You want 50 drivers (across all
> platforms) doing only that? Or, more sanely, do you want this one
> generic framework driver doing that?
>
>> Alternatively, if you have power domains, you can add a reference to
>> the power domain, and let the power domain driver handle it.
>
> I'm not sure what a power domain driver will do? We need a driver to
> _not_ give up references, that is all. :)
A power domain driver can do anything it wants.
That includes enabling your interconnect clocks, and keeping them enabled.
Now, if flexgen is the name of the clock source, then all devices connected
to flexgen are part of the flexgen clock domain, which can be represented
in Linux using the generic PM domain. Do all devices connected to flexgen
need to be handled similarly w.r.t. clocks? If yes, the genpd may be the place
to implement that.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> On Thu, Feb 19, 2015 at 11:43 AM, Lee Jones <[email protected]> wrote:
> > On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> >> On Thu, Feb 19, 2015 at 11:28 AM, Lee Jones <[email protected]> wrote:
> >> > On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> >> >> On Thu, Feb 19, 2015 at 11:11 AM, Lee Jones <[email protected]> wrote:
> >> >> > On Thu, 19 Feb 2015, Geert Uytterhoeven wrote:
> >> >> >> On Thu, Feb 19, 2015 at 10:42 AM, Lee Jones <[email protected]> wrote:
> >> >> >> >> What kind of clocks are these? What do they control?
> >> >> >> >> Memory controllers? Bus controllers?
> >> >> >> >>
> >> >> >> >> They must control some device(s), so there should be one or more device
> >> >> >> >> nodes in DT that reference these clocks.
> >> >> >> >> As soon as that information is in DT, support can be added to Linux to
> >> >> >> >> make sure the "critical" clocks stay enabled, either through a real driver,
> >> >> >> >> or through platform code.
> >> >> >> >
> >> >> >> > Some do, some don't. For instance, we have one clock which controls
> >> >> >> > SPI and I2C that must not be turned off. We discovered this then when
> >> >> >> > a suspend was attempted and the board refused to resume. This clock
> >> >> >> > also runs one of the critical interconnects that runs from the a9. It
> >> >> >> > would be wrong to remove the clk_disable() attempt from the SPI/I2C
> >> >> >> > drivers because the same IP on another board might be controlled by a
> >> >> >> > different clock which is able to be gated.
> >> >> >> >
> >> >> >> > There are also clocks which control other interconnects that are not
> >> >> >> > connected to any device drivers. If we fail to take references for
> >> >> >> > them before clk_disable_unused() is called, again the board hangs. We
> >> >> >> > even lose JTAG support.
> >> >> >>
> >> >> >> Interconnects are buses. Can't you represent those buses in the DT
> >> >> >> hierarchy, and give them clocks properties?
> >> >> >
> >> >> > So instead of this nice succinct, simple, cover all bases
> >> >> > (interconnects was just an example, there are bound to be others),
> >> >> > generic framework, you are suggesting to write drivers for devices
> >> >> > which other than "don't turn my clocks off", Linux can't actually see
> >> >> > or control?
> >> >>
> >> >> DT describes the hardware, not behavior.
> >> >
> >> > Okay so ...
> >> >
> >> > /*
> >> > * ICNs are not visible/controllable in Linux, but references to their
> >> > * clocks must be obtained and retained or the platform will become
> >> > * irrecoverably unresponsive.
> >> > */
> >> > interconnects@0 {
> >> > compatible = "always-on-clk-domain";
> >>
> >> st,...flexgen...
> >>
> >> > 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>;
> >> > };
> >>
> >> And then you can have platform code that binds against st,...flexgen...,
> >> and enables all referenced clocks.
> >
> > Flexgen isn't a device, it's a clk source. a) writing a device driver
>
> Sorry, I'm not familiar with ST nomenclature.
> So that should become the name of the interconnect/bus.
You're still talking about writing function-less drivers for multiple
pieces of h/w. We would have a few for ST alone, then multiply that
by the number of silicon vendors with similar issues -- which is
likely to be most of them.
I can understand Rob's "DT has to match h/w" point, but to insist we
write lots of empty drivers just to stop some clocks from being gated
is barking mad.
> > for a clk source seems wrong b) what if on another platform a
> > different clock source supplied the clock? Write another driver? And
> > what if the ICNs are connected to different clock sources? More
> > drivers? c) all of these drivers will only do one thing -- pull a
> > reference and keep hold of it. You want 50 drivers (across all
> > platforms) doing only that? Or, more sanely, do you want this one
> > generic framework driver doing that?
> >
> >> Alternatively, if you have power domains, you can add a reference to
> >> the power domain, and let the power domain driver handle it.
> >
> > I'm not sure what a power domain driver will do? We need a driver to
> > _not_ give up references, that is all. :)
>
> A power domain driver can do anything it wants.
> That includes enabling your interconnect clocks, and keeping them enabled.
>
> Now, if flexgen is the name of the clock source, then all devices connected
> to flexgen are part of the flexgen clock domain, which can be represented
> in Linux using the generic PM domain. Do all devices connected to flexgen
> need to be handled similarly w.r.t. clocks? If yes, the genpd may be the place
> to implement that.
Most of the FLEXGEN clocks are fully gateable. It's only a select few
which are critical to the running of the system.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On Wed, 18 Feb 2015, Lee Jones wrote:
> +/*
> + * ST Clock Domain
minor nit, as v2 is a generic driver, comment needs updating.
regards,
Peter.
Quoting Lee Jones (2015-02-18 08:15:00)
> Much h/w 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 <[email protected]>
> ---
> drivers/clk/Makefile | 1 +
> drivers/clk/clkdomain.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 64 insertions(+)
> create mode 100644 drivers/clk/clkdomain.c
>
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index d5fba5b..d9e2718 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) += clkdomain.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/clkdomain.c b/drivers/clk/clkdomain.c
> new file mode 100644
> index 0000000..8c83f99
> --- /dev/null
> +++ b/drivers/clk/clkdomain.c
Naming is hard. I'm not sure clkdomain.c is the best expression. Maybe
clk-alwon.c? I see ALWON used alot amongst hardware people who live in a
world of eight-character naming limitations.
> @@ -0,0 +1,63 @@
> +/*
> + * ST Clock Domain
> + *
> + * Copyright (C) 2015 STMicroelectronics – All Rights Reserved
> + *
> + * Author: Lee Jones <[email protected]>
> + *
> + * 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-private.h>
If this header still existed I would berate you mercilessly. Luckily for
you it no longer exists and only causes a compile error ;-)
> +#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->name);
> +}
> +
> +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");
Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
minutes writing that function and I need people to use it so I can get a
return on my investment.
Otherwise the patch looks good. I believe that this method is targeting
always-on clock in a production environment, which is different from the
CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
hardware or dealing with a platform that has incomplete driver support.
I wonder if there is a clever way for existing clock providers
(expressed in DT) to use this without having to create a separate node
of clocks with the "always-on-clk-domain" flag. Possibly the common
clock binding could declare some always-on flag that is standardized?
Then the framework core could use this code easily. Not sure if that is
a good idea though...
Regards,
Mike
> +
> + 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 = "always-on-clk-domain" },
> + { },
> +};
> +
> +static struct platform_driver ao_clock_domain_driver = {
> + .probe = ao_clock_domain_probe,
> + .driver = {
> + .name = "always-on-clk-domain",
> + .of_match_table = ao_clock_domain_match,
> + },
> +};
> +module_platform_driver(ao_clock_domain_driver);
> --
> 1.9.1
>
On Mon, 23 Feb 2015, Mike Turquette wrote:
> Quoting Lee Jones (2015-02-18 08:15:00)
> > Much h/w 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 <[email protected]>
> > ---
> > drivers/clk/Makefile | 1 +
> > drivers/clk/clkdomain.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 64 insertions(+)
> > create mode 100644 drivers/clk/clkdomain.c
> >
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index d5fba5b..d9e2718 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) += clkdomain.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/clkdomain.c b/drivers/clk/clkdomain.c
> > new file mode 100644
> > index 0000000..8c83f99
> > --- /dev/null
> > +++ b/drivers/clk/clkdomain.c
>
> Naming is hard. I'm not sure clkdomain.c is the best expression. Maybe
> clk-alwon.c? I see ALWON used alot amongst hardware people who live in a
> world of eight-character naming limitations.
If you can have clk-fractional-divider.c in your subsystem, I'm sure
clk-always-on.c will be suitable.
> > @@ -0,0 +1,63 @@
> > +/*
> > + * Always on Clock Domain
=;-)
> > + * Copyright (C) 2015 STMicroelectronics – All Rights Reserved
> > + *
> > + * Author: Lee Jones <[email protected]>
> > + *
> > + * 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-private.h>
>
> If this header still existed I would berate you mercilessly. Luckily for
> you it no longer exists and only causes a compile error ;-)
Noted.
You may wish to update: Documentation/clk.txt accordingly.
> > +#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->name);
> > +}
> > +
> > +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");
>
> Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
> minutes writing that function and I need people to use it so I can get a
> return on my investment.
My middle name is RoI. I'm on it.
> Otherwise the patch looks good. I believe that this method is targeting
> always-on clock in a production environment, which is different from the
> CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
> hardware or dealing with a platform that has incomplete driver support.
>
> I wonder if there is a clever way for existing clock providers
> (expressed in DT) to use this without having to create a separate node
> of clocks with the "always-on-clk-domain" flag. Possibly the common
> clock binding could declare some always-on flag that is standardized?
> Then the framework core could use this code easily. Not sure if that is
> a good idea though...
I think having both would be a good idea. If all clocks supplied by a
provider should be left on, then a property inside the provider node
could be a good way to describe that. In our case only some of them
are required, so I consider this concept to be better.
> > +
> > + 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 = "always-on-clk-domain" },
> > + { },
> > +};
> > +
> > +static struct platform_driver ao_clock_domain_driver = {
> > + .probe = ao_clock_domain_probe,
> > + .driver = {
> > + .name = "always-on-clk-domain",
> > + .of_match_table = ao_clock_domain_match,
> > + },
> > +};
> > +module_platform_driver(ao_clock_domain_driver);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Feb 23, 2015 at 11:23 AM, Mike Turquette <[email protected]> wrote:
> Quoting Lee Jones (2015-02-18 08:15:00)
>> Much h/w 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.
[...]
>> +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");
>
> Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
> minutes writing that function and I need people to use it so I can get a
> return on my investment.
>
> Otherwise the patch looks good. I believe that this method is targeting
> always-on clock in a production environment, which is different from the
> CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
> hardware or dealing with a platform that has incomplete driver support.
There is also the usecase of keep clocks on until I load a module that
properly handles my hardware (e.g simplefb). We have a simplefb node
with clocks and the simplefb driver jumps thru some hoops to hand-off
clocks to the real driver. I don't really like it and don't want to
see more examples. And there is the case of I thought I would never
manage this clock, but kernel subsystems evolve and now I want to
manage a clock. This should not require a DT update to do so.
Neither of these may be Lee's usecase, but I want to see them covered
by the binding.
> I wonder if there is a clever way for existing clock providers
> (expressed in DT) to use this without having to create a separate node
> of clocks with the "always-on-clk-domain" flag. Possibly the common
> clock binding could declare some always-on flag that is standardized?
> Then the framework core could use this code easily. Not sure if that is
> a good idea though...
I would prefer to see the always on clocks just listed within the
clock controller's node rather than creating made up nodes with clock
properties. This should be always-on until claimed IMO, but that
aspect is the OS's problem, not a DT problem.
Rob
On Wed, 25 Feb 2015, Rob Herring wrote:
> On Mon, Feb 23, 2015 at 11:23 AM, Mike Turquette <[email protected]> wrote:
> > Quoting Lee Jones (2015-02-18 08:15:00)
> >> Much h/w 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.
>
> [...]
>
> >> +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");
> >
> > Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
> > minutes writing that function and I need people to use it so I can get a
> > return on my investment.
> >
> > Otherwise the patch looks good. I believe that this method is targeting
> > always-on clock in a production environment, which is different from the
> > CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
> > hardware or dealing with a platform that has incomplete driver support.
>
> There is also the usecase of keep clocks on until I load a module that
> properly handles my hardware (e.g simplefb). We have a simplefb node
> with clocks and the simplefb driver jumps thru some hoops to hand-off
> clocks to the real driver. I don't really like it and don't want to
> see more examples. And there is the case of I thought I would never
> manage this clock, but kernel subsystems evolve and now I want to
> manage a clock. This should not require a DT update to do so.
>
> Neither of these may be Lee's usecase, but I want to see them covered
> by the binding.
>
> > I wonder if there is a clever way for existing clock providers
> > (expressed in DT) to use this without having to create a separate node
> > of clocks with the "always-on-clk-domain" flag. Possibly the common
> > clock binding could declare some always-on flag that is standardized?
> > Then the framework core could use this code easily. Not sure if that is
> > a good idea though...
>
> I would prefer to see the always on clocks just listed within the
> clock controller's node rather than creating made up nodes with clock
> properties.
> This should be always-on until claimed IMO, but that
> aspect is the OS's problem, not a DT problem.
I disagree with this point. There are likely to be many unclaimed,
but perfectly gateable clocks in a system, which will consume power
unnecessarily. The clk framework does the right thing by turning all
unclaimed clocks off IMHO. This only leaves a small use-case where we
need to artificially claim some which must not be gated.
The other way to do is, as you mentioned is list the clocks which must
stay on in the clock source node, but this will still require a
binding. It will also require a much more complicated framework
driver.
clkprovider@xxxxxxxx {
always-on-clks = <1, 2, 4, 5, 7>;
};
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Quoting Lee Jones (2015-02-25 07:48:08)
> On Wed, 25 Feb 2015, Rob Herring wrote:
>
> > On Mon, Feb 23, 2015 at 11:23 AM, Mike Turquette <[email protected]> wrote:
> > > Quoting Lee Jones (2015-02-18 08:15:00)
> > >> Much h/w 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.
> >
> > [...]
> >
> > >> +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");
> > >
> > > Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
> > > minutes writing that function and I need people to use it so I can get a
> > > return on my investment.
> > >
> > > Otherwise the patch looks good. I believe that this method is targeting
> > > always-on clock in a production environment, which is different from the
> > > CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
> > > hardware or dealing with a platform that has incomplete driver support.
> >
> > There is also the usecase of keep clocks on until I load a module that
> > properly handles my hardware (e.g simplefb). We have a simplefb node
> > with clocks and the simplefb driver jumps thru some hoops to hand-off
> > clocks to the real driver. I don't really like it and don't want to
> > see more examples. And there is the case of I thought I would never
> > manage this clock, but kernel subsystems evolve and now I want to
> > manage a clock. This should not require a DT update to do so.
> >
> > Neither of these may be Lee's usecase, but I want to see them covered
> > by the binding.
> >
> > > I wonder if there is a clever way for existing clock providers
> > > (expressed in DT) to use this without having to create a separate node
> > > of clocks with the "always-on-clk-domain" flag. Possibly the common
> > > clock binding could declare some always-on flag that is standardized?
> > > Then the framework core could use this code easily. Not sure if that is
> > > a good idea though...
> >
> > I would prefer to see the always on clocks just listed within the
> > clock controller's node rather than creating made up nodes with clock
> > properties.
>
> > This should be always-on until claimed IMO, but that
> > aspect is the OS's problem, not a DT problem.
>
> I disagree with this point. There are likely to be many unclaimed,
> but perfectly gateable clocks in a system, which will consume power
> unnecessarily. The clk framework does the right thing by turning all
> unclaimed clocks off IMHO. This only leaves a small use-case where we
> need to artificially claim some which must not be gated.
I might have misread both of your mails, but I think you two are
actually in agreement. You both support a common property which lists
the always-on clocks inside of the common clock binding, no?
>
> The other way to do is, as you mentioned is list the clocks which must
> stay on in the clock source node, but this will still require a
> binding. It will also require a much more complicated framework
> driver.
>
> clkprovider@xxxxxxxx {
> always-on-clks = <1, 2, 4, 5, 7>;
> };
This should pose no burden on the driver. Since always-on-clks is in the
common clock binding it should be handled by the framework core. At
clk_register-time we can check for always-on-clks, walk the list and see
if we have a match. It's ugly O(n^2) but it works.
Thoughts?
Mike
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog