| Message ID | 1465916848-8207-11-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote: > The current code that deploys the system PM support relies on the > "direct_complete" feature supported by the PM core. The goal is to > avoid > performing unnecessary operations during the system PM sequence. > > > static const struct dev_pm_ops dw_i2c_dev_pm_ops = { > - .prepare = dw_i2c_plat_prepare, > - .complete = dw_i2c_plat_complete, > - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, > dw_i2c_plat_resume) > - SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, > NULL) > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) > + SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend, > + dw_i2c_plat_runtime_resume, > + NULL) > }; UNIVERSAL_PM_OPS ?
On 14 June 2016 at 17:35, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote: >> The current code that deploys the system PM support relies on the >> "direct_complete" feature supported by the PM core. The goal is to >> avoid >> performing unnecessary operations during the system PM sequence. >> > >> >> static const struct dev_pm_ops dw_i2c_dev_pm_ops = { >> - .prepare = dw_i2c_plat_prepare, >> - .complete = dw_i2c_plat_complete, >> - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, >> dw_i2c_plat_resume) >> - SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, >> NULL) >> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >> + pm_runtime_force_resume) >> + SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend, >> + dw_i2c_plat_runtime_resume, >> + NULL) >> }; > > UNIVERSAL_PM_OPS ? The UNIVERSAL_DEV_PM_OPS assigns the same callbacks for system PM as runtime PM, that doesn't work here. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index b2c6037..4c31ff3 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -291,24 +291,8 @@ static const struct of_device_id dw_i2c_of_match[] = { MODULE_DEVICE_TABLE(of, dw_i2c_of_match); #endif -#ifdef CONFIG_PM_SLEEP -static int dw_i2c_plat_prepare(struct device *dev) -{ - return pm_runtime_suspended(dev); -} - -static void dw_i2c_plat_complete(struct device *dev) -{ - if (dev->power.direct_complete) - pm_request_resume(dev); -} -#else -#define dw_i2c_plat_prepare NULL -#define dw_i2c_plat_complete NULL -#endif - #ifdef CONFIG_PM -static int dw_i2c_plat_suspend(struct device *dev) +static int dw_i2c_plat_runtime_suspend(struct device *dev) { struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); @@ -320,7 +304,7 @@ static int dw_i2c_plat_suspend(struct device *dev) return 0; } -static int dw_i2c_plat_resume(struct device *dev) +static int dw_i2c_plat_runtime_resume(struct device *dev) { struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); int ret; @@ -336,10 +320,11 @@ static int dw_i2c_plat_resume(struct device *dev) } static const struct dev_pm_ops dw_i2c_dev_pm_ops = { - .prepare = dw_i2c_plat_prepare, - .complete = dw_i2c_plat_complete, - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume) - SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) + SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend, + dw_i2c_plat_runtime_resume, + NULL) }; #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
The current code that deploys the system PM support relies on the "direct_complete" feature supported by the PM core. The goal is to avoid performing unnecessary operations during the system PM sequence. Unfortunate in this case there are some drawbacks with this solution as explained below. 1) In case of the ->prepare() callback find the device runtime resumed it returns 0. The PM core will then run the regular set of the system PM callbacks for the device, to allow it to be suspended. Under these circumstances the device also becomes unconditionally resumed during the system PM resume sequence. It would be better to postpone the resume operations to be managed by runtime PM and thus only when actually needed. 2) It's good practice to keep the device's runtime PM status in sync with the the current state of the HW. In the same scenario as described in 1), the runtime PM status are RPM_ACTIVE the period in-between when the ->suspend() and ->resume() callbacks are invoked. This is wrong because the device has actually been put into a low power state. To address the limitation in 2) and to simplify the system PM code, let's convert to deploy the so called runtime PM centric approach. This is done by assigning the system PM ->suspend|resume() callbacks to the pm_runtime_force_suspend|resume() helper functions. In this way, the ->prepare() and ->complete() callbacks can also be removed. Currently pm_runtime_force_resume() is also unconditionally resuming the device, which is due to legacy reasons when CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP co-existed. Changing that behaviour is a reasonable improvement to make and it would also resolve the limitation in 1). Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Kevin Hilman <khilman@kernel.org> Cc: linux-pm@vger.kernel.org Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/i2c/busses/i2c-designware-platdrv.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)