Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros <rogerq@ti.com>]

Hi Alan,

On 07/11/2013 06:14 PM, Alan Stern wrote:
> On Thu, 11 Jul 2013, Roger Quadros wrote:
> 
>>> The other two problems are both related to the interaction between
>>> system PM and runtime PM.  Suppose the controller is already runtime
>>> suspended when the system goes to sleep.  Because it is runtime
>>> suspended, it is enabled for wakeup.  But device_may_wakeup() could
>>> return false; if this happens then you have to do a runtime-resume in
>>> omap_ehci_suspend() before calling ehci_suspend(), so that the
>>> controller can be suspended again with wakeups disabled.  (Or you could
>>> choose an alternative method for accomplishing the same result, such as
>>> disabling the wakeup signal from the pad without going through a whole
>>> EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
>>> true then you shouldn't do anything at all, because the controller is
>>> already suspended with the correct wakeup setting.
>>
>> I think this case is taken care of by the Runtime PM core at least for the OMAP
>> platform according to the documentation
>>
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647
> 
> No; that section refers only to races, not to wakeup settings.
> 
>> At the end of this mail is the log during system suspend/resume
>>
>> You can notice the following sequence
>>
>> -ehci runtime suspends
>> -system suspend triggered
>> -ehci runtime resumes
>> -ehci suspends (uses new wakeup settings)
>> -system wakeup triggered
>> -ehci resumes
>> -ehci runtime suspends
> 
> This is because the root hub was runtime suspended with the wrong
> wakeup setting.  The USB core, which is careful about these things,
> resumed and re-suspended it with the proper wakeup setting.  In the 
> process, the controller had to be runtime resumed as well.
> 
> Try doing the test over again, but this time with the root hub enabled 
> for wakeup and the controller disabled.  (I know this is a bizarre 
> combination, but try it anyway.)  Also, after the system wakes up, see 
> whether the root hub and controller get runtime suspended.
> 

The first part of the test caught the problem were we were trying to access
EHCI registers when HW is not accessible. So this was a good test case.

>>> For the third problem, suppose the controller was runtime suspended
>>> when the system went to sleep.  When the system wakes up, the
>>> controller will become active, so you have to inform the runtime PM
>>> core about its change of state.  Basically, if omap_ehci_resume() sees
>>> that ehci_resume() returned 0 then it must do:
>>>
>>> 	pm_runtime_disable(dev);
>>> 	pm_runtime_set_active(dev);
>>> 	pm_runtime_enable(dev);
>>>
>>> All of these issues are discussed (among lots of other material) in 
>>> Documentation/power/runtime_pm.txt.
>>
>> Is this still applicable? Documentation claims
>>
>>    "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
>>     for every device right after executing the subsystem-level .resume_early()
>>     callback and right after executing the subsystem-level .resume() callback
>>     for it, respectively."
> 
> Yes, this is applicable, but it is irrelevant to the problem I 
> described.  You still have to tell the runtime PM core that the device 
> is now active.

Right, I understand it now. How does the below code look?

+static int omap_ehci_suspend(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       bool do_wakeup = device_may_wakeup(dev);
+       int ret;
+
+       dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
+
+       if (pm_runtime_status_suspended(dev)) {
+               pm_runtime_get_sync(dev);
+               ehci_resume(hcd, false);
+               ret = ehci_suspend(hcd, do_wakeup);
+               pm_runtime_put_sync(dev);
+
+       } else {
+               ret = ehci_suspend(hcd, do_wakeup);
+       }
+
+       return ret;
+}
+
+static int omap_ehci_resume(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       int ret;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       ret = ehci_resume(hcd, false);
+       if (!ret) {
+               /*
+                * Controller was powered ON so reflect state
+                */
+               pm_runtime_disable(dev);
+               pm_runtime_set_active(dev);
+               pm_runtime_enable(dev);
+       }
+
+       return ret;
+}
+
+static int omap_ehci_runtime_suspend(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       if (omap->bound)
+               ehci_suspend(hcd, true);
+
+       return 0;
+}
+
+static int omap_ehci_runtime_resume(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       if (omap->bound)
+               ehci_resume(hcd, false);
+
+       return 0;
+}


cheers,
-roger