[PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Jisheng Zhang]

Commit 1fc2fe204cb9 ("i2c: designware: Add runtime PM hooks") adds
runtime pm support using the same ops for system sleep and runtime pm.
When suspend to ram, the i2c host may have been runtime suspended, thus
i2c_dw_disable() hangs.

This patch fixes this issue by separating ops for system sleep pm and
runtime pm, and in the system suspend/resume path, runtime pm apis are
used to ensure the device is at correct state.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 48 ++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0a80e4a..d306397 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -298,14 +298,16 @@ static const struct of_device_id dw_i2c_of_match[] = {
 MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #endif
 
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
 static int dw_i2c_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	pm_runtime_get_sync(dev);
 	i2c_dw_disable(i_dev);
-	clk_disable_unprepare(i_dev->clk);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 
 	return 0;
 }
@@ -315,6 +317,32 @@ static int dw_i2c_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	pm_runtime_get_sync(dev);
+	i2c_dw_init(i_dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+#endif
+
+#ifdef CONFIG_PM
+static int dw_i2c_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+
+	i2c_dw_disable(i_dev);
+	clk_disable_unprepare(i_dev->clk);
+
+	return 0;
+}
+
+static int dw_i2c_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+
 	clk_prepare_enable(i_dev->clk);
 
 	if (!i_dev->pm_runtime_disabled)
@@ -324,8 +352,18 @@ static int dw_i2c_resume(struct device *dev)
 }
 #endif
 
-static UNIVERSAL_DEV_PM_OPS(dw_i2c_dev_pm_ops, dw_i2c_suspend,
-			    dw_i2c_resume, NULL);
+#ifdef CONFIG_PM
+static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_suspend, dw_i2c_resume)
+	SET_RUNTIME_PM_OPS(dw_i2c_runtime_suspend,
+		dw_i2c_runtime_resume, NULL)
+};
+
+#define DW_I2C_DEV_PM_OPS (&dw_i2c_dev_pm_ops)
+
+#else
+#define DW_I2C_DEV_PM_OPS NULL
+#endif
 
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:i2c_designware");
@@ -337,7 +375,7 @@ static struct platform_driver dw_i2c_driver = {
 		.name	= "i2c_designware",
 		.of_match_table = of_match_ptr(dw_i2c_of_match),
 		.acpi_match_table = ACPI_PTR(dw_i2c_acpi_match),
-		.pm	= &dw_i2c_dev_pm_ops,
+		.pm	= DW_I2C_DEV_PM_OPS,
 	},
 };
 
-- 
2.1.4

Re: [PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Mika Westerberg]

On Fri, May 15, 2015 at 08:31:39PM +0800, Jisheng Zhang wrote:
> Commit 1fc2fe204cb9 ("i2c: designware: Add runtime PM hooks") adds
> runtime pm support using the same ops for system sleep and runtime pm.
> When suspend to ram, the i2c host may have been runtime suspended, thus
> i2c_dw_disable() hangs.

It hangs because it has already been powered off, right?

> This patch fixes this issue by separating ops for system sleep pm and
> runtime pm, and in the system suspend/resume path, runtime pm apis are
> used to ensure the device is at correct state.

I can see that this fixes the issue with the platform driver (as the
platform bus core doesn't power on the device automatically as opposed
to other buses, like PCI). However, I'm thinking that can we do better
here.

Instead of powering the device on again, can't we leave it in low power
state? Recently added 'dev->power.direct_complete' may be used to
achieve that, I think.

Re: [PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Jisheng Zhang]

Dear Mika,

On Mon, 18 May 2015 11:28:23 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Fri, May 15, 2015 at 08:31:39PM +0800, Jisheng Zhang wrote:
> > Commit 1fc2fe204cb9 ("i2c: designware: Add runtime PM hooks") adds
> > runtime pm support using the same ops for system sleep and runtime pm.
> > When suspend to ram, the i2c host may have been runtime suspended, thus
> > i2c_dw_disable() hangs.
> 
> It hangs because it has already been powered off, right?

Either be powered off or clock gated or even both.

> 
> > This patch fixes this issue by separating ops for system sleep pm and
> > runtime pm, and in the system suspend/resume path, runtime pm apis are
> > used to ensure the device is at correct state.
> 
> I can see that this fixes the issue with the platform driver (as the
> platform bus core doesn't power on the device automatically as opposed
> to other buses, like PCI). However, I'm thinking that can we do better
> here.
> 
> Instead of powering the device on again, can't we leave it in low power
> state? Recently added 'dev->power.direct_complete' may be used to
> achieve that, I think.

how to handle runtime suspended via just being clock gated? Currently the
only solution is using the runtime pm apis to ensure the device is in a working
state during s2ram. What's your opinion?

Thanks,
Jisheng

Re: [PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Mika Westerberg]

