* [PATCH] at91: cpuidle: Fix target_residency
@ 2013-06-21 12:36 Daniel Lezcano
2013-06-21 13:20 ` Nicolas Ferre
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2013-06-21 12:36 UTC (permalink / raw)
To: linux-arm-kernel
The following commit:
commit 7e348b9012522fa0efd854d20d210d5e57fcedd1
Author: Robert Lee <rob.lee@linaro.org>
Date: Tue Mar 20 15:22:43 2012 -0500
ARM: at91: Consolidate time keeping and irq enable
Enable core cpuidle timekeeping and irq enabling and remove that
handling from this code.
introduced a zero to the state1 (suspend) target residency.
[ ... ]
+ .states[1] = {
+ .enter = at91_enter_idle,
+ .exit_latency = 10,
+ .target_residency = 100000,
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .name = "RAM_SR",
+ .desc = "WFI and DDR Self Refresh",
+ },
[ ... ]
- /* Wait for interrupt and RAM self refresh state */
- driver->states[1].enter = at91_enter_idle;
- driver->states[1].exit_latency = 10;
- driver->states[1].target_residency = 10000;
- driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(driver->states[1].name, "RAM_SR");
- strcpy(driver->states[1].desc, "WFI and RAM Self Refresh");
[ ... ]
The cpuidle never enters this state since this commit.
Fix it by setting the value to 10ms again.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
arch/arm/mach-at91/cpuidle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index 69f9e3b..4ec6a6d 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
@@ -51,7 +51,7 @@ static struct cpuidle_driver at91_idle_driver = {
.states[1] = {
.enter = at91_enter_idle,
.exit_latency = 10,
- .target_residency = 100000,
+ .target_residency = 10000,
.flags = CPUIDLE_FLAG_TIME_VALID,
.name = "RAM_SR",
.desc = "WFI and DDR Self Refresh",
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH] at91: cpuidle: Fix target_residency 2013-06-21 12:36 [PATCH] at91: cpuidle: Fix target_residency Daniel Lezcano @ 2013-06-21 13:20 ` Nicolas Ferre 2013-06-21 13:22 ` Daniel Lezcano 0 siblings, 1 reply; 10+ messages in thread From: Nicolas Ferre @ 2013-06-21 13:20 UTC (permalink / raw) To: linux-arm-kernel On 21/06/2013 14:36, Daniel Lezcano : > The following commit: > > commit 7e348b9012522fa0efd854d20d210d5e57fcedd1 > Author: Robert Lee <rob.lee@linaro.org> > Date: Tue Mar 20 15:22:43 2012 -0500 > > ARM: at91: Consolidate time keeping and irq enable > > Enable core cpuidle timekeeping and irq enabling and remove that > handling from this code. > > introduced a zero to the state1 (suspend) target residency. > > [ ... ] > > + .states[1] = { > + .enter = at91_enter_idle, > + .exit_latency = 10, > + .target_residency = 100000, > + .flags = CPUIDLE_FLAG_TIME_VALID, > + .name = "RAM_SR", > + .desc = "WFI and DDR Self Refresh", > + }, > > [ ... ] > > - /* Wait for interrupt and RAM self refresh state */ > - driver->states[1].enter = at91_enter_idle; > - driver->states[1].exit_latency = 10; > - driver->states[1].target_residency = 10000; > - driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID; > - strcpy(driver->states[1].name, "RAM_SR"); > - strcpy(driver->states[1].desc, "WFI and RAM Self Refresh"); > > [ ... ] > > The cpuidle never enters this state since this commit. > > Fix it by setting the value to 10ms again. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > arch/arm/mach-at91/cpuidle.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c > index 69f9e3b..4ec6a6d 100644 > --- a/arch/arm/mach-at91/cpuidle.c > +++ b/arch/arm/mach-at91/cpuidle.c > @@ -51,7 +51,7 @@ static struct cpuidle_driver at91_idle_driver = { > .states[1] = { > .enter = at91_enter_idle, > .exit_latency = 10, > - .target_residency = 100000, > + .target_residency = 10000, > .flags = CPUIDLE_FLAG_TIME_VALID, > .name = "RAM_SR", > .desc = "WFI and DDR Self Refresh", > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] at91: cpuidle: Fix target_residency 2013-06-21 13:20 ` Nicolas Ferre @ 2013-06-21 13:22 ` Daniel Lezcano 2013-06-21 14:48 ` Nicolas Ferre 0 siblings, 1 reply; 10+ messages in thread From: Daniel Lezcano @ 2013-06-21 13:22 UTC (permalink / raw) To: linux-arm-kernel On 06/21/2013 03:20 PM, Nicolas Ferre wrote: > On 21/06/2013 14:36, Daniel Lezcano : >> The following commit: >> >> commit 7e348b9012522fa0efd854d20d210d5e57fcedd1 >> Author: Robert Lee <rob.lee@linaro.org> >> Date: Tue Mar 20 15:22:43 2012 -0500 >> >> ARM: at91: Consolidate time keeping and irq enable >> >> Enable core cpuidle timekeeping and irq enabling and remove that >> handling from this code. >> >> introduced a zero to the state1 (suspend) target residency. >> >> [ ... ] >> >> + .states[1] = { >> + .enter = at91_enter_idle, >> + .exit_latency = 10, >> + .target_residency = 100000, >> + .flags = CPUIDLE_FLAG_TIME_VALID, >> + .name = "RAM_SR", >> + .desc = "WFI and DDR Self Refresh", >> + }, >> >> [ ... ] >> >> - /* Wait for interrupt and RAM self refresh state */ >> - driver->states[1].enter = at91_enter_idle; >> - driver->states[1].exit_latency = 10; >> - driver->states[1].target_residency = 10000; >> - driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID; >> - strcpy(driver->states[1].name, "RAM_SR"); >> - strcpy(driver->states[1].desc, "WFI and RAM Self Refresh"); >> >> [ ... ] >> >> The cpuidle never enters this state since this commit. To be a bit more precise. With a periodic tick, the cpu never enters the state1 with both 10000 and 100000. With a tickless system, it enters to state1 much more often with the initial value, roughly x7 more. BTW, I am surpised with a sam926*, CONFIG_NO_HZ=y is not in the default config. >> Fix it by setting the value to 10ms again. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > >> --- >> arch/arm/mach-at91/cpuidle.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c >> index 69f9e3b..4ec6a6d 100644 >> --- a/arch/arm/mach-at91/cpuidle.c >> +++ b/arch/arm/mach-at91/cpuidle.c >> @@ -51,7 +51,7 @@ static struct cpuidle_driver at91_idle_driver = { >> .states[1] = { >> .enter = at91_enter_idle, >> .exit_latency = 10, >> - .target_residency = 100000, >> + .target_residency = 10000, >> .flags = CPUIDLE_FLAG_TIME_VALID, >> .name = "RAM_SR", >> .desc = "WFI and DDR Self Refresh", >> > > -- <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] at91: cpuidle: Fix target_residency 2013-06-21 13:22 ` Daniel Lezcano @ 2013-06-21 14:48 ` Nicolas Ferre 2013-06-21 15:44 ` Daniel Lezcano 0 siblings, 1 reply; 10+ messages in thread From: Nicolas Ferre @ 2013-06-21 14:48 UTC (permalink / raw) To: linux-arm-kernel On 21/06/2013 15:22, Daniel Lezcano : > On 06/21/2013 03:20 PM, Nicolas Ferre wrote: >> On 21/06/2013 14:36, Daniel Lezcano : >>> The following commit: >>> >>> commit 7e348b9012522fa0efd854d20d210d5e57fcedd1 >>> Author: Robert Lee <rob.lee@linaro.org> >>> Date: Tue Mar 20 15:22:43 2012 -0500 >>> >>> ARM: at91: Consolidate time keeping and irq enable >>> >>> Enable core cpuidle timekeeping and irq enabling and remove that >>> handling from this code. >>> >>> introduced a zero to the state1 (suspend) target residency. >>> >>> [ ... ] >>> >>> + .states[1] = { >>> + .enter = at91_enter_idle, >>> + .exit_latency = 10, >>> + .target_residency = 100000, >>> + .flags = CPUIDLE_FLAG_TIME_VALID, >>> + .name = "RAM_SR", >>> + .desc = "WFI and DDR Self Refresh", >>> + }, >>> >>> [ ... ] >>> >>> - /* Wait for interrupt and RAM self refresh state */ >>> - driver->states[1].enter = at91_enter_idle; >>> - driver->states[1].exit_latency = 10; >>> - driver->states[1].target_residency = 10000; >>> - driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID; >>> - strcpy(driver->states[1].name, "RAM_SR"); >>> - strcpy(driver->states[1].desc, "WFI and RAM Self Refresh"); >>> >>> [ ... ] >>> >>> The cpuidle never enters this state since this commit. > > To be a bit more precise. With a periodic tick, the cpu never enters the > state1 with both 10000 and 100000. > > With a tickless system, it enters to state1 much more often with the > initial value, roughly x7 more. BTW Daniel, I think I can stack this patch on my fixes-non-critical branch for 3.11: do you think that I should push for making it accepted for 3.10 (even if it seems very late)? > BTW, I am surpised with a sam926*, CONFIG_NO_HZ=y is not in the default > config. Yes, indeed: we have to consider it. Best regards, >>> Fix it by setting the value to 10ms again. >>> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> >> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >> >>> --- >>> arch/arm/mach-at91/cpuidle.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c >>> index 69f9e3b..4ec6a6d 100644 >>> --- a/arch/arm/mach-at91/cpuidle.c >>> +++ b/arch/arm/mach-at91/cpuidle.c >>> @@ -51,7 +51,7 @@ static struct cpuidle_driver at91_idle_driver = { >>> .states[1] = { >>> .enter = at91_enter_idle, >>> .exit_latency = 10, >>> - .target_residency = 100000, >>> + .target_residency = 10000, >>> .flags = CPUIDLE_FLAG_TIME_VALID, >>> .name = "RAM_SR", >>> .desc = "WFI and DDR Self Refresh", >>> >> >> > > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] at91: cpuidle: Fix target_residency 2013-06-21 14:48 ` Nicolas Ferre @ 2013-06-21 15:44 ` Daniel Lezcano 2013-06-21 16:11 ` Nicolas Ferre 0 siblings, 1 reply; 10+ messages in thread From: Daniel Lezcano @ 2013-06-21 15:44 UTC (permalink / raw) To: linux-arm-kernel On 06/21/2013 04:48 PM, Nicolas Ferre wrote: > On 21/06/2013 15:22, Daniel Lezcano : >> On 06/21/2013 03:20 PM, Nicolas Ferre wrote: >>> On 21/06/2013 14:36, Daniel Lezcano : >>>> The following commit: >>>> >>>> commit 7e348b9012522fa0efd854d20d210d5e57fcedd1 >>>> Author: Robert Lee <rob.lee@linaro.org> >>>> Date: Tue Mar 20 15:22:43 2012 -0500 >>>> >>>> ARM: at91: Consolidate time keeping and irq enable >>>> >>>> Enable core cpuidle timekeeping and irq enabling and remove that >>>> handling from this code. >>>> >>>> introduced a zero to the state1 (suspend) target residency. >>>> >>>> [ ... ] >>>> >>>> + .states[1] = { >>>> + .enter = at91_enter_idle, >>>> + .exit_latency = 10, >>>> + .target_residency = 100000, >>>> + .flags = CPUIDLE_FLAG_TIME_VALID, >>>> + .name = "RAM_SR", >>>> + .desc = "WFI and DDR Self Refresh", >>>> + }, >>>> >>>> [ ... ] >>>> >>>> - /* Wait for interrupt and RAM self refresh state */ >>>> - driver->states[1].enter = at91_enter_idle; >>>> - driver->states[1].exit_latency = 10; >>>> - driver->states[1].target_residency = 10000; >>>> - driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID; >>>> - strcpy(driver->states[1].name, "RAM_SR"); >>>> - strcpy(driver->states[1].desc, "WFI and RAM Self Refresh"); >>>> >>>> [ ... ] >>>> >>>> The cpuidle never enters this state since this commit. >> >> To be a bit more precise. With a periodic tick, the cpu never enters the >> state1 with both 10000 and 100000. >> >> With a tickless system, it enters to state1 much more often with the >> initial value, roughly x7 more. > > BTW Daniel, I think I can stack this patch on my fixes-non-critical > branch for 3.11: do you think that I should push for making it accepted > for 3.10 (even if it seems very late)? Yes, I think it should go for 3.10 as it is fix and also for 3.9.8 (stable). May be I should have Cc stable@ ... >> BTW, I am surpised with a sam926*, CONFIG_NO_HZ=y is not in the default >> config. > > Yes, indeed: we have to consider it. > > Best regards, > >>>> Fix it by setting the value to 10ms again. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> >>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >>> >>>> --- >>>> arch/arm/mach-at91/cpuidle.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm/mach-at91/cpuidle.c >>>> b/arch/arm/mach-at91/cpuidle.c >>>> index 69f9e3b..4ec6a6d 100644 >>>> --- a/arch/arm/mach-at91/cpuidle.c >>>> +++ b/arch/arm/mach-at91/cpuidle.c >>>> @@ -51,7 +51,7 @@ static struct cpuidle_driver at91_idle_driver = { >>>> .states[1] = { >>>> .enter = at91_enter_idle, >>>> .exit_latency = 10, >>>> - .target_residency = 100000, >>>> + .target_residency = 10000, >>>> .flags = CPUIDLE_FLAG_TIME_VALID, >>>> .name = "RAM_SR", >>>> .desc = "WFI and DDR Self Refresh", >>>> >>> >>> >> >> > > -- <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] at91: cpuidle: Fix target_residency 2013-06-21 15:44 ` Daniel Lezcano @ 2013-06-21 16:11 ` Nicolas Ferre 2013-06-21 16:30 ` Arnd Bergmann 2013-06-21 16:37 ` Daniel Lezcano 0 siblings, 2 replies; 10+ messages in thread From: Nicolas Ferre @ 2013-06-21 16:11 UTC (permalink / raw) To: linux-arm-kernel On 21/06/2013 17:44, Daniel Lezcano : > On 06/21/2013 04:48 PM, Nicolas Ferre wrote: >> On 21/06/2013 15:22, Daniel Lezcano : >>> On 06/21/2013 03:20 PM, Nicolas Ferre wrote: >>>> On 21/06/2013 14:36, Daniel Lezcano : >>>>> The following commit: >>>>> >>>>> commit 7e348b9012522fa0efd854d20d210d5e57fcedd1 >>>>> Author: Robert Lee <rob.lee@linaro.org> >>>>> Date: Tue Mar 20 15:22:43 2012 -0500 >>>>> >>>>> ARM: at91: Consolidate time keeping and irq enable >>>>> >>>>> Enable core cpuidle timekeeping and irq enabling and remove that >>>>> handling from this code. >>>>> >>>>> introduced a zero to the state1 (suspend) target residency. >>>>> >>>>> [ ... ] >>>>> >>>>> + .states[1] = { >>>>> + .enter = at91_enter_idle, >>>>> + .exit_latency = 10, >>>>> + .target_residency = 100000, >>>>> + .flags = CPUIDLE_FLAG_TIME_VALID, >>>>> + .name = "RAM_SR", >>>>> + .desc = "WFI and DDR Self Refresh", >>>>> + }, >>>>> >>>>> [ ... ] >>>>> >>>>> - /* Wait for interrupt and RAM self refresh state */ >>>>> - driver->states[1].enter = at91_enter_idle; >>>>> - driver->states[1].exit_latency = 10; >>>>> - driver->states[1].target_residency = 10000; >>>>> - driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID; >>>>> - strcpy(driver->states[1].name, "RAM_SR"); >>>>> - strcpy(driver->states[1].desc, "WFI and RAM Self Refresh"); >>>>> >>>>> [ ... ] >>>>> >>>>> The cpuidle never enters this state since this commit. >>> >>> To be a bit more precise. With a periodic tick, the cpu never enters the >>> state1 with both 10000 and 100000. >>> >>> With a tickless system, it enters to state1 much more often with the >>> initial value, roughly x7 more. >> >> BTW Daniel, I think I can stack this patch on my fixes-non-critical >> branch for 3.11: do you think that I should push for making it accepted >> for 3.10 (even if it seems very late)? > > Yes, I think it should go for 3.10 as it is fix and also for 3.9.8 > (stable). May be I should have Cc stable@ ... Well, so it doesn't sound like a regression if it was already present in 3.9... Moreover, it does not seem to be taken into account for all configuration (seems not triggered for !tickless kernels). So I suspect Arnd and Olof would not take it for 3.10-fixes... Guys, you thoughts? >>> BTW, I am surpised with a sam926*, CONFIG_NO_HZ=y is not in the default >>> config. >> >> Yes, indeed: we have to consider it. >> >> Best regards, >> >>>>> Fix it by setting the value to 10ms again. >>>>> >>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> >>>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >>>> >>>>> --- >>>>> arch/arm/mach-at91/cpuidle.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/arm/mach-at91/cpuidle.c >>>>> b/arch/arm/mach-at91/cpuidle.c >>>>> index 69f9e3b..4ec6a6d 100644 >>>>> --- a/arch/arm/mach-at91/cpuidle.c >>>>> +++ b/arch/arm/mach-at91/cpuidle.c >>>>> @@ -51,7 +51,7 @@ static struct cpuidle_driver at91_idle_driver = { >>>>> .states[1] = { >>>>> .enter = at91_enter_idle, >>>>> .exit_latency = 10, >>>>> - .target_residency = 100000, >>>>> + .target_residency = 10000, >>>>> .flags = CPUIDLE_FLAG_TIME_VALID, >>>>> .name = "RAM_SR", >>>>> .desc = "WFI and DDR Self Refresh", >>>>> >>>> >>>> >>> >>> >> >> > > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] at91: cpuidle: Fix target_residency 2013-06-21 16:11 ` Nicolas Ferre @ 2013-06-21 16:30 ` Arnd Bergmann 2013-06-21 16:40 ` Daniel Lezcano 2013-06-21 16:37 ` Daniel Lezcano 1 sibling, 1 reply; 10+ messages in thread From: Arnd Bergmann @ 2013-06-21 16:30 UTC (permalink / raw) To: linux-arm-kernel On Friday 21 June 2013, Nicolas Ferre wrote: > > > > Yes, I think it should go for 3.10 as it is fix and also for 3.9.8 > > (stable). May be I should have Cc stable@ ... > > Well, so it doesn't sound like a regression if it was already present in > 3.9... > > Moreover, it does not seem to be taken into account for all > configuration (seems not triggered for !tickless kernels). > > So I suspect Arnd and Olof would not take it for 3.10-fixes... > > Guys, you thoughts? > Ah, I just sent out a pull request for fixes a minute before I saw your mail. I can't tell how serious this bug is. We can certainly mark it for stable, but unless this is a significant regression, I'd apply it for 3.11 instead and wait for the backports to happen. Arnd ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] at91: cpuidle: Fix target_residency 2013-06-21 16:30 ` Arnd Bergmann @ 2013-06-21 16:40 ` Daniel Lezcano 0 siblings, 0 replies; 10+ messages in thread From: Daniel Lezcano @ 2013-06-21 16:40 UTC (permalink / raw) To: linux-arm-kernel On 06/21/2013 06:30 PM, Arnd Bergmann wrote: > On Friday 21 June 2013, Nicolas Ferre wrote: >>> >>> Yes, I think it should go for 3.10 as it is fix and also for 3.9.8 >>> (stable). May be I should have Cc stable@ ... >> >> Well, so it doesn't sound like a regression if it was already present in >> 3.9... >> >> Moreover, it does not seem to be taken into account for all >> configuration (seems not triggered for !tickless kernels). >> >> So I suspect Arnd and Olof would not take it for 3.10-fixes... >> >> Guys, you thoughts? >> > > Ah, I just sent out a pull request for fixes a minute before I saw > your mail. I can't tell how serious this bug is. We can certainly > mark it for stable, but unless this is a significant regression, > I'd apply it for 3.11 instead and wait for the backports to happen. Ok, this is not a critical bug. The regression makes the idle state1 to enter less often, thus consuming a bit more power with a tickless system, but nothing leading to a crash or an unstable system. -- <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] at91: cpuidle: Fix target_residency 2013-06-21 16:11 ` Nicolas Ferre 2013-06-21 16:30 ` Arnd Bergmann @ 2013-06-21 16:37 ` Daniel Lezcano 2013-06-21 16:43 ` Daniel Lezcano 1 sibling, 1 reply; 10+ messages in thread From: Daniel Lezcano @ 2013-06-21 16:37 UTC (permalink / raw) To: linux-arm-kernel On 06/21/2013 06:11 PM, Nicolas Ferre wrote: > On 21/06/2013 17:44, Daniel Lezcano : >> On 06/21/2013 04:48 PM, Nicolas Ferre wrote: >>> On 21/06/2013 15:22, Daniel Lezcano : >>>> On 06/21/2013 03:20 PM, Nicolas Ferre wrote: >>>>> On 21/06/2013 14:36, Daniel Lezcano : >>>>>> The following commit: >>>>>> >>>>>> commit 7e348b9012522fa0efd854d20d210d5e57fcedd1 >>>>>> Author: Robert Lee <rob.lee@linaro.org> >>>>>> Date: Tue Mar 20 15:22:43 2012 -0500 >>>>>> >>>>>> ARM: at91: Consolidate time keeping and irq enable >>>>>> >>>>>> Enable core cpuidle timekeeping and irq enabling and remove >>>>>> that >>>>>> handling from this code. >>>>>> >>>>>> introduced a zero to the state1 (suspend) target residency. >>>>>> >>>>>> [ ... ] >>>>>> >>>>>> + .states[1] = { >>>>>> + .enter = at91_enter_idle, >>>>>> + .exit_latency = 10, >>>>>> + .target_residency = 100000, >>>>>> + .flags = CPUIDLE_FLAG_TIME_VALID, >>>>>> + .name = "RAM_SR", >>>>>> + .desc = "WFI and DDR Self Refresh", >>>>>> + }, >>>>>> >>>>>> [ ... ] >>>>>> >>>>>> - /* Wait for interrupt and RAM self refresh state */ >>>>>> - driver->states[1].enter = at91_enter_idle; >>>>>> - driver->states[1].exit_latency = 10; >>>>>> - driver->states[1].target_residency = 10000; >>>>>> - driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID; >>>>>> - strcpy(driver->states[1].name, "RAM_SR"); >>>>>> - strcpy(driver->states[1].desc, "WFI and RAM Self Refresh"); >>>>>> >>>>>> [ ... ] >>>>>> >>>>>> The cpuidle never enters this state since this commit. >>>> >>>> To be a bit more precise. With a periodic tick, the cpu never enters >>>> the >>>> state1 with both 10000 and 100000. >>>> >>>> With a tickless system, it enters to state1 much more often with the >>>> initial value, roughly x7 more. >>> >>> BTW Daniel, I think I can stack this patch on my fixes-non-critical >>> branch for 3.11: do you think that I should push for making it accepted >>> for 3.10 (even if it seems very late)? >> >> Yes, I think it should go for 3.10 as it is fix and also for 3.9.8 >> (stable). May be I should have Cc stable@ ... > > Well, so it doesn't sound like a regression if it was already present in > 3.9... Or the regression is there since 3.5 and detected today :) > Moreover, it does not seem to be taken into account for all > configuration (seems not triggered for !tickless kernels). The periodic tick is IIRC, 100Hz, so 10ms, the same value as the idle state1 target residency, the governor won't use this state in any case because the next scheduled event occurs before the target residency. If it would have been, let's say, 8.5ms, or the periodic tick 50Hz, then it would have been picked up. > So I suspect Arnd and Olof would not take it for 3.10-fixes... > > Guys, you thoughts? > > >>>> BTW, I am surpised with a sam926*, CONFIG_NO_HZ=y is not in the default >>>> config. >>> >>> Yes, indeed: we have to consider it. >>> >>> Best regards, >>> >>>>>> Fix it by setting the value to 10ms again. >>>>>> >>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>> >>>>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >>>>> >>>>>> --- >>>>>> arch/arm/mach-at91/cpuidle.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/arm/mach-at91/cpuidle.c >>>>>> b/arch/arm/mach-at91/cpuidle.c >>>>>> index 69f9e3b..4ec6a6d 100644 >>>>>> --- a/arch/arm/mach-at91/cpuidle.c >>>>>> +++ b/arch/arm/mach-at91/cpuidle.c >>>>>> @@ -51,7 +51,7 @@ static struct cpuidle_driver at91_idle_driver = { >>>>>> .states[1] = { >>>>>> .enter = at91_enter_idle, >>>>>> .exit_latency = 10, >>>>>> - .target_residency = 100000, >>>>>> + .target_residency = 10000, >>>>>> .flags = CPUIDLE_FLAG_TIME_VALID, >>>>>> .name = "RAM_SR", >>>>>> .desc = "WFI and DDR Self Refresh", >>>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> > > -- <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] at91: cpuidle: Fix target_residency 2013-06-21 16:37 ` Daniel Lezcano @ 2013-06-21 16:43 ` Daniel Lezcano 0 siblings, 0 replies; 10+ messages in thread From: Daniel Lezcano @ 2013-06-21 16:43 UTC (permalink / raw) To: linux-arm-kernel On 06/21/2013 06:37 PM, Daniel Lezcano wrote: > On 06/21/2013 06:11 PM, Nicolas Ferre wrote: >> On 21/06/2013 17:44, Daniel Lezcano : >>> On 06/21/2013 04:48 PM, Nicolas Ferre wrote: >>>> On 21/06/2013 15:22, Daniel Lezcano : >>>>> On 06/21/2013 03:20 PM, Nicolas Ferre wrote: >>>>>> On 21/06/2013 14:36, Daniel Lezcano : >>>>>>> The following commit: >>>>>>> >>>>>>> commit 7e348b9012522fa0efd854d20d210d5e57fcedd1 >>>>>>> Author: Robert Lee <rob.lee@linaro.org> >>>>>>> Date: Tue Mar 20 15:22:43 2012 -0500 >>>>>>> >>>>>>> ARM: at91: Consolidate time keeping and irq enable >>>>>>> >>>>>>> Enable core cpuidle timekeeping and irq enabling and remove >>>>>>> that >>>>>>> handling from this code. >>>>>>> >>>>>>> introduced a zero to the state1 (suspend) target residency. >>>>>>> >>>>>>> [ ... ] >>>>>>> >>>>>>> + .states[1] = { >>>>>>> + .enter = at91_enter_idle, >>>>>>> + .exit_latency = 10, >>>>>>> + .target_residency = 100000, >>>>>>> + .flags = CPUIDLE_FLAG_TIME_VALID, >>>>>>> + .name = "RAM_SR", >>>>>>> + .desc = "WFI and DDR Self Refresh", >>>>>>> + }, >>>>>>> >>>>>>> [ ... ] >>>>>>> >>>>>>> - /* Wait for interrupt and RAM self refresh state */ >>>>>>> - driver->states[1].enter = at91_enter_idle; >>>>>>> - driver->states[1].exit_latency = 10; >>>>>>> - driver->states[1].target_residency = 10000; >>>>>>> - driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID; >>>>>>> - strcpy(driver->states[1].name, "RAM_SR"); >>>>>>> - strcpy(driver->states[1].desc, "WFI and RAM Self Refresh"); >>>>>>> >>>>>>> [ ... ] >>>>>>> >>>>>>> The cpuidle never enters this state since this commit. >>>>> >>>>> To be a bit more precise. With a periodic tick, the cpu never enters >>>>> the >>>>> state1 with both 10000 and 100000. >>>>> >>>>> With a tickless system, it enters to state1 much more often with the >>>>> initial value, roughly x7 more. >>>> >>>> BTW Daniel, I think I can stack this patch on my fixes-non-critical >>>> branch for 3.11: do you think that I should push for making it accepted >>>> for 3.10 (even if it seems very late)? >>> >>> Yes, I think it should go for 3.10 as it is fix and also for 3.9.8 >>> (stable). May be I should have Cc stable@ ... >> >> Well, so it doesn't sound like a regression if it was already present in >> 3.9... > > Or the regression is there since 3.5 and detected today :) > >> Moreover, it does not seem to be taken into account for all >> configuration (seems not triggered for !tickless kernels). > > The periodic tick is IIRC, 100Hz, so 10ms, the same value as the idle > state1 target residency, the governor won't use this state in any case > because the next scheduled event occurs before the target residency. If > it would have been, let's say, 8.5ms, or the periodic tick 50Hz, then it > would have been picked up. A question, BTW, is a target residency of 10ms not a bit long for self-refresh ? >> So I suspect Arnd and Olof would not take it for 3.10-fixes... >> >> Guys, you thoughts? >> >> >>>>> BTW, I am surpised with a sam926*, CONFIG_NO_HZ=y is not in the default >>>>> config. >>>> >>>> Yes, indeed: we have to consider it. >>>> >>>> Best regards, >>>> >>>>>>> Fix it by setting the value to 10ms again. >>>>>>> >>>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>>> >>>>>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >>>>>> >>>>>>> --- >>>>>>> arch/arm/mach-at91/cpuidle.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/arch/arm/mach-at91/cpuidle.c >>>>>>> b/arch/arm/mach-at91/cpuidle.c >>>>>>> index 69f9e3b..4ec6a6d 100644 >>>>>>> --- a/arch/arm/mach-at91/cpuidle.c >>>>>>> +++ b/arch/arm/mach-at91/cpuidle.c >>>>>>> @@ -51,7 +51,7 @@ static struct cpuidle_driver at91_idle_driver = { >>>>>>> .states[1] = { >>>>>>> .enter = at91_enter_idle, >>>>>>> .exit_latency = 10, >>>>>>> - .target_residency = 100000, >>>>>>> + .target_residency = 10000, >>>>>>> .flags = CPUIDLE_FLAG_TIME_VALID, >>>>>>> .name = "RAM_SR", >>>>>>> .desc = "WFI and DDR Self Refresh", >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> > > -- <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-06-21 16:43 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-06-21 12:36 [PATCH] at91: cpuidle: Fix target_residency Daniel Lezcano 2013-06-21 13:20 ` Nicolas Ferre 2013-06-21 13:22 ` Daniel Lezcano 2013-06-21 14:48 ` Nicolas Ferre 2013-06-21 15:44 ` Daniel Lezcano 2013-06-21 16:11 ` Nicolas Ferre 2013-06-21 16:30 ` Arnd Bergmann 2013-06-21 16:40 ` Daniel Lezcano 2013-06-21 16:37 ` Daniel Lezcano 2013-06-21 16:43 ` Daniel Lezcano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).