| Message ID | DA41BE1DDCA941489001C7FBD7A8820E3CB5512E@szxeml549-mbx.china.huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/31/2012 08:24 PM, Yangfei (Felix) wrote: > The current "WFI" opcode definiton causes CPU hot-plug feature fails to work > if the kernel is built with CONFIG_THUMB2_KERNEL/CONFIG_CPU_ENDIAN_BE8 being > defined. An invalid instruction exception will be generated. > > Signed-off-by: yangfei.kernel@gmail.com > --- > arch/arm/mach-exynos/hotplug.c | 8 +++++++- > arch/arm/mach-realview/hotplug.c | 8 +++++++- > arch/arm/mach-shmobile/hotplug.c | 8 +++++++- > 3 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c > index f4d7dd2..823a0e4 100644 > --- a/arch/arm/mach-exynos/hotplug.c > +++ b/arch/arm/mach-exynos/hotplug.c > @@ -18,11 +18,17 @@ > #include <asm/cacheflush.h> > #include <asm/cp15.h> > #include <asm/smp_plat.h> > +#include <asm/opcodes.h> > > #include <mach/regs-pmu.h> > > #include "common.h" > > +/* > + * Define opcode of the WFI instruction. > + */ > +#define __WFI __inst_arm_thumb16(0xe320f003, 0xbf30) > + > static inline void cpu_enter_lowpower(void) > { > unsigned int v; > @@ -72,7 +78,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious) > /* > * here's the WFI > */ > - asm(".word 0xe320f003\n" > + asm(__WFI Wouldn't using the actual wfi instruction fix this. There is a wfi() macro. Or just call cpu_do_idle() which will do any other things needed before wfi like a dsb instruction. Rob > : > : > : "memory", "cc"); > diff --git a/arch/arm/mach-realview/hotplug.c b/arch/arm/mach-realview/hotplug.c > index 53818e5..5271a1a 100644 > --- a/arch/arm/mach-realview/hotplug.c > +++ b/arch/arm/mach-realview/hotplug.c > @@ -15,6 +15,12 @@ > #include <asm/cacheflush.h> > #include <asm/cp15.h> > #include <asm/smp_plat.h> > +#include <asm/opcodes.h> > + > +/* > + * Define opcode of the WFI instruction. > + */ > +#define __WFI __inst_arm_thumb16(0xe320f003, 0xbf30) > > static inline void cpu_enter_lowpower(void) > { > @@ -64,7 +70,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious) > /* > * here's the WFI > */ > - asm(".word 0xe320f003\n" > + asm(__WFI > : > : > : "memory", "cc"); > diff --git a/arch/arm/mach-shmobile/hotplug.c b/arch/arm/mach-shmobile/hotplug.c > index b09a0bd..0d7b7d1 100644 > --- a/arch/arm/mach-shmobile/hotplug.c > +++ b/arch/arm/mach-shmobile/hotplug.c > @@ -20,6 +20,12 @@ > #include <mach/emev2.h> > #include <asm/cacheflush.h> > #include <asm/mach-types.h> > +#include <asm/opcodes.h> > + > +/* > + * Define opcode of the WFI instruction. > + */ > ++#define __WFI __inst_arm_thumb16(0xe320f003, 0xbf30) > > static cpumask_t dead_cpus; > > @@ -39,7 +45,7 @@ void shmobile_cpu_die(unsigned int cpu) > /* > * here's the WFI > */ > - asm(".word 0xe320f003\n" > + asm(__WFI > : > : > : "memory", "cc"); > -- > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
2012/11/1 Rob Herring <robherring2@gmail.com>: > On 10/31/2012 08:24 PM, Yangfei (Felix) wrote: >> The current "WFI" opcode definiton causes CPU hot-plug feature fails to >> work >> if the kernel is built with CONFIG_THUMB2_KERNEL/CONFIG_CPU_ENDIAN_BE8 >> being >> defined. An invalid instruction exception will be generated. >> >> Signed-off-by: yangfei.kernel@gmail.com >> --- >> arch/arm/mach-exynos/hotplug.c | 8 +++++++- >> arch/arm/mach-realview/hotplug.c | 8 +++++++- >> arch/arm/mach-shmobile/hotplug.c | 8 +++++++- >> 3 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/mach-exynos/hotplug.c >> b/arch/arm/mach-exynos/hotplug.c >> index f4d7dd2..823a0e4 100644 >> --- a/arch/arm/mach-exynos/hotplug.c >> +++ b/arch/arm/mach-exynos/hotplug.c >> @@ -18,11 +18,17 @@ >> #include <asm/cacheflush.h> >> #include <asm/cp15.h> >> #include <asm/smp_plat.h> >> +#include <asm/opcodes.h> >> >> #include <mach/regs-pmu.h> >> >> #include "common.h" >> >> +/* >> + * Define opcode of the WFI instruction. >> + */ >> +#define __WFI __inst_arm_thumb16(0xe320f003, 0xbf30) >> + >> static inline void cpu_enter_lowpower(void) >> { >> unsigned int v; >> @@ -72,7 +78,7 @@ static inline void platform_do_lowpower(unsigned int >> cpu, int *spurious) >> /* >> * here's the WFI >> */ >> - asm(".word 0xe320f003\n" >> + asm(__WFI > > Wouldn't using the actual wfi instruction fix this. There is a wfi() > macro. > > Or just call cpu_do_idle() which will do any other things needed before > wfi like a dsb instruction. > > Rob >> : >> : >> : "memory", "cc"); <Cut> Hi Rob, Thanks for the reply. The way you suggested is more elegant. But here we worried about the version of the compiler toolchain used to build the kernel. The "WFI" assembler instruction may not be recognized if the toolchain is too old. Need the related ARM board maintainers to confirm this.
On Thu, Nov 01, 2012 at 09:40:10PM +0800, Fei Yang wrote: > 2012/11/1 Rob Herring <robherring2@gmail.com>: > > On 10/31/2012 08:24 PM, Yangfei (Felix) wrote: > >> The current "WFI" opcode definiton causes CPU hot-plug feature fails to > >> work > >> if the kernel is built with CONFIG_THUMB2_KERNEL/CONFIG_CPU_ENDIAN_BE8 > >> being > >> defined. An invalid instruction exception will be generated. > >> > >> Signed-off-by: yangfei.kernel@gmail.com > >> --- > >> arch/arm/mach-exynos/hotplug.c | 8 +++++++- > >> arch/arm/mach-realview/hotplug.c | 8 +++++++- > >> arch/arm/mach-shmobile/hotplug.c | 8 +++++++- > >> 3 files changed, 21 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/arm/mach-exynos/hotplug.c > >> b/arch/arm/mach-exynos/hotplug.c > >> index f4d7dd2..823a0e4 100644 > >> --- a/arch/arm/mach-exynos/hotplug.c > >> +++ b/arch/arm/mach-exynos/hotplug.c > >> @@ -18,11 +18,17 @@ > >> #include <asm/cacheflush.h> > >> #include <asm/cp15.h> > >> #include <asm/smp_plat.h> > >> +#include <asm/opcodes.h> > >> > >> #include <mach/regs-pmu.h> > >> > >> #include "common.h" > >> > >> +/* > >> + * Define opcode of the WFI instruction. > >> + */ > >> +#define __WFI __inst_arm_thumb16(0xe320f003, 0xbf30) > >> + > >> static inline void cpu_enter_lowpower(void) > >> { > >> unsigned int v; > >> @@ -72,7 +78,7 @@ static inline void platform_do_lowpower(unsigned int > >> cpu, int *spurious) > >> /* > >> * here's the WFI > >> */ > >> - asm(".word 0xe320f003\n" > >> + asm(__WFI > > > > Wouldn't using the actual wfi instruction fix this. There is a wfi() > > macro. > > > > Or just call cpu_do_idle() which will do any other things needed before > > wfi like a dsb instruction. > > > > Rob > >> : > >> : > >> : "memory", "cc"); > > <Cut> > > Hi Rob, > Thanks for the reply. The way you suggested is more elegant. But > here we worried about the version of the compiler toolchain used to > build the kernel. The "WFI" assembler instruction may not be > recognized if the toolchain is too old. Need the related ARM board > maintainers to confirm this. Maybe all the exynos platforms are new enough for this not to be a problem? I think mach-exynos is pretty new and v7-only anyway. If so, then it may be better to put CFLAGS_hotplug.o := -march=armv7-a in arch/arm/mach-exynos/Makefile, and use the real "wfi" mnemonic directly. People should _really_ not be building kernels containig v7 board support with tools that are too old to support this. Cheers ---Dave
Dave Martin wrote: > [...] > > >> - asm(".word 0xe320f003\n" > > >> + asm(__WFI > > > > > > Wouldn't using the actual wfi instruction fix this. There is a wfi() > > > macro. > > > > > > Or just call cpu_do_idle() which will do any other things needed > before > > > wfi like a dsb instruction. > > > > > > Rob > > >> : > > >> : > > >> : "memory", "cc"); > > > > <Cut> > > > > Hi Rob, > > Thanks for the reply. The way you suggested is more elegant. But > > here we worried about the version of the compiler toolchain used to > > build the kernel. The "WFI" assembler instruction may not be > > recognized if the toolchain is too old. Need the related ARM board > > maintainers to confirm this. > > Maybe all the exynos platforms are new enough for this not to be a > problem? > Yeah, I think there is no problem on exynos now. > I think mach-exynos is pretty new and v7-only anyway. If so, then it Yes, right at the moment. BTW, if mach-exynos includes ARMv8 later?...ARMv8 platform codes will be put in the arch/arm/ or arch/arm/64/ if some platform codes share with ARMv7? Just wondering... > may be better to put > > CFLAGS_hotplug.o := -march=armv7-a > > in arch/arm/mach-exynos/Makefile, and use the real "wfi" mnemonic > directly. People should _really_ not be building kernels containig > v7 board support with tools that are too old to support this. > I think so... Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
On Tue, Nov 06, 2012 at 08:24:31PM +0900, Kukjin Kim wrote: > Dave Martin wrote: > > > > [...] > > > > >> - asm(".word 0xe320f003\n" > > > >> + asm(__WFI > > > > > > > > Wouldn't using the actual wfi instruction fix this. There is a wfi() > > > > macro. > > > > > > > > Or just call cpu_do_idle() which will do any other things needed > > before > > > > wfi like a dsb instruction. > > > > > > > > Rob > > > >> : > > > >> : > > > >> : "memory", "cc"); > > > > > > <Cut> > > > > > > Hi Rob, > > > Thanks for the reply. The way you suggested is more elegant. But > > > here we worried about the version of the compiler toolchain used to > > > build the kernel. The "WFI" assembler instruction may not be > > > recognized if the toolchain is too old. Need the related ARM board > > > maintainers to confirm this. > > > > Maybe all the exynos platforms are new enough for this not to be a > > problem? > > > Yeah, I think there is no problem on exynos now. > > > I think mach-exynos is pretty new and v7-only anyway. If so, then it > > Yes, right at the moment. > > BTW, if mach-exynos includes ARMv8 later?...ARMv8 platform codes will be put > in the arch/arm/ or arch/arm/64/ if some platform codes share with ARMv7? > Just wondering... That's a question for Catalin, I guess. > > may be better to put > > > > CFLAGS_hotplug.o := -march=armv7-a > > > > in arch/arm/mach-exynos/Makefile, and use the real "wfi" mnemonic > > directly. People should _really_ not be building kernels containig > > v7 board support with tools that are too old to support this. > > > I think so... OK, thanks for commenting. Cheers ---Dave
On 6 November 2012 11:24, Kukjin Kim <kgene.kim@samsung.com> wrote: > BTW, if mach-exynos includes ARMv8 later?...ARMv8 platform codes will be put > in the arch/arm/ or arch/arm/64/ if some platform codes share with ARMv7? > Just wondering... If mach-exynos would support ARMv8 at some point, I would expect most of the code to go under various drivers/ subsystems (mfd, power etc.). I don't see any point in using opcodes. For wfi just use a macro or cpu_do_idle() (as Rob said, it does the required dsb as well). I'm also pushing for a standard hotplug.c implementation that is shared by multiple platforms and uses the power state coordination interface implemented by the firmware.
diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c index f4d7dd2..823a0e4 100644 --- a/arch/arm/mach-exynos/hotplug.c +++ b/arch/arm/mach-exynos/hotplug.c @@ -18,11 +18,17 @@ #include <asm/cacheflush.h> #include <asm/cp15.h> #include <asm/smp_plat.h> +#include <asm/opcodes.h> #include <mach/regs-pmu.h> #include "common.h" +/* + * Define opcode of the WFI instruction. + */ +#define __WFI __inst_arm_thumb16(0xe320f003, 0xbf30) + static inline void cpu_enter_lowpower(void) { unsigned int v; @@ -72,7 +78,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious) /* * here's the WFI */ - asm(".word 0xe320f003\n" + asm(__WFI : : : "memory", "cc"); diff --git a/arch/arm/mach-realview/hotplug.c b/arch/arm/mach-realview/hotplug.c index 53818e5..5271a1a 100644 --- a/arch/arm/mach-realview/hotplug.c +++ b/arch/arm/mach-realview/hotplug.c @@ -15,6 +15,12 @@ #include <asm/cacheflush.h> #include <asm/cp15.h> #include <asm/smp_plat.h> +#include <asm/opcodes.h> + +/* + * Define opcode of the WFI instruction. + */ +#define __WFI __inst_arm_thumb16(0xe320f003, 0xbf30) static inline void cpu_enter_lowpower(void) { @@ -64,7 +70,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious) /* * here's the WFI */ - asm(".word 0xe320f003\n" + asm(__WFI : : : "memory", "cc"); diff --git a/arch/arm/mach-shmobile/hotplug.c b/arch/arm/mach-shmobile/hotplug.c index b09a0bd..0d7b7d1 100644 --- a/arch/arm/mach-shmobile/hotplug.c +++ b/arch/arm/mach-shmobile/hotplug.c @@ -20,6 +20,12 @@ #include <mach/emev2.h> #include <asm/cacheflush.h> #include <asm/mach-types.h> +#include <asm/opcodes.h> + +/* + * Define opcode of the WFI instruction. + */ ++#define __WFI __inst_arm_thumb16(0xe320f003, 0xbf30) static cpumask_t dead_cpus; @@ -39,7 +45,7 @@ void shmobile_cpu_die(unsigned int cpu) /* * here's the WFI */ - asm(".word 0xe320f003\n" + asm(__WFI : : : "memory", "cc");
The current "WFI" opcode definiton causes CPU hot-plug feature fails to work if the kernel is built with CONFIG_THUMB2_KERNEL/CONFIG_CPU_ENDIAN_BE8 being defined. An invalid instruction exception will be generated. Signed-off-by: yangfei.kernel@gmail.com --- arch/arm/mach-exynos/hotplug.c | 8 +++++++- arch/arm/mach-realview/hotplug.c | 8 +++++++- arch/arm/mach-shmobile/hotplug.c | 8 +++++++- 3 files changed, 21 insertions(+), 3 deletions(-) --