On Tue, May 19, 2015 at 08:32:42PM +0800, Jisheng Zhang wrote:
> > I can see that this fixes the issue with the platform driver (as the
> > platform bus core doesn't power on the device automatically as opposed
> > to other buses, like PCI). However, I'm thinking that can we do better
> > here.
> > 
> > Instead of powering the device on again, can't we leave it in low power
> > state? Recently added 'dev->power.direct_complete' may be used to
> > achieve that, I think.
> 
> how to handle runtime suspended via just being clock gated?

As far as I can tell driver's suspend hook does the clock gating so why
would you need to handle it differently? Once the device is runtime
suspended, it is both clock and power gated depending on the platform.

> Currently the only solution is using the runtime pm apis to ensure the
> device is in a working state during s2ram. What's your opinion?

Well, it sounds a bit silly to resume the device just because you want
to call i2c_dw_disable() for it before suspending again ;-)

At least, it would be nice to check if we can keep it runtime suspended
with the help of 'dev->power.direct_complete'. If not possible, then I
suppose your patch is sufficient.

Re: [PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Jisheng Zhang]

Dear Mika,

On Tue, 19 May 2015 16:15:16 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Tue, May 19, 2015 at 08:32:42PM +0800, Jisheng Zhang wrote:
> > > I can see that this fixes the issue with the platform driver (as the
> > > platform bus core doesn't power on the device automatically as opposed
> > > to other buses, like PCI). However, I'm thinking that can we do better
> > > here.
> > > 
> > > Instead of powering the device on again, can't we leave it in low power
> > > state? Recently added 'dev->power.direct_complete' may be used to
> > > achieve that, I think.
> > 
> > how to handle runtime suspended via just being clock gated?
> 
> As far as I can tell driver's suspend hook does the clock gating so why
> would you need to handle it differently? Once the device is runtime
> suspended, it is both clock and power gated depending on the platform.

Sorry for confusion. Considering one platform which doesn't support power off
the i2c host but it can disable the host's clock. So in such platform, when
the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will
hang when s2ram. Except using the runtime pm API to ensure the host is in
a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete'
doesn't help such case.

> 
> > Currently the only solution is using the runtime pm apis to ensure the
> > device is in a working state during s2ram. What's your opinion?
> 
> Well, it sounds a bit silly to resume the device just because you want
> to call i2c_dw_disable() for it before suspending again ;-)
> 

Agree. But it's the only solution I know so far.

> At least, it would be nice to check if we can keep it runtime suspended
> with the help of 'dev->power.direct_complete'. If not possible, then I
> suppose your patch is sufficient.

Thanks,
Jisheng

Re: [PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Jisheng Zhang]

Dear Mika,

On Wed, 20 May 2015 19:34:22 +0800
Jisheng Zhang <jszhang@marvell.com> wrote:

> Dear Mika,
> 
> On Tue, 19 May 2015 16:15:16 +0300
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > On Tue, May 19, 2015 at 08:32:42PM +0800, Jisheng Zhang wrote:
> > > > I can see that this fixes the issue with the platform driver (as the
> > > > platform bus core doesn't power on the device automatically as opposed
> > > > to other buses, like PCI). However, I'm thinking that can we do better
> > > > here.
> > > > 
> > > > Instead of powering the device on again, can't we leave it in low power
> > > > state? Recently added 'dev->power.direct_complete' may be used to
> > > > achieve that, I think.
> > > 
> > > how to handle runtime suspended via just being clock gated?
> > 
> > As far as I can tell driver's suspend hook does the clock gating so why
> > would you need to handle it differently? Once the device is runtime
> > suspended, it is both clock and power gated depending on the platform.
> 
> Sorry for confusion. Considering one platform which doesn't support power off
> the i2c host but it can disable the host's clock. So in such platform, when
> the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will
> hang when s2ram. Except using the runtime pm API to ensure the host is in
> a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete'
> doesn't help such case.

I misunderstood the direct_complete flag usage. I think it can help such case
will investigate and provide new patch if necessary.

Thanks a lot

Re: [PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Mika Westerberg]

On Wed, May 20, 2015 at 07:34:22PM +0800, Jisheng Zhang wrote:
> Sorry for confusion. Considering one platform which doesn't support power off
> the i2c host but it can disable the host's clock. So in such platform, when
> the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will
> hang when s2ram.

Right. This happens also when the platform powers off the device.

> Except using the runtime pm API to ensure the host is in
> a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete'
> doesn't help such case.

What I had in mind is something like below:

static int i2c_dw_prepare(struct device *dev)
{
	return pm_runtime_suspended(dev);
}

static void i2c_dw_complete(struct device *dev)
{
	if (dev->power.direct_complete)
		pm_request_resume(dev);
}

In other words it checks if the device is already runtime suspended and
prevents ->suspend() etc. from being called.

