lkml.org 
[lkml]   [2009]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [RFC][PATCH] PM: Introduce core framework for run-time PM of I/O devices (rev. 8)
    Date
    On Wednesday 08 July 2009, Magnus Damm wrote:
    > On Wed, Jul 8, 2009 at 7:07 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
    > > On Tuesday 07 July 2009, Magnus Damm wrote:
    > >> On Mon, Jul 6, 2009 at 9:52 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
    > >> > Hi,
    > >> >
    > >> > There's a rev. 8 of the run-time PM framework patch.
    >
    > >> All good with the code above, but there seem to be some issue with how
    > >> usage_count is counted up and down and when runtime_disabled is set:
    > >>
    > >> 1. pm_runtime_init(): usage_count = 1, runtime_disabled = true
    > >> 2. driver_probe_device(): pm_runtime_get_sync()
    > >> 3. pm_runtime_get_sync(): usage_count = 2
    > >> 4. device driver probe(): pm_runtime_enable()
    > >> 5. pm_runtime_enable(): usage_count = 1
    > >> 6. driver_probe_device(): pm_runtime_put()
    > >> 7. pm_runtime_put(): usage_count = 0
    > >>
    > >> I expect runtime_disabled = false in 7. Modifying the get/put calls to
    > >> do enable/disable may work around the issue, but that's probably not
    > >> what you guys want.
    > >
    > > Sure, that's my mistake. I should have used a separate counter for
    > > disable/enable, but I thought usage_counter would be sufficient. Will fix.
    >
    > Thank you. No problem.
    >
    > >> Issue 2:
    > >> ------------
    > >> I cannot get any bus ->runtime_resume() callbacks from probe(). This
    > >> also seems related to usage_count and pm_runtime_get_sync() in
    > >> driver_probe_device(). Basically, from probe(), calling
    > >> pm_runtime_resume() after pm_runtime_set_suspended() results in error
    > >> and not in a ->runtime_resume() callback. Some device drives access
    > >> hardware in probe(), so the ->runtime_resume() callback is needed at
    > >> that point to turn on clocks before the hardware can be accessed.
    > >
    > > I think the problem is that pm_runtime_get_sync() in driver_probe_device()
    > > calls ->runtime_resume(), so the device is active from the core's point of
    > > view when you call pm_runtime_resume() from probe().
    > >
    > > Hmm. OK, perhaps we should just increment usage_count in
    > > driver_device_probe() to prevent suspends from happening at that time, without
    > > calling ->runtime_resume() so that the driver can do it by itself. I'll do
    > > that in the next version.
    >
    > Sounds good.
    >
    > >> Random thought:
    > >> -------------------------
    > >> The runtime_pm_get() and runtime_pm_put() look very nice. I assume
    > >> that inteface is supposed to be used by bus code. I wonder if it would
    > >> be cleaner to use a similar counter based interface from the driver
    > >> instead of the pm_runtime_idle()/suspend()/resume()...
    > >>
    > >> Let me know what you think!
    > >
    > > In fact I thought drivers could also use pm_runtime_[get|put]() and the 'sync'
    > > versions. At least, I don't see why not at the moment (well, I'm a bit tired
    > > right now ...).
    >
    > I think that's a nicer interface, but I must figure out how to use
    > ->runtime_idle before I can switch to that...
    >
    > > However, I'm now thinking it should work like this:
    > >
    > > * pm_runtime_get() increments usage_count and if it was zero before the
    > > incrementation, it calls pm_request_resume() (pm_runtime_resume() is called
    > > by the 'sync' version).
    > >
    > > * pm_runtime_put() decrements usage_count and if it's zero after the
    > > decrementation, it calls pm_request_idle() (pm_runtime_idle() is called by
    > > the 'sync' version).
    > >
    > > * The 'suspend' callbacks won't succeed for usage_count > 0.
    > >
    > > This way we would avoid calling the 'suspend' and 'idle' functions each time
    > > unnecessarily, but then usage_count would have to be modified under the
    > > spinlock only.
    >
    > If all usage_count users are moved under the spinlock then there would
    > be no need for atomic operations, right?
    >
    > This get()/put() interface is interesting.
    >
    > So I'd like to tie in two levels of power management in our runtime PM
    > implementation. The most simple level is clock stopping, and I can do
    > that using the bus callbacks ->runtime_suspend() and
    > ->runtime_resume() with v8. The driver runtime callbacks are never
    > invoked for clock stopping.
    >
    > On top of the clock stopping I'd like to turn off power to the domain.
    > So if all clocks are stopped to the devices within a domain, then I'd
    > like to call the per-device ->runtime_suspend() callbacks provided by
    > the drivers.
    >
    > I wonder how to fit these two levels of power management into the
    > runtime PM in a nice way. My first attempts simply made use of
    > pm_runtime_resume() and pm_runtime_suspend(), but I'd like to move to
    > get()/put() if possible. But for that to work I need to implement
    > ->runtime_idle() in my bus code, and I wonder if the current runtime
    > PM idle behaviour is a good fit.
    >
    > Below is how I'd like to make use of the runtime PM code. I'm not sure
    > if it's compatible with your view. =)
    >
    > Drivers call pm_runtime_get_sync() and pm_runtime_put() before and
    > after using the hardware. The runtime PM code invokes the bus
    > ->runtime_idle() callback ASAP (of course depending on put() or
    > put_sync(), but no timer). The bus->runtime_idle() callback stops the
    > clock and decreases the power domain usage count. If the power domain
    > is unused, then the pm_schedule_suspend() is called for each of the
    > devices in the power domain. This in turn will invoke the
    > ->runtime_suspend() callback which starts the clock, calls the driver
    > ->runtime_suspend() and stops the clock again. When all devices are
    > runtime suspended the power domain is turned off.
    >
    > I can't get the above to work with v8 though. This is because after
    > the clock is stopped with ->runtime_idle() the runtime_status of the
    > device is still RPM_ACTIVE, so when pm_runtime_get_sync() gets called
    > the ->runtime_resume() never gets invoked and the clock is never
    > started...
    >
    > So I don't know if you think the ->runtime_idle usage above is a good
    > plan. I guess no, it's probably quite different from the USB case. I
    > can of course always skip using ->runtime_idle() and just use
    > suspend()/resume().
    >
    > Any thoughts?

    I think you'd need a separate bus type callback for that, call it
    ->runtime_deepen() for now, which could be executed for a _suspended_
    (from the core's point of view) device and the role of which would be to put
    the (already suspended) device into a deeper low power state.

    Something like this might also be used for PCI and it's worth discussing IMO.

    So, if we had such a callback, your scenario would be the following.

    Drivers call pm_runtime_get_sync() and pm_runtime_put() before and
    after using the hardware. The runtime PM code invokes the bus
    ->runtime_idle() callback that in turn calls pm_runtime_suspend() or
    pm_schedule_suspend() and the ->runtime_suspend() executed as a result
    stops the clock and decreases the power domain usage count. If the
    domain usage count happens to be zero, pm_runtime_deepen() or
    pm_schedule_deepen() is called for each device in the power domain.
    Consequently, the bus type's ->runtime_deepen() is invoked and that can
    call the device's ->runtime_suspend(), for example. If there's
    pm_runtime_get_sync() any time when this is happening, it will cancel the
    pending requests and run ->runtime_resume().

    Does it make sense?

    Rafael


    \
     
     \ /
      Last update: 2009-07-08 21:03    [W:0.037 / U:0.024 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site