[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Ulf Hansson]

To be able to make sure devices are put into runtime suspend
after a suspend sequence, the suspend_noirq callback is used.

Previously it was was possible for drivers doing pm_runtime_suspend
and pm_runtime_put_sync directly from it's suspend callbacks.
This is now not possible due to the following commit, which solve a
race issue:

PM: Limit race conditions between runtime PM and system sleep (v2)

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
---
 drivers/amba/bus.c |  113 ++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 79 insertions(+), 34 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index bd230e8..7ba523a 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -135,6 +135,79 @@ static void amba_pm_complete(struct device *dev)
 
 #endif /* !CONFIG_PM_SLEEP */
 
+#ifdef CONFIG_PM_RUNTIME
+/*
+ * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
+ * enable/disable the bus clock at runtime PM suspend/resume as this
+ * does not result in loss of context.  However, disabling vcore power
+ * would do, so we leave that to the driver.
+ */
+static int amba_pm_runtime_suspend(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	int ret = pm_generic_runtime_suspend(dev);
+
+	if (ret == 0 && dev->driver)
+		clk_disable(pcdev->pclk);
+
+	return ret;
+}
+
+static int amba_pm_runtime_suspend_noirq(struct device *dev)
+{
+	int ret = 0;
+
+	/*
+	 * If the amba device is not already runtime suspended
+	 * force it into this state to save power.
+	 */
+	if (!pm_runtime_status_suspended(dev))
+		ret = amba_pm_runtime_suspend(dev);
+
+	return ret;
+}
+
+static int amba_pm_runtime_resume(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	int ret;
+
+	if (dev->driver) {
+		ret = clk_enable(pcdev->pclk);
+		/* Failure is probably fatal to the system, but... */
+		if (ret)
+			return ret;
+	}
+
+	return pm_generic_runtime_resume(dev);
+}
+
+static int amba_pm_runtime_resume_noirq(struct device *dev)
+{
+	int ret = 0;
+
+	/*
+	 * If the amba device has been forced into runtime suspend state,
+	 * we must make sure to restore it back into runtime resumed state.
+	 */
+	if (!pm_runtime_status_suspended(dev))
+		ret = amba_pm_runtime_resume(dev);
+
+	return ret;
+}
+#else /* !CONFIG_PM_RUNTIME */
+
+static int amba_pm_runtime_suspend_noirq(struct device *dev)
+{
+	return 0;
+}
+static int amba_pm_runtime_resume_noirq(struct device *dev)
+{
+	return 0;
+}
+
+#endif /* !CONFIG_PM_RUNTIME */
+
 #ifdef CONFIG_SUSPEND
 
 static int amba_pm_suspend(struct device *dev)
@@ -163,7 +236,9 @@ static int amba_pm_suspend_noirq(struct device *dev)
 	if (!drv)
 		return 0;
 
-	if (drv->pm) {
+	ret = amba_pm_runtime_suspend_noirq(dev);
+
+	if (!ret && drv->pm) {
 		if (drv->pm->suspend_noirq)
 			ret = drv->pm->suspend_noirq(dev);
 	}
@@ -202,6 +277,9 @@ static int amba_pm_resume_noirq(struct device *dev)
 			ret = drv->pm->resume_noirq(dev);
 	}
 
+	if (!ret)
+		ret = amba_pm_runtime_resume_noirq(dev);
+
 	return ret;
 }
 
@@ -365,39 +443,6 @@ static int amba_pm_restore_noirq(struct device *dev)
 
 #endif /* !CONFIG_HIBERNATE_CALLBACKS */
 
-#ifdef CONFIG_PM_RUNTIME
-/*
- * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
- * enable/disable the bus clock at runtime PM suspend/resume as this
- * does not result in loss of context.  However, disabling vcore power
- * would do, so we leave that to the driver.
- */
-static int amba_pm_runtime_suspend(struct device *dev)
-{
-	struct amba_device *pcdev = to_amba_device(dev);
-	int ret = pm_generic_runtime_suspend(dev);
-
-	if (ret == 0 && dev->driver)
-		clk_disable(pcdev->pclk);
-
-	return ret;
-}
-
-static int amba_pm_runtime_resume(struct device *dev)
-{
-	struct amba_device *pcdev = to_amba_device(dev);
-	int ret;
-
-	if (dev->driver) {
-		ret = clk_enable(pcdev->pclk);
-		/* Failure is probably fatal to the system, but... */
-		if (ret)
-			return ret;
-	}
-
-	return pm_generic_runtime_resume(dev);
-}
-#endif
 
 #ifdef CONFIG_PM
 
