diff mbox

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

Message ID 20170905210034.57b4pqg3suz4m3bl@sig21.net (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Johannes Stezenbach Sept. 5, 2017, 9 p.m. UTC
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)


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

Comments

Rafael J. Wysocki Sept. 5, 2017, 9:22 p.m. UTC | #1
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

Patch

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