On SoCs like ARM/SH-mobile, gate clocks are available for modules, allowing
Runtime PM for a device controlled by a gate clock.
On legacy shmobile kernels, this is handled by the PM runtime code in
drivers/sh/pm_runtime.c, which installs a clock notifier for the platform
bus, registering the "NULL" clock of each platform device with the PM core.
This approach is also used on davinci, keystone, and omap1.
On multi-platform shmobile kernels, this was not handled at all, leading
to spurious disabled clocks on drivers relying on Runtime PM, depending on
implicit reset state, or on the bootloader.
A first solution, enabling the PM runtime code in drivers/sh/pm_runtime.c
in a multi-platform-safe way, was provided by the patch series
"[PATCH v2 00/17] ARM: shmobile: Enable drivers/sh/pm_runtime.c on
multi-platform" (http://www.spinics.net/lists/linux-sh/msg30887.html).
Here is an alternative approach, avoiding the reliance on C board files,
which are being phased out.
This is also related to a patch series by Felipe Balbi ("[RFC/PATCH] base:
platform: add generic clock handling for platform-bus",
https://lkml.org/lkml/2014/1/31/290)
This series:
1. Lets the MSTP clock driver indicate that its clocks are suitable for
Runtime PM,
2. Lets the DT code retrieve clock information when adding a device
(it already retrieves information for resources (registers, irq) ---
unfortunately clocks are not resources), and registering clocks
suitable for Runtime PM with the PM core.
If Runtime PM is disabled, the clocks are just enabled.
Note that this works for devices instantiated from DT only.
Fortunately the drivers for the remaining platform devices (SCI and CMT)
handle clocks theirselves, without Runtime PM, so they get properly enabled.
Patches:
- [1/4] clk: Add CLK_RUNTIME_PM and clk_may_runtime_pm()
- [2/4] PM / clock_ops: Add pm_clk_add_clk()
- [3/4] of/clk: Register clocks suitable for Runtime PM with the
- [4/4] clk: shmobile: mstp: Set CLK_RUNTIME_PM flag
This series was tested on Renesas r8a7791, using the Koelsch development
board.
Thanks for your comments!
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
When adding a device from DT, check if its clocks are suitable for Runtime
PM, and register them with the PM core.
If Runtime PM is disabled, just enable the clock.
This allows the PM core to automatically manage gate clocks of devices for
Runtime PM.
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
drivers/of/Makefile | 1 +
drivers/of/of_clk.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/of/platform.c | 3 ++
include/linux/of_clk.h | 18 +++++++++
4 files changed, 125 insertions(+)
create mode 100644 drivers/of/of_clk.c
create mode 100644 include/linux/of_clk.h
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index ed9660adad77..49bcd413906f 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI) += of_pci.o
obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
obj-$(CONFIG_OF_MTD) += of_mtd.o
obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
+obj-$(CONFIG_COMMON_CLK) += of_clk.o
diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
new file mode 100644
index 000000000000..35f5e9f3dd42
--- /dev/null
+++ b/drivers/of/of_clk.c
@@ -0,0 +1,103 @@
+/*
+ * Copyright (C) 2014 Glider bvba
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_clk.h>
+#include <linux/platform_device.h>
+#include <linux/pm_clock.h>
+#include <linux/pm_runtime.h>
+
+
+#ifdef CONFIG_PM_RUNTIME
+
+static int of_clk_pm_runtime_suspend(struct device *dev)
+{
+ int ret;
+
+ ret = pm_generic_runtime_suspend(dev);
+ if (ret)
+ return ret;
+
+ ret = pm_clk_suspend(dev);
+ if (ret) {
+ pm_generic_runtime_resume(dev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int of_clk_pm_runtime_resume(struct device *dev)
+{
+ pm_clk_resume(dev);
+ return pm_generic_runtime_resume(dev);
+}
+
+static struct dev_pm_domain of_clk_pm_domain = {
+ .ops = {
+ .runtime_suspend = of_clk_pm_runtime_suspend,
+ .runtime_resume = of_clk_pm_runtime_resume,
+ USE_PLATFORM_PM_SLEEP_OPS
+ },
+};
+
+static int of_clk_register(struct device *dev, struct clk *clk)
+{
+ int error;
+
+ if (!dev->pm_domain) {
+ error = pm_clk_create(dev);
+ if (error)
+ return error;
+
+ dev->pm_domain = &of_clk_pm_domain;
+ }
+
+ dev_dbg(dev, "Setting up clock for runtime PM management\n");
+ return pm_clk_add_clk(dev, clk);
+}
+
+#else /* !CONFIG_PM_RUNTIME */
+
+static int of_clk_register(struct device *dev, struct clk *clk)
+{
+ dev_dbg(dev, "Runtime PM disabled, enabling clock\n");
+ return clk_prepare_enable(clk);
+}
+
+#endif /* !CONFIG_PM_RUNTIME */
+
+
+/**
+ * of_clk_register_runtime_pm_clocks - Register clocks suitable for Runtime PM
+ * with the PM core
+ * @np: pointer to device tree node
+ * @pdev: pointer to corresponding device to register suitable clocks for
+ *
+ * Returns an error code
+ */
+int of_clk_register_runtime_pm_clocks(struct device_node *np,
+ struct device *dev)
+{
+ unsigned int i;
+ struct clk *clk;
+ int error;
+
+ for (i = 0; (clk = of_clk_get(np, i)) && !IS_ERR(clk); i++) {
+ if (!clk_may_runtime_pm(clk)) {
+ clk_put(clk);
+ continue;
+ }
+
+ error = of_clk_register(dev, clk);
+ if (error) {
+ clk_put(clk);
+ return error;
+ }
+ }
+
+ return 0;
+}
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1daebefa..29145302b6f8 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -18,6 +18,7 @@
#include <linux/dma-mapping.h>
#include <linux/slab.h>
#include <linux/of_address.h>
+#include <linux/of_clk.h>
#include <linux/of_device.h>
#include <linux/of_irq.h>
#include <linux/of_platform.h>
@@ -182,6 +183,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
else
of_device_make_bus_id(&dev->dev);
+ of_clk_register_runtime_pm_clocks(np, &dev->dev);
+
return dev;
}
EXPORT_SYMBOL(of_device_alloc);
diff --git a/include/linux/of_clk.h b/include/linux/of_clk.h
new file mode 100644
index 000000000000..b9b15614e39b
--- /dev/null
+++ b/include/linux/of_clk.h
@@ -0,0 +1,18 @@
+#ifndef _LINUX_OF_CLK_H
+#define _LINUX_OF_CLK_H
+
+struct device_node;
+struct device;
+
+#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
+int of_clk_register_runtime_pm_clocks(struct device_node *np,
+ struct device *dev);
+#else
+static inline int of_clk_register_runtime_pm_clocks(struct device_node *np,
+ struct device *dev)
+{
+ return 0;
+}
+#endif /* CONFIG_OF && CONFIG_COMMON_CLK */
+
+#endif /* _LINUX_OF_CLK_H */
--
1.7.9.5
The existing pm_clk_add() allows to pass a clock by con_id. However,
when referring to a specific clock from DT, no con_id is available.
Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
drivers/base/power/clock_ops.c | 40 ++++++++++++++++++++++++++++++----------
include/linux/pm_clock.h | 3 +++
2 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index b99e6c06ee67..2d5c9c1eceb1 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
*/
static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
{
- ce->clk = clk_get(dev, ce->con_id);
+ if (!ce->clk)
+ ce->clk = clk_get(dev, ce->con_id);
if (IS_ERR(ce->clk)) {
ce->status = PCE_STATUS_ERROR;
} else {
@@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
}
}
-/**
- * pm_clk_add - Start using a device clock for power management.
- * @dev: Device whose clock is going to be used for power management.
- * @con_id: Connection ID of the clock.
- *
- * Add the clock represented by @con_id to the list of clocks used for
- * the power management of @dev.
- */
-int pm_clk_add(struct device *dev, const char *con_id)
+static int __pm_clk_add(struct device *dev, const char *con_id,
+ struct clk *clk)
{
struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce;
@@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
kfree(ce);
return -ENOMEM;
}
+ } else {
+ ce->clk = clk;
}
pm_clk_acquire(dev, ce);
@@ -102,6 +98,30 @@ int pm_clk_add(struct device *dev, const char *con_id)
spin_unlock_irq(&psd->lock);
return 0;
}
+/**
+ * pm_clk_add - Start using a device clock for power management.
+ * @dev: Device whose clock is going to be used for power management.
+ * @con_id: Connection ID of the clock.
+ *
+ * Add the clock represented by @con_id to the list of clocks used for
+ * the power management of @dev.
+ */
+int pm_clk_add(struct device *dev, const char *con_id)
+{
+ return __pm_clk_add(dev, con_id, NULL);
+}
+
+/**
+ * pm_clk_add_clk - Start using a device clock for power management.
+ * @dev: Device whose clock is going to be used for power management.
+ * @clk: Clock pointer
+ *
+ * Add the clock to the list of clocks used for the power management of @dev.
+ */
+int pm_clk_add_clk(struct device *dev, struct clk *clk)
+{
+ return __pm_clk_add(dev, NULL, clk);
+}
/**
* __pm_clk_remove - Destroy PM clock entry.
diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h
index 8348866e7b05..6981aa288c45 100644
--- a/include/linux/pm_clock.h
+++ b/include/linux/pm_clock.h
@@ -18,6 +18,8 @@ struct pm_clk_notifier_block {
char *con_ids[];
};
+struct clk;
+
#ifdef CONFIG_PM_CLK
static inline bool pm_clk_no_clocks(struct device *dev)
{
@@ -29,6 +31,7 @@ extern void pm_clk_init(struct device *dev);
extern int pm_clk_create(struct device *dev);
extern void pm_clk_destroy(struct device *dev);
extern int pm_clk_add(struct device *dev, const char *con_id);
+extern int pm_clk_add_clk(struct device *dev, struct clk *clk);
extern void pm_clk_remove(struct device *dev, const char *con_id);
extern int pm_clk_suspend(struct device *dev);
extern int pm_clk_resume(struct device *dev);
--
1.7.9.5
Add a flag CLK_RUNTIME_PM, to let low-level clock drivers indicate that
a clock is suitable for Runtime PM.
Add clk_may_runtime_pm(), to get the status of the flag.
This will allow the device core to enable automatic Runtime PM management
for devices tied to clocks that are suitable for Runtime PM.
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
drivers/clk/clk.c | 12 ++++++++++++
include/linux/clk-provider.h | 1 +
include/linux/clk.h | 1 +
3 files changed, 14 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0b2819551756..a83a2cc0af67 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1115,6 +1115,18 @@ long clk_get_accuracy(struct clk *clk)
EXPORT_SYMBOL_GPL(clk_get_accuracy);
/**
+ * clk_may_runtime_pm - check if the clock is suitable for Runtime PM
+ * @clk: the clock to check
+ *
+ * Return true if the clock is suitable for Runtime PM
+ * Return false if clk is NULL, or if the clock is not suitable for Runtime PM.
+ */
+bool clk_may_runtime_pm(const struct clk *clk)
+{
+ return clk && clk->flags & CLK_RUNTIME_PM;
+}
+
+/**
* __clk_recalc_rates
* @clk: first clk in the subtree
* @msg: notification type (see include/linux/clk.h)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 24c8ba4fa6ae..3ca9a7c1f02d 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -30,6 +30,7 @@
#define CLK_GET_RATE_NOCACHE BIT(6) /* do not use the cached clk rate */
#define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
#define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
+#define CLK_RUNTIME_PM BIT(9) /* clock is suitable for Runtime PM */
struct clk_hw;
struct dentry;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index d030fce1d77e..07f156580064 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -106,6 +106,7 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
*/
long clk_get_accuracy(struct clk *clk);
+bool clk_may_runtime_pm(const struct clk *clk);
#else /* !CONFIG_COMMON_CLK */
static inline long clk_get_accuracy(struct clk *clk)
--
1.7.9.5
Renesas MSTP (Module Stop) clocks are suitable for Runtime PM.
Hence set the CLK_RUNTIME_PM flag, to make of_clk enable automatic Runtime
PM management for DT devices that are tied to an MSTP clock.
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
drivers/clk/shmobile/clk-mstp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c
index 2e3c08fff173..6f860bad551e 100644
--- a/drivers/clk/shmobile/clk-mstp.c
+++ b/drivers/clk/shmobile/clk-mstp.c
@@ -139,7 +139,7 @@ cpg_mstp_clock_register(const char *name, const char *parent_name,
init.name = name;
init.ops = &cpg_mstp_clock_ops;
- init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+ init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT | CLK_RUNTIME_PM;
init.parent_names = &parent_name;
init.num_parents = 1;
--
1.7.9.5
On 24 April 2014 12:13, Geert Uytterhoeven <[email protected]> wrote:
> When adding a device from DT, check if its clocks are suitable for Runtime
> PM, and register them with the PM core.
> If Runtime PM is disabled, just enable the clock.
>
> This allows the PM core to automatically manage gate clocks of devices for
> Runtime PM.
Normally I don't think it's a good idea to "automatically" manage
clocks from PM core or any other place but from the driver (and
possibly the subsystem).
The reason is simply that we hide things that normally is supposed to
be handled by the driver. Typically a cross SOC driver should work
fine both with and without a pm_domain. It should also not rely on
CONFIG_PM_RUNTIME.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> drivers/of/Makefile | 1 +
> drivers/of/of_clk.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/of/platform.c | 3 ++
> include/linux/of_clk.h | 18 +++++++++
> 4 files changed, 125 insertions(+)
> create mode 100644 drivers/of/of_clk.c
> create mode 100644 include/linux/of_clk.h
>
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index ed9660adad77..49bcd413906f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI) += of_pci.o
> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
> obj-$(CONFIG_OF_MTD) += of_mtd.o
> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> +obj-$(CONFIG_COMMON_CLK) += of_clk.o
> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
> new file mode 100644
> index 000000000000..35f5e9f3dd42
> --- /dev/null
> +++ b/drivers/of/of_clk.c
> @@ -0,0 +1,103 @@
> +/*
> + * Copyright (C) 2014 Glider bvba
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_clock.h>
> +#include <linux/pm_runtime.h>
> +
> +
> +#ifdef CONFIG_PM_RUNTIME
> +
> +static int of_clk_pm_runtime_suspend(struct device *dev)
> +{
> + int ret;
> +
> + ret = pm_generic_runtime_suspend(dev);
> + if (ret)
> + return ret;
> +
> + ret = pm_clk_suspend(dev);
> + if (ret) {
> + pm_generic_runtime_resume(dev);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int of_clk_pm_runtime_resume(struct device *dev)
> +{
> + pm_clk_resume(dev);
> + return pm_generic_runtime_resume(dev);
> +}
> +
> +static struct dev_pm_domain of_clk_pm_domain = {
> + .ops = {
> + .runtime_suspend = of_clk_pm_runtime_suspend,
> + .runtime_resume = of_clk_pm_runtime_resume,
> + USE_PLATFORM_PM_SLEEP_OPS
> + },
> +};
> +
> +static int of_clk_register(struct device *dev, struct clk *clk)
> +{
> + int error;
> +
> + if (!dev->pm_domain) {
> + error = pm_clk_create(dev);
> + if (error)
> + return error;
> +
> + dev->pm_domain = &of_clk_pm_domain;
I am concerned about how this will work in conjunction with the
generic power domain.
A device can't reside in more than one pm_domain; thus I think it
would be better to always use the generic power domain and not have a
specific one for clocks. Typically the genpd should invoke
pm_clk_resume|suspend from it's runtime PM callbacks.
> + }
> +
> + dev_dbg(dev, "Setting up clock for runtime PM management\n");
> + return pm_clk_add_clk(dev, clk);
> +}
> +
> +#else /* !CONFIG_PM_RUNTIME */
> +
> +static int of_clk_register(struct device *dev, struct clk *clk)
> +{
> + dev_dbg(dev, "Runtime PM disabled, enabling clock\n");
> + return clk_prepare_enable(clk);
> +}
> +
> +#endif /* !CONFIG_PM_RUNTIME */
> +
> +
> +/**
> + * of_clk_register_runtime_pm_clocks - Register clocks suitable for Runtime PM
> + * with the PM core
> + * @np: pointer to device tree node
> + * @pdev: pointer to corresponding device to register suitable clocks for
> + *
> + * Returns an error code
> + */
> +int of_clk_register_runtime_pm_clocks(struct device_node *np,
> + struct device *dev)
> +{
> + unsigned int i;
> + struct clk *clk;
> + int error;
> +
> + for (i = 0; (clk = of_clk_get(np, i)) && !IS_ERR(clk); i++) {
> + if (!clk_may_runtime_pm(clk)) {
> + clk_put(clk);
> + continue;
> + }
> +
> + error = of_clk_register(dev, clk);
> + if (error) {
> + clk_put(clk);
> + return error;
> + }
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1daebefa..29145302b6f8 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -18,6 +18,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/slab.h>
> #include <linux/of_address.h>
> +#include <linux/of_clk.h>
> #include <linux/of_device.h>
> #include <linux/of_irq.h>
> #include <linux/of_platform.h>
> @@ -182,6 +183,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
> else
> of_device_make_bus_id(&dev->dev);
>
> + of_clk_register_runtime_pm_clocks(np, &dev->dev);
> +
What about other device than platform devices? Could we handle the DT
binding at driver core at probe instead?
Kind regards
Ulf Hansson
> return dev;
> }
> EXPORT_SYMBOL(of_device_alloc);
> diff --git a/include/linux/of_clk.h b/include/linux/of_clk.h
> new file mode 100644
> index 000000000000..b9b15614e39b
> --- /dev/null
> +++ b/include/linux/of_clk.h
> @@ -0,0 +1,18 @@
> +#ifndef _LINUX_OF_CLK_H
> +#define _LINUX_OF_CLK_H
> +
> +struct device_node;
> +struct device;
> +
> +#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
> +int of_clk_register_runtime_pm_clocks(struct device_node *np,
> + struct device *dev);
> +#else
> +static inline int of_clk_register_runtime_pm_clocks(struct device_node *np,
> + struct device *dev)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_OF && CONFIG_COMMON_CLK */
> +
> +#endif /* _LINUX_OF_CLK_H */
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Hi Ulf,
On Thu, Apr 24, 2014 at 3:11 PM, Ulf Hansson <[email protected]> wrote:
>> +static int of_clk_register(struct device *dev, struct clk *clk)
>> +{
>> + int error;
>> +
>> + if (!dev->pm_domain) {
>> + error = pm_clk_create(dev);
>> + if (error)
>> + return error;
>> +
>> + dev->pm_domain = &of_clk_pm_domain;
>
> I am concerned about how this will work in conjunction with the
> generic power domain.
>
> A device can't reside in more than one pm_domain; thus I think it
> would be better to always use the generic power domain and not have a
> specific one for clocks. Typically the genpd should invoke
> pm_clk_resume|suspend from it's runtime PM callbacks.
On shmobile SoCs supporting power domains, the power domain is
overridden later by calling rmobile_add_devices_to_domains() and friends.
My patch doesn't change that: the code behaved the same in the
non-multi-platform case before: dev->pm_domain as set from
drivers/sh/pm_runtime.c was overridden later.
I'll have a deeper look into the power domain code later anyway.
Thanks!
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
Geert Uytterhoeven <[email protected]> writes:
> When adding a device from DT, check if its clocks are suitable for Runtime
> PM, and register them with the PM core.
> If Runtime PM is disabled, just enable the clock.
>
> This allows the PM core to automatically manage gate clocks of devices for
> Runtime PM.
...unless the device is already in an existing pm_domain, right?
I like this approach, and it extends nicely what we already do on
platforms using drivers/base/power/clock_ops.c into DT land.
My only concern is how this will interact if it's used along with
devices that have existing pm_domains. I don't have any specific
concerns (yet, because it's Friday, and my brain is turing off), but it
just made me wonder if this will be potentially confusing.
Also...
[...]
> +static int of_clk_register(struct device *dev, struct clk *clk)
> +{
> + int error;
> +
> + if (!dev->pm_domain) {
> + error = pm_clk_create(dev);
> + if (error)
> + return error;
> +
> + dev->pm_domain = &of_clk_pm_domain;
> + }
> +
> + dev_dbg(dev, "Setting up clock for runtime PM management\n");
> + return pm_clk_add_clk(dev, clk);
I would've expected these 2 lines to be inside the pm_domain check.
What's the reason for doing the pm_clk_add() when the pm_domain isn't
going to be used? I suppose it's harmless, but it's a bit confusing.
Kevin
On 24.04.2014 15:11, Ulf Hansson wrote:
> On 24 April 2014 12:13, Geert Uytterhoeven <[email protected]> wrote:
>> When adding a device from DT, check if its clocks are suitable for Runtime
>> PM, and register them with the PM core.
>> If Runtime PM is disabled, just enable the clock.
>>
>> This allows the PM core to automatically manage gate clocks of devices for
>> Runtime PM.
>
> Normally I don't think it's a good idea to "automatically" manage
> clocks from PM core or any other place but from the driver (and
> possibly the subsystem).
>
> The reason is simply that we hide things that normally is supposed to
> be handled by the driver. Typically a cross SOC driver should work
> fine both with and without a pm_domain. It should also not rely on
> CONFIG_PM_RUNTIME.
>
>>
>> Signed-off-by: Geert Uytterhoeven <[email protected]>
>> ---
>> drivers/of/Makefile | 1 +
>> drivers/of/of_clk.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++
>> drivers/of/platform.c | 3 ++
>> include/linux/of_clk.h | 18 +++++++++
>> 4 files changed, 125 insertions(+)
>> create mode 100644 drivers/of/of_clk.c
>> create mode 100644 include/linux/of_clk.h
>>
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index ed9660adad77..49bcd413906f 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI) += of_pci.o
>> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
>> obj-$(CONFIG_OF_MTD) += of_mtd.o
>> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
>> +obj-$(CONFIG_COMMON_CLK) += of_clk.o
>> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
>> new file mode 100644
>> index 000000000000..35f5e9f3dd42
>> --- /dev/null
>> +++ b/drivers/of/of_clk.c
>> @@ -0,0 +1,103 @@
>> +/*
>> + * Copyright (C) 2014 Glider bvba
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/of_clk.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_clock.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +
>> +#ifdef CONFIG_PM_RUNTIME
>> +
>> +static int of_clk_pm_runtime_suspend(struct device *dev)
>> +{
>> + int ret;
>> +
>> + ret = pm_generic_runtime_suspend(dev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = pm_clk_suspend(dev);
>> + if (ret) {
>> + pm_generic_runtime_resume(dev);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int of_clk_pm_runtime_resume(struct device *dev)
>> +{
>> + pm_clk_resume(dev);
>> + return pm_generic_runtime_resume(dev);
>> +}
>> +
>> +static struct dev_pm_domain of_clk_pm_domain = {
>> + .ops = {
>> + .runtime_suspend = of_clk_pm_runtime_suspend,
>> + .runtime_resume = of_clk_pm_runtime_resume,
>> + USE_PLATFORM_PM_SLEEP_OPS
>> + },
>> +};
>> +
>> +static int of_clk_register(struct device *dev, struct clk *clk)
>> +{
>> + int error;
>> +
>> + if (!dev->pm_domain) {
>> + error = pm_clk_create(dev);
>> + if (error)
>> + return error;
>> +
>> + dev->pm_domain = &of_clk_pm_domain;
>
> I am concerned about how this will work in conjunction with the
> generic power domain.
>
> A device can't reside in more than one pm_domain; thus I think it
> would be better to always use the generic power domain and not have a
> specific one for clocks. Typically the genpd should invoke
> pm_clk_resume|suspend from it's runtime PM callbacks.
I'm not sure about this. A typical use case would be to gate clocks ASAP
and then wait until device is idle long enough to consider turning off
the power domain worthwhile. Also sometimes we may want to gate the
clocks, but prevent power domain from being powered off to retain
hardware state (e.g. because there is no way to read it and restore later).
I believe, though, that for devices that are not inside a controllable
power domain, this might be a good solution.
Best regards,
Tomasz
On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <[email protected]> wrote:
> Geert Uytterhoeven <[email protected]> writes:
>
> > When adding a device from DT, check if its clocks are suitable for Runtime
> > PM, and register them with the PM core.
> > If Runtime PM is disabled, just enable the clock.
> >
> > This allows the PM core to automatically manage gate clocks of devices for
> > Runtime PM.
>
> ...unless the device is already in an existing pm_domain, right?
>
> I like this approach, and it extends nicely what we already do on
> platforms using drivers/base/power/clock_ops.c into DT land.
>
> My only concern is how this will interact if it's used along with
> devices that have existing pm_domains. I don't have any specific
> concerns (yet, because it's Friday, and my brain is turing off), but it
> just made me wonder if this will be potentially confusing.
I have big concerns about this approach. First, it will only work if
a clock is available at deivce creation time. The conversion of irq
controllers to normal device drivers has already shown that is a bad
idea.
I also don't like that it tries to set up every clock, but there is no
guarantee that the driver will even use it. I would rather see this
behaviour linked into the function that obtains the clock at driver
.probe() time. That way it can handle deferred probe correctly and it
only sets up clocks that are actually used by the driver.
g.
Hi Ulf and Geert,
On Thursday 24 April 2014 15:11:24 Ulf Hansson wrote:
> On 24 April 2014 12:13, Geert Uytterhoeven <[email protected]> wrote:
> > When adding a device from DT, check if its clocks are suitable for Runtime
> > PM, and register them with the PM core.
> > If Runtime PM is disabled, just enable the clock.
> >
> > This allows the PM core to automatically manage gate clocks of devices for
> > Runtime PM.
>
> Normally I don't think it's a good idea to "automatically" manage
> clocks from PM core or any other place but from the driver (and
> possibly the subsystem).
>
> The reason is simply that we hide things that normally is supposed to
> be handled by the driver. Typically a cross SOC driver should work
> fine both with and without a pm_domain. It should also not rely on
> CONFIG_PM_RUNTIME.
That's a very good point. Geert, what do you think should happen if
CONFIG_PM_RUNTIME is not set ? I don't have a strong opinion (yet) on whether
we could require CONFIG_PM_RUNTIME, but it would indeed be nice to support
both cases. One option would be to keep the clocks enabled unconditionally in
that case, as not setting CONFIG_PM_RUNTIME means that the user doesn't care
(or cares less) about power consumption.
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
--
Regards,
Laurent Pinchart
On Tuesday 29 April 2014 14:16:10 Grant Likely wrote:
> On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <[email protected]> wrote:
> > Geert Uytterhoeven <[email protected]> writes:
> > > When adding a device from DT, check if its clocks are suitable for
> > > Runtime PM, and register them with the PM core.
> > > If Runtime PM is disabled, just enable the clock.
> > >
> > > This allows the PM core to automatically manage gate clocks of devices
> > > for Runtime PM.
> >
> > ...unless the device is already in an existing pm_domain, right?
> >
> > I like this approach, and it extends nicely what we already do on
> > platforms using drivers/base/power/clock_ops.c into DT land.
> >
> > My only concern is how this will interact if it's used along with
> > devices that have existing pm_domains. I don't have any specific
> > concerns (yet, because it's Friday, and my brain is turing off), but it
> > just made me wonder if this will be potentially confusing.
>
> I have big concerns about this approach. First, it will only work if
> a clock is available at deivce creation time. The conversion of irq
> controllers to normal device drivers has already shown that is a bad
> idea.
>
> I also don't like that it tries to set up every clock, but there is no
> guarantee that the driver will even use it. I would rather see this
> behaviour linked into the function that obtains the clock at driver
> .probe() time. That way it can handle deferred probe correctly and it
> only sets up clocks that are actually used by the driver.
I like the idea, as it gives an opt-in approach to the problem: drivers could
decide whether they want the runtime PM core to handle clocks automatically
(which should cover most cases), but would have the option of handling clocks
manually if needed for special purposes.
--
Regards,
Laurent Pinchart
Hi Geert,
On Thursday 24 April 2014 12:13:19 Geert Uytterhoeven wrote:
> On SoCs like ARM/SH-mobile, gate clocks are available for modules, allowing
> Runtime PM for a device controlled by a gate clock.
>
> On legacy shmobile kernels, this is handled by the PM runtime code in
> drivers/sh/pm_runtime.c, which installs a clock notifier for the platform
> bus, registering the "NULL" clock of each platform device with the PM core.
> This approach is also used on davinci, keystone, and omap1.
This requires the device to have the MSTP clock defined as the first clock in
its DT node. I'm not against that, but the requirement should be clearly
documented, and we need to check existing DT bindings to make sure they comply
with that.
I'd like to also take this as an opportunity to discuss how we should name
clocks in DT bindings for Renesas devices. Most devices have a single MSTP
clock, in which case we don't specify a name. Other devices need several
clocks. Names for the non-MSTP clocks will obviously be device-dependent, but
how should the MSTP clock be called in that time ? Should it have an empty
name (a "" string in DT) ? Should it have a standard name ? Maybe "fck" for
"functional clock" ?
> On multi-platform shmobile kernels, this was not handled at all, leading
> to spurious disabled clocks on drivers relying on Runtime PM, depending on
> implicit reset state, or on the bootloader.
>
> A first solution, enabling the PM runtime code in drivers/sh/pm_runtime.c
> in a multi-platform-safe way, was provided by the patch series
> "[PATCH v2 00/17] ARM: shmobile: Enable drivers/sh/pm_runtime.c on
> multi-platform" (http://www.spinics.net/lists/linux-sh/msg30887.html).
>
> Here is an alternative approach, avoiding the reliance on C board files,
> which are being phased out.
>
> This is also related to a patch series by Felipe Balbi ("[RFC/PATCH] base:
> platform: add generic clock handling for platform-bus",
> https://lkml.org/lkml/2014/1/31/290)
>
> This series:
> 1. Lets the MSTP clock driver indicate that its clocks are suitable for
> Runtime PM,
> 2. Lets the DT code retrieve clock information when adding a device
> (it already retrieves information for resources (registers, irq) ---
> unfortunately clocks are not resources), and registering clocks
> suitable for Runtime PM with the PM core.
> If Runtime PM is disabled, the clocks are just enabled.
>
> Note that this works for devices instantiated from DT only.
> Fortunately the drivers for the remaining platform devices (SCI and CMT)
> handle clocks theirselves, without Runtime PM, so they get properly enabled.
>
> Patches:
> - [1/4] clk: Add CLK_RUNTIME_PM and clk_may_runtime_pm()
> - [2/4] PM / clock_ops: Add pm_clk_add_clk()
> - [3/4] of/clk: Register clocks suitable for Runtime PM with the
> - [4/4] clk: shmobile: mstp: Set CLK_RUNTIME_PM flag
>
> This series was tested on Renesas r8a7791, using the Koelsch development
> board.
--
Regards,
Laurent Pinchart
On 30/04/14 14:25, Laurent Pinchart wrote:
> On Tuesday 29 April 2014 14:16:10 Grant Likely wrote:
>> On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <[email protected]> wrote:
>>> Geert Uytterhoeven <[email protected]> writes:
>>>> When adding a device from DT, check if its clocks are suitable for
>>>> Runtime PM, and register them with the PM core.
>>>> If Runtime PM is disabled, just enable the clock.
>>>>
>>>> This allows the PM core to automatically manage gate clocks of devices
>>>> for Runtime PM.
>>>
>>> ...unless the device is already in an existing pm_domain, right?
>>>
>>> I like this approach, and it extends nicely what we already do on
>>> platforms using drivers/base/power/clock_ops.c into DT land.
>>>
>>> My only concern is how this will interact if it's used along with
>>> devices that have existing pm_domains. I don't have any specific
>>> concerns (yet, because it's Friday, and my brain is turing off), but it
>>> just made me wonder if this will be potentially confusing.
>>
>> I have big concerns about this approach. First, it will only work if
>> a clock is available at deivce creation time. The conversion of irq
>> controllers to normal device drivers has already shown that is a bad
>> idea.
>>
>> I also don't like that it tries to set up every clock, but there is no
>> guarantee that the driver will even use it. I would rather see this
>> behaviour linked into the function that obtains the clock at driver
>> .probe() time. That way it can handle deferred probe correctly and it
>> only sets up clocks that are actually used by the driver.
>
> I like the idea, as it gives an opt-in approach to the problem: drivers could
> decide whether they want the runtime PM core to handle clocks automatically
> (which should cover most cases), but would have the option of handling clocks
> manually if needed for special purposes.
If drivers could have a field to say that they allow the driver-core
or the pm-runtime would mean that drivers could easily be change without
having to deal with what the SoC/SoC family have to care about this.
It would also mean that we could change drivers without having to make
any changes to the SoC to say that it has to opt-in to the support.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
Hi Kevin,
On Sat, Apr 26, 2014 at 1:44 AM, Kevin Hilman <[email protected]> wrote:
> Geert Uytterhoeven <[email protected]> writes:
>> When adding a device from DT, check if its clocks are suitable for Runtime
>> PM, and register them with the PM core.
>> If Runtime PM is disabled, just enable the clock.
>>
>> This allows the PM core to automatically manage gate clocks of devices for
>> Runtime PM.
>
> ...unless the device is already in an existing pm_domain, right?
At this point in the kernel boot process, the device cannot be in a
pm_domain yet.
> I like this approach, and it extends nicely what we already do on
> platforms using drivers/base/power/clock_ops.c into DT land.
>
> My only concern is how this will interact if it's used along with
> devices that have existing pm_domains. I don't have any specific
> concerns (yet, because it's Friday, and my brain is turing off), but it
> just made me wonder if this will be potentially confusing.
Adding devices to pm_domains is done later, so it can be overridden.
> Also...
>
> [...]
>
>> +static int of_clk_register(struct device *dev, struct clk *clk)
>> +{
>> + int error;
>> +
>> + if (!dev->pm_domain) {
>> + error = pm_clk_create(dev);
>> + if (error)
>> + return error;
>> +
>> + dev->pm_domain = &of_clk_pm_domain;
>> + }
>> +
>> + dev_dbg(dev, "Setting up clock for runtime PM management\n");
>> + return pm_clk_add_clk(dev, clk);
>
> I would've expected these 2 lines to be inside the pm_domain check.
>
> What's the reason for doing the pm_clk_add() when the pm_domain isn't
> going to be used? I suppose it's harmless, but it's a bit confusing.
Sorry, the !dev->pm_domain check does deserve a comment explaining this.
If there are multiple clocks suitable for pm_runtime, the pm_clk_create(dev)
should be done only once.
Currently e.g. davinci registers 3 clocks with pm_clk ("fck",
"master", and "slave").
Omap has 2 ("fck" and "ick").
BTW, keystone doesn't seem to set pm_clk_notifier_block.con_ids. From a quick
look, this will crash with a NULL-pointer dereference in pm_clk_notify()? Or am
I missing something here?
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
Hi Grant,
On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <[email protected]> wrote:
> On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <[email protected]> wrote:
>> Geert Uytterhoeven <[email protected]> writes:
>>
>> > When adding a device from DT, check if its clocks are suitable for Runtime
>> > PM, and register them with the PM core.
>> > If Runtime PM is disabled, just enable the clock.
>> >
>> > This allows the PM core to automatically manage gate clocks of devices for
>> > Runtime PM.
>>
>> ...unless the device is already in an existing pm_domain, right?
>>
>> I like this approach, and it extends nicely what we already do on
>> platforms using drivers/base/power/clock_ops.c into DT land.
>>
>> My only concern is how this will interact if it's used along with
>> devices that have existing pm_domains. I don't have any specific
>> concerns (yet, because it's Friday, and my brain is turing off), but it
>> just made me wonder if this will be potentially confusing.
>
> I have big concerns about this approach. First, it will only work if
> a clock is available at deivce creation time. The conversion of irq
> controllers to normal device drivers has already shown that is a bad
> idea.
That's indeed a valid concern that needs to be addressed.
> I also don't like that it tries to set up every clock, but there is no
> guarantee that the driver will even use it. I would rather see this
> behaviour linked into the function that obtains the clock at driver
> .probe() time. That way it can handle deferred probe correctly and it
> only sets up clocks that are actually used by the driver.
Not every clock. Only the clocks that are advertised by the clock driver as
being suitable for runtime_pm management. These are typically module
clocks, that must be enabled for the module to work. The driver doesn't
always want to handle these explicitly.
In fact we have one case on shmobile where the module clock for an IP
core (rcar-gpio) is enabled unconditionally in one SoC, while it became
controllable through a gate clock in a later SoC.
With my patch, just adding the clock to the DT node is sufficient to make
it work.
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
Hi Laurent,
On Wed, Apr 30, 2014 at 11:23 PM, Laurent Pinchart
<[email protected]> wrote:
> On Thursday 24 April 2014 15:11:24 Ulf Hansson wrote:
>> On 24 April 2014 12:13, Geert Uytterhoeven <[email protected]> wrote:
>> > When adding a device from DT, check if its clocks are suitable for Runtime
>> > PM, and register them with the PM core.
>> > If Runtime PM is disabled, just enable the clock.
>> >
>> > This allows the PM core to automatically manage gate clocks of devices for
>> > Runtime PM.
>>
>> Normally I don't think it's a good idea to "automatically" manage
>> clocks from PM core or any other place but from the driver (and
>> possibly the subsystem).
>>
>> The reason is simply that we hide things that normally is supposed to
>> be handled by the driver. Typically a cross SOC driver should work
>> fine both with and without a pm_domain. It should also not rely on
>> CONFIG_PM_RUNTIME.
>
> That's a very good point. Geert, what do you think should happen if
> CONFIG_PM_RUNTIME is not set ? I don't have a strong opinion (yet) on whether
> we could require CONFIG_PM_RUNTIME, but it would indeed be nice to support
> both cases. One option would be to keep the clocks enabled unconditionally in
> that case, as not setting CONFIG_PM_RUNTIME means that the user doesn't care
> (or cares less) about power consumption.
This is already handled by my patch. If CONFIG_PM_RUNTIME is disabled,
the clocks are enabled by calling clk_prepare_enabled().
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
Hi Laurent,
On Wed, Apr 30, 2014 at 11:29 PM, Laurent Pinchart
<[email protected]> wrote:
> On Thursday 24 April 2014 12:13:19 Geert Uytterhoeven wrote:
>> On SoCs like ARM/SH-mobile, gate clocks are available for modules, allowing
>> Runtime PM for a device controlled by a gate clock.
>>
>> On legacy shmobile kernels, this is handled by the PM runtime code in
>> drivers/sh/pm_runtime.c, which installs a clock notifier for the platform
>> bus, registering the "NULL" clock of each platform device with the PM core.
>> This approach is also used on davinci, keystone, and omap1.
>
> This requires the device to have the MSTP clock defined as the first clock in
> its DT node. I'm not against that, but the requirement should be clearly
> documented, and we need to check existing DT bindings to make sure they comply
> with that.
Being the first clock is only required for the "NULL" clock. And that
is only done
for legacy shmobile kernels, not for multi-platform.
In this patch series, the clock would be chosen based on the presence of the
CLK_RUNTIME_PM flag, to be set by the clock driver. I.e. DT is not involved
directly (for a change... why does everybody think the whole world revolves
around DT these days ? :-)
> I'd like to also take this as an opportunity to discuss how we should name
> clocks in DT bindings for Renesas devices. Most devices have a single MSTP
> clock, in which case we don't specify a name. Other devices need several
> clocks. Names for the non-MSTP clocks will obviously be device-dependent, but
> how should the MSTP clock be called in that time ? Should it have an empty
> name (a "" string in DT) ? Should it have a standard name ? Maybe "fck" for
> "functional clock" ?
Empty names should not be used if there can be multiple clocks, right?
Grepping in arch/*/boot/dts/, "fck" seems to be popular (only) for TI SoCs.
Stadardizing across SoCs and architectures would be nice, though.
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
Use "clkops-clocks" property to specify clocks handled by
clock_ops domain PM domain. Only clocks defined in "clkops-clocks"
set of clocks will be handled by Runtime PM through clock_ops
Pm domain.
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/of/of_clk.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
index 35f5e9f..5f9b90e 100644
--- a/drivers/of/of_clk.c
+++ b/drivers/of/of_clk.c
@@ -86,11 +86,8 @@ int of_clk_register_runtime_pm_clocks(struct device_node *np,
struct clk *clk;
int error;
- for (i = 0; (clk = of_clk_get(np, i)) && !IS_ERR(clk); i++) {
- if (!clk_may_runtime_pm(clk)) {
- clk_put(clk);
- continue;
- }
+ for (i = 0; (clk = of_clk_get_from_set(np, "clkops", i)) &&
+ !IS_ERR(clk); i++) {
error = of_clk_register(dev, clk);
if (error) {
--
1.7.9.5
Hi Geert,
I've spent some time testing your patches on Keystone 2 SoC as I am interested
in these patches.
The Keystone 2 is pure DT platform, but we reuse some Drivers from Davinci SoC.
Now I have to dial with following problem:
- Some modules on Keystone need more then one clock to be managed by PM clock.
As result, I can solve this by filling cond_id list in structure
pm_clk_notifier_block.
For example:
static struct pm_clk_notifier_block platform_domain_notifier = {
.pm_domain = &keystone_pm_domain,
.con_ids = { "fck", "master", "slave", NULL },
};
But, in this case I'll need to add names for all clocks or rename existed
clock's names in DT to be compatible with above list, like:
clock-names = "gpio"; -> clock-names = "fck";
- or -
clocks = <&clkspi>;
+ clock-names = "fck";
Your series gracefully solves this problem for me, but I'd like to avoid
to use new CLK flag CLK_RUNTIME_PM, because:
- The same driver is used for all gated clocks for Keystone (and probably for
other SoCs)
- Some gated clocks can be optional.
Taking into account above, driver for gated clock will need to maintain additional
information internally about clocks which are suitable for Runtime PM -
it is too hard to support :(.
Therefore, I propose a solution which allows to specify clocks suitable for
Runtime PM in DT using special property "clkops-clocks" (name can be changed:).
Another possible option is to use DT definition like this:
spi2: spi@21000800 {
compatible = "ti,dm6441-spi";
reg = <0x21000800 0x200>;
num-cs = <4>;
ti,davinci-spi-intr-line = <0>;
interrupts = <GIC_SPI 300 IRQ_TYPE_EDGE_RISING>;
-> clkops-clocks {
-> clocks = <&clkspi>;
-> }
}
Regarding supporting of EPROBE_DEFER, in my opinion simplest solution would be to
call of_clk_register_runtime_pm_clocks() directly from drivers.
Another option 1, call of_clk_register_runtime_pm_clocks() before driver's probing
seems will be banned by Greg and Rafael.
Another option 2, continue to use Bus notifiers, but then error path need to be
handled somehow. Now BUS_NOTIFY_BIND_DRIVER even is sent before probing, but
it seems that nothing is sent in case if probe was failed.
Grygorii Strashko (2):
clk: of: introduce of_clk_get_from_set()
of/clk: use "clkops-clocks" to specify clocks handled by clock_ops
domain
drivers/clk/clkdev.c | 24 ++++++++++++++++++++++--
drivers/of/of_clk.c | 7 ++-----
include/linux/clk.h | 7 +++++++
3 files changed, 31 insertions(+), 7 deletions(-)
--
1.7.9.5
In many case it's useful to divide device's clocks into
few sets according to their designation.
- some clocks can be optional for the device
- some clocks can be managed by PM frameworks (like clock_ops for example)
while some need to be managed by driver directly.
This patch introduces new API of_clk_get_from_set() which allows
the callers to specify additional prefix to be used together with
generic DT clocks list property name "clocks".
In the example bellow, the caller asks CLK framework to retrieve
clock from the clocks list "clkops-clocks" and not from "clocks":
DT:
usb: usb@2680000 {
compatible = "ti,keystone-dwc3";
[...]
clkops-clocks = <&clkusb>;
Code:
clk = of_clk_get_from_set(np, "clkops", 0);
This changes will not affect on already existed code.
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/clk/clkdev.c | 24 ++++++++++++++++++++++--
include/linux/clk.h | 7 +++++++
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index f890b90..2308518 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -53,7 +53,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
return clk;
}
-struct clk *of_clk_get(struct device_node *np, int index)
+static struct clk *of_clk_get_named(struct device_node *np,
+ const char *propname, int index)
{
struct of_phandle_args clkspec;
struct clk *clk;
@@ -62,7 +63,7 @@ struct clk *of_clk_get(struct device_node *np, int index)
if (index < 0)
return ERR_PTR(-EINVAL);
- rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
+ rc = of_parse_phandle_with_args(np, propname, "#clock-cells", index,
&clkspec);
if (rc)
return ERR_PTR(rc);
@@ -71,8 +72,27 @@ struct clk *of_clk_get(struct device_node *np, int index)
of_node_put(clkspec.np);
return clk;
}
+
+struct clk *of_clk_get(struct device_node *np, int index)
+{
+ return of_clk_get_named(np, "clocks", index);
+}
EXPORT_SYMBOL(of_clk_get);
+struct clk *of_clk_get_from_set(struct device_node *np,
+ const char *set_id, int index)
+{
+ char prop_name[32]; /* 32 is max size of property name */
+
+ if (set_id)
+ snprintf(prop_name, 32, "%s-clocks", set_id);
+ else
+ snprintf(prop_name, 32, "clocks");
+
+ return of_clk_get_named(np, prop_name, index);
+}
+EXPORT_SYMBOL(of_clk_get_from_set);
+
/**
* of_clk_get_by_name() - Parse and lookup a clock referenced by a device node
* @np: pointer to clock consumer node
diff --git a/include/linux/clk.h b/include/linux/clk.h
index fb5e097..fc8865a 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -397,6 +397,8 @@ struct of_phandle_args;
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
struct clk *of_clk_get(struct device_node *np, int index);
+struct clk *of_clk_get_from_set(struct device_node *np,
+ const char *set_id, int index);
struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
#else
@@ -409,6 +411,11 @@ static inline struct clk *of_clk_get_by_name(struct device_node *np,
{
return ERR_PTR(-ENOENT);
}
+static inline struct clk *of_clk_get_from_set(struct device_node *np,
+ const char *set_id, int index)
+{
+ return ERR_PTR(-ENOENT);
+}
#endif
#endif
--
1.7.9.5
On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko <[email protected]> wrote:
> Use "clkops-clocks" property to specify clocks handled by
> clock_ops domain PM domain. Only clocks defined in "clkops-clocks"
> set of clocks will be handled by Runtime PM through clock_ops
> Pm domain.
>
> Signed-off-by: Grygorii Strashko <[email protected]>
> ---
> drivers/of/of_clk.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
> index 35f5e9f..5f9b90e 100644
> --- a/drivers/of/of_clk.c
> +++ b/drivers/of/of_clk.c
> @@ -86,11 +86,8 @@ int of_clk_register_runtime_pm_clocks(struct device_node *np,
> struct clk *clk;
> int error;
>
> - for (i = 0; (clk = of_clk_get(np, i)) && !IS_ERR(clk); i++) {
> - if (!clk_may_runtime_pm(clk)) {
> - clk_put(clk);
> - continue;
> - }
> + for (i = 0; (clk = of_clk_get_from_set(np, "clkops", i)) &&
> + !IS_ERR(clk); i++) {
This really looks like an ABI break to me. What happens to all the
existing platforms who don't have this new clkops-clocks in their device
tree?
g.
Hi Grant.
On 07/28/2014 05:05 PM, Grant Likely wrote:
> On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko <[email protected]> wrote:
>> Use "clkops-clocks" property to specify clocks handled by
>> clock_ops domain PM domain. Only clocks defined in "clkops-clocks"
>> set of clocks will be handled by Runtime PM through clock_ops
>> Pm domain.
>>
>> Signed-off-by: Grygorii Strashko <[email protected]>
>> ---
>> drivers/of/of_clk.c | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
>> index 35f5e9f..5f9b90e 100644
>> --- a/drivers/of/of_clk.c
>> +++ b/drivers/of/of_clk.c
>> @@ -86,11 +86,8 @@ int of_clk_register_runtime_pm_clocks(struct device_node *np,
>> struct clk *clk;
>> int error;
>>
>> - for (i = 0; (clk = of_clk_get(np, i)) && !IS_ERR(clk); i++) {
>> - if (!clk_may_runtime_pm(clk)) {
>> - clk_put(clk);
>> - continue;
>> - }
>> + for (i = 0; (clk = of_clk_get_from_set(np, "clkops", i)) &&
>> + !IS_ERR(clk); i++) {
>
> This really looks like an ABI break to me. What happens to all the
> existing platforms who don't have this new clkops-clocks in their device
> tree?
Agree. This patch as is will break such platforms.
As possible solution for above problem - the NULL can be used as clock's prefix
by default and platform code can configure new value of clock's prefix during
initialization.
In addition, to make this solution full the of_clk_get_by_name() will
need to be modified too.
But note pls, this is pure RFC patches which I did to find out the answer on questions:
- What is better: maintain Runtime PM clocks configuration in DT or in code?
- Where and when to call of_clk_register_runtime_pm_clocks()?
Bus notifier/ platform core/ device drivers
Also, May be platform dependent solution [1] can be acceptable for now?
[1] https://lkml.org/lkml/2014/7/25/630
Best regards,
-grygorii
On Mon, Jul 28, 2014 at 11:47 AM, Grygorii Strashko
<[email protected]> wrote:
> Hi Grant.
>
> On 07/28/2014 05:05 PM, Grant Likely wrote:
>> On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko <[email protected]> wrote:
>>> Use "clkops-clocks" property to specify clocks handled by
>>> clock_ops domain PM domain. Only clocks defined in "clkops-clocks"
>>> set of clocks will be handled by Runtime PM through clock_ops
>>> Pm domain.
>>>
>>> Signed-off-by: Grygorii Strashko <[email protected]>
>>> ---
>>> drivers/of/of_clk.c | 7 ++-----
>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
>>> index 35f5e9f..5f9b90e 100644
>>> --- a/drivers/of/of_clk.c
>>> +++ b/drivers/of/of_clk.c
>>> @@ -86,11 +86,8 @@ int of_clk_register_runtime_pm_clocks(struct device_node *np,
>>> struct clk *clk;
>>> int error;
>>>
>>> - for (i = 0; (clk = of_clk_get(np, i)) && !IS_ERR(clk); i++) {
>>> - if (!clk_may_runtime_pm(clk)) {
>>> - clk_put(clk);
>>> - continue;
>>> - }
>>> + for (i = 0; (clk = of_clk_get_from_set(np, "clkops", i)) &&
>>> + !IS_ERR(clk); i++) {
>>
>> This really looks like an ABI break to me. What happens to all the
>> existing platforms who don't have this new clkops-clocks in their device
>> tree?
>
> Agree. This patch as is will break such platforms.
> As possible solution for above problem - the NULL can be used as clock's prefix
> by default and platform code can configure new value of clock's prefix during
> initialization.
> In addition, to make this solution full the of_clk_get_by_name() will
> need to be modified too.
>
> But note pls, this is pure RFC patches which I did to find out the answer on questions:
> - What is better: maintain Runtime PM clocks configuration in DT or in code?
In code. I don't think it is workable to embed runtime PM behaviour
into the DT bindings. I think there will be too much variance in what
hardware requires. We can create helpers to make this simpler, but I
don't think it is a good idea to set it up automatically without any
control from the driver itself.
>
> - Where and when to call of_clk_register_runtime_pm_clocks()?
> Bus notifier/ platform core/ device drivers
I would say in device drivers.
> Also, May be platform dependent solution [1] can be acceptable for now?
>
> [1] https://lkml.org/lkml/2014/7/25/630
I need to look at the series before I comment. I've flagged it and
will hopefully look at it tomorrow.
g.
Hi Grygorii and Grant,
On Monday 28 July 2014 23:52:34 Grant Likely wrote:
> On Mon, Jul 28, 2014 at 11:47 AM, Grygorii Strashko wrote:
> > On 07/28/2014 05:05 PM, Grant Likely wrote:
> >> On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko wrote:
> >>> Use "clkops-clocks" property to specify clocks handled by
> >>> clock_ops domain PM domain. Only clocks defined in "clkops-clocks"
> >>> set of clocks will be handled by Runtime PM through clock_ops
> >>> Pm domain.
> >>>
> >>> Signed-off-by: Grygorii Strashko <[email protected]>
> >>> ---
> >>>
> >>> drivers/of/of_clk.c | 7 ++-----
> >>> 1 file changed, 2 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
> >>> index 35f5e9f..5f9b90e 100644
> >>> --- a/drivers/of/of_clk.c
> >>> +++ b/drivers/of/of_clk.c
> >>> @@ -86,11 +86,8 @@ int of_clk_register_runtime_pm_clocks(struct
> >>> device_node *np,>>>
> >>> struct clk *clk;
> >>> int error;
> >>>
> >>> - for (i = 0; (clk = of_clk_get(np, i)) && !IS_ERR(clk); i++) {
> >>> - if (!clk_may_runtime_pm(clk)) {
> >>> - clk_put(clk);
> >>> - continue;
> >>> - }
> >>> + for (i = 0; (clk = of_clk_get_from_set(np, "clkops", i)) &&
> >>> + !IS_ERR(clk); i++) {
> >>
> >> This really looks like an ABI break to me. What happens to all the
> >> existing platforms who don't have this new clkops-clocks in their device
> >> tree?
> >
> > Agree. This patch as is will break such platforms.
> > As possible solution for above problem - the NULL can be used as clock's
> > prefix by default and platform code can configure new value of clock's
> > prefix during initialization.
> > In addition, to make this solution full the of_clk_get_by_name() will
> > need to be modified too.
> >
> > But note pls, this is pure RFC patches which I did to find out the answer
> > on questions: - What is better: maintain Runtime PM clocks configuration
> > in DT or in code?
>
> In code. I don't think it is workable to embed runtime PM behaviour
> into the DT bindings. I think there will be too much variance in what
> hardware requires. We can create helpers to make this simpler, but I
> don't think it is a good idea to set it up automatically without any
> control from the driver itself.
>
> > - Where and when to call of_clk_register_runtime_pm_clocks()?
> >
> > Bus notifier/ platform core/ device drivers
>
> I would say in device drivers.
I tend to agree with that.
It will help here to take a step back and remember what the problem we're
trying to solve is.
At the root is clock management. Our system comprise many clocks, and they
need to be handled. The Common Clock Framework nicely models the clocks, and
offers an API for drivers to retrieve device clocks and control them. Drivers
can thus implement clock management manually without much pain.
A clock can be managed in roughly three different ways :
- it can be enabled at probe time and disabled at remove time ;
- it can be enabled right before the device leaves its idle state and disabled
when the device goes back to idle ; or
- it can be enabled and disabled in a more fine-grained, device-specific
manner.
The selected clock management granularity depends on constraints specific to
the device and on how aggressive power saving needs to be. Enabling the clocks
at probe time and disabling them at remove time is enough for most devices,
but leads to a high power consumption. For that reason the second clock
management scheme is often desired.
Managing clocks manually in the driver is a valid option. However, when adding
runtime PM to the equation, and realizing that the clocks need to be enabled
in the runtime PM resume handler and disabled in the suspend handler, the
clock management code starts looking very similar in most drivers. We're thus
tempted to factorize it away from the drivers into a shared location.
It's important to note at this point that the goal here is only to simplify
drivers. Moving clock management code out of the drivers doesn't (unless I'm
missing something) open the door to new possibilities, it just serves as a
simplification.
Now, as Grygorii mentioned, differences between how a given IP core is
integrated in various SoCs can make clock management SoC-dependent. In the
vast majority of cases (which is really what we need to target, given that our
target is simplifying drivers) SoC integration can be described as a list of
clocks that must be managed. That list can be common to all devices in a given
SoC, or can be device-dependent as well.
Few locations can be used to express a per-device list of per-SoC clocks. We
can have clocks lists in a per-SoC and per-device location, per-device clocks
lists in an SoC-specific location, or per-SoC clocks lists in a device-
specific location.
The first option would require listing clocks to be managed by runtime PM in
DT nodes, as proposed by this patch set. I don't think this is the best
option, as that information is a mix of hardware description and software
policy, with the hardware description part being already present in DT in the
clocks property.
The second option calls for storing the lists in SoC code under arch/. As
we're trying to minimize the amount of SoC code there (and even remove SoC
code completely when possible) I don't think that's a good option.
The third option would require storing the clocks lists in device drivers. I
believe this is our best option, as a trade-off between simplicity and
versatility. Drivers that use runtime PM already need to enable it explicitly
when probing devices. Passing a list of clock names to runtime PM at that
point wouldn't complicate drivers much. When the clocks list isn't SoC-
dependent it could be stored as static information. Otherwise it could be
derived from DT (or any other source of hardware description) using C code,
offering all the versatility we need.
The only drawback of this solution I can think of right now is that the
runtime PM core couldn't manage device clocks before probing the device.
Specifically device clocks couldn't be managed if no driver is loaded for that
device. I somehow recall that someone raised this as being a problem, but I
can't remember why.
> > Also, May be platform dependent solution [1] can be acceptable for now?
> >
> > [1] https://lkml.org/lkml/2014/7/25/630
>
> I need to look at the series before I comment. I've flagged it and
> will hopefully look at it tomorrow.
--
Regards,
Laurent Pinchart
Hi Laurent,
On 07/30/2014 03:06 AM, Laurent Pinchart wrote:
> Hi Grygorii and Grant,
>
> On Monday 28 July 2014 23:52:34 Grant Likely wrote:
>> On Mon, Jul 28, 2014 at 11:47 AM, Grygorii Strashko wrote:
>>> On 07/28/2014 05:05 PM, Grant Likely wrote:
>>>> On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko wrote:
>>>>> Use "clkops-clocks" property to specify clocks handled by
>>>>> clock_ops domain PM domain. Only clocks defined in "clkops-clocks"
>>>>> set of clocks will be handled by Runtime PM through clock_ops
>>>>> Pm domain.
>>>>>
>>>>> Signed-off-by: Grygorii Strashko <[email protected]>
>>>>> ---
>>>>>
>>>>> drivers/of/of_clk.c | 7 ++-----
>>>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
>>>>> index 35f5e9f..5f9b90e 100644
>>>>> --- a/drivers/of/of_clk.c
>>>>> +++ b/drivers/of/of_clk.c
>>>>> @@ -86,11 +86,8 @@ int of_clk_register_runtime_pm_clocks(struct
>>>>> device_node *np,>>>
>>>>> struct clk *clk;
>>>>> int error;
>>>>>
>>>>> - for (i = 0; (clk = of_clk_get(np, i)) && !IS_ERR(clk); i++) {
>>>>> - if (!clk_may_runtime_pm(clk)) {
>>>>> - clk_put(clk);
>>>>> - continue;
>>>>> - }
>>>>> + for (i = 0; (clk = of_clk_get_from_set(np, "clkops", i)) &&
>>>>> + !IS_ERR(clk); i++) {
>>>>
>>>> This really looks like an ABI break to me. What happens to all the
>>>> existing platforms who don't have this new clkops-clocks in their device
>>>> tree?
>>>
>>> Agree. This patch as is will break such platforms.
>>> As possible solution for above problem - the NULL can be used as clock's
>>> prefix by default and platform code can configure new value of clock's
>>> prefix during initialization.
>>> In addition, to make this solution full the of_clk_get_by_name() will
>>> need to be modified too.
>>>
>>> But note pls, this is pure RFC patches which I did to find out the answer
>>> on questions: - What is better: maintain Runtime PM clocks configuration
>>> in DT or in code?
>>
>> In code. I don't think it is workable to embed runtime PM behaviour
>> into the DT bindings. I think there will be too much variance in what
>> hardware requires. We can create helpers to make this simpler, but I
>> don't think it is a good idea to set it up automatically without any
>> control from the driver itself.
>>
>>> - Where and when to call of_clk_register_runtime_pm_clocks()?
>>>
>>> Bus notifier/ platform core/ device drivers
>>
>> I would say in device drivers.
>
> I tend to agree with that.
>
> It will help here to take a step back and remember what the problem we're
> trying to solve is.
>
> At the root is clock management. Our system comprise many clocks, and they
> need to be handled. The Common Clock Framework nicely models the clocks, and
> offers an API for drivers to retrieve device clocks and control them. Drivers
> can thus implement clock management manually without much pain.
>
> A clock can be managed in roughly three different ways :
>
> - it can be enabled at probe time and disabled at remove time ;
>
> - it can be enabled right before the device leaves its idle state and disabled
> when the device goes back to idle ; or
>
> - it can be enabled and disabled in a more fine-grained, device-specific
> manner.
>
> The selected clock management granularity depends on constraints specific to
> the device and on how aggressive power saving needs to be. Enabling the clocks
> at probe time and disabling them at remove time is enough for most devices,
> but leads to a high power consumption. For that reason the second clock
> management scheme is often desired.
>
> Managing clocks manually in the driver is a valid option. However, when adding
> runtime PM to the equation, and realizing that the clocks need to be enabled
> in the runtime PM resume handler and disabled in the suspend handler, the
> clock management code starts looking very similar in most drivers. We're thus
> tempted to factorize it away from the drivers into a shared location.
>
> It's important to note at this point that the goal here is only to simplify
> drivers. Moving clock management code out of the drivers doesn't (unless I'm
> missing something) open the door to new possibilities, it just serves as a
> simplification.
>
> Now, as Grygorii mentioned, differences between how a given IP core is
> integrated in various SoCs can make clock management SoC-dependent. In the
> vast majority of cases (which is really what we need to target, given that our
> target is simplifying drivers) SoC integration can be described as a list of
> clocks that must be managed. That list can be common to all devices in a given
> SoC, or can be device-dependent as well.
That's actually a problem - now we have static list of managed clocks per-SoC and
not per device.
>
> Few locations can be used to express a per-device list of per-SoC clocks. We
> can have clocks lists in a per-SoC and per-device location, per-device clocks
> lists in an SoC-specific location, or per-SoC clocks lists in a device-
> specific location.
>
> The first option would require listing clocks to be managed by runtime PM in
> DT nodes, as proposed by this patch set. I don't think this is the best
> option, as that information is a mix of hardware description and software
> policy, with the hardware description part being already present in DT in the
> clocks property.
I'm not fully agree here. The clock is "functional clock" If It's managed by runtime PM.
And all such clocks need to be enabled/disabled always when device is powered on/off.
So, from my point of view it's HW description and it follows TRM.
Other clocks are optional and only drivers should control them.
And question is how to define sets of such clocks in the best way?
>
> The second option calls for storing the lists in SoC code under arch/. As
> we're trying to minimize the amount of SoC code there (and even remove SoC
> code completely when possible) I don't think that's a good option.
>
> The third option would require storing the clocks lists in device drivers. I
> believe this is our best option, as a trade-off between simplicity and
> versatility. Drivers that use runtime PM already need to enable it explicitly
> when probing devices. Passing a list of clock names to runtime PM at that
> point wouldn't complicate drivers much. When the clocks list isn't SoC-
> dependent it could be stored as static information. Otherwise it could be
> derived from DT (or any other source of hardware description) using C code,
> offering all the versatility we need.
Ok. if I understand right, you propose smth like this:
1) DT based solution:
devA {
clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
rpm-clocks = <&clkpa>, <&clkcpgmac>;
- or -
clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
rpm-clocks = "clk_pa", "clk_cpgmac";
}
in driver:
pm_runtime_enable();
|- of_clk_register_runtime_pm_clocks()
- or -
of_clk_register_runtime_pm_clocks()
pm_runtime_enable();
2) Static solution:
char *con_ids_davinci[] =
{ "fck", "master", "slave", NULL };
char *con_ids_keystone[] =
{ "clk_pa", "clk_cpgmac" };
static struct of_device_id of_match[] = {
{ .compatible = "ti,keystone", con_ids_keystone},
{ .compatible = "ti,davinci", con_ids_davinci},
{},
};
Personally, I like option 1 and, seems, it will not break ABI.
>
> The only drawback of this solution I can think of right now is that the
> runtime PM core couldn't manage device clocks before probing the device.
> Specifically device clocks couldn't be managed if no driver is loaded for that
> device. I somehow recall that someone raised this as being a problem, but I
> can't remember why.
>
I can recollect only OMAP2+ SoCs where some abstraction called HW_MOD is used
during platform initialization to reset all devices and turn off unused ones
before probing the devices. But clock_ops are not used by OMAP2+:)
regards,
-grygorii
Hi Grygorii,
I've found this mail deep inside my inbox :-)
On Wednesday 30 July 2014 16:25:31 Grygorii Strashko wrote:
> On 07/30/2014 03:06 AM, Laurent Pinchart wrote:
> > On Monday 28 July 2014 23:52:34 Grant Likely wrote:
> >> On Mon, Jul 28, 2014 at 11:47 AM, Grygorii Strashko wrote:
> >>> On 07/28/2014 05:05 PM, Grant Likely wrote:
> >>>> On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko wrote:
> >>>>> Use "clkops-clocks" property to specify clocks handled by
> >>>>> clock_ops domain PM domain. Only clocks defined in "clkops-clocks"
> >>>>> set of clocks will be handled by Runtime PM through clock_ops
> >>>>> Pm domain.
> >>>>>
> >>>>> Signed-off-by: Grygorii Strashko <[email protected]>
> >>>>> ---
> >>>>>
> >>>>> drivers/of/of_clk.c | 7 ++-----
> >>>>> 1 file changed, 2 insertions(+), 5 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
> >>>>> index 35f5e9f..5f9b90e 100644
> >>>>> --- a/drivers/of/of_clk.c
> >>>>> +++ b/drivers/of/of_clk.c
> >>>>> @@ -86,11 +86,8 @@ int of_clk_register_runtime_pm_clocks(struct
> >>>>> device_node *np,>>>
> >>>>>
> >>>>> struct clk *clk;
> >>>>> int error;
> >>>>>
> >>>>> - for (i = 0; (clk = of_clk_get(np, i)) && !IS_ERR(clk); i++) {
> >>>>> - if (!clk_may_runtime_pm(clk)) {
> >>>>> - clk_put(clk);
> >>>>> - continue;
> >>>>> - }
> >>>>> + for (i = 0; (clk = of_clk_get_from_set(np, "clkops", i)) &&
> >>>>> + !IS_ERR(clk); i++) {
> >>>>
> >>>> This really looks like an ABI break to me. What happens to all the
> >>>> existing platforms who don't have this new clkops-clocks in their
> >>>> device tree?
> >>>
> >>> Agree. This patch as is will break such platforms.
> >>> As possible solution for above problem - the NULL can be used as clock's
> >>> prefix by default and platform code can configure new value of clock's
> >>> prefix during initialization.
> >>> In addition, to make this solution full the of_clk_get_by_name() will
> >>> need to be modified too.
> >>>
> >>> But note pls, this is pure RFC patches which I did to find out the
> >>> answer on questions: - What is better: maintain Runtime PM clocks
> >>> configuration in DT or in code?
> >>
> >> In code. I don't think it is workable to embed runtime PM behaviour
> >> into the DT bindings. I think there will be too much variance in what
> >> hardware requires. We can create helpers to make this simpler, but I
> >> don't think it is a good idea to set it up automatically without any
> >> control from the driver itself.
> >>
> >>> - Where and when to call of_clk_register_runtime_pm_clocks()?
> >>>
> >>> Bus notifier/ platform core/ device drivers
> >>
> >> I would say in device drivers.
> >
> > I tend to agree with that.
> >
> > It will help here to take a step back and remember what the problem we're
> > trying to solve is.
> >
> > At the root is clock management. Our system comprise many clocks, and they
> > need to be handled. The Common Clock Framework nicely models the clocks,
> > and offers an API for drivers to retrieve device clocks and control them.
> > Drivers can thus implement clock management manually without much pain.
> >
> > A clock can be managed in roughly three different ways :
> >
> > - it can be enabled at probe time and disabled at remove time ;
> >
> > - it can be enabled right before the device leaves its idle state and
> > disabled when the device goes back to idle ; or
> >
> > - it can be enabled and disabled in a more fine-grained, device-specific
> > manner.
> >
> > The selected clock management granularity depends on constraints specific
> > to the device and on how aggressive power saving needs to be. Enabling
> > the clocks at probe time and disabling them at remove time is enough for
> > most devices, but leads to a high power consumption. For that reason the
> > second clock management scheme is often desired.
> >
> > Managing clocks manually in the driver is a valid option. However, when
> > adding runtime PM to the equation, and realizing that the clocks need to
> > be enabled in the runtime PM resume handler and disabled in the suspend
> > handler, the clock management code starts looking very similar in most
> > drivers. We're thus tempted to factorize it away from the drivers into a
> > shared location.
> >
> > It's important to note at this point that the goal here is only to
> > simplify drivers. Moving clock management code out of the drivers doesn't
> > (unless I'm missing something) open the door to new possibilities, it just
> > serves as a simplification.
> >
> > Now, as Grygorii mentioned, differences between how a given IP core is
> > integrated in various SoCs can make clock management SoC-dependent. In the
> > vast majority of cases (which is really what we need to target, given that
> > our target is simplifying drivers) SoC integration can be described as a
> > list of clocks that must be managed. That list can be common to all
> > devices in a given SoC, or can be device-dependent as well.
>
> That's actually a problem - now we have static list of managed clocks
> per-SoC and not per device.
>
> > Few locations can be used to express a per-device list of per-SoC clocks.
> > We can have clocks lists in a per-SoC and per-device location, per-device
> > clocks lists in an SoC-specific location, or per-SoC clocks lists in a
> > device- specific location.
> >
> > The first option would require listing clocks to be managed by runtime PM
> > in DT nodes, as proposed by this patch set. I don't think this is the
> > best option, as that information is a mix of hardware description and
> > software policy, with the hardware description part being already present
> > in DT in the clocks property.
>
> I'm not fully agree here. The clock is "functional clock" If It's managed by
> runtime PM. And all such clocks need to be enabled/disabled always when
> device is powered on/off. So, from my point of view it's HW description and
> it follows TRM.
>
> Other clocks are optional
That's actually use-case dependent, some of them might be mandatory.
> and only drivers should control them.
> And question is how to define sets of such clocks in the best way?
>
> > The second option calls for storing the lists in SoC code under arch/. As
> > we're trying to minimize the amount of SoC code there (and even remove SoC
> > code completely when possible) I don't think that's a good option.
> >
> > The third option would require storing the clocks lists in device drivers.
> > I believe this is our best option, as a trade-off between simplicity and
> > versatility. Drivers that use runtime PM already need to enable it
> > explicitly when probing devices. Passing a list of clock names to runtime
> > PM at that point wouldn't complicate drivers much. When the clocks list
> > isn't SoC- dependent it could be stored as static information. Otherwise
> > it could be derived from DT (or any other source of hardware description)
> > using C code, offering all the versatility we need.
>
> Ok. if I understand right, you propose smth like this:
> 1) DT based solution:
>
> devA {
> clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
> rpm-clocks = <&clkpa>, <&clkcpgmac>;
> - or -
> clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
> clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
> rpm-clocks = "clk_pa", "clk_cpgmac";
> }
On a side note I believe the "rpm-clocks" name is too tied to the Linux
implementation. A name similar to "functional-clocks" would be better.
> in driver:
> pm_runtime_enable();
>
> |- of_clk_register_runtime_pm_clocks()
>
> - or -
> of_clk_register_runtime_pm_clocks()
> pm_runtime_enable();
I prefer the second option, as an explicit opt-in is less likely to cause
regressions, and would also offer an easy way for drivers to opt-out.
> 2) Static solution:
> char *con_ids_davinci[] =
> { "fck", "master", "slave", NULL };
> char *con_ids_keystone[] =
> { "clk_pa", "clk_cpgmac" };
>
> static struct of_device_id of_match[] = {
> { .compatible = "ti,keystone", con_ids_keystone},
> { .compatible = "ti,davinci", con_ids_davinci},
> {},
> };
>
> Personally, I like option 1 and, seems, it will not break ABI.
Is option 2 really representative of most use cases ? The list of clock inputs
to an IP core is a property of the IP core itself. How those inputs are
connected in the SoC is a property of the SoC integration. The clocks
references in DT can thus vary per-SoC, but the clock names should be pretty
much constant for a given IP core. Thus, if we have a single list of clocks to
manager for a given IP core, it shouldn't be difficult to pass that list to
the of_clk_register_runtime_pm_clocks() function.
> > The only drawback of this solution I can think of right now is that the
> > runtime PM core couldn't manage device clocks before probing the device.
> > Specifically device clocks couldn't be managed if no driver is loaded for
> > that device. I somehow recall that someone raised this as being a
> > problem, but I can't remember why.
>
> I can recollect only OMAP2+ SoCs where some abstraction called HW_MOD is
> used during platform initialization to reset all devices and turn off
> unused ones before probing the devices. But clock_ops are not used by
> OMAP2+:)
--
Regards,
Laurent Pinchart
Hi Kevin,
On Monday 08 September 2014 13:13:25 Kevin Hilman wrote:
> Laurent Pinchart <[email protected]> writes:
> > On Monday 28 July 2014 23:52:34 Grant Likely wrote:
> >> On Mon, Jul 28, 2014 at 11:47 AM, Grygorii Strashko wrote:
> >> > On 07/28/2014 05:05 PM, Grant Likely wrote:
> >> >> On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko wrote:
> [...]
>
> >> > - Where and when to call of_clk_register_runtime_pm_clocks()?
> >> >
> >> > Bus notifier/ platform core/ device drivers
> >>
> >> I would say in device drivers.
> >
> > I tend to agree with that.
> >
> > It will help here to take a step back and remember what the problem we're
> > trying to solve is.
>
> [jumping in late, after Grygorii ping'd me about looking at this]
>
> Laurent, thanks for summarizing the problem so well. It helped me
> catchup on the discussion.
You're welcome. Sorry for the very late reply.
> > At the root is clock management. Our system comprise many clocks, and they
> > need to be handled. The Common Clock Framework nicely models the clocks,
> > and offers an API for drivers to retrieve device clocks and control them.
> > Drivers can thus implement clock management manually without much pain.
> >
> > A clock can be managed in roughly three different ways :
> >
> > - it can be enabled at probe time and disabled at remove time ;
> >
> > - it can be enabled right before the device leaves its idle state and
> > disabled when the device goes back to idle ; or
> >
> > - it can be enabled and disabled in a more fine-grained, device-specific
> > manner.
> >
> > The selected clock management granularity depends on constraints specific
> > to the device and on how aggressive power saving needs to be. Enabling
> > the clocks at probe time and disabling them at remove time is enough for
> > most devices, but leads to a high power consumption. For that reason the
> > second clock management scheme is often desired.
> >
> > Managing clocks manually in the driver is a valid option. However, when
> > adding runtime PM to the equation, and realizing that the clocks need to
> > be enabled in the runtime PM resume handler and disabled in the suspend
> > handler, the clock management code starts looking very similar in most
> > drivers. We're thus tempted to factorize it away from the drivers into a
> > shared location.
> >
> > It's important to note at this point that the goal here is only to
> > simplify drivers. Moving clock management code out of the drivers doesn't
> > (unless I'm missing something) open the door to new possibilities, it just
> > serves as a simplification.
>
> I disagree. Actually, it opens up the door to lots of new possibilities
> that are crucial for fine-grained PM with QoS. It is not just
> simplification. There are many good reasons that some SoCs have moved all
> the management of PM-related clocks *out* of device drivers. More on that
> below...
>
> > Now, as Grygorii mentioned, differences between how a given IP core is
> > integrated in various SoCs can make clock management SoC-dependent. In the
> > vast majority of cases (which is really what we need to target, given that
> > our target is simplifying drivers) SoC integration can be described as a
> > list of clocks that must be managed. That list can be common to all
> > devices in a given SoC, or can be device-dependent as well.
>
> If we care about fine-grained PM, this is a way-too oversimplified
> version of what SoC integragion means.
>
> There are lots of pieces which fall under "SoC integration", for
> example: clock domains, power domains, voltage domains, SoC-specific
> wakeup capabilities, etc. etc. Then, for fun throw in QoS constraints,
> and things get really exciting.
>
> IOW, if you care about fine-grained PM and QoS, you simply can't reduce
> SoC integration down to "a list of clocks to be managed."
Of course. I was talking about SoC integration for clocks, not about SoC
integration in general.
> QoS makes this interesting as well because a device driver's decision to
> gate its own clocks may have serious repercussions on the wakeup latency
> of *other* devices in the same power domain. For example, the clock gating
> of the last active device in a powerdomain may cause the enclosing power-
> domain to be power gated, having a major impact on the wakup latency of
> *all* devices in that power domain.
>
> So if we're going to manage the list of PM-related clocks in the device
> driver, we'll also keep track of all the other devices in the same power
> domain, whether or not they're active, whether or not they have QoS
> constraints, etc. etc. Hopefully you can see that we're quickly way outside
> the scope of the IP block that the device driver is intended to manage.
>
> All of this is "SoC integration" knowledge, and IMO doen't belong in the
> device drivers. It belongs at the SoC integration level, and in todays
> kernel frameworks that means pm_domain/genpd.
Ok, there's more to it than I initially thought. Let's see how we can make
this happen then :-)
> > Few locations can be used to express a per-device list of per-SoC clocks.
> > We can have clocks lists in a per-SoC and per-device location, per-device
> > clocks lists in an SoC-specific location, or per-SoC clocks lists in a
> > device- specific location.
> >
> > The first option would require listing clocks to be managed by runtime PM
> > in DT nodes, as proposed by this patch set. I don't think this is the
> > best option, as that information is a mix of hardware description and
> > software policy, with the hardware description part being already present
> > in DT in the clocks property.
>
> I'm not seeing which part you think is software policy here? Which clocks
> are driving which IP blocks is a hardware description.
>
> Which clocks are actually gated, and if/when is software policy and should
> be decided by the SoC-specific runtime PM and genpd implementations, but
> describing which clocks are wired to which IP blocks is certainly hardware
> description IMO.
The clocks property is a hardware description, but a new property that would
hold a list of clocks to be managed by the runtime PM core is less so.
> > The second option calls for storing the lists in SoC code under arch/. As
> > we're trying to minimize the amount of SoC code there (and even remove SoC
> > code completely when possible) I don't think that's a good option.
> >
> > The third option would require storing the clocks lists in device drivers.
> > I believe this is our best option, as a trade-off between simplicity and
> > versatility. Drivers that use runtime PM already need to enable it
> > explicitly when probing devices. Passing a list of clock names to runtime
> > PM at that point wouldn't complicate drivers much. When the clocks list
> > isn't SoC- dependent it could be stored as static information. Otherwise
> > it could be derived from DT (or any other source of hardware description)
> > using C code, offering all the versatility we need.
>
> As is probably clear from above, I don't like this approach at all.
Do we at least agree that option 2 (storing the lists in arch/) is a bad idea
and should be ruled out ?
> > The only drawback of this solution I can think of right now is that the
> > runtime PM core couldn't manage device clocks before probing the device.
> > Specifically device clocks couldn't be managed if no driver is loaded for
> > that device. I somehow recall that someone raised this as being a
> > problem, but I can't remember why.
>
> The bigger drawback of this approach is that the device-drivers become a
> repository for SoC integration details that IMO don't belong in a device
> driver for a specific IP block.
I agree that SoC integration details don't belong in device drivers, but I
don't see this as SoC integration details. The driver knows that the IP core
it manages has two functional clock inputs, and knows how those clock inputs
are named. The name is a property of the IP core, not of the SoC. Only how
(and whether) those clocks are connected in the SoC is integration
information.
Now, if the IP core itself can be synthesized with different clock option,
those synthesis options belong to DT, either explicitly in the clock-names
property or implicitly in the compatible property.
> Over the last few years, we've created abstractions for this kind of SoC
> integration information (pm_domains) as well as frameworks for handling
> much of the common parts (genpd) and in doing so, have been able to
> remove PM-related clock management from device drivers altogether.
>
> I think managing this stuff back in device drivers would be a major step
> backwards.
--
Regards,
Laurent Pinchart