[RFT][PATCH 0/2] i2c: designware: Runtime PM aware system sleep handling

[RFT][PATCH 0/2] i2c: designware: Runtime PM aware system sleep handling

[Rafael J. Wysocki]

Hi,

The point here is to avoid runtime resuming i2c designware devices during
system suspend in the driver's ->suspend callback in the case when the
ACPI PM domain is not present.  That will allow us to deal with the
ACPI PM domain case going forward, among other things.

The first patch cleans up the runtime PM handling in the i2c-designware-platdrv
_probe() routine so as to make it enable runtime PM in all cases, but prevent
the device from being runtime suspended via pm_runtime_forbid() if
pm_disabled is set.

The second one get rids of some ugly code and makes the PM callbacks of the
driver handle runtime-suspended devices during system suspend/resume.

Please test if you can and let me know if anything breaks.

Thanks,
Rafael

[RFT][PATCH 1/2] i2c: designware: Clean up PM handling in dw_i2c_plat_probe()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The power management handling in dw_i2c_plat_probe() is somewhat
messy and it is rather hard to figure out the code intention for
the case when pm_disabled is set.  In that case, the driver doesn't
enable runtime PM at all, but in addition to that it calls
pm_runtime_forbid() as though it wasn't sure if runtime PM might
be enabled for the device later by someone else.

Although that concern doesn't seem to be actually valid, the
device is clearly still expected to be PM-capable even in the
pm_disabled set case, so a better approach would be to enable
runtime PM for it unconditionally and then prevent it from
being runtime suspended by using pm_runtime_forbid().

Make the driver do that as that will help to clean up its system
sleep handling in a relatively straightforward way.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |   36 +++++++++++++++++++---------
 1 file changed, 25 insertions(+), 11 deletions(-)

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
@@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct
 	}
 }
 
+static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
+{
+	pm_runtime_get_noresume(dev->dev);
+	if (dev->pm_disabled)
+		pm_runtime_allow(dev->dev);
+
+	pm_runtime_disable(dev->dev);
+	pm_runtime_put_noidle(dev->dev);
+}
+
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -362,14 +372,19 @@ 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;
 
-	if (dev->pm_disabled) {
+	/* The code below assumes runtime PM to be disabled. */
+	WARN_ON(pm_runtime_enabled(&pdev->dev));
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	if (dev->pm_disabled)
 		pm_runtime_forbid(&pdev->dev);
-	} else {
-		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-		pm_runtime_use_autosuspend(&pdev->dev);
-		pm_runtime_set_active(&pdev->dev);
-		pm_runtime_enable(&pdev->dev);
-	}
+
+	pm_runtime_put_noidle(&pdev->dev);
 
 	if (dev->mode == DW_IC_SLAVE)
 		ret = i2c_dw_probe_slave(dev);
@@ -382,8 +397,7 @@ static int dw_i2c_plat_probe(struct plat
 	return ret;
 
 exit_probe:
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
 exit_reset:
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
@@ -402,8 +416,8 @@ static int dw_i2c_plat_remove(struct pla
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
+
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
 

Re: [RFT][PATCH 1/2] i2c: designware: Clean up PM handling in dw_i2c_plat_probe()

[Jarkko Nikula]

Hi

On 09/04/2017 02:08 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The power management handling in dw_i2c_plat_probe() is somewhat
> messy and it is rather hard to figure out the code intention for
> the case when pm_disabled is set.  In that case, the driver doesn't
> enable runtime PM at all, but in addition to that it calls
> pm_runtime_forbid() as though it wasn't sure if runtime PM might
> be enabled for the device later by someone else.
> 
> Although that concern doesn't seem to be actually valid, the
> device is clearly still expected to be PM-capable even in the
> pm_disabled set case, so a better approach would be to enable
> runtime PM for it unconditionally and then prevent it from
> being runtime suspended by using pm_runtime_forbid().
> 
> Make the driver do that as that will help to clean up its system
> sleep handling in a relatively straightforward way.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c |   36 +++++++++++++++++++---------
>   1 file changed, 25 insertions(+), 11 deletions(-)
> 
> 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
> @@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct
>   	}
>   }
>   
> +static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
> +{
> +	pm_runtime_get_noresume(dev->dev);
> +	if (dev->pm_disabled)
> +		pm_runtime_allow(dev->dev);
> +
> +	pm_runtime_disable(dev->dev);
> +	pm_runtime_put_noidle(dev->dev);
> +}
> +
>   static int dw_i2c_plat_probe(struct platform_device *pdev)
>   {
>   	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -362,14 +372,19 @@ 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;
>   
> -	if (dev->pm_disabled) {
> +	/* The code below assumes runtime PM to be disabled. */
> +	WARN_ON(pm_runtime_enabled(&pdev->dev));
> +
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +	if (dev->pm_disabled)
>   		pm_runtime_forbid(&pdev->dev);
> -	} else {
> -		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> -		pm_runtime_use_autosuspend(&pdev->dev);
> -		pm_runtime_set_active(&pdev->dev);
> -		pm_runtime_enable(&pdev->dev);
> -	}
> +
> +	pm_runtime_put_noidle(&pdev->dev);
>   
Is pm_runtime_get_noresume()/pm_runtime_put_noidle() cycle needed here? 
My vague memory tells platform device won't power off instantly because 
of plain pm_runtime_enable() even if dev->power.usage_count is zero.

I guess it was the pm_request_idle() in driver_probe_device() that 
triggered the power transition after probe.

drivers/base/dd.c: driver_probe_device():
	pm_runtime_barrier(dev);
	ret = really_probe(dev, drv);
	pm_request_idle(dev);

-- 
Jarkko

Re: [RFT][PATCH 1/2] i2c: designware: Clean up PM handling in dw_i2c_plat_probe()

[Rafael J. Wysocki]

On Tuesday, September 5, 2017 4:40:22 PM CEST Jarkko Nikula wrote:
> Hi
> 
> On 09/04/2017 02:08 AM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > The power management handling in dw_i2c_plat_probe() is somewhat
> > messy and it is rather hard to figure out the code intention for
> > the case when pm_disabled is set.  In that case, the driver doesn't
> > enable runtime PM at all, but in addition to that it calls
> > pm_runtime_forbid() as though it wasn't sure if runtime PM might
> > be enabled for the device later by someone else.
> > 
> > Although that concern doesn't seem to be actually valid, the
> > device is clearly still expected to be PM-capable even in the
> > pm_disabled set case, so a better approach would be to enable
> > runtime PM for it unconditionally and then prevent it from
> > being runtime suspended by using pm_runtime_forbid().
> > 
> > Make the driver do that as that will help to clean up its system
> > sleep handling in a relatively straightforward way.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/i2c/busses/i2c-designware-platdrv.c |   36 +++++++++++++++++++---------
> >   1 file changed, 25 insertions(+), 11 deletions(-)
> > 
> > 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
> > @@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct
> >   	}
> >   }
> >   
> > +static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
> > +{
> > +	pm_runtime_get_noresume(dev->dev);
> > +	if (dev->pm_disabled)
> > +		pm_runtime_allow(dev->dev);
> > +
> > +	pm_runtime_disable(dev->dev);
> > +	pm_runtime_put_noidle(dev->dev);
> > +}
> > +
> >   static int dw_i2c_plat_probe(struct platform_device *pdev)
> >   {
> >   	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > @@ -362,14 +372,19 @@ 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;
> >   
> > -	if (dev->pm_disabled) {
> > +	/* The code below assumes runtime PM to be disabled. */
> > +	WARN_ON(pm_runtime_enabled(&pdev->dev));
> > +
> > +	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> > +	pm_runtime_use_autosuspend(&pdev->dev);
> > +	pm_runtime_set_active(&pdev->dev);
> > +
> > +	pm_runtime_get_noresume(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +	if (dev->pm_disabled)
> >   		pm_runtime_forbid(&pdev->dev);
> > -	} else {
> > -		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> > -		pm_runtime_use_autosuspend(&pdev->dev);
> > -		pm_runtime_set_active(&pdev->dev);
> > -		pm_runtime_enable(&pdev->dev);
> > -	}
> > +
> > +	pm_runtime_put_noidle(&pdev->dev);
> >   
> Is pm_runtime_get_noresume()/pm_runtime_put_noidle() cycle needed here? 
> My vague memory tells platform device won't power off instantly because 
> of plain pm_runtime_enable() even if dev->power.usage_count is zero.

Not by itself, but as a result of something running in parallel with the
probe it may in theory.

> I guess it was the pm_request_idle() in driver_probe_device() that 
> triggered the power transition after probe.
> 
> drivers/base/dd.c: driver_probe_device():
> 	pm_runtime_barrier(dev);
> 	ret = really_probe(dev, drv);
> 	pm_request_idle(dev);

This doesn't prevent runtime PM transitions from occurring in parallel
with really_probe() and the extra counter incrementation/decrementation
doesn't hurt. :-)

To me, the rule of thumb for runtime PM should be quite analogous to the one
for interrupts: expect it to happen immediately after you have enabled it
unless you know for a fact that there are protections in place.

Thanks,
Rafael

[RFT][PATCH 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit a23318feeff6 (i2c: designware: Fix system suspend)
the i2c-designware-platdrv driver always resumes the device in its
system sleep ->suspend callback which isn't particularly nice,
even though it is technically correct.

A better approach would be to make the driver track the PM state of
the device so that it doesn't need to resume it in ->suspend and
to drop its ->prepare and ->complete callbacks which would only be
marginally useful then, so implement it.

First, drop dw_i2c_plat_suspend() added by commit a23318feeff6 and
rename dw_i2c_plat_runtime_suspend() back to dw_i2c_plat_suspend().

Second, point the driver's ->late_suspend and ->early_resume
callbacks, rather than its ->suspend and ->resume callbacks,
to dw_i2c_plat_suspend() and dw_i2c_plat_resume(), respectively,
so that they are not executed in parallel with each other, for
example if runtime resume of the device takes place during system
suspend.  Also, since the driver enables runtime PM unconditionally
in dw_i2c_plat_probe(), this change allows the
pm_runtime_status_suspended() check to be used in the PM callbacks
to determine whether or not the device needs to be either suspended
or resumed (moving the callbacks into the late/early stages of
system suspend/resume, respectively, guarantees the stability
of the runtime PM status at the time when they are invoked).

Next, add a "skip_resume" flag to struct dw_i2c_dev and make
dw_i2c_plat_suspend() and dw_i2c_plat_resume() use it to avoid
resuming a previously runtime-suspended device during system resume.

Finally, drop the driver's ->prepare and ->complete PM callbacks,
because returning "true" from ->prepare for runtime-suspended
devices is marginally useful (the PM core may still ignore that
return value and invoke the driver's ->suspend callback anyway)
and ->complete is only needed because of what ->prepare does.

Overrides: a23318feeff6 (i2c: designware: Fix system suspend)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.h    |    1 
 drivers/i2c/busses/i2c-designware-platdrv.c |   47 +++++++++-------------------
 2 files changed, 17 insertions(+), 31 deletions(-)

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,6 +280,7 @@ struct dw_i2c_dev {
 	int			(*acquire_lock)(struct dw_i2c_dev *dev);
 	void			(*release_lock)(struct dw_i2c_dev *dev);
 	bool			pm_disabled;
+	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
@@ -434,28 +434,17 @@ static const struct of_device_id dw_i2c_
 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_runtime_suspend(struct device *dev)
+static int dw_i2c_plat_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	if (pm_runtime_status_suspended(dev)) {
+		i_dev->skip_resume = true;
+		return 0;
+	}
+
 	i_dev->disable(i_dev);
 	i2c_dw_plat_prepare_clk(i_dev, false);
 
@@ -467,27 +456,23 @@ static int dw_i2c_plat_resume(struct dev
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	if (!pm_runtime_status_suspended(dev))
+		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);
 
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_i2c_plat_suspend(struct device *dev)
-{
-	pm_runtime_resume(dev);
-	return dw_i2c_plat_runtime_suspend(dev);
-}
-#endif
-
 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_runtime_suspend,
-			   dw_i2c_plat_resume,
-			   NULL)
+	SET_LATE_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)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)

[RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit a23318feeff6 (i2c: designware: Fix system suspend)
the i2c-designware-platdrv driver always resumes the device in its
system sleep ->suspend callback which isn't particularly nice,
even though it is technically correct.

A better approach would be to make the driver track the PM state of
the device so that it doesn't need to resume it in ->suspend and
to drop its ->prepare and ->complete callbacks which are only
marginally useful, so implement that.

First, drop dw_i2c_plat_suspend() added by commit a23318feeff6 and
rename dw_i2c_plat_runtime_suspend() back to dw_i2c_plat_suspend().

Second, point the driver's ->late_suspend and ->early_resume
callbacks, rather than its ->suspend and ->resume callbacks,
to dw_i2c_plat_suspend() and dw_i2c_plat_resume(), respectively,
so that they are not executed in parallel with each other, for
example if runtime resume of the device takes place during system
suspend.

Next, add "suspended" and "skip_resume" flags to struct dw_i2c_dev
and make dw_i2c_plat_suspend() and dw_i2c_plat_resume() use them to
avoid suspending or resuming the device twice in a row and to avoid
resuming a previously runtime-suspended device during system resume.

Finally, drop the driver's ->prepare and ->complete PM callbacks,
because returning "true" from ->prepare for runtime-suspended
devices is marginally useful (the PM core may still ignore that
return value and invoke the driver's ->suspend callback anyway)
and ->complete is only needed because of what ->prepare does.

Overrides: a23318feeff6 (i2c: designware: Fix system suspend)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: Add the "suspended" flag, which is needed, because
       dw_i2c_plat_suspend() doesn't update the runtime PM
       status of the device, so dw_i2c_plat_resume() can't
       rely on the pm_runtime_status_suspended() check
       during system resume.

---
 drivers/i2c/busses/i2c-designware-core.h    |    2 +
 drivers/i2c/busses/i2c-designware-platdrv.c |   51 ++++++++++------------------
 2 files changed, 22 insertions(+), 31 deletions(-)

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,6 +280,8 @@ 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
@@ -434,31 +434,22 @@ static const struct of_device_id dw_i2c_
 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_runtime_suspend(struct device *dev)
+static int dw_i2c_plat_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	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;
 }
 
@@ -467,27 +458,25 @@ static int dw_i2c_plat_resume(struct dev
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	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);
 
-	return 0;
-}
+	i_dev->suspended = false;
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_i2c_plat_suspend(struct device *dev)
-{
-	pm_runtime_resume(dev);
-	return dw_i2c_plat_runtime_suspend(dev);
+	return 0;
 }
-#endif
 
 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_runtime_suspend,
-			   dw_i2c_plat_resume,
-			   NULL)
+	SET_LATE_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)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Mika Westerberg]