-- 
1.7.5.4

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Russell King - ARM Linux]

On Thu, Oct 27, 2011 at 05:48:44PM +0200, Ulf Hansson wrote:
> To be able to make sure devices are put into runtime suspend
> after a suspend sequence, the suspend_noirq callback is used.
> 
> Previously it was was possible for drivers doing pm_runtime_suspend
> and pm_runtime_put_sync directly from it's suspend callbacks.
> This is now not possible due to the following commit, which solve a
> race issue:
> 
> PM: Limit race conditions between runtime PM and system sleep (v2)

There's a patch which has been queued for the last 2 months adding
runtime PM support to the AMBA bus layer which is now merged into
mainline.  Please check whether this works for you.

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Linus Walleij]

2011/10/27 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> On Thu, Oct 27, 2011 at 05:48:44PM +0200, Ulf Hansson wrote:
>> To be able to make sure devices are put into runtime suspend
>> after a suspend sequence, the suspend_noirq callback is used.
>>
>> Previously it was was possible for drivers doing pm_runtime_suspend
>> and pm_runtime_put_sync directly from it's suspend callbacks.
>> This is now not possible due to the following commit, which solve a
>> race issue:
>>
>> PM: Limit race conditions between runtime PM and system sleep (v2)
>
> There's a patch which has been queued for the last 2 months adding
> runtime PM support to the AMBA bus layer which is now merged into
> mainline. ?Please check whether this works for you.

I think this patch is on top of that one. This moves the
runtime_[suspend|resume] so as to put all the runtime PM stuff
inside the same #ifdef block, then adds two new _noirq
functions to address the problems from the named
commit by Rafael.

Yours,
Linus Walleij

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Russell King - ARM Linux]

On Thu, Oct 27, 2011 at 08:27:36PM +0200, Linus Walleij wrote:
> I think this patch is on top of that one. This moves the
> runtime_[suspend|resume] so as to put all the runtime PM stuff
> inside the same #ifdef block, then adds two new _noirq
> functions to address the problems from the named
> commit by Rafael.

I don't think it does address any problems mentioned by Rafael - the
referred to commit is about avoiding races between the system suspend
method and the runtime PM methods which is an entirely different
problem to ensuring that the bus clock is shutdown on suspend.

Ignoring runtime PM entirely, what this patch is doing is _adding_
management of the primecell bus clock to the bus layer and taking
control of that away from drivers.  When I implemented the bus clock
support, it was based around the bus clock being enabled at probe
time, and only subsequently disabling it after the driver was
removed - all other management was left to the driver to manage as
appropriate.

That brings up the question whether there has been any check to
ensure that there are no drivers already managing the bus clock in
their suspend methods - if there are, this patch has the potential
to make the bus clock enable count to negative.

There's also the related point here that the behaviour that you get
for the bus clock from a system suspend depends on whether you have
runtime-PM configured or not - and that's a recipe for drivers being
scattered with ifdefs to 'undo' this variable semantic.

However, that's not the only problem - let's look closer at what is
being done in the patch.

The ordering on suspend is:

Enter amba_pm_suspend_noirq()
Call amba_pm_runtime_suspend_noirq()
	If runtime PM is not suspended
		Disable pclk
Call driver suspend_noirq method
Return
...
Enter amba_pm_resume_noirq()
	If runtime PM is not suspended
		Enable pclk
Call driver resume_noirq method
Call amba_pm_runtime_resume_noirq()
Return

This _can't_ be correct - we're potentially disabling the bus clock
and then calling the driver (which may try to access the registers)
which may cause a bus fault, a data abort and therefore a kernel oops.
That's really not nice and sane behaviour.

Also throw into the mix whether it's appropriate for the bus clock
to be turned off while the primecell is acting as a wakeup source -
can we safely say that it is legal and proper to disable the bus
clock for every primecell?

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Ulf Hansson]

Russell King - ARM Linux wrote:
> On Thu, Oct 27, 2011 at 08:27:36PM +0200, Linus Walleij wrote:
>> I think this patch is on top of that one. This moves the
>> runtime_[suspend|resume] so as to put all the runtime PM stuff
>> inside the same #ifdef block, then adds two new _noirq
>> functions to address the problems from the named
>> commit by Rafael.
> 
> I don't think it does address any problems mentioned by Rafael - the
> referred to commit is about avoiding races between the system suspend
> method and the runtime PM methods which is an entirely different
> problem to ensuring that the bus clock is shutdown on suspend.

Yes, but the solution in patch from Rafael is now preventing 
drivers/buses to do pm_runtime_suspend or pm_runtime_put_sync in their 
suspend hooks. So if devices are in runtime resume state when their 
suspend functions is entered, devices will be kept in runtime resume state.

To accomplish the same things as when a runtime_suspend happens the 
suspend|resume_noirq hooks are now the recommended way to be used which 
is the main reason for this patch.

> 
> Ignoring runtime PM entirely, what this patch is doing is _adding_
> management of the primecell bus clock to the bus layer and taking
> control of that away from drivers.  When I implemented the bus clock
> support, it was based around the bus clock being enabled at probe
> time, and only subsequently disabling it after the driver was
> removed - all other management was left to the driver to manage as
> appropriate.

If I not misunderstood something, the bus clock is supposed to be 
controlled by the amba bus runtime functions. The control of the vcore 
regulator is up to each amba driver (amba bus handles it in probe and 
remove only). Please clarify if I missed out something here.

> 
> That brings up the question whether there has been any check to
> ensure that there are no drivers already managing the bus clock in
> their suspend methods - if there are, this patch has the potential
> to make the bus clock enable count to negative.
> 

The count is always restored as well as the runtime state from 
amba_pm_resume_noirq.

> There's also the related point here that the behaviour that you get
> for the bus clock from a system suspend depends on whether you have
> runtime-PM configured or not - and that's a recipe for drivers being
> scattered with ifdefs to 'undo' this variable semantic.
> 
> However, that's not the only problem - let's look closer at what is
> being done in the patch.
> 
> The ordering on suspend is:
> 
> Enter amba_pm_suspend_noirq()
> Call amba_pm_runtime_suspend_noirq()
> 	If runtime PM is not suspended
> 		Disable pclk
> Call driver suspend_noirq method
> Return
> ...
> Enter amba_pm_resume_noirq()
> 	If runtime PM is not suspended
> 		Enable pclk
> Call driver resume_noirq method
> Call amba_pm_runtime_resume_noirq()
> Return
> 
> This _can't_ be correct - we're potentially disabling the bus clock
> and then calling the driver (which may try to access the registers)
> which may cause a bus fault, a data abort and therefore a kernel oops.
> That's really not nice and sane behaviour.

I agree! Let's reverse the order of when calling noirq callbacks!

Although I would like the status of pm_runtime_status_suspended to be 
fetched before we enter any noirq callback. This will make it possible 
for the noirq function to change runtime state, which may be wanted to 
for example prevent an unnecessary runtime resume during system resume.

> 
> Also throw into the mix whether it's appropriate for the bus clock
> to be turned off while the primecell is acting as a wakeup source -
> can we safely say that it is legal and proper to disable the bus
> clock for every primecell?

A most valid question.

If you foresee that any amba driver would like to keep the device/bus 
runtime resumed while the system is suspended; I can add a amba bus 
function to be called from a driver probe function to set a flag to 
prevent "force runtime suspend" from the suspend_noirq in the amba bus.

Or maybe there is another more generic way of telling that "this" device 
is a wake_up device, do not runtime_suspend it.

> 

Best regards
Ulf Hansson

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Russell King - ARM Linux]

On Fri, Oct 28, 2011 at 11:29:55AM +0200, Ulf Hansson wrote:
> Russell King - ARM Linux wrote:
>> On Thu, Oct 27, 2011 at 08:27:36PM +0200, Linus Walleij wrote:
>>> I think this patch is on top of that one. This moves the
>>> runtime_[suspend|resume] so as to put all the runtime PM stuff
>>> inside the same #ifdef block, then adds two new _noirq
>>> functions to address the problems from the named
>>> commit by Rafael.
>>
>> I don't think it does address any problems mentioned by Rafael - the
>> referred to commit is about avoiding races between the system suspend
>> method and the runtime PM methods which is an entirely different
>> problem to ensuring that the bus clock is shutdown on suspend.
>
> Yes, but the solution in patch from Rafael is now preventing
> drivers/buses to do pm_runtime_suspend or pm_runtime_put_sync in their
> suspend hooks. So if devices are in runtime resume state when their
> suspend functions is entered, devices will be kept in runtime resume
> state.
>
> To accomplish the same things as when a runtime_suspend happens the
> suspend|resume_noirq hooks are now the recommended way to be used which
> is the main reason for this patch.

I think the problem is actually much worse - what if the device is
already runtime-PM suspended at the point that we enter the suspend
callbacks. pm_runtime_get_noresume() in __device_suspend() won't
resume the device, which means potentially registers won't be
accessible.

So, as things now stand, suspend callbacks can be entered with runtime-PM
in either state, although the 'usage_count' will be incremented preventing
a runtime-PM suspend occuring.

>> Ignoring runtime PM entirely, what this patch is doing is _adding_
>> management of the primecell bus clock to the bus layer and taking
>> control of that away from drivers.  When I implemented the bus clock
>> support, it was based around the bus clock being enabled at probe
>> time, and only subsequently disabling it after the driver was
>> removed - all other management was left to the driver to manage as
>> appropriate.
>
> If I not misunderstood something, the bus clock is supposed to be  
> controlled by the amba bus runtime functions. The control of the vcore  
> regulator is up to each amba driver (amba bus handles it in probe and  
> remove only). Please clarify if I missed out something here.

For runtime-PM, it is the intention that the bus clock is controlled
by the bus-level runtime functions.  However, ihis opens a can of worms
as I mentioned in my previous message - and is quited below.

>> That brings up the question whether there has been any check to
>> ensure that there are no drivers already managing the bus clock in
>> their suspend methods - if there are, this patch has the potential
>> to make the bus clock enable count to negative.
>
> The count is always restored as well as the runtime state from  
> amba_pm_resume_noirq.

No clock counts are ever 'restored' - they're never saved.  If you do
this:

	clk_enable(clk);
	clk_disable(clk);
	clk_disable(clk);

then the second clk_disable() will spit out a warning and refuse to
decrement the count (because non-zero counts mean that the clock is
enabled.)

>> There's also the related point here that the behaviour that you get
>> for the bus clock from a system suspend depends on whether you have
>> runtime-PM configured or not - and that's a recipe for drivers being
>> scattered with ifdefs to 'undo' this variable semantic.

>> Also throw into the mix whether it's appropriate for the bus clock
>> to be turned off while the primecell is acting as a wakeup source -
>> can we safely say that it is legal and proper to disable the bus
>> clock for every primecell?
>
> A most valid question.
>
> If you foresee that any amba driver would like to keep the device/bus  
> runtime resumed while the system is suspended; I can add a amba bus  
> function to be called from a driver probe function to set a flag to  
> prevent "force runtime suspend" from the suspend_noirq in the amba bus.

We don't need flags.  If we go down the route of managing the bus clock
on system suspend and a driver wants to keep the bus clock on while
suspended it can simply issue an amba_pclk_enable() in its own suspend
method and an amba_pclk_disable() in its resume method.  Starting to
create flags for these corner cases just makes things a lot more
complex by adding exceptions and creating more hairly code.

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Ulf Hansson]

>> Yes, but the solution in patch from Rafael is now preventing
>> drivers/buses to do pm_runtime_suspend or pm_runtime_put_sync in their
>> suspend hooks. So if devices are in runtime resume state when their
>> suspend functions is entered, devices will be kept in runtime resume
>> state.
>>
>> To accomplish the same things as when a runtime_suspend happens the
>> suspend|resume_noirq hooks are now the recommended way to be used which
>> is the main reason for this patch.
> 
> I think the problem is actually much worse - what if the device is
> already runtime-PM suspended at the point that we enter the suspend
> callbacks. pm_runtime_get_noresume() in __device_suspend() won't
> resume the device, which means potentially registers won't be
> accessible.

That's true, but drivers must still do (and are able to) get_sync (or 
similar) to make it's device to be runtime resumed if they have a need 
for it. This has not changed.

> 
> So, as things now stand, suspend callbacks can be entered with runtime-PM
> in either state, although the 'usage_count' will be incremented preventing
> a runtime-PM suspend occuring.
>

Correct.

>>> Ignoring runtime PM entirely, what this patch is doing is _adding_
>>> management of the primecell bus clock to the bus layer and taking
>>> control of that away from drivers.  When I implemented the bus clock
>>> support, it was based around the bus clock being enabled at probe
>>> time, and only subsequently disabling it after the driver was
>>> removed - all other management was left to the driver to manage as
>>> appropriate.
>> If I not misunderstood something, the bus clock is supposed to be  
>> controlled by the amba bus runtime functions. The control of the vcore  
>> regulator is up to each amba driver (amba bus handles it in probe and  
>> remove only). Please clarify if I missed out something here.
> 
> For runtime-PM, it is the intention that the bus clock is controlled
> by the bus-level runtime functions.  However, ihis opens a can of worms
> as I mentioned in my previous message - and is quited below.
>

I don't think it is problematic. It is more a definition of were we 
decide to do the work.

As we do now, the AMBA bus do set up some prerequisites during probe 
(enable vcore and pclk) and as well do runtime handling of the pclk. 
This must each amba device take into consideration which can be a little 
bit tricky, I think.

The benefit is obviously more common code.

>>> That brings up the question whether there has been any check to
>>> ensure that there are no drivers already managing the bus clock in
>>> their suspend methods - if there are, this patch has the potential
>>> to make the bus clock enable count to negative.
>> The count is always restored as well as the runtime state from  
>> amba_pm_resume_noirq.
> 
> No clock counts are ever 'restored' - they're never saved.  If you do
> this:
> 
> 	clk_enable(clk);
> 	clk_disable(clk);
> 	clk_disable(clk);
> 
> then the second clk_disable() will spit out a warning and refuse to
> decrement the count (because non-zero counts mean that the clock is
> enabled.)

I see your concern, but this will never happen. If I have not completely 
missed something of course. :-)

We will not call the runtime hooks from suspend@|resume_noirq if the 
device already is runtime suspended.

If is not runtime suspended, we call the runtime_suspend callback from 
suspend_noirq. In resume_noirq we then will call the runtime_resume 
callback as well. This means the "state" (meaning clocks, regulators and 
other stuff) is restored to the state as they were when the driver left 
it's normal suspend callback.

> 
>>> There's also the related point here that the behaviour that you get
>>> for the bus clock from a system suspend depends on whether you have
>>> runtime-PM configured or not - and that's a recipe for drivers being
>>> scattered with ifdefs to 'undo' this variable semantic.
> 
>>> Also throw into the mix whether it's appropriate for the bus clock
>>> to be turned off while the primecell is acting as a wakeup source -
>>> can we safely say that it is legal and proper to disable the bus
>>> clock for every primecell?
>> A most valid question.
>>
>> If you foresee that any amba driver would like to keep the device/bus  
>> runtime resumed while the system is suspended; I can add a amba bus  
>> function to be called from a driver probe function to set a flag to  
>> prevent "force runtime suspend" from the suspend_noirq in the amba bus.
> 
> We don't need flags.  If we go down the route of managing the bus clock
> on system suspend and a driver wants to keep the bus clock on while
> suspended it can simply issue an amba_pclk_enable() in its own suspend
> method and an amba_pclk_disable() in its resume method.  Starting to
> create flags for these corner cases just makes things a lot more
> complex by adding exceptions and creating more hairly code.
> 

Agree! Additionally the driver can configure the device as a "wakeup" 
device. Then we use "device_may_wakeup" as a pre-condition for not 
calling the runtime callbacks during supend|resume_noirq.

BR
Ulf Hansson

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Russell King - ARM Linux]

On Wed, Nov 02, 2011 at 11:55:42AM +0100, Ulf Hansson wrote:
> As we do now, the AMBA bus do set up some prerequisites during probe  
> (enable vcore and pclk) and as well do runtime handling of the pclk.  
> This must each amba device take into consideration which can be a little  
> bit tricky, I think.

If you think that then you're missing a fundamental point about how the
bus layer has been setup.

Whatever the bus layer is doing has (and should continue to) remain
invisible to the driver.  Or to put it another way: before we had to
deal with the bus clock, drivers assumed that the bus clock was always
enabled while they owned the device.

When we had a platform where the bus clock could be disabled, I ensured
that we had a way for the bus layer to obtain that clock before probe
time and ensure that it was enabled until after remove time - this
ensures that we maintain the guarantee that the bus clock was enabled
while the driver owns the device.

If a driver _then_ decides it wants to disable the bus clock, it can, but
then the driver takes responsibility for ensuring that the bus clock is
appropriately managed _throughout_ the driver, and most importantly that
the bus clock is left enabled when returning from the ->remove callback
(to maintain the "illusion" that the bus clock has been enabled the whole
time.)

That same approach has been taken when runtime-PM support was added to the
bus layer: although runtime-PM support is enabled, it is left with a
ref-count of 1 at the point the drivers probe function is called.  This
ensures that we're in the runtime-resume state, which is the state that
the device should be in at that point.

At the point where the ->remove callback returns, we also expect the
device to be in the runtime-resume state.  Should the driver wish to make
use of the runtime-PM facility, it is up to the driver to start managing
that by doing a pm_runtime_put() in the probe function - but if it does
that it must do a pm_runtime_get() (or similar) in the remove function to
balance.

As for the suspend callback, it works like this:
- If the driver is not using runtime-PM, then the bus clock is still
  running anyway.  Effectively, the device is in 'runtime-resume' state.

- If the driver has enabled runtime-PM, the suspend callbacks can be
  entered in either runtime-resume or runtime-suspended state:

  + if it needs to access the device registers, it must do a
    pm_runtime_get_sync() call to ensure that the bus clock is running
    and the device is runtime-resumed - this gets us into the same
    position as the non-runtime PM drivers.

  + if it does not need to access the device registers, this is a more
    complicated case to handle because the final parts of suspend are
    conditional.  This can be resolved fairly easily though.

Now, if a driver wants to shut down the bus clock, _and_ the device is in
runtime-resume state (that test can be optimized away given the above
conditions), it can then call amba_pclk_disable().  If it wants to do more
than that (eg, all the runtime-suspend stuff) then it must do that itself.
Remember that a driver _should_ suspend properly with system suspend even
if runtime-PM is disabled.

On resume, if it called amba_pclk_disable() in the suspend callback, it
must call amba_pclk_enable() (and any other stuff that was shut down)
before accessing any registers.  Then it can access the registers etc.
Before returning from the resume callback:
- If the driver has enabled runtime-PM, it must call pm_runtime_put() to
  balance the call to pm_runtime_get_sync() in the suspend callback.
- If the driver is not using runtime-PM, then it is done as the bus clock
  is now running.

So, suspend callbacks should look something like this:

non_pm_runtime_driver_suspend(dev)
{
	save_device_state(dev);
	if (device_can_disable_pclk_at_suspend(dev) &&
	    (device_can_wakeup_with_pclk_disabled(dev) || !device_may_wakeup(dev)))
		amba_pclk_disable(dev);
	... and anything else which needs to be turned off ...
}

non_pm_runtime_driver_resume(dev)
{
	if (device_can_disable_pclk_at_suspend(dev) &&
	    (device_can_wakeup_with_pclk_disabled(dev) || !device_may_wakeup(dev)))
		amba_pclk_enable(dev);
	... and anything else which was turned off above needs to be turned back on ...
	restore_device_state(dev);
}

where device_can_disable_pclk_at_suspend() and device_can_wakeup_with_pclk_disabled()
are constants (and so the if condition is optimized appropriately or maybe
even eliminated completely.)

=== or ===

pm_runtime_driver_suspend(dev)
{
	pm_runtime_get_sync(dev);
	save_device_state(dev);
	if (device_can_disable_pclk_at_suspend(dev) &&
	    (device_can_wakeup_with_pclk_disabled(dev) || !device_may_wakeup(dev)))
		amba_pclk_disable(dev);
	... and anything else which needs to be turned off ...
}

pm_runtime_driver_resume(dev)
{
	if (device_can_disable_pclk_at_suspend(dev) &&
	    (device_can_wakeup_with_pclk_disabled(dev) || !device_may_wakeup(dev)))
		amba_pclk_enable(dev);
	... and anything else which was turned off above needs to be turned back on ...
	restore_device_state(dev);
	pm_runtime_put(dev);
}

=== or ===

pm_runtime_driver_suspend(dev)
{
	if (pm_runtime_status_suspended(dev))
		return;

	if (device_can_disable_pclk_at_suspend(dev) &&
	    (device_can_wakeup_with_pclk_disabled(dev) || !device_may_wakeup(dev)))
		amba_pclk_disable(dev);
	... and anything else which needs to be turned off ...
}

pm_runtime_driver_resume(dev)
{
	if (pm_runtime_status_suspended(dev))
		return;

	if (device_can_disable_pclk_at_suspend(dev) &&
	    (device_can_wakeup_with_pclk_disabled(dev) || !device_may_wakeup(dev)))
		amba_pclk_enable(dev);
	... and anything else which was turned off above needs to be turned back on ...
}

>>>> That brings up the question whether there has been any check to
>>>> ensure that there are no drivers already managing the bus clock in
>>>> their suspend methods - if there are, this patch has the potential
>>>> to make the bus clock enable count to negative.
>>> The count is always restored as well as the runtime state from   
>>> amba_pm_resume_noirq.
>>
>> No clock counts are ever 'restored' - they're never saved.  If you do
>> this:
>>
>> 	clk_enable(clk);
>> 	clk_disable(clk);
>> 	clk_disable(clk);
>>
>> then the second clk_disable() will spit out a warning and refuse to
>> decrement the count (because non-zero counts mean that the clock is
>> enabled.)
>
> I see your concern, but this will never happen. If I have not completely  
> missed something of course. :-)

Tell me why you think it won't happen.  I think it will:

	clk_enable(pclk) <-- before probe
	clk_disable(pclk) <-- inside amba_pclk_disable() in system
				suspend callback
	clk_disable(pclk) <-- amba_pm_suspend_noirq

> We will not call the runtime hooks from suspend@|resume_noirq if the  
> device already is runtime suspended.
>
> If is not runtime suspended, we call the runtime_suspend callback from  
> suspend_noirq. In resume_noirq we then will call the runtime_resume  
> callback as well. This means the "state" (meaning clocks, regulators and  
> other stuff) is restored to the state as they were when the driver left  
> it's normal suspend callback.

The problem with this is that it makes the driver writers task a lot
harder to comprehend what state things are in the various callbacks.
The driver writer can no longer be sure _what_ needs to be done in the
suspend callback because it depends whether they're writing a driver to
support runtime-PM, whether the device was in runtime-resume or runtime-
suspend state, etc.

So, the statement I made above becomes:

As for the suspend callback, it works like this:
- If the driver has enabled runtime-PM, it must do a pm_runtime_get_sync()
  call to ensure that the bus clock is running (and the device is
  runtime-resumed) if it needs to access the device registers.
- If the driver is not using runtime-PM, then the bus clock is still running
  anyway.
If the driver now wants the bus clock to remain on (eg, because the device
needs it for wakeup), it must call amba_pclk_enable() here.  Therefore,
this call must be added to _every_ existing driver right now to ensure
that the status-quo is maintained.

On resume, if it called amba_pclk_enable(), it must call amba_pclk_disable()
to balance the effects of the suspend callback, and this must be added to
every existing driver right now.  Before returning from the resume callback:
- If the driver has enabled runtime-PM, it must call pm_runtime_put() to
  balance the call to pm_runtime_get_sync() in the suspend callback.
- If the driver is not using runtime-PM, then it is done as the bus clock
  is now running.

I see in v3 you've dropped the bus clock management code for the noirq
path - so what we now have is yet more obfuscation of the suspend/resume
callback conditions.  We can now be partially runtime-suspended after
suspend_noirq (the effects of the runtime suspend handler but without
the bus clock being turned off.)

Given what I've said in the first hunk of this reply, I really don't see
any need for any patch to this code at all if a driver is implemented
using the scheme I outlined there.

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Alan Stern]

On Thu, 27 Oct 2011, Ulf Hansson wrote:

> To be able to make sure devices are put into runtime suspend
> after a suspend sequence, the suspend_noirq callback is used.
> 
> Previously it was was possible for drivers doing pm_runtime_suspend
> and pm_runtime_put_sync directly from it's suspend callbacks.
> This is now not possible due to the following commit, which solve a
> race issue:
> 
> PM: Limit race conditions between runtime PM and system sleep (v2)

I have a small request for this patch:

> +static int amba_pm_runtime_suspend_noirq(struct device *dev)
> +{
...
> +}

> +static int amba_pm_runtime_resume_noirq(struct device *dev)
> +{
...
> +}

Can the names of these functions be changed?  Since they run during
system sleep transitions with IRQs disabled, they aren't really runtime
suspend or resume routines.

In fact, since each routine is used in only one place, maybe they can 
be eliminated entirely and moved into their callers?

Alan Stern

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Ulf Hansson]

Alan Stern wrote:
> On Thu, 27 Oct 2011, Ulf Hansson wrote:
> 
>> To be able to make sure devices are put into runtime suspend
>> after a suspend sequence, the suspend_noirq callback is used.
>>
>> Previously it was was possible for drivers doing pm_runtime_suspend
>> and pm_runtime_put_sync directly from it's suspend callbacks.
>> This is now not possible due to the following commit, which solve a
>> race issue:
>>
>> PM: Limit race conditions between runtime PM and system sleep (v2)
> 
> I have a small request for this patch:
> 
>> +static int amba_pm_runtime_suspend_noirq(struct device *dev)
>> +{
> ...
>> +}
> 
>> +static int amba_pm_runtime_resume_noirq(struct device *dev)
>> +{
> ...
>> +}
> 
> Can the names of these functions be changed?  Since they run during
> system sleep transitions with IRQs disabled, they aren't really runtime
> suspend or resume routines.

Sure, we can rename/move the code - do you have a proposal? :-)

The idea with having them as separate functions and something with 
"runtime" in the name is because it is only when having 
CONFIG_PM_RUNTIME the functions actually does something.

If I move the the code into amba_pm_suspend|resume_noirq I will need 
ifdefs around the code. I am fine with that as well...

> 
> In fact, since each routine is used in only one place, maybe they can 
> be eliminated entirely and moved into their callers?
> 
> Alan Stern
> 
> 

BR
Ulf Hansson

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Alan Stern]

On Fri, 28 Oct 2011, Ulf Hansson wrote:

> The idea with having them as separate functions and something with 
> "runtime" in the name is because it is only when having 
> CONFIG_PM_RUNTIME the functions actually does something.

No, that's off the main point.

In fact, you're not really using these terms properly.  You shouldn't 
think of "runtime suspend" as a single verb -- that is, you shouldn't 
think "Okay, let's runtime-suspend this device now".

The verb is just "suspend", and it means the same thing as "put into a
low-power state".  The "runtime" part refers to _when_ the low-power 
transition takes place: while the rest of the system is running (as 
opposed to while the rest of the system is going to sleep).

Thus, it makes no sense at all for a comment in a suspend_noirq
callback to say "let's runtime-suspend the device".  That's a 
contradiction in terms.  The right way to think about it is more like 
this: "If the device isn't already in a low-power state, let's put it 
in a low-power state now".

In your case, the device already is (or should be) at low power.  A
better way to phrase this might be: "If the device's power hasn't
already been turned off, and the device doesn't need to generate wakeup
requests, turn off the power now".

Alan Stern

[PATCH] AMBA: Use suspend_noriq to force devices into runtime suspend

[Ulf Hansson]

Alan Stern wrote:
> On Fri, 28 Oct 2011, Ulf Hansson wrote:
> 
>> The idea with having them as separate functions and something with 
>> "runtime" in the name is because it is only when having 
>> CONFIG_PM_RUNTIME the functions actually does something.
> 
> No, that's off the main point.
> 
> In fact, you're not really using these terms properly.  You shouldn't 
> think of "runtime suspend" as a single verb -- that is, you shouldn't 
> think "Okay, let's runtime-suspend this device now".
> 
> The verb is just "suspend", and it means the same thing as "put into a
> low-power state".  The "runtime" part refers to _when_ the low-power 
> transition takes place: while the rest of the system is running (as 
> opposed to while the rest of the system is going to sleep).
> 
> Thus, it makes no sense at all for a comment in a suspend_noirq
> callback to say "let's runtime-suspend the device".  That's a 
> contradiction in terms.  The right way to think about it is more like 
> this: "If the device isn't already in a low-power state, let's put it 
> in a low-power state now".
> 
> In your case, the device already is (or should be) at low power.  A
> better way to phrase this might be: "If the device's power hasn't
> already been turned off, and the device doesn't need to generate wakeup
> requests, turn off the power now".
> 
> Alan Stern
> 
> 

Ok, thanks for input! I will clarify my comments and commit message and 
send a v3 patch (although the heading of the v3 patch will be changed to 
not including something with "runtime" suspend).

BR
Ulf Hansson