Message ID | 1729594.8Da7J7iljp@aspire.rjw.lan (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
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.
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)