i2c: designware: Do nothing in system suspend/resume when RT suspended

Submitted by Jarkko Nikula on March 30, 2017, 12:04 p.m.

Details

Message ID 20170330120444.12499-1-jarkko.nikula@linux.intel.com
State Under Review
Headers show

Commit Message

Jarkko Nikula March 30, 2017, 12:04 p.m.
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(+)

Comments

John Stultz April 4, 2017, 2:55 a.m.
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
Wolfram Sang April 19, 2017, 6:59 p.m.
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
Rafael J. Wysocki April 19, 2017, 8:24 p.m.
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
Rafael J. Wysocki April 19, 2017, 8:24 p.m.
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
Jarkko Nikula April 20, 2017, 7:25 a.m.
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);
Rafael J. Wysocki April 20, 2017, 10:33 a.m.
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
Jarkko Nikula April 24, 2017, 2:27 p.m.
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.
Ulf Hansson April 25, 2017, 9:24 a.m.
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
Jarkko Nikula April 25, 2017, 11:08 a.m.
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.
Ulf Hansson April 25, 2017, 11:12 a.m.
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
Andy Shevchenko April 25, 2017, 11:36 a.m.
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.
Ulf Hansson April 25, 2017, 12:04 p.m.
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
Ulf Hansson June 16, 2017, 1:49 p.m.
[...]

>>>> 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
Ulf Hansson June 20, 2017, 1:07 p.m.
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
Andy Shevchenko June 20, 2017, 4:08 p.m.
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!
Ulf Hansson June 21, 2017, 2:40 p.m.
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
Andy Shevchenko June 27, 2017, 2:34 p.m.
+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.

Patch hide | download patch | download mbox

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);