[PATCH V3] AMBA: Put devices into low-power state in suspend_noirq

[PATCH V3] AMBA: Put devices into low-power state in suspend_noirq

[Ulf Hansson]

Previously it was possible for device drivers doing
pm_runtime_suspend and pm_runtime_put_sync from their suspend
callbacks. From the following commit, which solved a race issue,
this is not possible anymore.

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

Therefore some devices might not be in full low-power state after
device's suspend callbacks has been executed. To make sure this is
done the suspend_noirq callback is used.

In the resume_noirq the power is restored to the device if it were
previously cut in suspend_noirq. This to make sure the device is
put back into the same state as the device driver left it in from
it's suspend callback.

If a device is configured as wakeup device this will prevent the
bus from putting it into low-power state during suspend_noirq.

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
---

Changes in v3:
	- Fixup comments and commit message (including the header).

Changes in v2:
	- Integrated code directly into suspend|resume_noirq and get rid
	of not needed ifdefs.
	- Prevent runtime suspend if device is configured as wakeup.

---
 drivers/amba/bus.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index bd230e8..e3ecf66 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -14,6 +14,7 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/pm.h>
+#include <linux/pm_wakeup.h>
 #include <linux/pm_runtime.h>
 #include <linux/amba/bus.h>
 
@@ -158,6 +159,7 @@ static int amba_pm_suspend(struct device *dev)
 static int amba_pm_suspend_noirq(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
+	bool is_suspended = pm_runtime_status_suspended(dev);
 	int ret = 0;
 
 	if (!drv)
@@ -168,6 +170,15 @@ static int amba_pm_suspend_noirq(struct device *dev)
 			ret = drv->pm->suspend_noirq(dev);
 	}
 
+	/*
+	 * If the device's power hasn't already been cut and the
+	 * device doesn't need to generate wakeup requests, cut
+	 * the power now.
+	 */
+	if (!ret && !is_suspended && !device_may_wakeup(dev))
+		if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend)
+			ret = dev->bus->pm->runtime_suspend(dev);
+
 	return ret;
 }
 
@@ -192,6 +203,7 @@ static int amba_pm_resume(struct device *dev)
 static int amba_pm_resume_noirq(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
+	bool is_suspended = pm_runtime_status_suspended(dev);
 	int ret = 0;
 
 	if (!drv)
@@ -202,6 +214,14 @@ static int amba_pm_resume_noirq(struct device *dev)
 			ret = drv->pm->resume_noirq(dev);
 	}
 
+	/*
+	 * If the device's power were cut during suspend_noirq
+	 * restore the power to the device now.
+	 */
+	if (!ret && !is_suspended && !device_may_wakeup(dev))
+		if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume)
+			ret = dev->bus->pm->runtime_resume(dev);
+
 	return ret;
 }
 
-- 
1.7.5.4

[PATCH V3] AMBA: Put devices into low-power state in suspend_noirq

[MyungJoo Ham]

On Tue, Nov 1, 2011 at 7:14 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> Previously it was possible for device drivers doing
> pm_runtime_suspend and pm_runtime_put_sync from their suspend
> callbacks. From the following commit, which solved a race issue,
> this is not possible anymore.
>
> PM: Limit race conditions between runtime PM and system sleep (v2)
>
> Therefore some devices might not be in full low-power state after
> device's suspend callbacks has been executed. To make sure this is
> done the suspend_noirq callback is used.
>
> In the resume_noirq the power is restored to the device if it were
> previously cut in suspend_noirq. This to make sure the device is
> put back into the same state as the device driver left it in from
> it's suspend callback.
>
> If a device is configured as wakeup device this will prevent the
> bus from putting it into low-power state during suspend_noirq.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
> ---
>
> Changes in v3:
> ? ? ? ?- Fixup comments and commit message (including the header).
>
> Changes in v2:
> ? ? ? ?- Integrated code directly into suspend|resume_noirq and get rid
> ? ? ? ?of not needed ifdefs.
> ? ? ? ?- Prevent runtime suspend if device is configured as wakeup.
>
> ---
> ?drivers/amba/bus.c | ? 20 ++++++++++++++++++++
> ?1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index bd230e8..e3ecf66 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -14,6 +14,7 @@
> ?#include <linux/slab.h>
> ?#include <linux/io.h>
> ?#include <linux/pm.h>
> +#include <linux/pm_wakeup.h>
> ?#include <linux/pm_runtime.h>
> ?#include <linux/amba/bus.h>
>
> @@ -158,6 +159,7 @@ static int amba_pm_suspend(struct device *dev)
> ?static int amba_pm_suspend_noirq(struct device *dev)
> ?{
> ? ? ? ?struct device_driver *drv = dev->driver;
> + ? ? ? bool is_suspended = pm_runtime_status_suspended(dev);
> ? ? ? ?int ret = 0;
>
> ? ? ? ?if (!drv)
> @@ -168,6 +170,15 @@ static int amba_pm_suspend_noirq(struct device *dev)
> ? ? ? ? ? ? ? ? ? ? ? ?ret = drv->pm->suspend_noirq(dev);
> ? ? ? ?}
>
> + ? ? ? /*
> + ? ? ? ?* If the device's power hasn't already been cut and the
> + ? ? ? ?* device doesn't need to generate wakeup requests, cut
> + ? ? ? ?* the power now.
> + ? ? ? ?*/
> + ? ? ? if (!ret && !is_suspended && !device_may_wakeup(dev))
> + ? ? ? ? ? ? ? if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend)
> + ? ? ? ? ? ? ? ? ? ? ? ret = dev->bus->pm->runtime_suspend(dev);
> +
> ? ? ? ?return ret;
> ?}