On Mon, Sep 04, 2017 at 12:01:54PM +0200, Rafael J. Wysocki wrote:
>  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_runtime_suspend,
> -			   dw_i2c_plat_resume,
> -			   NULL)
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)

This seems to cause problem with intel-lpss MFD driver because it uses
.suspend() and .resume() instead of .suspend_late() and .resume_early().
It only brings the device out of reset during .resume() which triggers
this:

[  221.066302] PM: noirq resume of devices complete after 162.461 msecs
[  221.079743] i2c_designware i2c_designware.0: Unknown Synopsys component type: 0x00000000
[  221.079749] i2c_designware i2c_designware.1: Unknown Synopsys component type: 0x00000000
[  221.079880] PM: early resume of devices complete after 13.538 msecs
...
[  222.115656] i2c_designware i2c_designware.1: controller timed out
[  222.756572] [drm] RC6 on
[  226.276006] i2c_hid i2c-DLL06E4:01: failed to reset device.
[  227.300012] i2c_designware i2c_designware.1: controller timed out
[  227.300037] i2c_hid i2c-DLL06E4:01: failed to change power setting.
[  227.300048] dpm_run_callback(): i2c_hid_resume+0x0/0xf0 [i2c_hid] returns -61
[  227.300057] PM: Device i2c-DLL06E4:01 failed to resume async: error -61

and the touchpad does not work from this point forward.

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Rafael J. Wysocki]

On Tuesday, September 5, 2017 4:45:11 PM CEST Mika Westerberg wrote:
> On Mon, Sep 04, 2017 at 12:01:54PM +0200, Rafael J. Wysocki wrote:
> >  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_runtime_suspend,
> > -			   dw_i2c_plat_resume,
> > -			   NULL)
> > +	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> 
> This seems to cause problem with intel-lpss MFD driver because it uses
> .suspend() and .resume() instead of .suspend_late() and .resume_early().

OK, so there is one more dependency here.

Can you please point me to this code?

> It only brings the device out of reset during .resume() which triggers
> this:
> 
> [  221.066302] PM: noirq resume of devices complete after 162.461 msecs
> [  221.079743] i2c_designware i2c_designware.0: Unknown Synopsys component type: 0x00000000
> [  221.079749] i2c_designware i2c_designware.1: Unknown Synopsys component type: 0x00000000
> [  221.079880] PM: early resume of devices complete after 13.538 msecs
> ...
> [  222.115656] i2c_designware i2c_designware.1: controller timed out
> [  222.756572] [drm] RC6 on
> [  226.276006] i2c_hid i2c-DLL06E4:01: failed to reset device.
> [  227.300012] i2c_designware i2c_designware.1: controller timed out
> [  227.300037] i2c_hid i2c-DLL06E4:01: failed to change power setting.
> [  227.300048] dpm_run_callback(): i2c_hid_resume+0x0/0xf0 [i2c_hid] returns -61
> [  227.300057] PM: Device i2c-DLL06E4:01 failed to resume async: error -61
> 
> and the touchpad does not work from this point forward.

Thanks for the testing!

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Mika Westerberg]

On Tue, Sep 05, 2017 at 04:46:11PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 5, 2017 4:45:11 PM CEST Mika Westerberg wrote:
> > On Mon, Sep 04, 2017 at 12:01:54PM +0200, Rafael J. Wysocki wrote:
> > >  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_runtime_suspend,
> > > -			   dw_i2c_plat_resume,
> > > -			   NULL)
> > > +	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> > 
> > This seems to cause problem with intel-lpss MFD driver because it uses
> > .suspend() and .resume() instead of .suspend_late() and .resume_early().
> 
> OK, so there is one more dependency here.
> 
> Can you please point me to this code?

It is in drivers/mfd/intel-lpss.c. See intel_lpss_resume().

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Rafael J. Wysocki]

On Tuesday, September 5, 2017 4:58:35 PM CEST Mika Westerberg wrote:
> On Tue, Sep 05, 2017 at 04:46:11PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, September 5, 2017 4:45:11 PM CEST Mika Westerberg wrote:
> > > On Mon, Sep 04, 2017 at 12:01:54PM +0200, Rafael J. Wysocki wrote:
> > > >  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_runtime_suspend,
> > > > -			   dw_i2c_plat_resume,
> > > > -			   NULL)
> > > > +	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> > > 
> > > This seems to cause problem with intel-lpss MFD driver because it uses
> > > .suspend() and .resume() instead of .suspend_late() and .resume_early().
> > 
> > OK, so there is one more dependency here.
> > 
> > Can you please point me to this code?
> 
> It is in drivers/mfd/intel-lpss.c. See intel_lpss_resume().
> 

Looking at it, but I don't quite see how this is related to the
i2c-designware-platedv suspend/resume ...

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Rafael J. Wysocki]

On Tuesday, September 5, 2017 4:55:44 PM CEST Rafael J. Wysocki wrote:
> On Tuesday, September 5, 2017 4:58:35 PM CEST Mika Westerberg wrote:
> > On Tue, Sep 05, 2017 at 04:46:11PM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, September 5, 2017 4:45:11 PM CEST Mika Westerberg wrote:
> > > > On Mon, Sep 04, 2017 at 12:01:54PM +0200, Rafael J. Wysocki wrote:
> > > > >  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_runtime_suspend,
> > > > > -			   dw_i2c_plat_resume,
> > > > > -			   NULL)
> > > > > +	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> > > > 
> > > > This seems to cause problem with intel-lpss MFD driver because it uses
> > > > .suspend() and .resume() instead of .suspend_late() and .resume_early().
> > > 
> > > OK, so there is one more dependency here.
> > > 
> > > Can you please point me to this code?
> > 
> > It is in drivers/mfd/intel-lpss.c. See intel_lpss_resume().
> > 
> 
> Looking at it, but I don't quite see how this is related to the
> i2c-designware-platedv suspend/resume ...
> 

My guess would be that i2c-designware is a child of the intel-lpss thing
and therefore it is expected to be suspended before it and resumed later,
right?

In that case doing runtime PM during the i2c-designware suspend/resume is
a no-go as well.

Oh well.

Would moving the intel_lpss_suspend/resume() to ->suspend_late/->resume_early
be viable?

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Mika Westerberg]

On Tue, Sep 05, 2017 at 04:55:44PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 5, 2017 4:58:35 PM CEST Mika Westerberg wrote:
> > On Tue, Sep 05, 2017 at 04:46:11PM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, September 5, 2017 4:45:11 PM CEST Mika Westerberg wrote:
> > > > On Mon, Sep 04, 2017 at 12:01:54PM +0200, Rafael J. Wysocki wrote:
> > > > >  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_runtime_suspend,
> > > > > -			   dw_i2c_plat_resume,
> > > > > -			   NULL)
> > > > > +	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> > > > 
> > > > This seems to cause problem with intel-lpss MFD driver because it uses
> > > > .suspend() and .resume() instead of .suspend_late() and .resume_early().
> > > 
> > > OK, so there is one more dependency here.
> > > 
> > > Can you please point me to this code?
> > 
> > It is in drivers/mfd/intel-lpss.c. See intel_lpss_resume().
> > 
> 
> Looking at it, but I don't quite see how this is related to the
> i2c-designware-platedv suspend/resume ...

