[PATCH/RFC 0/4] of: Register clocks for Runtime PM with PM core

[PATCH/RFC 0/4] of: Register clocks for Runtime PM with PM core

[Geert Uytterhoeven]

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 -- geert@linux-m68k.org

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

[PATCH/RFC 1/4] clk: Add CLK_RUNTIME_PM and clk_may_runtime_pm()

[Geert Uytterhoeven]

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 <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 2/4] PM / clock_ops: Add pm_clk_add_clk()

[Geert Uytterhoeven]

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 <geert+renesas@glider.be>
---
 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

[PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Geert Uytterhoeven]

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 <geert+renesas@glider.be>
---
 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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Ulf Hansson]

On 24 April 2014 12:13, Geert Uytterhoeven <geert+renesas@glider.be> 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 <geert+renesas@glider.be>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Apr 24, 2014 at 3:11 PM, Ulf Hansson <ulf.hansson@linaro.org> 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 -- geert@linux-m68k.org

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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Tomasz Figa]

On 24.04.2014 15:11, Ulf Hansson wrote:
> On 24 April 2014 12:13, Geert Uytterhoeven <geert+renesas@glider.be> 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 <geert+renesas@glider.be>
>> ---
>>   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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Ulf Hansson]

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

[Snip]

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

So, in principle you prefer to have driver's handle clock gating to
save power from their runtime PM callbacks, instead of from the power
domain, right? Just to clarify, that's my view as well.

Kind regards
Ulf Hansson

>
> I believe, though, that for devices that are not inside a controllable power
> domain, this might be a good solution.
>
> Best regards,
> Tomasz

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Geert Uytterhoeven]

Hi Ulf, Tomasz,

On Fri, May 2, 2014 at 10:13 AM, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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.
>>
>> 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).
>
> So, in principle you prefer to have driver's handle clock gating to
> save power from their runtime PM callbacks, instead of from the power
> domain, right? Just to clarify, that's my view as well.

If there's both a gate clock and a power domain, and the driver's Runtime PM
callbacks handle clock gating, who's handling the power domain?

Gr{oetje,eeting}s,

                        Geert (still trying to fit all pieces of the
puzzle together)

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Ulf Hansson]

On 2 May 2014 16:58, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf, Tomasz,
>
> On Fri, May 2, 2014 at 10:13 AM, Ulf Hansson <ulf.hansson@linaro.org> 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.
>>>
>>> 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).
>>
>> So, in principle you prefer to have driver's handle clock gating to
>> save power from their runtime PM callbacks, instead of from the power
>> domain, right? Just to clarify, that's my view as well.
>
> If there's both a gate clock and a power domain, and the driver's Runtime PM
> callbacks handle clock gating, who's handling the power domain?

This is my view, not sure everybody agrees :-)

1. If you have a hardware power domain you need to implement a
pm_domain (preferably use the generic power domain).

2. If you don't have a hardware power domain, but still cares about
having a centralized solution for dev_pm_qos - you may use the generic
power domain, since it supports this.

3. If none of the above, you don't need a pm_domain at all.

Kind regards
Ulf Hansson

>
> Gr{oetje,eeting}s,
>
>                         Geert (still trying to fit all pieces of the
> puzzle together)
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Laurent Pinchart]

Hi Ulf and Geert,

On Thursday 24 April 2014 15:11:24 Ulf Hansson wrote:
> On 24 April 2014 12:13, Geert Uytterhoeven <geert+renesas@glider.be> 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 <geert+renesas@glider.be>

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Apr 30, 2014 at 11:23 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thursday 24 April 2014 15:11:24 Ulf Hansson wrote:
>> On 24 April 2014 12:13, Geert Uytterhoeven <geert+renesas@glider.be> 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 -- geert@linux-m68k.org

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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Kevin Hilman]

Geert Uytterhoeven <geert+renesas@glider.be> 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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Grant Likely]

On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <khilman@linaro.org> wrote:
> Geert Uytterhoeven <geert+renesas@glider.be> 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.

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Laurent Pinchart]

On Tuesday 29 April 2014 14:16:10 Grant Likely wrote:
> On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org> 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Ben Dooks]

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 <khilman@linaro.org> wrote:
>>> Geert Uytterhoeven <geert+renesas@glider.be> 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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Geert Uytterhoeven]

Hi Grant,

On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <khilman@linaro.org> wrote:
>> Geert Uytterhoeven <geert+renesas@glider.be> 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 -- geert@linux-m68k.org

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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Grant Likely]

