Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
On Wednesday 31 October 2012 04:07 PM, Kevin Hilman wrote: Santosh Shilimkar writes: [...] Just to summaries, there are 3 things we are talking here. Santosh, thanks for the summary. You are right on. 1. Delaying the idle with a timeout which $subject patch is trying to do to reduce latency for interrupts. That itself is reasonable. I agree, this is reasonable. IMO, adding autosuspend should be done as an isolated patch and kept separate from any other cleanup or changes. Also, as Jon mentioned, I think this should be done with a default timeout of zero so that current behaviour is unchanged. Tim can then set the timeout from userspace for his usecase and everyone is happy. Default 0 sounds good to me as well. 2. Removal of the bank "mod_usage" which is also clubbed in $subject path. Ofcourse that break the current driver for idle. So that change needs to be made with better thought and in a separate patch. This is doable. I had this discussion with Charu/Tarun during the last round of cleanup because I didn't like this mod_usage either. The alternative is to do a 'get' for every GPIO in the bank since runtime PM is doing the usecounting already. As Santosh said, this is fine for suspend, but for idle it means calling 'put' for every enabled GPIO in the bank instead of just forcing things with a single 'put.' Exactly. The oherhead becomes too much when if you need to call put like 6 Banks * 32 GPIOS times. Actually even though there are 32 GPIOs in a bank, from PM point of view, it only needs to care about a bank level control. Because clocks and register set is per bank. And we model a GPIO device per bank as well. 3. Removing omap_gpio_[prepare/resume]_for_idle() with soome thing better. For this one though, so far I have not come across a good solution. Ideas/Solution is welcome !! I agree that the prepare/resume idle hooks a are ugly and should be removed, but this needs quite a bit more thought. In particular, the 'remove triggering' stuff that is done needs pretty close examination. I'm not saying it can't be done, but patches to do this should be separate, well described and well tested, especially with off-mode. Couldn't agree more. Thanks for confirming the points. Regards santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Santosh Shilimkar writes: [...] > Just to summaries, there are 3 things we are talking here. Santosh, thanks for the summary. You are right on. > 1. Delaying the idle with a timeout which $subject patch is > trying to do to reduce latency for interrupts. That itself > is reasonable. I agree, this is reasonable. IMO, adding autosuspend should be done as an isolated patch and kept separate from any other cleanup or changes. Also, as Jon mentioned, I think this should be done with a default timeout of zero so that current behaviour is unchanged. Tim can then set the timeout from userspace for his usecase and everyone is happy. > 2. Removal of the bank "mod_usage" which is also clubbed > in $subject path. Ofcourse that break the current driver > for idle. So that change needs to be made with better thought > and in a separate patch. This is doable. I had this discussion with Charu/Tarun during the last round of cleanup because I didn't like this mod_usage either. The alternative is to do a 'get' for every GPIO in the bank since runtime PM is doing the usecounting already. As Santosh said, this is fine for suspend, but for idle it means calling 'put' for every enabled GPIO in the bank instead of just forcing things with a single 'put.' > 3. Removing omap_gpio_[prepare/resume]_for_idle() with soome thing > better. For this one though, so far I have not come across a good > solution. Ideas/Solution is welcome !! I agree that the prepare/resume idle hooks a are ugly and should be removed, but this needs quite a bit more thought. In particular, the 'remove triggering' stuff that is done needs pretty close examination. I'm not saying it can't be done, but patches to do this should be separate, well described and well tested, especially with off-mode. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Hi, On Wed, Oct 31, 2012 at 05:15:02AM -0500, Jon Hunter wrote: > its bit of an issue to take care. How do you ensure that GPIO > does idle on SOC idle C-state attempts in such cases. Today that > job is done by omap_gpio_[prepare/resume]_for_idle. > >>> > >>> that's only there because we pm_runtime_get_sync() on gpio_request() and > >>> pm_runtime_put_sync() only on gpio_free(). > >>> > >>> That's the problem IMHO. And that's easy enough to 'fix': > >>> > >>> call pm_runtime_mark_last_busy(); pm_runtime_put_sync_autosuspend(); > >>> also on gpio_request() and pm_runtime_get_sync() on gpio_free(). > >> > >> Sounds like a good approach. I have been discussing with Kevin and I > >> need to look more at the resume handler as we are working around some > >> old issues in there and with this approach the resume following idle > >> will be delayed and we were not sure if there could be any implications > >> for omap2. I am hoping not, but we need to look into this. > >> > >> So I am wondering if we should just take Tim's original proposal for now > >> and then I will look into improving this long term. I really need to > >> clean-up the suspend/resume stuff for gpio and so may be we can make > >> that a separate change. What do you think? > > > > that'll cause a regression right ? > > Sorry, not sure I follow. $SUBJECT will prevent core idle. > >>> The difficult part, IMHO, is to figure out what's a good autosuspend > >>> timeout to use. Some GPIO lines are used as IRQ lines on some devices, > >>> that means that the GPIO will be periodically triggered and, depending > >>> on our timeout, we will either loose IRQs or prevent power domain from > >>> idling. We could figure out a way to let board code to choose what it > >>> wants on a per-bank basis (maybe some extra DT attribute). > >> > >> I have also been bending Kevin's ear on this, this week and we were > >> wondering if we should make the default 0 for now as this will have the > > > > I believe you mean -1 here ;-) > > I did mean 0, so that it will autosuspend right away. Basically, it will > behave like today, however, will allow people to change the timeout. I yeah, that's the -1 timeout, it disables pm timer and all pm_runtime_*_autosuspend() will act as pm_runtime_*(). > did not wish to make it -1 as then suspend/resume would not be exercised > and so people would need to change it via the sysfs to exercise deep > power states on the device. why won't be exercised ? > >> same behaviour that we have today but would allow Tim to customise via > >> the sysfs for his specific app. > > > > sysfs might be too late for his platform. What if he needs NFS root > > (just wondering, not sure about his use case) ? > > His use case was for SPI (see the original changelog). That's a good > point, but I am wondering if we can live with that for now. fair enough, but it's possible we will cause regressions. At least with current version of $SUBJECT, I guess. -- balbi signature.asc Description: Digital signature
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
On 10/30/2012 10:10 AM, Felipe Balbi wrote: > Hi, > > On Tue, Oct 30, 2012 at 09:16:34AM -0500, Jon Hunter wrote: >> Hi Felipe, >> >> On 10/30/2012 02:09 AM, Felipe Balbi wrote: >> >> ... >> its bit of an issue to take care. How do you ensure that GPIO does idle on SOC idle C-state attempts in such cases. Today that job is done by omap_gpio_[prepare/resume]_for_idle. >>> >>> that's only there because we pm_runtime_get_sync() on gpio_request() and >>> pm_runtime_put_sync() only on gpio_free(). >>> >>> That's the problem IMHO. And that's easy enough to 'fix': >>> >>> call pm_runtime_mark_last_busy(); pm_runtime_put_sync_autosuspend(); >>> also on gpio_request() and pm_runtime_get_sync() on gpio_free(). >> >> Sounds like a good approach. I have been discussing with Kevin and I >> need to look more at the resume handler as we are working around some >> old issues in there and with this approach the resume following idle >> will be delayed and we were not sure if there could be any implications >> for omap2. I am hoping not, but we need to look into this. >> >> So I am wondering if we should just take Tim's original proposal for now >> and then I will look into improving this long term. I really need to >> clean-up the suspend/resume stuff for gpio and so may be we can make >> that a separate change. What do you think? > > that'll cause a regression right ? Sorry, not sure I follow. >>> The difficult part, IMHO, is to figure out what's a good autosuspend >>> timeout to use. Some GPIO lines are used as IRQ lines on some devices, >>> that means that the GPIO will be periodically triggered and, depending >>> on our timeout, we will either loose IRQs or prevent power domain from >>> idling. We could figure out a way to let board code to choose what it >>> wants on a per-bank basis (maybe some extra DT attribute). >> >> I have also been bending Kevin's ear on this, this week and we were >> wondering if we should make the default 0 for now as this will have the > > I believe you mean -1 here ;-) I did mean 0, so that it will autosuspend right away. Basically, it will behave like today, however, will allow people to change the timeout. I did not wish to make it -1 as then suspend/resume would not be exercised and so people would need to change it via the sysfs to exercise deep power states on the device. >> same behaviour that we have today but would allow Tim to customise via >> the sysfs for his specific app. > > sysfs might be too late for his platform. What if he needs NFS root > (just wondering, not sure about his use case) ? His use case was for SPI (see the original changelog). That's a good point, but I am wondering if we can live with that for now. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Hi, On Tue, Oct 30, 2012 at 09:16:34AM -0500, Jon Hunter wrote: > Hi Felipe, > > On 10/30/2012 02:09 AM, Felipe Balbi wrote: > > ... > > >> its bit of an issue to take care. How do you ensure that GPIO > >> does idle on SOC idle C-state attempts in such cases. Today that > >> job is done by omap_gpio_[prepare/resume]_for_idle. > > > > that's only there because we pm_runtime_get_sync() on gpio_request() and > > pm_runtime_put_sync() only on gpio_free(). > > > > That's the problem IMHO. And that's easy enough to 'fix': > > > > call pm_runtime_mark_last_busy(); pm_runtime_put_sync_autosuspend(); > > also on gpio_request() and pm_runtime_get_sync() on gpio_free(). > > Sounds like a good approach. I have been discussing with Kevin and I > need to look more at the resume handler as we are working around some > old issues in there and with this approach the resume following idle > will be delayed and we were not sure if there could be any implications > for omap2. I am hoping not, but we need to look into this. > > So I am wondering if we should just take Tim's original proposal for now > and then I will look into improving this long term. I really need to > clean-up the suspend/resume stuff for gpio and so may be we can make > that a separate change. What do you think? that'll cause a regression right ? > > The difficult part, IMHO, is to figure out what's a good autosuspend > > timeout to use. Some GPIO lines are used as IRQ lines on some devices, > > that means that the GPIO will be periodically triggered and, depending > > on our timeout, we will either loose IRQs or prevent power domain from > > idling. We could figure out a way to let board code to choose what it > > wants on a per-bank basis (maybe some extra DT attribute). > > I have also been bending Kevin's ear on this, this week and we were > wondering if we should make the default 0 for now as this will have the I believe you mean -1 here ;-) > same behaviour that we have today but would allow Tim to customise via > the sysfs for his specific app. sysfs might be too late for his platform. What if he needs NFS root (just wondering, not sure about his use case) ? -- balbi signature.asc Description: Digital signature
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Hi Felipe, On 10/30/2012 02:09 AM, Felipe Balbi wrote: ... >> its bit of an issue to take care. How do you ensure that GPIO >> does idle on SOC idle C-state attempts in such cases. Today that >> job is done by omap_gpio_[prepare/resume]_for_idle. > > that's only there because we pm_runtime_get_sync() on gpio_request() and > pm_runtime_put_sync() only on gpio_free(). > > That's the problem IMHO. And that's easy enough to 'fix': > > call pm_runtime_mark_last_busy(); pm_runtime_put_sync_autosuspend(); > also on gpio_request() and pm_runtime_get_sync() on gpio_free(). Sounds like a good approach. I have been discussing with Kevin and I need to look more at the resume handler as we are working around some old issues in there and with this approach the resume following idle will be delayed and we were not sure if there could be any implications for omap2. I am hoping not, but we need to look into this. So I am wondering if we should just take Tim's original proposal for now and then I will look into improving this long term. I really need to clean-up the suspend/resume stuff for gpio and so may be we can make that a separate change. What do you think? ... > The difficult part, IMHO, is to figure out what's a good autosuspend > timeout to use. Some GPIO lines are used as IRQ lines on some devices, > that means that the GPIO will be periodically triggered and, depending > on our timeout, we will either loose IRQs or prevent power domain from > idling. We could figure out a way to let board code to choose what it > wants on a per-bank basis (maybe some extra DT attribute). I have also been bending Kevin's ear on this, this week and we were wondering if we should make the default 0 for now as this will have the same behaviour that we have today but would allow Tim to customise via the sysfs for his specific app. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Hi, On Tue, Oct 30, 2012 at 12:02:17PM +0530, Santosh Shilimkar wrote: > >>>I still think mod_usage needs to go, so does > >>>omap_gpio_prepare_for_idle() and omap_gpio_resume_from_idle(). To me, it > >>>looks like that needs to be done on ->prepare()/->complete() callbacks > >>>of system suspend and the gpio driver needs to learn proper runtime > >>>suspend. > >>> > >>I am not saying it shouldn't go :-) > >>The $subject patch isn't fixing it correctly is what I said. > >> > >>Don't get hung up on suspend case because thats the easiest > >>way to address it. The issue is with idle where GPIO can prevent > >>SOC idle if it isn't taken care. And since its just an IO, its not > >>easy to implement something like inactivity timer towards > >>autosupend. > > > >I don't see the relation here. Care to expand a bit ? > > > Let me try again... > > Before I answer you, a closer look at code, the multiple bank should > not be a problem since we create PIO device per bank so the runtime > PM layer can indeed manage the use-counting. The issue is with SOC > idle. > > System wide suspend, we suspend all the devices and hence all the > GPIO devices(banks) will be suspended letting the SOC to hit suspend > state. Hence suspend case is no problem as such. > > Devices like UART/USB can implement inactivity timers since they > do have state machine and after the timeout, those devices can be > suspended. GPIO doesn't fall exactly in that category and hence idling the device has nothing to do with their internal state machines, though :-) We just assume that if device hasn't been used for X ms, it's unlikely it will continue to be used. Likewise, If device has been used Y ms ago, it's likely it will continue to be used. > its bit of an issue to take care. How do you ensure that GPIO > does idle on SOC idle C-state attempts in such cases. Today that > job is done by omap_gpio_[prepare/resume]_for_idle. that's only there because we pm_runtime_get_sync() on gpio_request() and pm_runtime_put_sync() only on gpio_free(). That's the problem IMHO. And that's easy enough to 'fix': call pm_runtime_mark_last_busy(); pm_runtime_put_sync_autosuspend(); also on gpio_request() and pm_runtime_get_sync() on gpio_free(). > Idle if we had generic idle notifiers, that would been easy > but it doesn't exist today. > > In case we get rid of "mod_usage" which is doable, we need to > see how to handle the idle since the runtime callback will be > in that case controlled by runtime PM layer which is unaware > of idle. You can do put on all banks to trigger callback but I think we might have a clash of concepts here. What are you calling idle ? We _do_ have ->runtime_idle() callback, are we not using that ? I can see omap_device.c implements it properly and I can see that ->runtime_idle() will be called whenever pm_usage counter is decremented but is not zero. Looks like we can turn that into something useful. > that will highly UN-optimal. See summary in the end. > > >>Co-processor also makes use of GPIO via syslink proxy and thats > >>make things even harder. > > > >that's supposed to be solved with hwspinlock, isn't it ? > > > Spin locks are needed when same devices are shared across. That > is not the main concern. The PM is centralized on Linux side for > GPIO where as the users are outside Linux SW. They may use of > syslink proxy to request/free GPIO. fair enough, that's even better. > Just to summaries, there are 3 things we are talking here. > > 1. Delaying the idle with a timeout which $subject patch is > trying to do to reduce latency for interrupts. That itself > is reasonable. > > 2. Removal of the bank "mod_usage" which is also clubbed > in $subject path. Ofcourse that break the current driver > for idle. So that change needs to be made with better thought > and in a separate patch. This is doable. > > 3. Removing omap_gpio_[prepare/resume]_for_idle() with soome > thing better. For this one though, so far I have not come > across a good solution. Ideas/Solution is welcome !! make pm_runtime a bit more aggressive. You don't need to keep GPIO bank's clocks on just because a GPIO in that bank is requested. If context save & restore isn't buggy, you can disable clocks no problem. The difficult part, IMHO, is to figure out what's a good autosuspend timeout to use. Some GPIO lines are used as IRQ lines on some devices, that means that the GPIO will be periodically triggered and, depending on our timeout, we will either loose IRQs or prevent power domain from idling. We could figure out a way to let board code to choose what it wants on a per-bank basis (maybe some extra DT attribute). -- balbi signature.asc Description: Digital signature
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
On Tuesday 30 October 2012 01:33 AM, Felipe Balbi wrote: Hi, On Mon, Oct 29, 2012 at 01:53:37PM +0530, Santosh Shilimkar wrote: Just to expand a bit, Out of 6 GPIO banks, GPIO1 bank is in always ON domain where as remaing 5 are in peripheral domain. Letting individual banks idle allowed you let the clock domain idle than keeping all the 6 banks and hence respective clock/power domain in ON state. So the adding timeout might be reasonable but I am not sure about the mod_usage change here. IMHO that whole mod_usage is broken. I remember sending a big series of patches getting rid of that long ago. I _did_ break a few things but just because of omap_gpio_prepare_for_idle() / omap_gpio_resume_from_idle() hackery to get GPIO suspended early enough. Well so far I haven't seen/come across a patch/proposal which fixes it. fair point I still think mod_usage needs to go, so does omap_gpio_prepare_for_idle() and omap_gpio_resume_from_idle(). To me, it looks like that needs to be done on ->prepare()/->complete() callbacks of system suspend and the gpio driver needs to learn proper runtime suspend. I am not saying it shouldn't go :-) The $subject patch isn't fixing it correctly is what I said. Don't get hung up on suspend case because thats the easiest way to address it. The issue is with idle where GPIO can prevent SOC idle if it isn't taken care. And since its just an IO, its not easy to implement something like inactivity timer towards autosupend. I don't see the relation here. Care to expand a bit ? Let me try again... Before I answer you, a closer look at code, the multiple bank should not be a problem since we create PIO device per bank so the runtime PM layer can indeed manage the use-counting. The issue is with SOC idle. System wide suspend, we suspend all the devices and hence all the GPIO devices(banks) will be suspended letting the SOC to hit suspend state. Hence suspend case is no problem as such. Devices like UART/USB can implement inactivity timers since they do have state machine and after the timeout, those devices can be suspended. GPIO doesn't fall exactly in that category and hence its bit of an issue to take care. How do you ensure that GPIO does idle on SOC idle C-state attempts in such cases. Today that job is done by omap_gpio_[prepare/resume]_for_idle. Idle if we had generic idle notifiers, that would been easy but it doesn't exist today. In case we get rid of "mod_usage" which is doable, we need to see how to handle the idle since the runtime callback will be in that case controlled by runtime PM layer which is unaware of idle. You can do put on all banks to trigger callback but that will highly UN-optimal. See summary in the end. Co-processor also makes use of GPIO via syslink proxy and thats make things even harder. that's supposed to be solved with hwspinlock, isn't it ? Spin locks are needed when same devices are shared across. That is not the main concern. The PM is centralized on Linux side for GPIO where as the users are outside Linux SW. They may use of syslink proxy to request/free GPIO. Just to summaries, there are 3 things we are talking here. 1. Delaying the idle with a timeout which $subject patch is trying to do to reduce latency for interrupts. That itself is reasonable. 2. Removal of the bank "mod_usage" which is also clubbed in $subject path. Ofcourse that break the current driver for idle. So that change needs to be made with better thought and in a separate patch. This is doable. 3. Removing omap_gpio_[prepare/resume]_for_idle() with soome thing better. For this one though, so far I have not come across a good solution. Ideas/Solution is welcome !! Regards Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Hi, On Mon, Oct 29, 2012 at 01:53:37PM +0530, Santosh Shilimkar wrote: > >>Just to expand a bit, Out of 6 GPIO banks, GPIO1 bank is in always ON > >>domain where as remaing 5 are in peripheral domain. Letting individual > >>banks idle allowed you let the clock domain idle than keeping all the > >>6 banks and hence respective clock/power domain in ON state. > >> > >>So the adding timeout might be reasonable but I am not sure about > >>the mod_usage change here. > > > >IMHO that whole mod_usage is broken. I remember sending a big series of > >patches getting rid of that long ago. I _did_ break a few things but > >just because of omap_gpio_prepare_for_idle() / > >omap_gpio_resume_from_idle() hackery to get GPIO suspended early enough. > > > Well so far I haven't seen/come across a patch/proposal which fixes it. fair point > >I still think mod_usage needs to go, so does > >omap_gpio_prepare_for_idle() and omap_gpio_resume_from_idle(). To me, it > >looks like that needs to be done on ->prepare()/->complete() callbacks > >of system suspend and the gpio driver needs to learn proper runtime > >suspend. > > > I am not saying it shouldn't go :-) > The $subject patch isn't fixing it correctly is what I said. > > Don't get hung up on suspend case because thats the easiest > way to address it. The issue is with idle where GPIO can prevent > SOC idle if it isn't taken care. And since its just an IO, its not > easy to implement something like inactivity timer towards > autosupend. I don't see the relation here. Care to expand a bit ? > Co-processor also makes use of GPIO via syslink proxy and thats > make things even harder. that's supposed to be solved with hwspinlock, isn't it ? cheers -- balbi signature.asc Description: Digital signature
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Hello Am Freitag, den 26.10.2012, 23:01 +0300 schrieb Felipe Balbi: > Hi, > > On Fri, Oct 26, 2012 at 03:19:13PM +0200, Tim Niemeyer wrote: > > Adds support for configuring the omap-gpio driver use autosuspend for > > runtime power management. This can reduce the latency in using it by > > not suspending the device immediately on idle. If another access takes > > place before the autosuspend timeout (2 secs), the call to resume the > > device can return immediately saving some save/ restore cycles. > > > > This removes also the bank->mod_usage counter, because this is already > > handled in pm_runtime. > > > > I use a gpio to monitor a spi transfer which occurs every 250µs. The > > suspend overhead is to high, so almost every second transfer is lost. > > This patch fixes that. > > > > Signed-off-by: Tim Niemeyer > > --- > > drivers/gpio/gpio-omap.c | 81 > > - > > 1 files changed, 43 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > index 94cbc84..708d5a9 100644 > > --- a/drivers/gpio/gpio-omap.c > > +++ b/drivers/gpio/gpio-omap.c > > @@ -31,6 +31,7 @@ > > #include > > > > #define OFF_MODE 1 > > +#define GPIO_AUTOSUSPEND_TIMEOUT2000 > > something just hit me... If you keep timeout at 2000 ms and you hook > this up to an IRQ line, it's very unlikely GPIO will ever sleep. > > Why did you choose 2000 ms ? Arbitrary value ? Got it from the spi driver (27b5284cfbe187732ebb184b03ea693f44837f9d). I have no problem with reducing this. As i don't know how much power-overhead the suspend needs, what do you think is a good value in terms of power-saving? -- Tim Niemeyer Corscience GmbH & Co. KG Henkestr. 91 D-91052 Erlangen Germany e-mail: tim.nieme...@corscience.de Internet: www.corscience.de -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Am Montag, den 29.10.2012, 12:17 +0530 schrieb Santosh Shilimkar:
> + Jon,
>
> On Friday 26 October 2012 06:49 PM, Tim Niemeyer wrote:
> > Adds support for configuring the omap-gpio driver use autosuspend for
> > runtime power management. This can reduce the latency in using it by
> > not suspending the device immediately on idle. If another access takes
> > place before the autosuspend timeout (2 secs), the call to resume the
> > device can return immediately saving some save/ restore cycles.
> >
> > This removes also the bank->mod_usage counter, because this is already
> > handled in pm_runtime.
> >
> > I use a gpio to monitor a spi transfer which occurs every 250µs. The
> > suspend overhead is to high, so almost every second transfer is lost.
> > This patch fixes that.
> >
> > Signed-off-by: Tim Niemeyer
> > ---
> > drivers/gpio/gpio-omap.c | 81
> > -
> > 1 files changed, 43 insertions(+), 38 deletions(-)
> >
> From patch it appears your main motive is to delay the idle kicking in
> with a timeout to avoid GPIO on cpuidle path. Some comments
cpuidle? I set 'CPU_IDLE=n' in my .config..
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index 94cbc84..708d5a9 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -31,6 +31,7 @@
> > #include
> >
> > #define OFF_MODE 1
> > +#define GPIO_AUTOSUSPEND_TIMEOUT2000
> >
> > static LIST_HEAD(omap_gpio_list);
> >
> > @@ -64,7 +65,6 @@ struct gpio_bank {
> > spinlock_t lock;
> > struct gpio_chip chip;
> > struct clk *dbck;
> > - u32 mod_usage;
> How have you tested 'mod_suage' change ?
Nothing special, just applied the patch to a 3.4.14 kernel and used one
gpio to show the status of my spi communication.
> > u32 dbck_enable_mask;
> > bool dbck_enabled;
> > struct device *dev;
> > @@ -557,10 +557,9 @@ static int omap_gpio_request(struct gpio_chip *chip,
> > unsigned offset)
> >
> > /*
> > * If this is the first gpio_request for the bank,
> > -* enable the bank module.
> > +* resume the bank module.
> Since you removing bank notion, "If this is the first gpio_request
> for the bank," becomes irrelevant from code perspective.
Hu, i thought pm_runtime_get_sync(bank->dev) handles the use counting
per bank?
> > @@ -608,28 +594,15 @@ static void omap_gpio_free(struct gpio_chip *chip,
> > unsigned offset)
> > __raw_readl(bank->base + bank->regs->wkup_en);
> > }
> >
> > - bank->mod_usage &= ~(1 << offset);
> > -
> > - if (bank->regs->ctrl && !bank->mod_usage) {
> > - void __iomem *reg = bank->base + bank->regs->ctrl;
> > - u32 ctrl;
> > -
> > - ctrl = __raw_readl(reg);
> > - /* Module is disabled, clocks are gated */
> > - ctrl |= GPIO_MOD_CTRL_BIT;
> > - __raw_writel(ctrl, reg);
> > - bank->context.ctrl = ctrl;
> > - }
> > -
> > _reset_gpio(bank, bank->chip.base + offset);
> > spin_unlock_irqrestore(&bank->lock, flags);
> >
> > /*
> > * If this is the last gpio to be freed in the bank,
> > -* disable the bank module.
> > +* put the bank module into suspend.
> > */
> > - if (!bank->mod_usage)
> > - pm_runtime_put(bank->dev);
> > + pm_runtime_mark_last_busy(bank->dev);
> > + pm_runtime_put_autosuspend(bank->dev);
> Waiting for 2 seconds timeout even on GPIO free
> seems to be wrong.
Yes, you are right, if something frees the gpio it should suspend
immediately.
> > }
> >
> > /*
> > @@ -715,7 +688,8 @@ static void gpio_irq_handler(unsigned int irq, struct
> > irq_desc *desc)
> > exit:
> > if (!unmasked)
> > chained_irq_exit(chip, desc);
> > - pm_runtime_put(bank->dev);
> > + pm_runtime_mark_last_busy(bank->dev);
> > + pm_runtime_put_autosuspend(bank->dev);
> This is what is the main change from this patch which helps your
> case.
> > }
> >
> > static void gpio_irq_shutdown(struct irq_data *d)
> > @@ -1132,6 +1106,8 @@ static int __devinit omap_gpio_probe(struct
> > platform_device *pdev)
> >
> > platform_set_drvdata(pdev, bank);
> >
> > + pm_runtime_use_autosuspend(bank->dev);
> > + pm_runtime_set_autosuspend_delay(bank->dev, GPIO_AUTOSUSPEND_TIMEOUT);
>
> Can you please report how have you tested this change ? What other PM
> tests you have done?
My main goal isn't to have a good power-saving, i need a small latency.
The runtime-pm disturbed that. I tested to disable CONFIG_PM_RUNTIME,
but than the gptimer couldn't be initialized.
My scenario: gptimer triggers every 250µs and starts an async_spi
transfer while it sets one gpio line to high. The spi_complete then puts
this gpio to low again.
Every second interrupt was lost and i wanted to know why so i removed
the spi stuff and just turned a gpio on and off in the timer-isr. It
turned out that the system wasn't able to tick with 4kHz. I then
disabled the CONFIG_PM_R
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
On Monday 29 October 2012 01:35 PM, Felipe Balbi wrote:
On Mon, Oct 29, 2012 at 12:17:08PM +0530, Santosh Shilimkar wrote:
+ Jon,
On Friday 26 October 2012 06:49 PM, Tim Niemeyer wrote:
Adds support for configuring the omap-gpio driver use autosuspend for
runtime power management. This can reduce the latency in using it by
not suspending the device immediately on idle. If another access takes
place before the autosuspend timeout (2 secs), the call to resume the
device can return immediately saving some save/ restore cycles.
This removes also the bank->mod_usage counter, because this is already
handled in pm_runtime.
I use a gpio to monitor a spi transfer which occurs every 250µs. The
suspend overhead is to high, so almost every second transfer is lost.
This patch fixes that.
Signed-off-by: Tim Niemeyer
---
drivers/gpio/gpio-omap.c | 81 -
1 files changed, 43 insertions(+), 38 deletions(-)
From patch it appears your main motive is to delay the idle kicking in
with a timeout to avoid GPIO on cpuidle path. Some comments
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 94cbc84..708d5a9 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -31,6 +31,7 @@
#include
#define OFF_MODE 1
+#define GPIO_AUTOSUSPEND_TIMEOUT2000
static LIST_HEAD(omap_gpio_list);
@@ -64,7 +65,6 @@ struct gpio_bank {
spinlock_t lock;
struct gpio_chip chip;
struct clk *dbck;
- u32 mod_usage;
How have you tested 'mod_suage' change ?
u32 dbck_enable_mask;
bool dbck_enabled;
struct device *dev;
@@ -557,10 +557,9 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
/*
* If this is the first gpio_request for the bank,
-* enable the bank module.
+* resume the bank module.
Since you removing bank notion, "If this is the first gpio_request
for the bank," becomes irrelevant from code perspective.
[..]
@@ -608,28 +594,15 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
__raw_readl(bank->base + bank->regs->wkup_en);
}
- bank->mod_usage &= ~(1 << offset);
-
- if (bank->regs->ctrl && !bank->mod_usage) {
- void __iomem *reg = bank->base + bank->regs->ctrl;
- u32 ctrl;
-
- ctrl = __raw_readl(reg);
- /* Module is disabled, clocks are gated */
- ctrl |= GPIO_MOD_CTRL_BIT;
- __raw_writel(ctrl, reg);
- bank->context.ctrl = ctrl;
- }
-
_reset_gpio(bank, bank->chip.base + offset);
spin_unlock_irqrestore(&bank->lock, flags);
/*
* If this is the last gpio to be freed in the bank,
-* disable the bank module.
+* put the bank module into suspend.
*/
- if (!bank->mod_usage)
- pm_runtime_put(bank->dev);
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_autosuspend(bank->dev);
Waiting for 2 seconds timeout even on GPIO free
seems to be wrong.
}
/*
@@ -715,7 +688,8 @@ static void gpio_irq_handler(unsigned int irq, struct
irq_desc *desc)
exit:
if (!unmasked)
chained_irq_exit(chip, desc);
- pm_runtime_put(bank->dev);
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_autosuspend(bank->dev);
This is what is the main change from this patch which helps your
case.
}
static void gpio_irq_shutdown(struct irq_data *d)
@@ -1132,6 +1106,8 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
platform_set_drvdata(pdev, bank);
+ pm_runtime_use_autosuspend(bank->dev);
+ pm_runtime_set_autosuspend_delay(bank->dev, GPIO_AUTOSUSPEND_TIMEOUT);
Can you please report how have you tested this change ? What other PM
tests you have done?
Removing mod usage might just break this driver because now individual
banks which can idle before, can no longer idle.
why's that ?
Just to expand a bit, Out of 6 GPIO banks, GPIO1 bank is in always ON
domain where as remaing 5 are in peripheral domain. Letting individual
banks idle allowed you let the clock domain idle than keeping all the
6 banks and hence respective clock/power domain in ON state.
So the adding timeout might be reasonable but I am not sure about
the mod_usage change here.
IMHO that whole mod_usage is broken. I remember sending a big series of
patches getting rid of that long ago. I _did_ break a few things but
just because of omap_gpio_prepare_for_idle() /
omap_gpio_resume_from_idle() hackery to get GPIO suspended early enough.
Well so far I haven't seen/come across a patch/proposal which fixes it.
I still think mod_usage needs to go, so does
omap_gpio_prepare_for_idle() and omap_gpio_resume_from_idle(). To me, it
looks like that needs to be done on ->prepare()/->complete() callbacks
of system suspend and the gp
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
On Mon, Oct 29, 2012 at 12:17:08PM +0530, Santosh Shilimkar wrote:
> + Jon,
>
> On Friday 26 October 2012 06:49 PM, Tim Niemeyer wrote:
> >Adds support for configuring the omap-gpio driver use autosuspend for
> >runtime power management. This can reduce the latency in using it by
> >not suspending the device immediately on idle. If another access takes
> >place before the autosuspend timeout (2 secs), the call to resume the
> >device can return immediately saving some save/ restore cycles.
> >
> >This removes also the bank->mod_usage counter, because this is already
> >handled in pm_runtime.
> >
> >I use a gpio to monitor a spi transfer which occurs every 250µs. The
> >suspend overhead is to high, so almost every second transfer is lost.
> >This patch fixes that.
> >
> >Signed-off-by: Tim Niemeyer
> >---
> > drivers/gpio/gpio-omap.c | 81
> > -
> > 1 files changed, 43 insertions(+), 38 deletions(-)
> >
> From patch it appears your main motive is to delay the idle kicking in
> with a timeout to avoid GPIO on cpuidle path. Some comments
>
> >diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> >index 94cbc84..708d5a9 100644
> >--- a/drivers/gpio/gpio-omap.c
> >+++ b/drivers/gpio/gpio-omap.c
> >@@ -31,6 +31,7 @@
> > #include
> >
> > #define OFF_MODE 1
> >+#define GPIO_AUTOSUSPEND_TIMEOUT2000
> >
> > static LIST_HEAD(omap_gpio_list);
> >
> >@@ -64,7 +65,6 @@ struct gpio_bank {
> > spinlock_t lock;
> > struct gpio_chip chip;
> > struct clk *dbck;
> >-u32 mod_usage;
> How have you tested 'mod_suage' change ?
>
> > u32 dbck_enable_mask;
> > bool dbck_enabled;
> > struct device *dev;
> >@@ -557,10 +557,9 @@ static int omap_gpio_request(struct gpio_chip *chip,
> >unsigned offset)
> >
> > /*
> > * If this is the first gpio_request for the bank,
> >- * enable the bank module.
> >+ * resume the bank module.
> Since you removing bank notion, "If this is the first gpio_request
> for the bank," becomes irrelevant from code perspective.
>
> [..]
>
> >@@ -608,28 +594,15 @@ static void omap_gpio_free(struct gpio_chip *chip,
> >unsigned offset)
> > __raw_readl(bank->base + bank->regs->wkup_en);
> > }
> >
> >-bank->mod_usage &= ~(1 << offset);
> >-
> >-if (bank->regs->ctrl && !bank->mod_usage) {
> >-void __iomem *reg = bank->base + bank->regs->ctrl;
> >-u32 ctrl;
> >-
> >-ctrl = __raw_readl(reg);
> >-/* Module is disabled, clocks are gated */
> >-ctrl |= GPIO_MOD_CTRL_BIT;
> >-__raw_writel(ctrl, reg);
> >-bank->context.ctrl = ctrl;
> >-}
> >-
> > _reset_gpio(bank, bank->chip.base + offset);
> > spin_unlock_irqrestore(&bank->lock, flags);
> >
> > /*
> > * If this is the last gpio to be freed in the bank,
> >- * disable the bank module.
> >+ * put the bank module into suspend.
> > */
> >-if (!bank->mod_usage)
> >-pm_runtime_put(bank->dev);
> >+pm_runtime_mark_last_busy(bank->dev);
> >+pm_runtime_put_autosuspend(bank->dev);
> Waiting for 2 seconds timeout even on GPIO free
> seems to be wrong.
>
> > }
> >
> > /*
> >@@ -715,7 +688,8 @@ static void gpio_irq_handler(unsigned int irq, struct
> >irq_desc *desc)
> > exit:
> > if (!unmasked)
> > chained_irq_exit(chip, desc);
> >-pm_runtime_put(bank->dev);
> >+pm_runtime_mark_last_busy(bank->dev);
> >+pm_runtime_put_autosuspend(bank->dev);
> This is what is the main change from this patch which helps your
> case.
> > }
> >
> > static void gpio_irq_shutdown(struct irq_data *d)
> >@@ -1132,6 +1106,8 @@ static int __devinit omap_gpio_probe(struct
> >platform_device *pdev)
> >
> > platform_set_drvdata(pdev, bank);
> >
> >+pm_runtime_use_autosuspend(bank->dev);
> >+pm_runtime_set_autosuspend_delay(bank->dev, GPIO_AUTOSUSPEND_TIMEOUT);
>
> Can you please report how have you tested this change ? What other PM
> tests you have done?
>
> Removing mod usage might just break this driver because now individual
> banks which can idle before, can no longer idle.
why's that ?
> Just to expand a bit, Out of 6 GPIO banks, GPIO1 bank is in always ON
> domain where as remaing 5 are in peripheral domain. Letting individual
> banks idle allowed you let the clock domain idle than keeping all the
> 6 banks and hence respective clock/power domain in ON state.
>
> So the adding timeout might be reasonable but I am not sure about
> the mod_usage change here.
IMHO that whole mod_usage is broken. I remember sending a big series of
patches getting rid of that long ago. I _did_ break a few things but
just because of omap_gpio_prepare_for_idle() /
omap_gpio_resume_from_idle() hackery to get GPIO suspended early enough.
I still think mod_usage needs to go, so does
omap_gpio_prepare_for_idle() and omap_gpio_resume_from_idle(). To me, it
looks like that needs t
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
+ Jon,
On Friday 26 October 2012 06:49 PM, Tim Niemeyer wrote:
Adds support for configuring the omap-gpio driver use autosuspend for
runtime power management. This can reduce the latency in using it by
not suspending the device immediately on idle. If another access takes
place before the autosuspend timeout (2 secs), the call to resume the
device can return immediately saving some save/ restore cycles.
This removes also the bank->mod_usage counter, because this is already
handled in pm_runtime.
I use a gpio to monitor a spi transfer which occurs every 250µs. The
suspend overhead is to high, so almost every second transfer is lost.
This patch fixes that.
Signed-off-by: Tim Niemeyer
---
drivers/gpio/gpio-omap.c | 81 -
1 files changed, 43 insertions(+), 38 deletions(-)
From patch it appears your main motive is to delay the idle kicking in
with a timeout to avoid GPIO on cpuidle path. Some comments
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 94cbc84..708d5a9 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -31,6 +31,7 @@
#include
#define OFF_MODE 1
+#define GPIO_AUTOSUSPEND_TIMEOUT2000
static LIST_HEAD(omap_gpio_list);
@@ -64,7 +65,6 @@ struct gpio_bank {
spinlock_t lock;
struct gpio_chip chip;
struct clk *dbck;
- u32 mod_usage;
How have you tested 'mod_suage' change ?
u32 dbck_enable_mask;
bool dbck_enabled;
struct device *dev;
@@ -557,10 +557,9 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
/*
* If this is the first gpio_request for the bank,
-* enable the bank module.
+* resume the bank module.
Since you removing bank notion, "If this is the first gpio_request
for the bank," becomes irrelevant from code perspective.
[..]
@@ -608,28 +594,15 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
__raw_readl(bank->base + bank->regs->wkup_en);
}
- bank->mod_usage &= ~(1 << offset);
-
- if (bank->regs->ctrl && !bank->mod_usage) {
- void __iomem *reg = bank->base + bank->regs->ctrl;
- u32 ctrl;
-
- ctrl = __raw_readl(reg);
- /* Module is disabled, clocks are gated */
- ctrl |= GPIO_MOD_CTRL_BIT;
- __raw_writel(ctrl, reg);
- bank->context.ctrl = ctrl;
- }
-
_reset_gpio(bank, bank->chip.base + offset);
spin_unlock_irqrestore(&bank->lock, flags);
/*
* If this is the last gpio to be freed in the bank,
-* disable the bank module.
+* put the bank module into suspend.
*/
- if (!bank->mod_usage)
- pm_runtime_put(bank->dev);
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_autosuspend(bank->dev);
Waiting for 2 seconds timeout even on GPIO free
seems to be wrong.
}
/*
@@ -715,7 +688,8 @@ static void gpio_irq_handler(unsigned int irq, struct
irq_desc *desc)
exit:
if (!unmasked)
chained_irq_exit(chip, desc);
- pm_runtime_put(bank->dev);
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_autosuspend(bank->dev);
This is what is the main change from this patch which helps your
case.
}
static void gpio_irq_shutdown(struct irq_data *d)
@@ -1132,6 +1106,8 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
platform_set_drvdata(pdev, bank);
+ pm_runtime_use_autosuspend(bank->dev);
+ pm_runtime_set_autosuspend_delay(bank->dev, GPIO_AUTOSUSPEND_TIMEOUT);
Can you please report how have you tested this change ? What other PM
tests you have done?
Removing mod usage might just break this driver because now individual
banks which can idle before, can no longer idle.
Just to expand a bit, Out of 6 GPIO banks, GPIO1 bank is in always ON
domain where as remaing 5 are in peripheral domain. Letting individual
banks idle allowed you let the clock domain idle than keeping all the
6 banks and hence respective clock/power domain in ON state.
So the adding timeout might be reasonable but I am not sure about
the mod_usage change here.
Jon, What you say ?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
On Saturday 27 October 2012 03:09 AM, Jon Hunter wrote: On 10/26/2012 03:01 PM, Felipe Balbi wrote: Hi, On Fri, Oct 26, 2012 at 03:19:13PM +0200, Tim Niemeyer wrote: Adds support for configuring the omap-gpio driver use autosuspend for runtime power management. This can reduce the latency in using it by not suspending the device immediately on idle. If another access takes place before the autosuspend timeout (2 secs), the call to resume the device can return immediately saving some save/ restore cycles. This removes also the bank->mod_usage counter, because this is already handled in pm_runtime. I use a gpio to monitor a spi transfer which occurs every 250µs. The suspend overhead is to high, so almost every second transfer is lost. This patch fixes that. Signed-off-by: Tim Niemeyer --- drivers/gpio/gpio-omap.c | 81 - 1 files changed, 43 insertions(+), 38 deletions(-) diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 94cbc84..708d5a9 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -31,6 +31,7 @@ #include #define OFF_MODE 1 +#define GPIO_AUTOSUSPEND_TIMEOUT2000 something just hit me... If you keep timeout at 2000 ms and you hook this up to an IRQ line, it's very unlikely GPIO will ever sleep. Why did you choose 2000 ms ? Arbitrary value ? It does seem quite large. I wonder if the default timeout should be something much smaller and then users can set the timeout needed for their specific application via the sysfs. By the way, it appears that I keep getting unsubscribed from linux-omap mailing list and I only saw this because you copied me. Thanks Felipe! Can you bounce the thread to me? I didn't get this email either though am getting other LOML emails. Please bounce the thread. Am curious to see the implementation, since GPIO IP as such doesn't have any state machine of any sort. Regards Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
On 10/26/2012 03:01 PM, Felipe Balbi wrote: > Hi, > > On Fri, Oct 26, 2012 at 03:19:13PM +0200, Tim Niemeyer wrote: >> Adds support for configuring the omap-gpio driver use autosuspend for >> runtime power management. This can reduce the latency in using it by >> not suspending the device immediately on idle. If another access takes >> place before the autosuspend timeout (2 secs), the call to resume the >> device can return immediately saving some save/ restore cycles. >> >> This removes also the bank->mod_usage counter, because this is already >> handled in pm_runtime. >> >> I use a gpio to monitor a spi transfer which occurs every 250µs. The >> suspend overhead is to high, so almost every second transfer is lost. >> This patch fixes that. >> >> Signed-off-by: Tim Niemeyer >> --- >> drivers/gpio/gpio-omap.c | 81 >> - >> 1 files changed, 43 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >> index 94cbc84..708d5a9 100644 >> --- a/drivers/gpio/gpio-omap.c >> +++ b/drivers/gpio/gpio-omap.c >> @@ -31,6 +31,7 @@ >> #include >> >> #define OFF_MODE1 >> +#define GPIO_AUTOSUSPEND_TIMEOUT2000 > > something just hit me... If you keep timeout at 2000 ms and you hook > this up to an IRQ line, it's very unlikely GPIO will ever sleep. > > Why did you choose 2000 ms ? Arbitrary value ? It does seem quite large. I wonder if the default timeout should be something much smaller and then users can set the timeout needed for their specific application via the sysfs. By the way, it appears that I keep getting unsubscribed from linux-omap mailing list and I only saw this because you copied me. Thanks Felipe! Can you bounce the thread to me? Also if anyone knows why I could be getting unsubscribed let me know. May be my emails have a bad signal to noise ratio :-( Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Hi, On Fri, Oct 26, 2012 at 03:19:13PM +0200, Tim Niemeyer wrote: > Adds support for configuring the omap-gpio driver use autosuspend for > runtime power management. This can reduce the latency in using it by > not suspending the device immediately on idle. If another access takes > place before the autosuspend timeout (2 secs), the call to resume the > device can return immediately saving some save/ restore cycles. > > This removes also the bank->mod_usage counter, because this is already > handled in pm_runtime. > > I use a gpio to monitor a spi transfer which occurs every 250µs. The > suspend overhead is to high, so almost every second transfer is lost. > This patch fixes that. > > Signed-off-by: Tim Niemeyer > --- > drivers/gpio/gpio-omap.c | 81 - > 1 files changed, 43 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 94cbc84..708d5a9 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -31,6 +31,7 @@ > #include > > #define OFF_MODE 1 > +#define GPIO_AUTOSUSPEND_TIMEOUT2000 something just hit me... If you keep timeout at 2000 ms and you hook this up to an IRQ line, it's very unlikely GPIO will ever sleep. Why did you choose 2000 ms ? Arbitrary value ? -- balbi signature.asc Description: Digital signature
[PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Adds support for configuring the omap-gpio driver use autosuspend for
runtime power management. This can reduce the latency in using it by
not suspending the device immediately on idle. If another access takes
place before the autosuspend timeout (2 secs), the call to resume the
device can return immediately saving some save/ restore cycles.
This removes also the bank->mod_usage counter, because this is already
handled in pm_runtime.
I use a gpio to monitor a spi transfer which occurs every 250µs. The
suspend overhead is to high, so almost every second transfer is lost.
This patch fixes that.
Signed-off-by: Tim Niemeyer
---
drivers/gpio/gpio-omap.c | 81 -
1 files changed, 43 insertions(+), 38 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 94cbc84..708d5a9 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -31,6 +31,7 @@
#include
#define OFF_MODE 1
+#define GPIO_AUTOSUSPEND_TIMEOUT2000
static LIST_HEAD(omap_gpio_list);
@@ -64,7 +65,6 @@ struct gpio_bank {
spinlock_t lock;
struct gpio_chip chip;
struct clk *dbck;
- u32 mod_usage;
u32 dbck_enable_mask;
bool dbck_enabled;
struct device *dev;
@@ -557,10 +557,9 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
/*
* If this is the first gpio_request for the bank,
-* enable the bank module.
+* resume the bank module.
*/
- if (!bank->mod_usage)
- pm_runtime_get_sync(bank->dev);
+ pm_runtime_get_sync(bank->dev);
spin_lock_irqsave(&bank->lock, flags);
/* Set trigger to none. You need to enable the desired trigger with
@@ -575,19 +574,6 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
__raw_writel(__raw_readl(reg) | (1 << offset), reg);
}
- if (bank->regs->ctrl && !bank->mod_usage) {
- void __iomem *reg = bank->base + bank->regs->ctrl;
- u32 ctrl;
-
- ctrl = __raw_readl(reg);
- /* Module is enabled, clocks are not gated */
- ctrl &= ~GPIO_MOD_CTRL_BIT;
- __raw_writel(ctrl, reg);
- bank->context.ctrl = ctrl;
- }
-
- bank->mod_usage |= 1 << offset;
-
spin_unlock_irqrestore(&bank->lock, flags);
return 0;
@@ -608,28 +594,15 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
__raw_readl(bank->base + bank->regs->wkup_en);
}
- bank->mod_usage &= ~(1 << offset);
-
- if (bank->regs->ctrl && !bank->mod_usage) {
- void __iomem *reg = bank->base + bank->regs->ctrl;
- u32 ctrl;
-
- ctrl = __raw_readl(reg);
- /* Module is disabled, clocks are gated */
- ctrl |= GPIO_MOD_CTRL_BIT;
- __raw_writel(ctrl, reg);
- bank->context.ctrl = ctrl;
- }
-
_reset_gpio(bank, bank->chip.base + offset);
spin_unlock_irqrestore(&bank->lock, flags);
/*
* If this is the last gpio to be freed in the bank,
-* disable the bank module.
+* put the bank module into suspend.
*/
- if (!bank->mod_usage)
- pm_runtime_put(bank->dev);
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_autosuspend(bank->dev);
}
/*
@@ -715,7 +688,8 @@ static void gpio_irq_handler(unsigned int irq, struct
irq_desc *desc)
exit:
if (!unmasked)
chained_irq_exit(chip, desc);
- pm_runtime_put(bank->dev);
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_autosuspend(bank->dev);
}
static void gpio_irq_shutdown(struct irq_data *d)
@@ -1132,6 +1106,8 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
platform_set_drvdata(pdev, bank);
+ pm_runtime_use_autosuspend(bank->dev);
+ pm_runtime_set_autosuspend_delay(bank->dev, GPIO_AUTOSUSPEND_TIMEOUT);
pm_runtime_enable(bank->dev);
pm_runtime_irq_safe(bank->dev);
pm_runtime_get_sync(bank->dev);
@@ -1146,7 +1122,8 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
if (bank->loses_context)
bank->get_context_loss_count = pdata->get_context_loss_count;
- pm_runtime_put(bank->dev);
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_autosuspend(bank->dev);
list_add_tail(&bank->node, &omap_gpio_list);
@@ -1168,6 +1145,17 @@ static int omap_gpio_runtime_suspend(struct device *dev)
spin_lock_irqsave(&bank->lock, flags);
+ if (bank->regs->ctrl) {
+ void __iomem *reg = bank->base + bank->regs->ctrl;
+ u32 ctrl;
+
+ ctrl = __raw_readl(reg);
+ /* Module is disabled, clocks are gated */
+
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
On Fri, Oct 26, 2012 at 12:42:35PM +0200, Tim Niemeyer wrote:
> Am Freitag, den 26.10.2012, 11:03 +0300 schrieb Felipe Balbi:
> > Hi,
> >
> > On Fri, Oct 26, 2012 at 09:55:30AM +0200, Tim Niemeyer wrote:
> > > Adds support for configuring the omap-gpio driver use autosuspend for
> > > runtime power management. This can reduce the latency in using it by
> > > not suspending the device immediately on idle. If another access takes
> > > place before the autosuspend timeout (2 secs), the call to resume the
> > > device can return immediately saving some save/ restore cycles.
> > >
> > > I use a gpio to monitor a spi transfer which occurs every 250µs. The
> > > suspend overhead is to high, so almost every second transfer is lost.
> > > This patch fixes that.
> > >
> > > Signed-off-by: Tim Niemeyer
> > > ---
> > > drivers/gpio/gpio-omap.c | 23 ++-
> > > 1 files changed, 18 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > > index 94cbc84..92f48cb 100644
> > > --- a/drivers/gpio/gpio-omap.c
> > > +++ b/drivers/gpio/gpio-omap.c
> > > @@ -31,6 +31,7 @@
> > > #include
> > >
> > > #define OFF_MODE 1
> > > +#define GPIO_AUTOSUSPEND_TIMEOUT2000
> > >
> > > static LIST_HEAD(omap_gpio_list);
> > >
> > > @@ -628,8 +629,10 @@ static void omap_gpio_free(struct gpio_chip *chip,
> > > unsigned offset)
> > >* If this is the last gpio to be freed in the bank,
> > >* disable the bank module.
> > >*/
> > > - if (!bank->mod_usage)
> > > - pm_runtime_put(bank->dev);
> > > + if (!bank->mod_usage) {
> >
> > while at that I would drop this bank->mod_usage nonsense and let
> > power core handle reference counting.
> I looked at it, but i'm unsure about the GPIO_MOD_CTRL_BIT. The
> bank->mod_usage counter prevents omap_gpio_free() to disable the hole
> bank while another gpio is still in use.
pm core's usage count will do the same thing. If you request 6 gpios,
you will have 6 pm_runtime_get(). If you free 5 of those, you will have
5 pm_runtime_put_autosuspend() which will only decrement the counter and
do nothing else.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Am Freitag, den 26.10.2012, 11:03 +0300 schrieb Felipe Balbi:
> Hi,
>
> On Fri, Oct 26, 2012 at 09:55:30AM +0200, Tim Niemeyer wrote:
> > Adds support for configuring the omap-gpio driver use autosuspend for
> > runtime power management. This can reduce the latency in using it by
> > not suspending the device immediately on idle. If another access takes
> > place before the autosuspend timeout (2 secs), the call to resume the
> > device can return immediately saving some save/ restore cycles.
> >
> > I use a gpio to monitor a spi transfer which occurs every 250µs. The
> > suspend overhead is to high, so almost every second transfer is lost.
> > This patch fixes that.
> >
> > Signed-off-by: Tim Niemeyer
> > ---
> > drivers/gpio/gpio-omap.c | 23 ++-
> > 1 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index 94cbc84..92f48cb 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -31,6 +31,7 @@
> > #include
> >
> > #define OFF_MODE 1
> > +#define GPIO_AUTOSUSPEND_TIMEOUT2000
> >
> > static LIST_HEAD(omap_gpio_list);
> >
> > @@ -628,8 +629,10 @@ static void omap_gpio_free(struct gpio_chip *chip,
> > unsigned offset)
> > * If this is the last gpio to be freed in the bank,
> > * disable the bank module.
> > */
> > - if (!bank->mod_usage)
> > - pm_runtime_put(bank->dev);
> > + if (!bank->mod_usage) {
>
> while at that I would drop this bank->mod_usage nonsense and let
> power core handle reference counting.
I looked at it, but i'm unsure about the GPIO_MOD_CTRL_BIT. The
bank->mod_usage counter prevents omap_gpio_free() to disable the hole
bank while another gpio is still in use.
> > + pm_runtime_mark_last_busy(bank->dev);
> > + pm_runtime_put_autosuspend(bank->dev);
> > + }
> > }
> >
> > /*
> > @@ -715,7 +718,8 @@ static void gpio_irq_handler(unsigned int irq, struct
> > irq_desc *desc)
> > exit:
> > if (!unmasked)
> > chained_irq_exit(chip, desc);
> > - pm_runtime_put(bank->dev);
> > + pm_runtime_mark_last_busy(bank->dev);
> > + pm_runtime_put_autosuspend(bank->dev);
> > }
> >
> > static void gpio_irq_shutdown(struct irq_data *d)
> > @@ -1132,6 +1136,8 @@ static int __devinit omap_gpio_probe(struct
> > platform_device *pdev)
> >
> > platform_set_drvdata(pdev, bank);
> >
> > + pm_runtime_use_autosuspend(bank->dev);
> > + pm_runtime_set_autosuspend_delay(bank->dev, GPIO_AUTOSUSPEND_TIMEOUT);
> > pm_runtime_enable(bank->dev);
> > pm_runtime_irq_safe(bank->dev);
> > pm_runtime_get_sync(bank->dev);
> > @@ -1146,7 +1152,8 @@ static int __devinit omap_gpio_probe(struct
> > platform_device *pdev)
> > if (bank->loses_context)
> > bank->get_context_loss_count = pdata->get_context_loss_count;
> >
> > - pm_runtime_put(bank->dev);
> > + pm_runtime_mark_last_busy(bank->dev);
> > + pm_runtime_put_autosuspend(bank->dev);
> >
> > list_add_tail(&bank->node, &omap_gpio_list);
> >
> > @@ -1333,7 +1340,13 @@ void omap2_gpio_prepare_for_idle(int pwr_mode)
> >
> > bank->power_mode = pwr_mode;
> >
> > - pm_runtime_put_sync_suspend(bank->dev);
> > + /* direct pm_runtime on pwroff */
> > + if (pwr_mode)
>
> you also need braces here.
Yes. I resend this later.
> > + pm_runtime_put_sync_suspend(bank->dev);
> > + else {
> > + pm_runtime_mark_last_busy(bank->dev);
> > + pm_runtime_put_sync_autosuspend(bank->dev);
> > + }
> > }
> > }
> >
> > --
> > 1.7.2.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Tim Niemeyer
Corscience GmbH & Co. KG
Henkestr. 91
D-91052 Erlangen
Germany
e-mail: tim.nieme...@corscience.de
Internet: www.corscience.de
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Hi,
On Fri, Oct 26, 2012 at 09:55:30AM +0200, Tim Niemeyer wrote:
> Adds support for configuring the omap-gpio driver use autosuspend for
> runtime power management. This can reduce the latency in using it by
> not suspending the device immediately on idle. If another access takes
> place before the autosuspend timeout (2 secs), the call to resume the
> device can return immediately saving some save/ restore cycles.
>
> I use a gpio to monitor a spi transfer which occurs every 250µs. The
> suspend overhead is to high, so almost every second transfer is lost.
> This patch fixes that.
>
> Signed-off-by: Tim Niemeyer
> ---
> drivers/gpio/gpio-omap.c | 23 ++-
> 1 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 94cbc84..92f48cb 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -31,6 +31,7 @@
> #include
>
> #define OFF_MODE 1
> +#define GPIO_AUTOSUSPEND_TIMEOUT2000
>
> static LIST_HEAD(omap_gpio_list);
>
> @@ -628,8 +629,10 @@ static void omap_gpio_free(struct gpio_chip *chip,
> unsigned offset)
>* If this is the last gpio to be freed in the bank,
>* disable the bank module.
>*/
> - if (!bank->mod_usage)
> - pm_runtime_put(bank->dev);
> + if (!bank->mod_usage) {
while at that I would drop this bank->mod_usage nonsense and let
power core handle reference counting.
> + pm_runtime_mark_last_busy(bank->dev);
> + pm_runtime_put_autosuspend(bank->dev);
> + }
> }
>
> /*
> @@ -715,7 +718,8 @@ static void gpio_irq_handler(unsigned int irq, struct
> irq_desc *desc)
> exit:
> if (!unmasked)
> chained_irq_exit(chip, desc);
> - pm_runtime_put(bank->dev);
> + pm_runtime_mark_last_busy(bank->dev);
> + pm_runtime_put_autosuspend(bank->dev);
> }
>
> static void gpio_irq_shutdown(struct irq_data *d)
> @@ -1132,6 +1136,8 @@ static int __devinit omap_gpio_probe(struct
> platform_device *pdev)
>
> platform_set_drvdata(pdev, bank);
>
> + pm_runtime_use_autosuspend(bank->dev);
> + pm_runtime_set_autosuspend_delay(bank->dev, GPIO_AUTOSUSPEND_TIMEOUT);
> pm_runtime_enable(bank->dev);
> pm_runtime_irq_safe(bank->dev);
> pm_runtime_get_sync(bank->dev);
> @@ -1146,7 +1152,8 @@ static int __devinit omap_gpio_probe(struct
> platform_device *pdev)
> if (bank->loses_context)
> bank->get_context_loss_count = pdata->get_context_loss_count;
>
> - pm_runtime_put(bank->dev);
> + pm_runtime_mark_last_busy(bank->dev);
> + pm_runtime_put_autosuspend(bank->dev);
>
> list_add_tail(&bank->node, &omap_gpio_list);
>
> @@ -1333,7 +1340,13 @@ void omap2_gpio_prepare_for_idle(int pwr_mode)
>
> bank->power_mode = pwr_mode;
>
> - pm_runtime_put_sync_suspend(bank->dev);
> + /* direct pm_runtime on pwroff */
> + if (pwr_mode)
you also need braces here.
> + pm_runtime_put_sync_suspend(bank->dev);
> + else {
> + pm_runtime_mark_last_busy(bank->dev);
> + pm_runtime_put_sync_autosuspend(bank->dev);
> + }
> }
> }
>
> --
> 1.7.2.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
balbi
signature.asc
Description: Digital signature
[PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend
Adds support for configuring the omap-gpio driver use autosuspend for
runtime power management. This can reduce the latency in using it by
not suspending the device immediately on idle. If another access takes
place before the autosuspend timeout (2 secs), the call to resume the
device can return immediately saving some save/ restore cycles.
I use a gpio to monitor a spi transfer which occurs every 250µs. The
suspend overhead is to high, so almost every second transfer is lost.
This patch fixes that.
Signed-off-by: Tim Niemeyer
---
drivers/gpio/gpio-omap.c | 23 ++-
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 94cbc84..92f48cb 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -31,6 +31,7 @@
#include
#define OFF_MODE 1
+#define GPIO_AUTOSUSPEND_TIMEOUT2000
static LIST_HEAD(omap_gpio_list);
@@ -628,8 +629,10 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
* If this is the last gpio to be freed in the bank,
* disable the bank module.
*/
- if (!bank->mod_usage)
- pm_runtime_put(bank->dev);
+ if (!bank->mod_usage) {
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_autosuspend(bank->dev);
+ }
}
/*
@@ -715,7 +718,8 @@ static void gpio_irq_handler(unsigned int irq, struct
irq_desc *desc)
exit:
if (!unmasked)
chained_irq_exit(chip, desc);
- pm_runtime_put(bank->dev);
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_autosuspend(bank->dev);
}
static void gpio_irq_shutdown(struct irq_data *d)
@@ -1132,6 +1136,8 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
platform_set_drvdata(pdev, bank);
+ pm_runtime_use_autosuspend(bank->dev);
+ pm_runtime_set_autosuspend_delay(bank->dev, GPIO_AUTOSUSPEND_TIMEOUT);
pm_runtime_enable(bank->dev);
pm_runtime_irq_safe(bank->dev);
pm_runtime_get_sync(bank->dev);
@@ -1146,7 +1152,8 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
if (bank->loses_context)
bank->get_context_loss_count = pdata->get_context_loss_count;
- pm_runtime_put(bank->dev);
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_autosuspend(bank->dev);
list_add_tail(&bank->node, &omap_gpio_list);
@@ -1333,7 +1340,13 @@ void omap2_gpio_prepare_for_idle(int pwr_mode)
bank->power_mode = pwr_mode;
- pm_runtime_put_sync_suspend(bank->dev);
+ /* direct pm_runtime on pwroff */
+ if (pwr_mode)
+ pm_runtime_put_sync_suspend(bank->dev);
+ else {
+ pm_runtime_mark_last_busy(bank->dev);
+ pm_runtime_put_sync_autosuspend(bank->dev);
+ }
}
}
--
1.7.2.5
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