intel-lpss is the parent device for i2c-designware-platdrv. It is
supposed to handle all LPSS specific stuff, like bringing the PCI device
out of reset before the i2c-designware-platdrv does its own resume
things.

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Rafael J. Wysocki]

On Tuesday, September 5, 2017 5:07:44 PM CEST Mika Westerberg wrote:
> On Tue, Sep 05, 2017 at 04:55:44PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, September 5, 2017 4:58:35 PM CEST Mika Westerberg wrote:
> > > On Tue, Sep 05, 2017 at 04:46:11PM +0200, Rafael J. Wysocki wrote:
> > > > On Tuesday, September 5, 2017 4:45:11 PM CEST Mika Westerberg wrote:
> > > > > On Mon, Sep 04, 2017 at 12:01:54PM +0200, Rafael J. Wysocki wrote:
> > > > > >  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_runtime_suspend,
> > > > > > -			   dw_i2c_plat_resume,
> > > > > > -			   NULL)
> > > > > > +	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> > > > > 
> > > > > This seems to cause problem with intel-lpss MFD driver because it uses
> > > > > .suspend() and .resume() instead of .suspend_late() and .resume_early().
> > > > 
> > > > OK, so there is one more dependency here.
> > > > 
> > > > Can you please point me to this code?
> > > 
> > > It is in drivers/mfd/intel-lpss.c. See intel_lpss_resume().
> > > 
> > 
> > Looking at it, but I don't quite see how this is related to the
> > i2c-designware-platedv suspend/resume ...
> 
> intel-lpss is the parent device for i2c-designware-platdrv. It is
> supposed to handle all LPSS specific stuff, like bringing the PCI device
> out of reset before the i2c-designware-platdrv does its own resume
> things.

Yes, I see.

OK, so what about moving its suspend/resume to the late/early stages?

Would the parent of it be confused?

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Mika Westerberg]

On Tue, Sep 05, 2017 at 05:04:21PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 5, 2017 5:07:44 PM CEST Mika Westerberg wrote:
> > On Tue, Sep 05, 2017 at 04:55:44PM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, September 5, 2017 4:58:35 PM CEST Mika Westerberg wrote:
> > > > On Tue, Sep 05, 2017 at 04:46:11PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tuesday, September 5, 2017 4:45:11 PM CEST Mika Westerberg wrote:
> > > > > > On Mon, Sep 04, 2017 at 12:01:54PM +0200, Rafael J. Wysocki wrote:
> > > > > > >  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_runtime_suspend,
> > > > > > > -			   dw_i2c_plat_resume,
> > > > > > > -			   NULL)
> > > > > > > +	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> > > > > > 
> > > > > > This seems to cause problem with intel-lpss MFD driver because it uses
> > > > > > .suspend() and .resume() instead of .suspend_late() and .resume_early().
> > > > > 
> > > > > OK, so there is one more dependency here.
> > > > > 
> > > > > Can you please point me to this code?
> > > > 
> > > > It is in drivers/mfd/intel-lpss.c. See intel_lpss_resume().
> > > > 
> > > 
> > > Looking at it, but I don't quite see how this is related to the
> > > i2c-designware-platedv suspend/resume ...
> > 
> > intel-lpss is the parent device for i2c-designware-platdrv. It is
> > supposed to handle all LPSS specific stuff, like bringing the PCI device
> > out of reset before the i2c-designware-platdrv does its own resume
> > things.
> 
> Yes, I see.
> 
> OK, so what about moving its suspend/resume to the late/early stages?
> 
> Would the parent of it be confused?

It seems to work. I did following change and now suspend/resume works
fine with your patch series.

diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
index 694116630ffa..c987f7fe6c74 100644
--- a/drivers/mfd/intel-lpss.h
+++ b/drivers/mfd/intel-lpss.h
@@ -38,8 +38,8 @@ int intel_lpss_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS			\
 	.prepare = intel_lpss_prepare,		\
-	.suspend = intel_lpss_suspend,		\
-	.resume = intel_lpss_resume,		\
+	.suspend_late = intel_lpss_suspend,	\
+	.resume_early = intel_lpss_resume,	\
 	.freeze = intel_lpss_suspend,		\
 	.thaw = intel_lpss_resume,		\
 	.poweroff = intel_lpss_suspend,		\

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Rafael J. Wysocki]

On Tue, Sep 5, 2017 at 5:24 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Sep 05, 2017 at 05:04:21PM +0200, Rafael J. Wysocki wrote:
>> On Tuesday, September 5, 2017 5:07:44 PM CEST Mika Westerberg wrote:
>> > On Tue, Sep 05, 2017 at 04:55:44PM +0200, Rafael J. Wysocki wrote:
>> > > On Tuesday, September 5, 2017 4:58:35 PM CEST Mika Westerberg wrote:
>> > > > On Tue, Sep 05, 2017 at 04:46:11PM +0200, Rafael J. Wysocki wrote:
>> > > > > On Tuesday, September 5, 2017 4:45:11 PM CEST Mika Westerberg wrote:
>> > > > > > On Mon, Sep 04, 2017 at 12:01:54PM +0200, Rafael J. Wysocki wrote:
>> > > > > > >  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_runtime_suspend,
>> > > > > > > -                    dw_i2c_plat_resume,
>> > > > > > > -                    NULL)
>> > > > > > > + SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>> > > > > >
>> > > > > > This seems to cause problem with intel-lpss MFD driver because it uses
>> > > > > > .suspend() and .resume() instead of .suspend_late() and .resume_early().
>> > > > >
>> > > > > OK, so there is one more dependency here.
>> > > > >
>> > > > > Can you please point me to this code?
>> > > >
>> > > > It is in drivers/mfd/intel-lpss.c. See intel_lpss_resume().
>> > > >
>> > >
>> > > Looking at it, but I don't quite see how this is related to the
>> > > i2c-designware-platedv suspend/resume ...
>> >
>> > intel-lpss is the parent device for i2c-designware-platdrv. It is
>> > supposed to handle all LPSS specific stuff, like bringing the PCI device
>> > out of reset before the i2c-designware-platdrv does its own resume
>> > things.
>>
>> Yes, I see.
>>
>> OK, so what about moving its suspend/resume to the late/early stages?
>>
>> Would the parent of it be confused?
>
> It seems to work. I did following change and now suspend/resume works
> fine with your patch series.

OK, thanks!

> diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> index 694116630ffa..c987f7fe6c74 100644
> --- a/drivers/mfd/intel-lpss.h
> +++ b/drivers/mfd/intel-lpss.h
> @@ -38,8 +38,8 @@ int intel_lpss_resume(struct device *dev);
>  #ifdef CONFIG_PM_SLEEP
>  #define INTEL_LPSS_SLEEP_PM_OPS                        \
>         .prepare = intel_lpss_prepare,          \
> -       .suspend = intel_lpss_suspend,          \
> -       .resume = intel_lpss_resume,            \
> +       .suspend_late = intel_lpss_suspend,     \
> +       .resume_early = intel_lpss_resume,      \
>         .freeze = intel_lpss_suspend,           \
>         .thaw = intel_lpss_resume,              \
>         .poweroff = intel_lpss_suspend,         \

Of course, freeze/thaw, poweroff/restore need to be moved to the
late/early stages too.

I'll add this patch to the series and resend, then.

BTW, is the parent of intel-lpss in this case a PCI device or something else?

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Mika Westerberg]

On Tue, Sep 05, 2017 at 05:32:07PM +0200, Rafael J. Wysocki wrote:
> On Tue, Sep 5, 2017 at 5:24 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Sep 05, 2017 at 05:04:21PM +0200, Rafael J. Wysocki wrote:
> >> On Tuesday, September 5, 2017 5:07:44 PM CEST Mika Westerberg wrote:
> >> > On Tue, Sep 05, 2017 at 04:55:44PM +0200, Rafael J. Wysocki wrote:
> >> > > On Tuesday, September 5, 2017 4:58:35 PM CEST Mika Westerberg wrote:
> >> > > > On Tue, Sep 05, 2017 at 04:46:11PM +0200, Rafael J. Wysocki wrote:
> >> > > > > On Tuesday, September 5, 2017 4:45:11 PM CEST Mika Westerberg wrote:
> >> > > > > > On Mon, Sep 04, 2017 at 12:01:54PM +0200, Rafael J. Wysocki wrote:
> >> > > > > > >  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_runtime_suspend,
> >> > > > > > > -                    dw_i2c_plat_resume,
> >> > > > > > > -                    NULL)
> >> > > > > > > + SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> >> > > > > >
> >> > > > > > This seems to cause problem with intel-lpss MFD driver because it uses
> >> > > > > > .suspend() and .resume() instead of .suspend_late() and .resume_early().
> >> > > > >
> >> > > > > OK, so there is one more dependency here.
> >> > > > >
> >> > > > > Can you please point me to this code?
> >> > > >
> >> > > > It is in drivers/mfd/intel-lpss.c. See intel_lpss_resume().
> >> > > >
> >> > >
> >> > > Looking at it, but I don't quite see how this is related to the
> >> > > i2c-designware-platedv suspend/resume ...
> >> >
> >> > intel-lpss is the parent device for i2c-designware-platdrv. It is
> >> > supposed to handle all LPSS specific stuff, like bringing the PCI device
> >> > out of reset before the i2c-designware-platdrv does its own resume
> >> > things.
> >>
> >> Yes, I see.
> >>
> >> OK, so what about moving its suspend/resume to the late/early stages?
> >>
> >> Would the parent of it be confused?
> >
> > It seems to work. I did following change and now suspend/resume works
> > fine with your patch series.
> 
> OK, thanks!
> 
> > diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> > index 694116630ffa..c987f7fe6c74 100644
> > --- a/drivers/mfd/intel-lpss.h
> > +++ b/drivers/mfd/intel-lpss.h
> > @@ -38,8 +38,8 @@ int intel_lpss_resume(struct device *dev);
> >  #ifdef CONFIG_PM_SLEEP
> >  #define INTEL_LPSS_SLEEP_PM_OPS                        \
> >         .prepare = intel_lpss_prepare,          \
> > -       .suspend = intel_lpss_suspend,          \
> > -       .resume = intel_lpss_resume,            \
> > +       .suspend_late = intel_lpss_suspend,     \
> > +       .resume_early = intel_lpss_resume,      \
> >         .freeze = intel_lpss_suspend,           \
> >         .thaw = intel_lpss_resume,              \
> >         .poweroff = intel_lpss_suspend,         \
> 
> Of course, freeze/thaw, poweroff/restore need to be moved to the
> late/early stages too.

Right.

> I'll add this patch to the series and resend, then.

Thanks!

> BTW, is the parent of intel-lpss in this case a PCI device or
> something else?

