| Message ID | 1515495251-22906-1-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Tuesday, January 9, 2018 11:54:11 AM CET Ulf Hansson wrote: > In some cases, a driver may not require its device to be powered on to be > able to deliver wakeup signals during system suspend. That quite often is the case. It is the standard way wakeup signaling works in PCI and in ACPI. > Likely the most common scenario when this is the case, is a driver routing > the wakeup signal through a GPIO IRQ, instead of using the regular in-band > IRQ logic, via the device itself. > > Obviously the driver may put its device into a low power state during > system suspend in cases like this. For example it may gate clocks, put > pinctrls into sleep state, etc. > > However, for middle-layers and PM domains (like genpd), which checks the > return value from device_may_wakeup() and/or the ->dev.power.wakeup_path > status flag, there is information missing about scenarios like these. > > In the case of genpd, when it finds the ->dev.power.wakeup_path status flag > being set for a device during system suspend, it leaves the device in > powered on state including its PM domain. In other words, wasting power. > > To address this issue, add a new ->dev.power.no_inband_wakeup flag for the > device, which drivers may set/clear to inform the PM core, in case > device_may_wakeup() returns true, whether that shall make the > ->dev.power.wakeup_path status flag to be set or not for the device. > > The PM core checks the ->dev.power.no_inband_wakeup flag in the > __device_suspend() phase, after invoking the ->suspend() callback for the > device. At that point, the driver is responsible that it has set a correct > value to the flag. > > Let's also introduce a helper function, device_set_no_inband_wakeup(), > which drivers shall use to set/clear the ->dev.power.no_inband_wakeup flag. Wait, wait. > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > > To get some additional background, one may look at earlier discussions from > various threads. The most recent is in and RFD [1] from Rafael and from an mmc > series by Adrian [2]. Let's first conclude that discussion, please. > [1] > https://marc.info/?l=linux-pm&m=151541229425689&w=2 > > [2] > https://www.spinics.net/lists/linux-mmc/msg47549.html > > --- > drivers/base/power/main.c | 2 +- > include/linux/pm.h | 1 + > include/linux/pm_wakeup.h | 8 ++++++++ > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index 02a497e..376c275 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -1794,7 +1794,7 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async) > End: > if (!error) { > dev->power.is_suspended = true; > - if (device_may_wakeup(dev)) > + if (device_may_wakeup(dev) && !dev->power.no_inband_wakeup) That is not just a system-wide PM concept. It affects runtime PM potentially too. > dev->power.wakeup_path = true; > > dpm_propagate_wakeup_to_parent(dev); > diff --git a/include/linux/pm.h b/include/linux/pm.h > index e723b78..d131834 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -600,6 +600,7 @@ struct dev_pm_info { > struct completion completion; > struct wakeup_source *wakeup; > bool wakeup_path:1; > + bool no_inband_wakeup:1; > bool syscore:1; > bool no_pm_callbacks:1; /* Owned by the PM core */ > unsigned int must_resume:1; /* Owned by the PM core */ > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > index 4238dde..5d40887 100644 > --- a/include/linux/pm_wakeup.h > +++ b/include/linux/pm_wakeup.h > @@ -93,6 +93,11 @@ static inline void device_set_wakeup_path(struct device *dev) > dev->power.wakeup_path = true; > } > > +static inline void device_set_no_inband_wakeup(struct device *dev, bool wakeup) > +{ > + dev->power.no_inband_wakeup = wakeup; > +} > + > /* drivers/base/power/wakeup.c */ > extern void wakeup_source_prepare(struct wakeup_source *ws, const char *name); > extern struct wakeup_source *wakeup_source_create(const char *name); > @@ -181,6 +186,9 @@ static inline bool device_may_wakeup(struct device *dev) > > static inline void device_set_wakeup_path(struct device *dev) {} > > +static inline void device_set_no_inband_wakeup(struct device *dev, > + bool wakeup) {} > + > static inline void __pm_stay_awake(struct wakeup_source *ws) {} > > static inline void pm_stay_awake(struct device *dev) {} >
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 02a497e..376c275 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -1794,7 +1794,7 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async) End: if (!error) { dev->power.is_suspended = true; - if (device_may_wakeup(dev)) + if (device_may_wakeup(dev) && !dev->power.no_inband_wakeup) dev->power.wakeup_path = true; dpm_propagate_wakeup_to_parent(dev); diff --git a/include/linux/pm.h b/include/linux/pm.h index e723b78..d131834 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -600,6 +600,7 @@ struct dev_pm_info { struct completion completion; struct wakeup_source *wakeup; bool wakeup_path:1; + bool no_inband_wakeup:1; bool syscore:1; bool no_pm_callbacks:1; /* Owned by the PM core */ unsigned int must_resume:1; /* Owned by the PM core */ diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h index 4238dde..5d40887 100644 --- a/include/linux/pm_wakeup.h +++ b/include/linux/pm_wakeup.h @@ -93,6 +93,11 @@ static inline void device_set_wakeup_path(struct device *dev) dev->power.wakeup_path = true; } +static inline void device_set_no_inband_wakeup(struct device *dev, bool wakeup) +{ + dev->power.no_inband_wakeup = wakeup; +} + /* drivers/base/power/wakeup.c */ extern void wakeup_source_prepare(struct wakeup_source *ws, const char *name); extern struct wakeup_source *wakeup_source_create(const char *name); @@ -181,6 +186,9 @@ static inline bool device_may_wakeup(struct device *dev) static inline void device_set_wakeup_path(struct device *dev) {} +static inline void device_set_no_inband_wakeup(struct device *dev, + bool wakeup) {} + static inline void __pm_stay_awake(struct wakeup_source *ws) {} static inline void pm_stay_awake(struct device *dev) {}
In some cases, a driver may not require its device to be powered on to be able to deliver wakeup signals during system suspend. Likely the most common scenario when this is the case, is a driver routing the wakeup signal through a GPIO IRQ, instead of using the regular in-band IRQ logic, via the device itself. Obviously the driver may put its device into a low power state during system suspend in cases like this. For example it may gate clocks, put pinctrls into sleep state, etc. However, for middle-layers and PM domains (like genpd), which checks the return value from device_may_wakeup() and/or the ->dev.power.wakeup_path status flag, there is information missing about scenarios like these. In the case of genpd, when it finds the ->dev.power.wakeup_path status flag being set for a device during system suspend, it leaves the device in powered on state including its PM domain. In other words, wasting power. To address this issue, add a new ->dev.power.no_inband_wakeup flag for the device, which drivers may set/clear to inform the PM core, in case device_may_wakeup() returns true, whether that shall make the ->dev.power.wakeup_path status flag to be set or not for the device. The PM core checks the ->dev.power.no_inband_wakeup flag in the __device_suspend() phase, after invoking the ->suspend() callback for the device. At that point, the driver is responsible that it has set a correct value to the flag. Let's also introduce a helper function, device_set_no_inband_wakeup(), which drivers shall use to set/clear the ->dev.power.no_inband_wakeup flag. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- To get some additional background, one may look at earlier discussions from various threads. The most recent is in and RFD [1] from Rafael and from an mmc series by Adrian [2]. Kind regards Uffe [1] https://marc.info/?l=linux-pm&m=151541229425689&w=2 [2] https://www.spinics.net/lists/linux-mmc/msg47549.html --- drivers/base/power/main.c | 2 +- include/linux/pm.h | 1 + include/linux/pm_wakeup.h | 8 ++++++++ 3 files changed, 10 insertions(+), 1 deletion(-)