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

[Felipe Balbi]

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.

-- 
balbi

Attachment: signature.asc
Description: Digital signature