| Message ID | 2298462.BTY6P2Oy1l@aspire.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Mon, Oct 16, 2017 at 03:31:17AM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Optimize the power management in i2c-designware-platdrv by making it > set the DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED which > allows some code to be dropped from its PM callbacks. > > First, setting DPM_FLAG_SMART_SUSPEND causes the intel-lpss driver > to avoid resuming i2c-designware-platdrv devices in its ->prepare > callback, so they can stay in runtime suspend after that point even > if the direct-complete feature is not used for them. > > It also causes the PM core to avoid invoking "late" and "noirq" > suspend callbacks for these devices if they are in runtime suspend > at the beginning of the "late" phase of device suspend during > system suspend. That guarantees dw_i2c_plat_suspend() to be > called for a device only if it is not in runtime suspend. > Moreover, it also causes the PM core to set the device's runtime > PM status to "active" after calling dw_i2c_plat_resume() for > it, so the driver doesn't need internal flags to avoid invoking > either dw_i2c_plat_suspend() or dw_i2c_plat_resume() twice in > a row. > > Second, setting DPM_FLAG_LEAVE_SUSPENDED enables the optimization > allowing the device to stay suspended after system resume under > suitable conditions, so again the driver doesn't need to take > care of that by itself. > > Accordingly, the internal "suspended" and "skip_resume" flags > used by the driver are not necessary any more, so drop them and > simplify the driver's PM callbacks. > > Additionally, notice that dw_i2c_plat_complete() only needs > to schedule runtime PM for the device if platform firmware > has been involved in resuming the system, so make it call > pm_resume_via_firmware() to check that. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> So, if the designware maintainers ack it, I will, too.
On Thursday, October 26, 2017 10:41:40 PM CEST Wolfram Sang wrote: > On Mon, Oct 16, 2017 at 03:31:17AM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Optimize the power management in i2c-designware-platdrv by making it > > set the DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED which > > allows some code to be dropped from its PM callbacks. > > > > First, setting DPM_FLAG_SMART_SUSPEND causes the intel-lpss driver > > to avoid resuming i2c-designware-platdrv devices in its ->prepare > > callback, so they can stay in runtime suspend after that point even > > if the direct-complete feature is not used for them. > > > > It also causes the PM core to avoid invoking "late" and "noirq" > > suspend callbacks for these devices if they are in runtime suspend > > at the beginning of the "late" phase of device suspend during > > system suspend. That guarantees dw_i2c_plat_suspend() to be > > called for a device only if it is not in runtime suspend. > > Moreover, it also causes the PM core to set the device's runtime > > PM status to "active" after calling dw_i2c_plat_resume() for > > it, so the driver doesn't need internal flags to avoid invoking > > either dw_i2c_plat_suspend() or dw_i2c_plat_resume() twice in > > a row. > > > > Second, setting DPM_FLAG_LEAVE_SUSPENDED enables the optimization > > allowing the device to stay suspended after system resume under > > suitable conditions, so again the driver doesn't need to take > > care of that by itself. > > > > Accordingly, the internal "suspended" and "skip_resume" flags > > used by the driver are not necessary any more, so drop them and > > simplify the driver's PM callbacks. > > > > Additionally, notice that dw_i2c_plat_complete() only needs > > to schedule runtime PM for the device if platform firmware > > has been involved in resuming the system, so make it call > > pm_resume_via_firmware() to check that. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > So, if the designware maintainers ack it, I will, too. Thanks! I need to post a new revision of the core patches, so I'll send this one again later. Likely during the next cycle. Thanks, Rafael
Index: linux-pm/drivers/i2c/busses/i2c-designware-core.h =================================================================== --- linux-pm.orig/drivers/i2c/busses/i2c-designware-core.h +++ linux-pm/drivers/i2c/busses/i2c-designware-core.h @@ -280,8 +280,6 @@ struct dw_i2c_dev { int (*acquire_lock)(struct dw_i2c_dev *dev); void (*release_lock)(struct dw_i2c_dev *dev); bool pm_disabled; - bool suspended; - bool skip_resume; void (*disable)(struct dw_i2c_dev *dev); void (*disable_int)(struct dw_i2c_dev *dev); int (*init)(struct dw_i2c_dev *dev); Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c =================================================================== --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c @@ -42,6 +42,7 @@ #include <linux/reset.h> #include <linux/sched.h> #include <linux/slab.h> +#include <linux/suspend.h> #include "i2c-designware-core.h" @@ -370,7 +371,10 @@ static int dw_i2c_plat_probe(struct plat ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); adap->dev.of_node = pdev->dev.of_node; - dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_SMART_PREPARE); + dev_pm_set_driver_flags(&pdev->dev, + DPM_FLAG_SMART_PREPARE | + DPM_FLAG_SMART_SUSPEND | + DPM_FLAG_LEAVE_SUSPENDED); /* The code below assumes runtime PM to be disabled. */ WARN_ON(pm_runtime_enabled(&pdev->dev)); @@ -446,7 +450,7 @@ static int dw_i2c_plat_prepare(struct de static void dw_i2c_plat_complete(struct device *dev) { - if (dev->power.direct_complete) + if (dev->power.direct_complete && pm_resume_via_firmware()) pm_request_resume(dev); } #else @@ -459,16 +463,9 @@ static int dw_i2c_plat_suspend(struct de { struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); - if (i_dev->suspended) { - i_dev->skip_resume = true; - return 0; - } - i_dev->disable(i_dev); i2c_dw_plat_prepare_clk(i_dev, false); - i_dev->suspended = true; - return 0; } @@ -476,19 +473,9 @@ static int dw_i2c_plat_resume(struct dev { struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); - if (!i_dev->suspended) - return 0; - - if (i_dev->skip_resume) { - i_dev->skip_resume = false; - return 0; - } - i2c_dw_plat_prepare_clk(i_dev, true); i_dev->init(i_dev); - i_dev->suspended = false; - return 0; }