It is the PCI host bridge:

00:00.0 Host bridge: Intel Corporation Skylake Host Bridge/DRAM Registers (rev 07)
00:01.0 PCI bridge: Intel Corporation Skylake PCIe Controller (x16) (rev 07)
00:02.0 VGA compatible controller: Intel Corporation HD Graphics 530 (rev 06)
00:04.0 Signal processing controller: Intel Corporation Skylake Processor Thermal Subsystem (rev 07)
00:14.0 USB controller: Intel Corporation Sunrise Point-H USB 3.0 xHCI Controller (rev 31)
00:14.2 Signal processing controller: Intel Corporation Sunrise Point-H Thermal subsystem (rev 31)

The following are the two LPSS I2C devices where intel-lpss binds to:

00:15.0 Signal processing controller: Intel Corporation Sunrise Point-H Serial IO I2C Controller #0 (rev 31)
00:15.1 Signal processing controller: Intel Corporation Sunrise Point-H Serial IO I2C Controller #1 (rev 31)

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Johannes Stezenbach]

On Tue, Sep 05, 2017 at 06:41:47PM +0300, Mika Westerberg wrote:
> On Tue, Sep 05, 2017 at 05:32:07PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Sep 5, 2017 at 5:24 PM, Mika Westerberg wrote:
> > > diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> > > index 694116630ffa..c987f7fe6c74 100644
> > > --- a/drivers/mfd/intel-lpss.h
> > > +++ b/drivers/mfd/intel-lpss.h
> > > @@ -38,8 +38,8 @@ int intel_lpss_resume(struct device *dev);
> > >  #ifdef CONFIG_PM_SLEEP
> > >  #define INTEL_LPSS_SLEEP_PM_OPS                        \
> > >         .prepare = intel_lpss_prepare,          \
> > > -       .suspend = intel_lpss_suspend,          \
> > > -       .resume = intel_lpss_resume,            \
> > > +       .suspend_late = intel_lpss_suspend,     \
> > > +       .resume_early = intel_lpss_resume,      \
> > >         .freeze = intel_lpss_suspend,           \
> > >         .thaw = intel_lpss_resume,              \
> > >         .poweroff = intel_lpss_suspend,         \
> > 
> > Of course, freeze/thaw, poweroff/restore need to be moved to the
> > late/early stages too.
> 
> Right.

I tested the patches on Asus E200HA with the above + freeze_late/thaw_early,
works fine.  Then, wrt Rafael's comment in earlier mail about
ordering of i2c designware vs. other drivers calling it via
ACPI OpRegion, I changed it to noirq:
(probably it's off-topic here but I tested while at it)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 335b2b236faa..6b1b3938c405 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -475,7 +475,7 @@ static int dw_i2c_plat_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
-	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+	SET_NOIRQ_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)
 };
 
diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
index 694116630ffa..7069d67160a0 100644
--- a/drivers/mfd/intel-lpss.h
+++ b/drivers/mfd/intel-lpss.h
@@ -38,10 +38,10 @@ int intel_lpss_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS			\
 	.prepare = intel_lpss_prepare,		\
-	.suspend = intel_lpss_suspend,		\
-	.resume = intel_lpss_resume,		\
-	.freeze = intel_lpss_suspend,		\
-	.thaw = intel_lpss_resume,		\
+	.suspend_noirq = intel_lpss_suspend,		\
+	.resume_noirq = intel_lpss_resume,		\
+	.freeze_noirq = intel_lpss_suspend,		\
+	.thaw_noirq = intel_lpss_resume,		\
 	.poweroff = intel_lpss_suspend,		\
 	.restore = intel_lpss_resume,
 #else

It causes errors in dmesg:

[   62.460369] PM: late suspend of devices complete after 35.484 msecs
[   62.492283] i2c_designware 808622C1:02: timeout in disabling adapter
[   62.519527] i2c_designware 808622C1:01: timeout in disabling adapter
[   62.546930] i2c_designware 808622C1:00: timeout in disabling adapter
[   62.565844] PM: noirq suspend of devices complete after 105.431 msecs
[   62.565853] PM: suspend-to-idle
...
[   65.590077] Suspended for 3.104 seconds
[   65.591669] i2c_designware 808622C1:00: Unknown Synopsys component type: 0xffffffff
[   65.591689] i2c_designware 808622C1:01: Unknown Synopsys component type: 0xffffffff
[   65.591704] i2c_designware 808622C1:02: Unknown Synopsys component type: 0xffffffff
[   65.612136] PM: noirq resume of devices complete after 21.451 msecs
[   65.615059] PM: resume from suspend-to-idle

But at least keyboard attached to 808622C1:00 still worked, it is:
[    3.264799] input: PDEC3393:00 0B05:8585 as /devices/pci0000:00/808622C1:00/i2c-0/i2c-PDEC3393:00/0018:0B05:8585.0001
/input/input5
[    3.266476] asus 0018:0B05:8585.0001: input,hidraw0: I2C HID v1.00 Keyboard [PDEC3393:00 0B05:8585] on i2c-PDEC3393:00

Re: [RFT][PATCH v2 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Rafael J. Wysocki]

On Tue, Sep 5, 2017 at 11:00 PM, Johannes Stezenbach <js@sig21.net> wrote:
> On Tue, Sep 05, 2017 at 06:41:47PM +0300, Mika Westerberg wrote:
>> On Tue, Sep 05, 2017 at 05:32:07PM +0200, Rafael J. Wysocki wrote:
>> > On Tue, Sep 5, 2017 at 5:24 PM, Mika Westerberg wrote:
>> > > diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
>> > > index 694116630ffa..c987f7fe6c74 100644
>> > > --- a/drivers/mfd/intel-lpss.h
>> > > +++ b/drivers/mfd/intel-lpss.h
>> > > @@ -38,8 +38,8 @@ int intel_lpss_resume(struct device *dev);
>> > >  #ifdef CONFIG_PM_SLEEP
>> > >  #define INTEL_LPSS_SLEEP_PM_OPS                        \
>> > >         .prepare = intel_lpss_prepare,          \
>> > > -       .suspend = intel_lpss_suspend,          \
>> > > -       .resume = intel_lpss_resume,            \
>> > > +       .suspend_late = intel_lpss_suspend,     \
>> > > +       .resume_early = intel_lpss_resume,      \
>> > >         .freeze = intel_lpss_suspend,           \
>> > >         .thaw = intel_lpss_resume,              \
>> > >         .poweroff = intel_lpss_suspend,         \
>> >
>> > Of course, freeze/thaw, poweroff/restore need to be moved to the
>> > late/early stages too.
>>
>> Right.
>
> I tested the patches on Asus E200HA with the above + freeze_late/thaw_early,
> works fine.  Then, wrt Rafael's comment in earlier mail about
> ordering of i2c designware vs. other drivers calling it via
> ACPI OpRegion, I changed it to noirq:
> (probably it's off-topic here but I tested while at it)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 335b2b236faa..6b1b3938c405 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -475,7 +475,7 @@ static int dw_i2c_plat_resume(struct device *dev)
>  }
>
>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
> -       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> +       SET_NOIRQ_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)
>  };
>
> diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> index 694116630ffa..7069d67160a0 100644
> --- a/drivers/mfd/intel-lpss.h
> +++ b/drivers/mfd/intel-lpss.h
> @@ -38,10 +38,10 @@ int intel_lpss_resume(struct device *dev);
>  #ifdef CONFIG_PM_SLEEP
>  #define INTEL_LPSS_SLEEP_PM_OPS                        \
>         .prepare = intel_lpss_prepare,          \
> -       .suspend = intel_lpss_suspend,          \
> -       .resume = intel_lpss_resume,            \
> -       .freeze = intel_lpss_suspend,           \
> -       .thaw = intel_lpss_resume,              \
> +       .suspend_noirq = intel_lpss_suspend,            \
> +       .resume_noirq = intel_lpss_resume,              \
> +       .freeze_noirq = intel_lpss_suspend,             \
> +       .thaw_noirq = intel_lpss_resume,                \
>         .poweroff = intel_lpss_suspend,         \
>         .restore = intel_lpss_resume,
>  #else
>
> It causes errors in dmesg:
>
> [   62.460369] PM: late suspend of devices complete after 35.484 msecs
> [   62.492283] i2c_designware 808622C1:02: timeout in disabling adapter
> [   62.519527] i2c_designware 808622C1:01: timeout in disabling adapter
> [   62.546930] i2c_designware 808622C1:00: timeout in disabling adapter
> [   62.565844] PM: noirq suspend of devices complete after 105.431 msecs
> [   62.565853] PM: suspend-to-idle
> ...
> [   65.590077] Suspended for 3.104 seconds
> [   65.591669] i2c_designware 808622C1:00: Unknown Synopsys component type: 0xffffffff
> [   65.591689] i2c_designware 808622C1:01: Unknown Synopsys component type: 0xffffffff
> [   65.591704] i2c_designware 808622C1:02: Unknown Synopsys component type: 0xffffffff
> [   65.612136] PM: noirq resume of devices complete after 21.451 msecs
> [   65.615059] PM: resume from suspend-to-idle
>
> But at least keyboard attached to 808622C1:00 still worked, it is:
> [    3.264799] input: PDEC3393:00 0B05:8585 as /devices/pci0000:00/808622C1:00/i2c-0/i2c-PDEC3393:00/0018:0B05:8585.0001
> /input/input5
> [    3.266476] asus 0018:0B05:8585.0001: input,hidraw0: I2C HID v1.00 Keyboard [PDEC3393:00 0B05:8585] on i2c-PDEC3393:00

That probably goes too far, at least for the time being.

When I was making that comment I was not aware of the entire
complexity involved here, sorry about that.

Re: [RFT][PATCH 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Rafael J. Wysocki]

