[PATCH 0/2] ARM: AMBA: Fix clock disable/enable issue in conventional suspend/resume

[PATCH 0/2] ARM: AMBA: Fix clock disable/enable issue in conventional suspend/resume

[Vipul Kumar Samar]

Vipul Kumar Samar (2):
  spi:pl022: Disable/Enable functional clock from suspend/resume
  ARM: ABMA: Disable/Enable interface clock from suspend/resume

 drivers/amba/bus.c      |   42 +++++++++++++++++++++++++++++++++++++++---
 drivers/spi/spi-pl022.c |    8 ++++++++
 2 files changed, 47 insertions(+), 3 deletions(-)

-- 
1.7.10

[PATCH 1/2] spi:pl022: Disable/Enable functional clock from suspend/resume

[Vipul Kumar Samar]

SPI functional clock must be disalble/enable in non RTPM suspend/resume
hooks. Currently it is only done for RTPM cases.

This patch add support to disable/enbale clock for conventional
suspend/resume calls.

Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com>
---
 drivers/spi/spi-pl022.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index f2a80ff..09fb09e 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2310,6 +2310,8 @@ static int pl022_suspend(struct device *dev)
 	}
 
 	dev_dbg(dev, "suspended\n");
+	clk_disable(pl022->clk);
+
 	return 0;
 }
 
@@ -2318,6 +2320,12 @@ static int pl022_resume(struct device *dev)
 	struct pl022 *pl022 = dev_get_drvdata(dev);
 	int ret;
 
+	ret = clk_enable(pl022->clk);
+	if (ret) {
+		dev_err(dev, "could not enable SSP/SPI bus clock\n");
+		return ret;
+	}
+
 	/* Start the queue running */
 	ret = spi_master_resume(pl022->master);
 	if (ret)
-- 
1.7.10

[PATCH 1/2] spi:pl022: Disable/Enable functional clock from suspend/resume

[Linus Walleij]

On Wed, Sep 26, 2012 at 1:24 PM, Vipul Kumar Samar
<vipulkumar.samar@st.com> wrote:

> SPI functional clock must be disalble/enable in non RTPM suspend/resume
> hooks. Currently it is only done for RTPM cases.
>
> This patch add support to disable/enbale clock for conventional
> suspend/resume calls.
>
> Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com>

Cross dependency between runtime suspend/resume and
common suspend/resume. Oh the horror ...

Ulf Hansson has experienced pain with this as well, let's
discuss this a bit.

> @@ -2310,6 +2310,8 @@ static int pl022_suspend(struct device *dev)
>         }
>
>         dev_dbg(dev, "suspended\n");
> +       clk_disable(pl022->clk);
> +
>         return 0;
>  }
>
> @@ -2318,6 +2320,12 @@ static int pl022_resume(struct device *dev)
>         struct pl022 *pl022 = dev_get_drvdata(dev);
>         int ret;
>
> +       ret = clk_enable(pl022->clk);
> +       if (ret) {
> +               dev_err(dev, "could not enable SSP/SPI bus clock\n");
> +               return ret;
> +       }
> +

There is a potential race between the runtime
suspend/resume and ordinary suspend/resume hooks here
I'm afraid.

I think in this case since we're not reading nor writing
registers, we should just wait for the device to
go down to runtime suspend in the ordinary suspend
hook, just wait for runtime suspend to happen in
suspend, do nothing in resume (and wait for the device
to wake itself as needed).

So something like:

while (!pm_runtime_status_suspended(&dev))
   cpu_relax();  // or usleep_range()?
/* Here you know the block is gated off */

Or is this better:

pm_runtime_get_sync();
/* Now we know for sure it's on! */
pm_runtime_put_sync();
/* Now we know for sure it's off! */

Is there a *good* way to await runtime suspend?

I don't know if any of this is the proper solution so let
Rafael and Magnus comment on how it's supposed
to be done.


Ramblings:

The semantics between runtime suspend/resume and
ordinary suspend/resume are unclear to me, it seems like
this is all up to the drivers and busses to figure out. Like
you weren't supposed to use both at the same time.

What we've done in other drivers here at ST-Ericsson is
to make the .suspend hook actually do a runtime get so that
runtime PM is "running", then hammer off all resources
and go to suspend with PM runtime actually enabled.

Something like this:

suspend()
  pm_runtime_get_sync()
  /* Maybe poke some registers here */
  clk_disable();

resume():
  clk_enable();
  /* Maybe poke some registers here */
  pm_runtime_put();

This is to be sure that there is not a race between runtime
suspend/resume and ordinary suspend/resume.

I don't like it since it actually turns things upside-down
completely, during ordinary suspend the device is
"runtime resumed" for example.

Rafael, Magnus: help.

Yours,
Linus Walleij

[PATCH 1/2] spi:pl022: Disable/Enable functional clock from suspend/resume

[Mark Brown]

On Wed, Sep 26, 2012 at 02:17:36PM +0200, Linus Walleij wrote:
> On Wed, Sep 26, 2012 at 1:24 PM, Vipul Kumar Samar

