diff mbox series

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

Message ID 1729594.8Da7J7iljp@aspire.rjw.lan
State Superseded
Headers show
Series None | expand

Commit Message

Rafael J. Wysocki Sept. 4, 2017, 10:01 a.m. UTC 
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(-)

Comments

Mika Westerberg Sept. 5, 2017, 2:45 p.m. UTC | #1 
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.
Rafael J. Wysocki Sept. 5, 2017, 2:46 p.m. UTC | #2 
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!
Rafael J. Wysocki Sept. 5, 2017, 2:55 p.m. UTC | #3 
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 ...
Mika Westerberg Sept. 5, 2017, 2:58 p.m. UTC | #4 
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().
Rafael J. Wysocki Sept. 5, 2017, 3:02 p.m. UTC | #5 
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?
Rafael J. Wysocki Sept. 5, 2017, 3:04 p.m. UTC | #6 
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?
Mika Westerberg Sept. 5, 2017, 3:07 p.m. UTC | #7 
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.
Mika Westerberg Sept. 5, 2017, 3:24 p.m. UTC | #8 
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,		\
Rafael J. Wysocki Sept. 5, 2017, 3:32 p.m. UTC | #9 
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?
Mika Westerberg Sept. 5, 2017, 3:41 p.m. UTC | #10 
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)
Johannes Stezenbach Sept. 5, 2017, 9 p.m. UTC | #11 
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
Rafael J. Wysocki Sept. 5, 2017, 9:22 p.m. UTC | #12 
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.
diff mbox series

Patch

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)