On Monday, September 4, 2017 1:14:44 AM CEST Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After commit a23318feeff6 (i2c: designware: Fix system suspend)
> the i2c-designware-platdrv driver always resumes the device in its
> system sleep ->suspend callback which isn't particularly nice,
> even though it is technically correct.
> 
> A better approach would be to make the driver track the PM state of
> the device so that it doesn't need to resume it in ->suspend and
> to drop its ->prepare and ->complete callbacks which would only be
> marginally useful then, so implement it.
> 
> First, drop dw_i2c_plat_suspend() added by commit a23318feeff6 and
> rename dw_i2c_plat_runtime_suspend() back to dw_i2c_plat_suspend().
> 
> Second, point the driver's ->late_suspend and ->early_resume
> callbacks, rather than its ->suspend and ->resume callbacks,
> to dw_i2c_plat_suspend() and dw_i2c_plat_resume(), respectively,
> so that they are not executed in parallel with each other, for
> example if runtime resume of the device takes place during system
> suspend.  Also, since the driver enables runtime PM unconditionally
> in dw_i2c_plat_probe(), this change allows the
> pm_runtime_status_suspended() check to be used in the PM callbacks
> to determine whether or not the device needs to be either suspended
> or resumed (moving the callbacks into the late/early stages of
> system suspend/resume, respectively, guarantees the stability
> of the runtime PM status at the time when they are invoked).
> 
> Next, add a "skip_resume" flag to struct dw_i2c_dev and make
> dw_i2c_plat_suspend() and dw_i2c_plat_resume() use it to avoid
> resuming a previously runtime-suspended device during system resume.
> 
> Finally, drop the driver's ->prepare and ->complete PM callbacks,
> because returning "true" from ->prepare for runtime-suspended
> devices is marginally useful (the PM core may still ignore that
> return value and invoke the driver's ->suspend callback anyway)
> and ->complete is only needed because of what ->prepare does.
> 
> Overrides: a23318feeff6 (i2c: designware: Fix system suspend)
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.h    |    1 
>  drivers/i2c/busses/i2c-designware-platdrv.c |   47 +++++++++-------------------
>  2 files changed, 17 insertions(+), 31 deletions(-)
> 
> 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,6 +280,7 @@ struct dw_i2c_dev {
>  	int			(*acquire_lock)(struct dw_i2c_dev *dev);
>  	void			(*release_lock)(struct dw_i2c_dev *dev);
>  	bool			pm_disabled;
> +	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
> @@ -434,28 +434,17 @@ static const struct of_device_id dw_i2c_
>  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_runtime_suspend(struct device *dev)
> +static int dw_i2c_plat_suspend(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>  
> +	if (pm_runtime_status_suspended(dev)) {
> +		i_dev->skip_resume = true;
> +		return 0;
> +	}
> +
>  	i_dev->disable(i_dev);
>  	i2c_dw_plat_prepare_clk(i_dev, false);
>  
> @@ -467,27 +456,23 @@ static int dw_i2c_plat_resume(struct dev
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>  
> +	if (!pm_runtime_status_suspended(dev))

This check is incorrect, I've just sent a v2 of the patch.

Sorry about this.

> +		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);
>  
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int dw_i2c_plat_suspend(struct device *dev)
> -{
> -	pm_runtime_resume(dev);
> -	return dw_i2c_plat_runtime_suspend(dev);
> -}
> -#endif
> -
>  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_runtime_suspend,
> -			   dw_i2c_plat_resume,
> -			   NULL)
> +	SET_LATE_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)
>  };
>  
>  #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)

Re: [RFT][PATCH 2/2] PM / i2c: designware: Clean up system sleep handling without ACPI

[Andy Shevchenko]

On Mon, 2017-09-04 at 01:14 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After commit a23318feeff6 (i2c: designware: Fix system suspend)
> the i2c-designware-platdrv driver always resumes the device in its
> system sleep ->suspend callback which isn't particularly nice,
> even though it is technically correct.
> 
> A better approach would be to make the driver track the PM state of
> the device so that it doesn't need to resume it in ->suspend and
> to drop its ->prepare and ->complete callbacks which would only be
> marginally useful then, so implement it.
> 
> First, drop dw_i2c_plat_suspend() added by commit a23318feeff6 and
> rename dw_i2c_plat_runtime_suspend() back to dw_i2c_plat_suspend().
> 
> Second, point the driver's ->late_suspend and ->early_resume
> callbacks, rather than its ->suspend and ->resume callbacks,
> to dw_i2c_plat_suspend() and dw_i2c_plat_resume(), respectively,
> so that they are not executed in parallel with each other, for
> example if runtime resume of the device takes place during system
> suspend.  Also, since the driver enables runtime PM unconditionally
> in dw_i2c_plat_probe(), this change allows the
> pm_runtime_status_suspended() check to be used in the PM callbacks
> to determine whether or not the device needs to be either suspended
> or resumed (moving the callbacks into the late/early stages of
> system suspend/resume, respectively, guarantees the stability
> of the runtime PM status at the time when they are invoked).
> 
> Next, add a "skip_resume" flag to struct dw_i2c_dev and make
> dw_i2c_plat_suspend() and dw_i2c_plat_resume() use it to avoid
> resuming a previously runtime-suspended device during system resume.
> 
> Finally, drop the driver's ->prepare and ->complete PM callbacks,
> because returning "true" from ->prepare for runtime-suspended
> devices is marginally useful (the PM core may still ignore that
> return value and invoke the driver's ->suspend callback anyway)
> and ->complete is only needed because of what ->prepare does.

> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend,
> dw_i2c_plat_resume)

Luckily I2C doesn't use DMA.

For cases when we have a separate DMA controller devices (CherryTrail /
BayTrail) we have to be sure that I2C device is going to be suspended
first and resumed last to be operational. DMA itself uses LATE PM ops
IIRC (drivers/dma/dw/platform.c) and it becomes possible ordering issue
if both drivers are on the same level of PM ops.

P.S. For now, as I noted above, it would be not a problem, I think I
just need to mention this potential issue in the future with some
drivers (like SPI or UART, which are using DMA) on SoCs like CherryTrail
or if I2C going to support DMA.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[RFT][PATCH v3 0/3] i2c: designware: Runtime PM aware system sleep handling

[Rafael J. Wysocki]

On Monday, September 4, 2017 1:05:57 AM CEST Rafael J. Wysocki wrote:
> Hi,

An update related to the dependency on intel-lpss.

> The point here is to avoid runtime resuming i2c designware devices during
> system suspend in the driver's ->suspend callback in the case when the
> ACPI PM domain is not present.  That will allow us to deal with the
> ACPI PM domain case going forward, among other things.
> 
> The first patch cleans up the runtime PM handling in the i2c-designware-platdrv
> _probe() routine so as to make it enable runtime PM in all cases, but prevent
> the device from being runtime suspended via pm_runtime_forbid() if
> pm_disabled is set.

The second one pushed the intel-lpss system suspend/resume callbacks to the
late/early stages of suspend/resume, respectively, so that the i2c-designware-platdrv
ones can be pushed too.

> The second one get rids of some ugly code and makes the PM callbacks of the
> driver handle runtime-suspended devices during system suspend/resume.

This is the third one and it does less than it did before now.

> Please test if you can and let me know if anything breaks.

That still applies. :-)

Thanks,
Rafael

[RFT][PATCH v3 1/3] i2c: designware: Clean up PM handling in dw_i2c_plat_probe()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The power management handling in dw_i2c_plat_probe() is somewhat
messy and it is rather hard to figure out the code intention for
the case when pm_disabled is set.  In that case, the driver doesn't
enable runtime PM at all, but in addition to that it calls
pm_runtime_forbid() as though it wasn't sure if runtime PM might
be enabled for the device later by someone else.

Although that concern doesn't seem to be actually valid, the
device is clearly still expected to be PM-capable even in the
pm_disabled set case, so a better approach would be to enable
runtime PM for it unconditionally and then prevent it from
being runtime-suspended by using pm_runtime_forbid().

Make the driver do that.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v3: Small update of the changelog.

---
 drivers/i2c/busses/i2c-designware-platdrv.c |   36 +++++++++++++++++++---------
 1 file changed, 25 insertions(+), 11 deletions(-)

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
@@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct
 	}
 }
 
+static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
+{
+	pm_runtime_get_noresume(dev->dev);
+	if (dev->pm_disabled)
+		pm_runtime_allow(dev->dev);
+
+	pm_runtime_disable(dev->dev);
+	pm_runtime_put_noidle(dev->dev);
+}
+
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -362,14 +372,19 @@ 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;
 
-	if (dev->pm_disabled) {
+	/* The code below assumes runtime PM to be disabled. */
+	WARN_ON(pm_runtime_enabled(&pdev->dev));
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	if (dev->pm_disabled)
 		pm_runtime_forbid(&pdev->dev);
-	} else {
-		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-		pm_runtime_use_autosuspend(&pdev->dev);
-		pm_runtime_set_active(&pdev->dev);
-		pm_runtime_enable(&pdev->dev);
-	}
+
+	pm_runtime_put_noidle(&pdev->dev);
 
 	if (dev->mode == DW_IC_SLAVE)
 		ret = i2c_dw_probe_slave(dev);
@@ -382,8 +397,7 @@ static int dw_i2c_plat_probe(struct plat
 	return ret;
 
 exit_probe:
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
 exit_reset:
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
@@ -402,8 +416,8 @@ static int dw_i2c_plat_remove(struct pla
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
+
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
 

[RFT][PATCH v3 2/3] PM / mfd: intel-lpss: Push system sleep callbacks to late/early stages

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Push the system suspend/resume callbacks of intel-lpss to the late
suspend/early resume stages to allow child device callbacks to be
pushed to the late/early stages of suspend/resume too, so as to
make it possible to avoid resuming the children if they are runtime-
suspended during system suspend going forward.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/mfd/intel-lpss.h |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Index: linux-pm/drivers/mfd/intel-lpss.h
===================================================================
--- linux-pm.orig/drivers/mfd/intel-lpss.h
+++ linux-pm/drivers/mfd/intel-lpss.h
@@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS			\
 	.prepare = intel_lpss_prepare,		\
-	.suspend = intel_lpss_suspend,		\
-	.resume = intel_lpss_resume,		\
-	.freeze = intel_lpss_suspend,		\
-	.thaw = intel_lpss_resume,		\
-	.poweroff = intel_lpss_suspend,		\
-	.restore = intel_lpss_resume,
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
 #endif

[RFT][PATCH v3 3/3] PM / i2c: designware: Clean up system sleep handling

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit a23318feeff6 (i2c: designware: Fix system suspend)
the i2c-designware-platdrv driver always resumes the device in its
system sleep ->suspend callback which isn't particularly nice,
even though it is technically correct.

A better approach would be to make the driver track the PM state of
the device so that it doesn't need to resume it in ->suspend, so
implement it.

First, drop dw_i2c_plat_suspend() added by commit a23318feeff6 and
rename dw_i2c_plat_runtime_suspend() back to dw_i2c_plat_suspend().

Second, point the driver's ->late_suspend and ->early_resume
callbacks, rather than its ->suspend and ->resume callbacks,
to dw_i2c_plat_suspend() and dw_i2c_plat_resume(), respectively,
so that they are not executed in parallel with each other, for
example if runtime resume of the device takes place during system
suspend.

Finally, add "suspended" and "skip_resume" flags to struct dw_i2c_dev
and make dw_i2c_plat_suspend() and dw_i2c_plat_resume() use them to
avoid suspending or resuming the device twice in a row and to avoid
resuming a previously runtime-suspended device during system resume.

Overrides: a23318feeff6 (i2c: designware: Fix system suspend)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2:
  Add the "suspended" flag, which is needed, because
  dw_i2c_plat_suspend() doesn't update the runtime PM status of
  the device, so dw_i2c_plat_resume() can't rely on this check
  during system resume.

v2 -> v3:
  Keep the ->prepare and ->complete system sleep callbacks as
  direct_complete is still useful for helping to avoid resuming
  the parent of this device in the intel-lpss case.

---
 drivers/i2c/busses/i2c-designware-core.h    |    2 +
 drivers/i2c/busses/i2c-designware-platdrv.c |   33 ++++++++++++++++------------
 2 files changed, 22 insertions(+), 13 deletions(-)

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,6 +280,8 @@ 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
@@ -451,14 +451,21 @@ static void dw_i2c_plat_complete(struct
 #endif
 
 #ifdef CONFIG_PM
-static int dw_i2c_plat_runtime_suspend(struct device *dev)
+static int dw_i2c_plat_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	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;
 }
 
@@ -467,27 +474,27 @@ static int dw_i2c_plat_resume(struct dev
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	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);
 
-	return 0;
-}
+	i_dev->suspended = false;
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_i2c_plat_suspend(struct device *dev)
-{
-	pm_runtime_resume(dev);
-	return dw_i2c_plat_runtime_suspend(dev);
+	return 0;
 }
-#endif
 
 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_runtime_suspend,
-			   dw_i2c_plat_resume,
-			   NULL)
+	SET_LATE_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)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)

