| Message ID | 1371818219-13060-1-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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", >
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", >> > >
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", >>> >> >> > >
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", >>>> >>> >>> >> >> > >
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", >>>>> >>>> >>>> >>> >>> >> >> > >
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
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", >>>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> > >
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.
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", >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> > >
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",
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(-)