| Message ID | 1402675276-538682-2-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Arnd Bergmann, On Fri, 13 Jun 2014 18:01:03 +0200, Arnd Bergmann wrote: > @@ -200,7 +201,9 @@ static noinline int do_armada_370_xp_cpu_suspend(unsigned long deepidle) > /* If we are here, wfi failed. As processors run out of > * coherency for some time, tlbs might be stale, so flush them > */ > +#ifdef CONFIG_MMU > local_flush_tlb_all(); > +#endif So noMMU builds are supported in a single image with MMU builds? Instead of having to #ifdef all around the place, wouldn't it be better for the noMMU headers to provide an empty definition for this function? > static int armada_370_xp_cpu_suspend(unsigned long deepidle) > { > + if (!IS_ENABLED(CONFIG_CPU_IDLE)) > + return -ENODEV; > + > return cpu_suspend(deepidle, do_armada_370_xp_cpu_suspend); > } This unfortunately isn't really correct, and the patch I sent in "[PATCH] ARM: mvebu: select ARM_CPU_SUSPEND for Marvell EBU v7 platforms" on June, 11th already fixes the cpu_suspend()/cpu_resume() problem. The issue with your solution is that it assumes this function is only used for cpuidle. That is indeed the case in v3.16, but will be wrong as soon as we add CPU hotplug support, which I have posted on May, 30th in "[PATCH 0/5] CPU hotplug for Marvell Armada XP", and which I hope to see included in v3.17. Thanks! Thomas
On Tuesday 17 June 2014 16:00:11 Thomas Petazzoni wrote: > Dear Arnd Bergmann, > > On Fri, 13 Jun 2014 18:01:03 +0200, Arnd Bergmann wrote: > > > @@ -200,7 +201,9 @@ static noinline int do_armada_370_xp_cpu_suspend(unsigned long deepidle) > > /* If we are here, wfi failed. As processors run out of > > * coherency for some time, tlbs might be stale, so flush them > > */ > > +#ifdef CONFIG_MMU > > local_flush_tlb_all(); > > +#endif > > So noMMU builds are supported in a single image with MMU builds? It's not supported yet, but I have a patch to do that, which I used for testing this code. I try to have the platform code ready before submitting the main patch. > Instead of having to #ifdef all around the place, wouldn't it be better > for the noMMU headers to provide an empty definition for this function? Yes, good point, it should probably be part of the main patch then. > > static int armada_370_xp_cpu_suspend(unsigned long deepidle) > > { > > + if (!IS_ENABLED(CONFIG_CPU_IDLE)) > > + return -ENODEV; > > + > > return cpu_suspend(deepidle, do_armada_370_xp_cpu_suspend); > > } > > This unfortunately isn't really correct, and the patch I sent in > "[PATCH] ARM: mvebu: select ARM_CPU_SUSPEND for Marvell EBU v7 > platforms" on June, 11th already fixes the cpu_suspend()/cpu_resume() > problem. ok. > The issue with your solution is that it assumes this function is only > used for cpuidle. That is indeed the case in v3.16, but will be wrong > as soon as we add CPU hotplug support, which I have posted on May, 30th > in "[PATCH 0/5] CPU hotplug for Marvell Armada XP", and which I hope to > see included in v3.17. Right. I'll drop this patch from the submission then. You came just in time, as I had originally planned to send it out this morning. I'll also follow up with a separate patch to address the isb() issue. Arnd
Dear Arnd Bergmann, On Tue, 17 Jun 2014 17:03:28 +0200, Arnd Bergmann wrote: > > So noMMU builds are supported in a single image with MMU builds? > > It's not supported yet, but I have a patch to do that, which I used for > testing this code. I try to have the platform code ready before submitting > the main patch. Ok. > > This unfortunately isn't really correct, and the patch I sent in > > "[PATCH] ARM: mvebu: select ARM_CPU_SUSPEND for Marvell EBU v7 > > platforms" on June, 11th already fixes the cpu_suspend()/cpu_resume() > > problem. > > ok. > > > The issue with your solution is that it assumes this function is only > > used for cpuidle. That is indeed the case in v3.16, but will be wrong > > as soon as we add CPU hotplug support, which I have posted on May, 30th > > in "[PATCH 0/5] CPU hotplug for Marvell Armada XP", and which I hope to > > see included in v3.17. > > Right. I'll drop this patch from the submission then. You came just in > time, as I had originally planned to send it out this morning. > > I'll also follow up with a separate patch to address the isb() issue. Ok, thanks a lot! So, should my patch adding the "select ARM_CPU_SUSPEND" be taken by Jason and then pushed to you, or will you apply it directly? Thanks! Thomas
On Tuesday 17 June 2014 17:17:00 Thomas Petazzoni wrote: > > > > This unfortunately isn't really correct, and the patch I sent in > > > "[PATCH] ARM: mvebu: select ARM_CPU_SUSPEND for Marvell EBU v7 > > > platforms" on June, 11th already fixes the cpu_suspend()/cpu_resume() > > > problem. > > > > ok. > > > > > The issue with your solution is that it assumes this function is only > > > used for cpuidle. That is indeed the case in v3.16, but will be wrong > > > as soon as we add CPU hotplug support, which I have posted on May, 30th > > > in "[PATCH 0/5] CPU hotplug for Marvell Armada XP", and which I hope to > > > see included in v3.17. > > > > Right. I'll drop this patch from the submission then. You came just in > > time, as I had originally planned to send it out this morning. > > > > I'll also follow up with a separate patch to address the isb() issue. > > Ok, thanks a lot! > > So, should my patch adding the "select ARM_CPU_SUSPEND" be taken by > Jason and then pushed to you, or will you apply it directly? I'll wait for Jason to pick it up. It was clearly a mistake for me to include my own patch directly and without an explicit Ack, I'll go back to the normal way of doing things ;-) ARnd
diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig index 6090b9e..bcfe094 100644 --- a/arch/arm/mach-mvebu/Kconfig +++ b/arch/arm/mach-mvebu/Kconfig @@ -18,6 +18,7 @@ menu "Marvell EBU SoC variants" config MACH_MVEBU_V7 bool select ARMADA_370_XP_TIMER + select ARM_CPU_SUSPEND if CPU_IDLE select CACHE_L2X0 config MACH_ARMADA_370 diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c index 53a55c8..14351bf 100644 --- a/arch/arm/mach-mvebu/pmsu.c +++ b/arch/arm/mach-mvebu/pmsu.c @@ -26,6 +26,7 @@ #include <linux/platform_device.h> #include <linux/smp.h> #include <linux/resource.h> +#include <asm/barrier.h> #include <asm/cacheflush.h> #include <asm/cp15.h> #include <asm/smp_plat.h> @@ -200,7 +201,9 @@ static noinline int do_armada_370_xp_cpu_suspend(unsigned long deepidle) /* If we are here, wfi failed. As processors run out of * coherency for some time, tlbs might be stale, so flush them */ +#ifdef CONFIG_MMU local_flush_tlb_all(); +#endif ll_enable_coherency(); @@ -210,9 +213,10 @@ static noinline int do_armada_370_xp_cpu_suspend(unsigned long deepidle) "tst %0, #(1 << 2) \n\t" "orreq %0, %0, #(1 << 2) \n\t" "mcreq p15, 0, %0, c1, c0, 0 \n\t" - "isb " : : "r" (0)); + isb(); + pr_warn("Failed to suspend the system\n"); return 0; @@ -220,6 +224,9 @@ static noinline int do_armada_370_xp_cpu_suspend(unsigned long deepidle) static int armada_370_xp_cpu_suspend(unsigned long deepidle) { + if (!IS_ENABLED(CONFIG_CPU_IDLE)) + return -ENODEV; + return cpu_suspend(deepidle, do_armada_370_xp_cpu_suspend); }
local_flush_tlb_all() is not available for NOMMU NOSMP builds. The isb() assembly instruction is not available when building a combined ARMv6/v7 kernel, unlike the isb() inline that falls back to the ARMv6 opcode. If CPUIDLE support is disabled globally, we must not call arm_cpu_suspend. If it is enabled, we should ensure that it is available. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Gregory Clement <gregory.clement@free-electrons.com> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> --- arch/arm/mach-mvebu/Kconfig | 1 + arch/arm/mach-mvebu/pmsu.c | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-)