[FOR DISCUSSION 0/9] Dove PMU support

[FOR DISCUSSION 0/9] Dove PMU support

[Russell King - ARM Linux]

This is a re-posting of the patch set which I posted almost 10 months
ago to support the Dove PMU, with a few additional changes.  This set
is based upon 3.19.

In this set are:

* two patches which Rafael originally acked, but there was indecision
  last time around how to handle them due to potential conflicts with
  work that Ulf was doing.  These patches have been updated to apply
  cleanly to 3.19.  I don't know if people want to take these as
  fixes to the PM domain code or not (hence why I'm posting this
  series during the merge window - if it weren't for this, I'd hold
  it off.)

* what I regard as a fix to the PM domain code; as a result of a
  previous commit, the PM domain code mismatches the runtime PM state,
  which leads to the PM domain being unexpectedly left on.  This patch
  has been re-worked to try an alternative approach, syncing the PM
  domain state with the runtime PM state after the probe has completed.

* the addition of the core Dove PMU driver, which consists of a reset,
  IRQ controller, and power domains.  The reset and power domain code
  has to be closely related due to the power up/down requirements of
  the GPU/VPU subsystems needing to be performed atomically.  (This
  requirement prevents it using the MFD infrastructure, because we
  would need to hold spinlocks while calling several different
  sub-drivers.)

* addition of the RTC interrupt, so we can now receive and act on
  alarms generated by the Dove RTC.

* addition of the DT descriptions for the GPU and VPU power domains.
  These patches do not themselves add the DT descriptions for these
  units, so these patches serve as illustrations how these should be
  described.

I've also included the DT binding documentation as a separate patch this
time around, and moved the PMU driver to drivers/soc/dove.

There are some gotchas with it - PM domains need to be created prior
to any device which uses them being probed, so it is best if the driver
is initialised early in the kernel boot.  We may be able to get away
with a core_initcall() or a postcore_initcall().

I'd ideally like to get these queued for merging in the _next_ merge
window if at all possible, if only to reduce the number of patches I've
been carrying for the last few years.  Failing that, if at least the
patches which Rafael has already acked could be applied, that would at
least get /some/ form of forward progress.

 Documentation/devicetree/bindings/soc/dove/pmu.txt |  49 +++
 arch/arm/boot/dts/dove.dtsi                        |  25 ++
 arch/arm/mach-mvebu/Kconfig                        |   1 +
 drivers/base/platform.c                            |   2 +
 drivers/base/power/common.c                        |  15 +
 drivers/base/power/domain.c                        |  33 +-
 drivers/soc/Makefile                               |   1 +
 drivers/soc/dove/Makefile                          |   1 +
 drivers/soc/dove/pmu.c                             | 399 +++++++++++++++++++++
 include/linux/pm.h                                 |   1 +
 include/linux/pm_domain.h                          |   4 +
 include/linux/soc/dove/pmu.h                       |   6 +
 12 files changed, 532 insertions(+), 5 deletions(-)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/9] pm: domains: quieten down generic pm domains

[Russell King]

PM domains are rather noisy; scheduling behaviour can cause callbacks
to take longer, which causes them to spit out a warning-level message
each time a callback takes a little longer than the previous time.
There really isn't a need for this, except when debugging.

Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0d8780c04a5e..b3fbc21da2dc 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -173,8 +173,8 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
 	genpd->power_on_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
 	genpd_recalc_cpu_exit_latency(genpd);
-	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
-		genpd->name, "on", elapsed_ns);
+	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
+		 genpd->name, "on", elapsed_ns);
 
 	return ret;
 }
@@ -199,8 +199,8 @@ static int genpd_power_off(struct generic_pm_domain *genpd)
 
 	genpd->power_off_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
-	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
-		genpd->name, "off", elapsed_ns);
+	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
+		 genpd->name, "off", elapsed_ns);
 
 	return ret;
 }
-- 
1.8.3.1

Re: [PATCH 1/9] pm: domains: quieten down generic pm domains

[Ulf Hansson]

On 12 March 2015 at 19:30, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> PM domains are rather noisy; scheduling behaviour can cause callbacks
> to take longer, which causes them to spit out a warning-level message
> each time a callback takes a little longer than the previous time.
> There really isn't a need for this, except when debugging.
>
> Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/base/power/domain.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0d8780c04a5e..b3fbc21da2dc 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -173,8 +173,8 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
>         genpd->power_on_latency_ns = elapsed_ns;
>         genpd->max_off_time_changed = true;
>         genpd_recalc_cpu_exit_latency(genpd);
> -       pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
> -               genpd->name, "on", elapsed_ns);
> +       pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
> +                genpd->name, "on", elapsed_ns);
>
>         return ret;
>  }
> @@ -199,8 +199,8 @@ static int genpd_power_off(struct generic_pm_domain *genpd)
>
>         genpd->power_off_latency_ns = elapsed_ns;
>         genpd->max_off_time_changed = true;
> -       pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
> -               genpd->name, "off", elapsed_ns);
> +       pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
> +                genpd->name, "off", elapsed_ns);
>
>         return ret;
>  }
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/9] pm: domains: quieten down generic pm domains

[Kevin Hilman]

Russell King <rmk+kernel@arm.linux.org.uk> writes:

> PM domains are rather noisy; scheduling behaviour can cause callbacks
> to take longer, which causes them to spit out a warning-level message
> each time a callback takes a little longer than the previous time.
> There really isn't a need for this, except when debugging.
>
> Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Acked-by: Kevin Hilman <khilman@linaro.org>

We also take several timings even for callbacks that don't exist, and
I've got a forthcoming patch to clean that up as well.

Kevin

[PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Russell King]

pm_genpd_remove_device() should only be called with valid and present
pm domain.  There are circumstances where we may end up with something
that isn't a generic PM domain in dev->pm_domain (eg, vga_switcheroo
stuff.)

Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b3fbc21da2dc..11a1023fa64a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1509,7 +1509,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	dev_dbg(dev, "%s()\n", __func__);
 
-	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)
+	if (!pm_genpd_present(genpd) || IS_ERR_OR_NULL(dev)
 	    ||  IS_ERR_OR_NULL(dev->pm_domain)
 	    ||  pd_to_genpd(dev->pm_domain) != genpd)
 		return -EINVAL;
-- 
1.8.3.1

Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Ulf Hansson]

On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> pm_genpd_remove_device() should only be called with valid and present
> pm domain.  There are circumstances where we may end up with something
> that isn't a generic PM domain in dev->pm_domain (eg, vga_switcheroo
> stuff.)

Could the "vga_switcheroo" code deal with this instead?

>
> Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/base/power/domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index b3fbc21da2dc..11a1023fa64a 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1509,7 +1509,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>
>         dev_dbg(dev, "%s()\n", __func__);
>
> -       if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)
> +       if (!pm_genpd_present(genpd) || IS_ERR_OR_NULL(dev)

The are two issues with this approach.

1. pm_genpd_present() is build only for CONFIG_PM_SLEEP set.
2. pm_genpd_present() totally ignores holding the mutex which protects
the list of GPDs.

>             ||  IS_ERR_OR_NULL(dev->pm_domain)
>             ||  pd_to_genpd(dev->pm_domain) != genpd)
>                 return -EINVAL;
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kind regards
Uffe

Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Russell King - ARM Linux]

On Fri, Mar 13, 2015 at 09:56:02AM +0100, Ulf Hansson wrote:
> On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> > pm_genpd_remove_device() should only be called with valid and present
> > pm domain.  There are circumstances where we may end up with something
> > that isn't a generic PM domain in dev->pm_domain (eg, vga_switcheroo
> > stuff.)
> 
> Could the "vga_switcheroo" code deal with this instead?

How is there any possibility what so ever that vga_switcherroo could
deal with this?

The problem is if something which isn't a generic PM domain is registered
in dev->pm_domain, pm_genpd_remove_device() will treat it as a generic PM
domain, and try and de-reference it as if it were a generic PM domain.

The problem is the generic PM domain code _assuming_ that whatever it
finds in dev->pm_domain is always a generic PM domain.  That is a broken
assumption.

> 
> >
> > Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/base/power/domain.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index b3fbc21da2dc..11a1023fa64a 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -1509,7 +1509,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> >
> >         dev_dbg(dev, "%s()\n", __func__);
> >
> > -       if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)
> > +       if (!pm_genpd_present(genpd) || IS_ERR_OR_NULL(dev)
> 
> The are two issues with this approach.
> 
> 1. pm_genpd_present() is build only for CONFIG_PM_SLEEP set.
> 2. pm_genpd_present() totally ignores holding the mutex which protects
> the list of GPDs.

Okay, I'll fix both of those by making it always available and by
taking the appropriate lock, and removing the duplication in
genpd_dev_pm_detach().

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Geert Uytterhoeven]

On Fri, Mar 13, 2015 at 10:20 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Mar 13, 2015 at 09:56:02AM +0100, Ulf Hansson wrote:
>> On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
>> > pm_genpd_remove_device() should only be called with valid and present
>> > pm domain.  There are circumstances where we may end up with something
>> > that isn't a generic PM domain in dev->pm_domain (eg, vga_switcheroo
>> > stuff.)
>>
>> Could the "vga_switcheroo" code deal with this instead?
>
> How is there any possibility what so ever that vga_switcherroo could
> deal with this?
>
> The problem is if something which isn't a generic PM domain is registered
> in dev->pm_domain, pm_genpd_remove_device() will treat it as a generic PM
> domain, and try and de-reference it as if it were a generic PM domain.
>
> The problem is the generic PM domain code _assuming_ that whatever it
> finds in dev->pm_domain is always a generic PM domain.  That is a broken
> assumption.

Indeed, currently it's not possible to know which derivative of dev_pm_domain
is used. For genpd, you have to look it up in the genpd list.
This also complicates adding debug code.
In addition, it can be statically or dynamically allocated. Hence sometimes %ps
can tell you the type, sometimes it can't.

Should we add a type field to struct dev_pm_domain?

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 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Rafael J. Wysocki]

On Friday, March 13, 2015 01:45:52 PM Geert Uytterhoeven wrote:
> On Fri, Mar 13, 2015 at 10:20 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Fri, Mar 13, 2015 at 09:56:02AM +0100, Ulf Hansson wrote:
> >> On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> >> > pm_genpd_remove_device() should only be called with valid and present
> >> > pm domain.  There are circumstances where we may end up with something
> >> > that isn't a generic PM domain in dev->pm_domain (eg, vga_switcheroo
> >> > stuff.)
> >>
> >> Could the "vga_switcheroo" code deal with this instead?
> >
> > How is there any possibility what so ever that vga_switcherroo could
> > deal with this?
> >
> > The problem is if something which isn't a generic PM domain is registered
> > in dev->pm_domain, pm_genpd_remove_device() will treat it as a generic PM
> > domain, and try and de-reference it as if it were a generic PM domain.
> >
> > The problem is the generic PM domain code _assuming_ that whatever it
> > finds in dev->pm_domain is always a generic PM domain.  That is a broken
> > assumption.
> 
> Indeed, currently it's not possible to know which derivative of dev_pm_domain
> is used. For genpd, you have to look it up in the genpd list.
> This also complicates adding debug code.
> In addition, it can be statically or dynamically allocated. Hence sometimes %ps
> can tell you the type, sometimes it can't.
> 
> Should we add a type field to struct dev_pm_domain?

Then we'd have to keep track of all dev_pm_domain derivatives somehow and I'd
prefer to avoid that if possible.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Russell King - ARM Linux]

On Fri, Mar 13, 2015 at 09:56:02AM +0100, Ulf Hansson wrote:
> On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index b3fbc21da2dc..11a1023fa64a 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -1509,7 +1509,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> >
> >         dev_dbg(dev, "%s()\n", __func__);
> >
> > -       if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)
> > +       if (!pm_genpd_present(genpd) || IS_ERR_OR_NULL(dev)
> 
> The are two issues with this approach.
> 
> 1. pm_genpd_present() is build only for CONFIG_PM_SLEEP set.
> 2. pm_genpd_present() totally ignores holding the mutex which protects
> the list of GPDs.

Well, having looked more deeply at this, the PM domain code has more
problems.

void pm_genpd_syscore_poweroff(struct device *dev)
{
        genpd_syscore_switch(dev, true);
}
EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweroff);

void pm_genpd_syscore_poweron(struct device *dev)
{
        genpd_syscore_switch(dev, false);
}
EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);

both of these functions are externally callable without holding the
gpd_list_lock mutex, which results in:

static void genpd_syscore_switch(struct device *dev, bool suspend)
{
        struct generic_pm_domain *genpd;

        genpd = dev_to_genpd(dev);
        if (!pm_genpd_present(genpd))
                return;

calling this function without holding the gpd_list_lock either.

It also seems that there's a variety of ways which the generic PM domain
code gets from a struct device to a generic_pm_domain - one is the above.
Another is having this open coded:

        mutex_lock(&gpd_list_lock);
        list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
                if (&gpd->domain == dev->pm_domain) {
                        pd = gpd;
                        break;
                }
        }
        mutex_unlock(&gpd_list_lock);

This is only _not_ buggy because we only ever add to the list, never
remove from it, so we can be sure that a successfully obtained "pd"
will remain valid after the lock is dropped.

Another is:

        genpd = dev_to_genpd(dev);
        if (IS_ERR(genpd))
		...

when we're sure that dev->pm_domain points at a generic PM domain (in
the various runtime PM functions.)  It should be also noted that
dev_to_genpd() is exported to the whole kernel - is that safe?

