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

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2484 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]