Re: [PATCH 6/6 v2] arm: omap: usb: global Suspend and resume supportof ehci and ohci

[Felipe Balbi]

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.

Kevin ? Paul ?

-- 
balbi

Attachment: signature.asc
Description: Digital signature