Re: [PATCH 7/10 v6] PM / Domains: Don't stop wakeup devices during system sleep transitions

[Kevin Hilman <khilman@ti.com>]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Friday, July 01, 2011, Kevin Hilman wrote:
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> 
>> > On Thursday, June 30, 2011, Kevin Hilman wrote:
>> >> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> >> 
>> >> > From: Rafael J. Wysocki <rjw@sisk.pl>
>> >> >
>> >> > Devices that are set up to wake up the system from sleep states
>> >> > should not be stopped and power should not be removed from them
>> >> > when the system goes into a sleep state.  
>> >> 
>> >> I don't think this belongs in the generic layer since the two
>> >> assumptions above are not generally true on embedded systems, and would
>> >> result in rather significant power consumption unnecessarily.
>> >
>> > As to whether or not this belongs to the generic layer, I don't quite agree
>> > (see below), but the changelog seems to be a bit inaccurate.
>> >
>> >> First, whether the device should be stopped on device_may_wakeup():
>> >> b
>> >> Some IP blocks (at least on OMAP) have "asynchronous" wakeups.  Meaning
>> >> that they can generate wakeups even when they're not clocked (a.k.a
>> >> stopped).  So in this case, even after a ->stop_device (which clock
>> >> gates the IP), it can still generate wakeups.
>> >> 
>> >> Second, whether the device should be powered off if device_may_wakeup():
>> >> 
>> >> Embedded SoCs have other ways to wakeup than device-level wakeups.
>> >> 
>> >> For example, on OMAP, every pad on the SoC can be configured as a wakeup
>> >> source So, for example, you could completely power down the UART IP
>> >> blocks (and the enclosing power domain), configure the UART RX pad as a
>> >> wakeup source, and still wakeup the system on UART activity.  The OMAP
>> >> docs call these IO pad wakeups.
>> >> 
>> >> On OMAP in fact, this is the common, default behavior when we enable
>> >> "off-mode" in idle and/or suspend, since most of the IPs are powered off
>> >> but can still wake up the system.
>> >> 
>> >> So in summary, even if device_may_wakeup() is true, many devices (with
>> >> additional SoC magic) can still generate wakeups even when stopped and
>> >> powered off.
>> >
>> > Well, on the other hand, on some SoCs there are devices that can't be
>> > powered off (or "declocked") if they are supposed to generate wakeups.
>> 
>> Correct.
>> 
>> > Also, I'm sure there are cases in which wakeups can be generated for devices
>> > with their clocks off, but only if power is present.  
>> 
>> Yes.
>> 
>> > So there are multiple
>> > cases, but not so many overall.  So, IMO, it makes sense to handle that at
>> > the generic level, although not necessarily in such a simplistic way.
>> >
>> > Now, at this point, I want to do something very simple, which I think is
>> > done by this patch. 
>> >
>> > Is this optimal power comsumption-wise for every potential
>> > user of the framework?  
>> 
>> Well, sub-optimal would be an understatement.  I would consider this a
>> major regression since if we were to use this for OMAP, we would never
>> hit the full-chip low-power states if *any* device had wakeups enabled,
>> whereas today we can.
>> 
>> > No, but certainly for some it's sufficient.  Is it
>> > going to work in general?  I think it is.
>> >
>> > Of course, there's the question how to handle that more accurately and I have
>> > some ideas.  If you have any, please let me know.
>> >
>> > In the meantime, I'm going to modify the changelog so that it's clear that
>> > it's a "first approximation" thing, like in the patch below.
>> >
>> > Thanks,
>> > Rafael
>> >
>> > ---
>> > From: Rafael J. Wysocki <rjw@sisk.pl>
>> > Subject: PM / Domains: Don't stop wakeup devices during system sleep transitions
>> >
>> > There is the problem how to handle devices set up to wake up the
>> > system from sleep states during system-wide power transitions.
>> > In some cases, those devices can be turned off entirely, because the
>> > wakeup signals will be generated on their behalf anyway.  In some
>> > other cases, they will generate wakeup signals if their clocks are
>> > stopped, but only if power is not removed from them.  Finally, in
>> > some cases, they can only generate wakeup signals if power is not
>> > removed from them and their clocks are enabled.
>> 
>> That's a good summary.
>> 
>> > In the future, it will be necessary to take all of the above
>> > situations into account, but for starters it is possible to use
>> > the observation that if all wakeup devices are treated like the
>> > last group (i.e. their clocks are enabled and power in not removed
>> > from them during system suspend transitions), they all will be able
>> > to generate wakeups, although power consumption in the resulting
>> > system sleep state may not be optimal in some cases.
>> 
>> I'm not opposed to this kind of check happening.  I'm only opposed to it
>> happening in this "generic" layer because..., well, it's not generic.
>> 
>> Not only is it not generic, it would be a major regression in power
>> consumption for anyone moving to this layer that has the various
>> different wakeup capabilities already described.
>> 
>> The decision of whether or not to clock gate and/or power gate based on
>> wakeup capabilies has to be made somewhere (and in fact is already made
>> by existing code.)  But IMO, that decision should only be made where
>> wakeup capabilies are known, so that sensible decisions (for power
>> management) can be made.
>> 
>> Until there is a way in the generic code to distinguish between the
>> various ways a device can wakeup, this decision should be left up to the
>> code that knows how.
>
> OK, so I suppose your suggestion is to drop the patch and let the
> .stop_device() and .power_off() PM domain callbacks to hand that, is this
> correct?

Correct.

Initially I was thinking only about .power_off(), but you'd probably
want this at .stop_device() too.  In order to do that, probably want
.stop_device() to be able to return an error code such that an error
would prevent .power_off().

Kevin