| Message ID | 1309546474-15363-7-git-send-email-keshava_mgowda@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 2 Jul 2011, Keshava Munegowda wrote: > From: Keshava Munegowda <Keshava_mgowda@ti.com> > > The global suspend and resume functions for ehci and ohci > drivers are implemented; these functions does the > pm_runtime_get_sync and pm_runtime_put_sync of the > parent device usbhs core driver respectively. > > Signed-off-by: Keshava Munegowda <keshava_mgowda@ti.com> > --- > drivers/usb/host/ehci-omap.c | 22 ++++++++++++++++++++-- > drivers/usb/host/ohci-omap3.c | 21 +++++++++++++++++++++ > 2 files changed, 41 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c > index 178f63e..a02a684 100644 > --- a/drivers/usb/host/ehci-omap.c > +++ b/drivers/usb/host/ehci-omap.c > @@ -259,14 +259,32 @@ static void ehci_hcd_omap_shutdown(struct platform_device *pdev) > hcd->driver->shutdown(hcd); > } > > +static int omap_ehci_resume(struct device *dev) > +{ > + if (dev->parent) > + pm_runtime_get_sync(dev->parent); > + return 0; > +} > + > +static int omap_ehci_suspend(struct device *dev) > +{ > + if (dev->parent) > + pm_runtime_put_sync(dev->parent); > + return 0; > +} I don't see any point in these routines (and likewise for omap_ohci_suspend/resume). When the whole system is going to sleep anyway, what reason is there for enabling runtime PM on the parent device? Alan Stern -- 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
>-----Original Message----- >From: Alan Stern [mailto:stern@rowland.harvard.edu] >Sent: Saturday, July 02, 2011 12:37 AM >To: Keshava Munegowda >Cc: linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; linux- >kernel@vger.kernel.org; balbi@ti.com; gadiyar@ti.com; >sameo@linux.intel.com; parthab@india.ti.com; tony@atomide.com; >khilman@ti.com; b-cousson@ti.com; paul@pwsan.com; johnstul@us.ibm.com; >vishwanath.bs@ti.com >Subject: Re: [PATCH 6/6 v2] arm: omap: usb: global Suspend and resume >support of ehci and ohci > >On Sat, 2 Jul 2011, Keshava Munegowda wrote: > >> From: Keshava Munegowda <Keshava_mgowda@ti.com> >> >> The global suspend and resume functions for ehci and ohci >> drivers are implemented; these functions does the >> pm_runtime_get_sync and pm_runtime_put_sync of the >> parent device usbhs core driver respectively. >> >> Signed-off-by: Keshava Munegowda <keshava_mgowda@ti.com> >> --- >> drivers/usb/host/ehci-omap.c | 22 ++++++++++++++++++++-- >> drivers/usb/host/ohci-omap3.c | 21 +++++++++++++++++++++ >> 2 files changed, 41 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci- >omap.c >> index 178f63e..a02a684 100644 >> --- a/drivers/usb/host/ehci-omap.c >> +++ b/drivers/usb/host/ehci-omap.c >> @@ -259,14 +259,32 @@ static void ehci_hcd_omap_shutdown(struct >platform_device *pdev) >> hcd->driver->shutdown(hcd); >> } >> >> +static int omap_ehci_resume(struct device *dev) >> +{ >> + if (dev->parent) >> + pm_runtime_get_sync(dev->parent); >> + return 0; >> +} >> + >> +static int omap_ehci_suspend(struct device *dev) >> +{ >> + if (dev->parent) >> + pm_runtime_put_sync(dev->parent); >> + return 0; >> +} > >I don't see any point in these routines (and likewise for >omap_ohci_suspend/resume). When the whole system is going to sleep >anyway, what reason is there for enabling runtime PM on the parent >device? Both for EHCI & OHCI, the clocks are owned by the parent (uhh-tll). Calling pm_runtime_put_sync(dev->parent) within omap_ehci_suspend will turn-off the parent clocks in the Suspend path. Similarly, calling pm_runtime_get_sync(dev->parent) within omap_ehci_resume will turn-on the parent clocks in the resume path. This way, all reference counting are implicit within the Runtime PM layer and takes care of all combinations of only EHCI insmoded, OHCI insmoded, both insmoded etc. When both EHCI & OHCI are suspended, parent clocks will actually be turned OFF and vice-versa. Note that the parent per-se does not have any .suspend & .resume hooked up. At the end of the _probe of parent, the clocks are turned OFF. Subsequently, enabling the parent clocks are entirely done implicitly by the children get_sync() in their _probe. Therefore while .suspend/.resume of children are called they call back into the parent to turn-off the clocks. FYI, Keshava is out of office due to some emergency this week, there will be some delays in replying from his side. > >Alan Stern -- 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 Mon, Jul 04, 2011 at 10:36:54AM +0530, Partha Basak wrote: > >-----Original Message----- > >From: Alan Stern [mailto:stern@rowland.harvard.edu] > >Sent: Saturday, July 02, 2011 12:37 AM > >To: Keshava Munegowda > >Cc: linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; linux- > >kernel@vger.kernel.org; balbi@ti.com; gadiyar@ti.com; > >sameo@linux.intel.com; parthab@india.ti.com; tony@atomide.com; > >khilman@ti.com; b-cousson@ti.com; paul@pwsan.com; johnstul@us.ibm.com; > >vishwanath.bs@ti.com > >Subject: Re: [PATCH 6/6 v2] arm: omap: usb: global Suspend and resume > >support of ehci and ohci > > > >On Sat, 2 Jul 2011, Keshava Munegowda wrote: > > > >> From: Keshava Munegowda <Keshava_mgowda@ti.com> > >> > >> The global suspend and resume functions for ehci and ohci > >> drivers are implemented; these functions does the > >> pm_runtime_get_sync and pm_runtime_put_sync of the > >> parent device usbhs core driver respectively. > >> > >> Signed-off-by: Keshava Munegowda <keshava_mgowda@ti.com> > >> --- > >> drivers/usb/host/ehci-omap.c | 22 ++++++++++++++++++++-- > >> drivers/usb/host/ohci-omap3.c | 21 +++++++++++++++++++++ > >> 2 files changed, 41 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci- > >omap.c > >> index 178f63e..a02a684 100644 > >> --- a/drivers/usb/host/ehci-omap.c > >> +++ b/drivers/usb/host/ehci-omap.c > >> @@ -259,14 +259,32 @@ static void ehci_hcd_omap_shutdown(struct > >platform_device *pdev) > >> hcd->driver->shutdown(hcd); > >> } > >> > >> +static int omap_ehci_resume(struct device *dev) > >> +{ > >> + if (dev->parent) > >> + pm_runtime_get_sync(dev->parent); > >> + return 0; > >> +} > >> + > >> +static int omap_ehci_suspend(struct device *dev) > >> +{ > >> + if (dev->parent) > >> + pm_runtime_put_sync(dev->parent); > >> + return 0; > >> +} > > > >I don't see any point in these routines (and likewise for > >omap_ohci_suspend/resume). When the whole system is going to sleep > >anyway, what reason is there for enabling runtime PM on the parent > >device? > > Both for EHCI & OHCI, the clocks are owned by the parent (uhh-tll). > > Calling pm_runtime_put_sync(dev->parent) within omap_ehci_suspend > will turn-off the parent clocks in the Suspend path. > > Similarly, calling pm_runtime_get_sync(dev->parent) within > omap_ehci_resume > will turn-on the parent clocks in the resume path. > > This way, all reference counting are implicit within the Runtime PM layer > and takes care of all combinations of only EHCI insmoded, OHCI insmoded, > both insmoded etc. > > When both EHCI & OHCI are suspended, parent clocks will actually be > turned OFF and vice-versa. not sure this is necessary. I would expect: pm_runtime_get_sync(dev) to propagate up the parent tree and enable all necessary resources to get the child in a working state. IOW, you shouldn't need to manuall access the parent device. Kevin ? Paul ?
>-----Original Message----- >From: Felipe Balbi [mailto:balbi@ti.com] >Sent: Monday, July 04, 2011 1:56 PM >To: Partha Basak >Cc: Alan Stern; Keshava Munegowda; linux-usb@vger.kernel.org; linux- >omap@vger.kernel.org; linux-kernel@vger.kernel.org; Felipe Balbi; Anand >Gadiyar; sameo@linux.intel.com; parthab@india.ti.com; tony@atomide.com; >Kevin Hilman; 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 Mon, Jul 04, 2011 at 10:36:54AM +0530, Partha Basak wrote: >> >-----Original Message----- >> >From: Alan Stern [mailto:stern@rowland.harvard.edu] >> >Sent: Saturday, July 02, 2011 12:37 AM >> >To: Keshava Munegowda >> >Cc: linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; linux- >> >kernel@vger.kernel.org; balbi@ti.com; gadiyar@ti.com; >> >sameo@linux.intel.com; parthab@india.ti.com; tony@atomide.com; >> >khilman@ti.com; b-cousson@ti.com; paul@pwsan.com; >> >johnstul@us.ibm.com; vishwanath.bs@ti.com >> >Subject: Re: [PATCH 6/6 v2] arm: omap: usb: global Suspend and resume >> >support of ehci and ohci >> > >> >On Sat, 2 Jul 2011, Keshava Munegowda wrote: >> > >> >> From: Keshava Munegowda <Keshava_mgowda@ti.com> >> >> >> >> The global suspend and resume functions for ehci and ohci drivers >> >> are implemented; these functions does the pm_runtime_get_sync and >> >> pm_runtime_put_sync of the parent device usbhs core driver >> >> respectively. >> >> >> >> Signed-off-by: Keshava Munegowda <keshava_mgowda@ti.com> >> >> --- >> >> drivers/usb/host/ehci-omap.c | 22 ++++++++++++++++++++-- >> >> drivers/usb/host/ohci-omap3.c | 21 +++++++++++++++++++++ >> >> 2 files changed, 41 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci- >> >omap.c >> >> index 178f63e..a02a684 100644 >> >> --- a/drivers/usb/host/ehci-omap.c >> >> +++ b/drivers/usb/host/ehci-omap.c >> >> @@ -259,14 +259,32 @@ static void ehci_hcd_omap_shutdown(struct >> >platform_device *pdev) >> >> hcd->driver->shutdown(hcd); >> >> } >> >> >> >> +static int omap_ehci_resume(struct device *dev) { >> >> + if (dev->parent) >> >> + pm_runtime_get_sync(dev->parent); >> >> + return 0; >> >> +} >> >> + >> >> +static int omap_ehci_suspend(struct device *dev) { >> >> + if (dev->parent) >> >> + pm_runtime_put_sync(dev->parent); >> >> + return 0; >> >> +} >> > >> >I don't see any point in these routines (and likewise for >> >omap_ohci_suspend/resume). When the whole system is going to sleep >> >anyway, what reason is there for enabling runtime PM on the parent >> >device? >> >> Both for EHCI & OHCI, the clocks are owned by the parent (uhh-tll). >> >> Calling pm_runtime_put_sync(dev->parent) within omap_ehci_suspend will >> turn-off the parent clocks in the Suspend path. >> >> Similarly, calling pm_runtime_get_sync(dev->parent) within >> omap_ehci_resume will turn-on the parent clocks in the resume path. >> >> This way, all reference counting are implicit within the Runtime PM >> layer and takes care of all combinations of only EHCI insmoded, OHCI >> insmoded, both insmoded etc. >> >> When both EHCI & OHCI are suspended, parent clocks will actually be >> turned OFF and vice-versa. > >not sure this is necessary. I would expect: > >pm_runtime_get_sync(dev) to propagate up the parent tree and enable all >necessary resources to get the child in a working state. IOW, you >shouldn't need to manuall access the parent device. > Refer to the description in Patch(5/6) <snip> In fact, the runtime framework takes care the get sync and put sync of the child in turn call the get sync and put sync of parent too; but calling get sync and put sync of parent is by ASYNC mode; This mode queues the work item in runtime pm work queue, which not getting scheduled in case of global suspend path. <snip> This approach was tried, but did not work in the Suspend path static int rpm_suspend(struct device *dev, int rpmflags) __releases(&dev->power.lock) __acquires(&dev->power.lock) { . . . no_callback: . . . /* Maybe the parent is now able to suspend. */ if (parent && !parent->power.ignore_children && !dev->power.irq_safe) { spin_unlock(&dev->power.lock); spin_lock(&parent->power.lock); rpm_idle(parent, RPM_ASYNC); spin_unlock(&parent->power.lock); spin_lock(&dev->power.lock); } This is the reason of directly calling the parent Runtime PM calls from the children. If directly calling Runtime PM APIs with parent dev-pointer isn't acceptable, this can be achieved by exporting wrapper APIs from the parent and calling them from the chidren .suspend/.resume routines. >Kevin ? Paul ? > >-- >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 Mon, Jul 04, 2011 at 02:56:30PM +0530, Partha Basak wrote: > >> Both for EHCI & OHCI, the clocks are owned by the parent (uhh-tll). > >> > >> Calling pm_runtime_put_sync(dev->parent) within omap_ehci_suspend will > >> turn-off the parent clocks in the Suspend path. > >> > >> Similarly, calling pm_runtime_get_sync(dev->parent) within > >> omap_ehci_resume will turn-on the parent clocks in the resume path. > >> > >> This way, all reference counting are implicit within the Runtime PM > >> layer and takes care of all combinations of only EHCI insmoded, OHCI > >> insmoded, both insmoded etc. > >> > >> When both EHCI & OHCI are suspended, parent clocks will actually be > >> turned OFF and vice-versa. > > > >not sure this is necessary. I would expect: > > > >pm_runtime_get_sync(dev) to propagate up the parent tree and enable all > >necessary resources to get the child in a working state. IOW, you > >shouldn't need to manuall access the parent device. > > > Refer to the description in Patch(5/6) > <snip> > In fact, the runtime framework takes care the get sync and put sync of the > child > in turn call the get sync and put sync of parent too; but calling get sync > and > put sync of parent is by ASYNC mode; > This mode queues the work item in runtime pm work queue, > which not getting scheduled in case of global suspend path. > <snip> > This approach was tried, but did not work in the Suspend path sounds to me like a bug on pm runtime ? If you're calling pm_runtime_*_sync() family, shouldn't all calls be _sync() too ? > static int rpm_suspend(struct device *dev, int rpmflags) > __releases(&dev->power.lock) __acquires(&dev->power.lock) > { > . > . > . > no_callback: > . > . > . > /* Maybe the parent is now able to suspend. */ > if (parent && !parent->power.ignore_children && > !dev->power.irq_safe) { > spin_unlock(&dev->power.lock); > > spin_lock(&parent->power.lock); > rpm_idle(parent, RPM_ASYNC); to me this is bogus, if you called pm_runtime_put_sync() should should be sync too. Shouldn't it ? > spin_unlock(&parent->power.lock); > > spin_lock(&dev->power.lock); > } > This is the reason of directly calling the parent Runtime PM calls from > the children. > If directly calling Runtime PM APIs with parent dev-pointer isn't > acceptable, > this can be achieved by exporting wrapper APIs from the > parent and calling them from the chidren .suspend/.resume routines. Still no good, IMHO.
On Mon, 4 Jul 2011, Partha Basak wrote: > >I don't see any point in these routines (and likewise for > >omap_ohci_suspend/resume). When the whole system is going to sleep > >anyway, what reason is there for enabling runtime PM on the parent > >device? > > Both for EHCI & OHCI, the clocks are owned by the parent (uhh-tll). > > Calling pm_runtime_put_sync(dev->parent) within omap_ehci_suspend > will turn-off the parent clocks in the Suspend path. > > Similarly, calling pm_runtime_get_sync(dev->parent) within > omap_ehci_resume > will turn-on the parent clocks in the resume path. > > This way, all reference counting are implicit within the Runtime PM layer > and takes care of all combinations of only EHCI insmoded, OHCI insmoded, > both insmoded etc. > > When both EHCI & OHCI are suspended, parent clocks will actually be > turned OFF and vice-versa. > > Note that the parent per-se does not have any .suspend & .resume hooked > up. Why not? That sounds like a big bug. > At the end of the _probe of parent, the clocks are turned OFF. > Subsequently, enabling > the parent clocks are entirely done implicitly by the children get_sync() > in their _probe. > > Therefore while .suspend/.resume of children are called they call back > into the parent to turn-off the clocks. You have ignored a few very important points: Firstly, system suspend is supposed to work even when runtime PM is not configured. Secondly, the user can disable runtime PM via sysfs at any time. This shouldn't mess up system suspend. Basically, it's a bad idea to mix up system suspend with runtime PM. Alan Stern -- 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 Mon, Jul 04, 2011 at 12:01:24PM -0400, Alan Stern wrote: > On Mon, 4 Jul 2011, Felipe Balbi wrote: > > > sounds to me like a bug on pm runtime ? If you're calling > > pm_runtime_*_sync() family, shouldn't all calls be _sync() too ? > > No. This was a deliberate design decision. It minimizes stack usage > and it gives a chance for some other child to resume before the parent > is powered down. fair enough. > > > spin_unlock(&parent->power.lock); > > > > > > spin_lock(&dev->power.lock); > > > } > > > This is the reason of directly calling the parent Runtime PM calls from > > > the children. > > > If directly calling Runtime PM APIs with parent dev-pointer isn't > > > acceptable, > > > this can be achieved by exporting wrapper APIs from the > > > parent and calling them from the chidren .suspend/.resume routines. > > > > Still no good, IMHO. > > The real problem here is that you guys are trying to use the runtime PM > framework to carry out activities during system suspend. That won't > work; it's just a bad idea all round. Use the proper callbacks to do > what you want. then what's the point in even having runtime PM if we will still have to implement the same functionality on the other callbacks ? Well, of course runtime PM will conserve power on runtime, but system suspend should be no different other than an "always deepest sleep state" decision. The thing now is that pm_runtime was done so that drivers would stop caring about clocks, which is a big plus, but if we still have to handle ->suspend()/->resume() differently, we will still need to clk_get(); clk_enable(); clk_disable(); Then what was the big deal with runtime PM? IMHO, we should have only one PM layer. System suspend/resume should be implemented so that core PM "forcefully" calls ->runtime_suspend()/->runtime_resume() of call drivers, all synchronously. Maybe we need an extra RPM_STATIC_SUSPEND_PLEASE_HANDLE_IT_ALL_SYNCHRONOUSLY flag, but that's another detail. If drivers are really supposed to stop handling clocks directly, then runtime PM is THE framework to do that, but if we still have system suspend/resume the old way, I don't see the benefit for the driver (other than the uAmps saved during runtime, which is great, don't get me wrong ;-) that this will bring. Having two PM layers which, in fact, are doing the same thing - reducing power consumption - is just too much IMO.
>-----Original Message----- >From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- >owner@vger.kernel.org] On Behalf Of Alan Stern >Sent: Monday, July 04, 2011 9:21 PM >To: Partha Basak >Cc: Keshava Munegowda; linux-usb@vger.kernel.org; linux- >omap@vger.kernel.org; linux-kernel@vger.kernel.org; Felipe Balbi; Anand >Gadiyar; sameo@linux.intel.com; parthab@india.ti.com; tony@atomide.com; >Kevin Hilman; 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 > >On Mon, 4 Jul 2011, Partha Basak wrote: > >> >I don't see any point in these routines (and likewise for >> >omap_ohci_suspend/resume). When the whole system is going to sleep >> >anyway, what reason is there for enabling runtime PM on the parent >> >device? >> >> Both for EHCI & OHCI, the clocks are owned by the parent (uhh-tll). >> >> Calling pm_runtime_put_sync(dev->parent) within omap_ehci_suspend >> will turn-off the parent clocks in the Suspend path. >> >> Similarly, calling pm_runtime_get_sync(dev->parent) within >> omap_ehci_resume >> will turn-on the parent clocks in the resume path. >> >> This way, all reference counting are implicit within the Runtime PM >layer >> and takes care of all combinations of only EHCI insmoded, OHCI >insmoded, >> both insmoded etc. >> >> When both EHCI & OHCI are suspended, parent clocks will actually be >> turned OFF and vice-versa. >> >> Note that the parent per-se does not have any .suspend & .resume >hooked >> up. > >Why not? That sounds like a big bug. This was a design decision since the parent needs to be activated only when at-least one child is insmoded. If the chidren are suspended, automatically the parent is suspended via the pm_runtime_putsync calls to the parent. So, effectively, we do not need an explicit suspend for the parent. > >> At the end of the _probe of parent, the clocks are turned OFF. >> Subsequently, enabling >> the parent clocks are entirely done implicitly by the children >get_sync() >> in their _probe. >> >> Therefore while .suspend/.resume of children are called they call back >> into the parent to turn-off the clocks. > >You have ignored a few very important points: > >Firstly, system suspend is supposed to work even when runtime PM is not >configured. > >Secondly, the user can disable runtime PM via sysfs at any time. This >shouldn't mess up system suspend. > >Basically, it's a bad idea to mix up system suspend with runtime PM. Your observations are correct but this is a generic limitation and Kevin is working on this problem in parallel. As of now, all OMAP drivers are mandated to use ONLY runtime pm framework for enabling/disabling clocks. I will let Kevin comment further. > >Alan Stern > >-- >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 -- 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
On Tue, 5 Jul 2011, Felipe Balbi wrote: > > The real problem here is that you guys are trying to use the runtime PM > > framework to carry out activities during system suspend. That won't > > work; it's just a bad idea all round. Use the proper callbacks to do > > what you want. > > then what's the point in even having runtime PM if we will still have to > implement the same functionality on the other callbacks ? You don't have to duplicate the functionality. You can use exactly the same functions for both sets of callbacks if you want; just make sure the callbacks point to them. > Well, of > course runtime PM will conserve power on runtime, but system suspend > should be no different other than an "always deepest sleep state" > decision. No, it is significantly different for several reasons. Some of the most important differences are concerned with freezing userspace and deciding what events should be allowed to wake up the system. Also, there are systems which can achieve greater power savings by system sleep than they can by runtime PM + cpuidle. > The thing now is that pm_runtime was done so that drivers would stop > caring about clocks, which is a big plus, but if we still have to handle > ->suspend()/->resume() differently, we will still need to clk_get(); > clk_enable(); clk_disable(); Then what was the big deal with runtime PM? I don't know about that. Clock usage has always been internal to the implementation you guys have been working on, and I haven't followed it. If your implementation was designed incorrectly, well, that's a shame but it's understandable. Things like that happen. It shouldn't be too hard to fix. But first you do need to understand that system suspend really _is_ different from runtime suspend. Sure, you may be able to share some code between them, but you should not expect to be able to use one in place of the other. > IMHO, we should have only one PM layer. System suspend/resume should be > implemented so that core PM "forcefully" calls > ->runtime_suspend()/->runtime_resume() of call drivers, all > synchronously. Maybe we need an extra > RPM_STATIC_SUSPEND_PLEASE_HANDLE_IT_ALL_SYNCHRONOUSLY flag, but that's > another detail. Statements like this should be posted to linux-pm where they can be discussed properly. It certainly isn't fair to make such claims without even CC-ing the PM maintainer. Besides, handling runtime PM synchronously won't do you any good if the user has disabled runtime PM via sysfs or not enabled CONFIG_PM_RUNTIME in the first place. Have you forgotten about those possibilities? Furthermore, from what I've gathered so far from this thread, the _real_ problem is that nobody has written suspend and resume callbacks for the parent device. You're relying on runtime PM to do things with the parent, but instead you should make use of the usual system sleep mechanism: Parents are always suspended after their children and awakened before. Have the parent's suspend routine disable the clocks and have the resume routine enable them. Problem solved, no changes needed in the child's driver code. > If drivers are really supposed to stop handling clocks directly, then > runtime PM is THE framework to do that, but if we still have system > suspend/resume the old way, I don't see the benefit for the driver > (other than the uAmps saved during runtime, which is great, don't get me > wrong ;-) that this will bring. Having two PM layers which, in fact, are > doing the same thing - reducing power consumption - is just too much > IMO. They aren't doing the same thing. If you don't believe me, ask the PM maintainer. And if you actually do need your callbacks to do the same thing, simply use shared subroutines. Alan Stern -- 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 Tue, Jul 05, 2011 at 10:17:14AM -0400, Alan Stern wrote: > On Tue, 5 Jul 2011, Felipe Balbi wrote: > > > > The real problem here is that you guys are trying to use the runtime PM > > > framework to carry out activities during system suspend. That won't > > > work; it's just a bad idea all round. Use the proper callbacks to do > > > what you want. > > > > then what's the point in even having runtime PM if we will still have to > > implement the same functionality on the other callbacks ? > > You don't have to duplicate the functionality. You can use exactly the > same functions for both sets of callbacks if you want; just make sure > the callbacks point to them. true, good point. > > Well, of > > course runtime PM will conserve power on runtime, but system suspend > > should be no different other than an "always deepest sleep state" > > decision. > > No, it is significantly different for several reasons. Some of the > most important differences are concerned with freezing userspace and > deciding what events should be allowed to wake up the system. Also, > there are systems which can achieve greater power savings by system > sleep than they can by runtime PM + cpuidle. I remember we've been through this discussion before and it's just nonsensical to make such statement. What does freezing userspace have to do with power consumption ? If you can't reach lower power consumption with runtime PM it only means userspace is waking the system too much. > > The thing now is that pm_runtime was done so that drivers would stop > > caring about clocks, which is a big plus, but if we still have to handle > > ->suspend()/->resume() differently, we will still need to clk_get(); > > clk_enable(); clk_disable(); Then what was the big deal with runtime PM? > > I don't know about that. Clock usage has always been internal to the > implementation you guys have been working on, and I haven't followed > it. If your implementation was designed incorrectly, well, that's a > shame but it's understandable. Things like that happen. It shouldn't > be too hard to fix. > > But first you do need to understand that system suspend really _is_ > different from runtime suspend. Sure, you may be able to share some > code between them, but you should not expect to be able to use one in > place of the other. I really fail to see why not, and maybe it's only my fault and I need to read the Documentation/ more carefully :-s > > IMHO, we should have only one PM layer. System suspend/resume should be > > implemented so that core PM "forcefully" calls > > ->runtime_suspend()/->runtime_resume() of call drivers, all > > synchronously. Maybe we need an extra > > RPM_STATIC_SUSPEND_PLEASE_HANDLE_IT_ALL_SYNCHRONOUSLY flag, but that's > > another detail. > > Statements like this should be posted to linux-pm where they can be > discussed properly. It certainly isn't fair to make such claims > without even CC-ing the PM maintainer. > > Besides, handling runtime PM synchronously won't do you any good if the > user has disabled runtime PM via sysfs or not enabled > CONFIG_PM_RUNTIME in the first place. Have you forgotten about those > possibilities? I thought that the "we should have only one PM layer" already carried the idea that CONFIG_PM and CONFIG_PM_RUNTIME would be combined into one, and sysfs would need a little re-factoring... > Furthermore, from what I've gathered so far from this thread, the > _real_ problem is that nobody has written suspend and resume callbacks > for the parent device. You're relying on runtime PM to do things with > the parent, but instead you should make use of the usual system sleep > mechanism: Parents are always suspended after their children and > awakened before. Have the parent's suspend routine disable the clocks > and have the resume routine enable them. Problem solved, no changes > needed in the child's driver code. that's currently hidden on the omap rutime pm support. No driver is to talk to clk API directly anymore. Granted, now that I read what I just wrote it does sound like it's a limitation, although it's really nice not to have to remember all the numerous clocks needed for a particular device to work properly. So, if there would be a way, other than pm_runtime_resume(), to enable all clocks a particular device has without really having to clk_get(); clk_enable() each one of them, fine, this would be solved. But as of today, we only have pm_runtime_resume() to achieve that, unless I'm missing something.
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c index 178f63e..a02a684 100644 --- a/drivers/usb/host/ehci-omap.c +++ b/drivers/usb/host/ehci-omap.c @@ -259,14 +259,32 @@ static void ehci_hcd_omap_shutdown(struct platform_device *pdev) hcd->driver->shutdown(hcd); } +static int omap_ehci_resume(struct device *dev) +{ + if (dev->parent) + pm_runtime_get_sync(dev->parent); + return 0; +} + +static int omap_ehci_suspend(struct device *dev) +{ + if (dev->parent) + pm_runtime_put_sync(dev->parent); + return 0; +} + +static const struct dev_pm_ops omap_ehci_dev_pm_ops = { + .suspend = omap_ehci_suspend, + .resume = omap_ehci_resume, +}; + static struct platform_driver ehci_hcd_omap_driver = { .probe = ehci_hcd_omap_probe, .remove = ehci_hcd_omap_remove, .shutdown = ehci_hcd_omap_shutdown, - /*.suspend = ehci_hcd_omap_suspend, */ - /*.resume = ehci_hcd_omap_resume, */ .driver = { .name = "ehci-omap", + .pm = &omap_ehci_dev_pm_ops, } }; diff --git a/drivers/usb/host/ohci-omap3.c b/drivers/usb/host/ohci-omap3.c index 6ce50de..a1a7981 100644 --- a/drivers/usb/host/ohci-omap3.c +++ b/drivers/usb/host/ohci-omap3.c @@ -230,12 +230,33 @@ static void ohci_hcd_omap3_shutdown(struct platform_device *pdev) hcd->driver->shutdown(hcd); } + +static int omap_ohci_resume(struct device *dev) +{ + if (dev->parent) + pm_runtime_get_sync(dev->parent); + return 0; +} + +static int omap_ohci_suspend(struct device *dev) +{ + if (dev->parent) + pm_runtime_put_sync(dev->parent); + return 0; +} + +static const struct dev_pm_ops omap_ohci_dev_pm_ops = { + .suspend = omap_ohci_suspend, + .resume = omap_ohci_resume, +}; + static struct platform_driver ohci_hcd_omap3_driver = { .probe = ohci_hcd_omap3_probe, .remove = __devexit_p(ohci_hcd_omap3_remove), .shutdown = ohci_hcd_omap3_shutdown, .driver = { .name = "ohci-omap3", + .pm = &omap_ohci_dev_pm_ops, }, };