[RFC] PM / Platform: Remove __weak definitions of runtime PM callbacks

Commit Message

Kevin Hilman April 6, 2011, 10:35 p.m. UTC

Hi Rafael, Magnus,

"Rafael J. Wysocki" <[e-mail]> writes:

> From: Rafael J. Wysocki <[e-mail]>
>
> Remove the __weak definitions of platform bus type runtime PM
> callbacks, make platform_dev_pm_ops point to the generic routines
> as appropriate and allow architectures using platform_dev_pm_ops to
> replace the runtime PM callbacks in that structure with their own
> set.
>
> Convert architectures providing its own definitions of the platform
> runtime PM callbacks to use the new mechanism.
>
> Signed-off-by: Rafael J. Wysocki <[e-mail]>

I dont't think we should be adding yet another new interface for setting
platform-specific runtime PM ops.

We now have 3.  Two existing ones:

1) new device power domains (presumably preferred)
2) platform_bus_set_pm_ops() (disliked by many)

and now the new one you create here

3) platform_set_runtime_pm_ops()

This new one is basically the same as platform_bus_set_pm_ops(), but
targetted only at runtime PM ops, and also has all the same problems
that have been discussed before.  Namely, it overrides the pm ops for
*every* device on the platform_bus, instead of targetting only specific
devices.  With the new device power domains, we can target specific
devices.

Wouldn't the right way to go here be to convert mach-shmobile over to
using device power domains?

