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

[Kevin Hilman]

Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:

[...]

>>>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.
>
> Okay, let's see what Kevin says.

While I did design the OMAP PM core to be runtime PM centric, and we
implemented several drivers based on runtime PM alone, after some long
discussions on linux-pm[1] with Alan and Rafael (PM maintainer) over the
last couple weeks, I'm now convinced I had the wrong design/approach.

Rafael and Alan have been patient with my stubborness, but now I've been
pursuaded.  Rafael has detailed on linux-pm the various
problems/limitations/races between runtime PM and system PM[2], so I
don't plan debating them again here.

That being said, today we have several drivers that use runtime PM calls
in their suspend/resume path and our PM domain implementation (inside
omap_device) deals with most of the limitations fine.  However, there
are 2 main problems/limitation with this that we've chosen to live with
(for now):
        
1) synchronous calls must be used in the suspend/resume path (because
   PM workqueue is frozen during suspend/resumea)
2) disabling runtime PM from userspace will prevent that device
   from hitting its low-power state during suspend.

For v3.1 (and before), we've lived with these limitations, and I'm OK
with merging other drivers for v3.1 with these limitations.  After 3.1,
this will be changing (more on this below.)  

So, while I've been OK with merging drivers with the above limitations
for one more merge window, $SUBJECT patch adds a new twist by forcibly
managing the parent device from the child device.  Personally, I really
don't like that approach and it serves as a good illustration of yet
another reason why system PM and runtime PM need understood as
conceptually very different.

For v3.2, the PM core will change[2] to futher limit/protect
interactions between runtime PM and system PM, and I will be reworking
our PM domain (omap_device) implementation accordingly.  

Basically, what that will mean is that our PM domain layer (omap_device)
will also call omap_device_idle() in the suspend path, but only if the
device is *not* already idle (from previous runtime suspend.)  The PM
domain layer will then omap_device_enable() the device in the system
resume path if it was suspended in the system suspend path.  A minimally
tested patch to do this is below.

So, the driver still does not have to care about it's specific clocks
etc. (which should address Felipe's concern), clocks and other
IP-specific PM details will all continue to be handled by omap_device,
just like it is with runtime PM.

The primary change to the driver, is that whatever needs to be done to
prepare for both runtime PM and system PM (including context
save/restore etc.) will have to be done in a common function(s) that
will be called by *both* of its ->runtime_suspend() and ->suspend()
callbacks, and similar for ->runtime_resume() and ->resume().  

Some drivers will have additional work to do for system PM though.  This
is mainly because system PM can happen at *any* time, including in the
middle of ongoing activity, whereas runtime PM transitions happen when
the device is known to be idle.  What that means is that for example, a
drivers ->suspend() method might need to wait for (or forcibly stop) any
ongoing activity in order to be sure the device is ready to be suspended.

Frankly, this is not a very big change for the drivers, as the
device-specific idle work will still be handled by the PM domain layer.

Hope that helps clarify the background.

As for this particular patch, since it is rather late in the development
cycle for v3.1, I would recommend that it wait until the omap_device
changes, and then let the PM core (for system PM and runtime PM) handle
the parent/child relationships as they are designed to.   But that is up
to Felipe and USB maintainers to decide.

Kevin

[1] https://lists.linux-foundation.org/pipermail/linux-pm/2011-June/031559.html
[2] https://lists.linux-foundation.org/pipermail/linux-pm/2011-June/031977.html