In any case, I think dev_to_genpd() at least needs some commentry before
it saying that it is only for use when we know for certain that the
device has a generic_pm_domain attached, or the PM domain power is NULL
or an error pointer.

This is turning out to be a right can of worms...

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Kevin Hilman]

Russell King <rmk+kernel@arm.linux.org.uk> writes:

> pm_genpd_remove_device() should only be called with valid and present
> pm domain.  

Terminology nit: the patch adds a check for a valid an present genpd,
not pm domain (which I think of as dev->pm_domain.)

> There are circumstances where we may end up with something
> that isn't a generic PM domain in dev->pm_domain (eg, vga_switcheroo
> stuff.)

Wouldn't that particular case be caught by this check in pm_genpd_remove_device():

        || pd_to_genpd(dev->pm_domain) != genpd

Maybe I'm not quite following your description, but it looks like
someone is trying to call pm_genpd_remove_device() on something that was
not added with pm_genpd_add_device().  Where/how is that happening?

Kevin

Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Russell King - ARM Linux]

On Fri, Mar 13, 2015 at 09:33:14AM -0700, Kevin Hilman wrote:
> Russell King <rmk+kernel@arm.linux.org.uk> writes:
> 
> > pm_genpd_remove_device() should only be called with valid and present
> > pm domain.  
> 
> Terminology nit: the patch adds a check for a valid an present genpd,
> not pm domain (which I think of as dev->pm_domain.)

Right, people are sloppy with terminology, it's my turn to be equally
sloppy :)  I'll fix it eventually (but I've just sent the next round of
patches.)  Can you re-comment on the new set so it doesn't get lost
please?  I'm not about to immediately go through another update round for
such a trivial change (which alone takes about 5 minutes due to the
number of branches and patches on top of this stuff).

> > There are circumstances where we may end up with something
> > that isn't a generic PM domain in dev->pm_domain (eg, vga_switcheroo
> > stuff.)
> 
> Wouldn't that particular case be caught by this check in pm_genpd_remove_device():
> 
>         || pd_to_genpd(dev->pm_domain) != genpd

Well, let's look at what pd_to_genpd() does:

static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
{
        return container_of(pd, struct generic_pm_domain, domain);
}

Now, both pd_to_genpd() and dev_to_genpd() are available for use outside
of the genpd code.  What that means is that other code _can_ decide to
do this (and indeed I do have code which does this):

        struct generic_pm_domain *genpd = dev_to_genpd(dev);

        while (1) {
                if (pm_genpd_remove_device(genpd, dev) != -EAGAIN)
                        break;
                cond_resched();
        }

Now, as there is no way for anything outside of genpd to know whether the
return value from this is a real genpd structure or not - and
pm_genpd_remove_device() tries hard to validate the genpd passed in - but
not sufficiently to catch bad cases where dev->pm_domain is not attached
to a genpd.

What I'm doing is ensuring that the weak-but-looking-like-belt-and-braces
test in pm_genpd_remove_device() is actually sufficiently strong to catch
bad uses which may otherwise result in subtle out-of-bounds corruption or
list corruption.

Now, the alternative solution would be to have pm_genpd_lookup_name()
exported from the genpd code, so I can actually look up the genpd which
is _supposed_ to be registered against the device and use that instead.
However, that's potentially dangerous because there is no refcounting
against genpds obtained in this way.

So I decided better validation was the lesser of the evils.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe

[Russell King]

Synchronise the PM domain status with runtime PM's status after a
platform device has been probed.  This augments the solution in commit
2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach
completes").

The above commit added a call to power up the PM domain when a device
attaches to the domain in order to match the behaviour required by
drivers that make no use of runtime PM.  The assumption is that the
device driver will cause a runtime PM transition, which will synchronise
the PM domain state with the runtime PM state.

However, by default, runtime PM will assume that the device is initially
suspended, and some drivers may make use of this should they not need to
touch the hardware during probe.

In order to allow such drivers, trigger the PM domain code to check
whether the PM domain can be suspended after the probe function, undoing
the effect of the power-on prior to the probe.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/platform.c     |  2 ++
 drivers/base/power/common.c | 15 +++++++++++++++
 drivers/base/power/domain.c | 23 +++++++++++++++++++++++
 include/linux/pm.h          |  1 +
 include/linux/pm_domain.h   |  4 ++++
 5 files changed, 45 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 9421fed40905..552d1affc060 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev)
 		ret = drv->probe(dev);
 		if (ret)
 			dev_pm_domain_detach(_dev, true);
+		else
+			dev_pm_domain_sync(_dev);
 	}
 
 	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index b0f138806bbc..8c739a14d3c7 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
 		dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_sync - synchronise the PM domain state with its devices
+ * @dev: device corresponding with domain
+ *
+ * Synchronise the PM domain state with the recently probed device, which
+ * may be in a variety of PM states.  This ensures that a device which
+ * enables runtime PM in suspended state, and never transitions to active
+ * in its probe handler is properly suspended after the probe.
+ */
+void dev_pm_domain_sync(struct device *dev)
+{
+	if (dev->pm_domain && dev->pm_domain->sync)
+		dev->pm_domain->sync(dev);
+}
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 11a1023fa64a..13ae3355dff7 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
 	genpd_queue_power_off_work(pd);
 }
 
+static void genpd_dev_pm_sync(struct device *dev)
+{
+	struct generic_pm_domain *pd = NULL, *gpd;
+
+	if (!dev->pm_domain)
+		return;
+
+	mutex_lock(&gpd_list_lock);
+	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+		if (&gpd->domain == dev->pm_domain) {
+			pd = gpd;
+			break;
+		}
+	}
+	mutex_unlock(&gpd_list_lock);
+
+	if (!pd)
+		return;
+
+	genpd_queue_power_off_work(pd);
+}
+
 /**
  * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
  * @dev: Device to attach.
@@ -2223,6 +2245,7 @@ int genpd_dev_pm_attach(struct device *dev)
 	}
 
 	dev->pm_domain->detach = genpd_dev_pm_detach;
+	dev->pm_domain->sync = genpd_dev_pm_sync;
 	pm_genpd_poweron(pd);
 
 	return 0;
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 8b5976364619..676ca4055239 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev);
 struct dev_pm_domain {
 	struct dev_pm_ops	ops;
 	void (*detach)(struct device *dev, bool power_off);
+	void (*sync)(struct device *dev);
 };
 
 /*
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index a9edab2c787a..8d58b30e23ac 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,
 #ifdef CONFIG_PM
 extern int dev_pm_domain_attach(struct device *dev, bool power_on);
 extern void dev_pm_domain_detach(struct device *dev, bool power_off);
+void dev_pm_domain_sync(struct device *dev);
 #else
 static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
 {
 	return -ENODEV;
 }
 static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
+static inline void dev_pm_domain_sync(struct device *dev)
+{
+}
 #endif
 
 #endif /* _LINUX_PM_DOMAIN_H */
-- 
1.8.3.1

Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe

[Rafael J. Wysocki]

On Thursday, March 12, 2015 06:31:05 PM Russell King wrote:
> Synchronise the PM domain status with runtime PM's status after a
> platform device has been probed.  This augments the solution in commit
> 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach
> completes").
> 
> The above commit added a call to power up the PM domain when a device
> attaches to the domain in order to match the behaviour required by
> drivers that make no use of runtime PM.  The assumption is that the
> device driver will cause a runtime PM transition, which will synchronise
> the PM domain state with the runtime PM state.
> 
> However, by default, runtime PM will assume that the device is initially
> suspended, and some drivers may make use of this should they not need to
> touch the hardware during probe.
> 
> In order to allow such drivers, trigger the PM domain code to check
> whether the PM domain can be suspended after the probe function, undoing
> the effect of the power-on prior to the probe.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

That works for me.

Kevin, Ulf?

> ---
>  drivers/base/platform.c     |  2 ++
>  drivers/base/power/common.c | 15 +++++++++++++++
>  drivers/base/power/domain.c | 23 +++++++++++++++++++++++
>  include/linux/pm.h          |  1 +
>  include/linux/pm_domain.h   |  4 ++++
>  5 files changed, 45 insertions(+)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 9421fed40905..552d1affc060 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev)
>  		ret = drv->probe(dev);
>  		if (ret)
>  			dev_pm_domain_detach(_dev, true);
> +		else
> +			dev_pm_domain_sync(_dev);
>  	}
>  
>  	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index b0f138806bbc..8c739a14d3c7 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
>  		dev->pm_domain->detach(dev, power_off);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
> +
> +/**
> + * dev_pm_domain_sync - synchronise the PM domain state with its devices
> + * @dev: device corresponding with domain
> + *
> + * Synchronise the PM domain state with the recently probed device, which
> + * may be in a variety of PM states.  This ensures that a device which
> + * enables runtime PM in suspended state, and never transitions to active
> + * in its probe handler is properly suspended after the probe.
> + */
> +void dev_pm_domain_sync(struct device *dev)
> +{
> +	if (dev->pm_domain && dev->pm_domain->sync)
> +		dev->pm_domain->sync(dev);
> +}
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 11a1023fa64a..13ae3355dff7 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>  	genpd_queue_power_off_work(pd);
>  }
>  
> +static void genpd_dev_pm_sync(struct device *dev)
> +{
> +	struct generic_pm_domain *pd = NULL, *gpd;
> +
> +	if (!dev->pm_domain)
> +		return;
> +
> +	mutex_lock(&gpd_list_lock);
> +	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> +		if (&gpd->domain == dev->pm_domain) {
> +			pd = gpd;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&gpd_list_lock);
> +
> +	if (!pd)
> +		return;
> +
> +	genpd_queue_power_off_work(pd);
> +}
> +
>  /**
>   * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
>   * @dev: Device to attach.
> @@ -2223,6 +2245,7 @@ int genpd_dev_pm_attach(struct device *dev)
>  	}
>  
>  	dev->pm_domain->detach = genpd_dev_pm_detach;
> +	dev->pm_domain->sync = genpd_dev_pm_sync;
>  	pm_genpd_poweron(pd);
>  
>  	return 0;
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 8b5976364619..676ca4055239 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev);
>  struct dev_pm_domain {
>  	struct dev_pm_ops	ops;
>  	void (*detach)(struct device *dev, bool power_off);
> +	void (*sync)(struct device *dev);
>  };
>  
>  /*
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index a9edab2c787a..8d58b30e23ac 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,
>  #ifdef CONFIG_PM
>  extern int dev_pm_domain_attach(struct device *dev, bool power_on);
>  extern void dev_pm_domain_detach(struct device *dev, bool power_off);
> +void dev_pm_domain_sync(struct device *dev);
>  #else
>  static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
>  {
>  	return -ENODEV;
>  }
>  static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
> +static inline void dev_pm_domain_sync(struct device *dev)
> +{
> +}
>  #endif
>  
>  #endif /* _LINUX_PM_DOMAIN_H */
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe

[Ulf Hansson]

On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> Synchronise the PM domain status with runtime PM's status after a
> platform device has been probed.  This augments the solution in commit
> 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach
> completes").
>
> The above commit added a call to power up the PM domain when a device
> attaches to the domain in order to match the behaviour required by
> drivers that make no use of runtime PM.  The assumption is that the
> device driver will cause a runtime PM transition, which will synchronise
> the PM domain state with the runtime PM state.
>
> However, by default, runtime PM will assume that the device is initially
> suspended, and some drivers may make use of this should they not need to
> touch the hardware during probe.
>
> In order to allow such drivers, trigger the PM domain code to check
> whether the PM domain can be suspended after the probe function, undoing
> the effect of the power-on prior to the probe.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/base/platform.c     |  2 ++

Don't we need this for more buses than the platform bus?

>  drivers/base/power/common.c | 15 +++++++++++++++
>  drivers/base/power/domain.c | 23 +++++++++++++++++++++++
>  include/linux/pm.h          |  1 +
>  include/linux/pm_domain.h   |  4 ++++
>  5 files changed, 45 insertions(+)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 9421fed40905..552d1affc060 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev)
>                 ret = drv->probe(dev);
>                 if (ret)
>                         dev_pm_domain_detach(_dev, true);
> +               else
> +                       dev_pm_domain_sync(_dev);
>         }
>
>         if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index b0f138806bbc..8c739a14d3c7 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
>                 dev->pm_domain->detach(dev, power_off);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
> +
> +/**
> + * dev_pm_domain_sync - synchronise the PM domain state with its devices
> + * @dev: device corresponding with domain
> + *
> + * Synchronise the PM domain state with the recently probed device, which
> + * may be in a variety of PM states.  This ensures that a device which
> + * enables runtime PM in suspended state, and never transitions to active
> + * in its probe handler is properly suspended after the probe.
> + */
> +void dev_pm_domain_sync(struct device *dev)
> +{
> +       if (dev->pm_domain && dev->pm_domain->sync)
> +               dev->pm_domain->sync(dev);
> +}

This is more of a taste and flavour comment, regarding the design approach.

To address the issue which @subject patch does, and together with the
original problem, which was about making sure a PM domain stays
powered during probe, that to me seems like a perfect match for a
get/put API.

