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

Commit Message

Geert Uytterhoeven April 24, 2014, 10:13 a.m. UTC

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 <[e-mail]>
---
 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

Comments

Ulf Hansson April 24, 2014, 1:11 p.m. UTC | #1

On 24 April 2014 12:13, Geert Uytterhoeven <[e-mail]> 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 <[e-mail]>
> ---
>  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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven April 24, 2014, 2:09 p.m. UTC | #2

Hi Ulf,

On Thu, Apr 24, 2014 at 3:11 PM, Ulf Hansson <[e-mail]> 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 -- [e-mail]

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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman April 25, 2014, 11:44 p.m. UTC | #3

Geert Uytterhoeven <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tomasz Figa April 26, 2014, 1:59 a.m. UTC | #4

On 24.04.2014 15:11, Ulf Hansson wrote:
> On 24 April 2014 12:13, Geert Uytterhoeven <[e-mail]> 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 <[e-mail]>
>> ---
>>   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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Likely April 29, 2014, 1:16 p.m. UTC | #5

On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <[e-mail]> wrote:
> Geert Uytterhoeven <[e-mail]> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart April 30, 2014, 9:23 p.m. UTC | #6

Hi Ulf and Geert,

On Thursday 24 April 2014 15:11:24 Ulf Hansson wrote:
> On 24 April 2014 12:13, Geert Uytterhoeven <[e-mail]> 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 <[e-mail]>

Laurent Pinchart April 30, 2014, 9:25 p.m. UTC | #7

On Tuesday 29 April 2014 14:16:10 Grant Likely wrote:
> On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <[e-mail]> wrote:
> > Geert Uytterhoeven <[e-mail]> 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.

Ben Dooks April 30, 2014, 9:33 p.m. UTC | #8

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 <[e-mail]> wrote:
>>> Geert Uytterhoeven <[e-mail]> 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.

Geert Uytterhoeven April 30, 2014, 9:47 p.m. UTC | #9

Hi Kevin,

On Sat, Apr 26, 2014 at 1:44 AM, Kevin Hilman <[e-mail]> wrote:
> Geert Uytterhoeven <[e-mail]> 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 -- [e-mail]

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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven April 30, 2014, 9:54 p.m. UTC | #10

Hi Grant,

On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <[e-mail]> wrote:
> On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <[e-mail]> wrote:
>> Geert Uytterhoeven <[e-mail]> 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 -- [e-mail]

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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven April 30, 2014, 10:06 p.m. UTC | #11

Hi Laurent,

On Wed, Apr 30, 2014 at 11:23 PM, Laurent Pinchart
<[e-mail]> wrote:
> On Thursday 24 April 2014 15:11:24 Ulf Hansson wrote:
>> On 24 April 2014 12:13, Geert Uytterhoeven <[e-mail]> 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 -- [e-mail]

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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Likely May 1, 2014, 8:03 a.m. UTC | #12

On Wed, 30 Apr 2014 23:54:37 +0200, Geert Uytterhoeven <[e-mail]> wrote:
> Hi Grant,
> 
> On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <[e-mail]> wrote:
> > On Fri, 25 Apr 2014 16:44:58 -0700, Kevin Hilman <[e-mail]> wrote:
> >> Geert Uytterhoeven <[e-mail]> 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 -- [e-mail]
> 
> 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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven May 1, 2014, 1:41 p.m. UTC | #13

Hi Grant,

On Thu, May 1, 2014 at 10:03 AM, Grant Likely <[e-mail]> wrote:
> On Wed, 30 Apr 2014 23:54:37 +0200, Geert Uytterhoeven <[e-mail]> wrote:
>> On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <[e-mail]> 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 -- [e-mail]

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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Likely May 1, 2014, 1:56 p.m. UTC | #14

On Thu, May 1, 2014 at 2:41 PM, Geert Uytterhoeven <[e-mail]> wrote:
> Hi Grant,
>
> On Thu, May 1, 2014 at 10:03 AM, Grant Likely <[e-mail]> wrote:
>> On Wed, 30 Apr 2014 23:54:37 +0200, Geert Uytterhoeven <[e-mail]> wrote:
>>> On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <[e-mail]> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven May 1, 2014, 2:46 p.m. UTC | #15

Hi Grant,

On Thu, May 1, 2014 at 3:56 PM, Grant Likely <[e-mail]> wrote:
> On Thu, May 1, 2014 at 2:41 PM, Geert Uytterhoeven <[e-mail]> wrote:
>> On Thu, May 1, 2014 at 10:03 AM, Grant Likely <[e-mail]> wrote:
>>> On Wed, 30 Apr 2014 23:54:37 +0200, Geert Uytterhoeven <[e-mail]> wrote:
>>>> On Tue, Apr 29, 2014 at 3:16 PM, Grant Likely <[e-mail]> 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 -- [e-mail]

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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Hansson May 2, 2014, 8:13 a.m. UTC | #16

>>
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Hansson May 2, 2014, 8:56 a.m. UTC | #17

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven May 2, 2014, 2:35 p.m. UTC | #18

Hi Ulf,

On Fri, May 2, 2014 at 10:56 AM, Ulf Hansson <[e-mail]> 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 -- [e-mail]

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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven May 2, 2014, 2:58 p.m. UTC | #19

Hi Ulf, Tomasz,

On Fri, May 2, 2014 at 10:13 AM, Ulf Hansson <[e-mail]> 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 -- [e-mail]

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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Hansson May 6, 2014, 7:43 a.m. UTC | #20

On 2 May 2014 16:35, Geert Uytterhoeven <[e-mail]> wrote:
> Hi Ulf,
>
> On Fri, May 2, 2014 at 10:56 AM, Ulf Hansson <[e-mail]> 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 -- [e-mail]
>
> 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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Hansson May 6, 2014, 7:58 a.m. UTC | #21

On 2 May 2014 16:58, Geert Uytterhoeven <[e-mail]> wrote:
> Hi Ulf, Tomasz,
>
> On Fri, May 2, 2014 at 10:13 AM, Ulf Hansson <[e-mail]> 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 -- [e-mail]
>
> 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 linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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 */