Re: [RFT][PATCH v3 0/3] i2c: designware: Runtime PM aware system sleep handling

[Johannes Stezenbach]

On Wed, Sep 06, 2017 at 01:41:37AM +0200, Rafael J. Wysocki wrote:
> On Monday, September 4, 2017 1:05:57 AM CEST Rafael J. Wysocki wrote:
> > Please test if you can and let me know if anything breaks.
> 
> That still applies. :-)

Done, works for me on Asus E200HA.

In case it matters, what I tested was v4.13 + sound/topic/dollar-cove-ti-4.13-v5
(PMIC etc) + sound/topic/soc-cx2072x-4.13 (sound codec driver)
+ "S0ix blocker debug patch v3" from https://bugzilla.kernel.org/show_bug.cgi?id=193891
+ your 3 patches.

I suspend + resume a few times, no problems.  (Of course S0ix
still doesn't work according to /sys/kernel/debug/pmc_atom/sleep_state.)

Re: [RFT][PATCH v3 0/3] i2c: designware: Runtime PM aware system sleep handling

[Rafael J. Wysocki]

On Wednesday, September 6, 2017 11:16:46 AM CEST Johannes Stezenbach wrote:
> On Wed, Sep 06, 2017 at 01:41:37AM +0200, Rafael J. Wysocki wrote:
> > On Monday, September 4, 2017 1:05:57 AM CEST Rafael J. Wysocki wrote:
> > > Please test if you can and let me know if anything breaks.
> > 
> > That still applies. :-)
> 
> Done, works for me on Asus E200HA.

Thanks!

> In case it matters, what I tested was v4.13 + sound/topic/dollar-cove-ti-4.13-v5
> (PMIC etc) + sound/topic/soc-cx2072x-4.13 (sound codec driver)
> + "S0ix blocker debug patch v3" from https://bugzilla.kernel.org/show_bug.cgi?id=193891
> + your 3 patches.
> 
> I suspend + resume a few times, no problems.  (Of course S0ix
> still doesn't work according to /sys/kernel/debug/pmc_atom/sleep_state.)
> 

Well, we'll need to address the S0ix thing, but that will require some
investigation.

Can you please open a bug entry for that at bugzilla.kernel.org
(against Power Management -> Hibernation/Suspend) and CC it to me
so that it doesn't fall of the radar?

Thanks,
Rafael

Re: [RFT][PATCH v3 0/3] i2c: designware: Runtime PM aware system sleep handling

[Johannes Stezenbach]

On Wed, Sep 06, 2017 at 11:55:01AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, September 6, 2017 11:16:46 AM CEST Johannes Stezenbach wrote:
> 
> > In case it matters, what I tested was v4.13 + sound/topic/dollar-cove-ti-4.13-v5
> > (PMIC etc) + sound/topic/soc-cx2072x-4.13 (sound codec driver)
> > + "S0ix blocker debug patch v3" from https://bugzilla.kernel.org/show_bug.cgi?id=193891
> > + your 3 patches.
> > 
> > I suspend + resume a few times, no problems.  (Of course S0ix
> > still doesn't work according to /sys/kernel/debug/pmc_atom/sleep_state.)
> > 
> 
> Well, we'll need to address the S0ix thing, but that will require some
> investigation.
> 
> Can you please open a bug entry for that at bugzilla.kernel.org
> (against Power Management -> Hibernation/Suspend) and CC it to me
> so that it doesn't fall of the radar?

I'll do that tomorrow, TIA for your interest in the issue.

Today I spent several hours to bisect the S0ix "regression"
on Asus E200HA, I put regression in quotes because S0ix
only ever worked with gross hacks like poking registers
using busybox devmem.  Anyway, the gross hack worked
on v4.12 and stopped working in v4.13-rc and the
culprit is d31fd43c0f9a4 "clk: x86: Do not gate clocks
enabled by the firmware" (I'll report this properly in a seperate mail).

But I guess that's not the part you're interested in,
the bug entry should cover interaction of dw i2c, acpi pm,
acpi-lpss and ACPI OpRegion, right?


Thanks,
Johannes

Re: [RFT][PATCH v3 0/3] i2c: designware: Runtime PM aware system sleep handling

[Rafael J. Wysocki]

On Wednesday, September 6, 2017 9:59:16 PM CEST Johannes Stezenbach wrote:
> On Wed, Sep 06, 2017 at 11:55:01AM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, September 6, 2017 11:16:46 AM CEST Johannes Stezenbach wrote:
> > 
> > > In case it matters, what I tested was v4.13 + sound/topic/dollar-cove-ti-4.13-v5
> > > (PMIC etc) + sound/topic/soc-cx2072x-4.13 (sound codec driver)
> > > + "S0ix blocker debug patch v3" from https://bugzilla.kernel.org/show_bug.cgi?id=193891
> > > + your 3 patches.
> > > 
> > > I suspend + resume a few times, no problems.  (Of course S0ix
> > > still doesn't work according to /sys/kernel/debug/pmc_atom/sleep_state.)
> > > 
> > 
> > Well, we'll need to address the S0ix thing, but that will require some
> > investigation.
> > 
> > Can you please open a bug entry for that at bugzilla.kernel.org
> > (against Power Management -> Hibernation/Suspend) and CC it to me
> > so that it doesn't fall of the radar?
> 
> I'll do that tomorrow, TIA for your interest in the issue.
> 
> Today I spent several hours to bisect the S0ix "regression"
> on Asus E200HA, I put regression in quotes because S0ix
> only ever worked with gross hacks like poking registers
> using busybox devmem.  Anyway, the gross hack worked
> on v4.12 and stopped working in v4.13-rc and the
> culprit is d31fd43c0f9a4 "clk: x86: Do not gate clocks
> enabled by the firmware" (I'll report this properly in a seperate mail).

Thanks!

> But I guess that's not the part you're interested in,
> the bug entry should cover interaction of dw i2c, acpi pm,
> acpi-lpss and ACPI OpRegion, right?

Right.  Whatever information you have already and is likely to be useful in
diagnosing this.

Thanks,
Rafael

Re: [RFT][PATCH v3 0/3] i2c: designware: Runtime PM aware system sleep handling

[Johannes Stezenbach]

On Wed, Sep 06, 2017 at 11:55:01AM +0200, Rafael J. Wysocki wrote:
> Well, we'll need to address the S0ix thing, but that will require some
> investigation.
> 
> Can you please open a bug entry for that at bugzilla.kernel.org
> (against Power Management -> Hibernation/Suspend) and CC it to me
> so that it doesn't fall of the radar?

The Cc of this mail is rather long, just in case someone
is interested to know I filed bug #196861 "S0ix enablement on Intel Atom"
https://bugzilla.kernel.org/show_bug.cgi?id=196861

Re: [RFT][PATCH v3 0/3] i2c: designware: Runtime PM aware system sleep handling

[Mika Westerberg]

On Wed, Sep 06, 2017 at 01:41:37AM +0200, Rafael J. Wysocki wrote:
> > Please test if you can and let me know if anything breaks.
> 
> That still applies. :-)

Tested on Dell XPS 9550 which is using intel-lpss and everything still
works fine :)

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [RFT][PATCH v3 0/3] i2c: designware: Runtime PM aware system sleep handling

[Rafael J. Wysocki]

On Wed, Sep 6, 2017 at 1:06 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Sep 06, 2017 at 01:41:37AM +0200, Rafael J. Wysocki wrote:
>> > Please test if you can and let me know if anything breaks.
>>
>> That still applies. :-)
>
> Tested on Dell XPS 9550 which is using intel-lpss and everything still
> works fine :)
>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

OK, thanks!

Re: [RFT][PATCH v3 0/3] i2c: designware: Runtime PM aware system sleep handling

[Jarkko Nikula]

On 09/06/2017 02:06 PM, Mika Westerberg wrote:
> On Wed, Sep 06, 2017 at 01:41:37AM +0200, Rafael J. Wysocki wrote:
>>> Please test if you can and let me know if anything breaks.
>>
>> That still applies. :-)
> 
> Tested on Dell XPS 9550 which is using intel-lpss and everything still
> works fine :)
> 
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
Tested on platforms using ACPI LPSS PM domain and intel-lpss MFD driver.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

[PATCH v4 0/3] i2c: designware: Runtime PM aware system sleep handling

[Rafael J. Wysocki]

Hi,

The original purpose of this set was as stated below:

On Wednesday, September 6, 2017 1:41:37 AM CEST Rafael J. Wysocki wrote:
> On Monday, September 4, 2017 1:05:57 AM CEST Rafael J. Wysocki wrote:
> > Hi,
> 
> An update related to the dependency on intel-lpss.
> 
> > The point here is to avoid runtime resuming i2c designware devices during
> > system suspend in the driver's ->suspend callback in the case when the
> > ACPI PM domain is not present.  That will allow us to deal with the
> > ACPI PM domain case going forward, among other things.
> > 
> > The first patch cleans up the runtime PM handling in the i2c-designware-platdrv
> > _probe() routine so as to make it enable runtime PM in all cases, but prevent
> > the device from being runtime suspended via pm_runtime_forbid() if
> > pm_disabled is set.
> 
> The second one pushed the intel-lpss system suspend/resume callbacks to the
> late/early stages of suspend/resume, respectively, so that the i2c-designware-platdrv
> ones can be pushed too.
> 
> > The second one get rids of some ugly code and makes the PM callbacks of the
> > driver handle runtime-suspended devices during system suspend/resume.
> 
> This is the third one and it does less than it did before now.
> 

In the meantime, though, Rajat reported that there actually were problems with
i2c-designware being suspended too early (as in some cases dependent device
drivers attempted to use i2c during the late suspend phase of system suspend),
so here it goes again as a fix.

Patches [1-2/3] have not changed since v3 (other than by acquiring tags) and
patch [3/3] has only been rebased on top of 4.14-rc1 (and has acquired some
tags too).

Thanks,
Rafael