On Wed, 30 Apr 2014 23:54:37 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Grant,
> 
> On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <khilman@linaro.org> wrote:
> >> Geert Uytterhoeven <geert+renesas@glider.be> 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.

Help me out here becasue I don't understand how that works with this
patch set. From my, admittedly naive, reading it looks like the setup is
being done at device creation time, but if the driver (or module) gets
to declare which clocks need to be enabled in order to work, then that
information is not available at device creation time.

> 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 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Geert Uytterhoeven]

Hi Grant,

On Thu, May 1, 2014 at 10:03 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, 30 Apr 2014 23:54:37 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> > 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.
>
> Help me out here becasue I don't understand how that works with this
> patch set. From my, admittedly naive, reading it looks like the setup is
> being done at device creation time, but if the driver (or module) gets
> to declare which clocks need to be enabled in order to work, then that
> information is not available at device creation time.

Setup is indeed done at registration time. Note the check calling
clk_may_runtime_pm(), which is introduced in "[PATCH/RFC 1/4] clk: Add
CLK_RUNTIME_PM and clk_may_runtime_pm()".

Clock drivers are initialized much earlier, so they can set the CLK_RUNTIME_PM
flag for suitable clocks before platform devices are created from DT, cfr. the
example for shmobile MSTP clocks in "[PATCH/RFC 4/4] clk: shmobile: mstp:
Set CLK_RUNTIME_PM flag".

I hope this makes it clear.

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Grant Likely]

On Thu, May 1, 2014 at 2:41 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Grant,
>
> On Thu, May 1, 2014 at 10:03 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Wed, 30 Apr 2014 23:54:37 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> > 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.
>>
>> Help me out here becasue I don't understand how that works with this
>> patch set. From my, admittedly naive, reading it looks like the setup is
>> being done at device creation time, but if the driver (or module) gets
>> to declare which clocks need to be enabled in order to work, then that
>> information is not available at device creation time.
>
> Setup is indeed done at registration time. Note the check calling
> clk_may_runtime_pm(), which is introduced in "[PATCH/RFC 1/4] clk: Add
> CLK_RUNTIME_PM and clk_may_runtime_pm()".
>
> Clock drivers are initialized much earlier, so they can set the CLK_RUNTIME_PM
> flag for suitable clocks before platform devices are created from DT, cfr. the
> example for shmobile MSTP clocks in "[PATCH/RFC 4/4] clk: shmobile: mstp:
> Set CLK_RUNTIME_PM flag".

This is where I have issue. You're *assuming* clock drivers are
initialized much earlier. That is not guaranteed. It is perfectly
valid for clocks to be set up by a normal device driver, just like for
interrupt controllers or gpio controllers.

g.

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Geert Uytterhoeven]

Hi Grant,

On Thu, May 1, 2014 at 3:56 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, May 1, 2014 at 2:41 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, May 1, 2014 at 10:03 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Wed, 30 Apr 2014 23:54:37 +0200, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>> > 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.
>>>
>>> Help me out here becasue I don't understand how that works with this
>>> patch set. From my, admittedly naive, reading it looks like the setup is
>>> being done at device creation time, but if the driver (or module) gets
>>> to declare which clocks need to be enabled in order to work, then that
>>> information is not available at device creation time.
>>
>> Setup is indeed done at registration time. Note the check calling
>> clk_may_runtime_pm(), which is introduced in "[PATCH/RFC 1/4] clk: Add
>> CLK_RUNTIME_PM and clk_may_runtime_pm()".
>>
>> Clock drivers are initialized much earlier, so they can set the CLK_RUNTIME_PM
>> flag for suitable clocks before platform devices are created from DT, cfr. the
>> example for shmobile MSTP clocks in "[PATCH/RFC 4/4] clk: shmobile: mstp:
>> Set CLK_RUNTIME_PM flag".
>
> This is where I have issue. You're *assuming* clock drivers are
> initialized much earlier. That is not guaranteed. It is perfectly
> valid for clocks to be set up by a normal device driver, just like for
> interrupt controllers or gpio controllers.

OK, I didn't know that. In that case, nothing happens, and everything
works like before. So the drivers still have to take care of the
clocks themselves.

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Geert Uytterhoeven]

Hi Kevin,

On Sat, Apr 26, 2014 at 1:44 AM, Kevin Hilman <khilman@linaro.org> wrote:
> Geert Uytterhoeven <geert+renesas@glider.be> 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 -- geert@linux-m68k.org

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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Ulf Hansson]

 Hi Geert,

