lkml.org 
[lkml]   [2011]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRE: [PATCH 6/6 v2] arm: omap: usb: global Suspend and resume support of ehci and ohci
    >-----Original Message-----
    >From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
    >owner@vger.kernel.org] On Behalf Of Felipe Balbi
    >Sent: Monday, July 04, 2011 3:01 PM
    >To: Partha Basak
    >Cc: balbi@ti.com; Alan Stern; Keshava Munegowda; linux-
    >usb@vger.kernel.org; linux-omap@vger.kernel.org; linux-
    >kernel@vger.kernel.org; Anand Gadiyar; sameo@linux.intel.com;
    >parthab@india.ti.com; tony@atomide.com; Kevin Hilman; Benoit Cousson;
    >paul@pwsan.com; johnstul@us.ibm.com; Vishwanath Sripathy
    >Subject: Re: [PATCH 6/6 v2] arm: omap: usb: global Suspend and resume
    >support of ehci and ohci
    >
    >Hi,
    >
    >On Mon, Jul 04, 2011 at 02:56:30PM +0530, Partha Basak wrote:
    >> >> 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.
    >> >
    >> Refer to the description in Patch(5/6) <snip> In fact, the runtime
    >> framework takes care the get sync and put sync of the child in turn
    >> call the get sync and put sync of parent too; but calling get sync and
    >> put sync of parent is by ASYNC mode; This mode queues the work item in
    >> runtime pm work queue, which not getting scheduled in case of global
    >> suspend path.
    >> <snip>
    >> This approach was tried, but did not work in the Suspend path
    >
    >sounds to me like a bug on pm runtime ? If you're calling
    >pm_runtime_*_sync() family, shouldn't all calls be _sync() too ?
    >
    >> static int rpm_suspend(struct device *dev, int rpmflags)
    >> __releases(&dev->power.lock) __acquires(&dev->power.lock) { .
    >> .
    >> .
    >> no_callback:
    >> .
    >> .
    >> .
    >> /* Maybe the parent is now able to suspend. */
    >> if (parent && !parent->power.ignore_children &&
    >> !dev->power.irq_safe) {
    >> spin_unlock(&dev->power.lock);
    >>
    >> spin_lock(&parent->power.lock);
    >> rpm_idle(parent, RPM_ASYNC);
    >
    >to me this is bogus, if you called pm_runtime_put_sync() should should
    >be sync too. Shouldn't it ?
    >
    >> 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.

    Kevin, any comments?

    Shouldn't the framework ensure that if put_sync(dev) is called
    it should ensure that the parent is rmp_idled synchronously?
    >
    >--
    >balbi


    \
     
     \ /
      Last update: 2011-07-04 13:05    [W:0.028 / U:3.760 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site