If a device of AMBA bus is still in "running" state, the bus will
simply need to call pm->suspend() / pm->suspend_noirq(). And the
suspend/resume callbacks of the devices should be able to handle the
situation. In other words, the device driver's suspend() and
suspend_noirq() callbacks should be able to handle the transition from
running to suspended regardless whether it is already in "suspended"
(by runtime-pm) or in "running".

Yes, even if we cannot call pm_runtime_get/put in pm's suspend/resume
callbacks, that is same.

If the contents of runtime_pm_suspend() and suspend() callbacks are
the same, we only need to make another static function and use it at
the both callbacks at the device driver.

Things could be a bit different for devices whether it is being
suspended in runtime or in system-wide suspend, and the devices should
be able to distinguish them. (so, call suspend callback if it is
suspend, call runtime_pm_suspend callback if it is being suspended in
runtime.)



If the reason of this patch is for device_may_wakeup condition where
AMBA needs to keep something up if any of its devices may wakeup, I
guess you need another approach. For example, checking
devices_may_wakeup conditions of all devices at ABMA suspend callback,
save the information at AMBA's data, use the saved information, set
the ABMA bus device with AMBA's suspend_noirq callback based on the
saved information or at syscore.


Cheers!
MyungJoo

>
> @@ -192,6 +203,7 @@ static int amba_pm_resume(struct device *dev)
> ?static int amba_pm_resume_noirq(struct device *dev)
> ?{
> ? ? ? ?struct device_driver *drv = dev->driver;
> + ? ? ? bool is_suspended = pm_runtime_status_suspended(dev);
> ? ? ? ?int ret = 0;
>
> ? ? ? ?if (!drv)
> @@ -202,6 +214,14 @@ static int amba_pm_resume_noirq(struct device *dev)
> ? ? ? ? ? ? ? ? ? ? ? ?ret = drv->pm->resume_noirq(dev);
> ? ? ? ?}
>
> + ? ? ? /*
> + ? ? ? ?* If the device's power were cut during suspend_noirq
> + ? ? ? ?* restore the power to the device now.
> + ? ? ? ?*/
> + ? ? ? if (!ret && !is_suspended && !device_may_wakeup(dev))
> + ? ? ? ? ? ? ? if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume)
> + ? ? ? ? ? ? ? ? ? ? ? ret = dev->bus->pm->runtime_resume(dev);
> +
> ? ? ? ?return ret;
> ?}
>
> --
> 1.7.5.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics

[PATCH V3] AMBA: Put devices into low-power state in suspend_noirq

[Russell King - ARM Linux]

On Wed, Nov 02, 2011 at 07:33:03PM +0900, MyungJoo Ham wrote:
> If the reason of this patch is for device_may_wakeup condition where
> AMBA needs to keep something up if any of its devices may wakeup, I
> guess you need another approach. For example, checking
> devices_may_wakeup conditions of all devices at ABMA suspend callback,
> save the information at AMBA's data, use the saved information, set
> the ABMA bus device with AMBA's suspend_noirq callback based on the
> saved information or at syscore.

I think this patch is trying to address the variability in the state of
the bus clock when a device is suspended.

Originally, we didn't do any bus clock management, so all the calls into
the driver were assured that the bus clock would always be running.

Then, when bus clock management was added, we ensured that it was enabled
before the driver was probed, and disabled after the driver has been
removed.  A driver wishing to manage the bus clock for its device must
then manage it appropriately given those pre-conditions (which means if
it turns off the bus clock before probe returns, it has to be prepared
to turn the bus clock back on when the suspend or remove callbacks are
made.)