Some more review comments.

> +
> +
> +#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);

What about slow clocks? Those aren't handled with pm_clk_suspend().

> +       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);

What about slow clocks? Those aren't handled with pm_clk_resume().

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

Drivers/subsystems may invoke pm_runtime_force_suspend|resume() from
some of their system PM callbacks, which requires the runtime PM
callbacks to be defined for CONFIG_PM instead of CONFIG_PM_RUNTIME, I
believe that should be changed here as well.

> +               USE_PLATFORM_PM_SLEEP_OPS

What about other buses beside the platfrom bus. Certainly we need to
handle devices attached to any other subsystem type as well.

Kind regards
Ulf Hansson

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Geert Uytterhoeven]

Hi Ulf,

On Fri, May 2, 2014 at 10:56 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> +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);
>
> What about slow clocks? Those aren't handled with pm_clk_suspend().

How are slow clocks handled?

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Ulf Hansson]

On 2 May 2014 16:35, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Fri, May 2, 2014 at 10:56 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> +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);
>>
>> What about slow clocks? Those aren't handled with pm_clk_suspend().
>
> How are slow clocks handled?

clk_prepare|unprepare - these functions may sleep.

Kind regards
Ulf Hansson

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[PATCH/RFC 4/4] clk: shmobile: mstp: Set CLK_RUNTIME_PM flag

[Geert Uytterhoeven]

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 <geert+renesas@glider.be>
---
 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

Re: [PATCH/RFC 0/4] of: Register clocks for Runtime PM with PM core

[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

Re: [PATCH/RFC 0/4] of: Register clocks for Runtime PM with PM core

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Apr 30, 2014 at 11:29 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> 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 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 0/2] use named clocks list to register clocks for PM clock domain

[Grygorii Strashko]

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

[RFC PATCH 1/2] clk: of: introduce of_clk_get_from_set()

[Grygorii Strashko]

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 <grygorii.strashko@ti.com>
---
 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

[RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Grygorii Strashko]

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 <grygorii.strashko@ti.com>
---
 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

Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Grant Likely]

On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko <grygorii.strashko@ti.com> 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 <grygorii.strashko@ti.com>
> ---
>  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.

Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Grygorii Strashko]

Hi Grant.

On 07/28/2014 05:05 PM, Grant Likely wrote:
> On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> 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 <grygorii.strashko-l0cyMroinI0@public.gmane.org>
>> ---
>>   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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Grant Likely]

On Mon, Jul 28, 2014 at 11:47 AM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> Hi Grant.
>
> On 07/28/2014 05:05 PM, Grant Likely wrote:
>> On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko <grygorii.strashko@ti.com> 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 <grygorii.strashko@ti.com>
>>> ---
>>>   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.

Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Laurent Pinchart]

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 <grygorii.strashko@ti.com>
> >>> ---
> >>> 
> >>>   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

Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Grygorii Strashko]

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 <grygorii.strashko@ti.com>
>>>>> ---
>>>>>
>>>>>    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

Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Laurent Pinchart]

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 <grygorii.strashko@ti.com>
> >>>>> ---
> >>>>> 
> >>>>>    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

Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Jul 30, 2014 at 2:06 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> 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.

Perhaps you're thinking of clocks that were enabled (by the boot loader or
implicit reset state) before running Linux, and aren't disabled?

That was fixed by commit bb178da701382a230e26d90cf94e8a24b280e0d9
("clk: shmobile: mstp: Fix the is_enabled() operation").

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Laurent Pinchart]

Hi Geert,

On Monday 04 August 2014 13:28:32 Geert Uytterhoeven wrote:
> On Wed, Jul 30, 2014 at 2:06 AM, Laurent Pinchart wrote:
> > 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.
> 
> Perhaps you're thinking of clocks that were enabled (by the boot loader or
> implicit reset state) before running Linux, and aren't disabled?

That wasn't the reason, I know that clk_disable_unused() takes care of that 
problem (provided the clock drivers behave correctly, the commit you mention 
below shows that's not always the case, but that's an unrelated issue).

> That was fixed by commit bb178da701382a230e26d90cf94e8a24b280e0d9
> ("clk: shmobile: mstp: Fix the is_enabled() operation").

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Kevin Hilman]

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> 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:

[...]

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

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

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.

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

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

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.

Kevin

Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain

[Laurent Pinchart]

Hi Kevin,

On Monday 08 September 2014 13:13:25 Kevin Hilman wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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