The current solution from commit 2ed127697eb1 ("PM / Domains: Power on
the PM domain right after attach completes"), is to me just a hacky
workaround (which obviously wasn't the proper solution) . $subject
patch follows that approach.

What do you think of using a get/put approach instead, that would also
give us nice symmetry of the API. As you know I have patches available
for this, I am happy to post them if needed.

> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 11a1023fa64a..13ae3355dff7 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>         genpd_queue_power_off_work(pd);
>  }
>
> +static void genpd_dev_pm_sync(struct device *dev)
> +{
> +       struct generic_pm_domain *pd = NULL, *gpd;
> +
> +       if (!dev->pm_domain)
> +               return;
> +
> +       mutex_lock(&gpd_list_lock);
> +       list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> +               if (&gpd->domain == dev->pm_domain) {
> +                       pd = gpd;
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&gpd_list_lock);

Walking through the gpd list seems a bit "heavy". Can't we just expect
the caller to have a valid generic_pm_domain pointer for its device?

> +
> +       if (!pd)
> +               return;
> +
> +       genpd_queue_power_off_work(pd);
> +}
> +
>  /**
>   * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
>   * @dev: Device to attach.
> @@ -2223,6 +2245,7 @@ int genpd_dev_pm_attach(struct device *dev)
>         }
>
>         dev->pm_domain->detach = genpd_dev_pm_detach;
> +       dev->pm_domain->sync = genpd_dev_pm_sync;
>         pm_genpd_poweron(pd);
>
>         return 0;
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 8b5976364619..676ca4055239 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev);
>  struct dev_pm_domain {
>         struct dev_pm_ops       ops;
>         void (*detach)(struct device *dev, bool power_off);
> +       void (*sync)(struct device *dev);
>  };
>
>  /*
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index a9edab2c787a..8d58b30e23ac 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,
>  #ifdef CONFIG_PM
>  extern int dev_pm_domain_attach(struct device *dev, bool power_on);
>  extern void dev_pm_domain_detach(struct device *dev, bool power_off);
> +void dev_pm_domain_sync(struct device *dev);
>  #else
>  static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
>  {
>         return -ENODEV;
>  }
>  static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
> +static inline void dev_pm_domain_sync(struct device *dev)
> +{
> +}
>  #endif
>
>  #endif /* _LINUX_PM_DOMAIN_H */
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kind regards
Uffe

Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe

[Russell King - ARM Linux]

On Fri, Mar 13, 2015 at 10:30:59AM +0100, Ulf Hansson wrote:
> On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> > Synchronise the PM domain status with runtime PM's status after a
> > platform device has been probed.  This augments the solution in commit
> > 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach
> > completes").
> >
> > The above commit added a call to power up the PM domain when a device
> > attaches to the domain in order to match the behaviour required by
> > drivers that make no use of runtime PM.  The assumption is that the
> > device driver will cause a runtime PM transition, which will synchronise
> > the PM domain state with the runtime PM state.
> >
> > However, by default, runtime PM will assume that the device is initially
> > suspended, and some drivers may make use of this should they not need to
> > touch the hardware during probe.
> >
> > In order to allow such drivers, trigger the PM domain code to check
> > whether the PM domain can be suspended after the probe function, undoing
> > the effect of the power-on prior to the probe.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/base/platform.c     |  2 ++
> 
> Don't we need this for more buses than the platform bus?

As you very well know, only the platform bus does this automatic binding
of a PM domain based on OF - something that you yourself were involved in
adding (your sign-off is on the patch adding it, so I assume that you
reviewed that patch as thoroughly as you seem to be reviewing mine) which
is the cause of my problems.

> >  drivers/base/power/common.c | 15 +++++++++++++++
> >  drivers/base/power/domain.c | 23 +++++++++++++++++++++++
> >  include/linux/pm.h          |  1 +
> >  include/linux/pm_domain.h   |  4 ++++
> >  5 files changed, 45 insertions(+)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 9421fed40905..552d1affc060 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev)
> >                 ret = drv->probe(dev);
> >                 if (ret)
> >                         dev_pm_domain_detach(_dev, true);
> > +               else
> > +                       dev_pm_domain_sync(_dev);
> >         }
> >
> >         if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> > index b0f138806bbc..8c739a14d3c7 100644
> > --- a/drivers/base/power/common.c
> > +++ b/drivers/base/power/common.c
> > @@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
> >                 dev->pm_domain->detach(dev, power_off);
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
> > +
> > +/**
> > + * dev_pm_domain_sync - synchronise the PM domain state with its devices
> > + * @dev: device corresponding with domain
> > + *
> > + * Synchronise the PM domain state with the recently probed device, which
> > + * may be in a variety of PM states.  This ensures that a device which
> > + * enables runtime PM in suspended state, and never transitions to active
> > + * in its probe handler is properly suspended after the probe.
> > + */
> > +void dev_pm_domain_sync(struct device *dev)
> > +{
> > +       if (dev->pm_domain && dev->pm_domain->sync)
> > +               dev->pm_domain->sync(dev);
> > +}
> 
> This is more of a taste and flavour comment, regarding the design approach.
> 
> To address the issue which @subject patch does, and together with the
> original problem, which was about making sure a PM domain stays
> powered during probe, that to me seems like a perfect match for a
> get/put API.
> 
> The current solution from commit 2ed127697eb1 ("PM / Domains: Power on
> the PM domain right after attach completes"), is to me just a hacky
> workaround (which obviously wasn't the proper solution) . $subject
> patch follows that approach.
> 
> What do you think of using a get/put approach instead, that would also
> give us nice symmetry of the API. As you know I have patches available
> for this, I am happy to post them if needed.

What I think you're proposing is nothing less than a total rewrite of the
PM domain code.

If that's what it's going to take to get this stuff in, then I'm just not
interested in persuing this anymore, sorry.  I don't have the time and
effort for that - something that people well know when they send me emails
that go unanswered...

I'm just trying to fix the problem which you created - and this is the way
which was discussed and settled upon.  If you _now_ want a different
approach, that's up to _you_ to implement.  Stop wasting my time.

> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 11a1023fa64a..13ae3355dff7 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
> >         genpd_queue_power_off_work(pd);
> >  }
> >
> > +static void genpd_dev_pm_sync(struct device *dev)
> > +{
> > +       struct generic_pm_domain *pd = NULL, *gpd;
> > +
> > +       if (!dev->pm_domain)
> > +               return;
> > +
> > +       mutex_lock(&gpd_list_lock);
> > +       list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> > +               if (&gpd->domain == dev->pm_domain) {
> > +                       pd = gpd;
> > +                       break;
> > +               }
> > +       }
> > +       mutex_unlock(&gpd_list_lock);
> 
> Walking through the gpd list seems a bit "heavy". Can't we just expect
> the caller to have a valid generic_pm_domain pointer for its device?

No you can't.  See the second patch in this series.  dev->pm_domain can
contain other stuff which isn't a generic_pm_domain.  generic_pm_domain
is just one instance of a pm_domain implementation.  Others exist.

I would have assumed you would know these details as you have decided to
co-maintain this code, or if not, you'd be prepared to audit the kernel
to find out what might be in dev->pm_domain so that you have due diligence
before commenting on something you clearly know nothing about...

What I find even _more_ unacceptable is that you are question this, but
you didn't question the exact same code which was part of Tomasz Figa's
patch to add the OF-based PM domain code.  On the face of it, this
strikes of double standards - somehow I have to justify my code more
than other people do.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe

[Ulf Hansson]

On 13 March 2015 at 11:14, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Mar 13, 2015 at 10:30:59AM +0100, Ulf Hansson wrote:
>> On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
>> > Synchronise the PM domain status with runtime PM's status after a
>> > platform device has been probed.  This augments the solution in commit
>> > 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach
>> > completes").
>> >
>> > The above commit added a call to power up the PM domain when a device
>> > attaches to the domain in order to match the behaviour required by
>> > drivers that make no use of runtime PM.  The assumption is that the
>> > device driver will cause a runtime PM transition, which will synchronise
>> > the PM domain state with the runtime PM state.
>> >
>> > However, by default, runtime PM will assume that the device is initially
>> > suspended, and some drivers may make use of this should they not need to
>> > touch the hardware during probe.
>> >
>> > In order to allow such drivers, trigger the PM domain code to check
>> > whether the PM domain can be suspended after the probe function, undoing
>> > the effect of the power-on prior to the probe.
>> >
>> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
>> > ---
>> >  drivers/base/platform.c     |  2 ++
>>
>> Don't we need this for more buses than the platform bus?
>
> As you very well know, only the platform bus does this automatic binding
> of a PM domain based on OF - something that you yourself were involved in
> adding (your sign-off is on the patch adding it, so I assume that you
> reviewed that patch as thoroughly as you seem to be reviewing mine) which
> is the cause of my problems.

Besides the platform bus, we also have spi, sdio, i2c, amba. I was
thinking that those should suffer some that same issue you are trying
to fix, right?

>
>> >  drivers/base/power/common.c | 15 +++++++++++++++
>> >  drivers/base/power/domain.c | 23 +++++++++++++++++++++++
>> >  include/linux/pm.h          |  1 +
>> >  include/linux/pm_domain.h   |  4 ++++
>> >  5 files changed, 45 insertions(+)
>> >
>> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> > index 9421fed40905..552d1affc060 100644
>> > --- a/drivers/base/platform.c
>> > +++ b/drivers/base/platform.c
>> > @@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev)
>> >                 ret = drv->probe(dev);
>> >                 if (ret)
>> >                         dev_pm_domain_detach(_dev, true);
>> > +               else
>> > +                       dev_pm_domain_sync(_dev);
>> >         }
>> >
>> >         if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
>> > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>> > index b0f138806bbc..8c739a14d3c7 100644
>> > --- a/drivers/base/power/common.c
>> > +++ b/drivers/base/power/common.c
>> > @@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
>> >                 dev->pm_domain->detach(dev, power_off);
>> >  }
>> >  EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
>> > +
>> > +/**
>> > + * dev_pm_domain_sync - synchronise the PM domain state with its devices
>> > + * @dev: device corresponding with domain
>> > + *
>> > + * Synchronise the PM domain state with the recently probed device, which
>> > + * may be in a variety of PM states.  This ensures that a device which
>> > + * enables runtime PM in suspended state, and never transitions to active
>> > + * in its probe handler is properly suspended after the probe.
>> > + */
>> > +void dev_pm_domain_sync(struct device *dev)
>> > +{
>> > +       if (dev->pm_domain && dev->pm_domain->sync)
>> > +               dev->pm_domain->sync(dev);
>> > +}
>>
>> This is more of a taste and flavour comment, regarding the design approach.
>>
>> To address the issue which @subject patch does, and together with the
>> original problem, which was about making sure a PM domain stays
>> powered during probe, that to me seems like a perfect match for a
>> get/put API.
>>
>> The current solution from commit 2ed127697eb1 ("PM / Domains: Power on
>> the PM domain right after attach completes"), is to me just a hacky
>> workaround (which obviously wasn't the proper solution) . $subject
>> patch follows that approach.
>>
>> What do you think of using a get/put approach instead, that would also
>> give us nice symmetry of the API. As you know I have patches available
>> for this, I am happy to post them if needed.
>
> What I think you're proposing is nothing less than a total rewrite of the
> PM domain code.

No, that's not what I proposed. Sorry if I was unclear.

Instead of the >sync() API+callback, my proposual was to add a ->get()
and a ->put() API+callback. The APIs will have to be called from the
buses probe functions, to make sure the PM domain stays powered during
probe and gets gated if possible when leaving probe.

That will then make it possible to revert 2ed127697eb1 ("PM / Domains:
Power on the PM domain right after attach completes")

I will (re)post these patches, since it's easier to discuss/look at
code directly.

>
> If that's what it's going to take to get this stuff in, then I'm just not
> interested in persuing this anymore, sorry.  I don't have the time and
> effort for that - something that people well know when they send me emails
> that go unanswered...
>
> I'm just trying to fix the problem which you created - and this is the way
> which was discussed and settled upon.  If you _now_ want a different
> approach, that's up to _you_ to implement.  Stop wasting my time.
>
>> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> > index 11a1023fa64a..13ae3355dff7 100644
>> > --- a/drivers/base/power/domain.c
>> > +++ b/drivers/base/power/domain.c
>> > @@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>> >         genpd_queue_power_off_work(pd);
>> >  }
>> >
>> > +static void genpd_dev_pm_sync(struct device *dev)
>> > +{
>> > +       struct generic_pm_domain *pd = NULL, *gpd;
>> > +
>> > +       if (!dev->pm_domain)
>> > +               return;
>> > +
>> > +       mutex_lock(&gpd_list_lock);
>> > +       list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
>> > +               if (&gpd->domain == dev->pm_domain) {
>> > +                       pd = gpd;
>> > +                       break;
>> > +               }
>> > +       }
>> > +       mutex_unlock(&gpd_list_lock);
>>
>> Walking through the gpd list seems a bit "heavy". Can't we just expect
>> the caller to have a valid generic_pm_domain pointer for its device?
>
> No you can't.  See the second patch in this series.  dev->pm_domain can
> contain other stuff which isn't a generic_pm_domain.  generic_pm_domain
> is just one instance of a pm_domain implementation.  Others exist.
>
> I would have assumed you would know these details as you have decided to
> co-maintain this code, or if not, you'd be prepared to audit the kernel
> to find out what might be in dev->pm_domain so that you have due diligence
> before commenting on something you clearly know nothing about...
>
> What I find even _more_ unacceptable is that you are question this, but
> you didn't question the exact same code which was part of Tomasz Figa's
> patch to add the OF-based PM domain code.  On the face of it, this
> strikes of double standards - somehow I have to justify my code more
> than other people do.

I am really sorry if I made you upset Russell, I was trying to ask for
you most valuable feedback, but apparently I failed miserably.

Kind regards
Uffe

Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe

[Russell King - ARM Linux]

On Fri, Mar 13, 2015 at 10:30:59AM +0100, Ulf Hansson wrote:
> On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> > Synchronise the PM domain status with runtime PM's status after a
> > platform device has been probed.  This augments the solution in commit
> > 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach
> > completes").
> >
> > The above commit added a call to power up the PM domain when a device
> > attaches to the domain in order to match the behaviour required by
> > drivers that make no use of runtime PM.  The assumption is that the
> > device driver will cause a runtime PM transition, which will synchronise
> > the PM domain state with the runtime PM state.
> >
> > However, by default, runtime PM will assume that the device is initially
> > suspended, and some drivers may make use of this should they not need to
> > touch the hardware during probe.
> >
> > In order to allow such drivers, trigger the PM domain code to check
> > whether the PM domain can be suspended after the probe function, undoing
> > the effect of the power-on prior to the probe.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/base/platform.c     |  2 ++
> 
> Don't we need this for more buses than the platform bus?

It's worth noting that I won't be fixing SDIO:

        sdio_acpi_set_handle(func);
        ret = device_add(&func->dev);
        if (ret == 0) {
                sdio_func_set_present(func);
                dev_pm_domain_attach(&func->dev, false);
        }

which is inherently racy - the driver can be probed in device_add().
What happened to "setup stuff, then publish"...

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe

[Kevin Hilman]

Russell King <rmk+kernel@arm.linux.org.uk> writes:

> Synchronise the PM domain status with runtime PM's status after a
> platform device has been probed.  This augments the solution in commit
> 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach
> completes").
>
> The above commit added a call to power up the PM domain when a device
> attaches to the domain in order to match the behaviour required by
> drivers that make no use of runtime PM.  The assumption is that the
> device driver will cause a runtime PM transition, which will synchronise
> the PM domain state with the runtime PM state.
>
> However, by default, runtime PM will assume that the device is initially
> suspended, and some drivers may make use of this should they not need to
> touch the hardware during probe.
>
> In order to allow such drivers, trigger the PM domain code to check
> whether the PM domain can be suspended after the probe function, undoing
> the effect of the power-on prior to the probe.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

I think this is a good fix to the existing problem.  One minor nit on a
comment below, otherwise

Acked-by: Kevin Hilman <khilman@linaro.org>

[...]

> +
> +/**
> + * dev_pm_domain_sync - synchronise the PM domain state with its devices
> + * @dev: device corresponding with domain
> + *
> + * Synchronise the PM domain state with the recently probed device, which
> + * may be in a variety of PM states.  This ensures that a device which
> + * enables runtime PM in suspended state, and never transitions to active
> + * in its probe handler is properly suspended after the probe.
> + */

Tt's not the *device* tha needs to be properly suspended after the probe
(since it's already/still runtime suspended), but the pm_domain that would
be potentially powered down.  Hence, I'd reword the last sentence slightly:

   This ensures that a device which enables runtime PM in suspended
   state, and never transitions to active in its probe handler gives an
   opportunity for the PM domain to be powered down after the probe.

Kevin

Re: [FOR DISCUSSION 0/9] Dove PMU support

[Arnd Bergmann]

On Thursday 12 March 2015 18:30:21 Russell King - ARM Linux wrote:
> This is a re-posting of the patch set which I posted almost 10 months
> ago to support the Dove PMU, with a few additional changes.  This set
> is based upon 3.19.
> 
> In this set are:
> 
> * two patches which Rafael originally acked, but there was indecision
>   last time around how to handle them due to potential conflicts with
>   work that Ulf was doing.  These patches have been updated to apply
>   cleanly to 3.19.  I don't know if people want to take these as
>   fixes to the PM domain code or not (hence why I'm posting this
>   series during the merge window - if it weren't for this, I'd hold
>   it off.)

I don't seem to have received the first three patches for some reason.
Can you check if you got them back from the mailing list?

> 
>  Documentation/devicetree/bindings/soc/dove/pmu.txt |  49 +++
>  arch/arm/boot/dts/dove.dtsi                        |  25 ++
>  arch/arm/mach-mvebu/Kconfig                        |   1 +
>  drivers/base/platform.c                            |   2 +
>  drivers/base/power/common.c                        |  15 +
>  drivers/base/power/domain.c                        |  33 +-
>  drivers/soc/Makefile                               |   1 +
>  drivers/soc/dove/Makefile                          |   1 +
>  drivers/soc/dove/pmu.c                             | 399 +++++++++++++++++++++
>  include/linux/pm.h                                 |   1 +
>  include/linux/pm_domain.h                          |   4 +
>  include/linux/soc/dove/pmu.h                       |   6 +
>  12 files changed, 532 insertions(+), 5 deletions(-)

I see add the header file and the global dove_init_pmu() function,
but I don't see a caller of that function. Is that intentional, or
did you accidentally leave out another patch that you meant to include?

	Arnd
--
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: [FOR DISCUSSION 0/9] Dove PMU support

[Russell King - ARM Linux]

On Fri, Mar 13, 2015 at 12:57:11PM +0100, Arnd Bergmann wrote:
> On Thursday 12 March 2015 18:30:21 Russell King - ARM Linux wrote:
> > This is a re-posting of the patch set which I posted almost 10 months
> > ago to support the Dove PMU, with a few additional changes.  This set
> > is based upon 3.19.
> > 
> > In this set are:
> > 
> > * two patches which Rafael originally acked, but there was indecision
> >   last time around how to handle them due to potential conflicts with
> >   work that Ulf was doing.  These patches have been updated to apply
> >   cleanly to 3.19.  I don't know if people want to take these as
> >   fixes to the PM domain code or not (hence why I'm posting this
> >   series during the merge window - if it weren't for this, I'd hold
> >   it off.)
> 
> I don't seem to have received the first three patches for some reason.
> Can you check if you got them back from the mailing list?

That's because you're not listed as the maintainer for PM domain stuff,
and I forgot to ensure lakml was copied on all patches.

> >  Documentation/devicetree/bindings/soc/dove/pmu.txt |  49 +++
> >  arch/arm/boot/dts/dove.dtsi                        |  25 ++
> >  arch/arm/mach-mvebu/Kconfig                        |   1 +
> >  drivers/base/platform.c                            |   2 +
> >  drivers/base/power/common.c                        |  15 +
> >  drivers/base/power/domain.c                        |  33 +-
> >  drivers/soc/Makefile                               |   1 +
> >  drivers/soc/dove/Makefile                          |   1 +
> >  drivers/soc/dove/pmu.c                             | 399 +++++++++++++++++++++
> >  include/linux/pm.h                                 |   1 +
> >  include/linux/pm_domain.h                          |   4 +
> >  include/linux/soc/dove/pmu.h                       |   6 +
> >  12 files changed, 532 insertions(+), 5 deletions(-)
> 
> I see add the header file and the global dove_init_pmu() function,
> but I don't see a caller of that function. Is that intentional, or
> did you accidentally leave out another patch that you meant to include?

I kind'a did - it needs an explicit call from arch/arm/mach-mvebu/dove.c
which I haven't added even in my tree (because I don't use that path,
even when I test DT booting - I still use most of the arch/arm/mach-dove
code when DT booting.)  I'll add that now.

Of course, I also have a patch which adds legacy support to
arch/arm/mach-dove, but I've assumed you're not interested in that...

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [FOR DISCUSSION 0/9] Dove PMU support

[Arnd Bergmann]

On Friday 13 March 2015 12:11:27 Russell King - ARM Linux wrote:
> On Fri, Mar 13, 2015 at 12:57:11PM +0100, Arnd Bergmann wrote:
> > On Thursday 12 March 2015 18:30:21 Russell King - ARM Linux wrote:
> > >  Documentation/devicetree/bindings/soc/dove/pmu.txt |  49 +++
> > >  arch/arm/boot/dts/dove.dtsi                        |  25 ++
> > >  arch/arm/mach-mvebu/Kconfig                        |   1 +
> > >  drivers/base/platform.c                            |   2 +
> > >  drivers/base/power/common.c                        |  15 +
> > >  drivers/base/power/domain.c                        |  33 +-
> > >  drivers/soc/Makefile                               |   1 +
> > >  drivers/soc/dove/Makefile                          |   1 +
> > >  drivers/soc/dove/pmu.c                             | 399 +++++++++++++++++++++
> > >  include/linux/pm.h                                 |   1 +
> > >  include/linux/pm_domain.h                          |   4 +
> > >  include/linux/soc/dove/pmu.h                       |   6 +
> > >  12 files changed, 532 insertions(+), 5 deletions(-)
> > 
> > I see add the header file and the global dove_init_pmu() function,
> > but I don't see a caller of that function. Is that intentional, or
> > did you accidentally leave out another patch that you meant to include?
> 
> I kind'a did - it needs an explicit call from arch/arm/mach-mvebu/dove.c
> which I haven't added even in my tree (because I don't use that path,
> even when I test DT booting - I still use most of the arch/arm/mach-dove
> code when DT booting.)  I'll add that now.
> 
> Of course, I also have a patch which adds legacy support to
> arch/arm/mach-dove, but I've assumed you're not interested in that...

You mean legacy support in mach-mvebu?

I don't mind that at all, it was the mvebu maintainers that decided
it would be best to combine the multiplatform and DT work in order
to simplify both. I've also sent a patch set that moves mach-dove
into multiplatform, which is a different way of doing the same thing.

	Arnd

Re: [FOR DISCUSSION 0/9] Dove PMU support

[Russell King - ARM Linux]

On Fri, Mar 13, 2015 at 01:26:17PM +0100, Arnd Bergmann wrote:
> On Friday 13 March 2015 12:11:27 Russell King - ARM Linux wrote:
> > On Fri, Mar 13, 2015 at 12:57:11PM +0100, Arnd Bergmann wrote:
> > > On Thursday 12 March 2015 18:30:21 Russell King - ARM Linux wrote:
> > > >  Documentation/devicetree/bindings/soc/dove/pmu.txt |  49 +++
> > > >  arch/arm/boot/dts/dove.dtsi                        |  25 ++
> > > >  arch/arm/mach-mvebu/Kconfig                        |   1 +
> > > >  drivers/base/platform.c                            |   2 +
> > > >  drivers/base/power/common.c                        |  15 +
> > > >  drivers/base/power/domain.c                        |  33 +-
> > > >  drivers/soc/Makefile                               |   1 +
> > > >  drivers/soc/dove/Makefile                          |   1 +
> > > >  drivers/soc/dove/pmu.c                             | 399 +++++++++++++++++++++
> > > >  include/linux/pm.h                                 |   1 +
> > > >  include/linux/pm_domain.h                          |   4 +
> > > >  include/linux/soc/dove/pmu.h                       |   6 +
> > > >  12 files changed, 532 insertions(+), 5 deletions(-)
> > > 
> > > I see add the header file and the global dove_init_pmu() function,
> > > but I don't see a caller of that function. Is that intentional, or
> > > did you accidentally leave out another patch that you meant to include?
> > 
> > I kind'a did - it needs an explicit call from arch/arm/mach-mvebu/dove.c
> > which I haven't added even in my tree (because I don't use that path,
> > even when I test DT booting - I still use most of the arch/arm/mach-dove
> > code when DT booting.)  I'll add that now.
> > 
> > Of course, I also have a patch which adds legacy support to
> > arch/arm/mach-dove, but I've assumed you're not interested in that...
> 
> You mean legacy support in mach-mvebu?

