| Message ID | 87zko3dn4b.fsf@ti.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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.
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.
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
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.
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;