| Message ID | 8762ngtyqj.fsf@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@ti.com> > 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@ti.com> > --- > 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) > + > + 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.
Felipe Balbi <balbi@ti.com> 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@ti.com> >> 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@ti.com> >> --- >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Jul 06, 2011 at 12:20:40PM -0700, Kevin Hilman wrote: > >> + 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. yes, that's true. Who should take the hwmod patches btw ? I'm still wondering if we should patch hwmod data first and push the _correct_ PM part on 3.2.
>-----Original Message----- >From: Felipe Balbi [mailto:balbi@ti.com] >Sent: Thursday, July 07, 2011 3:48 AM >To: Kevin Hilman >Cc: balbi@ti.com; Alan Stern; Partha Basak; Keshava Munegowda; linux- >usb@vger.kernel.org; linux-omap@vger.kernel.org; linux- >kernel@vger.kernel.org; Anand Gadiyar; sameo@linux.intel.com; >parthab@india.ti.com; tony@atomide.com; Benoit Cousson; paul@pwsan.com; >johnstul@us.ibm.com; Vishwanath Sripathy >Subject: Re: [PATCH 6/6 v2] arm: omap: usb: global Suspend and resume >support of ehci and ohci > >Hi, > >On Wed, Jul 06, 2011 at 12:20:40PM -0700, Kevin Hilman wrote: >> >> + 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. > >yes, that's true. Who should take the hwmod patches btw ? I'm still >wondering if we should patch hwmod data first and push the _correct_ PM >part on 3.2. Once Kevin pushes this infrastructure enhancement, all drivers have to rework the Suspend/Resume anyways. Another way would be to go by the current approach now and then do the rework in 3.2 along with other drivers. In this way, we can soak the Runtime support of EHCI/OHCI for one merge window. > >-- >balbi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Jul 07, 2011 at 10:23:59AM +0530, Partha Basak wrote: > >-----Original Message----- > >From: Felipe Balbi [mailto:balbi@ti.com] > >Sent: Thursday, July 07, 2011 3:48 AM > >To: Kevin Hilman > >Cc: balbi@ti.com; Alan Stern; Partha Basak; Keshava Munegowda; linux- > >usb@vger.kernel.org; linux-omap@vger.kernel.org; linux- > >kernel@vger.kernel.org; Anand Gadiyar; sameo@linux.intel.com; > >parthab@india.ti.com; tony@atomide.com; Benoit Cousson; paul@pwsan.com; > >johnstul@us.ibm.com; Vishwanath Sripathy > >Subject: Re: [PATCH 6/6 v2] arm: omap: usb: global Suspend and resume > >support of ehci and ohci > > > >Hi, > > > >On Wed, Jul 06, 2011 at 12:20:40PM -0700, Kevin Hilman wrote: > >> >> + 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. > > > >yes, that's true. Who should take the hwmod patches btw ? I'm still > >wondering if we should patch hwmod data first and push the _correct_ PM > >part on 3.2. > > Once Kevin pushes this infrastructure enhancement, all drivers have to > rework > the Suspend/Resume anyways. Another way would be to go by the current > approach > now and then do the rework in 3.2 along with other drivers. > > In this way, we can soak the Runtime support of EHCI/OHCI for one merge > window. but if it's known to be broken, why add something which is broken anyway ? Tony, can you queue the arch/arm/*omap*/ patches ? I'll take care of the EHCI stuff for the next merge window, will keep them pending in my queue.
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); + + 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, } };