When runtime-PM was added to the bus layer, this took control of the
bus clock management, but we've kept the same guarantees - although
runtime PM will be enabled, it is up to the driver to drop the use
count to make it runtime-suspend the device - and if that is done it is
up to the driver to make whatever runtime PM calls are necessary to
bring the bus clock back on when required.

I don't see any reason drivers can't continue to use pm_runtime_get_sync()
in their suspend callback if they're doing runtime-PM - the runtime PM
stuff hasn't been disabled at this point and so the runtime resume should
still happen.

What won't happen is that you won't be able to use runtime PM to turn the
bus clock off - but that's also the case for a non-runtime PM driver as
well.  The solution to this is to -at the end- of the suspend callback
to call amba_pclk_disable(), and -at the beginning- of the resume callback
to call amba_pclk_enable(), both provided that the device can cope with it.
At the end of the resume callback, call pm_runtime_put() to balance the
pm_runtime_get_sync() in the suspend callback.

So, I don't see any bus-layer patches are necessary or desirable.  Trying
to fix this at the bus layer changes the entry conditions for the suspend
and resume callbacks, and makes it _much_ harder for drivers to work out
what state things are in at that point - which means we're creating a
recipe for bugs.

[PATCH V3] AMBA: Put devices into low-power state in suspend_noirq

[Ulf Hansson]

Russell King - ARM Linux wrote:
> On Wed, Nov 02, 2011 at 07:33:03PM +0900, MyungJoo Ham wrote:
>> If the reason of this patch is for device_may_wakeup condition where
>> AMBA needs to keep something up if any of its devices may wakeup, I
>> guess you need another approach. For example, checking
>> devices_may_wakeup conditions of all devices at ABMA suspend callback,
>> save the information at AMBA's data, use the saved information, set
>> the ABMA bus device with AMBA's suspend_noirq callback based on the
>> saved information or at syscore.
> 
> I think this patch is trying to address the variability in the state of
> the bus clock when a device is suspended.
> 
> Originally, we didn't do any bus clock management, so all the calls into
> the driver were assured that the bus clock would always be running.
> 
> Then, when bus clock management was added, we ensured that it was enabled
> before the driver was probed, and disabled after the driver has been
> removed.  A driver wishing to manage the bus clock for its device must
> then manage it appropriately given those pre-conditions (which means if
> it turns off the bus clock before probe returns, it has to be prepared
> to turn the bus clock back on when the suspend or remove callbacks are
> made.)
> 
> When runtime-PM was added to the bus layer, this took control of the
> bus clock management, but we've kept the same guarantees - although
> runtime PM will be enabled, it is up to the driver to drop the use
> count to make it runtime-suspend the device - and if that is done it is
> up to the driver to make whatever runtime PM calls are necessary to
> bring the bus clock back on when required.
> 
> I don't see any reason drivers can't continue to use pm_runtime_get_sync()
> in their suspend callback if they're doing runtime-PM - the runtime PM
> stuff hasn't been disabled at this point and so the runtime resume should
> still happen.
> 
> What won't happen is that you won't be able to use runtime PM to turn the
> bus clock off - but that's also the case for a non-runtime PM driver as
> well.  The solution to this is to -at the end- of the suspend callback
> to call amba_pclk_disable(), and -at the beginning- of the resume callback
> to call amba_pclk_enable(), both provided that the device can cope with it.
> At the end of the resume callback, call pm_runtime_put() to balance the
> pm_runtime_get_sync() in the suspend callback.

Just some thinking, you may comment if you like....

When using a power_domain this will then mean that the power domain 
callbacks for suspend|resume should take similar actions as they do in 
their runtime_suspend|resume callbacks to put the devices into low power 
state. This should be in line of what you prefer?

Alright, let's skip this patch and make sure each device driver handles 
the things from their suspend|suspend_noirq instead.

This will mean some duplicated code but at the same time give the device 
driver full control and responsibility of putting its device in 
low-power state.

In parallel it would then also make sense to remove the pm_runtime 
handling completely from the amba bus and let the pclk be fully runtime 
handled from the device driver instead. Since it must handle it from 
suspend anyway...

> 
> So, I don't see any bus-layer patches are necessary or desirable.  Trying
> to fix this at the bus layer changes the entry conditions for the suspend
> and resume callbacks, and makes it _much_ harder for drivers to work out
> what state things are in at that point - which means we're creating a
> recipe for bugs.
> 