No.

As you'll see, this uses a platform device notifier to hook the devices
onto the appropriate PM domain, which is why the driver needs to be
registered early.  This also gets used in DT mode with one legacy
platform device which is dynamically registered (something which can't
happen with DT.)

 arch/arm/Kconfig                     |  1 +
 arch/arm/mach-dove/common.c          | 25 ++++++++++
 arch/arm/mach-dove/include/mach/pm.h | 17 -------
 arch/arm/mach-dove/irq.c             | 87 --------------------------------
 drivers/soc/Makefile                 |  1 +
 drivers/soc/dove/pmu.c               | 97 ++++++++++++++++++++++++++++++++++++
 include/linux/soc/dove/pmu.h         | 18 +++++++
 7 files changed, 142 insertions(+), 104 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 97d07ed60a0b..08e7608d1c52 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -519,6 +519,7 @@ config ARCH_DOVE
 	select PINCTRL
 	select PINCTRL_DOVE
 	select PLAT_ORION_LEGACY
+	select PM_GENERIC_DOMAINS if PM
 	help
 	  Support for the Marvell Dove SoC 88AP510
 
diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index 0d1a89298ece..6f3887217674 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -16,6 +16,7 @@
 #include <linux/platform_data/dma-mv_xor.h>
 #include <linux/platform_data/usb-ehci-orion.h>
 #include <linux/platform_device.h>
+#include <linux/soc/dove/pmu.h>
 #include <asm/hardware/cache-tauros2.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
@@ -375,6 +376,29 @@ void __init dove_setup_cpu_wins(void)
 				    DOVE_SCRATCHPAD_SIZE);
 }
 