[PATCH v4 1/3] PM: i2c-designware-platdrv: Clean up PM handling in probe

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The power management handling in dw_i2c_plat_probe() is somewhat
messy and it is rather hard to figure out the code intention for
the case when pm_disabled is set.  In that case, the driver doesn't
enable runtime PM at all, but in addition to that it calls
pm_runtime_forbid() as though it wasn't sure if runtime PM might
be enabled for the device later by someone else.

Although that concern doesn't seem to be actually valid, the
device is clearly still expected to be PM-capable even in the
pm_disabled set case, so a better approach would be to enable
runtime PM for it unconditionally and then prevent it from
being runtime-suspended by using pm_runtime_forbid().

Make the driver do that.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Tested-by: Johannes Stezenbach <js@sig21.net>
Tested-by: Rajat Jain <rajatja@google.com>
---

v3 -> v4: Added Tested-by tags.

-> v3: Small update of the changelog.

---
 drivers/i2c/busses/i2c-designware-platdrv.c |   36 +++++++++++++++++++---------
 1 file changed, 25 insertions(+), 11 deletions(-)

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
@@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct
 	}
 }
 
+static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
+{
+	pm_runtime_get_noresume(dev->dev);
+	if (dev->pm_disabled)
+		pm_runtime_allow(dev->dev);
+
+	pm_runtime_disable(dev->dev);
+	pm_runtime_put_noidle(dev->dev);
+}
+
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -362,14 +372,19 @@ 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;
 
-	if (dev->pm_disabled) {
+	/* The code below assumes runtime PM to be disabled. */
+	WARN_ON(pm_runtime_enabled(&pdev->dev));
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	if (dev->pm_disabled)
 		pm_runtime_forbid(&pdev->dev);
-	} else {
-		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-		pm_runtime_use_autosuspend(&pdev->dev);
-		pm_runtime_set_active(&pdev->dev);
-		pm_runtime_enable(&pdev->dev);
-	}
+
+	pm_runtime_put_noidle(&pdev->dev);
 
 	if (dev->mode == DW_IC_SLAVE)
 		ret = i2c_dw_probe_slave(dev);
@@ -382,8 +397,7 @@ static int dw_i2c_plat_probe(struct plat
 	return ret;
 
 exit_probe:
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
 exit_reset:
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
@@ -402,8 +416,8 @@ static int dw_i2c_plat_remove(struct pla
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
+
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
 

Re: [PATCH v4 1/3] PM: i2c-designware-platdrv: Clean up PM handling in probe

[Ulf Hansson]

On 25 September 2017 at 01:17, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The power management handling in dw_i2c_plat_probe() is somewhat
> messy and it is rather hard to figure out the code intention for
> the case when pm_disabled is set.  In that case, the driver doesn't
> enable runtime PM at all, but in addition to that it calls
> pm_runtime_forbid() as though it wasn't sure if runtime PM might
> be enabled for the device later by someone else.
>
> Although that concern doesn't seem to be actually valid, the
> device is clearly still expected to be PM-capable even in the
> pm_disabled set case, so a better approach would be to enable
> runtime PM for it unconditionally and then prevent it from
> being runtime-suspended by using pm_runtime_forbid().

This is nice cleanup! However I have one suggestion/comment.

Using pm_runtime_forbid() to prevent the device from being runtime
suspended may be a bit fragile, as userspace can then still change to
"allow" it. Wouldn't it be better to bump the runtime PM usage count
(pm_runtime_get_noresume()) instead?

[...]

Kind regards
Uffe

Re: [PATCH v4 1/3] PM: i2c-designware-platdrv: Clean up PM handling in probe

[Rafael J. Wysocki]

On Monday, September 25, 2017 9:12:28 PM CEST Ulf Hansson wrote:
> On 25 September 2017 at 01:17, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The power management handling in dw_i2c_plat_probe() is somewhat
> > messy and it is rather hard to figure out the code intention for
> > the case when pm_disabled is set.  In that case, the driver doesn't
> > enable runtime PM at all, but in addition to that it calls
> > pm_runtime_forbid() as though it wasn't sure if runtime PM might
> > be enabled for the device later by someone else.
> >
> > Although that concern doesn't seem to be actually valid, the
> > device is clearly still expected to be PM-capable even in the
> > pm_disabled set case, so a better approach would be to enable
> > runtime PM for it unconditionally and then prevent it from
> > being runtime-suspended by using pm_runtime_forbid().
> 
> This is nice cleanup! However I have one suggestion/comment.
> 
> Using pm_runtime_forbid() to prevent the device from being runtime
> suspended may be a bit fragile, as userspace can then still change to
> "allow" it. Wouldn't it be better to bump the runtime PM usage count
> (pm_runtime_get_noresume()) instead?

Right, this is not a PCI driver. :-)

I'll send an update of this patch shortly.

Thanks,
Rafael

[Update][PATCH v5 1/3] PM: i2c-designware-platdrv: Clean up PM handling in probe

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The power management handling in dw_i2c_plat_probe() is somewhat
messy and it is rather hard to figure out the code intention for
the case when pm_disabled is set.  In that case, the driver doesn't
enable runtime PM at all, but in addition to that it calls
pm_runtime_forbid() as though it wasn't sure if runtime PM might
be enabled for the device later by someone else.

Although that concern doesn't seem to be actually valid, the
device is clearly still expected to be PM-capable even in the
pm_disabled set case, so a better approach would be to enable
runtime PM for it unconditionally and prevent it from being
runtime-suspended by using pm_runtime_get_noresume().

Make the driver do that.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v4 -> v5: Address the Ulf's comment that pm_runtime_forbid() was not
  suitable for preventing runtime suspends of the device from occurring
  (as it could be overridden by user space) and update the changelog.

  Also drop the tags as the patch changed (although that change shouldn't
  matter on systems where pm_disabled is not set).

v3 -> v4: Added Tested-by tags.

-> v3: Small update of the changelog.

---
 drivers/i2c/busses/i2c-designware-platdrv.c |   34 ++++++++++++++++++----------
 1 file changed, 22 insertions(+), 12 deletions(-)

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
@@ -249,6 +249,14 @@ static void dw_i2c_set_fifo_size(struct
 	}
 }
 
+static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
+{
+	pm_runtime_disable(dev->dev);
+
+	if (dev->pm_disabled)
+		pm_runtime_put_noidle(dev->dev);
+}
+
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -362,14 +370,17 @@ 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;
 
-	if (dev->pm_disabled) {
-		pm_runtime_forbid(&pdev->dev);
-	} else {
-		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-		pm_runtime_use_autosuspend(&pdev->dev);
-		pm_runtime_set_active(&pdev->dev);
-		pm_runtime_enable(&pdev->dev);
-	}
+	/* The code below assumes runtime PM to be disabled. */
+	WARN_ON(pm_runtime_enabled(&pdev->dev));
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+
+	if (dev->pm_disabled)
+		pm_runtime_get_noresume(&pdev->dev);
+
+	pm_runtime_enable(&pdev->dev);
 
 	if (dev->mode == DW_IC_SLAVE)
 		ret = i2c_dw_probe_slave(dev);
@@ -382,8 +393,7 @@ static int dw_i2c_plat_probe(struct plat
 	return ret;
 
 exit_probe:
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
 exit_reset:
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
@@ -402,8 +412,8 @@ static int dw_i2c_plat_remove(struct pla
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
+
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
 

Re: [Update][PATCH v5 1/3] PM: i2c-designware-platdrv: Clean up PM handling in probe

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

On Mon, Sep 25, 2017 at 11:10:06PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The power management handling in dw_i2c_plat_probe() is somewhat
> messy and it is rather hard to figure out the code intention for
> the case when pm_disabled is set.  In that case, the driver doesn't
> enable runtime PM at all, but in addition to that it calls
> pm_runtime_forbid() as though it wasn't sure if runtime PM might
> be enabled for the device later by someone else.
> 
> Although that concern doesn't seem to be actually valid, the
> device is clearly still expected to be PM-capable even in the
> pm_disabled set case, so a better approach would be to enable
> runtime PM for it unconditionally and prevent it from being
> runtime-suspended by using pm_runtime_get_noresume().
> 
> Make the driver do that.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 2/3] PM / mfd: intel-lpss: Push system sleep callbacks to late/early stages

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Push the system suspend/resume callbacks of intel-lpss to the late
suspend/early resume stages to allow child device callbacks to be
pushed to the late/early stages of suspend/resume too, so as to
make it possible to avoid resuming the children if they are runtime-
suspended during system suspend going forward.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Tested-by: Johannes Stezenbach <js@sig21.net>
Tested-by: Rajat Jain <rajatja@google.com>
---

v3 -> v4: Added Tested-by tags.

---
 drivers/mfd/intel-lpss.h |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Index: linux-pm/drivers/mfd/intel-lpss.h
===================================================================
--- linux-pm.orig/drivers/mfd/intel-lpss.h
+++ linux-pm/drivers/mfd/intel-lpss.h
@@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS			\
 	.prepare = intel_lpss_prepare,		\
-	.suspend = intel_lpss_suspend,		\
-	.resume = intel_lpss_resume,		\
-	.freeze = intel_lpss_suspend,		\
-	.thaw = intel_lpss_resume,		\
-	.poweroff = intel_lpss_suspend,		\
-	.restore = intel_lpss_resume,
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
 #endif

Re: [PATCH v4 2/3] PM / mfd: intel-lpss: Push system sleep callbacks to late/early stages

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]

On Mon, Sep 25, 2017 at 01:20:35AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Push the system suspend/resume callbacks of intel-lpss to the late
> suspend/early resume stages to allow child device callbacks to be
> pushed to the late/early stages of suspend/resume too, so as to
> make it possible to avoid resuming the children if they are runtime-
> suspended during system suspend going forward.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Johannes Stezenbach <js@sig21.net>
> Tested-by: Rajat Jain <rajatja@google.com>

If this series shall go in via I2C, I'd need an ack from Lee for this
one. Also, I'd assume this should go in for v4.14 already?

> ---
> 
> v3 -> v4: Added Tested-by tags.
> 
> ---
>  drivers/mfd/intel-lpss.h |    7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/mfd/intel-lpss.h
> ===================================================================
> --- linux-pm.orig/drivers/mfd/intel-lpss.h
> +++ linux-pm/drivers/mfd/intel-lpss.h
> @@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev
>  #ifdef CONFIG_PM_SLEEP
>  #define INTEL_LPSS_SLEEP_PM_OPS			\
>  	.prepare = intel_lpss_prepare,		\
> -	.suspend = intel_lpss_suspend,		\
> -	.resume = intel_lpss_resume,		\
> -	.freeze = intel_lpss_suspend,		\
> -	.thaw = intel_lpss_resume,		\
> -	.poweroff = intel_lpss_suspend,		\
> -	.restore = intel_lpss_resume,
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
>  #else
>  #define INTEL_LPSS_SLEEP_PM_OPS
>  #endif
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 2/3] PM / mfd: intel-lpss: Push system sleep callbacks to late/early stages