[PATCH V3] AMBA: Put devices into low-power state in suspend_noirq

[Russell King - ARM Linux]

On Wed, Nov 02, 2011 at 02:50:51PM +0100, Ulf Hansson wrote:
> Just some thinking, you may comment if you like....
>
> When using a power_domain this will then mean that the power domain  
> callbacks for suspend|resume should take similar actions as they do in  
> their runtime_suspend|resume callbacks to put the devices into low power  
> state. This should be in line of what you prefer?

If it is appropriate to cut the vcore for the device when in runtime
suspend state, then a driver itself has to explicitly do that.  vcore
handling is different from pclk handling because loss of vcore requires
the device to be reinitialized, whereas pclk does not.

> This will mean some duplicated code but at the same time give the device  
> driver full control and responsibility of putting its device in  
> low-power state.

It should not mean any duplicated code - if a driver itself wishes to call
its runtime suspend and resume handlers from its normal suspend/resume
handlers, that's a choice it can make (and it alone can make).  Again,
take a moment to think a little more about your original proposal of
pushing that into the bus layer.

And consider the case of a non-runtime PM system (where the runtime PM
callbacks are NULL) yet you have system suspend enabled, and you trigger
a system suspend.  Do you really want the suspend of the devices to be
skipped because the runtime PM callbacks were NULL because of system
configuration?

With them being explicit calls in the driver, this doesn't happen because
if you omit those functions for non-runtime PM, you'll get a link error.

> In parallel it would then also make sense to remove the pm_runtime  
> handling completely from the amba bus and let the pclk be fully runtime  
> handled from the device driver instead. Since it must handle it from  
> suspend anyway...

We know that it's safe to handle the pclk at the bus layer for normal
runtime PM as it causes no state loss.  We also know that throwing the
vcore handling in there is the wrong thing to do because it does cause
state loss and therefore needs proper handling at the driver level.

Hence, the bus code can take care of the pclk but not for vcore.

So, I don't see any reason to change the bus code.

[PATCH V3] AMBA: Put devices into low-power state in suspend_noirq

[Rafael J. Wysocki]

On Wednesday, November 02, 2011, Russell King - ARM Linux wrote:
> On Wed, Nov 02, 2011 at 02:50:51PM +0100, Ulf Hansson wrote:
> > Just some thinking, you may comment if you like....
> >
> > When using a power_domain this will then mean that the power domain  
> > callbacks for suspend|resume should take similar actions as they do in  
> > their runtime_suspend|resume callbacks to put the devices into low power  
> > state. This should be in line of what you prefer?
> 
> If it is appropriate to cut the vcore for the device when in runtime
> suspend state, then a driver itself has to explicitly do that.  vcore
> handling is different from pclk handling because loss of vcore requires
> the device to be reinitialized, whereas pclk does not.

Well, I have a comment here.

There are systems having separate logic for removing power from power
domains, so individual drivers obviously can't do that.  What they
can do is to indicate that their devices don't need power at the moment,
which is done by using the runtime PM reference counting, and the
subsystem layer has to take care of the actual power removal.

> > This will mean some duplicated code but at the same time give the device  
> > driver full control and responsibility of putting its device in  
> > low-power state.
> 
> It should not mean any duplicated code - if a driver itself wishes to call
> its runtime suspend and resume handlers from its normal suspend/resume
> handlers, that's a choice it can make (and it alone can make).  Again,
> take a moment to think a little more about your original proposal of
> pushing that into the bus layer.
> 
> And consider the case of a non-runtime PM system (where the runtime PM
> callbacks are NULL) yet you have system suspend enabled, and you trigger
> a system suspend.  Do you really want the suspend of the devices to be
> skipped because the runtime PM callbacks were NULL because of system
> configuration?
> 
> With them being explicit calls in the driver, this doesn't happen because
> if you omit those functions for non-runtime PM, you'll get a link error.
> 
> > In parallel it would then also make sense to remove the pm_runtime  
> > handling completely from the amba bus and let the pclk be fully runtime  
> > handled from the device driver instead. Since it must handle it from  
> > suspend anyway...
> 
> We know that it's safe to handle the pclk at the bus layer for normal
> runtime PM as it causes no state loss.  We also know that throwing the
> vcore handling in there is the wrong thing to do because it does cause
> state loss and therefore needs proper handling at the driver level.
> 
> Hence, the bus code can take care of the pclk but not for vcore.
> 
> So, I don't see any reason to change the bus code.

I agree.

