| Message ID | 9838a4aeee256adeaef90efe56df2c9988206982.1360475150.git.len.brown@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Len Brown |
| Headers | show |
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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());