If that does not work (I didn't try as this problem does not exist on
ACPI platforms because we have PM domain handling things) then we do not
have much of a choice than do what you suggest and runtime resume the
device on ->suspend()

Re: [PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Jisheng Zhang]

Dear Mika,

On Wed, 20 May 2015 15:15:06 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Wed, May 20, 2015 at 07:34:22PM +0800, Jisheng Zhang wrote:
> > Sorry for confusion. Considering one platform which doesn't support power off
> > the i2c host but it can disable the host's clock. So in such platform, when
> > the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will
> > hang when s2ram.
> 
> Right. This happens also when the platform powers off the device.
> 
> > Except using the runtime pm API to ensure the host is in
> > a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete'
> > doesn't help such case.
> 
> What I had in mind is something like below:
> 
> static int i2c_dw_prepare(struct device *dev)
> {
> 	return pm_runtime_suspended(dev);
> }
> 
> static void i2c_dw_complete(struct device *dev)
> {
> 	if (dev->power.direct_complete)
> 		pm_request_resume(dev);
> }
> 
> In other words it checks if the device is already runtime suspended and
> prevents ->suspend() etc. from being called.

What amazing! I wrote the same code as yours after sending out the last email.

> 
> If that does not work (I didn't try as this problem does not exist on

It works! How to submit the patch? Do you mind if I cook the patch and add
you signed-off?

Thanks a lot,
Jisheng

Re: [PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Jisheng Zhang]

On Wed, 20 May 2015 20:34:30 +0800
Jisheng Zhang <jszhang@marvell.com> wrote:

> Dear Mika,
> 
> On Wed, 20 May 2015 15:15:06 +0300
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > On Wed, May 20, 2015 at 07:34:22PM +0800, Jisheng Zhang wrote:
> > > Sorry for confusion. Considering one platform which doesn't support power off
> > > the i2c host but it can disable the host's clock. So in such platform, when
> > > the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will
> > > hang when s2ram.
> > 
> > Right. This happens also when the platform powers off the device.
> > 
> > > Except using the runtime pm API to ensure the host is in
> > > a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete'
> > > doesn't help such case.
> > 
> > What I had in mind is something like below:
> > 
> > static int i2c_dw_prepare(struct device *dev)
> > {
> > 	return pm_runtime_suspended(dev);
> > }
> > 
> > static void i2c_dw_complete(struct device *dev)
> > {
> > 	if (dev->power.direct_complete)
> > 		pm_request_resume(dev);
> > }
> > 
> > In other words it checks if the device is already runtime suspended and
> > prevents ->suspend() etc. from being called.
> 
> What amazing! I wrote the same code as yours after sending out the last email.
> 
> > 
> > If that does not work (I didn't try as this problem does not exist on
> 
> It works! How to submit the patch? Do you mind if I cook the patch and add
> you signed-off?
> 

PS: If you cook the patch instead,  feel free to add my acked-by and tested-by

Thanks a lot for the direct_complete idea,
Jisheng

Re: [PATCH] i2c: designware: separate ops for system_sleep_pm and runtime_pm

[Mika Westerberg]

On Wed, May 20, 2015 at 08:34:30PM +0800, Jisheng Zhang wrote:
> Dear Mika,
> 
> On Wed, 20 May 2015 15:15:06 +0300
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > On Wed, May 20, 2015 at 07:34:22PM +0800, Jisheng Zhang wrote:
> > > Sorry for confusion. Considering one platform which doesn't support power off
> > > the i2c host but it can disable the host's clock. So in such platform, when
> > > the host is runtime suspended, its clock is disabled, then i2c_dw_disable() will
> > > hang when s2ram.
> > 
> > Right. This happens also when the platform powers off the device.
> > 
> > > Except using the runtime pm API to ensure the host is in
> > > a correct state, is there any other solution? AFAIK, 'dev->power.direct_complete'
> > > doesn't help such case.
> > 
> > What I had in mind is something like below:
> > 
> > static int i2c_dw_prepare(struct device *dev)
> > {
> > 	return pm_runtime_suspended(dev);
> > }
> > 
> > static void i2c_dw_complete(struct device *dev)
> > {
> > 	if (dev->power.direct_complete)
> > 		pm_request_resume(dev);
> > }
> > 
> > In other words it checks if the device is already runtime suspended and
> > prevents ->suspend() etc. from being called.
> 
> What amazing! I wrote the same code as yours after sending out the last email.

Cool :)

> > 
> > If that does not work (I didn't try as this problem does not exist on
> 
> It works! How to submit the patch? Do you mind if I cook the patch and add
> you signed-off?

That's fine and you don't need to add my SoB there. I'll test the patch
here on some our platforms to ensure it does not break anything and if
it turns out working you can have my Tested-by then.