+static const struct dove_pmu_domain_initdata pmu_domains[] __initconst = {
+	{
+		.pwr_mask = PMU_PWR_VPU_PWR_DWN_MASK,
+		.rst_mask = PMU_SW_RST_VIDEO_MASK,
+		.iso_mask = PMU_ISO_VIDEO_MASK,
+		.name = "vpu-domain",
+	}, {
+		.pwr_mask = PMU_PWR_GPU_PWR_DWN_MASK,
+		.rst_mask = PMU_SW_RST_GPU_MASK,
+		.iso_mask = PMU_ISO_GPU_MASK,
+		.name = "gpu-domain",
+	}, {
+		/* sentinel */
+	},
+};
+
+static const struct dove_pmu_initdata pmu_data __initconst = {
+	.pmc_base = DOVE_PMU_VIRT_BASE,
+	.pmu_base = DOVE_PMU_VIRT_BASE + 0x8000,
+	.irq = IRQ_DOVE_PMU,
+	.domains = pmu_domains,
+};
+
 void __init dove_init(void)
 {
 	pr_info("Dove 88AP510 SoC, TCLK = %d MHz.\n",
@@ -389,6 +413,7 @@ void __init dove_init(void)
 	dove_clk_init();
 
 	/* internal devices that every board has */
+	dove_init_pmu_legacy(&pmu_data);
 	dove_rtc_init();
 	dove_xor0_init();
 	dove_xor1_init();
diff --git a/arch/arm/mach-dove/include/mach/pm.h b/arch/arm/mach-dove/include/mach/pm.h
index b47f75038686..625a89c15c1f 100644
--- a/arch/arm/mach-dove/include/mach/pm.h
+++ b/arch/arm/mach-dove/include/mach/pm.h
@@ -51,22 +51,5 @@
 #define  CLOCK_GATING_GIGA_PHY_MASK	(1 << CLOCK_GATING_BIT_GIGA_PHY)
 
 #define PMU_INTERRUPT_CAUSE	(DOVE_PMU_VIRT_BASE + 0x50)
-#define PMU_INTERRUPT_MASK	(DOVE_PMU_VIRT_BASE + 0x54)
-
-static inline int pmu_to_irq(int pin)
-{
-	if (pin < NR_PMU_IRQS)
-		return pin + IRQ_DOVE_PMU_START;
-
-	return -EINVAL;
-}
-
-static inline int irq_to_pmu(int irq)
-{
-	if (IRQ_DOVE_PMU_START <= irq && irq < NR_IRQS)
-		return irq - IRQ_DOVE_PMU_START;
-
-	return -EINVAL;
-}
 
 #endif
diff --git a/arch/arm/mach-dove/irq.c b/arch/arm/mach-dove/irq.c
index 4a5a7aedcb76..924d8afe4597 100644
--- a/arch/arm/mach-dove/irq.c
+++ b/arch/arm/mach-dove/irq.c
@@ -7,86 +7,14 @@
  * License version 2.  This program is licensed "as is" without any
  * warranty of any kind, whether express or implied.
  */
-
-#include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/irq.h>
-#include <linux/gpio.h>
 #include <linux/io.h>
-#include <asm/mach/arch.h>
 #include <plat/irq.h>
-#include <asm/mach/irq.h>
-#include <mach/pm.h>
 #include <mach/bridge-regs.h>
 #include <plat/orion-gpio.h>
 #include "common.h"
 
-static void pmu_irq_mask(struct irq_data *d)
-{
-	int pin = irq_to_pmu(d->irq);
-	u32 u;
-
-	u = readl(PMU_INTERRUPT_MASK);
-	u &= ~(1 << (pin & 31));
-	writel(u, PMU_INTERRUPT_MASK);
-}
-
-static void pmu_irq_unmask(struct irq_data *d)
-{
-	int pin = irq_to_pmu(d->irq);
-	u32 u;
-
-	u = readl(PMU_INTERRUPT_MASK);
-	u |= 1 << (pin & 31);
-	writel(u, PMU_INTERRUPT_MASK);
-}
-
-static void pmu_irq_ack(struct irq_data *d)
-{
-	int pin = irq_to_pmu(d->irq);
-	u32 u;
-
-	/*
-	 * The PMU mask register is not RW0C: it is RW.  This means that
-	 * the bits take whatever value is written to them; if you write
-	 * a '1', you will set the interrupt.
-	 *
-	 * Unfortunately this means there is NO race free way to clear
-	 * these interrupts.
-	 *
-	 * So, let's structure the code so that the window is as small as
-	 * possible.
-	 */
-	u = ~(1 << (pin & 31));
-	u &= readl_relaxed(PMU_INTERRUPT_CAUSE);
-	writel_relaxed(u, PMU_INTERRUPT_CAUSE);
-}
-
-static struct irq_chip pmu_irq_chip = {
-	.name		= "pmu_irq",
-	.irq_mask	= pmu_irq_mask,
-	.irq_unmask	= pmu_irq_unmask,
-	.irq_ack	= pmu_irq_ack,
-};
-
-static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc)
-{
-	unsigned long cause = readl(PMU_INTERRUPT_CAUSE);
-
-	cause &= readl(PMU_INTERRUPT_MASK);
-	if (cause == 0) {
-		do_bad_IRQ(irq, desc);
-		return;
-	}
-
-	for (irq = 0; irq < NR_PMU_IRQS; irq++) {
-		if (!(cause & (1 << irq)))
-			continue;
-		irq = pmu_to_irq(irq);
-		generic_handle_irq(irq);
-	}
-}
-
 static int __initdata gpio0_irqs[4] = {
 	IRQ_DOVE_GPIO_0_7,
 	IRQ_DOVE_GPIO_8_15,
@@ -142,8 +70,6 @@ __exception_irq_entry dove_legacy_handle_irq(struct pt_regs *regs)
 
 void __init dove_init_irq(void)
 {
-	int i;
-
 	orion_irq_init(0, IRQ_VIRT_BASE + IRQ_MASK_LOW_OFF);
 	orion_irq_init(32, IRQ_VIRT_BASE + IRQ_MASK_HIGH_OFF);
 
@@ -162,17 +88,4 @@ void __init dove_init_irq(void)
 
 	orion_gpio_init(NULL, 64, 8, DOVE_GPIO2_VIRT_BASE, 0,
 			IRQ_DOVE_GPIO_START + 64, gpio2_irqs);
-
-	/*
-	 * Mask and clear PMU interrupts
-	 */
-	writel(0, PMU_INTERRUPT_MASK);
-	writel(0, PMU_INTERRUPT_CAUSE);
-
-	for (i = IRQ_DOVE_PMU_START; i < NR_IRQS; i++) {
-		irq_set_chip_and_handler(i, &pmu_irq_chip, handle_level_irq);
-		irq_set_status_flags(i, IRQ_LEVEL);
-		set_irq_flags(i, IRQF_VALID);
-	}
-	irq_set_chained_handler(IRQ_DOVE_PMU, pmu_irq_handler);
 }
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 7382bfe92743..0e2b360c57d2 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -2,6 +2,7 @@
 # Makefile for the Linux Kernel SOC specific device drivers.
 #
 
+obj-$(CONFIG_ARCH_DOVE)		+= dove/
 obj-$(CONFIG_MACH_DOVE)		+= dove/
 obj-$(CONFIG_ARCH_QCOM)		+= qcom/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
diff --git a/drivers/soc/dove/pmu.c b/drivers/soc/dove/pmu.c
index 41539743ecf9..3f6a43fce8d3 100644
--- a/drivers/soc/dove/pmu.c
+++ b/drivers/soc/dove/pmu.c
@@ -305,6 +305,103 @@ static int __init dove_init_pmu_irq(struct pmu_data *pmu, int irq)
 	return 0;
 }
 
+static void pmu_add_genpd_name(const char *name, struct device *dev)
+{
+	while (1) {
+		if (pm_genpd_name_add_device(name, dev) != -EAGAIN)
+			break;
+		cond_resched();
+	}
+}
+
+static void pmu_remove_genpd(struct device *dev)
+{
+	struct generic_pm_domain *genpd = dev_to_genpd(dev);
+
+	while (1) {
+		if (pm_genpd_remove_device(genpd, dev) != -EAGAIN)
+			break;
+		cond_resched();
+	}
+}
+
+static int pmu_platform_call(struct notifier_block *nb,
+	unsigned long event, void *data)
+{
+	struct device *dev = data;
+	const char *name = NULL;
+
+	if (dev->of_node)
+		return NOTIFY_OK;
+
+	if (strcmp(dev_name(dev), "ap510-vmeta.0") == 0 ||
+	    strcmp(dev_name(dev), "ap510-vmeta") == 0)
+		name = "vpu-domain";
+	else if (strcmp(dev_name(dev), "galcore.0") == 0)
+		name = "gpu-domain";
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		if (name)
+			pmu_add_genpd_name(name, dev);
+		break;
+
+	case BUS_NOTIFY_DEL_DEVICE:
+		pmu_remove_genpd(dev);
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block platform_nb = {
+	.notifier_call = pmu_platform_call,
+};
+
+int __init dove_init_pmu_legacy(const struct dove_pmu_initdata *initdata)
+{
+	const struct dove_pmu_domain_initdata *domain_initdata;
+	struct pmu_data *pmu;
+	int ret;
+
+	pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
+	if (!pmu)
+		return -ENOMEM;
+
+	spin_lock_init(&pmu->lock);
+	pmu->pmc_base = initdata->pmc_base;
+	pmu->pmu_base = initdata->pmu_base;
+
+	pmu_reset_init(pmu);
+	for (domain_initdata = initdata->domains; domain_initdata->name;
+	     domain_initdata++) {
+		struct pmu_domain *domain;
+
+		domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+		if (domain) {
+			domain->pmu = pmu;
+			domain->pwr_mask = domain_initdata->pwr_mask;
+			domain->rst_mask = domain_initdata->rst_mask;
+			domain->iso_mask = domain_initdata->iso_mask;
+			domain->base.name = domain_initdata->name;
+
+			__pmu_domain_register(domain, NULL);
+		}
+	}
+	pm_genpd_poweroff_unused();
+
+	ret = dove_init_pmu_irq(pmu, initdata->irq);
+	if (ret)
+		pr_err("dove_init_pmu_irq() failed: %d\n", ret);
+
+	if (pmu->irq_domain)
+		irq_domain_associate_many(pmu->irq_domain, IRQ_DOVE_PMU_START,
+					  0, NR_PMU_IRQS);
+
+	bus_register_notifier(&platform_bus_type, &platform_nb);
+
+	return 0;
+}
+
 /*
  * pmu: power-manager@d0000 {
  *	compatible = "marvell,dove-pmu";
diff --git a/include/linux/soc/dove/pmu.h b/include/linux/soc/dove/pmu.h
index 9c99f84bcc0e..431dfac595e7 100644
--- a/include/linux/soc/dove/pmu.h
+++ b/include/linux/soc/dove/pmu.h
@@ -1,6 +1,24 @@
 #ifndef LINUX_SOC_DOVE_PMU_H
 #define LINUX_SOC_DOVE_PMU_H
 
+#include <linux/types.h>
+
+struct dove_pmu_domain_initdata {
+	u32 pwr_mask;
+	u32 rst_mask;
+	u32 iso_mask;
+	const char *name;
+};
+
+struct dove_pmu_initdata {
+	void __iomem *pmc_base;
+	void __iomem *pmu_base;
+	int irq;
+	const struct dove_pmu_domain_initdata *domains;
+};
+
+int dove_init_pmu_legacy(const struct dove_pmu_initdata *);
+
 int dove_init_pmu(void);
 
 #endif


-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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: [FOR DISCUSSION 0/9] Dove PMU support

[Arnd Bergmann]

On Friday 13 March 2015 12:32:11 Russell King - ARM Linux wrote:
> On Fri, Mar 13, 2015 at 01:26:17PM +0100, Arnd Bergmann wrote:
> > On Friday 13 March 2015 12:11:27 Russell King - ARM Linux wrote:
> > > Of course, I also have a patch which adds legacy support to
> > > arch/arm/mach-dove, but I've assumed you're not interested in that...
> > 
> > You mean legacy support in mach-mvebu?
> 
> No.
> 
> As you'll see, this uses a platform device notifier to hook the devices
> onto the appropriate PM domain, which is why the driver needs to be
> registered early.  This also gets used in DT mode with one legacy
> platform device which is dynamically registered (something which can't
> happen with DT.)

Ok, got it. I think that's fine too. If this helps you spend less
time on forward-porting your patches and in turn completing the
full dove support with DT faster, I'm all for merging this one too.

	Arnd

[PATCH 04/10] pm: domains: sync runtime PM status with PM domains after probe

[Russell King]

Synchronise the PM domain status with runtime PM's status after a
platform device has been probed.  This augments the solution in commit
2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach
completes").

The above commit added a call to power up the PM domain when a device
attaches to the domain in order to match the behaviour required by
drivers that make no use of runtime PM.  The assumption is that the
device driver will cause a runtime PM transition, which will synchronise
the PM domain state with the runtime PM state.

However, by default, runtime PM will assume that the device is initially
suspended, and some drivers may make use of this should they not need to
touch the hardware during probe.

In order to allow such drivers, trigger the PM domain code to check
whether the PM domain can be suspended after the probe function, undoing
the effect of the power-on prior to the probe.

Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
---
 drivers/amba/bus.c          |  4 +++-
 drivers/base/platform.c     |  2 ++
 drivers/base/power/common.c | 15 +++++++++++++++
 drivers/base/power/domain.c | 12 ++++++++++++
 drivers/i2c/i2c-core.c      |  2 ++
 drivers/spi/spi.c           |  2 ++
 include/linux/pm.h          |  1 +
 include/linux/pm_domain.h   |  4 ++++
 8 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 52ddd9fbb55e..8d4097f982d1 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -205,8 +205,10 @@ static int amba_probe(struct device *dev)
 		pm_runtime_enable(dev);
 
 		ret = pcdrv->probe(pcdev, id);
-		if (ret == 0)
+		if (ret == 0) {
+			dev_pm_domain_sync(dev);
 			break;
+		}
 
 		pm_runtime_disable(dev);
 		pm_runtime_set_suspended(dev);
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 9421fed40905..552d1affc060 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev)
 		ret = drv->probe(dev);
 		if (ret)
 			dev_pm_domain_detach(_dev, true);
+		else
+			dev_pm_domain_sync(_dev);
 	}
 
 	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index b0f138806bbc..8c739a14d3c7 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
 		dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_sync - synchronise the PM domain state with its devices
+ * @dev: device corresponding with domain
+ *
+ * Synchronise the PM domain state with the recently probed device, which
+ * may be in a variety of PM states.  This ensures that a device which
+ * enables runtime PM in suspended state, and never transitions to active
+ * in its probe handler is properly suspended after the probe.
+ */
+void dev_pm_domain_sync(struct device *dev)
+{
+	if (dev->pm_domain && dev->pm_domain->sync)
+		dev->pm_domain->sync(dev);
+}
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 69fa87aa3b52..2b552a2d3544 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2169,6 +2169,17 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
 	genpd_queue_power_off_work(pd);
 }
 
+static void genpd_dev_pm_sync(struct device *dev)
+{
+	struct generic_pm_domain *pd;
+
+	pd = pm_genpd_lookup_dev(dev);
+	if (!pd)
+		return;
+
+	genpd_queue_power_off_work(pd);
+}
+
 /**
  * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
  * @dev: Device to attach.
@@ -2235,6 +2246,7 @@ int genpd_dev_pm_attach(struct device *dev)
 	}
 
 	dev->pm_domain->detach = genpd_dev_pm_detach;
+	dev->pm_domain->sync = genpd_dev_pm_sync;
 	pm_genpd_poweron(pd);
 
 	return 0;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e9eae57a2b50..a6d628542907 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -659,6 +659,8 @@ static int i2c_device_probe(struct device *dev)
 					client));
 		if (status)
 			dev_pm_domain_detach(&client->dev, true);
+		else
+			dev_pm_domain_sync(&client->dev);
 	}
 
 	return status;
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 66a70e9bc743..9f697cb51097 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -270,6 +270,8 @@ static int spi_drv_probe(struct device *dev)
 		ret = sdrv->probe(to_spi_device(dev));
 		if (ret)
 			dev_pm_domain_detach(dev, true);
+		else
+			dev_pm_domain_sync(dev);
 	}
 
 	return ret;
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 8b5976364619..676ca4055239 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev);
 struct dev_pm_domain {
 	struct dev_pm_ops	ops;
 	void (*detach)(struct device *dev, bool power_off);
+	void (*sync)(struct device *dev);
 };
 
 /*
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index a9edab2c787a..8d58b30e23ac 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,
 #ifdef CONFIG_PM
 extern int dev_pm_domain_attach(struct device *dev, bool power_on);
 extern void dev_pm_domain_detach(struct device *dev, bool power_off);
+void dev_pm_domain_sync(struct device *dev);
 #else
 static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
 {
 	return -ENODEV;
 }
 static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
+static inline void dev_pm_domain_sync(struct device *dev)
+{
+}
 #endif
 
 #endif /* _LINUX_PM_DOMAIN_H */
-- 
1.8.3.1

Re: [PATCH 04/10] pm: domains: sync runtime PM status with PM domains after probe

[Kevin Hilman]

Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> writes:

> Synchronise the PM domain status with runtime PM's status after a
> platform device has been probed.  This augments the solution in commit
> 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach
> completes").
>
> The above commit added a call to power up the PM domain when a device
> attaches to the domain in order to match the behaviour required by
> drivers that make no use of runtime PM.  The assumption is that the
> device driver will cause a runtime PM transition, which will synchronise
> the PM domain state with the runtime PM state.
>
> However, by default, runtime PM will assume that the device is initially
> suspended, and some drivers may make use of this should they not need to
> touch the hardware during probe.
>
> In order to allow such drivers, trigger the PM domain code to check
> whether the PM domain can be suspended after the probe function, undoing
> the effect of the power-on prior to the probe.
>
> Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>

[ Since I hadn't seen this version yet, repeating comment from the
  previous version so it doesn't get lost. ]

I think this is a good fix to the existing problem.  One minor nit on a
comment below, otherwise:

Acked-by: Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> +/**
> + * dev_pm_domain_sync - synchronise the PM domain state with its devices
> + * @dev: device corresponding with domain
> + *
> + * Synchronise the PM domain state with the recently probed device, which
> + * may be in a variety of PM states.  This ensures that a device which
> + * enables runtime PM in suspended state, and never transitions to active
> + * in its probe handler is properly suspended after the probe.
> + */

It's not the *device* tha needs to be properly suspended after the probe
(since it's already/still runtime suspended), but the pm_domain that
would be potentially powered down.  Hence, I'd reword the last sentence
slightly:

   This ensures that a device which enables runtime PM in suspended
   state, and never transitions to active in its probe handler gives an
   opportunity for the PM domain to be powered down after the probe.

Kevin

[FOR DISCUSSION 0/10] Dove PMU support

[Russell King - ARM Linux]

This is the third re-posting of the patch set which I posted almost 11
months ago to support the Dove PMU, with a few additional changes.
This set is based upon 3.19.

In this set are:

* one patch which Rafael originally acked, but there was indecision
  last time around how to handle them due to potential conflicts with
  work that Ulf was doing.  This patches have been updated to apply
  cleanly to 3.19.  This patch should be applied anyway.

* factor out code which gets a validated generic PM domain, which we
  will make use of in later patches. (new)

* improve the validation of the generic PM domain pointer passed into
  pm_genpd_remove_device(). (updated)

* synchronise the state of the generic PM domain after a device is
  probed.  Other solutions may be possible, but require a larger
  patch series to resolve. (updated for patch 2)

* DT binding documentation for the Dove PMU driver, updated with
  comments from Rob.

* the addition of the core Dove PMU driver, which consists of a reset,
  IRQ controller, and power domains.  The reset and power domain code
  has to be closely related due to the power up/down requirements of
  the GPU/VPU subsystems needing to be performed atomically.  (This
  requirement prevents it using the MFD/syscon infrastructure, because
  we would need to hold spinlocks while calling several different
  sub-drivers.)  This currently needs to be available early on in the
  init sequence, so an explicit initialisation call is added to
  mach-mvebu to achieve this. (updated)

* addition of the RTC interrupt, so we can now receive and act on
  alarms generated by the Dove RTC.

* addition of the DT descriptions for the GPU and VPU power domains.
  These patches do not themselves add the DT descriptions for these
  units, so these patches serve as illustrations how these should be
  described.

 Documentation/devicetree/bindings/soc/dove/pmu.txt |  49 +++
 arch/arm/boot/dts/dove.dtsi                        |  25 ++
 arch/arm/mach-mvebu/Kconfig                        |   1 +
 arch/arm/mach-mvebu/dove.c                         |   2 +
 drivers/amba/bus.c                                 |   4 +-
 drivers/base/platform.c                            |   2 +
 drivers/base/power/common.c                        |  15 +
 drivers/base/power/domain.c                        |  64 +++-
 drivers/i2c/i2c-core.c                             |   2 +
 drivers/soc/Makefile                               |   1 +
 drivers/soc/dove/Makefile                          |   1 +
 drivers/soc/dove/pmu.c                             | 406 +++++++++++++++++++++
 drivers/spi/spi.c                                  |   2 +
 include/linux/pm.h                                 |   1 +
 include/linux/pm_domain.h                          |   4 +
 include/linux/soc/dove/pmu.h                       |   6 +
 16 files changed, 564 insertions(+), 21 deletions(-)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 01/10] pm: domains: quieten down generic pm domains

[Russell King]

PM domains are rather noisy; scheduling behaviour can cause callbacks
to take longer, which causes them to spit out a warning-level message
each time a callback takes a little longer than the previous time.
There really isn't a need for this, except when debugging.

Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0d8780c04a5e..b3fbc21da2dc 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -173,8 +173,8 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
 	genpd->power_on_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
 	genpd_recalc_cpu_exit_latency(genpd);
-	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
-		genpd->name, "on", elapsed_ns);
+	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
+		 genpd->name, "on", elapsed_ns);
 
 	return ret;
 }
