[PATCH 06/16] ARM idle: delete pm_idle

[PATCH 06/16] ARM idle: delete pm_idle

[Len Brown]

From: Len Brown <len.brown@intel.com>

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

Signed-off-by: Len Brown <len.brown@intel.com>
Cc: linux-arm-kernel at lists.infradead.org
---
 arch/arm/kernel/process.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

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());
-- 
1.8.1.3.535.ga923c31

[PATCH 07/16] ARM64 idle: delete pm_idle

[Len Brown]

From: Len Brown <len.brown@intel.com>

pm_idle() on arm64 was a synonym for default_idle(),
so remove it and invoke default_idle() directly.

Signed-off-by: Len Brown <len.brown@intel.com>
Cc: linux-arm-kernel at lists.infradead.org
---
 arch/arm64/kernel/process.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index cb0956b..c7002d4 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -97,14 +97,9 @@ static void default_idle(void)
 	local_irq_enable();
 }
 
-void (*pm_idle)(void) = default_idle;
-EXPORT_SYMBOL_GPL(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)
 {
@@ -122,10 +117,10 @@ void cpu_idle(void)
 			local_irq_disable();
 			if (!need_resched()) {
 				stop_critical_timings();
-				pm_idle();
+				default_idle();
 				start_critical_timings();
 				/*
-				 * pm_idle functions should always return
+				 * default_idle functions should always return
 				 * with IRQs enabled.
 				 */
 				WARN_ON(irqs_disabled());
-- 
1.8.1.3.535.ga923c31

[PATCH 06/16] ARM idle: delete pm_idle

[Catalin Marinas]

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.

-- 
Catalin

[PATCH 06/16] ARM idle: delete pm_idle

[Russell King - ARM Linux]

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.

[PATCH 06/16] ARM idle: delete pm_idle

[Len Brown]

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

[PATCH 07/16] ARM64 idle: delete pm_idle

[Catalin Marinas]

On Sun, Feb 10, 2013 at 05:58:14AM +0000, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
> 
> pm_idle() on arm64 was a synonym for default_idle(),
> so remove it and invoke default_idle() directly.
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> Cc: linux-arm-kernel at lists.infradead.org

For arm64:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 06/16] ARM idle: delete pm_idle

[Kevin Hilman]

Len Brown <lenb@kernel.org> writes:

> From: Len Brown <len.brown@intel.com>
>
> pm_idle() on ARM was a synonym for default_idle(),
> so simply invoke default_idle() directly.
>
> Signed-off-by: Len Brown <len.brown@intel.com>
> Cc: linux-arm-kernel at lists.infradead.org

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 <khilman@linaro.org>
Tested-by: Kevin Hilman <khilman@linaro.org>

Kevin