[06/16] ARM idle: delete pm_idle

Commit Message

Len Brown Feb. 10, 2013, 5:58 a.m. UTC

From: Len Brown <[e-mail]>

pm_idle() on ARM was a synonym for default_idle(),
so simply invoke default_idle() directly.

Signed-off-by: Len Brown <[e-mail]>
Cc: [e-mail]
---
 arch/arm/kernel/process.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Comments

Catalin Marinas Feb. 11, 2013, 4:02 p.m. UTC | #1

On Sun, Feb 10, 2013 at 05:58:13AM +0000, Len Brown wrote:
> pm_idle() on ARM was a synonym for default_idle(),
> so simply invoke default_idle() directly.

The clean-up looks fine as we already have an arm_pm_idle but longer
term I was thinking about having a common declaration similar to
pm_power_off that code under drivers/power/(reset/) can override (and
such driver may be shared by multiple architectures). OTOH, if you get
rid of the generic linux/pm.h declaration architectures can use a common
pm_idle name and type (though I think having it in the common header
would be better). For ARM this would mean s/arm_pm_idle/pm_idle/ on top
if your patch.

Do you have any plans with pm_power_off as well?

Thanks.

Russell King - ARM Linux Feb. 11, 2013, 4:11 p.m. UTC | #2

On Mon, Feb 11, 2013 at 04:02:30PM +0000, Catalin Marinas wrote:
> On Sun, Feb 10, 2013 at 05:58:13AM +0000, Len Brown wrote:
> > pm_idle() on ARM was a synonym for default_idle(),
> > so simply invoke default_idle() directly.
> 
> The clean-up looks fine as we already have an arm_pm_idle but longer
> term I was thinking about having a common declaration similar to
> pm_power_off that code under drivers/power/(reset/) can override (and
> such driver may be shared by multiple architectures). OTOH, if you get
> rid of the generic linux/pm.h declaration architectures can use a common
> pm_idle name and type (though I think having it in the common header
> would be better). For ARM this would mean s/arm_pm_idle/pm_idle/ on top
> if your patch.

pm_idle() was that common declaration - but it had the side effect that
it was defined to be called with interrupts disabled, but return with
interrupts enabled.

arm_pm_idle() "fixed" that weirdness such that it's now expected to
return with IRQs in the same state that it was called.

pm_power_off() is a cross-arch hook already.

Len Brown Feb. 11, 2013, 10:42 p.m. UTC | #3

On 02/11/2013 11:11 AM, Russell King - ARM Linux wrote:
> On Mon, Feb 11, 2013 at 04:02:30PM +0000, Catalin Marinas wrote:
>> On Sun, Feb 10, 2013 at 05:58:13AM +0000, Len Brown wrote:
>>> pm_idle() on ARM was a synonym for default_idle(),
>>> so simply invoke default_idle() directly.
>>
>> The clean-up looks fine as we already have an arm_pm_idle but longer
>> term I was thinking about having a common declaration similar to
>> pm_power_off that code under drivers/power/(reset/) can override (and
>> such driver may be shared by multiple architectures). OTOH, if you get
>> rid of the generic linux/pm.h declaration architectures can use a common
>> pm_idle name and type (though I think having it in the common header
>> would be better). For ARM this would mean s/arm_pm_idle/pm_idle/ on top
>> if your patch.
> 
> pm_idle() was that common declaration - but it had the side effect that
> it was defined to be called with interrupts disabled, but return with
> interrupts enabled.

Yeah, I expect that grew out of the x86 idiom "STI;HLT",
which dictated default_idle(), which in-turn dictated pm_idle().
x86's MWAIT instruction now has more flexibility WRT interrupts,
but the cast was already set...

Please let me know if you Ack the patch as is,
or would like it changed.

thanks,
-Len Brown, Intel Open Source Technology Center

> arm_pm_idle() "fixed" that weirdness such that it's now expected to
> return with IRQs in the same state that it was called.
> 
> pm_power_off() is a cross-arch hook already.
>

Kevin Hilman Feb. 12, 2013, 10:04 p.m. UTC | #4

Len Brown <[e-mail]> writes:

> From: Len Brown <[e-mail]>
>
> pm_idle() on ARM was a synonym for default_idle(),
> so simply invoke default_idle() directly.
>
> Signed-off-by: Len Brown <[e-mail]>
> Cc: [e-mail]

Looks good to me.

Tested this patch along with 15/16 (removes global declaration) on an
ARM platform (TI OMAP3) with and without CPUidle, and things continue to
work as expected:

Reviewed-by: Kevin Hilman <[e-mail]>
Tested-by: Kevin Hilman <[e-mail]>

Kevin

Patch

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index c6dec5f..047d3e4 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -172,14 +172,9 @@  static void default_idle(void)
 	local_irq_enable();
 }
 
-void (*pm_idle)(void) = default_idle;
-EXPORT_SYMBOL(pm_idle);
-
 /*
- * The idle thread, has rather strange semantics for calling pm_idle,
- * but this is what x86 does and we need to do the same, so that
- * things like cpuidle get called in the same way.  The only difference
- * is that we always respect 'hlt_counter' to prevent low power idle.
+ * The idle thread.
+ * We always respect 'hlt_counter' to prevent low power idle.
  */
 void cpu_idle(void)
 {
@@ -210,10 +205,10 @@  void cpu_idle(void)
 			} else if (!need_resched()) {
 				stop_critical_timings();
 				if (cpuidle_idle_call())
-					pm_idle();
+					default_idle();
 				start_critical_timings();
 				/*
-				 * pm_idle functions must always
+				 * default_idle functions must always
 				 * return with IRQs enabled.
 				 */
 				WARN_ON(irqs_disabled());