At least calling a driver's .runtime_resume() callback from the bus type's
_resume_noirq() one doesn't seem quite right (and analogously for suspend).
It generally is expected that drivers will provide their own .suspend_noirq()
and .resume_noirq() callbacks, which may point to the same callback routines
as .runtime_suspend() and .runtime_resume(), respectively, in some cases.

It _may_ lead to some code duplication in some cases, but that may be avoided
by using the struct dev_pm_domain pointer in struct device.  In some cases.

Thanks,
Rafael

[PATCH V3] AMBA: Put devices into low-power state in suspend_noirq

[Ulf Hansson]

Rafael J. Wysocki wrote:
> On Wednesday, November 02, 2011, Russell King - ARM Linux wrote:
>> On Wed, Nov 02, 2011 at 02:50:51PM +0100, Ulf Hansson wrote:
>>> Just some thinking, you may comment if you like....
>>>
>>> When using a power_domain this will then mean that the power domain  
>>> callbacks for suspend|resume should take similar actions as they do in  
>>> their runtime_suspend|resume callbacks to put the devices into low power  
>>> state. This should be in line of what you prefer?
>> If it is appropriate to cut the vcore for the device when in runtime
>> suspend state, then a driver itself has to explicitly do that.  vcore
>> handling is different from pclk handling because loss of vcore requires
>> the device to be reinitialized, whereas pclk does not.
> 
> Well, I have a comment here.
> 
> There are systems having separate logic for removing power from power
> domains, so individual drivers obviously can't do that.  What they
> can do is to indicate that their devices don't need power at the moment,
> which is done by using the runtime PM reference counting, and the
> subsystem layer has to take care of the actual power removal.
> 
>>> This will mean some duplicated code but at the same time give the device  
>>> driver full control and responsibility of putting its device in  
>>> low-power state.
>> It should not mean any duplicated code - if a driver itself wishes to call
>> its runtime suspend and resume handlers from its normal suspend/resume
>> handlers, that's a choice it can make (and it alone can make).  Again,
>> take a moment to think a little more about your original proposal of
>> pushing that into the bus layer.
>>
>> And consider the case of a non-runtime PM system (where the runtime PM
>> callbacks are NULL) yet you have system suspend enabled, and you trigger
>> a system suspend.  Do you really want the suspend of the devices to be
>> skipped because the runtime PM callbacks were NULL because of system
>> configuration?

This is a fact and is as I see it the main reason to why not this patch 
should not be accepted.

>>
>> With them being explicit calls in the driver, this doesn't happen because
>> if you omit those functions for non-runtime PM, you'll get a link error.
>>
>>> In parallel it would then also make sense to remove the pm_runtime  
>>> handling completely from the amba bus and let the pclk be fully runtime  
>>> handled from the device driver instead. Since it must handle it from  
>>> suspend anyway...
>> We know that it's safe to handle the pclk at the bus layer for normal
>> runtime PM as it causes no state loss.  We also know that throwing the
>> vcore handling in there is the wrong thing to do because it does cause
>> state loss and therefore needs proper handling at the driver level.

The main reason for putting this into the bus was to remove the handling 
of pclk from the device drivers since it was common for all drivers. Due 
to the above discussion it will be there anyway, but for 
suspend|suspend_noirq instead.

In my perfect world :-), either the bus controls the pclk completely or 
not, to have something in between just does not feel right.

So either I suggest to remove the runtime handling of the pclk and then 
move the responsibility to the drivers (I think it will be somewhat 
cleaner code in each device driver if this is done) or add handling of 
the pclk to the amba normal suspend|resume functions as well, that could 
be feasible as well.

One additional thought, considering using a power domain with runtime 
functions. Ideally the power domain will do it's runtime stuff and then 
would like to call the pm_generic_runtime_* functions which right now is 
going directly towards the driver and skipping the bus etc. This might 
be another discussion though, but never the less, it could be a reason 
to why not have runtime functions at all in the bus.

>>
>> Hence, the bus code can take care of the pclk but not for vcore.
>>
>> So, I don't see any reason to change the bus code.
> 
> I agree.
> 
> At least calling a driver's .runtime_resume() callback from the bus type's
> _resume_noirq() one doesn't seem quite right (and analogously for suspend).
> It generally is expected that drivers will provide their own .suspend_noirq()
> and .resume_noirq() callbacks, which may point to the same callback routines
> as .runtime_suspend() and .runtime_resume(), respectively, in some cases.
> 
> It _may_ lead to some code duplication in some cases, but that may be avoided
> by using the struct dev_pm_domain pointer in struct device.  In some cases.
> 
> Thanks,
> Rafael
>