Date:	Wed, 20 Jan 2016 16:23:13 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	"Rafael J. Wysocki" <rafael@...nel.org>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh@...nel.org>,
	Grant Likely <grant.likely@...aro.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Thierry Reding <treding@...dia.com>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] driver-core: platform: automatically mark wakeup devices

On Thu, Jan 21, 2016 at 12:01:59AM +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 20, 2016 at 2:51 PM, Rafael J. Wysocki <rafael@...nel.org> wrote:
> > On Wed, Jan 20, 2016 at 3:40 AM, Rafael J. Wysocki <rafael@...nel.org> wrote:
> >> On Wed, Jan 20, 2016 at 1:45 AM, Dmitry Torokhov
> >> <dmitry.torokhov@...il.com> wrote:
> >
> > [cut]
> >
> >>>> > I am not aware of any drivers denoting itself as being wakeup sources
> >>>> > and not enabling wakeup. Do you have examples?
> >>>>
> >>>> Yes, I do.
> >>>>
> >>>> i8042 is one of them, exactly to avoid enabling those devices to do
> >>>> system wakeup on systems where they were not enabled to do that in the
> >>>> past.
> >>>
> >>> Ah, yes, you added it to i8042. Well, maybe that is an argument for not
> >>> automatically enabling wakeup on ACPI systems because that was the
> >>> default behavior for them. OF/board platforms have different default
> >>> behavior. I guess if we do not do anything besides what the patch is
> >>> doing, then ACPI will not have the property I propose, so they will be
> >>> capable but not enabled, and OF/boards will have the property and will
> >>> be capable and enabled.
> >>
> >> The defaults to use should generally depend on two things IMO: On what
> >> the firmware tells us to use (given the argument that the firmware
> >> people probably have a good reason to tell us what they are telling
> >> us) and on what we were doing for this class of devices in the past.
> >> Ignoring the last bit generally leads to regressions in the cases when
> >> the firmware people are wrong after all, so I'm always cautious about
> >> this.  And this rule applies to DT as well as to ACPI, although the DT
> >> people usually won't admit it. :-)
> >
> > OK, I realize that the example I gave was not really a good one,
> > because the wakeup part was added to i8042 to support wakeup from
> > suspend-to-idle which is really special and the implementation is
> > based on what the driver was doing previously.
> 
> Which is not to say that there are no good (or at least better) examples.
> 
> USB HID devices pretty much universally support remote wakeup
> officially, but enough of them cause problems to happen while using it
> (spurious wakeups etc) that we don't enable them for system wakeup by
> default.  Plus some of them are cordless mice and such and you really
> need to be careful to avoid waking up a sleeping system by accident
> with them (when enabled).
> 
> Ethernet NICs support wakeup signaling, but at least some of them are
> not enabled for system wakeup by default.  Moreover, there are
> multiple ways to trigger the wakeup there that the user needs to
> choose from in the first place.
> 
> Some wireless adapters support WoW, but enabling them to wake up the
> system from sleep by default would be asking for troubles.
> 
> All in all, combining the information that the device can signal
> wakeup with the requirement to enable it to wake up the system from
> sleep states by default doesn't look like a good idea to me.

You are making a convincing argument here for having an option to allow
specifying that device is just wakeup capable, but not enabled by
default. For ACPI you have your own ways of defining whether the device
is wakeup capable (and you also have some rules to mark certain devices
as wakeup enabled by default - button GPEs for example), so that leaves
DT and static board files where historically we enabled wakeup for
platform/I2C/SPI devices that board believes are wakeup-capable.

I wonder if I should do that property parsing in genpd code, similarly
to what you are doing in ACPI power domain. What does not help is that
we have different notions of power domain at once - one where device is
under control of ACPI and another that denotes grouping of devices
together.