@@ -199,8 +199,8 @@ static int genpd_power_off(struct generic_pm_domain *genpd)
 
 	genpd->power_off_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
-	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
-		genpd->name, "off", elapsed_ns);
+	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
+		 genpd->name, "off", elapsed_ns);
 
 	return ret;
 }
-- 
1.8.3.1

Re: [PATCH 01/10] pm: domains: quieten down generic pm domains

[Kevin Hilman]

Russell King <rmk+kernel@arm.linux.org.uk> writes:

> PM domains are rather noisy; scheduling behaviour can cause callbacks
> to take longer, which causes them to spit out a warning-level message
> each time a callback takes a little longer than the previous time.
> There really isn't a need for this, except when debugging.
>
> Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Acked-by: Kevin Hilman <khilman@linaro.org>

[PATCH 02/10] pm: domains: factor out code to get the generic PM domain from a struct device

[Russell King]

The PM domain code contains two methods to get the generic PM domain
for a struct device.  One is dev_to_genpd() which is only safe when
we know for certain that the device has a generic PM domain attached.
The other is coded into genpd_dev_pm_detach() which ensures that the
PM domain in the struct device is a generic PM domain (and so is safer).

This commit factors out the safer version and documents it.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b3fbc21da2dc..b9000b245e62 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -68,6 +68,31 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 	return genpd;
 }
 
+/*
+ * Get the generic PM domain for a particular struct device.
+ * This validates the struct device pointer, the PM domain pointer,
+ * and checks that the PM domain pointer is a real generic PM domain.
+ * Any failure results in NULL being returned.
+ */
+static struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
+{
+	struct generic_pm_domain *genpd = NULL, *gpd;
+
+	if (IS_ERR_OR_NULL(dev) || IS_ERR_OR_NULL(dev->pm_domain))
+		return NULL;
+
+	mutex_lock(&gpd_list_lock);
+	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+		if (&gpd->domain == dev->pm_domain) {
+			genpd = gpd;
+			break;
+		}
+	}
+	mutex_unlock(&gpd_list_lock);
+
+	return genpd;
+}
+
 struct generic_pm_domain *dev_to_genpd(struct device *dev)
 {
 	if (IS_ERR_OR_NULL(dev->pm_domain))
@@ -2120,21 +2145,10 @@ EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);
  */
 static void genpd_dev_pm_detach(struct device *dev, bool power_off)
 {
-	struct generic_pm_domain *pd = NULL, *gpd;
+	struct generic_pm_domain *pd;
 	int ret = 0;
 
-	if (!dev->pm_domain)
-		return;
-
-	mutex_lock(&gpd_list_lock);
-	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
-		if (&gpd->domain == dev->pm_domain) {
-			pd = gpd;
-			break;
-		}
-	}
-	mutex_unlock(&gpd_list_lock);
-
+	pd = pm_genpd_lookup_dev(dev);
 	if (!pd)
 		return;
 
-- 
1.8.3.1

Re: [PATCH 02/10] pm: domains: factor out code to get the generic PM domain from a struct device

[Kevin Hilman]

Russell King <rmk+kernel@arm.linux.org.uk> writes:

> The PM domain code contains two methods to get the generic PM domain
> for a struct device.  One is dev_to_genpd() which is only safe when
> we know for certain that the device has a generic PM domain attached.
> The other is coded into genpd_dev_pm_detach() which ensures that the
> PM domain in the struct device is a generic PM domain (and so is safer).
>
> This commit factors out the safer version and documents it.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Acked-by: Kevin Hilman <khilman@linaro.org>

On top of this, I assume you're thinking that the externally available
dev_to_genpd() should probably be replaced by pm_genpd_lookup_dev(), and
dev_to_genpd() should only be used internally to genpd?

Kevin

Re: [PATCH 02/10] pm: domains: factor out code to get the generic PM domain from a struct device

[Russell King - ARM Linux]

On Fri, Mar 13, 2015 at 10:20:21AM -0700, Kevin Hilman wrote:
> Russell King <rmk+kernel@arm.linux.org.uk> writes:
> 
> > The PM domain code contains two methods to get the generic PM domain
> > for a struct device.  One is dev_to_genpd() which is only safe when
> > we know for certain that the device has a generic PM domain attached.
> > The other is coded into genpd_dev_pm_detach() which ensures that the
> > PM domain in the struct device is a generic PM domain (and so is safer).
> >
> > This commit factors out the safer version and documents it.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> Acked-by: Kevin Hilman <khilman@linaro.org>
> 
> On top of this, I assume you're thinking that the externally available
> dev_to_genpd() should probably be replaced by pm_genpd_lookup_dev(), and
> dev_to_genpd() should only be used internally to genpd?

I think that would be a sensible improvement over leaving the unsafe
dev_to_genpd() exposed.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 03/10] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Russell King]

pm_genpd_remove_device() tries hard to validate the generic PM domain
passed to it, but the validation is not complete.

dev->pm_domain contains a struct dev_pm_domain, which is the "base
class" of generic PM domains.  Other users of dev_pm_domains include
stuff like vga_switheroo.  Hence, a device could have a generic PM
domain or a vga_switcheroo PM domain in dev->pm_domain.

We need ot be certain that the PM domain is actually valid before we
try to remove it.  We can do this easily as we have a way to get the
current validated generic PM domain for a struct device.  This must
match the generic PM domain being requested for removal.

Convert the code to use this alternative validation method instead.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b9000b245e62..69fa87aa3b52 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1534,9 +1534,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	dev_dbg(dev, "%s()\n", __func__);
 
