at91: cpuidle: Fix target_residency

PATCH New Daniel Lezcano 3 participants 10 messages Jun 21, 2013

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm/Microchip (AT91) SoC support

lore

Share & tools

lore

Daniel Lezcano

author

2013-06-21 12:36 UTC #0 lore raw

PATCH 1/1 at91: cpuidle: Fix target_residency

The following commit:

commit 7e348b9012522fa0efd854d20d210d5e57fcedd1

Author: Robert Lee <[email protected]>
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 <[email protected]>

---
 arch/arm/mach-at91/cpuidle.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mach-at91/cpuidle.c +1 -1

index 69f9e3b..4ec6a6d 100644
@@ -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",

Signature · 1 line

1.7.9.5

Nicolas Ferre

2013-06-21 13:20 UTC #1 lore raw

PATCH 1/1 at91: cpuidle: Fix target_residency

On 21/06/2013 14:36, Daniel Lezcano :

Quoted 36 lines

The following commit:

commit 7e348b9012522fa0efd854d20d210d5e57fcedd1

Author: Robert Lee <[email protected]>
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.

[ ... ]

(quoted diff) +8 -7

+       .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");
 
[ ... ]

Quoted 5 lines

The cpuidle never enters this state since this commit.
Fix it by setting the value to 10ms again.
Signed-off-by Daniel Lezcano <[email protected]>

Acked-by Nicolas Ferre <[email protected]>

Quoted 18 lines

---
  arch/arm/mach-at91/cpuidle.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mach-at91/cpuidle.c +1 -1

index 69f9e3b..4ec6a6d 100644
@@ -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",

Signature · 1 line

Nicolas Ferre

Daniel Lezcano

author

2013-06-21 13:22 UTC #2 lore raw

PATCH 1/1 at91: cpuidle: Fix target_residency

Quoted 38 lines · Nicolas Ferre

On 21/06/2013 14:36, Daniel Lezcano :

Quoted 37 lines

The following commit:

commit 7e348b9012522fa0efd854d20d210d5e57fcedd1

Author: Robert Lee <[email protected]>
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.

[ ... ]

(quoted diff) +8 -7

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

Quoted 26 lines

Quoted 3 lines

Fix it by setting the value to 10ms again.
Signed-off-by Daniel Lezcano <[email protected]>

Acked-by Nicolas Ferre <[email protected]>

Quoted 18 lines

---
  arch/arm/mach-at91/cpuidle.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mach-at91/cpuidle.c +1 -1

index 69f9e3b..4ec6a6d 100644
@@ -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",

Signature · 5 lines

 <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

Nicolas Ferre

2013-06-21 14:48 UTC #3 lore raw

PATCH 1/1 at91: cpuidle: Fix target_residency

On 21/06/2013 15:22, Daniel Lezcano :

Quoted 38 lines · Nicolas Ferre

Quoted 37 lines

On 21/06/2013 14:36, Daniel Lezcano :

Quoted 36 lines

The following commit:

commit 7e348b9012522fa0efd854d20d210d5e57fcedd1

Author: Robert Lee <[email protected]>
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.

[ ... ]

(quoted diff) +8 -7

+       .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");
 
[ ... ]

Quoted 7 lines

Quoted 1 line
Quoted 1 line
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)?

Quoted 2 lines

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,

Quoted 28 lines

Quoted 26 lines

Quoted 3 lines

Fix it by setting the value to 10ms again.
Signed-off-by Daniel Lezcano <[email protected]>

Acked-by Nicolas Ferre <[email protected]>

Quoted 18 lines

---
   arch/arm/mach-at91/cpuidle.c |    2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mach-at91/cpuidle.c +1 -1

index 69f9e3b..4ec6a6d 100644
@@ -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",

Signature · 1 line

Nicolas Ferre

Daniel Lezcano

author

2013-06-21 15:44 UTC #4 lore raw

PATCH 1/1 at91: cpuidle: Fix target_residency

Quoted 44 lines · Nicolas Ferre

On 21/06/2013 15:22, Daniel Lezcano :
Quoted 43 lines · Nicolas Ferre
Quoted 38 lines
On 21/06/2013 14:36, Daniel Lezcano :
Quoted 37 lines
The following commit:
commit 7e348b9012522fa0efd854d20d210d5e57fcedd1
Author: Robert Lee <[email protected]>
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.
[ ... ]
(quoted diff) +8 -7
+       .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.

Quoted 6 lines

Quoted 2 lines
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@ ...

Quoted 38 lines

Quoted 2 lines
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,
Quoted 29 lines
Quoted 27 lines
Quoted 3 lines
Fix it by setting the value to 10ms again.
Signed-off-by Daniel Lezcano <[email protected]>
Acked-by Nicolas Ferre <[email protected]>
Quoted 19 lines
---
   arch/arm/mach-at91/cpuidle.c |    2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mach-at91/cpuidle.c +1 -1
index 69f9e3b..4ec6a6d 100644
@@ -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",

Signature · 5 lines

 <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

Nicolas Ferre

2013-06-21 16:11 UTC #5 lore raw

PATCH 1/1 at91: cpuidle: Fix target_residency

On 21/06/2013 17:44, Daniel Lezcano :