[Rafael J. Wysocki]

On Monday, September 25, 2017 9:28:47 AM CEST Wolfram Sang wrote:
> On Mon, Sep 25, 2017 at 01:20:35AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Push the system suspend/resume callbacks of intel-lpss to the late
> > suspend/early resume stages to allow child device callbacks to be
> > pushed to the late/early stages of suspend/resume too, so as to
> > make it possible to avoid resuming the children if they are runtime-
> > suspended during system suspend going forward.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Tested-by: Johannes Stezenbach <js@sig21.net>
> > Tested-by: Rajat Jain <rajatja@google.com>
> 
> If this series shall go in via I2C, I'd need an ack from Lee for this
> one.

Sure.

> Also, I'd assume this should go in for v4.14 already?

Well, it may be slightly late for that.  Realistically, the earliest -rc it
can get into seems to be -rc4 and I'd prefer it to get a bit more exposure
before it gets into a release.  It's up to you, though. :-)

Thanks,
Rafael

Re: [PATCH v4 2/3] PM / mfd: intel-lpss: Push system sleep callbacks to late/early stages

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]


> Well, it may be slightly late for that.  Realistically, the earliest -rc it
> can get into seems to be -rc4 and I'd prefer it to get a bit more exposure
> before it gets into a release.  It's up to you, though. :-)

Fine with me, I'll schedule it for v4.15 then.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 2/3] PM / mfd: intel-lpss: Push system sleep callbacks to late/early stages

[Rafael J. Wysocki]

On Mon, Sep 25, 2017 at 9:28 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Mon, Sep 25, 2017 at 01:20:35AM +0200, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Push the system suspend/resume callbacks of intel-lpss to the late
>> suspend/early resume stages to allow child device callbacks to be
>> pushed to the late/early stages of suspend/resume too, so as to
>> make it possible to avoid resuming the children if they are runtime-
>> suspended during system suspend going forward.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Tested-by: Johannes Stezenbach <js@sig21.net>
>> Tested-by: Rajat Jain <rajatja@google.com>
>
> If this series shall go in via I2C, I'd need an ack from Lee for this
> one.

So this one has been ACKed now, so the series should be good to go.

Do you need me to resend it (because of the v5 of patch [1/3])?

Thanks,
Rafael

Re: [PATCH v4 2/3] PM / mfd: intel-lpss: Push system sleep callbacks to late/early stages

[Lee Jones]

On Mon, 25 Sep 2017, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Push the system suspend/resume callbacks of intel-lpss to the late
> suspend/early resume stages to allow child device callbacks to be
> pushed to the late/early stages of suspend/resume too, so as to
> make it possible to avoid resuming the children if they are runtime-
> suspended during system suspend going forward.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Johannes Stezenbach <js@sig21.net>
> Tested-by: Rajat Jain <rajatja@google.com>
> ---
> 
> v3 -> v4: Added Tested-by tags.
> 
> ---
>  drivers/mfd/intel-lpss.h |    7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/mfd/intel-lpss.h
> ===================================================================
> --- linux-pm.orig/drivers/mfd/intel-lpss.h
> +++ linux-pm/drivers/mfd/intel-lpss.h

Weird.  What format is this?

Don't you use `git format-patch` and `git send-email`?

> @@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev
>  #ifdef CONFIG_PM_SLEEP
>  #define INTEL_LPSS_SLEEP_PM_OPS			\
>  	.prepare = intel_lpss_prepare,		\
> -	.suspend = intel_lpss_suspend,		\
> -	.resume = intel_lpss_resume,		\
> -	.freeze = intel_lpss_suspend,		\
> -	.thaw = intel_lpss_resume,		\
> -	.poweroff = intel_lpss_suspend,		\
> -	.restore = intel_lpss_resume,
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)

For the diff:

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 2/3] PM / mfd: intel-lpss: Push system sleep callbacks to late/early stages

[Rafael J. Wysocki]

On Thursday, September 28, 2017 9:19:09 PM CEST Lee Jones wrote:
> On Mon, 25 Sep 2017, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Push the system suspend/resume callbacks of intel-lpss to the late
> > suspend/early resume stages to allow child device callbacks to be
> > pushed to the late/early stages of suspend/resume too, so as to
> > make it possible to avoid resuming the children if they are runtime-
> > suspended during system suspend going forward.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Tested-by: Johannes Stezenbach <js@sig21.net>
> > Tested-by: Rajat Jain <rajatja@google.com>
> > ---
> > 
> > v3 -> v4: Added Tested-by tags.
> > 
> > ---
> >  drivers/mfd/intel-lpss.h |    7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > Index: linux-pm/drivers/mfd/intel-lpss.h
> > ===================================================================
> > --- linux-pm.orig/drivers/mfd/intel-lpss.h
> > +++ linux-pm/drivers/mfd/intel-lpss.h
> 
> Weird.  What format is this?
> 
> Don't you use `git format-patch` and `git send-email`?

No, I don't.

I'm an old-timer, I use quilt. :-)

> > @@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev
> >  #ifdef CONFIG_PM_SLEEP
> >  #define INTEL_LPSS_SLEEP_PM_OPS			\
> >  	.prepare = intel_lpss_prepare,		\
> > -	.suspend = intel_lpss_suspend,		\
> > -	.resume = intel_lpss_resume,		\
> > -	.freeze = intel_lpss_suspend,		\
> > -	.thaw = intel_lpss_resume,		\
> > -	.poweroff = intel_lpss_suspend,		\
> > -	.restore = intel_lpss_resume,
> > +	SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
> 
> For the diff:
> 
> Acked-by: Lee Jones <lee.jones@linaro.org>

Thank you!

Re: [PATCH v4 2/3] PM / mfd: intel-lpss: Push system sleep callbacks to late/early stages

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

On Mon, Sep 25, 2017 at 01:20:35AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Push the system suspend/resume callbacks of intel-lpss to the late
> suspend/early resume stages to allow child device callbacks to be
> pushed to the late/early stages of suspend/resume too, so as to
> make it possible to avoid resuming the children if they are runtime-
> suspended during system suspend going forward.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Johannes Stezenbach <js@sig21.net>
> Tested-by: Rajat Jain <rajatja@google.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 3/3] PM: i2c-designware-platdrv: Suspend/resume at the late/early stages

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

As reported by Rajat Jain, there are problems when ACPI operation
region handlers or similar, called at the ->resume_early() time, for
I2C client devices try to access an I2C controller that has already
been suspended at that point.  To avoid that, move the suspend/resume
of i2c-designware-platdrv to the late/early stages, respectively.

While at it, avoid resuming the device from runtime suspend in the
driver's ->suspend callback which isn't particularly nice.  [A better
approach would be to make the driver track the PM state of the device
so that it doesn't need to resume it in ->suspend, so implement it.]

First, drop dw_i2c_plat_suspend() added by commit a23318feeff6 (i2c:
designware: Fix system suspend) and rename dw_i2c_plat_runtime_suspend()
back to dw_i2c_plat_suspend().

Second, point the driver's ->late_suspend and ->early_resume
callbacks, rather than its ->suspend and ->resume callbacks,
to dw_i2c_plat_suspend() and dw_i2c_plat_resume(), respectively,
so that they are not executed in parallel with each other, for
example if runtime resume of the device takes place during system
suspend.

Finally, add "suspended" and "skip_resume" flags to struct dw_i2c_dev
and make dw_i2c_plat_suspend() and dw_i2c_plat_resume() use them to
avoid suspending or resuming the device twice in a row and to avoid
resuming a previously runtime-suspended device during system resume.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Tested-by: Johannes Stezenbach <js@sig21.net>
Tested-by: Rajat Jain <rajatja@google.com>
---

v3 ->v4: Modify changelog to mention the problem addressed by this
  patch, rebase on top of 4.14-rc1 and add Tested-by tags.

v2 -> v3:
  Do not drop ->prepare and ->complete system sleep callbacks as
  direct_complete is still useful for helping to avoid resuming
  the parent of this device in the intel-lpss case.

-> v2:
  Add the "suspended" flag, which is needed, because
  dw_i2c_plat_suspend() doesn't update the runtime PM status of
  the device, so dw_i2c_plat_resume() can't rely on this check
  during system resume.

---
 drivers/i2c/busses/i2c-designware-core.h    |    2 +
 drivers/i2c/busses/i2c-designware-platdrv.c |   33 ++++++++++++++++------------
 2 files changed, 22 insertions(+), 13 deletions(-)

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,6 +280,8 @@ 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
@@ -451,13 +451,20 @@ static void dw_i2c_plat_complete(struct
 #endif
 
 #ifdef CONFIG_PM
-static int dw_i2c_plat_runtime_suspend(struct device *dev)
+static int dw_i2c_plat_suspend(struct device *dev)
 {
 	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;
 }
 
@@ -465,27 +472,27 @@ 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);
 
-	return 0;
-}
+	i_dev->suspended = false;
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_i2c_plat_suspend(struct device *dev)
-{
-	pm_runtime_resume(dev);
-	return dw_i2c_plat_runtime_suspend(dev);
+	return 0;
 }
-#endif
 
 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_runtime_suspend,
-			   dw_i2c_plat_resume,
-			   NULL)
+	SET_LATE_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)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)

Re: [PATCH v4 3/3] PM: i2c-designware-platdrv: Suspend/resume at the late/early stages

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

On Mon, Sep 25, 2017 at 01:30:51AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> As reported by Rajat Jain, there are problems when ACPI operation
> region handlers or similar, called at the ->resume_early() time, for
> I2C client devices try to access an I2C controller that has already
> been suspended at that point.  To avoid that, move the suspend/resume
> of i2c-designware-platdrv to the late/early stages, respectively.
> 
> While at it, avoid resuming the device from runtime suspend in the
> driver's ->suspend callback which isn't particularly nice.  [A better
> approach would be to make the driver track the PM state of the device
> so that it doesn't need to resume it in ->suspend, so implement it.]
> 
> First, drop dw_i2c_plat_suspend() added by commit a23318feeff6 (i2c:
> designware: Fix system suspend) and rename dw_i2c_plat_runtime_suspend()
> back to dw_i2c_plat_suspend().
> 
> Second, point the driver's ->late_suspend and ->early_resume
> callbacks, rather than its ->suspend and ->resume callbacks,
> to dw_i2c_plat_suspend() and dw_i2c_plat_resume(), respectively,
> so that they are not executed in parallel with each other, for
> example if runtime resume of the device takes place during system
> suspend.
> 
> Finally, add "suspended" and "skip_resume" flags to struct dw_i2c_dev
> and make dw_i2c_plat_suspend() and dw_i2c_plat_resume() use them to
> avoid suspending or resuming the device twice in a row and to avoid
> resuming a previously runtime-suspended device during system resume.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Johannes Stezenbach <js@sig21.net>
> Tested-by: Rajat Jain <rajatja@google.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]