-	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)
-	    ||  IS_ERR_OR_NULL(dev->pm_domain)
-	    ||  pd_to_genpd(dev->pm_domain) != genpd)
+	if (!genpd || genpd != pm_genpd_lookup_dev(dev))
 		return -EINVAL;
 
 	genpd_acquire_lock(genpd);
-- 
1.8.3.1

Re: [PATCH 03/10] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Kevin Hilman]

Russell King <rmk+kernel@arm.linux.org.uk> writes:

> pm_genpd_remove_device() tries hard to validate the generic PM domain
> passed to it, but the validation is not complete.
>
> dev->pm_domain contains a struct dev_pm_domain, which is the "base
> class" of generic PM domains.  Other users of dev_pm_domains include
> stuff like vga_switheroo.  Hence, a device could have a generic PM
> domain or a vga_switcheroo PM domain in dev->pm_domain.
>
> We need ot be certain that the PM domain is actually valid before we
> try to remove it.  

nit: We need to be certain that the PM domain is a valid genpd before we
try to remove the device.

> We can do this easily as we have a way to get the
> current validated generic PM domain for a struct device.  This must
> match the generic PM domain being requested for removal.
>
> Convert the code to use this alternative validation method instead.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Otherwise,

Acked-by: Kevin Hilman <khilman@linaro.org>

Re: [FOR DISCUSSION 0/9] Dove PMU support

[Rafael J. Wysocki]

On Thursday, March 12, 2015 06:30:21 PM Russell King - ARM Linux wrote:
> This is a re-posting of the patch set which I posted almost 10 months
> ago to support the Dove PMU, with a few additional changes.  This set
> is based upon 3.19.
> 
> In this set are:
> 
> * two patches which Rafael originally acked, but there was indecision
>   last time around how to handle them due to potential conflicts with
>   work that Ulf was doing.  These patches have been updated to apply
>   cleanly to 3.19.  I don't know if people want to take these as
>   fixes to the PM domain code or not (hence why I'm posting this
>   series during the merge window - if it weren't for this, I'd hold
>   it off.)
> 
> * what I regard as a fix to the PM domain code; as a result of a
>   previous commit, the PM domain code mismatches the runtime PM state,
>   which leads to the PM domain being unexpectedly left on.  This patch
>   has been re-worked to try an alternative approach, syncing the PM
>   domain state with the runtime PM state after the probe has completed.
> 
> * the addition of the core Dove PMU driver, which consists of a reset,
>   IRQ controller, and power domains.  The reset and power domain code
>   has to be closely related due to the power up/down requirements of
>   the GPU/VPU subsystems needing to be performed atomically.  (This
>   requirement prevents it using the MFD infrastructure, because we
>   would need to hold spinlocks while calling several different
>   sub-drivers.)
> 
> * addition of the RTC interrupt, so we can now receive and act on
>   alarms generated by the Dove RTC.
> 
> * addition of the DT descriptions for the GPU and VPU power domains.
>   These patches do not themselves add the DT descriptions for these
>   units, so these patches serve as illustrations how these should be
>   described.
> 

I can apply patches [1-3/9] from this series if that helps.

Rafael

Re: [FOR DISCUSSION 0/9] Dove PMU support

[Rafael J. Wysocki]

On Thursday, March 19, 2015 10:59:20 PM Rafael J. Wysocki wrote:
> On Thursday, March 12, 2015 06:30:21 PM Russell King - ARM Linux wrote:
> > This is a re-posting of the patch set which I posted almost 10 months
> > ago to support the Dove PMU, with a few additional changes.  This set
> > is based upon 3.19.
> > 
> > In this set are:
> > 
> > * two patches which Rafael originally acked, but there was indecision
> >   last time around how to handle them due to potential conflicts with
> >   work that Ulf was doing.  These patches have been updated to apply
> >   cleanly to 3.19.  I don't know if people want to take these as
> >   fixes to the PM domain code or not (hence why I'm posting this
> >   series during the merge window - if it weren't for this, I'd hold
> >   it off.)
> > 
> > * what I regard as a fix to the PM domain code; as a result of a
> >   previous commit, the PM domain code mismatches the runtime PM state,
> >   which leads to the PM domain being unexpectedly left on.  This patch
> >   has been re-worked to try an alternative approach, syncing the PM
> >   domain state with the runtime PM state after the probe has completed.
> > 
> > * the addition of the core Dove PMU driver, which consists of a reset,
> >   IRQ controller, and power domains.  The reset and power domain code
> >   has to be closely related due to the power up/down requirements of
> >   the GPU/VPU subsystems needing to be performed atomically.  (This
> >   requirement prevents it using the MFD infrastructure, because we
> >   would need to hold spinlocks while calling several different
> >   sub-drivers.)
> > 
> > * addition of the RTC interrupt, so we can now receive and act on
> >   alarms generated by the Dove RTC.
> > 
> > * addition of the DT descriptions for the GPU and VPU power domains.
> >   These patches do not themselves add the DT descriptions for these
> >   units, so these patches serve as illustrations how these should be
> >   described.
> > 
> 
> I can apply patches [1-3/9] from this series if that helps.

I mean from the next version of it ([FOR DISCUSSION 0/10] Dove PMU support).

These patches to be precise:

https://patchwork.kernel.org/patch/6006991/
https://patchwork.kernel.org/patch/6007001/
https://patchwork.kernel.org/patch/6007021/

unless there are objectsions (which I haven't seen).

Rafael

Re: [FOR DISCUSSION 0/9] Dove PMU support

[Russell King - ARM Linux]

On Thu, Mar 19, 2015 at 11:02:35PM +0100, Rafael J. Wysocki wrote:
> On Thursday, March 19, 2015 10:59:20 PM Rafael J. Wysocki wrote:
> > On Thursday, March 12, 2015 06:30:21 PM Russell King - ARM Linux wrote:
> > > This is a re-posting of the patch set which I posted almost 10 months
> > > ago to support the Dove PMU, with a few additional changes.  This set
> > > is based upon 3.19.
> > > 
> > > In this set are:
> > > 
> > > * two patches which Rafael originally acked, but there was indecision
> > >   last time around how to handle them due to potential conflicts with
> > >   work that Ulf was doing.  These patches have been updated to apply
> > >   cleanly to 3.19.  I don't know if people want to take these as
> > >   fixes to the PM domain code or not (hence why I'm posting this
> > >   series during the merge window - if it weren't for this, I'd hold
> > >   it off.)
> > > 
> > > * what I regard as a fix to the PM domain code; as a result of a
> > >   previous commit, the PM domain code mismatches the runtime PM state,
> > >   which leads to the PM domain being unexpectedly left on.  This patch
> > >   has been re-worked to try an alternative approach, syncing the PM
> > >   domain state with the runtime PM state after the probe has completed.
> > > 
> > > * the addition of the core Dove PMU driver, which consists of a reset,
> > >   IRQ controller, and power domains.  The reset and power domain code
> > >   has to be closely related due to the power up/down requirements of
> > >   the GPU/VPU subsystems needing to be performed atomically.  (This
> > >   requirement prevents it using the MFD infrastructure, because we
> > >   would need to hold spinlocks while calling several different
> > >   sub-drivers.)
> > > 
> > > * addition of the RTC interrupt, so we can now receive and act on
> > >   alarms generated by the Dove RTC.
> > > 
> > > * addition of the DT descriptions for the GPU and VPU power domains.
> > >   These patches do not themselves add the DT descriptions for these
> > >   units, so these patches serve as illustrations how these should be
> > >   described.
> > > 
> > 
> > I can apply patches [1-3/9] from this series if that helps.
> 
> I mean from the next version of it ([FOR DISCUSSION 0/10] Dove PMU support).

Kevin had some comments on patch 2 which I think ought to be addressed.
He's quite right that dev_to_genpd() should be hidden from view and
replaced with the safer pm_genpd_lookup_dev() version - even though
pm_genpd_lookup_dev() itself is not fully safe against the domain
on a device changing beneath it.

If you want to hold on, I'll respin with his comments there addressed,
or if you prefer to merge them as-is and fix Kevin's comments afterwards,
that's also fine by me.

I'd just be happy to see some progress on this series. :)

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [FOR DISCUSSION 0/9] Dove PMU support

[Rafael J. Wysocki]

On Friday, March 20, 2015 12:16:59 PM Russell King - ARM Linux wrote:
> On Thu, Mar 19, 2015 at 11:02:35PM +0100, Rafael J. Wysocki wrote:
> > On Thursday, March 19, 2015 10:59:20 PM Rafael J. Wysocki wrote:
> > > On Thursday, March 12, 2015 06:30:21 PM Russell King - ARM Linux wrote:
> > > > This is a re-posting of the patch set which I posted almost 10 months
> > > > ago to support the Dove PMU, with a few additional changes.  This set
> > > > is based upon 3.19.
> > > > 
> > > > In this set are:
> > > > 
> > > > * two patches which Rafael originally acked, but there was indecision
> > > >   last time around how to handle them due to potential conflicts with
> > > >   work that Ulf was doing.  These patches have been updated to apply
> > > >   cleanly to 3.19.  I don't know if people want to take these as
> > > >   fixes to the PM domain code or not (hence why I'm posting this
> > > >   series during the merge window - if it weren't for this, I'd hold
> > > >   it off.)
> > > > 
> > > > * what I regard as a fix to the PM domain code; as a result of a
> > > >   previous commit, the PM domain code mismatches the runtime PM state,
> > > >   which leads to the PM domain being unexpectedly left on.  This patch
> > > >   has been re-worked to try an alternative approach, syncing the PM
> > > >   domain state with the runtime PM state after the probe has completed.
> > > > 
> > > > * the addition of the core Dove PMU driver, which consists of a reset,
> > > >   IRQ controller, and power domains.  The reset and power domain code
> > > >   has to be closely related due to the power up/down requirements of
> > > >   the GPU/VPU subsystems needing to be performed atomically.  (This
> > > >   requirement prevents it using the MFD infrastructure, because we
> > > >   would need to hold spinlocks while calling several different
> > > >   sub-drivers.)
> > > > 
> > > > * addition of the RTC interrupt, so we can now receive and act on
> > > >   alarms generated by the Dove RTC.
> > > > 
> > > > * addition of the DT descriptions for the GPU and VPU power domains.
> > > >   These patches do not themselves add the DT descriptions for these
> > > >   units, so these patches serve as illustrations how these should be
> > > >   described.
> > > > 
> > > 
> > > I can apply patches [1-3/9] from this series if that helps.
> > 
> > I mean from the next version of it ([FOR DISCUSSION 0/10] Dove PMU support).
> 
> Kevin had some comments on patch 2 which I think ought to be addressed.
> He's quite right that dev_to_genpd() should be hidden from view and
> replaced with the safer pm_genpd_lookup_dev() version - even though
> pm_genpd_lookup_dev() itself is not fully safe against the domain
> on a device changing beneath it.
> 
> If you want to hold on, I'll respin with his comments there addressed,
> or if you prefer to merge them as-is and fix Kevin's comments afterwards,
> that's also fine by me.

Both work for me, actually.  I think that Kevin has ACKed the patch despite
the comments, so it would be fine to apply it I guess.

> I'd just be happy to see some progress on this series. :)

Sure.

Re: [FOR DISCUSSION 0/9] Dove PMU support

[Russell King - ARM Linux]

On Fri, Mar 20, 2015 at 01:44:30PM +0100, Rafael J. Wysocki wrote:
> Both work for me, actually.  I think that Kevin has ACKed the patch despite
> the comments, so it would be fine to apply it I guess.

Here's the set of three - with the second one reworked.

 drivers/base/power/domain.c | 58 +++++++++++++++++++++++++++++----------------
 include/linux/pm_domain.h   |  6 ++---
 2 files changed, 40 insertions(+), 24 deletions(-)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/3] pm: domains: quieten down generic pm domains

[Russell King]

PM domains are rather noisy; scheduling behaviour can cause callbacks
to take longer, which causes them to spit out a warning-level message
each time a callback takes a little longer than the previous time.
There really isn't a need for this, except when debugging.

Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0d8780c04a5e..b3fbc21da2dc 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -173,8 +173,8 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
 	genpd->power_on_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
 	genpd_recalc_cpu_exit_latency(genpd);
-	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
-		genpd->name, "on", elapsed_ns);
+	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
+		 genpd->name, "on", elapsed_ns);
 
 	return ret;
 }
@@ -199,8 +199,8 @@ static int genpd_power_off(struct generic_pm_domain *genpd)
 
 	genpd->power_off_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
-	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
-		genpd->name, "off", elapsed_ns);
+	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
+		 genpd->name, "off", elapsed_ns);
 
 	return ret;
 }
-- 
1.8.3.1

[PATCH 2/3] pm: domains: factor out code to get the generic PM domain from a struct device

[Russell King]

The PM domain code contains two methods to get the generic PM domain
for a struct device.  One is dev_to_genpd() which is only safe when
we know for certain that the device has a generic PM domain attached.
The other is coded into genpd_dev_pm_detach() which ensures that the
PM domain in the struct device is a generic PM domain (and so is safer).

This commit factors out the safer version, documents it, and hides the
unsafe dev_to_genpd().

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++--------------
 include/linux/pm_domain.h   |  6 +++---
 2 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b3fbc21da2dc..89d2eea7f561 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -68,7 +68,36 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 	return genpd;
 }
 