Quoted 48 lines · Nicolas Ferre

Quoted 47 lines
On 21/06/2013 15:22, Daniel Lezcano :
Quoted 45 lines · Nicolas Ferre
Quoted 38 lines
On 21/06/2013 14:36, Daniel Lezcano :
Quoted 37 lines
The following commit:
commit 7e348b9012522fa0efd854d20d210d5e57fcedd1
Author: Robert Lee <[email protected]>
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.
[ ... ]
(quoted diff) +8 -7
+       .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.

Quoted 6 lines

Quoted 3 lines
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?

Quoted 40 lines

Quoted 38 lines
Quoted 2 lines
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,
Quoted 29 lines
Quoted 27 lines
Quoted 3 lines
Fix it by setting the value to 10ms again.
Signed-off-by Daniel Lezcano <[email protected]>
Acked-by Nicolas Ferre <[email protected]>
Quoted 19 lines
---
    arch/arm/mach-at91/cpuidle.c |    2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mach-at91/cpuidle.c +1 -1
index 69f9e3b..4ec6a6d 100644
@@ -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",

Signature · 1 line

Nicolas Ferre

Arnd Bergmann

2013-06-21 16:30 UTC #6 lore raw

PATCH 1/1 at91: cpuidle: Fix target_residency

Quoted 7 lines · Nicolas Ferre

Quoted 3 lines
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...

Quoted 7 lines

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

Daniel Lezcano

author

2013-06-21 16:40 UTC #7 lore raw

PATCH 1/1 at91: cpuidle: Fix target_residency

Quoted 13 lines · Arnd Bergmann

Quoted 12 lines · Nicolas Ferre
Quoted 3 lines
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...

Quoted 7 lines

Quoted 2 lines
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.

Signature · 5 lines

 <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

Daniel Lezcano

author

2013-06-21 16:37 UTC #8 lore raw

PATCH 1/1 at91: cpuidle: Fix target_residency

In reply to Nicolas Ferre

Quoted 55 lines · Nicolas Ferre

On 21/06/2013 17:44, Daniel Lezcano :
Quoted 54 lines · Nicolas Ferre
Quoted 52 lines
On 21/06/2013 15:22, Daniel Lezcano :
Quoted 39 lines · Nicolas Ferre
Quoted 38 lines
On 21/06/2013 14:36, Daniel Lezcano :
Quoted 37 lines
The following commit:
commit 7e348b9012522fa0efd854d20d210d5e57fcedd1
Author: Robert Lee <[email protected]>
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.
[ ... ]
(quoted diff) +8 -7
+       .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");
 
[ ... ]
Quoted 8 lines
Quoted 1 line
Quoted 1 line
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)?

Quoted 5 lines

Quoted 2 lines
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 :)

Quoted 2 lines

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.

Quoted 47 lines

So I suspect Arnd and Olof would not take it for 3.10-fixes...
Guys, you thoughts?
Quoted 40 lines
Quoted 38 lines
Quoted 2 lines
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,
Quoted 29 lines
Quoted 27 lines
Quoted 3 lines
Fix it by setting the value to 10ms again.
Signed-off-by Daniel Lezcano <[email protected]>
Acked-by Nicolas Ferre <[email protected]>
Quoted 19 lines
---
    arch/arm/mach-at91/cpuidle.c |    2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mach-at91/cpuidle.c +1 -1
index 69f9e3b..4ec6a6d 100644
@@ -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",

Signature · 5 lines

 <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

Daniel Lezcano

author

2013-06-21 16:43 UTC #9 lore raw

PATCH 1/1 at91: cpuidle: Fix target_residency

Quoted 64 lines · Daniel Lezcano

Quoted 55 lines · Nicolas Ferre
On 21/06/2013 17:44, Daniel Lezcano :
Quoted 54 lines · Nicolas Ferre
Quoted 52 lines
On 21/06/2013 15:22, Daniel Lezcano :
Quoted 39 lines · Nicolas Ferre
Quoted 38 lines
On 21/06/2013 14:36, Daniel Lezcano :
Quoted 37 lines
The following commit:
commit 7e348b9012522fa0efd854d20d210d5e57fcedd1
Author: Robert Lee <[email protected]>
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.
[ ... ]
(quoted diff) +8 -7
+       .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");
 
[ ... ]
Quoted 8 lines
Quoted 1 line
Quoted 1 line
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)?
Quoted 5 lines
Quoted 2 lines
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 :)

Quoted 8 lines

Quoted 2 lines
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 ?

Quoted 49 lines

Quoted 47 lines
So I suspect Arnd and Olof would not take it for 3.10-fixes...
Guys, you thoughts?
Quoted 40 lines
Quoted 38 lines
Quoted 2 lines
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,
Quoted 29 lines
Quoted 27 lines
Quoted 3 lines
Fix it by setting the value to 10ms again.
Signed-off-by Daniel Lezcano <[email protected]>
Acked-by Nicolas Ferre <[email protected]>
Quoted 19 lines
---
    arch/arm/mach-at91/cpuidle.c |    2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mach-at91/cpuidle.c +1 -1
index 69f9e3b..4ec6a6d 100644
@@ -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",

Signature · 5 lines

 <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