>-----Original Message----- >From: Felipe Balbi [mailto:balbi@xxxxxx] >Sent: Monday, July 04, 2011 1:56 PM >To: Partha Basak >Cc: Alan Stern; Keshava Munegowda; linux-usb@xxxxxxxxxxxxxxx; linux- >omap@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Felipe Balbi; Anand >Gadiyar; sameo@xxxxxxxxxxxxxxx; parthab@xxxxxxxxxxxx; tony@xxxxxxxxxxx; >Kevin Hilman; Benoit Cousson; paul@xxxxxxxxx; johnstul@xxxxxxxxxx; >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@xxxxxxxxxxxxxxxxxxx] >> >Sent: Saturday, July 02, 2011 12:37 AM >> >To: Keshava Munegowda >> >Cc: linux-usb@xxxxxxxxxxxxxxx; linux-omap@xxxxxxxxxxxxxxx; linux- >> >kernel@xxxxxxxxxxxxxxx; balbi@xxxxxx; gadiyar@xxxxxx; >> >sameo@xxxxxxxxxxxxxxx; parthab@xxxxxxxxxxxx; tony@xxxxxxxxxxx; >> >khilman@xxxxxx; b-cousson@xxxxxx; paul@xxxxxxxxx; >> >johnstul@xxxxxxxxxx; vishwanath.bs@xxxxxx >> >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@xxxxxx> >> >> >> >> 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@xxxxxx> >> >> --- >> >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