-struct generic_pm_domain *dev_to_genpd(struct device *dev)
+/*
+ * Get the generic PM domain for a particular struct device.
+ * This validates the struct device pointer, the PM domain pointer,
+ * and checks that the PM domain pointer is a real generic PM domain.
+ * Any failure results in NULL being returned.
+ */
+struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
+{
+	struct generic_pm_domain *genpd = NULL, *gpd;
+
+	if (IS_ERR_OR_NULL(dev) || IS_ERR_OR_NULL(dev->pm_domain))
+		return NULL;
+
+	mutex_lock(&gpd_list_lock);
+	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+		if (&gpd->domain == dev->pm_domain) {
+			genpd = gpd;
+			break;
+		}
+	}
+	mutex_unlock(&gpd_list_lock);
+
+	return genpd;
+}
+
+/*
+ * This should only be used where we are certain that the pm_domain
+ * attached to the device is a genpd domain.
+ */
+static struct generic_pm_domain *dev_to_genpd(struct device *dev)
 {
 	if (IS_ERR_OR_NULL(dev->pm_domain))
 		return ERR_PTR(-EINVAL);
@@ -2120,21 +2149,10 @@ EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);
  */
 static void genpd_dev_pm_detach(struct device *dev, bool power_off)
 {
-	struct generic_pm_domain *pd = NULL, *gpd;
+	struct generic_pm_domain *pd;
 	int ret = 0;
 
-	if (!dev->pm_domain)
-		return;
-
-	mutex_lock(&gpd_list_lock);
-	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
-		if (&gpd->domain == dev->pm_domain) {
-			pd = gpd;
-			break;
-		}
-	}
-	mutex_unlock(&gpd_list_lock);
-
+	pd = pm_genpd_lookup_dev(dev);
 	if (!pd)
 		return;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index a9edab2c787a..441f17bdcb0a 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -129,7 +129,7 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
 	return to_gpd_data(dev->power.subsys_data->domain_data);
 }
 
-extern struct generic_pm_domain *dev_to_genpd(struct device *dev);
+extern struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev);
 extern int __pm_genpd_add_device(struct generic_pm_domain *genpd,
 				 struct device *dev,
 				 struct gpd_timing_data *td);
@@ -166,9 +166,9 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
 {
 	return ERR_PTR(-ENOSYS);
 }
-static inline struct generic_pm_domain *dev_to_genpd(struct device *dev)
+static inline struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd,
 					struct device *dev,
-- 
1.8.3.1

Re: [PATCH 2/3] pm: domains: factor out code to get the generic PM domain from a struct device

[Ulf Hansson]

On 20 March 2015 at 18:20, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> The PM domain code contains two methods to get the generic PM domain
> for a struct device.  One is dev_to_genpd() which is only safe when
> we know for certain that the device has a generic PM domain attached.
> The other is coded into genpd_dev_pm_detach() which ensures that the
> PM domain in the struct device is a generic PM domain (and so is safer).
>
> This commit factors out the safer version, documents it, and hides the
> unsafe dev_to_genpd().
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++--------------
>  include/linux/pm_domain.h   |  6 +++---
>  2 files changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index b3fbc21da2dc..89d2eea7f561 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -68,7 +68,36 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>         return genpd;
>  }
>
> -struct generic_pm_domain *dev_to_genpd(struct device *dev)
> +/*
> + * Get the generic PM domain for a particular struct device.
> + * This validates the struct device pointer, the PM domain pointer,
> + * and checks that the PM domain pointer is a real generic PM domain.
> + * Any failure results in NULL being returned.
> + */
> +struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)

I wouldn't mind keeping also this function static, unless you foresee
users outside genpd?

If converting to static please rename it to genpd_lookup_dev().

> +{
> +       struct generic_pm_domain *genpd = NULL, *gpd;
> +
> +       if (IS_ERR_OR_NULL(dev) || IS_ERR_OR_NULL(dev->pm_domain))
> +               return NULL;
> +
> +       mutex_lock(&gpd_list_lock);
> +       list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> +               if (&gpd->domain == dev->pm_domain) {
> +                       genpd = gpd;
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return genpd;
> +}
> +
> +/*
> + * This should only be used where we are certain that the pm_domain
> + * attached to the device is a genpd domain.
> + */
> +static struct generic_pm_domain *dev_to_genpd(struct device *dev)
>  {
>         if (IS_ERR_OR_NULL(dev->pm_domain))
>                 return ERR_PTR(-EINVAL);
> @@ -2120,21 +2149,10 @@ EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);
>   */
>  static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>  {
> -       struct generic_pm_domain *pd = NULL, *gpd;
> +       struct generic_pm_domain *pd;
>         int ret = 0;
>
> -       if (!dev->pm_domain)
> -               return;
> -
> -       mutex_lock(&gpd_list_lock);
> -       list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> -               if (&gpd->domain == dev->pm_domain) {
> -                       pd = gpd;
> -                       break;
> -               }
> -       }
> -       mutex_unlock(&gpd_list_lock);
> -
> +       pd = pm_genpd_lookup_dev(dev);
>         if (!pd)
>                 return;
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index a9edab2c787a..441f17bdcb0a 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -129,7 +129,7 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
>         return to_gpd_data(dev->power.subsys_data->domain_data);
>  }
>
> -extern struct generic_pm_domain *dev_to_genpd(struct device *dev);
> +extern struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev);
>  extern int __pm_genpd_add_device(struct generic_pm_domain *genpd,
>                                  struct device *dev,
>                                  struct gpd_timing_data *td);
> @@ -166,9 +166,9 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
>  {
>         return ERR_PTR(-ENOSYS);
>  }
> -static inline struct generic_pm_domain *dev_to_genpd(struct device *dev)
> +static inline struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
>  {
> -       return ERR_PTR(-ENOSYS);
> +       return NULL;
>  }
>  static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd,
>                                         struct device *dev,
> --
> 1.8.3.1
>

Kind regards
Uffe

Re: [PATCH 2/3] pm: domains: factor out code to get the generic PM domain from a struct device

[Russell King - ARM Linux]

On Mon, Mar 23, 2015 at 02:28:14PM +0100, Ulf Hansson wrote:
> On 20 March 2015 at 18:20, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> > The PM domain code contains two methods to get the generic PM domain
> > for a struct device.  One is dev_to_genpd() which is only safe when
> > we know for certain that the device has a generic PM domain attached.
> > The other is coded into genpd_dev_pm_detach() which ensures that the
> > PM domain in the struct device is a generic PM domain (and so is safer).
> >
> > This commit factors out the safer version, documents it, and hides the
> > unsafe dev_to_genpd().
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++--------------
> >  include/linux/pm_domain.h   |  6 +++---
> >  2 files changed, 35 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index b3fbc21da2dc..89d2eea7f561 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -68,7 +68,36 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
> >         return genpd;
> >  }
> >
> > -struct generic_pm_domain *dev_to_genpd(struct device *dev)
> > +/*
> > + * Get the generic PM domain for a particular struct device.
> > + * This validates the struct device pointer, the PM domain pointer,
> > + * and checks that the PM domain pointer is a real generic PM domain.
> > + * Any failure results in NULL being returned.
> > + */
> > +struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
> 
> I wouldn't mind keeping also this function static, unless you foresee
> users outside genpd?

_I_ have users of it outside at the moment.

Once the major churn in PM domains is over and I have no use for this
externally, I'll consider making it static in my patch set - but all
the time that doing so results in breakage for me...

Plus, you're asking me to do yet another 20+ minute re-spin of this
patch set.  I'm going to refuse; I'm almost at the point of just not
giving a damn on this stuff, it's wasting too much of my time.
Especially as, yet again, you should have replied to the emails in the
previous round suggesting this change.

Rafael, please merge these three as we previously agreed.  Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/3] pm: domains: factor out code to get the generic PM domain from a struct device

[Rafael J. Wysocki]

On Monday, March 23, 2015 03:17:40 PM Russell King - ARM Linux wrote:
> On Mon, Mar 23, 2015 at 02:28:14PM +0100, Ulf Hansson wrote:
> > On 20 March 2015 at 18:20, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> > > The PM domain code contains two methods to get the generic PM domain
> > > for a struct device.  One is dev_to_genpd() which is only safe when
> > > we know for certain that the device has a generic PM domain attached.
> > > The other is coded into genpd_dev_pm_detach() which ensures that the
> > > PM domain in the struct device is a generic PM domain (and so is safer).
> > >
> > > This commit factors out the safer version, documents it, and hides the
> > > unsafe dev_to_genpd().
> > >
> > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > ---
> > >  drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++--------------
> > >  include/linux/pm_domain.h   |  6 +++---
> > >  2 files changed, 35 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > > index b3fbc21da2dc..89d2eea7f561 100644
> > > --- a/drivers/base/power/domain.c
> > > +++ b/drivers/base/power/domain.c
> > > @@ -68,7 +68,36 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
> > >         return genpd;
> > >  }
> > >
> > > -struct generic_pm_domain *dev_to_genpd(struct device *dev)
> > > +/*
> > > + * Get the generic PM domain for a particular struct device.
> > > + * This validates the struct device pointer, the PM domain pointer,
> > > + * and checks that the PM domain pointer is a real generic PM domain.
> > > + * Any failure results in NULL being returned.
> > > + */
> > > +struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
> > 
> > I wouldn't mind keeping also this function static, unless you foresee
> > users outside genpd?
> 
> _I_ have users of it outside at the moment.
> 
> Once the major churn in PM domains is over and I have no use for this
> externally, I'll consider making it static in my patch set - but all
> the time that doing so results in breakage for me...
> 
> Plus, you're asking me to do yet another 20+ minute re-spin of this
> patch set.  I'm going to refuse; I'm almost at the point of just not
> giving a damn on this stuff, it's wasting too much of my time.
> Especially as, yet again, you should have replied to the emails in the
> previous round suggesting this change.
> 
> Rafael, please merge these three as we previously agreed.  Thanks.

I said I would do that, didn't I?

I've applied this series plus https://patchwork.kernel.org/patch/6056201/
and https://patchwork.kernel.org/patch/6057441/ on top of that.

Please let me know if you need anything more from the core.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/3] pm: domains: factor out code to get the generic PM domain from a struct device

[Russell King - ARM Linux]

On Tue, Mar 24, 2015 at 01:29:05AM +0100, Rafael J. Wysocki wrote:
> On Monday, March 23, 2015 03:17:40 PM Russell King - ARM Linux wrote:
> > Rafael, please merge these three as we previously agreed.  Thanks.
> 
> I said I would do that, didn't I?

I wasn't sure if you were going to hold back on it as a result of Uwe's
comments.

> I've applied this series plus https://patchwork.kernel.org/patch/6056201/
> and https://patchwork.kernel.org/patch/6057441/ on top of that.
> Please let me know if you need anything more from the core.

I'll replicate what you've applied and test it here (build in progress...)

Thanks!

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/3] pm: domains: factor out code to get the generic PM domain from a struct device

[Russell King - ARM Linux]

On Thu, Mar 26, 2015 at 03:20:43PM +0000, Russell King - ARM Linux wrote:
> I'll replicate what you've applied and test it here (build in progress...)

It looks like it works as desired.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 3/3] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Russell King]

pm_genpd_remove_device() tries hard to validate the generic PM domain
passed to it, but the validation is not complete.

dev->pm_domain contains a struct dev_pm_domain, which is the "base
class" of generic PM domains.  Other users of dev_pm_domains include
stuff like vga_switheroo.  Hence, a device could have a generic PM
domain or a vga_switcheroo PM domain in dev->pm_domain.

We need ot be certain that the PM domain is actually valid before we
try to remove it.  We can do this easily as we have a way to get the
current validated generic PM domain for a struct device.  This must
match the generic PM domain being requested for removal.

Convert the code to use this alternative validation method instead.

Acked-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/power/domain.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 89d2eea7f561..5f560fa45a32 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1538,9 +1538,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	dev_dbg(dev, "%s()\n", __func__);
 
-	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)
-	    ||  IS_ERR_OR_NULL(dev->pm_domain)
-	    ||  pd_to_genpd(dev->pm_domain) != genpd)
+	if (!genpd || genpd != pm_genpd_lookup_dev(dev))
 		return -EINVAL;
 
 	genpd_acquire_lock(genpd);
-- 
1.8.3.1

Re: [PATCH 3/3] pm: domains: avoid potential oops in pm_genpd_remove_device()

[Ulf Hansson]

On 20 March 2015 at 18:20, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> pm_genpd_remove_device() tries hard to validate the generic PM domain
> passed to it, but the validation is not complete.
>
> dev->pm_domain contains a struct dev_pm_domain, which is the "base
> class" of generic PM domains.  Other users of dev_pm_domains include
> stuff like vga_switheroo.  Hence, a device could have a generic PM
> domain or a vga_switcheroo PM domain in dev->pm_domain.
>
> We need ot be certain that the PM domain is actually valid before we
> try to remove it.  We can do this easily as we have a way to get the
> current validated generic PM domain for a struct device.  This must
> match the generic PM domain being requested for removal.
>
> Convert the code to use this alternative validation method instead.
>
> Acked-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/base/power/domain.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 89d2eea7f561..5f560fa45a32 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1538,9 +1538,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>
>         dev_dbg(dev, "%s()\n", __func__);
>
> -       if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)
> -           ||  IS_ERR_OR_NULL(dev->pm_domain)
> -           ||  pd_to_genpd(dev->pm_domain) != genpd)
> +       if (!genpd || genpd != pm_genpd_lookup_dev(dev))
>                 return -EINVAL;
>
>         genpd_acquire_lock(genpd);
> --
> 1.8.3.1
>