Message ID | 1729594.8Da7J7iljp@aspire.rjw.lan |
---|---|
State | Superseded |
Headers | show |
Series | None | expand |
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.
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!
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 ...
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().
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?
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?
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.
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, \
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?
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)
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
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.
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)