[PATCH] ARM: Fix the "WFI" instruction opcode definition.

[PATCH] ARM: Fix the "WFI" instruction opcode definition.

[Yangfei (Felix)]

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 at 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
 		    :
 		    :
 		    : "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");
--

[PATCH] ARM: Fix the "WFI" instruction opcode definition.

[Rob Herring]

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 at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH] ARM: Fix the "WFI" instruction opcode definition.

[Fei Yang]

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

[PATCH] ARM: Fix the "WFI" instruction opcode definition.

[Dave Martin]

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

[PATCH] ARM: Fix the "WFI" instruction opcode definition.

[Kukjin Kim]

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.

[PATCH] ARM: Fix the "WFI" instruction opcode definition.

[Dave Martin]

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

[PATCH] ARM: Fix the "WFI" instruction opcode definition.

[Catalin Marinas]

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.

-- 
Catalin