Felipe Balbi <balbi@xxxxxx> writes: > Hi, > > On Tue, Jul 05, 2011 at 10:37:40AM -0700, Kevin Hilman wrote: >> While I did design the OMAP PM core to be runtime PM centric, and we >> implemented several drivers based on runtime PM alone, after some long >> discussions on linux-pm[1] with Alan and Rafael (PM maintainer) over the >> last couple weeks, I'm now convinced I had the wrong design/approach. >> >> Rafael and Alan have been patient with my stubborness, but now I've been >> pursuaded. Rafael has detailed on linux-pm the various >> problems/limitations/races between runtime PM and system PM[2], so I >> don't plan debating them again here. >> >> That being said, today we have several drivers that use runtime PM calls >> in their suspend/resume path and our PM domain implementation (inside >> omap_device) deals with most of the limitations fine. However, there >> are 2 main problems/limitation with this that we've chosen to live with >> (for now): >> >> 1) synchronous calls must be used in the suspend/resume path (because >> PM workqueue is frozen during suspend/resumea) >> 2) disabling runtime PM from userspace will prevent that device >> from hitting its low-power state during suspend. >> >> For v3.1 (and before), we've lived with these limitations, and I'm OK >> with merging other drivers for v3.1 with these limitations. After 3.1, >> this will be changing (more on this below.) >> >> So, while I've been OK with merging drivers with the above limitations >> for one more merge window, $SUBJECT patch adds a new twist by forcibly >> managing the parent device from the child device. Personally, I really >> don't like that approach and it serves as a good illustration of yet >> another reason why system PM and runtime PM need understood as >> conceptually very different. >> >> For v3.2, the PM core will change[2] to futher limit/protect >> interactions between runtime PM and system PM, and I will be reworking >> our PM domain (omap_device) implementation accordingly. >> >> Basically, what that will mean is that our PM domain layer (omap_device) >> will also call omap_device_idle() in the suspend path, but only if the >> device is *not* already idle (from previous runtime suspend.) The PM >> domain layer will then omap_device_enable() the device in the system >> resume path if it was suspended in the system suspend path. A minimally >> tested patch to do this is below. >> >> So, the driver still does not have to care about it's specific clocks >> etc. (which should address Felipe's concern), clocks and other >> IP-specific PM details will all continue to be handled by omap_device, >> just like it is with runtime PM. >> >> The primary change to the driver, is that whatever needs to be done to >> prepare for both runtime PM and system PM (including context >> save/restore etc.) will have to be done in a common function(s) that >> will be called by *both* of its ->runtime_suspend() and ->suspend() >> callbacks, and similar for ->runtime_resume() and ->resume(). >> >> Some drivers will have additional work to do for system PM though. This >> is mainly because system PM can happen at *any* time, including in the >> middle of ongoing activity, whereas runtime PM transitions happen when >> the device is known to be idle. What that means is that for example, a >> drivers ->suspend() method might need to wait for (or forcibly stop) any >> ongoing activity in order to be sure the device is ready to be suspended. >> >> Frankly, this is not a very big change for the drivers, as the >> device-specific idle work will still be handled by the PM domain layer. >> >> Hope that helps clarify the background. >> >> As for this particular patch, since it is rather late in the development >> cycle for v3.1, I would recommend that it wait until the omap_device >> changes, and then let the PM core (for system PM and runtime PM) handle >> the parent/child relationships as they are designed to. But that is up >> to Felipe and USB maintainers to decide. >> >> Kevin >> >> [1] https://lists.linux-foundation.org/pipermail/linux-pm/2011-June/031559.html >> [2] https://lists.linux-foundation.org/pipermail/linux-pm/2011-June/031977.html >> >> >> From 6696e9a2b106ca9c9936e5c2ad89650010120e10 Mon Sep 17 00:00:00 2001 >> From: Kevin Hilman <khilman@xxxxxx> >> Date: Tue, 7 Jun 2011 16:07:28 -0700 >> Subject: [PATCH] OMAP: PM: omap_device: add system PM methods for PM domain handling >> >> Using PM domain callbacks, use omap_device idle/enable to >> automatically suspend/resume devices. Also use pm_generic_* routines >> to ensure driver's callbacks are correctly called. >> >> Driver ->suspend callback is needed to ensure the driver is in a state >> that it can be suspended. >> >> If device is already idle (typically because of previous runtime PM >> activity), there's nothing extra to do. >> >> KJH: The omap_device_* calls should probably actually be done in the >> _noirq() methods. >> >> Not-yet-Signed-off-by: Kevin Hilman <khilman@xxxxxx> >> --- >> arch/arm/plat-omap/include/plat/omap_device.h | 4 +++ >> arch/arm/plat-omap/omap_device.c | 32 +++++++++++++++++++++++++ >> 2 files changed, 36 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h >> index e4c349f..bc36d05 100644 >> --- a/arch/arm/plat-omap/include/plat/omap_device.h >> +++ b/arch/arm/plat-omap/include/plat/omap_device.h >> @@ -44,6 +44,9 @@ extern struct device omap_device_parent; >> #define OMAP_DEVICE_STATE_IDLE 2 >> #define OMAP_DEVICE_STATE_SHUTDOWN 3 >> >> +/* omap_device.flags values */ >> +#define OMAP_DEVICE_SUSPENDED BIT(0) >> + >> /** >> * struct omap_device - omap_device wrapper for platform_devices >> * @pdev: platform_device >> @@ -73,6 +76,7 @@ struct omap_device { >> s8 pm_lat_level; >> u8 hwmods_cnt; >> u8 _state; >> + u8 flags; >> }; >> >> /* Device driver interface (call via platform_data fn ptrs) */ >> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c >> index 49fc0df..f2711c3 100644 >> --- a/arch/arm/plat-omap/omap_device.c >> +++ b/arch/arm/plat-omap/omap_device.c >> @@ -564,12 +564,44 @@ static int _od_runtime_resume(struct device *dev) >> return pm_generic_runtime_resume(dev); >> } >> >> +static int _od_suspend(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct omap_device *od = to_omap_device(pdev); >> + int ret; >> + >> + ret = pm_generic_suspend(dev); >> + >> + od->flags &= ~OMAP_DEVICE_SUSPENDED; >> + >> + if (od->_state == OMAP_DEVICE_STATE_ENABLED) { >> + omap_device_idle(pdev); >> + od->flags |= OMAP_DEVICE_SUSPENDED; >> + } >> + >> + return ret; >> +} >> + >> +static int _od_resume(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct omap_device *od = to_omap_device(pdev); > > seems like you guys have duplicated helpers for this. There's > _find_by_pdev() and to_omap_device and both do the exact same thing: > > static inline struct omap_device *_find_by_pdev(struct platform_device *pdev) > { > return container_of(pdev, struct omap_device, pdev); > } > > #define to_omap_device(x) container_of((x), struct omap_device, pdev) Yeah, I know. >> + >> + if ((od->flags & OMAP_DEVICE_SUSPENDED) && >> + (od->_state == OMAP_DEVICE_STATE_IDLE)) >> + omap_device_enable(pdev); >> + >> + return pm_generic_resume(dev); >> +} >> + >> static struct dev_power_domain omap_device_power_domain = { >> .ops = { >> .runtime_suspend = _od_runtime_suspend, >> .runtime_idle = _od_runtime_idle, >> .runtime_resume = _od_runtime_resume, >> USE_PLATFORM_PM_SLEEP_OPS >> + .suspend = _od_suspend, >> + .resume = _od_resume, >> } >> }; > > it all depends on when are you planning to get this patch upstream. I'm > considering getting some PM working on USB host and remove the > pm_runtime calls from system suspend/resume either during -rc or next > merge window. Well, IMO it's way too late for this kind of change for -rc, so I'm considering it for the upcoming merge window. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
