* Re: [PATCH] i2c: designware: Fix system suspend
2017-08-09 13:28 [PATCH] i2c: designware: Fix system suspend Ulf Hansson
@ 2017-08-09 20:43 ` Rafael J. Wysocki
2017-08-14 19:04 ` Wolfram Sang
2017-08-09 20:55 ` John Stultz
2017-08-14 19:07 ` Wolfram Sang
2 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2017-08-09 20:43 UTC (permalink / raw)
To: Ulf Hansson
Cc: Wolfram Sang, linux-i2c, Jarkko Nikula, Andy Shevchenko,
Mika Westerberg, Jisheng Zhang, John Stultz, Guodong Xu,
Sumit Semwal, Haojian Zhuang, linux-arm-kernel, linux-pm,
# v4 . 4+
On Wednesday, August 9, 2017 3:28:22 PM CEST Ulf Hansson wrote:
> The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
> during system suspend"), may suggest to the PM core to try out the so
> called direct_complete path for system sleep. In this path, the PM core
> treats a runtime suspended device as it's already in a proper low power
> state for system sleep, which makes it skip calling the system sleep
> callbacks for the device, except for the ->prepare() and the ->complete()
> callbacks.
>
> However, the PM core may unset the direct_complete flag for a parent
> device, in case its child device are being system suspended before. In this
> scenario, the PM core invokes the system sleep callbacks, no matter if the
> device is runtime suspended or not.
>
> Particularly in cases of an existing i2c slave device, the above path is
> triggered, which breaks the assumption that the i2c device is always
> runtime resumed whenever the dw_i2c_plat_suspend() is being called.
>
> More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> a splat in the log about clocks calls being wrongly balanced and breaking
> system sleep.
>
> To still allow the direct_complete path in cases when it's possible, but
> also to keep the fix simple, let's runtime resume the i2c device in the
> ->suspend() callback, before continuing to put the device into low power
> state.
>
> Note, in cases when the i2c device is attached to the ACPI PM domain, this
> problem doesn't occur, because ACPI's ->suspend() callback, assigned to
> acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.
>
> It should also be noted that this change does not fix commit 8503ff166504
> ("i2c: designware: Avoid unnecessary resuming during system suspend").
> Because for the non-ACPI case, the system sleep support was already broken
> prior that point.
>
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Looks OK to me.
> ---
>
> I decided to post this as a separate change, instead of as earlier folding it
> in the series that extends the ACPI PM domain to cope with the runtime PM
> centric path for system sleep.
>
> This change applies on a fresh v4.4+. If someone wants it's applied for earlier
> version, please send a backport yourself.
>
> It's based on 4.13 rc4 and I assume it should go as a fix via the i2c tree.
>
> Changes in v2:
> - Updated changelog.
> - Runtime resume the device in ->suspend() instead of in ->prepare().
>
> ---
> drivers/i2c/busses/i2c-designware-platdrv.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 2ea6d0d..bb3b8c8 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -426,7 +426,7 @@ static void dw_i2c_plat_complete(struct device *dev)
> #endif
>
> #ifdef CONFIG_PM
> -static int dw_i2c_plat_suspend(struct device *dev)
> +static int dw_i2c_plat_runtime_suspend(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
> @@ -448,11 +448,21 @@ static int dw_i2c_plat_resume(struct device *dev)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int dw_i2c_plat_suspend(struct device *dev)
> +{
> + pm_runtime_resume(dev);
> + return dw_i2c_plat_runtime_suspend(dev);
> +}
> +#endif
> +
> static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
> .prepare = dw_i2c_plat_prepare,
> .complete = dw_i2c_plat_complete,
> SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> - SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
> + SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
> + dw_i2c_plat_resume,
> + NULL)
> };
>
> #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] i2c: designware: Fix system suspend
2017-08-09 20:43 ` Rafael J. Wysocki
@ 2017-08-14 19:04 ` Wolfram Sang
0 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2017-08-14 19:04 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, linux-i2c, Jarkko Nikula, Andy Shevchenko,
Mika Westerberg, Jisheng Zhang, John Stultz, Guodong Xu,
Sumit Semwal, Haojian Zhuang, linux-arm-kernel, linux-pm,
# v4 . 4+
[-- Attachment #1: Type: text/plain, Size: 2393 bytes --]
On Wed, Aug 09, 2017 at 10:43:22PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, August 9, 2017 3:28:22 PM CEST Ulf Hansson wrote:
> > The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
> > during system suspend"), may suggest to the PM core to try out the so
> > called direct_complete path for system sleep. In this path, the PM core
> > treats a runtime suspended device as it's already in a proper low power
> > state for system sleep, which makes it skip calling the system sleep
> > callbacks for the device, except for the ->prepare() and the ->complete()
> > callbacks.
> >
> > However, the PM core may unset the direct_complete flag for a parent
> > device, in case its child device are being system suspended before. In this
> > scenario, the PM core invokes the system sleep callbacks, no matter if the
> > device is runtime suspended or not.
> >
> > Particularly in cases of an existing i2c slave device, the above path is
> > triggered, which breaks the assumption that the i2c device is always
> > runtime resumed whenever the dw_i2c_plat_suspend() is being called.
> >
> > More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
> > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> > a splat in the log about clocks calls being wrongly balanced and breaking
> > system sleep.
> >
> > To still allow the direct_complete path in cases when it's possible, but
> > also to keep the fix simple, let's runtime resume the i2c device in the
> > ->suspend() callback, before continuing to put the device into low power
> > state.
> >
> > Note, in cases when the i2c device is attached to the ACPI PM domain, this
> > problem doesn't occur, because ACPI's ->suspend() callback, assigned to
> > acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.
> >
> > It should also be noted that this change does not fix commit 8503ff166504
> > ("i2c: designware: Avoid unnecessary resuming during system suspend").
> > Because for the non-ACPI case, the system sleep support was already broken
> > prior that point.
> >
> > Cc: <stable@vger.kernel.org> # v4.4+
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Looks OK to me.
I read this as Acked-by. If so, using the real "Acked-by" tag would be
helpful because patchwork collects them automatically.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: designware: Fix system suspend
2017-08-09 13:28 [PATCH] i2c: designware: Fix system suspend Ulf Hansson
2017-08-09 20:43 ` Rafael J. Wysocki
@ 2017-08-09 20:55 ` John Stultz
2017-08-10 10:25 ` Jarkko Nikula
2017-08-14 19:07 ` Wolfram Sang
2 siblings, 1 reply; 7+ messages in thread
From: John Stultz @ 2017-08-09 20:55 UTC (permalink / raw)
To: Ulf Hansson
Cc: Wolfram Sang, Rafael J . Wysocki, linux-i2c, Jarkko Nikula,
Andy Shevchenko, Mika Westerberg, Jisheng Zhang, Guodong Xu,
Sumit Semwal, Haojian Zhuang,
linux-arm-kernel@lists.infradead.org, Linux PM list, # v4 . 4+
On Wed, Aug 9, 2017 at 6:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
> during system suspend"), may suggest to the PM core to try out the so
> called direct_complete path for system sleep. In this path, the PM core
> treats a runtime suspended device as it's already in a proper low power
> state for system sleep, which makes it skip calling the system sleep
> callbacks for the device, except for the ->prepare() and the ->complete()
> callbacks.
>
> However, the PM core may unset the direct_complete flag for a parent
> device, in case its child device are being system suspended before. In this
> scenario, the PM core invokes the system sleep callbacks, no matter if the
> device is runtime suspended or not.
>
> Particularly in cases of an existing i2c slave device, the above path is
> triggered, which breaks the assumption that the i2c device is always
> runtime resumed whenever the dw_i2c_plat_suspend() is being called.
>
> More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> a splat in the log about clocks calls being wrongly balanced and breaking
> system sleep.
>
> To still allow the direct_complete path in cases when it's possible, but
> also to keep the fix simple, let's runtime resume the i2c device in the
> ->suspend() callback, before continuing to put the device into low power
> state.
>
> Note, in cases when the i2c device is attached to the ACPI PM domain, this
> problem doesn't occur, because ACPI's ->suspend() callback, assigned to
> acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.
>
> It should also be noted that this change does not fix commit 8503ff166504
> ("i2c: designware: Avoid unnecessary resuming during system suspend").
> Because for the non-ACPI case, the system sleep support was already broken
> prior that point.
>
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> I decided to post this as a separate change, instead of as earlier folding it
> in the series that extends the ACPI PM domain to cope with the runtime PM
> centric path for system sleep.
>
> This change applies on a fresh v4.4+. If someone wants it's applied for earlier
> version, please send a backport yourself.
>
> It's based on 4.13 rc4 and I assume it should go as a fix via the i2c tree.
>
> Changes in v2:
> - Updated changelog.
> - Runtime resume the device in ->suspend() instead of in ->prepare().
This avoids the suspend/resume warning I've seen w/o the patch on HiKey.
Tested-by: John Stultz <john.stultz@linaro.org>
thanks
-john
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] i2c: designware: Fix system suspend
2017-08-09 20:55 ` John Stultz
@ 2017-08-10 10:25 ` Jarkko Nikula
2017-08-10 10:31 ` Mika Westerberg
0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Nikula @ 2017-08-10 10:25 UTC (permalink / raw)
To: John Stultz, Ulf Hansson
Cc: Wolfram Sang, Rafael J . Wysocki, linux-i2c, Andy Shevchenko,
Mika Westerberg, Jisheng Zhang, Guodong Xu, Sumit Semwal,
Haojian Zhuang, linux-arm-kernel@lists.infradead.org,
Linux PM list, # v4 . 4+
On 08/09/2017 11:55 PM, John Stultz wrote:
> On Wed, Aug 9, 2017 at 6:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
>> during system suspend"), may suggest to the PM core to try out the so
>> called direct_complete path for system sleep. In this path, the PM core
>> treats a runtime suspended device as it's already in a proper low power
>> state for system sleep, which makes it skip calling the system sleep
>> callbacks for the device, except for the ->prepare() and the ->complete()
>> callbacks.
>>
>> However, the PM core may unset the direct_complete flag for a parent
>> device, in case its child device are being system suspended before. In this
>> scenario, the PM core invokes the system sleep callbacks, no matter if the
>> device is runtime suspended or not.
>>
>> Particularly in cases of an existing i2c slave device, the above path is
>> triggered, which breaks the assumption that the i2c device is always
>> runtime resumed whenever the dw_i2c_plat_suspend() is being called.
>>
>> More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>> a splat in the log about clocks calls being wrongly balanced and breaking
>> system sleep.
>>
>> To still allow the direct_complete path in cases when it's possible, but
>> also to keep the fix simple, let's runtime resume the i2c device in the
>> ->suspend() callback, before continuing to put the device into low power
>> state.
>>
>> Note, in cases when the i2c device is attached to the ACPI PM domain, this
>> problem doesn't occur, because ACPI's ->suspend() callback, assigned to
>> acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.
>>
>> It should also be noted that this change does not fix commit 8503ff166504
>> ("i2c: designware: Avoid unnecessary resuming during system suspend").
>> Because for the non-ACPI case, the system sleep support was already broken
>> prior that point.
>>
>> Cc: <stable@vger.kernel.org> # v4.4+
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> I decided to post this as a separate change, instead of as earlier folding it
>> in the series that extends the ACPI PM domain to cope with the runtime PM
>> centric path for system sleep.
>>
>> This change applies on a fresh v4.4+. If someone wants it's applied for earlier
>> version, please send a backport yourself.
>>
>> It's based on 4.13 rc4 and I assume it should go as a fix via the i2c tree.
>>
>> Changes in v2:
>> - Updated changelog.
>> - Runtime resume the device in ->suspend() instead of in ->prepare().
>
>
> This avoids the suspend/resume warning I've seen w/o the patch on HiKey.
>
> Tested-by: John Stultz <john.stultz@linaro.org>
>
Cool, no issues from acpi_lpss.c nor mfd/intel-lpss.c platforms.
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] i2c: designware: Fix system suspend
2017-08-10 10:25 ` Jarkko Nikula
@ 2017-08-10 10:31 ` Mika Westerberg
0 siblings, 0 replies; 7+ messages in thread
From: Mika Westerberg @ 2017-08-10 10:31 UTC (permalink / raw)
To: Jarkko Nikula
Cc: John Stultz, Ulf Hansson, Wolfram Sang, Rafael J . Wysocki,
linux-i2c, Andy Shevchenko, Jisheng Zhang, Guodong Xu,
Sumit Semwal, Haojian Zhuang,
linux-arm-kernel@lists.infradead.org, Linux PM list, # v4 . 4+
On Thu, Aug 10, 2017 at 01:25:33PM +0300, Jarkko Nikula wrote:
> Cool, no issues from acpi_lpss.c nor mfd/intel-lpss.c platforms.
>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Looks good to me as well,
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: designware: Fix system suspend
2017-08-09 13:28 [PATCH] i2c: designware: Fix system suspend Ulf Hansson
2017-08-09 20:43 ` Rafael J. Wysocki
2017-08-09 20:55 ` John Stultz
@ 2017-08-14 19:07 ` Wolfram Sang
2 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2017-08-14 19:07 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J . Wysocki, linux-i2c, Jarkko Nikula, Andy Shevchenko,
Mika Westerberg, Jisheng Zhang, John Stultz, Guodong Xu,
Sumit Semwal, Haojian Zhuang, linux-arm-kernel, linux-pm,
# v4 . 4+
[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]
On Wed, Aug 09, 2017 at 03:28:22PM +0200, Ulf Hansson wrote:
> The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
> during system suspend"), may suggest to the PM core to try out the so
> called direct_complete path for system sleep. In this path, the PM core
> treats a runtime suspended device as it's already in a proper low power
> state for system sleep, which makes it skip calling the system sleep
> callbacks for the device, except for the ->prepare() and the ->complete()
> callbacks.
>
> However, the PM core may unset the direct_complete flag for a parent
> device, in case its child device are being system suspended before. In this
> scenario, the PM core invokes the system sleep callbacks, no matter if the
> device is runtime suspended or not.
>
> Particularly in cases of an existing i2c slave device, the above path is
> triggered, which breaks the assumption that the i2c device is always
> runtime resumed whenever the dw_i2c_plat_suspend() is being called.
>
> More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and
> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> a splat in the log about clocks calls being wrongly balanced and breaking
> system sleep.
>
> To still allow the direct_complete path in cases when it's possible, but
> also to keep the fix simple, let's runtime resume the i2c device in the
> ->suspend() callback, before continuing to put the device into low power
> state.
>
> Note, in cases when the i2c device is attached to the ACPI PM domain, this
> problem doesn't occur, because ACPI's ->suspend() callback, assigned to
> acpi_subsys_suspend(), already calls pm_runtime_resume() for the device.
>
> It should also be noted that this change does not fix commit 8503ff166504
> ("i2c: designware: Avoid unnecessary resuming during system suspend").
> Because for the non-ACPI case, the system sleep support was already broken
> prior that point.
>
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread