AMBA: Use suspend_noriq to force devices into runtime suspend

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)

---
 drivers/amba/bus.c |  113 ++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 79 insertions(+), 34 deletions(-)

index bd230e8..7ba523a 100644
@@ -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

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.

2011/10/27 Russell King - ARM Linux <[email protected]>:

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

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?

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.

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.

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

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.

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

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.

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.

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

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.

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.

Correct.

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.

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.

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

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 ...
}

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

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.

I have a small request for this patch:

...

...

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

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...

BR
Ulf Hansson

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