The patch below against v2.6.39-rc2 combined with your patch (minus the
mach-shmobile/* changes) should do it.

Magnus, care to test?

If SH-mobile is converted to use device powerdomains, not only can we
drop this new platform_set_runtime_pm_ops(), but we can also drop
platform_bus_set_pm_ops() (I have a patch for this as soon as I post
the OMAP conversions to device power domains.)

That will leave only a single interface for overriding the runtime PM
ops: device power domains.  Personally, I prefer that as it's flexible
enough, and also allows platforms to target only specific devices
instead of the whole bus.

Kevin


From c8176cdb019ebbb055d70212b7d69c778d3b4b35 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <[e-mail]>
Date: Wed, 6 Apr 2011 15:25:11 -0700
Subject: [PATCH] ARM: sh-mobile: runtime PM: convert to device powerdomains

Remove the deprecated use of weak platform_bus symbols in favor of using
the new device power domains.

Cc: Magnus Damm <[e-mail]>
Cc: Paul Mundt <[e-mail]>
Signed-off-by: Kevin Hilman <[e-mail]>
---
 arch/arm/mach-shmobile/pm_runtime.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

Comments

Rafael Wysocki April 7, 2011, 5:29 a.m. UTC | #1

On Thursday, April 07, 2011, Kevin Hilman wrote:
> Hi Rafael, Magnus,
> 
> "Rafael J. Wysocki" <[e-mail]> writes:
> 
> > From: Rafael J. Wysocki <[e-mail]>
> >
> > Remove the __weak definitions of platform bus type runtime PM
> > callbacks, make platform_dev_pm_ops point to the generic routines
> > as appropriate and allow architectures using platform_dev_pm_ops to
> > replace the runtime PM callbacks in that structure with their own
> > set.
> >
> > Convert architectures providing its own definitions of the platform
> > runtime PM callbacks to use the new mechanism.
> >
> > Signed-off-by: Rafael J. Wysocki <[e-mail]>
> 
> I dont't think we should be adding yet another new interface for setting
> platform-specific runtime PM ops.
> 
> We now have 3.  Two existing ones:
> 
> 1) new device power domains (presumably preferred)
> 2) platform_bus_set_pm_ops() (disliked by many)

Hmm, I wasn't aware of that one, will have a look.

> and now the new one you create here
> 
> 3) platform_set_runtime_pm_ops()
> 
> This new one is basically the same as platform_bus_set_pm_ops(), but
> targetted only at runtime PM ops, and also has all the same problems
> that have been discussed before.  Namely, it overrides the pm ops for
> *every* device on the platform_bus, instead of targetting only specific
> devices.

This is not a problem for this particular use case.  We really want to
replace the PM ops for all of the platform devices on that platform.

Though I agree it probably makes more sense to use the existing
platform_bus_set_pm_ops() for this purpose.

> With the new device power domains, we can target specific devices.
> 
> Wouldn't the right way to go here be to convert mach-shmobile over to
> using device power domains?

Not for this particular purpose.
 
> The patch below against v2.6.39-rc2 combined with your patch (minus the
> mach-shmobile/* changes) should do it.

Unfortunately it would conflict with work in progress introducing _real_
power domains on shmobile.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Likely April 7, 2011, 5:44 a.m. UTC | #2

On Wed, Apr 06, 2011 at 03:35:32PM -0700, Kevin Hilman wrote:
> Hi Rafael, Magnus,
> 
> "Rafael J. Wysocki" <[e-mail]> writes:
> 
> > From: Rafael J. Wysocki <[e-mail]>
> >
> > Remove the __weak definitions of platform bus type runtime PM
> > callbacks, make platform_dev_pm_ops point to the generic routines
> > as appropriate and allow architectures using platform_dev_pm_ops to
> > replace the runtime PM callbacks in that structure with their own
> > set.
> >
> > Convert architectures providing its own definitions of the platform
> > runtime PM callbacks to use the new mechanism.
> >
> > Signed-off-by: Rafael J. Wysocki <[e-mail]>
> 
> I dont't think we should be adding yet another new interface for setting
> platform-specific runtime PM ops.
> 
> We now have 3.  Two existing ones:
> 
> 1) new device power domains (presumably preferred)
> 2) platform_bus_set_pm_ops() (disliked by many)
> 
> and now the new one you create here
> 
> 3) platform_set_runtime_pm_ops()
> 
> This new one is basically the same as platform_bus_set_pm_ops(), but
> targetted only at runtime PM ops, and also has all the same problems
> that have been discussed before.  Namely, it overrides the pm ops for
> *every* device on the platform_bus, instead of targetting only specific
> devices.  With the new device power domains, we can target specific
> devices.
> 
> Wouldn't the right way to go here be to convert mach-shmobile over to
> using device power domains?

I agree.  +1

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Likely April 7, 2011, 5:48 a.m. UTC | #3

On Thu, Apr 07, 2011 at 07:29:45AM +0200, Rafael J. Wysocki wrote:
> On Thursday, April 07, 2011, Kevin Hilman wrote:
> > Hi Rafael, Magnus,
> > 
> > "Rafael J. Wysocki" <[e-mail]> writes:
> > 
> > > From: Rafael J. Wysocki <[e-mail]>
> > >
> > > Remove the __weak definitions of platform bus type runtime PM
> > > callbacks, make platform_dev_pm_ops point to the generic routines
> > > as appropriate and allow architectures using platform_dev_pm_ops to
> > > replace the runtime PM callbacks in that structure with their own
> > > set.
> > >
> > > Convert architectures providing its own definitions of the platform
> > > runtime PM callbacks to use the new mechanism.
> > >
> > > Signed-off-by: Rafael J. Wysocki <[e-mail]>
> > 
> > I dont't think we should be adding yet another new interface for setting
> > platform-specific runtime PM ops.
> > 
> > We now have 3.  Two existing ones:
> > 
> > 1) new device power domains (presumably preferred)
> > 2) platform_bus_set_pm_ops() (disliked by many)
> 
> Hmm, I wasn't aware of that one, will have a look.
> 
> > and now the new one you create here
> > 
> > 3) platform_set_runtime_pm_ops()
> > 
> > This new one is basically the same as platform_bus_set_pm_ops(), but
> > targetted only at runtime PM ops, and also has all the same problems
> > that have been discussed before.  Namely, it overrides the pm ops for
> > *every* device on the platform_bus, instead of targetting only specific
> > devices.
> 
> This is not a problem for this particular use case.  We really want to
> replace the PM ops for all of the platform devices on that platform.

I strongly doubt that you really want to do that.  platform_devices
can appear anywhere in the system, and many of them will end up being
entirely outside the SoC, and hence outside of any SoC specific
behaviour.  What is the use case for overriding every
platform_device's PM ops?

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael Wysocki April 7, 2011, 6:15 a.m. UTC | #4

On Thursday, April 07, 2011, Grant Likely wrote:
> On Thu, Apr 07, 2011 at 07:29:45AM +0200, Rafael J. Wysocki wrote:
> > On Thursday, April 07, 2011, Kevin Hilman wrote:
> > > Hi Rafael, Magnus,
> > > 
> > > "Rafael J. Wysocki" <[e-mail]> writes:
> > > 
> > > > From: Rafael J. Wysocki <[e-mail]>
> > > >
> > > > Remove the __weak definitions of platform bus type runtime PM
> > > > callbacks, make platform_dev_pm_ops point to the generic routines
> > > > as appropriate and allow architectures using platform_dev_pm_ops to
> > > > replace the runtime PM callbacks in that structure with their own
> > > > set.
> > > >
> > > > Convert architectures providing its own definitions of the platform
> > > > runtime PM callbacks to use the new mechanism.
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <[e-mail]>
> > > 
> > > I dont't think we should be adding yet another new interface for setting
> > > platform-specific runtime PM ops.
> > > 
> > > We now have 3.  Two existing ones:
> > > 
> > > 1) new device power domains (presumably preferred)
> > > 2) platform_bus_set_pm_ops() (disliked by many)
> > 
> > Hmm, I wasn't aware of that one, will have a look.
> > 
> > > and now the new one you create here
> > > 
> > > 3) platform_set_runtime_pm_ops()
> > > 
> > > This new one is basically the same as platform_bus_set_pm_ops(), but
> > > targetted only at runtime PM ops, and also has all the same problems
> > > that have been discussed before.  Namely, it overrides the pm ops for
> > > *every* device on the platform_bus, instead of targetting only specific
> > > devices.
> > 
> > This is not a problem for this particular use case.  We really want to
> > replace the PM ops for all of the platform devices on that platform.
> 
> I strongly doubt that you really want to do that.  platform_devices
> can appear anywhere in the system, and many of them will end up being
> entirely outside the SoC, and hence outside of any SoC specific
> behaviour.

That is a valid observation, but I still think the way Kevin attempted to
use the power domain callbacks wasn't the right one for addressing this
particular issue.

> What is the use case for overriding every platform_device's PM ops?

The basic idea, which I agree with, is that we should avoid saving device
registers when the device is not going to be powered down (i.e. we only
want to gate its clock).  Since the saving of device registers is generally
done by device drivers' suspend callbacks, it's better to avoid executing
those callbacks until we know the devices in question are going to be powered
down.  That, however, is not known to the default platform bus type
callbacks that automatically invoke the drivers' callbacks if they exist.
Hence, it's better to replace the default platform bus type callbacks with
other ones that only disable the devices' clocks and let power domain
callbacks (that should know whether or not the devices will be powered down)
handle the rest.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Likely April 7, 2011, 7:09 a.m. UTC | #5

On Thu, Apr 07, 2011 at 08:15:41AM +0200, Rafael J. Wysocki wrote:
> On Thursday, April 07, 2011, Grant Likely wrote:
> > On Thu, Apr 07, 2011 at 07:29:45AM +0200, Rafael J. Wysocki wrote:
> > > On Thursday, April 07, 2011, Kevin Hilman wrote:
> > > > Hi Rafael, Magnus,
> > > > 
> > > > "Rafael J. Wysocki" <[e-mail]> writes:
> > > > 
> > > > > From: Rafael J. Wysocki <[e-mail]>
> > > > >
> > > > > Remove the __weak definitions of platform bus type runtime PM
> > > > > callbacks, make platform_dev_pm_ops point to the generic routines
> > > > > as appropriate and allow architectures using platform_dev_pm_ops to
> > > > > replace the runtime PM callbacks in that structure with their own
> > > > > set.
> > > > >
> > > > > Convert architectures providing its own definitions of the platform
> > > > > runtime PM callbacks to use the new mechanism.
> > > > >
> > > > > Signed-off-by: Rafael J. Wysocki <[e-mail]>
> > > > 
> > > > I dont't think we should be adding yet another new interface for setting
> > > > platform-specific runtime PM ops.
> > > > 
> > > > We now have 3.  Two existing ones:
> > > > 
> > > > 1) new device power domains (presumably preferred)
> > > > 2) platform_bus_set_pm_ops() (disliked by many)
> > > 
> > > Hmm, I wasn't aware of that one, will have a look.
> > > 
> > > > and now the new one you create here
> > > > 
> > > > 3) platform_set_runtime_pm_ops()
> > > > 
> > > > This new one is basically the same as platform_bus_set_pm_ops(), but
> > > > targetted only at runtime PM ops, and also has all the same problems
> > > > that have been discussed before.  Namely, it overrides the pm ops for
> > > > *every* device on the platform_bus, instead of targetting only specific
> > > > devices.
> > > 
> > > This is not a problem for this particular use case.  We really want to
> > > replace the PM ops for all of the platform devices on that platform.
> > 
> > I strongly doubt that you really want to do that.  platform_devices
> > can appear anywhere in the system, and many of them will end up being
> > entirely outside the SoC, and hence outside of any SoC specific
> > behaviour.
> 
> That is a valid observation, but I still think the way Kevin attempted to
> use the power domain callbacks wasn't the right one for addressing this
> particular issue.
> 
> > What is the use case for overriding every platform_device's PM ops?
> 
> The basic idea, which I agree with, is that we should avoid saving device
> registers when the device is not going to be powered down (i.e. we only
> want to gate its clock).  Since the saving of device registers is generally
> done by device drivers' suspend callbacks, it's better to avoid executing
> those callbacks until we know the devices in question are going to be powered
> down.  That, however, is not known to the default platform bus type
> callbacks that automatically invoke the drivers' callbacks if they exist.
> Hence, it's better to replace the default platform bus type callbacks with
> other ones that only disable the devices' clocks and let power domain
> callbacks (that should know whether or not the devices will be powered down)
> handle the rest.

Okay, I think I understand the scenario.  However, replacing the default
behaviour for the entire platform_bus_type I still think is too large
a hammer.  The default behaviour is to assume worst case behaviour for
platform_devices, which means that it doesn't know what the parent of
the device is going to do after the suspend event.  It could do
anything, so platform_bus_type assumes the worst.  Since
platform_devices can turn up anywhere, I think that is the right
behaviour and any override really needs to be per-device.

That said, I'll let you and Kevin work out what the /correct/ approach
for doing those per-device overrides should be. :-)

g.

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/mach-shmobile/pm_runtime.c b/arch/arm/mach-shmobile/pm_runtime.c
index 94912d3..6c75c3f 100644
--- a/arch/arm/mach-shmobile/pm_runtime.c
+++ b/arch/arm/mach-shmobile/pm_runtime.c
@@ -66,7 +66,7 @@  static void platform_pm_runtime_bug(struct device *dev,
 		dev_err(dev, "runtime pm suspend before resume\n");
 }
 
-int platform_pm_runtime_suspend(struct device *dev)
+static int platform_pm_runtime_suspend(struct device *dev)
 {
 	struct pm_runtime_data *prd = __to_prd(dev);
 
@@ -82,7 +82,7 @@  int platform_pm_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-int platform_pm_runtime_resume(struct device *dev)
+static int platform_pm_runtime_resume(struct device *dev)
 {
 	struct pm_runtime_data *prd = __to_prd(dev);
 
@@ -98,12 +98,20 @@  int platform_pm_runtime_resume(struct device *dev)
 	return 0;
 }
 
-int platform_pm_runtime_idle(struct device *dev)
+static int platform_pm_runtime_idle(struct device *dev)
 {
 	/* suspend synchronously to disable clocks immediately */
 	return pm_runtime_suspend(dev);
 }
 
+static struct dev_power_domain platform_pm_power_domain = {
+	.ops = {
+		.runtime_suspend = platform_pm_runtime_suspend,
+		.runtime_resume = platform_pm_runtime_resume,
+		.runtime_idle = platform_pm_runtime_idle,
+	},
+};
+
 static int platform_bus_notify(struct notifier_block *nb,
 			       unsigned long action, void *data)
 {
@@ -114,10 +122,12 @@  static int platform_bus_notify(struct notifier_block *nb,
 
 	if (action == BUS_NOTIFY_BIND_DRIVER) {
 		prd = devres_alloc(__devres_release, sizeof(*prd), GFP_KERNEL);
-		if (prd)
+		if (prd) {
 			devres_add(dev, prd);
-		else
+			dev->pwr_domain = &platform_pm_power_domain;
+		} else {
 			dev_err(dev, "unable to alloc memory for runtime pm\n");
+		}
 	}
 
 	return 0;