> > SPI functional clock must be disalble/enable in non RTPM suspend/resume
> > hooks. Currently it is only done for RTPM cases.

> > This patch add support to disable/enbale clock for conventional
> > suspend/resume calls.

> Cross dependency between runtime suspend/resume and
> common suspend/resume. Oh the horror ...

This should be fine, we runtime resume before we suspend.

> The semantics between runtime suspend/resume and
> ordinary suspend/resume are unclear to me, it seems like
> this is all up to the drivers and busses to figure out. Like
> you weren't supposed to use both at the same time.

This was clarified at some point relatively recently with the above
(which is essentially the same as the solution you describe).

[PATCH 1/2] spi:pl022: Disable/Enable functional clock from suspend/resume

[Linus Walleij]

On Wed, Sep 26, 2012 at 2:19 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Sep 26, 2012 at 02:17:36PM +0200, Linus Walleij wrote:
>> On Wed, Sep 26, 2012 at 1:24 PM, Vipul Kumar Samar
>
>> > SPI functional clock must be disalble/enable in non RTPM suspend/resume
>> > hooks. Currently it is only done for RTPM cases.
>
>> > This patch add support to disable/enbale clock for conventional
>> > suspend/resume calls.
>
>> Cross dependency between runtime suspend/resume and
>> common suspend/resume. Oh the horror ...
>
> This should be fine, we runtime resume before we suspend.
(...)
> This was clarified at some point relatively recently with the above
> (which is essentially the same as the solution you describe).

Oh. How come that whenever I poke my nose into this stuff I
feel like a compleat n00b X-D

Can you point me to the relevant posts/doc so I can read up on
it, would be much appreciated!

Yours,
Linus Walleij

[PATCH 1/2] spi:pl022: Disable/Enable functional clock from suspend/resume

[Mark Brown]

On Wed, Sep 26, 2012 at 02:41:56PM +0200, Linus Walleij wrote:

> Can you point me to the relevant posts/doc so I can read up on
> it, would be much appreciated!

I can't remember anything specific off the top of my head, sorry.

[PATCH 1/2] spi:pl022: Disable/Enable functional clock from suspend/resume

[viresh kumar]

On Wed, Sep 26, 2012 at 5:49 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Sep 26, 2012 at 02:17:36PM +0200, Linus Walleij wrote:
>> On Wed, Sep 26, 2012 at 1:24 PM, Vipul Kumar Samar
>
>> > SPI functional clock must be disalble/enable in non RTPM suspend/resume
>> > hooks. Currently it is only done for RTPM cases.
>
>> > This patch add support to disable/enbale clock for conventional
>> > suspend/resume calls.
>
>> Cross dependency between runtime suspend/resume and
>> common suspend/resume. Oh the horror ...
>
> This should be fine, we runtime resume before we suspend.

I believe Vipul sent this patch for the cases where RTPM in not
enabled in the configs.

--
viresh

[PATCH 1/2] spi:pl022: Disable/Enable functional clock from suspend/resume

[Linus Walleij]

On Wed, Sep 26, 2012 at 4:08 PM, viresh kumar <viresh.kumar@linaro.org> wrote:
> On Wed, Sep 26, 2012 at 5:49 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Wed, Sep 26, 2012 at 02:17:36PM +0200, Linus Walleij wrote:
>>> On Wed, Sep 26, 2012 at 1:24 PM, Vipul Kumar Samar
>>
>>> > SPI functional clock must be disalble/enable in non RTPM suspend/resume
>>> > hooks. Currently it is only done for RTPM cases.
>>
>>> > This patch add support to disable/enbale clock for conventional
>>> > suspend/resume calls.
>>
>>> Cross dependency between runtime suspend/resume and
>>> common suspend/resume. Oh the horror ...
>>
>> This should be fine, we runtime resume before we suspend.
>
> I believe Vipul sent this patch for the cases where RTPM in not
> enabled in the configs.

OK so we need to handle the cases where either, both or
just one of them is enabled...

Mark says the defined semantics is that runtime PM is
resumed across suspend/resume but I'd just like to
understand the overall mechanism that makes sure
this happens and I'm go...

However there is another problem with the patch,
because in -next there is also pin control handling
in the runtime hooks, so we need to duplicate
not only clocks but also that in each of the functions.

