| Message ID | 20170330120444.12499-1-jarkko.nikula@linux.intel.com |
|---|---|
| State | Rejected |
| Headers | show |
On Thu, Mar 30, 2017 at 5:04 AM, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > There is possibility to enter dw_i2c_plat_suspend() callback twice > during system suspend under certain cases which is causing here warnings > from clk_core_disable() and clk_core_unprepare() as well as accessing the > registers that can be power gated. > > Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during > system suspend") implemented a prepare callback that checks for runtime > suspended device which allow PM core to set direct_complete flag and > skip system suspend and resume callbacks. > > However it can still happen if nothing resumes the device prior system > syspend (e.g. acpi_subsys_suspend()) and there is a slave device which > unsets the direct_complete flag of the parent in __device_suspend() thus > causing PM code to not skip the system suspend/resume callbacks. > > Avoid this by checking runtime status in suspend and resume callbacks > and return directly if device is runtime suspended. This affects only > system suspend/resume since during runtime suspend/resume runtime status > is suspending (not suspended) or resuming. > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> This works for me on my HiKey board! Tested-by: John Stultz <john.stultz@linaro.org> thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 30, 2017 at 03:04:44PM +0300, Jarkko Nikula wrote: > There is possibility to enter dw_i2c_plat_suspend() callback twice > during system suspend under certain cases which is causing here warnings > from clk_core_disable() and clk_core_unprepare() as well as accessing the > registers that can be power gated. > > Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during > system suspend") implemented a prepare callback that checks for runtime > suspended device which allow PM core to set direct_complete flag and > skip system suspend and resume callbacks. > > However it can still happen if nothing resumes the device prior system > syspend (e.g. acpi_subsys_suspend()) and there is a slave device which > unsets the direct_complete flag of the parent in __device_suspend() thus > causing PM code to not skip the system suspend/resume callbacks. > > Avoid this by checking runtime status in suspend and resume callbacks > and return directly if device is runtime suspended. This affects only > system suspend/resume since during runtime suspend/resume runtime status > is suspending (not suspended) or resuming. > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > --- > I'm able to trigger system suspend callback while device is runtime > suspended by removing the pm_runtime_resume() call from > drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C > slave (ACPI enumerated but doesn't bind due an error in probe function). > In that case __device_suspend() for that unbound device has NULL suspend > callback, and thus doesn't cause any runtime resume chain but still unsets > the parent's direct_complete flag. > John Stult <john.stultz@linaro.org> has reported he can trigger this on > HiKey board too. > > I'm not sure is this the right thing to do. It feels something the PM core > should do but I'm not sure that either. One alternative could be to resume > runtime suspended parent in in __device_suspend() right after where > parent's direct_complete flag is unset. Because of the last paragraph, I'd like a positive comment or tag from one of the PM people before I apply this one. > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index a597ba32de7e..42a9cd09aa64 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -377,6 +377,9 @@ 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_suspended(dev)) > + return 0; > + > i2c_dw_disable(i_dev); > i2c_dw_plat_prepare_clk(i_dev, false); > > @@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev) > struct platform_device *pdev = to_platform_device(dev); > struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); > > + if (pm_runtime_suspended(dev)) > + return 0; > + > i2c_dw_plat_prepare_clk(i_dev, true); > i2c_dw_init(i_dev); > > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 30, 2017 at 2:04 PM, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > There is possibility to enter dw_i2c_plat_suspend() callback twice > during system suspend under certain cases which is causing here warnings > from clk_core_disable() and clk_core_unprepare() as well as accessing the > registers that can be power gated. > > Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during > system suspend") implemented a prepare callback that checks for runtime > suspended device which allow PM core to set direct_complete flag and > skip system suspend and resume callbacks. > > However it can still happen if nothing resumes the device prior system > syspend (e.g. acpi_subsys_suspend()) and there is a slave device which > unsets the direct_complete flag of the parent in __device_suspend() thus > causing PM code to not skip the system suspend/resume callbacks. > > Avoid this by checking runtime status in suspend and resume callbacks > and return directly if device is runtime suspended. This affects only > system suspend/resume since during runtime suspend/resume runtime status > is suspending (not suspended) or resuming. > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > --- > I'm able to trigger system suspend callback while device is runtime > suspended by removing the pm_runtime_resume() call from > drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C > slave (ACPI enumerated but doesn't bind due an error in probe function). > In that case __device_suspend() for that unbound device has NULL suspend > callback, and thus doesn't cause any runtime resume chain but still unsets > the parent's direct_complete flag. > John Stult <john.stultz@linaro.org> has reported he can trigger this on > HiKey board too. > > I'm not sure is this the right thing to do. It feels something the PM core > should do but I'm not sure that either. One alternative could be to resume > runtime suspended parent in in __device_suspend() right after where > parent's direct_complete flag is unset. In that case the core expects that the ->prepare callback for the slave will also return 1 (or a positive number in general). If that doesn't happen, then from the core's perspective it is not safe to allow the master's system PM callbacks to be skipped and that's why direct_complete is unset for it. > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index a597ba32de7e..42a9cd09aa64 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -377,6 +377,9 @@ 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_suspended(dev)) > + return 0; > + > i2c_dw_disable(i_dev); > i2c_dw_plat_prepare_clk(i_dev, false); > > @@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev) > struct platform_device *pdev = to_platform_device(dev); > struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); > > + if (pm_runtime_suspended(dev)) > + return 0; > + > i2c_dw_plat_prepare_clk(i_dev, true); > i2c_dw_init(i_dev); > > -- > 2.11.0 > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 19, 2017 at 8:59 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Thu, Mar 30, 2017 at 03:04:44PM +0300, Jarkko Nikula wrote: >> There is possibility to enter dw_i2c_plat_suspend() callback twice >> during system suspend under certain cases which is causing here warnings >> from clk_core_disable() and clk_core_unprepare() as well as accessing the >> registers that can be power gated. >> >> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during >> system suspend") implemented a prepare callback that checks for runtime >> suspended device which allow PM core to set direct_complete flag and >> skip system suspend and resume callbacks. >> >> However it can still happen if nothing resumes the device prior system >> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which >> unsets the direct_complete flag of the parent in __device_suspend() thus >> causing PM code to not skip the system suspend/resume callbacks. >> >> Avoid this by checking runtime status in suspend and resume callbacks >> and return directly if device is runtime suspended. This affects only >> system suspend/resume since during runtime suspend/resume runtime status >> is suspending (not suspended) or resuming. >> >> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> --- >> I'm able to trigger system suspend callback while device is runtime >> suspended by removing the pm_runtime_resume() call from >> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C >> slave (ACPI enumerated but doesn't bind due an error in probe function). >> In that case __device_suspend() for that unbound device has NULL suspend >> callback, and thus doesn't cause any runtime resume chain but still unsets >> the parent's direct_complete flag. >> John Stult <john.stultz@linaro.org> has reported he can trigger this on >> HiKey board too. >> >> I'm not sure is this the right thing to do. It feels something the PM core >> should do but I'm not sure that either. One alternative could be to resume >> runtime suspended parent in in __device_suspend() right after where >> parent's direct_complete flag is unset. > > Because of the last paragraph, I'd like a positive comment or tag from > one of the PM people before I apply this one. No, sorry. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On 04/19/2017 11:24 PM, Rafael J. Wysocki wrote: > On Thu, Mar 30, 2017 at 2:04 PM, Jarkko Nikula > <jarkko.nikula@linux.intel.com> wrote: >> There is possibility to enter dw_i2c_plat_suspend() callback twice >> during system suspend under certain cases which is causing here warnings >> from clk_core_disable() and clk_core_unprepare() as well as accessing the >> registers that can be power gated. >> >> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during >> system suspend") implemented a prepare callback that checks for runtime >> suspended device which allow PM core to set direct_complete flag and >> skip system suspend and resume callbacks. >> >> However it can still happen if nothing resumes the device prior system >> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which >> unsets the direct_complete flag of the parent in __device_suspend() thus >> causing PM code to not skip the system suspend/resume callbacks. >> >> Avoid this by checking runtime status in suspend and resume callbacks >> and return directly if device is runtime suspended. This affects only >> system suspend/resume since during runtime suspend/resume runtime status >> is suspending (not suspended) or resuming. >> >> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> --- >> I'm able to trigger system suspend callback while device is runtime >> suspended by removing the pm_runtime_resume() call from >> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C >> slave (ACPI enumerated but doesn't bind due an error in probe function). >> In that case __device_suspend() for that unbound device has NULL suspend >> callback, and thus doesn't cause any runtime resume chain but still unsets >> the parent's direct_complete flag. >> John Stult <john.stultz@linaro.org> has reported he can trigger this on >> HiKey board too. >> >> I'm not sure is this the right thing to do. It feels something the PM core >> should do but I'm not sure that either. One alternative could be to resume >> runtime suspended parent in in __device_suspend() right after where >> parent's direct_complete flag is unset. > > In that case the core expects that the ->prepare callback for the > slave will also return 1 (or a positive number in general). > > If that doesn't happen, then from the core's perspective it is not > safe to allow the master's system PM callbacks to be skipped and > that's why direct_complete is unset for it. > So it's then right thing to check runtime PM status in driver as patch does below? >> --- >> drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c >> index a597ba32de7e..42a9cd09aa64 100644 >> --- a/drivers/i2c/busses/i2c-designware-platdrv.c >> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c >> @@ -377,6 +377,9 @@ 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_suspended(dev)) >> + return 0; >> + >> i2c_dw_disable(i_dev); >> i2c_dw_plat_prepare_clk(i_dev, false); >> >> @@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev) >> struct platform_device *pdev = to_platform_device(dev); >> struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); >> >> + if (pm_runtime_suspended(dev)) >> + return 0; >> + >> i2c_dw_plat_prepare_clk(i_dev, true); >> i2c_dw_init(i_dev);
On Thu, Apr 20, 2017 at 9:25 AM, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > Hi > > > On 04/19/2017 11:24 PM, Rafael J. Wysocki wrote: >> >> On Thu, Mar 30, 2017 at 2:04 PM, Jarkko Nikula >> <jarkko.nikula@linux.intel.com> wrote: >>> >>> There is possibility to enter dw_i2c_plat_suspend() callback twice >>> during system suspend under certain cases which is causing here warnings >>> from clk_core_disable() and clk_core_unprepare() as well as accessing the >>> registers that can be power gated. >>> >>> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during >>> system suspend") implemented a prepare callback that checks for runtime >>> suspended device which allow PM core to set direct_complete flag and >>> skip system suspend and resume callbacks. >>> >>> However it can still happen if nothing resumes the device prior system >>> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which >>> unsets the direct_complete flag of the parent in __device_suspend() thus >>> causing PM code to not skip the system suspend/resume callbacks. >>> >>> Avoid this by checking runtime status in suspend and resume callbacks >>> and return directly if device is runtime suspended. This affects only >>> system suspend/resume since during runtime suspend/resume runtime status >>> is suspending (not suspended) or resuming. >>> >>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >>> --- >>> I'm able to trigger system suspend callback while device is runtime >>> suspended by removing the pm_runtime_resume() call from >>> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C >>> slave (ACPI enumerated but doesn't bind due an error in probe function). >>> In that case __device_suspend() for that unbound device has NULL suspend >>> callback, and thus doesn't cause any runtime resume chain but still >>> unsets >>> the parent's direct_complete flag. >>> John Stult <john.stultz@linaro.org> has reported he can trigger this on >>> HiKey board too. >>> >>> I'm not sure is this the right thing to do. It feels something the PM >>> core >>> should do but I'm not sure that either. One alternative could be to >>> resume >>> runtime suspended parent in in __device_suspend() right after where >>> parent's direct_complete flag is unset. >> >> >> In that case the core expects that the ->prepare callback for the >> slave will also return 1 (or a positive number in general). >> >> If that doesn't happen, then from the core's perspective it is not >> safe to allow the master's system PM callbacks to be skipped and >> that's why direct_complete is unset for it. >> > So it's then right thing to check runtime PM status in driver as patch does > below? If you know for a fact that none of the device's children and none of the children thereof and so on and nothing that may depend on the device via a device_link, either directly or indirectly, will ever need to be resumed during system suspend, then yes, it is. Otherwise, no, it isn't. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/20/2017 01:33 PM, Rafael J. Wysocki wrote: > On Thu, Apr 20, 2017 at 9:25 AM, Jarkko Nikula > <jarkko.nikula@linux.intel.com> wrote: >> Hi >> >> >> On 04/19/2017 11:24 PM, Rafael J. Wysocki wrote: >>> >>> On Thu, Mar 30, 2017 at 2:04 PM, Jarkko Nikula >>> <jarkko.nikula@linux.intel.com> wrote: >>>> >>>> There is possibility to enter dw_i2c_plat_suspend() callback twice >>>> during system suspend under certain cases which is causing here warnings >>>> from clk_core_disable() and clk_core_unprepare() as well as accessing the >>>> registers that can be power gated. >>>> >>>> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during >>>> system suspend") implemented a prepare callback that checks for runtime >>>> suspended device which allow PM core to set direct_complete flag and >>>> skip system suspend and resume callbacks. >>>> >>>> However it can still happen if nothing resumes the device prior system >>>> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which >>>> unsets the direct_complete flag of the parent in __device_suspend() thus >>>> causing PM code to not skip the system suspend/resume callbacks. >>>> >>>> Avoid this by checking runtime status in suspend and resume callbacks >>>> and return directly if device is runtime suspended. This affects only >>>> system suspend/resume since during runtime suspend/resume runtime status >>>> is suspending (not suspended) or resuming. >>>> >>>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >>>> --- >>>> I'm able to trigger system suspend callback while device is runtime >>>> suspended by removing the pm_runtime_resume() call from >>>> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C >>>> slave (ACPI enumerated but doesn't bind due an error in probe function). >>>> In that case __device_suspend() for that unbound device has NULL suspend >>>> callback, and thus doesn't cause any runtime resume chain but still >>>> unsets >>>> the parent's direct_complete flag. >>>> John Stult <john.stultz@linaro.org> has reported he can trigger this on >>>> HiKey board too. >>>> >>>> I'm not sure is this the right thing to do. It feels something the PM >>>> core >>>> should do but I'm not sure that either. One alternative could be to >>>> resume >>>> runtime suspended parent in in __device_suspend() right after where >>>> parent's direct_complete flag is unset. >>> >>> >>> In that case the core expects that the ->prepare callback for the >>> slave will also return 1 (or a positive number in general). >>> >>> If that doesn't happen, then from the core's perspective it is not >>> safe to allow the master's system PM callbacks to be skipped and >>> that's why direct_complete is unset for it. >>> >> So it's then right thing to check runtime PM status in driver as patch does >> below? > > If you know for a fact that none of the device's children and none of > the children thereof and so on and nothing that may depend on the > device via a device_link, either directly or indirectly, will ever > need to be resumed during system suspend, then yes, it is. > > Otherwise, no, it isn't. > If you were wondering the pm_runtime_suspended() check in dw_i2c_plat_resume() that too was for skipping double callback by system resume followed by later runtime resume leading to ever increasing clock enable count over repeated system suspend/resume cycles. Anyway, normal case here is we do runtime resume during system suspend from a few places. E.g. ACPI enumerated slave does suspend through acpi_subsys_suspend() chain and for Intel platforms i2c-designware is resumer either from drivers/acpi/acpi_lpss.c or drivers/mfd/intel-lpss.c. If we are doing expected suspend/resume callback then our runtime_status is other than RPM_SUSPENDED and pm_runtime_suspended() returns false.
On 30 March 2017 at 14:04, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > There is possibility to enter dw_i2c_plat_suspend() callback twice > during system suspend under certain cases which is causing here warnings > from clk_core_disable() and clk_core_unprepare() as well as accessing the > registers that can be power gated. > > Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during > system suspend") implemented a prepare callback that checks for runtime > suspended device which allow PM core to set direct_complete flag and > skip system suspend and resume callbacks. > > However it can still happen if nothing resumes the device prior system > syspend (e.g. acpi_subsys_suspend()) and there is a slave device which > unsets the direct_complete flag of the parent in __device_suspend() thus > causing PM code to not skip the system suspend/resume callbacks. > > Avoid this by checking runtime status in suspend and resume callbacks > and return directly if device is runtime suspended. This affects only > system suspend/resume since during runtime suspend/resume runtime status > is suspending (not suspended) or resuming. > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > --- > I'm able to trigger system suspend callback while device is runtime > suspended by removing the pm_runtime_resume() call from > drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C > slave (ACPI enumerated but doesn't bind due an error in probe function). > In that case __device_suspend() for that unbound device has NULL suspend > callback, and thus doesn't cause any runtime resume chain but still unsets > the parent's direct_complete flag. > John Stult <john.stultz@linaro.org> has reported he can trigger this on > HiKey board too. > > I'm not sure is this the right thing to do. It feels something the PM core > should do but I'm not sure that either. One alternative could be to resume > runtime suspended parent in in __device_suspend() right after where > parent's direct_complete flag is unset. > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index a597ba32de7e..42a9cd09aa64 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -377,6 +377,9 @@ 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_suspended(dev)) > + return 0; This looks weird. I don't find any other drivers that needs this, what is so different with this one? > + > i2c_dw_disable(i_dev); > i2c_dw_plat_prepare_clk(i_dev, false); > > @@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev) > struct platform_device *pdev = to_platform_device(dev); > struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); > > + if (pm_runtime_suspended(dev)) > + return 0; > + Ditto. > i2c_dw_plat_prepare_clk(i_dev, true); > i2c_dw_init(i_dev); > > -- > 2.11.0 > I have been following the development for the i2c-designware driver for a while. Some time back I also tried to fix the similar problems as you are currently [1] are. Back then, I picked the same approach as John Stultz did recently [2]. To summarize my view, I don't understand the justification of using the direct_complete feature for i2c-designware. To me, it just add complexity to the driver that we really should try to avoid. I think we need something else here. To me, the proper solution is to use the pm_runtime_force_suspend|resume() helpers to deal with system suspend/resume. However I understand that the behavior of the ACPI PM domain currently prevents us from doing this. That said, perhaps we should instead try to make the ACPI PM domain to better collaborate with drivers using pm_runtime_force_suspend|resume()? I have been investigating that and started to cook some patches, although I have not yet been able to post something. If you think it could make sense, I can pick it up. [1] https://www.spinics.net/lists/arm-kernel/msg511277.html [2] https://patchwork.ozlabs.org/patch/745041/ Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On 04/25/2017 12:24 PM, Ulf Hansson wrote: > On 30 March 2017 at 14:04, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: >> There is possibility to enter dw_i2c_plat_suspend() callback twice >> during system suspend under certain cases which is causing here warnings >> from clk_core_disable() and clk_core_unprepare() as well as accessing the >> registers that can be power gated. >> >> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during >> system suspend") implemented a prepare callback that checks for runtime >> suspended device which allow PM core to set direct_complete flag and >> skip system suspend and resume callbacks. >> >> However it can still happen if nothing resumes the device prior system >> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which >> unsets the direct_complete flag of the parent in __device_suspend() thus >> causing PM code to not skip the system suspend/resume callbacks. >> >> Avoid this by checking runtime status in suspend and resume callbacks >> and return directly if device is runtime suspended. This affects only >> system suspend/resume since during runtime suspend/resume runtime status >> is suspending (not suspended) or resuming. >> >> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> --- >> I'm able to trigger system suspend callback while device is runtime >> suspended by removing the pm_runtime_resume() call from >> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C >> slave (ACPI enumerated but doesn't bind due an error in probe function). >> In that case __device_suspend() for that unbound device has NULL suspend >> callback, and thus doesn't cause any runtime resume chain but still unsets >> the parent's direct_complete flag. >> John Stult <john.stultz@linaro.org> has reported he can trigger this on >> HiKey board too. >> >> I'm not sure is this the right thing to do. It feels something the PM core >> should do but I'm not sure that either. One alternative could be to resume >> runtime suspended parent in in __device_suspend() right after where >> parent's direct_complete flag is unset. >> --- >> drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c >> index a597ba32de7e..42a9cd09aa64 100644 >> --- a/drivers/i2c/busses/i2c-designware-platdrv.c >> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c >> @@ -377,6 +377,9 @@ 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_suspended(dev)) >> + return 0; > > This looks weird. I don't find any other drivers that needs this, what > is so different with this one? > There are some drivers that are checking pm_runtime_suspended() or pm_runtime_status_suspended() in their suspend/resume callbacks and doing things only when they return false so the problem is not so unique here. I didn't try to find are there drivers that are using some own state variable for the same purpose but I wouldn't surprise if there are. > I have been following the development for the i2c-designware driver > for a while. Some time back I also tried to fix the similar problems > as you are currently [1] are. Back then, I picked the same approach as > John Stultz did recently [2]. > > To summarize my view, I don't understand the justification of using > the direct_complete feature for i2c-designware. To me, it just add > complexity to the driver that we really should try to avoid. I think > we need something else here. > Dates back to commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") but I agree it's worth to consider. > To me, the proper solution is to use the > pm_runtime_force_suspend|resume() helpers to deal with system > suspend/resume. However I understand that the behavior of the ACPI PM > domain currently prevents us from doing this. That said, perhaps we > should instead try to make the ACPI PM domain to better collaborate > with drivers using pm_runtime_force_suspend|resume()? I have been > investigating that and started to cook some patches, although I have > not yet been able to post something. If you think it could make sense, > I can pick it up. > That's a good idea. I didn't think about it at all.
On 25 April 2017 at 13:08, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > Hi > > > On 04/25/2017 12:24 PM, Ulf Hansson wrote: >> >> On 30 March 2017 at 14:04, Jarkko Nikula <jarkko.nikula@linux.intel.com> >> wrote: >>> >>> There is possibility to enter dw_i2c_plat_suspend() callback twice >>> during system suspend under certain cases which is causing here warnings >>> from clk_core_disable() and clk_core_unprepare() as well as accessing the >>> registers that can be power gated. >>> >>> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during >>> system suspend") implemented a prepare callback that checks for runtime >>> suspended device which allow PM core to set direct_complete flag and >>> skip system suspend and resume callbacks. >>> >>> However it can still happen if nothing resumes the device prior system >>> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which >>> unsets the direct_complete flag of the parent in __device_suspend() thus >>> causing PM code to not skip the system suspend/resume callbacks. >>> >>> Avoid this by checking runtime status in suspend and resume callbacks >>> and return directly if device is runtime suspended. This affects only >>> system suspend/resume since during runtime suspend/resume runtime status >>> is suspending (not suspended) or resuming. >>> >>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >>> --- >>> I'm able to trigger system suspend callback while device is runtime >>> suspended by removing the pm_runtime_resume() call from >>> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C >>> slave (ACPI enumerated but doesn't bind due an error in probe function). >>> In that case __device_suspend() for that unbound device has NULL suspend >>> callback, and thus doesn't cause any runtime resume chain but still >>> unsets >>> the parent's direct_complete flag. >>> John Stult <john.stultz@linaro.org> has reported he can trigger this on >>> HiKey board too. >>> >>> I'm not sure is this the right thing to do. It feels something the PM >>> core >>> should do but I'm not sure that either. One alternative could be to >>> resume >>> runtime suspended parent in in __device_suspend() right after where >>> parent's direct_complete flag is unset. >>> --- >>> drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c >>> b/drivers/i2c/busses/i2c-designware-platdrv.c >>> index a597ba32de7e..42a9cd09aa64 100644 >>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c >>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c >>> @@ -377,6 +377,9 @@ 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_suspended(dev)) >>> + return 0; >> >> >> This looks weird. I don't find any other drivers that needs this, what >> is so different with this one? >> > There are some drivers that are checking pm_runtime_suspended() or > pm_runtime_status_suspended() in their suspend/resume callbacks and doing > things only when they return false so the problem is not so unique here. > > I didn't try to find are there drivers that are using some own state > variable for the same purpose but I wouldn't surprise if there are. > >> I have been following the development for the i2c-designware driver >> for a while. Some time back I also tried to fix the similar problems >> as you are currently [1] are. Back then, I picked the same approach as >> John Stultz did recently [2]. >> >> To summarize my view, I don't understand the justification of using >> the direct_complete feature for i2c-designware. To me, it just add >> complexity to the driver that we really should try to avoid. I think >> we need something else here. >> > Dates back to commit 8503ff166504 ("i2c: designware: Avoid unnecessary > resuming during system suspend") but I agree it's worth to consider. > >> To me, the proper solution is to use the >> pm_runtime_force_suspend|resume() helpers to deal with system >> suspend/resume. However I understand that the behavior of the ACPI PM >> domain currently prevents us from doing this. That said, perhaps we >> should instead try to make the ACPI PM domain to better collaborate >> with drivers using pm_runtime_force_suspend|resume()? I have been >> investigating that and started to cook some patches, although I have >> not yet been able to post something. If you think it could make sense, >> I can pick it up. >> > That's a good idea. I didn't think about it at all. Okay, thanks! I will continue to look into this and try to submit something within a reasonable time frame, keep you posted. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-04-25 at 13:12 +0200, Ulf Hansson wrote: > On 25 April 2017 at 13:08, Jarkko Nikula <jarkko.nikula@linux.intel.co > m> wrote: > > On 04/25/2017 12:24 PM, Ulf Hansson wrote: > > > To me, the proper solution is to use the > > > pm_runtime_force_suspend|resume() helpers to deal with system > > > suspend/resume. However I understand that the behavior of the ACPI > > > PM > > > domain currently prevents us from doing this. That said, perhaps > > > we > > > should instead try to make the ACPI PM domain to better > > > collaborate > > > with drivers using pm_runtime_force_suspend|resume()? I have been > > > investigating that and started to cook some patches, although I > > > have > > > not yet been able to post something. If you think it could make > > > sense, > > > I can pick it up. > > > > > > > That's a good idea. I didn't think about it at all. > > Okay, thanks! I will continue to look into this and try to submit > something within a reasonable time frame, keep you posted. But please keep in mind that ACPI PM domain for BayTrail / CherryTrail is used by several IPs including most nasty GPDMA case. If something breaks that it would be show stopper.
On 25 April 2017 at 13:36, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, 2017-04-25 at 13:12 +0200, Ulf Hansson wrote: >> On 25 April 2017 at 13:08, Jarkko Nikula <jarkko.nikula@linux.intel.co >> m> wrote: >> > On 04/25/2017 12:24 PM, Ulf Hansson wrote: > >> > > To me, the proper solution is to use the >> > > pm_runtime_force_suspend|resume() helpers to deal with system >> > > suspend/resume. However I understand that the behavior of the ACPI >> > > PM >> > > domain currently prevents us from doing this. That said, perhaps >> > > we >> > > should instead try to make the ACPI PM domain to better >> > > collaborate >> > > with drivers using pm_runtime_force_suspend|resume()? I have been >> > > investigating that and started to cook some patches, although I >> > > have >> > > not yet been able to post something. If you think it could make >> > > sense, >> > > I can pick it up. >> > > >> > >> > That's a good idea. I didn't think about it at all. >> >> Okay, thanks! I will continue to look into this and try to submit >> something within a reasonable time frame, keep you posted. > > But please keep in mind that ACPI PM domain for BayTrail / CherryTrail > is used by several IPs including most nasty GPDMA case. If something > breaks that it would be show stopper. Thanks for pointing this out. I think I have an overall understanding of the complexity. This driver is a cross SoC driver and not only should we expect the ACPI PM domain to be used but also the generic PM domain for ARM SoCs. To me, this makes it even more interesting and a very good candidate to explore the usage of pm_runtime_force_suspend|resume(). :-) Nevertheless, I will rely and appreciate help in testing and of course also reviewing. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c >>>> b/drivers/i2c/busses/i2c-designware-platdrv.c >>>> index a597ba32de7e..42a9cd09aa64 100644 >>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c >>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c >>>> @@ -377,6 +377,9 @@ 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_suspended(dev)) >>>> + return 0; >>> >>> >>> This looks weird. I don't find any other drivers that needs this, what >>> is so different with this one? >>> >> There are some drivers that are checking pm_runtime_suspended() or >> pm_runtime_status_suspended() in their suspend/resume callbacks and doing >> things only when they return false so the problem is not so unique here. >> >> I didn't try to find are there drivers that are using some own state >> variable for the same purpose but I wouldn't surprise if there are. >> >>> I have been following the development for the i2c-designware driver >>> for a while. Some time back I also tried to fix the similar problems >>> as you are currently [1] are. Back then, I picked the same approach as >>> John Stultz did recently [2]. >>> >>> To summarize my view, I don't understand the justification of using >>> the direct_complete feature for i2c-designware. To me, it just add >>> complexity to the driver that we really should try to avoid. I think >>> we need something else here. >>> >> Dates back to commit 8503ff166504 ("i2c: designware: Avoid unnecessary >> resuming during system suspend") but I agree it's worth to consider. >> >>> To me, the proper solution is to use the >>> pm_runtime_force_suspend|resume() helpers to deal with system >>> suspend/resume. However I understand that the behavior of the ACPI PM >>> domain currently prevents us from doing this. That said, perhaps we >>> should instead try to make the ACPI PM domain to better collaborate >>> with drivers using pm_runtime_force_suspend|resume()? I have been >>> investigating that and started to cook some patches, although I have >>> not yet been able to post something. If you think it could make sense, >>> I can pick it up. >>> >> That's a good idea. I didn't think about it at all. > > Okay, thanks! I will continue to look into this and try to submit > something within a reasonable time frame, keep you posted. > > Kind regards > Uffe Jarkko, Andy, I just wanted to mention that I haven't forgot about this, I am doing the final changes for the ACPI PM domain at this very moment, however I need a couple of more days more before I can post something. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16 June 2017 at 15:49, Ulf Hansson <ulf.hansson@linaro.org> wrote: > [...] > >>>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c >>>>> b/drivers/i2c/busses/i2c-designware-platdrv.c >>>>> index a597ba32de7e..42a9cd09aa64 100644 >>>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c >>>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c >>>>> @@ -377,6 +377,9 @@ 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_suspended(dev)) >>>>> + return 0; >>>> >>>> >>>> This looks weird. I don't find any other drivers that needs this, what >>>> is so different with this one? >>>> >>> There are some drivers that are checking pm_runtime_suspended() or >>> pm_runtime_status_suspended() in their suspend/resume callbacks and doing >>> things only when they return false so the problem is not so unique here. >>> >>> I didn't try to find are there drivers that are using some own state >>> variable for the same purpose but I wouldn't surprise if there are. >>> >>>> I have been following the development for the i2c-designware driver >>>> for a while. Some time back I also tried to fix the similar problems >>>> as you are currently [1] are. Back then, I picked the same approach as >>>> John Stultz did recently [2]. >>>> >>>> To summarize my view, I don't understand the justification of using >>>> the direct_complete feature for i2c-designware. To me, it just add >>>> complexity to the driver that we really should try to avoid. I think >>>> we need something else here. >>>> >>> Dates back to commit 8503ff166504 ("i2c: designware: Avoid unnecessary >>> resuming during system suspend") but I agree it's worth to consider. >>> >>>> To me, the proper solution is to use the >>>> pm_runtime_force_suspend|resume() helpers to deal with system >>>> suspend/resume. However I understand that the behavior of the ACPI PM >>>> domain currently prevents us from doing this. That said, perhaps we >>>> should instead try to make the ACPI PM domain to better collaborate >>>> with drivers using pm_runtime_force_suspend|resume()? I have been >>>> investigating that and started to cook some patches, although I have >>>> not yet been able to post something. If you think it could make sense, >>>> I can pick it up. >>>> >>> That's a good idea. I didn't think about it at all. >> >> Okay, thanks! I will continue to look into this and try to submit >> something within a reasonable time frame, keep you posted. >> >> Kind regards >> Uffe > > Jarkko, Andy, > > I just wanted to mention that I haven't forgot about this, I am doing > the final changes for the ACPI PM domain at this very moment, however > I need a couple of more days more before I can post something. > I have now published a branch at: git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git acpi_pm_domain_wip It contains both changes for the i2c designware platform driver and for the ACPI PM domain. If you by coincidence happen to have slack time (probably not), then please try it out. The branch is based upon today's version of Rafael's pm tree linux-next branch. If everything goes well, I intend to post the patches after some more tests. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-06-20 at 15:07 +0200, Ulf Hansson wrote: > On 16 June 2017 at 15:49, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > Jarkko, Andy, > > > > I just wanted to mention that I haven't forgot about this, I am > > doing > > the final changes for the ACPI PM domain at this very moment, > > however > > I need a couple of more days more before I can post something. > > > > I have now published a branch at: > > git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git > acpi_pm_domain_wip > > It contains both changes for the i2c designware platform driver and > for the ACPI PM domain. If you by coincidence happen to have slack > time (probably not), then please try it out. > > The branch is based upon today's version of Rafael's pm tree linux- > next branch. > > If everything goes well, I intend to post the patches after some more > tests. > I have gone through the series briefly. My concern is a quite nasty bug we have workaround for in acpi_lpss.c, i.e. auto power gating of DesignWare DMA on Intel Braswell (CherryTrail) platforms when it's enumerated via ACPI. Below is the sequence to test if it works and survives removal and system sleep (lpss-power.sh is the script which shows a power state of selected devices along with PMC Atom status registers and runtime PM): 0 lpss-power.sh 1 mount -t debugfs none /sys/kernel/debug 2 lpss-power.sh 3 modprobe i2c-designware-platform 4 lpss-power.sh 5 modprobe sdhci-acpi 6 lpss-power.sh 7 lsmod 8 modprobe dw-dmac 9 lpss-power.sh 10 modprobe -r dw-dmac 11 lpss-power.sh 12 modprobe dw-dmac 13 lpss-power.sh 14 rtcwake -m mem -s3 15 lpss-power.sh 16 modprobe -r dw-dmac 17 lpss-power.sh 18 modprobe dw-dmac 19 lpss-power.sh 20 stty -F /dev/ttyS2 921600 21 dmesg > /dev/ttyS2 22 cat /proc/interrupts 23 cat < /dev/ttyS1 24 lpss-power.sh 25 cat < /dev/ttyS1 & 26 lpss-power.sh 27 rtcwake -m mem -s3 28 lpss-power.sh 29 fg 30 lpss-power.sh Be aware that DMA is not enabled for I2C!
On 20 June 2017 at 18:08, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, 2017-06-20 at 15:07 +0200, Ulf Hansson wrote: >> On 16 June 2017 at 15:49, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> > Jarkko, Andy, >> > >> > I just wanted to mention that I haven't forgot about this, I am >> > doing >> > the final changes for the ACPI PM domain at this very moment, >> > however >> > I need a couple of more days more before I can post something. >> > >> >> I have now published a branch at: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git >> acpi_pm_domain_wip >> >> It contains both changes for the i2c designware platform driver and >> for the ACPI PM domain. If you by coincidence happen to have slack >> time (probably not), then please try it out. >> >> The branch is based upon today's version of Rafael's pm tree linux- >> next branch. >> >> If everything goes well, I intend to post the patches after some more >> tests. >> > > I have gone through the series briefly. > My concern is a quite nasty bug we have workaround for in acpi_lpss.c, > i.e. auto power gating of DesignWare DMA on Intel Braswell (CherryTrail) > platforms when it's enumerated via ACPI. First, as long as there is no driver calling the new API acpi_dev_disable_direct_comlete(), the ACPI PM domain should behave exactly the same as before these changes. However, regarding your concern, can you please be a bit more precise on how you deal with the problems. I would appreciate if you could give me real pointers to the code for the workaround, the above is too hand wavy for me to understand. So where things really starts to change is in the final i2c patch in the series, which converts the i2c designware platform driver to use the runtime PM centric approach, and to do that, it calls the acpi_dev_disable_direct_complete(). > > Below is the sequence to test if it works and survives removal and > system sleep (lpss-power.sh is the script which shows a power state of > selected devices along with PMC Atom status registers and runtime PM): > > 0 lpss-power.sh > 1 mount -t debugfs none /sys/kernel/debug > 2 lpss-power.sh > 3 modprobe i2c-designware-platform > 4 lpss-power.sh > 5 modprobe sdhci-acpi > 6 lpss-power.sh > 7 lsmod > 8 modprobe dw-dmac > 9 lpss-power.sh > 10 modprobe -r dw-dmac > 11 lpss-power.sh > 12 modprobe dw-dmac > 13 lpss-power.sh > 14 rtcwake -m mem -s3 > 15 lpss-power.sh > 16 modprobe -r dw-dmac > 17 lpss-power.sh > 18 modprobe dw-dmac > 19 lpss-power.sh > 20 stty -F /dev/ttyS2 921600 > 21 dmesg > /dev/ttyS2 > 22 cat /proc/interrupts > 23 cat < /dev/ttyS1 > 24 lpss-power.sh > 25 cat < /dev/ttyS1 & > 26 lpss-power.sh > 27 rtcwake -m mem -s3 > 28 lpss-power.sh > 29 fg > 30 lpss-power.sh > > Be aware that DMA is not enabled for I2C! So if DMA isn't enabled for I2C, what is there to worry about? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
+Cc: Hans. On Wed, 2017-06-21 at 16:40 +0200, Ulf Hansson wrote: > On 20 June 2017 at 18:08, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, 2017-06-20 at 15:07 +0200, Ulf Hansson wrote: > > > On 16 June 2017 at 15:49, Ulf Hansson <ulf.hansson@linaro.org> > > > wrote: > > > > Jarkko, Andy, > > > > > > > > I just wanted to mention that I haven't forgot about this, I am > > > > doing > > > > the final changes for the ACPI PM domain at this very moment, > > > > however > > > > I need a couple of more days more before I can post something. > > > > > > I have gone through the series briefly. > > My concern is a quite nasty bug we have workaround for in > > acpi_lpss.c, > > i.e. auto power gating of DesignWare DMA on Intel Braswell > > (CherryTrail) > > platforms when it's enumerated via ACPI. > > First, as long as there is no driver calling the new API > acpi_dev_disable_direct_comlete(), the ACPI PM domain should behave > exactly the same as before these changes. > > However, regarding your concern, can you please be a bit more precise > on how you deal with the problems. I would appreciate if you could > give me real pointers to the code for the workaround, the above is too > hand wavy for me to understand. In acpi_lpss.c there are big comments about this issue. Hans, Cc'ed, is working on that in relation to PWM power problems. > > So where things really starts to change is in the final i2c patch in > the series, which converts the i2c designware platform driver to use > the runtime PM centric approach, and to do that, it calls the > acpi_dev_disable_direct_complete(). Does it mean we will loose a possibility to use DMA for I2C (not much I care about and would be unlikely a user for this, just wondering)? > > > > Below is the sequence to test if it works and survives removal and > > system sleep (lpss-power.sh is the script which shows a power state > > of > > selected devices along with PMC Atom status registers and runtime > > PM): > > > > 0 lpss-power.sh > > 1 mount -t debugfs none /sys/kernel/debug > > 2 lpss-power.sh > > 3 modprobe i2c-designware-platform > > 4 lpss-power.sh > > 5 modprobe sdhci-acpi > > 6 lpss-power.sh > > 7 lsmod > > 8 modprobe dw-dmac > > 9 lpss-power.sh > > 10 modprobe -r dw-dmac > > 11 lpss-power.sh > > 12 modprobe dw-dmac > > 13 lpss-power.sh > > 14 rtcwake -m mem -s3 > > 15 lpss-power.sh > > 16 modprobe -r dw-dmac > > 17 lpss-power.sh > > 18 modprobe dw-dmac > > 19 lpss-power.sh > > 20 stty -F /dev/ttyS2 921600 > > 21 dmesg > /dev/ttyS2 > > 22 cat /proc/interrupts > > 23 cat < /dev/ttyS1 > > 24 lpss-power.sh > > 25 cat < /dev/ttyS1 & > > 26 lpss-power.sh > > 27 rtcwake -m mem -s3 > > 28 lpss-power.sh > > 29 fg > > 30 lpss-power.sh > > > > Be aware that DMA is not enabled for I2C! > > So if DMA isn't enabled for I2C, what is there to worry about? Since LPSS are all in the same ACPI PM, and I would really carefully change behaviour for any of LPSS component driver without wide testing.
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index a597ba32de7e..42a9cd09aa64 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -377,6 +377,9 @@ 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_suspended(dev)) + return 0; + i2c_dw_disable(i_dev); i2c_dw_plat_prepare_clk(i_dev, false); @@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev) struct platform_device *pdev = to_platform_device(dev); struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); + if (pm_runtime_suspended(dev)) + return 0; + i2c_dw_plat_prepare_clk(i_dev, true); i2c_dw_init(i_dev);
There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated. Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks. However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks. Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming. Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> --- I'm able to trigger system suspend callback while device is runtime suspended by removing the pm_runtime_resume() call from drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C slave (ACPI enumerated but doesn't bind due an error in probe function). In that case __device_suspend() for that unbound device has NULL suspend callback, and thus doesn't cause any runtime resume chain but still unsets the parent's direct_complete flag. John Stult <john.stultz@linaro.org> has reported he can trigger this on HiKey board too. I'm not sure is this the right thing to do. It feels something the PM core should do but I'm not sure that either. One alternative could be to resume runtime suspended parent in in __device_suspend() right after where parent's direct_complete flag is unset. --- drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++ 1 file changed, 6 insertions(+)