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

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

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

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 <rjw@sisk.pl>
---
 arch/arm/mach-shmobile/pm_runtime.c      |    3 ++
 arch/sh/kernel/cpu/shmobile/pm_runtime.c |    3 ++
 drivers/base/platform.c                  |   43 ++++++++++---------------------
 include/linux/platform_device.h          |    4 ++
 4 files changed, 24 insertions(+), 29 deletions(-)

Index: linux-2.6/drivers/base/platform.c
===================================================================
--- linux-2.6.orig/drivers/base/platform.c
+++ linux-2.6/drivers/base/platform.c
@@ -922,32 +922,7 @@ static int platform_pm_restore_noirq(str
 
 #endif /* !CONFIG_HIBERNATION */
 
-#ifdef CONFIG_PM_RUNTIME
-
-int __weak platform_pm_runtime_suspend(struct device *dev)
-{
-	return pm_generic_runtime_suspend(dev);
-};
-
-int __weak platform_pm_runtime_resume(struct device *dev)
-{
-	return pm_generic_runtime_resume(dev);
-};
-
-int __weak platform_pm_runtime_idle(struct device *dev)
-{
-	return pm_generic_runtime_idle(dev);
-};
-
-#else /* !CONFIG_PM_RUNTIME */
-
-#define platform_pm_runtime_suspend NULL
-#define platform_pm_runtime_resume NULL
-#define platform_pm_runtime_idle NULL
-
-#endif /* !CONFIG_PM_RUNTIME */
-
-static const struct dev_pm_ops platform_dev_pm_ops = {
+static struct dev_pm_ops platform_dev_pm_ops = {
 	.prepare = platform_pm_prepare,
 	.complete = platform_pm_complete,
 	.suspend = platform_pm_suspend,
@@ -962,9 +937,9 @@ static const struct dev_pm_ops platform_
 	.thaw_noirq = platform_pm_thaw_noirq,
 	.poweroff_noirq = platform_pm_poweroff_noirq,
 	.restore_noirq = platform_pm_restore_noirq,
-	.runtime_suspend = platform_pm_runtime_suspend,
-	.runtime_resume = platform_pm_runtime_resume,
-	.runtime_idle = platform_pm_runtime_idle,
+	SET_RUNTIME_PM_OPS(pm_generic_runtime_suspend,
+			   pm_generic_runtime_resume,
+			   pm_generic_runtime_idle)
 };
 
 struct bus_type platform_bus_type = {
@@ -976,6 +951,16 @@ struct bus_type platform_bus_type = {
 };
 EXPORT_SYMBOL_GPL(platform_bus_type);
 
+void platform_set_runtime_pm_ops(int (*suspend)(struct device *dev),
+				 int (*resume)(struct device *dev),
+				 int (*idle)(struct device *dev))
+{
+	platform_dev_pm_ops.runtime_suspend = suspend;
+	platform_dev_pm_ops.runtime_resume = resume;
+	platform_dev_pm_ops.runtime_idle = idle;
+}
+EXPORT_SYMBOL_GPL(platform_set_runtime_pm_ops);
+
 /**
  * platform_bus_get_pm_ops() - return pointer to busses dev_pm_ops
  *
Index: linux-2.6/include/linux/platform_device.h
===================================================================
--- linux-2.6.orig/include/linux/platform_device.h
+++ linux-2.6/include/linux/platform_device.h
@@ -200,4 +200,8 @@ static inline char *early_platform_drive
 }
 #endif /* MODULE */
 
+extern void platform_set_runtime_pm_ops(int (*suspend)(struct device *dev),
+					int (*resume)(struct device *dev),
+					int (*idle)(struct device *dev));
+
 #endif /* _PLATFORM_DEVICE_H_ */
Index: linux-2.6/arch/arm/mach-shmobile/pm_runtime.c
===================================================================
--- linux-2.6.orig/arch/arm/mach-shmobile/pm_runtime.c
+++ linux-2.6/arch/arm/mach-shmobile/pm_runtime.c
@@ -163,6 +163,9 @@ static struct notifier_block platform_bu
 
 static int __init sh_pm_runtime_init(void)
 {
+	platform_set_runtime_pm_ops(platform_pm_runtime_suspend,
+				    platform_pm_runtime_resume,
+				    platform_pm_runtime_idle);
 	bus_register_notifier(&platform_bus_type, &platform_bus_notifier);
 	return 0;
 }
Index: linux-2.6/arch/sh/kernel/cpu/shmobile/pm_runtime.c
===================================================================
--- linux-2.6.orig/arch/sh/kernel/cpu/shmobile/pm_runtime.c
+++ linux-2.6/arch/sh/kernel/cpu/shmobile/pm_runtime.c
@@ -302,6 +302,9 @@ static int __init sh_pm_runtime_init(voi
 {
 	INIT_WORK(&hwblk_work, platform_pm_runtime_work);
 
+	platform_set_runtime_pm_ops(platform_pm_runtime_suspend,
+				    platform_pm_runtime_resume,
+				    platform_pm_runtime_idle);
 	bus_register_notifier(&platform_bus_type, &platform_bus_notifier);
 	return 0;
 }

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

[Magnus Damm]

On Sun, Mar 27, 2011 at 8:58 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> 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 <rjw@sisk.pl>

Looking good, thanks Rafael. Tested on the sh7372 Mackerel board.

Acked-by: Magnus Damm <damm@opensource.se>

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

[Rafael J. Wysocki]

On Monday, March 28, 2011, Magnus Damm wrote:
> On Sun, Mar 27, 2011 at 8:58 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > 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 <rjw@sisk.pl>
> 
> Looking good, thanks Rafael. Tested on the sh7372 Mackerel board.
> 
> Acked-by: Magnus Damm <damm@opensource.se>

Thanks, I'll put it into my for-2.6.40 queue as soon as 2.6.39-rc1 is out.

Rafael

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

[Paul Mundt]

On Sun, Mar 27, 2011 at 12:58:41AM +0100, Rafael J. Wysocki wrote:
> 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 <rjw@sisk.pl>

Acked-by: Paul Mundt <lethal@linux-sh.org>

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

[Magnus Damm]

On Tue, Mar 29, 2011 at 4:43 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, March 28, 2011, Magnus Damm wrote:
>> On Sun, Mar 27, 2011 at 8:58 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > From: Rafael J. Wysocki <rjw@sisk.pl>
>> >
>> > 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 <rjw@sisk.pl>
>>
>> Looking good, thanks Rafael. Tested on the sh7372 Mackerel board.
>>
>> Acked-by: Magnus Damm <damm@opensource.se>
>
> Thanks, I'll put it into my for-2.6.40 queue as soon as 2.6.39-rc1 is out.

Thanks. By the way, I think the symbols should be converted to static
as well. Do you prefer to make a V2 or shall we do that incrementally?

/ magnus

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

[Rafael J. Wysocki]

On Tuesday, April 05, 2011, Magnus Damm wrote:
> On Tue, Mar 29, 2011 at 4:43 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Monday, March 28, 2011, Magnus Damm wrote:
> >> On Sun, Mar 27, 2011 at 8:58 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >> >
> >> > 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 <rjw@sisk.pl>
> >>
> >> Looking good, thanks Rafael. Tested on the sh7372 Mackerel board.
> >>
> >> Acked-by: Magnus Damm <damm@opensource.se>
> >
> > Thanks, I'll put it into my for-2.6.40 queue as soon as 2.6.39-rc1 is out.
> 
> Thanks. By the way, I think the symbols should be converted to static
> as well. Do you prefer to make a V2 or shall we do that incrementally?

I can fold that changed into the patch as I haven't pushed it yet.

Thanks,
Rafael

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

[Kevin Hilman]

Hi Rafael, Magnus,

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> 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 <rjw@sisk.pl>

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 <khilman@ti.com>
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 <damm@opensource.se>
Cc: Paul Mundt <lethal@linux-sh.org>
Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 arch/arm/mach-shmobile/pm_runtime.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

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;
-- 
1.7.4

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

[Rafael J. Wysocki]

On Thursday, April 07, 2011, Kevin Hilman wrote:
> Hi Rafael, Magnus,
> 
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > 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 <rjw@sisk.pl>
> 
> 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

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

[Grant Likely]

On Wed, Apr 06, 2011 at 03:35:32PM -0700, Kevin Hilman wrote:
> Hi Rafael, Magnus,
> 
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > 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 <rjw@sisk.pl>
> 
> 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.

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

[Grant Likely]

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" <rjw@sisk.pl> writes:
> > 
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > >
> > > 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 <rjw@sisk.pl>
> > 
> > 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.

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

[Rafael J. Wysocki]

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" <rjw@sisk.pl> writes:
> > > 
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > >
> > > > 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 <rjw@sisk.pl>
> > > 
> > > 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

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

[Grant Likely]

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" <rjw@sisk.pl> writes:
> > > > 
> > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > >
> > > > > 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 <rjw@sisk.pl>
> > > > 
> > > > 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.