Maybe we can first make a patch that breaks out
resource handling so we can call that from each
of the suspend/resume calls? (I'll try.)

Yours,
Linus Walleij

[PATCH 2/2] ARM: ABMA: Disable/Enable interface clock from suspend/resume

[Vipul Kumar Samar]

AMBA devices interface clock is disabled in RTPM suspend/resume hooks
but not in conventional hooks.

This patch adds support to disable/enable clock for conventional
suspend/resume calls.

Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com>
---
 drivers/amba/bus.c |   42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index b7e7285..ded3d98 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -120,6 +120,7 @@ static int amba_legacy_resume(struct device *dev)
 static int amba_pm_suspend(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
+	struct amba_device *pcdev = to_amba_device(dev);
 	int ret = 0;
 
 	if (!drv)
@@ -132,16 +133,27 @@ static int amba_pm_suspend(struct device *dev)
 		ret = amba_legacy_suspend(dev, PMSG_SUSPEND);
 	}
 
+	if (!ret)
+		clk_disable(pcdev->pclk);
+
 	return ret;
 }
 
 static int amba_pm_resume(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
+	struct amba_device *pcdev = to_amba_device(dev);
 	int ret = 0;
 
-	if (!drv)
+	if (!drv) {
 		return 0;
+	} else {
+		ret = clk_enable(pcdev->pclk);
+		if (ret) {
+			dev_err(dev, "Resume: Clk enable failed");
+			return ret;
+		}
+	}
 
 	if (drv->pm) {
 		if (drv->pm->resume)
@@ -165,6 +177,7 @@ static int amba_pm_resume(struct device *dev)
 static int amba_pm_freeze(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
+	struct amba_device *pcdev = to_amba_device(dev);
 	int ret = 0;
 
 	if (!drv)
@@ -177,16 +190,27 @@ static int amba_pm_freeze(struct device *dev)
 		ret = amba_legacy_suspend(dev, PMSG_FREEZE);
 	}
 
+	if (!ret)
+		clk_disable(pcdev->pclk);
+
 	return ret;
 }
 
 static int amba_pm_thaw(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
+	struct amba_device *pcdev = to_amba_device(dev);
 	int ret = 0;
 
-	if (!drv)
+	if (!drv) {
 		return 0;
+	} else {
+		ret = clk_enable(pcdev->pclk);
+		if (ret) {
+			dev_err(dev, "Thaw: Clk enable failed");
+			return ret;
+		}
+	}
 
 	if (drv->pm) {
 		if (drv->pm->thaw)
@@ -201,6 +225,7 @@ static int amba_pm_thaw(struct device *dev)
 static int amba_pm_poweroff(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
+	struct amba_device *pcdev = to_amba_device(dev);
 	int ret = 0;
 
 	if (!drv)
@@ -213,16 +238,27 @@ static int amba_pm_poweroff(struct device *dev)
 		ret = amba_legacy_suspend(dev, PMSG_HIBERNATE);
 	}
 
+	if (!ret)
+		clk_disable(pcdev->pclk);
+
 	return ret;
 }
 
 static int amba_pm_restore(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
+	struct amba_device *pcdev = to_amba_device(dev);
 	int ret = 0;
 
-	if (!drv)
+	if (!drv) {
 		return 0;
+	} else {
+		ret = clk_enable(pcdev->pclk);
+		if (ret) {
+			dev_err(dev, "Restore: Clk enable failed");
+			return ret;
+		}
+	}
 
 	if (drv->pm) {
 		if (drv->pm->restore)
-- 
1.7.10

[PATCH 2/2] ARM: ABMA: Disable/Enable interface clock from suspend/resume

[Linus Walleij]

On Wed, Sep 26, 2012 at 1:24 PM, Vipul Kumar Samar
<vipulkumar.samar@st.com> wrote:

> AMBA devices interface clock is disabled in RTPM suspend/resume hooks
> but not in conventional hooks.
>
> This patch adds support to disable/enable clock for conventional
> suspend/resume calls.
(...)
> +       struct amba_device *pcdev = to_amba_device(dev);
>         int ret = 0;
>
>         if (!drv)
> @@ -132,16 +133,27 @@ static int amba_pm_suspend(struct device *dev)
>                 ret = amba_legacy_suspend(dev, PMSG_SUSPEND);
>         }
>
> +       if (!ret)
> +               clk_disable(pcdev->pclk);
> +
>         return ret;
>  }

You're not accounting for the case where pcdev->pclk
is an error pointer (as happens if pclk lookup fails at
probe).

I think you can simplify some of the code using
the external accessors from <linux/amba/bus.h>:

amba_pclk_enable();
amba_pclk_disable();

But:

Again this is a case where you have a race between runtime
suspend/resume and ordinary suspend/resume.

These ordinary suspend/resume operations should probably
wait for runtime suspend to happen *first* if and only if
runtime PM is enabled, and then the block will be gated
off. So we really need to figure out how we can make sure
that this happens.

If just ordinary PM is enabled, the above makes sense but in
that case I think we should have that #ifdef:ed as an alternative
or something.

So something like:

suspend():
#ifdef CONFIG_PM_RUNTIME
   /* Wait for runtime PM to hammer down the pclk */
#elif CONFIG_PM
   amba_pclk_disable();
#endif

resume():
#if defined(CONFIG_PM) && !defined(CONFIG_PM_RUNTIME)
  amba_pclk_enable();
#endif
  /* Let runtime PM lazily enable the clock when needed */

To complicate things further the bus operations can be
overridden by e.g. voltage domains. In this case we (ux500)
have choosen to call out to the AMBA level to make sure
semantics are preserved.

Yours,
Linus Walleij