| Message ID | 1510588003-16650-3-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PM / core: Invent a WAKEUP_POWERED driver flag | expand |
Hi Ulf, you have left some old naming WAKEUP_POWERED in your patch On 13 November 2017 at 16:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > For some bus types and PM domains, it's not sufficient to only check the > return value from device_may_wakeup(), to fully understand how to configure > wakeup settings for the device during system suspend. > [snip] > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 65d3911..4efb16a 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -559,6 +559,7 @@ struct pm_subsys_data { > * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device. > * SMART_PREPARE: Check the return value of the driver's ->prepare callback. > * SMART_SUSPEND: No need to resume the device from runtime suspend. > + * WAKEUP_POWERED: Keep the device powered if it has wakeup enabled. it should be IN_BAND_WAKEUP ? > * > * Setting SMART_PREPARE instructs bus types and PM domains which may want > * system suspend/resume callbacks to be skipped for the device to return 0 from > @@ -572,10 +573,14 @@ struct pm_subsys_data { > * necessary from the driver's perspective. It also may cause them to skip > * invocations of the ->suspend_late and ->suspend_noirq callbacks provided by > * the driver if they decide to leave the device in runtime suspend. > + * > + * Setting WAKEUP_POWERED instructs bus types and PM domains that the device it should be IN_BAND_WAKEUP ? > + * needs to remain powered in system suspend, in case wakeup is enabled for it. > */ > #define DPM_FLAG_NEVER_SKIP BIT(0) > #define DPM_FLAG_SMART_PREPARE BIT(1) > #define DPM_FLAG_SMART_SUSPEND BIT(2) > +#define DPM_FLAG_IN_BAND_WAKEUP BIT(3) > > struct dev_pm_info { > pm_message_t power_state; > @@ -595,6 +600,7 @@ struct dev_pm_info { > struct completion completion; > struct wakeup_source *wakeup; > bool wakeup_path:1; > + bool wakeup_path_in_band:1; > bool syscore:1; > bool no_pm_callbacks:1; /* Owned by the PM core */ > #else > -- > 2.7.4 >
On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote: > For some bus types and PM domains, it's not sufficient to only check the > return value from device_may_wakeup(), to fully understand how to configure > wakeup settings for the device during system suspend. > > In particular, sometimes the device may need to remain in its power state, > in case the driver has configured it for in band IRQs, as to be able to > generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP > driver flag, to enable drivers to instruct bus types and PM domains about > this setting. > > Of course, in case the bus type and PM domain has additional information > about wakeup settings of the device, they may override the behaviour. > > Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus > types and PM domains should treat the device's parent during system > suspend. Therefore, in __device_suspend(), let the PM core propagate the > wakeup setting by using a status flag in the struct dev_pm_info for the > parent. This also makes it consistent with how the existing "wakeup_path" > status flag is being assigned. I've been thinking about this quite a bit recently and my conclusion is that the flag makes perfect sence (as it covers a valid use case), but I would define it and design the handling of it a bit differently. First off, the genpd changes in patch [3/3] effectively skip the "noirq" driver callbacks for the device, but question is why this is just "noirq". Arguably, the "late suspend" callback should be skipped too (the regular "suspend" one not necessarily, because the state of the device can still be changed via runtime PM at that point). Moreover, if we go for the skipping at the suspend time, all of the resume should be skipped for the device too, because it has not been suspended in the first place. But, if the device is in suspend at the suspend time already, the resume part should not be skipped for it at all (unless it has LEAVE_SUSPENDED set too), so it looks like the skipping should only happen if the device has not been suspended when the system transition starts. Second, it looks like the flag should be handled in the core in accordance with what genpd does with it or drivers will have to check the middle layer configuration. So below is my version of the core part (on top of the series I posted earlier today: https://marc.info/?l=linux-pm&m=151286423304445&w=2) and the genpd one should be analogous IMO. --- Documentation/driver-api/pm/devices.rst | 21 ++++++++- drivers/base/power/main.c | 71 +++++++++++++++++++++++++------- include/linux/pm.h | 8 +++ 3 files changed, 82 insertions(+), 18 deletions(-) Index: linux-pm/include/linux/pm.h =================================================================== --- linux-pm.orig/include/linux/pm.h +++ linux-pm/include/linux/pm.h @@ -560,6 +560,7 @@ struct pm_subsys_data { * SMART_PREPARE: Check the return value of the driver's ->prepare callback. * SMART_SUSPEND: No need to resume the device from runtime suspend. * LEAVE_SUSPENDED: Avoid resuming the device during system resume if possible. + * IN_BAND_WAKEUP: Avoid suspending the device if configured for system wakeup. * * Setting SMART_PREPARE instructs bus types and PM domains which may want * system suspend/resume callbacks to be skipped for the device to return 0 from @@ -576,11 +577,16 @@ struct pm_subsys_data { * * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that the * driver prefers the device to be left in suspend after system resume. + * + * Setting IN_BAND_WAKEUP informs the PM core and middle-layer code that, as far + * as the driver knows, the device cannot be suspended to be able to wake up the + * system from sleep. */ #define DPM_FLAG_NEVER_SKIP BIT(0) #define DPM_FLAG_SMART_PREPARE BIT(1) #define DPM_FLAG_SMART_SUSPEND BIT(2) #define DPM_FLAG_LEAVE_SUSPENDED BIT(3) +#define DPM_FLAG_IN_BAND_WAKEUP BIT(4) struct dev_pm_info { pm_message_t power_state; @@ -604,6 +610,7 @@ struct dev_pm_info { bool no_pm_callbacks:1; /* Owned by the PM core */ unsigned int must_resume:1; /* Owned by the PM core */ unsigned int may_skip_resume:1; /* Set by subsystems */ + unsigned int skip_suspend:1; /* Owned by the PM core */ #else unsigned int should_wakeup:1; #endif @@ -774,6 +781,7 @@ extern void pm_generic_complete(struct d extern void dev_pm_skip_next_resume_phases(struct device *dev); extern bool dev_pm_may_skip_resume(struct device *dev); +extern bool dev_pm_skip_suspend_and_not_suspended(struct device *dev); extern bool dev_pm_smart_suspend_and_suspended(struct device *dev); #else /* !CONFIG_PM_SLEEP */ Index: linux-pm/Documentation/driver-api/pm/devices.rst =================================================================== --- linux-pm.orig/Documentation/driver-api/pm/devices.rst +++ linux-pm/Documentation/driver-api/pm/devices.rst @@ -401,6 +401,10 @@ the phases are: ``prepare``, ``suspend`` generated by some other device after its own device had been set to low power. +If any of these callbacks returns an error, the system won't enter the desired +low-power state. Instead, the PM core will unwind its actions by resuming all +the devices that were suspended. + At the end of these phases, drivers should have stopped all I/O transactions (DMA, IRQs), saved enough state that they can re-initialize or restore previous state (as needed by the hardware), and placed the device into a low-power state. @@ -414,9 +418,20 @@ when the system is in the sleep state. might identify GPIO signals hooked up to a switch or other external hardware, and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal. -If any of these callbacks returns an error, the system won't enter the desired -low-power state. Instead, the PM core will unwind its actions by resuming all -the devices that were suspended. +Some devices can only generate any signals if they are not suspended at all. If +:c:func:`device_may_wakeup(dev)` returns ``true`` for such a device, it should +not be suspended during system-wide transitions to sleep states. Device drivers +can indicate that condition to the PM core and middle-layer code (bus types or +PM domains) by setting the ``DPM_FLAG_IN_BAND_WAKEUP`` driver flag (at the probe +time). The PM core and middle-layer code respond to that by avoiding to suspend +the device unless they have additional information on the device's wakeup +capabilities (for example, they may know that the device is in fact capable of +generating wakeup signals from a low-power state which the driver may not be +aware of). In particular, the PM core will not invoke driver callbacks in the +``suspend_late`` and ``suspend_noirq`` phases for devices with +``DPM_FLAG_IN_BAND_WAKEUP`` set and for their ancestors and suppliers, but it +still will invoke middle-layer callbacks for them (if present) and those +callbacks are then responsible for handling the devices as appropriate. Leaving System Suspend Index: linux-pm/drivers/base/power/main.c =================================================================== --- linux-pm.orig/drivers/base/power/main.c +++ linux-pm/drivers/base/power/main.c @@ -609,6 +609,14 @@ static pm_callback_t dpm_subsys_suspend_ pm_message_t state, const char **info_p); +static bool device_no_subsys_suspend(struct device *dev, pm_message_t state) +{ + pm_message_t suspend_msg = suspend_event(state); + + return !dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) && + !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL); +} + /** * device_resume_noirq - Execute a "noirq resume" callback for given device. * @dev: Device to handle. @@ -645,25 +653,26 @@ static int device_resume_noirq(struct de if (skip_resume) goto Skip; - if (dev_pm_smart_suspend_and_suspended(dev)) { - pm_message_t suspend_msg = suspend_event(state); - - /* - * If "freeze" callbacks have been skipped during a transition - * related to hibernation, the subsequent "thaw" callbacks must - * be skipped too or bad things may happen. Otherwise, resume - * callbacks are going to be run for the device, so its runtime - * PM status must be changed to reflect the new state after the - * transition under way. - */ - if (!dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) && - !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL)) { + if (device_no_subsys_suspend(dev, state)) { + if (dev_pm_smart_suspend_and_suspended(dev)) { + /* + * If "freeze" callbacks have been skipped during a + * transition related to hibernation, the subsequent + * "thaw" callbacks must be skipped too or bad things + * may happen. Otherwise, resume callbacks are going to + * be run for the device, so its runtime PM status must + * be changed to reflect the new state after the + * transition under way. + */ if (state.event == PM_EVENT_THAW) { skip_resume = true; goto Skip; } else { pm_runtime_set_active(dev); } + } else if (dev_pm_skip_suspend_and_not_suspended(dev)) { + skip_resume = true; + goto Skip; } } @@ -1317,7 +1326,8 @@ static int __device_suspend_noirq(struct no_subsys_cb = !dpm_subsys_suspend_late_cb(dev, state, NULL); - if (dev_pm_smart_suspend_and_suspended(dev) && no_subsys_cb) + if ((dev_pm_smart_suspend_and_suspended(dev) || + dev_pm_skip_suspend_and_not_suspended(dev)) && no_subsys_cb) goto Skip; if (dev->driver && dev->driver->pm) { @@ -1450,6 +1460,22 @@ int dpm_suspend_noirq(pm_message_t state return ret; } +static void dpm_superior_set_skip_suspend(struct device *dev) +{ + struct device_link *link; + int idx; + + if (dev->parent) + dev->parent->power.skip_suspend = true; + + idx = device_links_read_lock(); + + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) + link->supplier->power.skip_suspend = true; + + device_links_read_unlock(idx); +} + static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev, pm_message_t state, const char **info_p) @@ -1511,11 +1537,17 @@ static int __device_suspend_late(struct if (dev->power.syscore || dev->power.direct_complete) goto Complete; + if ((state.event == PM_EVENT_SUSPEND || state.event == PM_EVENT_HIBERNATE) && + dev_pm_test_driver_flags(dev, DPM_FLAG_IN_BAND_WAKEUP) && + device_may_wakeup(dev)) + dev->power.skip_suspend = true; + callback = dpm_subsys_suspend_late_cb(dev, state, &info); if (callback) goto Run; - if (dev_pm_smart_suspend_and_suspended(dev) && + if ((dev_pm_smart_suspend_and_suspended(dev) || + dev_pm_skip_suspend_and_not_suspended(dev)) && !dpm_subsys_suspend_noirq_cb(dev, state, NULL)) goto Skip; @@ -1534,6 +1566,9 @@ Run: Skip: dev->power.is_late_suspended = true; + if (dev->power.skip_suspend) + dpm_superior_set_skip_suspend(dev); + Complete: TRACE_SUSPEND(error); complete_all(&dev->power.completion); @@ -1746,6 +1781,7 @@ static int __device_suspend(struct devic dev->power.may_skip_resume = false; dev->power.must_resume = false; + dev->power.skip_suspend = false; dpm_watchdog_set(&wd, dev); device_lock(dev); @@ -2112,6 +2148,11 @@ void device_pm_check_callbacks(struct de spin_unlock_irq(&dev->power.lock); } +bool dev_pm_skip_suspend_and_not_suspended(struct device *dev) +{ + return dev->power.skip_suspend && !pm_runtime_status_suspended(dev); +} + bool dev_pm_smart_suspend_and_suspended(struct device *dev) { return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) &&
On 10 December 2017 at 03:30, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote: >> For some bus types and PM domains, it's not sufficient to only check the >> return value from device_may_wakeup(), to fully understand how to configure >> wakeup settings for the device during system suspend. >> >> In particular, sometimes the device may need to remain in its power state, >> in case the driver has configured it for in band IRQs, as to be able to >> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP >> driver flag, to enable drivers to instruct bus types and PM domains about >> this setting. >> >> Of course, in case the bus type and PM domain has additional information >> about wakeup settings of the device, they may override the behaviour. >> >> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus >> types and PM domains should treat the device's parent during system >> suspend. Therefore, in __device_suspend(), let the PM core propagate the >> wakeup setting by using a status flag in the struct dev_pm_info for the >> parent. This also makes it consistent with how the existing "wakeup_path" >> status flag is being assigned. > > I've been thinking about this quite a bit recently and my conclusion is that > the flag makes perfect sence (as it covers a valid use case), but I would > define it and design the handling of it a bit differently. > > First off, the genpd changes in patch [3/3] effectively skip the "noirq" > driver callbacks for the device, but question is why this is just "noirq". > Arguably, the "late suspend" callback should be skipped too (the regular > "suspend" one not necessarily, because the state of the device can still > be changed via runtime PM at that point). Moreover, if we go for the > skipping at the suspend time, all of the resume should be skipped for the > device too, because it has not been suspended in the first place. No, sorry, but this is not the purpose of the flag. Actually there is no reason at all to why genpd needs to skip invoking the driver's noirq callbacks, no matter of whether the flag is set or not. Moreover, skipping invoking callbacks would not work for those cases where the driver needs to configure its wakeup setting in those callbacks. Or if the driver has anything additional todo, like disabling a request queue or whatever things, that it needs to manage in those callbacks. Instead this is about telling genpd that it must not power off the PM domain (and in the special case of when genpd has the ->dev_ops.stop|start() assigned for the device, avoid to call them). For that reason, I don't think the PM core needs to be involved, besides what the patch already does. Kind regards Uffe > > But, if the device is in suspend at the suspend time already, the > resume part should not be skipped for it at all (unless it has > LEAVE_SUSPENDED set too), so it looks like the skipping should only > happen if the device has not been suspended when the system transition > starts. > > Second, it looks like the flag should be handled in the core in > accordance with what genpd does with it or drivers will have to > check the middle layer configuration. > > So below is my version of the core part (on top of the series I posted > earlier today: https://marc.info/?l=linux-pm&m=151286423304445&w=2) > and the genpd one should be analogous IMO. > > --- > Documentation/driver-api/pm/devices.rst | 21 ++++++++- > drivers/base/power/main.c | 71 +++++++++++++++++++++++++------- > include/linux/pm.h | 8 +++ > 3 files changed, 82 insertions(+), 18 deletions(-) > > Index: linux-pm/include/linux/pm.h > =================================================================== > --- linux-pm.orig/include/linux/pm.h > +++ linux-pm/include/linux/pm.h > @@ -560,6 +560,7 @@ struct pm_subsys_data { > * SMART_PREPARE: Check the return value of the driver's ->prepare callback. > * SMART_SUSPEND: No need to resume the device from runtime suspend. > * LEAVE_SUSPENDED: Avoid resuming the device during system resume if possible. > + * IN_BAND_WAKEUP: Avoid suspending the device if configured for system wakeup. > * > * Setting SMART_PREPARE instructs bus types and PM domains which may want > * system suspend/resume callbacks to be skipped for the device to return 0 from > @@ -576,11 +577,16 @@ struct pm_subsys_data { > * > * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that the > * driver prefers the device to be left in suspend after system resume. > + * > + * Setting IN_BAND_WAKEUP informs the PM core and middle-layer code that, as far > + * as the driver knows, the device cannot be suspended to be able to wake up the > + * system from sleep. > */ > #define DPM_FLAG_NEVER_SKIP BIT(0) > #define DPM_FLAG_SMART_PREPARE BIT(1) > #define DPM_FLAG_SMART_SUSPEND BIT(2) > #define DPM_FLAG_LEAVE_SUSPENDED BIT(3) > +#define DPM_FLAG_IN_BAND_WAKEUP BIT(4) > > struct dev_pm_info { > pm_message_t power_state; > @@ -604,6 +610,7 @@ struct dev_pm_info { > bool no_pm_callbacks:1; /* Owned by the PM core */ > unsigned int must_resume:1; /* Owned by the PM core */ > unsigned int may_skip_resume:1; /* Set by subsystems */ > + unsigned int skip_suspend:1; /* Owned by the PM core */ > #else > unsigned int should_wakeup:1; > #endif > @@ -774,6 +781,7 @@ extern void pm_generic_complete(struct d > > extern void dev_pm_skip_next_resume_phases(struct device *dev); > extern bool dev_pm_may_skip_resume(struct device *dev); > +extern bool dev_pm_skip_suspend_and_not_suspended(struct device *dev); > extern bool dev_pm_smart_suspend_and_suspended(struct device *dev); > > #else /* !CONFIG_PM_SLEEP */ > Index: linux-pm/Documentation/driver-api/pm/devices.rst > =================================================================== > --- linux-pm.orig/Documentation/driver-api/pm/devices.rst > +++ linux-pm/Documentation/driver-api/pm/devices.rst > @@ -401,6 +401,10 @@ the phases are: ``prepare``, ``suspend`` > generated by some other device after its own device had been set to low > power. > > +If any of these callbacks returns an error, the system won't enter the desired > +low-power state. Instead, the PM core will unwind its actions by resuming all > +the devices that were suspended. > + > At the end of these phases, drivers should have stopped all I/O transactions > (DMA, IRQs), saved enough state that they can re-initialize or restore previous > state (as needed by the hardware), and placed the device into a low-power state. > @@ -414,9 +418,20 @@ when the system is in the sleep state. > might identify GPIO signals hooked up to a switch or other external hardware, > and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal. > > -If any of these callbacks returns an error, the system won't enter the desired > -low-power state. Instead, the PM core will unwind its actions by resuming all > -the devices that were suspended. > +Some devices can only generate any signals if they are not suspended at all. If > +:c:func:`device_may_wakeup(dev)` returns ``true`` for such a device, it should > +not be suspended during system-wide transitions to sleep states. Device drivers > +can indicate that condition to the PM core and middle-layer code (bus types or > +PM domains) by setting the ``DPM_FLAG_IN_BAND_WAKEUP`` driver flag (at the probe > +time). The PM core and middle-layer code respond to that by avoiding to suspend > +the device unless they have additional information on the device's wakeup > +capabilities (for example, they may know that the device is in fact capable of > +generating wakeup signals from a low-power state which the driver may not be > +aware of). In particular, the PM core will not invoke driver callbacks in the > +``suspend_late`` and ``suspend_noirq`` phases for devices with > +``DPM_FLAG_IN_BAND_WAKEUP`` set and for their ancestors and suppliers, but it > +still will invoke middle-layer callbacks for them (if present) and those > +callbacks are then responsible for handling the devices as appropriate. > > > Leaving System Suspend > Index: linux-pm/drivers/base/power/main.c > =================================================================== > --- linux-pm.orig/drivers/base/power/main.c > +++ linux-pm/drivers/base/power/main.c > @@ -609,6 +609,14 @@ static pm_callback_t dpm_subsys_suspend_ > pm_message_t state, > const char **info_p); > > +static bool device_no_subsys_suspend(struct device *dev, pm_message_t state) > +{ > + pm_message_t suspend_msg = suspend_event(state); > + > + return !dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) && > + !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL); > +} > + > /** > * device_resume_noirq - Execute a "noirq resume" callback for given device. > * @dev: Device to handle. > @@ -645,25 +653,26 @@ static int device_resume_noirq(struct de > if (skip_resume) > goto Skip; > > - if (dev_pm_smart_suspend_and_suspended(dev)) { > - pm_message_t suspend_msg = suspend_event(state); > - > - /* > - * If "freeze" callbacks have been skipped during a transition > - * related to hibernation, the subsequent "thaw" callbacks must > - * be skipped too or bad things may happen. Otherwise, resume > - * callbacks are going to be run for the device, so its runtime > - * PM status must be changed to reflect the new state after the > - * transition under way. > - */ > - if (!dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) && > - !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL)) { > + if (device_no_subsys_suspend(dev, state)) { > + if (dev_pm_smart_suspend_and_suspended(dev)) { > + /* > + * If "freeze" callbacks have been skipped during a > + * transition related to hibernation, the subsequent > + * "thaw" callbacks must be skipped too or bad things > + * may happen. Otherwise, resume callbacks are going to > + * be run for the device, so its runtime PM status must > + * be changed to reflect the new state after the > + * transition under way. > + */ > if (state.event == PM_EVENT_THAW) { > skip_resume = true; > goto Skip; > } else { > pm_runtime_set_active(dev); > } > + } else if (dev_pm_skip_suspend_and_not_suspended(dev)) { > + skip_resume = true; > + goto Skip; > } > } > > @@ -1317,7 +1326,8 @@ static int __device_suspend_noirq(struct > > no_subsys_cb = !dpm_subsys_suspend_late_cb(dev, state, NULL); > > - if (dev_pm_smart_suspend_and_suspended(dev) && no_subsys_cb) > + if ((dev_pm_smart_suspend_and_suspended(dev) || > + dev_pm_skip_suspend_and_not_suspended(dev)) && no_subsys_cb) > goto Skip; > > if (dev->driver && dev->driver->pm) { > @@ -1450,6 +1460,22 @@ int dpm_suspend_noirq(pm_message_t state > return ret; > } > > +static void dpm_superior_set_skip_suspend(struct device *dev) > +{ > + struct device_link *link; > + int idx; > + > + if (dev->parent) > + dev->parent->power.skip_suspend = true; > + > + idx = device_links_read_lock(); > + > + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) > + link->supplier->power.skip_suspend = true; > + > + device_links_read_unlock(idx); > +} > + > static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev, > pm_message_t state, > const char **info_p) > @@ -1511,11 +1537,17 @@ static int __device_suspend_late(struct > if (dev->power.syscore || dev->power.direct_complete) > goto Complete; > > + if ((state.event == PM_EVENT_SUSPEND || state.event == PM_EVENT_HIBERNATE) && > + dev_pm_test_driver_flags(dev, DPM_FLAG_IN_BAND_WAKEUP) && > + device_may_wakeup(dev)) > + dev->power.skip_suspend = true; > + > callback = dpm_subsys_suspend_late_cb(dev, state, &info); > if (callback) > goto Run; > > - if (dev_pm_smart_suspend_and_suspended(dev) && > + if ((dev_pm_smart_suspend_and_suspended(dev) || > + dev_pm_skip_suspend_and_not_suspended(dev)) && > !dpm_subsys_suspend_noirq_cb(dev, state, NULL)) > goto Skip; > > @@ -1534,6 +1566,9 @@ Run: > Skip: > dev->power.is_late_suspended = true; > > + if (dev->power.skip_suspend) > + dpm_superior_set_skip_suspend(dev); > + > Complete: > TRACE_SUSPEND(error); > complete_all(&dev->power.completion); > @@ -1746,6 +1781,7 @@ static int __device_suspend(struct devic > > dev->power.may_skip_resume = false; > dev->power.must_resume = false; > + dev->power.skip_suspend = false; > > dpm_watchdog_set(&wd, dev); > device_lock(dev); > @@ -2112,6 +2148,11 @@ void device_pm_check_callbacks(struct de > spin_unlock_irq(&dev->power.lock); > } > > +bool dev_pm_skip_suspend_and_not_suspended(struct device *dev) > +{ > + return dev->power.skip_suspend && !pm_runtime_status_suspended(dev); > +} > + > bool dev_pm_smart_suspend_and_suspended(struct device *dev) > { > return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) && >
Hi Rafael, Ulf, On Sun, Dec 10, 2017 at 3:30 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote: >> For some bus types and PM domains, it's not sufficient to only check the >> return value from device_may_wakeup(), to fully understand how to configure >> wakeup settings for the device during system suspend. >> >> In particular, sometimes the device may need to remain in its power state, >> in case the driver has configured it for in band IRQs, as to be able to >> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP >> driver flag, to enable drivers to instruct bus types and PM domains about >> this setting. >> >> Of course, in case the bus type and PM domain has additional information >> about wakeup settings of the device, they may override the behaviour. >> >> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus >> types and PM domains should treat the device's parent during system >> suspend. Therefore, in __device_suspend(), let the PM core propagate the >> wakeup setting by using a status flag in the struct dev_pm_info for the >> parent. This also makes it consistent with how the existing "wakeup_path" >> status flag is being assigned. > > I've been thinking about this quite a bit recently and my conclusion is that > the flag makes perfect sence (as it covers a valid use case), but I would > define it and design the handling of it a bit differently. I'm also still thinking about this, cfr. my recent silence w.r.t. these matters... On Renesas ARM SoCs (and at least some SH SoCs, but these are stuck in the pre-DT legacy clock domain), a device needs to be kept running if it is a wake-up source (e.g. WoL), or if it's part of the wake-up patch (e.g. irqchip). So in the absence of the GENPD_FLAG_ACTIVE_WAKEUP flag in the PM Domain driver, all individual drivers would need to set the IN_BAND_WAKEUP flag (but see the exception below) Hence for this class of SoCs, this would duplicate the existing dev->power.wakeup and dev->power.wakeup_path flags, so that's why I would prefer using GENPD_FLAG_ACTIVE_WAKEUP instead (like we already do for the older SH-Mobile SoCs, but not for newer R-Car and RZ series). For other SoC families, the situation may be different. For us, the only exception are devices where the wakeup-source is not the device itself, but a GPIO, e.g. SDHI card detect. In such a case, only the device driver knows if the device is to be woken up through a dedicated CD line, or through a GPIO CD. So that would call for a method to opt-out, e.g. OUT_BAND_WAKEUP. To complicate matters, some drivers may be used on SoCs where the device needs to be kept running (clock and/or power domain), and on SoCs where the device is always running. This difference is typically handled by genpd, and the device driver may not even be aware. Of course the driver can just set IN_BAND_WAKEUP regardless, (else it has to check for the presence of clocks and/or power-domains properties itself, duplicating genpd core/driver code). So what about if (IN_BAND_WAKEUP || (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) { ... suspend device... } ? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Rafael, Ulf, > > On Sun, Dec 10, 2017 at 3:30 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote: >>> For some bus types and PM domains, it's not sufficient to only check the >>> return value from device_may_wakeup(), to fully understand how to configure >>> wakeup settings for the device during system suspend. >>> >>> In particular, sometimes the device may need to remain in its power state, >>> in case the driver has configured it for in band IRQs, as to be able to >>> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP >>> driver flag, to enable drivers to instruct bus types and PM domains about >>> this setting. >>> >>> Of course, in case the bus type and PM domain has additional information >>> about wakeup settings of the device, they may override the behaviour. >>> >>> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus >>> types and PM domains should treat the device's parent during system >>> suspend. Therefore, in __device_suspend(), let the PM core propagate the >>> wakeup setting by using a status flag in the struct dev_pm_info for the >>> parent. This also makes it consistent with how the existing "wakeup_path" >>> status flag is being assigned. >> >> I've been thinking about this quite a bit recently and my conclusion is that >> the flag makes perfect sence (as it covers a valid use case), but I would >> define it and design the handling of it a bit differently. > > I'm also still thinking about this, cfr. my recent silence w.r.t. > these matters... > > On Renesas ARM SoCs (and at least some SH SoCs, but these are stuck in the > pre-DT legacy clock domain), a device needs to be kept running if it is a > wake-up source (e.g. WoL), or if it's part of the wake-up patch (e.g. > irqchip). So in the absence of the GENPD_FLAG_ACTIVE_WAKEUP flag in the PM > Domain driver, all individual drivers would need to set the IN_BAND_WAKEUP > flag (but see the exception below) Could you elaborate a bit about the wakeup-path? Let me start and you can fill in. In $subject patch, the PM core propagates the IN_BAND_WAKEUP flag of the device to its parent device, via the ->power.wakeup_path_in_band flag. That becomes additional new information, as we already has the PM core to propagate the value of device_may_wakeup() (if true) to the parent device, via the ->power.wakeup_path flag. I assume that isn't sufficient, because the wakeup path isn't reflected in the child/parent hierarchy? So, I guess this is about drivers who deals with wakeup enabled devices and which are consumers of other resources (irqchips for example). As part of the wakup path, these resources also needs to be kept running, right? In your case, what other resources besides the irqchip, can be considered as a part of the wakeup path, but also being outside the parent/child hierarchy of the wakeup enabled device? Note that, an important fact as of today, is that when GENPD_FLAG_ACTIVE_WAKEUP is set for the genpd, genpd also checks the ->power.wakeup_path flag for the device. Both conditions must be true, else genpd powers off the PM domain (and the device). In other words, all devices being part of the wakeup path must either have device_may_wakeup() to return true for it or the ->power.wakeup_path set, else genpd will power off the PM domain, regardless of whether the GENPD_FLAG_ACTIVE_WAKEUP flag is set or not. > Hence for this class of SoCs, this would duplicate the existing > dev->power.wakeup and dev->power.wakeup_path flags, so that's why I would > prefer using GENPD_FLAG_ACTIVE_WAKEUP instead (like we already do for the > older SH-Mobile SoCs, but not for newer R-Car and RZ series). > For other SoC families, the situation may be different. > > For us, the only exception are devices where the wakeup-source is not the > device itself, but a GPIO, e.g. SDHI card detect. In such a case, only the > device driver knows if the device is to be woken up through a dedicated CD > line, or through a GPIO CD. So that would call for a method to opt-out, > e.g. OUT_BAND_WAKEUP. The main reason to why I chosen the IN_BAND_WAKEUP method, is because genpd's *default* method is to power off the PM domain (and the device) even if wakeup is enabled for the device. Hence, genpd treats all wakeups as default being out-of-band IRQs. Having an OUT_BAND_WAKEUP flag, would thus only make sense for those genpds that has GENPD_FLAG_ACTIVE_WAKEUP set, I guess that is what you are saying as well? > > To complicate matters, some drivers may be used on SoCs where the device > needs to be kept running (clock and/or power domain), and on SoCs where the > device is always running. This difference is typically handled by genpd, > and the device driver may not even be aware. Of course the driver can just > set IN_BAND_WAKEUP regardless, (else it has to check for the presence of > clocks and/or power-domains properties itself, duplicating genpd > core/driver code). > > So what about > > if (IN_BAND_WAKEUP || > (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) { We don't want to suspend the device in case of IN_BAND_WAKEUP, right!? > ... suspend device... > } Kind regards Uffe
Hi Ulf, On Mon, Dec 11, 2017 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Sun, Dec 10, 2017 at 3:30 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>> On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote: >>>> For some bus types and PM domains, it's not sufficient to only check the >>>> return value from device_may_wakeup(), to fully understand how to configure >>>> wakeup settings for the device during system suspend. >>>> >>>> In particular, sometimes the device may need to remain in its power state, >>>> in case the driver has configured it for in band IRQs, as to be able to >>>> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP >>>> driver flag, to enable drivers to instruct bus types and PM domains about >>>> this setting. >>>> >>>> Of course, in case the bus type and PM domain has additional information >>>> about wakeup settings of the device, they may override the behaviour. >>>> >>>> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus >>>> types and PM domains should treat the device's parent during system >>>> suspend. Therefore, in __device_suspend(), let the PM core propagate the >>>> wakeup setting by using a status flag in the struct dev_pm_info for the >>>> parent. This also makes it consistent with how the existing "wakeup_path" >>>> status flag is being assigned. >>> >>> I've been thinking about this quite a bit recently and my conclusion is that >>> the flag makes perfect sence (as it covers a valid use case), but I would >>> define it and design the handling of it a bit differently. >> >> I'm also still thinking about this, cfr. my recent silence w.r.t. >> these matters... >> >> On Renesas ARM SoCs (and at least some SH SoCs, but these are stuck in the >> pre-DT legacy clock domain), a device needs to be kept running if it is a >> wake-up source (e.g. WoL), or if it's part of the wake-up patch (e.g. >> irqchip). So in the absence of the GENPD_FLAG_ACTIVE_WAKEUP flag in the PM >> Domain driver, all individual drivers would need to set the IN_BAND_WAKEUP >> flag (but see the exception below) > > Could you elaborate a bit about the wakeup-path? Let me start and you > can fill in. > > In $subject patch, the PM core propagates the IN_BAND_WAKEUP flag of > the device to its parent device, via the ->power.wakeup_path_in_band > flag. That becomes additional new information, as we already has the > PM core to propagate the value of device_may_wakeup() (if true) to the > parent device, via the ->power.wakeup_path flag. > > I assume that isn't sufficient, because the wakeup path isn't > reflected in the child/parent hierarchy? That's correct. Interrupts are not part of the parent/child relationship. Traditionally, irqchips were not platform devices, but now many irqchips used on embedded platforms are. > So, I guess this is about drivers who deals with wakeup enabled > devices and which are consumers of other resources (irqchips for > example). As part of the wakup path, these resources also needs to be > kept running, right? Correct. > In your case, what other resources besides the irqchip, can be > considered as a part of the wakeup path, but also being outside the > parent/child hierarchy of the wakeup enabled device? GPIOs used for wake-up? E.g. SDHI GPIO. We don't seem to have that enabled for the Renesas SDHI drivers, though. For gpio-keys, this is handled through enable_irq_wake(), so again it uses an irqchip. > Note that, an important fact as of today, is that when > GENPD_FLAG_ACTIVE_WAKEUP is set for the genpd, genpd also checks the > ->power.wakeup_path flag for the device. Both conditions must be true, > else genpd powers off the PM domain (and the device). > > In other words, all devices being part of the wakeup path must either > have device_may_wakeup() to return true for it or the > ->power.wakeup_path set, else genpd will power off the PM domain, > regardless of whether the GENPD_FLAG_ACTIVE_WAKEUP flag is set or not. Correct. >> Hence for this class of SoCs, this would duplicate the existing >> dev->power.wakeup and dev->power.wakeup_path flags, so that's why I would >> prefer using GENPD_FLAG_ACTIVE_WAKEUP instead (like we already do for the >> older SH-Mobile SoCs, but not for newer R-Car and RZ series). >> For other SoC families, the situation may be different. >> >> For us, the only exception are devices where the wakeup-source is not the >> device itself, but a GPIO, e.g. SDHI card detect. In such a case, only the >> device driver knows if the device is to be woken up through a dedicated CD >> line, or through a GPIO CD. So that would call for a method to opt-out, >> e.g. OUT_BAND_WAKEUP. > > The main reason to why I chosen the IN_BAND_WAKEUP method, is because > genpd's *default* method is to power off the PM domain (and the > device) even if wakeup is enabled for the device. Hence, genpd treats > all wakeups as default being out-of-band IRQs. OK. > Having an OUT_BAND_WAKEUP flag, would thus only make sense for those > genpds that has GENPD_FLAG_ACTIVE_WAKEUP set, I guess that is what you > are saying as well? Yes. IMHO this is orthogonal to the device: the device driver knows if the device itself is generating the wake-up event (e.g. Wake-on-LAN, dedicated SDHI CD), or some other device is taking care (e.g. GPIO SDHI CD). The device driver may not know if the device needs to be kept awake to to handle wake-up events, that may be platform knowledge handled by genpd. >> To complicate matters, some drivers may be used on SoCs where the device >> needs to be kept running (clock and/or power domain), and on SoCs where the >> device is always running. This difference is typically handled by genpd, >> and the device driver may not even be aware. Of course the driver can just >> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of >> clocks and/or power-domains properties itself, duplicating genpd >> core/driver code). >> >> So what about >> >> if (IN_BAND_WAKEUP || >> (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) { > > We don't want to suspend the device in case of IN_BAND_WAKEUP, right!? > >> ... suspend device... >> } Oops, inverted logic. I should not write technical emails on Sunday morning. Yes, the device must be kept awake if either IN_BAND_WAKEUP is set, or if GENPD_FLAG_ACTIVE_WAKEUP is set but OUT_BAND_WAKEUP isn't. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 11 December 2017 at 11:48, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Ulf, > > On Mon, Dec 11, 2017 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> On Sun, Dec 10, 2017 at 3:30 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>>> On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote: >>>>> For some bus types and PM domains, it's not sufficient to only check the >>>>> return value from device_may_wakeup(), to fully understand how to configure >>>>> wakeup settings for the device during system suspend. >>>>> >>>>> In particular, sometimes the device may need to remain in its power state, >>>>> in case the driver has configured it for in band IRQs, as to be able to >>>>> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP >>>>> driver flag, to enable drivers to instruct bus types and PM domains about >>>>> this setting. >>>>> >>>>> Of course, in case the bus type and PM domain has additional information >>>>> about wakeup settings of the device, they may override the behaviour. >>>>> >>>>> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus >>>>> types and PM domains should treat the device's parent during system >>>>> suspend. Therefore, in __device_suspend(), let the PM core propagate the >>>>> wakeup setting by using a status flag in the struct dev_pm_info for the >>>>> parent. This also makes it consistent with how the existing "wakeup_path" >>>>> status flag is being assigned. >>>> >>>> I've been thinking about this quite a bit recently and my conclusion is that >>>> the flag makes perfect sence (as it covers a valid use case), but I would >>>> define it and design the handling of it a bit differently. >>> >>> I'm also still thinking about this, cfr. my recent silence w.r.t. >>> these matters... >>> >>> On Renesas ARM SoCs (and at least some SH SoCs, but these are stuck in the >>> pre-DT legacy clock domain), a device needs to be kept running if it is a >>> wake-up source (e.g. WoL), or if it's part of the wake-up patch (e.g. >>> irqchip). So in the absence of the GENPD_FLAG_ACTIVE_WAKEUP flag in the PM >>> Domain driver, all individual drivers would need to set the IN_BAND_WAKEUP >>> flag (but see the exception below) >> >> Could you elaborate a bit about the wakeup-path? Let me start and you >> can fill in. >> >> In $subject patch, the PM core propagates the IN_BAND_WAKEUP flag of >> the device to its parent device, via the ->power.wakeup_path_in_band >> flag. That becomes additional new information, as we already has the >> PM core to propagate the value of device_may_wakeup() (if true) to the >> parent device, via the ->power.wakeup_path flag. >> >> I assume that isn't sufficient, because the wakeup path isn't >> reflected in the child/parent hierarchy? > > That's correct. Interrupts are not part of the parent/child relationship. > Traditionally, irqchips were not platform devices, but now many irqchips > used on embedded platforms are. > >> So, I guess this is about drivers who deals with wakeup enabled >> devices and which are consumers of other resources (irqchips for >> example). As part of the wakup path, these resources also needs to be >> kept running, right? > > Correct. > >> In your case, what other resources besides the irqchip, can be >> considered as a part of the wakeup path, but also being outside the >> parent/child hierarchy of the wakeup enabled device? > > GPIOs used for wake-up? E.g. SDHI GPIO. We don't seem to have that > enabled for the Renesas SDHI drivers, though. > > For gpio-keys, this is handled through enable_irq_wake(), so again it > uses an irqchip. Yes, this is clearly a common use case. Although, I am sure we have more. For example, phys may need a very similar treatment, in case their consumers requires its phy to be running - as to be able to receive signals for wakeups. Anyway, let's discuss other cases separately and focus on yours with the irqchip for now. > >> Note that, an important fact as of today, is that when >> GENPD_FLAG_ACTIVE_WAKEUP is set for the genpd, genpd also checks the >> ->power.wakeup_path flag for the device. Both conditions must be true, >> else genpd powers off the PM domain (and the device). >> >> In other words, all devices being part of the wakeup path must either >> have device_may_wakeup() to return true for it or the >> ->power.wakeup_path set, else genpd will power off the PM domain, >> regardless of whether the GENPD_FLAG_ACTIVE_WAKEUP flag is set or not. > > Correct. Okay, thanks for confirming this. This also clarifies why you set the ->power.wakeup_path flag for the renesas irqchip device in your other series [1]. That makes perfect sense to me, in this regards. > >>> Hence for this class of SoCs, this would duplicate the existing >>> dev->power.wakeup and dev->power.wakeup_path flags, so that's why I would >>> prefer using GENPD_FLAG_ACTIVE_WAKEUP instead (like we already do for the >>> older SH-Mobile SoCs, but not for newer R-Car and RZ series). >>> For other SoC families, the situation may be different. >>> >>> For us, the only exception are devices where the wakeup-source is not the >>> device itself, but a GPIO, e.g. SDHI card detect. In such a case, only the >>> device driver knows if the device is to be woken up through a dedicated CD >>> line, or through a GPIO CD. So that would call for a method to opt-out, >>> e.g. OUT_BAND_WAKEUP. >> >> The main reason to why I chosen the IN_BAND_WAKEUP method, is because >> genpd's *default* method is to power off the PM domain (and the >> device) even if wakeup is enabled for the device. Hence, genpd treats >> all wakeups as default being out-of-band IRQs. > > OK. > >> Having an OUT_BAND_WAKEUP flag, would thus only make sense for those >> genpds that has GENPD_FLAG_ACTIVE_WAKEUP set, I guess that is what you >> are saying as well? > > Yes. > > IMHO this is orthogonal to the device: the device driver knows if the > device itself is generating the wake-up event (e.g. Wake-on-LAN, dedicated > SDHI CD), or some other device is taking care (e.g. GPIO SDHI CD). > The device driver may not know if the device needs to be kept awake to > to handle wake-up events, that may be platform knowledge handled by genpd. Agree. > >>> To complicate matters, some drivers may be used on SoCs where the device >>> needs to be kept running (clock and/or power domain), and on SoCs where the >>> device is always running. This difference is typically handled by genpd, >>> and the device driver may not even be aware. Of course the driver can just >>> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of >>> clocks and/or power-domains properties itself, duplicating genpd >>> core/driver code). >>> >>> So what about >>> >>> if (IN_BAND_WAKEUP || >>> (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) { >> >> We don't want to suspend the device in case of IN_BAND_WAKEUP, right!? >> >>> ... suspend device... >>> } > > Oops, inverted logic. I should not write technical emails on Sunday morning. > > Yes, the device must be kept awake if either IN_BAND_WAKEUP is set, or > if GENPD_FLAG_ACTIVE_WAKEUP is set but OUT_BAND_WAKEUP isn't. > Putting together the pieces of information received here, you have convinced me that we should stick to use the current GENPD_FLAG_ACTIVE_WAKEUP for now, which allows genpds to opt-in for start dealing with in-band-wakeups. Then, as you suggest, we need a method for the driver to set an opt-out flag for its device, in case it configures an out-band-wakeup. In other words, we should have an OUT_BAND_WAKEUP flag instead of an IN_BAND_WAKEUP flag. That together with an option of allowing "consumed resource-devices" (irqchip) to be included in the wakeup path. I am thinking, perhaps another driver PM flag (DPM_FLAG_WAKEUP_PATH), that the PM core looks at and sets ->power.wakeup_path flag for the device and its parents. Let me re-spin this series. Perhaps I fold in some of the changes from your series as well, as to provide people with a complete picture. Any further comments? Does it makes sense? Kind regards Uffe [1] https://www.spinics.net/lists/linux-renesas-soc/msg19947.html
Hi Ulf, On Mon, Dec 11, 2017 at 9:59 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > That together with an option of allowing "consumed resource-devices" > (irqchip) to be included in the wakeup path. I am thinking, perhaps > another driver PM flag (DPM_FLAG_WAKEUP_PATH), that the PM core looks > at and sets ->power.wakeup_path flag for the device and its parents. This is complicated by the fact that currently the device and irq subsystems don't really share data. E.g. {en,dis}able_irq_wake() and irq_set_irq_wake() don't take a device parameter, only an interrupt number, and conversion from interrupt numbers to devices is non-trivial. That's why I handled it in the irqchip drivers in the series referenced below. > [1] > https://www.spinics.net/lists/linux-renesas-soc/msg19947.html Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 12 December 2017 at 09:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Ulf, > > On Mon, Dec 11, 2017 at 9:59 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> That together with an option of allowing "consumed resource-devices" >> (irqchip) to be included in the wakeup path. I am thinking, perhaps >> another driver PM flag (DPM_FLAG_WAKEUP_PATH), that the PM core looks >> at and sets ->power.wakeup_path flag for the device and its parents. > > This is complicated by the fact that currently the device and irq > subsystems don't really share data. E.g. {en,dis}able_irq_wake() and > irq_set_irq_wake() don't take a device parameter, only an interrupt number, > and conversion from interrupt numbers to devices is non-trivial. > That's why I handled it in the irqchip drivers in the series referenced > below. Yes, that seems like a good way forward for now. We can always look into how to extend the irq subsystem to deal with this internally, but I rather leave that to be done as a separate task. [...] Kind regards Uffe
Hi Ulf, On Mon, Dec 11, 2017 at 9:59 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 11 December 2017 at 11:48, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Mon, Dec 11, 2017 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>>> To complicate matters, some drivers may be used on SoCs where the device >>>> needs to be kept running (clock and/or power domain), and on SoCs where the >>>> device is always running. This difference is typically handled by genpd, >>>> and the device driver may not even be aware. Of course the driver can just >>>> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of >>>> clocks and/or power-domains properties itself, duplicating genpd >>>> core/driver code). >>>> >>>> So what about >>>> >>>> if (IN_BAND_WAKEUP || >>>> (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) { >>> >>> We don't want to suspend the device in case of IN_BAND_WAKEUP, right!? >>> >>>> ... suspend device... >>>> } >> >> Oops, inverted logic. I should not write technical emails on Sunday morning. >> >> Yes, the device must be kept awake if either IN_BAND_WAKEUP is set, or >> if GENPD_FLAG_ACTIVE_WAKEUP is set but OUT_BAND_WAKEUP isn't. > > Putting together the pieces of information received here, you have > convinced me that we should stick to use the current > GENPD_FLAG_ACTIVE_WAKEUP for now, which allows genpds to opt-in for > start dealing with in-band-wakeups. Thank you! So I'll move forward with "[PATCH v2 0/3] PM / Domain: renesas: Fix active wakeup behavior" (https://www.spinics.net/lists/linux-renesas-soc/msg19941.html) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 14 December 2017 at 11:52, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Ulf, > > On Mon, Dec 11, 2017 at 9:59 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 11 December 2017 at 11:48, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> On Mon, Dec 11, 2017 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>>>> To complicate matters, some drivers may be used on SoCs where the device >>>>> needs to be kept running (clock and/or power domain), and on SoCs where the >>>>> device is always running. This difference is typically handled by genpd, >>>>> and the device driver may not even be aware. Of course the driver can just >>>>> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of >>>>> clocks and/or power-domains properties itself, duplicating genpd >>>>> core/driver code). >>>>> >>>>> So what about >>>>> >>>>> if (IN_BAND_WAKEUP || >>>>> (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) { >>>> >>>> We don't want to suspend the device in case of IN_BAND_WAKEUP, right!? >>>> >>>>> ... suspend device... >>>>> } >>> >>> Oops, inverted logic. I should not write technical emails on Sunday morning. >>> >>> Yes, the device must be kept awake if either IN_BAND_WAKEUP is set, or >>> if GENPD_FLAG_ACTIVE_WAKEUP is set but OUT_BAND_WAKEUP isn't. >> >> Putting together the pieces of information received here, you have >> convinced me that we should stick to use the current >> GENPD_FLAG_ACTIVE_WAKEUP for now, which allows genpds to opt-in for >> start dealing with in-band-wakeups. > > Thank you! > > So I'll move forward with "[PATCH v2 0/3] PM / Domain: renesas: Fix active > wakeup behavior" > (https://www.spinics.net/lists/linux-renesas-soc/msg19941.html) Yes! I just added my reviewed-by tag to these. Kind regards Uffe
Hi Ulf, On Thu, Dec 14, 2017 at 3:13 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 14 December 2017 at 11:52, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Mon, Dec 11, 2017 at 9:59 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> On 11 December 2017 at 11:48, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>>> On Mon, Dec 11, 2017 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>>> On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>>>>> To complicate matters, some drivers may be used on SoCs where the device >>>>>> needs to be kept running (clock and/or power domain), and on SoCs where the >>>>>> device is always running. This difference is typically handled by genpd, >>>>>> and the device driver may not even be aware. Of course the driver can just >>>>>> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of >>>>>> clocks and/or power-domains properties itself, duplicating genpd >>>>>> core/driver code). >>>>>> >>>>>> So what about >>>>>> >>>>>> if (IN_BAND_WAKEUP || >>>>>> (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) { >>>>> >>>>> We don't want to suspend the device in case of IN_BAND_WAKEUP, right!? >>>>> >>>>>> ... suspend device... >>>>>> } >>>> >>>> Oops, inverted logic. I should not write technical emails on Sunday morning. >>>> >>>> Yes, the device must be kept awake if either IN_BAND_WAKEUP is set, or >>>> if GENPD_FLAG_ACTIVE_WAKEUP is set but OUT_BAND_WAKEUP isn't. >>> >>> Putting together the pieces of information received here, you have >>> convinced me that we should stick to use the current >>> GENPD_FLAG_ACTIVE_WAKEUP for now, which allows genpds to opt-in for >>> start dealing with in-band-wakeups. >> >> Thank you! >> >> So I'll move forward with "[PATCH v2 0/3] PM / Domain: renesas: Fix active >> wakeup behavior" >> (https://www.spinics.net/lists/linux-renesas-soc/msg19941.html) > > Yes! I just added my reviewed-by tag to these. Thanks a lot! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst index 53c1b0b..b7fddf2 100644 --- a/Documentation/driver-api/pm/devices.rst +++ b/Documentation/driver-api/pm/devices.rst @@ -414,6 +414,17 @@ when the system is in the sleep state. For example, :c:func:`enable_irq_wake()` might identify GPIO signals hooked up to a switch or other external hardware, and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal. +Sometimes :c:func:`device_may_wakeup(dev)` is not sufficient to understand how a +wakeup signal needs to be configured. Particularly, in some cases the driver +needs to instruct bus types and PM domains, that it has configured the device to +use an in-band wakeup setting, which may require the device to remain in its +power state to be able to generate wakeup signals. Therefore, drivers can set +``DPM_FLAG_IN_BAND_WAKEUP`` in :c:member:`power.driver_flags`, by passing the +flag to :c:func:`dev_pm_set_driver_flags` helper, which instructs bus types and +PM domains to use this configuration. However, the bus types and PM domains may +override this setting, in case they have additional information about wakeup +settings of the device. + If any of these callbacks returns an error, the system won't enter the desired low-power state. Instead, the PM core will unwind its actions by resuming all the devices that were suspended. diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 8089e72..8ec3189 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -1432,8 +1432,11 @@ static void dpm_propagate_to_parent(struct device *dev) spin_lock_irq(&parent->power.lock); parent->power.direct_complete = false; - if (dev->power.wakeup_path && !parent->power.ignore_children) + if (dev->power.wakeup_path && !parent->power.ignore_children) { parent->power.wakeup_path = true; + parent->power.wakeup_path_in_band = + dev->power.wakeup_path_in_band; + } spin_unlock_irq(&parent->power.lock); } @@ -1547,6 +1550,8 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async) End: if (!error) { dev->power.is_suspended = true; + if (dev_pm_test_driver_flags(dev, DPM_FLAG_IN_BAND_WAKEUP)) + dev->power.wakeup_path_in_band = true; dpm_propagate_to_parent(dev); dpm_clear_suppliers_direct_complete(dev); } @@ -1671,6 +1676,7 @@ static int device_prepare(struct device *dev, pm_message_t state) device_lock(dev); dev->power.wakeup_path = device_may_wakeup(dev); + dev->power.wakeup_path_in_band = false; if (dev->power.no_pm_callbacks) { ret = 1; /* Let device go direct_complete */ diff --git a/include/linux/pm.h b/include/linux/pm.h index 65d3911..4efb16a 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -559,6 +559,7 @@ struct pm_subsys_data { * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device. * SMART_PREPARE: Check the return value of the driver's ->prepare callback. * SMART_SUSPEND: No need to resume the device from runtime suspend. + * WAKEUP_POWERED: Keep the device powered if it has wakeup enabled. * * Setting SMART_PREPARE instructs bus types and PM domains which may want * system suspend/resume callbacks to be skipped for the device to return 0 from @@ -572,10 +573,14 @@ struct pm_subsys_data { * necessary from the driver's perspective. It also may cause them to skip * invocations of the ->suspend_late and ->suspend_noirq callbacks provided by * the driver if they decide to leave the device in runtime suspend. + * + * Setting WAKEUP_POWERED instructs bus types and PM domains that the device + * needs to remain powered in system suspend, in case wakeup is enabled for it. */ #define DPM_FLAG_NEVER_SKIP BIT(0) #define DPM_FLAG_SMART_PREPARE BIT(1) #define DPM_FLAG_SMART_SUSPEND BIT(2) +#define DPM_FLAG_IN_BAND_WAKEUP BIT(3) struct dev_pm_info { pm_message_t power_state; @@ -595,6 +600,7 @@ struct dev_pm_info { struct completion completion; struct wakeup_source *wakeup; bool wakeup_path:1; + bool wakeup_path_in_band:1; bool syscore:1; bool no_pm_callbacks:1; /* Owned by the PM core */ #else
For some bus types and PM domains, it's not sufficient to only check the return value from device_may_wakeup(), to fully understand how to configure wakeup settings for the device during system suspend. In particular, sometimes the device may need to remain in its power state, in case the driver has configured it for in band IRQs, as to be able to generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP driver flag, to enable drivers to instruct bus types and PM domains about this setting. Of course, in case the bus type and PM domain has additional information about wakeup settings of the device, they may override the behaviour. Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus types and PM domains should treat the device's parent during system suspend. Therefore, in __device_suspend(), let the PM core propagate the wakeup setting by using a status flag in the struct dev_pm_info for the parent. This also makes it consistent with how the existing "wakeup_path" status flag is being assigned. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- Changes in v2: - Fixed comments from Rafael: - Rename flag to IN_BAND_WAKEUP. - Clarify doc and changelog. - Use a separate status flag when propagating to parents in PM core. - I didn't add Geert's Reviewed-by, because of new changes. --- Documentation/driver-api/pm/devices.rst | 11 +++++++++++ drivers/base/power/main.c | 8 +++++++- include/linux/pm.h | 6 ++++++ 3 files changed, 24 insertions(+), 1 deletion(-) -- 2.7.4