> 
> > But this also makes me think that the problem is really more
> > complicated than it may seem to be, so what about starting over and
> > looking at the wakeup problem in general?
> >
> > To that end, there are two categories of wakeup support in devices.
> > The first one is support for something called "remote wakeup" in USB
> > and which means that the device is able to generate wakeup signals
> > when it (the device) is suspended and the rest of the system can
> > generally be considered as working.  I'll use the "remote wakeup" term
> > in general for the lack of a better one.
> >
> > Remote wakeup support is used in runtime PM and suspend-to-idle (which
> > essentially uses the same kind of hardware mechanics, but in a
> > different way).
> >
> > Note, though, that "device is suspended" need not map 1:1 onto a
> > particular hardware state.  What it really means is that either it has
> > been suspended using the runtime PM framework, or all devices have
> > gone through the device suspend sequence during suspend-to-idle.  The
> > hardware state the device ends up in depends on the driver/bus type/PM
> > domain combination and is generally expected to be low-power.
> >
> > It is easy in PCI or USB and on ACPI systems where low-power states of
> > devices are well defined and the connection between them and the
> > ability to generate wakeup signals is known.  It is not so easy in the
> > other cases, though.  My personal opinion is that if wakeup support is
> > required, the device should be put into the lowest-power state from
> > which it can generate wakeup signals.  That very well may be the
> > full-power state of it if that's the only suitable one.
> >
> > If that point of view is adopted, then any device that (a) can take
> > input and (b) uses interrupts can do remote wakeup.  We don't really
> > have a good way to express that in the driver core.
> >
> > The second category of wakeup support is for platform-assisted
> > low-power states of the whole system where there's a big switch (or a
> > bunch of them) allowing multiple things to be powered off in one go
> > from the OS perspective.
> >
> > I'll write about that in the next message, as I need to take care of
> > some urgent stuff now.
> 
> Continuing.
> 
> If the device in question belongs to the part of the system powered
> off by the big switch, it has to be provided with some special
> "wakeup" power source so it can generate signals.  There needs to be a
> physical line (or bus or similar) that will be activated when wakeup
> is signaled by the device.  The activation of it needs to be noticed
> by something and turned into a signal that eventually will wake up the
> CPU (or make it start executing a power-up sequence or equivalent).
> All of that generally requires specific setup.
> 
> The device has to be left in a state in which it can use the "wakeup"
> power source in the first place.  The "wakeup" power source for it has
> to be turned on.  The signal "line" needs to be prepared for
> activation by the device's wakeup signals.  The part of the system
> responsible for waking up the CPU when that "line" is active has to be
> prepared too.  All this means that generally it is not enough to say
> "things are wired up properly" for the right stuff to happen.

There are usually 2 components to this, one from device/driver
perspective and another from system perspective. From the device
perspective there is not difference between "remote wakeup" and "big
switch" wakeup. In both cases the driver should put the device into
lowest power state that still allows it to react to external inputs.
Putting the device into that power state may also affect other devices
it is connected to, for example it may request to power down some
regulators and/or stop some clocks if they are not needed in that low
power mode. When there is activity the device sends signal (could be
dedicated interrupt pin or in-bound interrupt pin - this is device
specific) and how they are wired is board-specific.

>From the system perspective it provides notion of power domains as in
grouping (so that we know when we can turn off certain supplies and what
parts of the system should stay powered up even when system transitions
into lower power state), performs PMU set up (the bit that actually sets
up the system to process wake sources and it is PMU specific and
board-specific and is usually expressed via various DT properties, both
on PMU node and individual device nodes) and it does know how wakeup
signals are set up. In the past most of the drivers assumed that in-band
IRQ is the one that should wake up the system, now in I2C bus we added
OF "wakeup-interrupts" that can be used to set up a dedicated interrupt
as wakeup irq, otherwise we set client's in-band irq (client->irq) as
wakeup source (provided that device is annotated as "wakeup-source").
For platform devices this does not quite work as we do not know how many
interrupts they are using.

I think we are talking basically about the same thing, just in different
terms. I do not expect that by simply marking device as "wakeup-source"
it will magically wake up my system, but there is normally support for
wakeup support in both driver and platform code that actually does the
right thing when requested via this property. The property _has_ to be
documented in binding for the driver, which ensures driver-side support
is present, and making sure that system part (power domain, PMIC, PMU)
are set up appropriately is system integrators task, no different from
the task of providing appropriate BIOS/ACPI tables for system
integrators creating X86 boxes.

>
> Traditionally, we set the can_wakeup bit if (a) there is an interface
> we can use to set up those things (either by accessing some registers
> available to us or by invoking the platform firmware to do it) and (b)
> the device is hooked up to that interface in a known way.  IOW, that
> bit is inferred from what we can find out about the device's
> configuration rather that just taken from the firmware for granted.

It _really_ depends on the device. Some of them are non-discoverable and
thus you need to trust firmware. For example for I2C device you need to
know interrupt number, you can't normally query device for it.

> 
> Now, this generally may not be the right approach.
> 
> Maybe we just don't need the can_wakeup bit at all.  Maybe we should
> just create the "wakeup" sysfs attribute for all devices, let user
> space set it the way it wants and handle it on the best effort basis.
> That is, it will wake up from the sleep states it can wake up from,
> but it may not wake up from any of them if there's no support.
> Granted, we really can't guarantee that it will work anyway (even if
> the firmware exposes the interface to us, it may not be correctly
> implemented etc and there's no way to verify that upfront).  And we
> may allow the same attribute to be used for disabling remote wakeup
> for runtime PM if someone wants to.
> 
> In that case, though, we really won't need the firmware to tell us
> whether or not the device is "wakeup-capable".  We will always treat
> it this way and if it turns out that something is missing, wakeup will
> just not work.

This is a non-starter. Quite often there is difference in power
consumption between deep sleep/powered off state and state in which
device can signal activity so that the system (or subsystem) can wake
up. So if firmware knows that device is not wired to wake up the system
we better communicate it to kernel/userspace instead of them having to
cross their fingers and hope for the best.

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists