[RFC,1/3] ARM: iwmmxt: Fix Makefile rules for building iwmmxt for Thumb-2

Submitted by Dave Martin on Sept. 7, 2011, 3:59 p.m.

Details

Message ID 1315411158-17479-2-git-send-email-dave.martin@linaro.org
State RFC
Headers show

Commit Message

Dave Martin Sept. 7, 2011, 3:59 p.m.
Because gcc/gas have no sane way to turn on individual CPU
extensions from the command-line, iwmmxt.S was previously built
with -mcpu=iwmmxt.  Unfortunately, this also downgrades the CPU to
v5, with the result that this file fails to build for a Thumb-2
kernel.

New versions of the tools support -march=<base arch>+iwmmxt, and it
seems reasonable to require up-to-date tools when building in
Thumb-2.  So, this patch uses -march=armv7-a+iwmmxt for
CONFIG_THUMB2_KERNEL=y.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/kernel/Makefile |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

Comments

Eric Miao Sept. 7, 2011, 4:10 p.m.
On Wed, Sep 7, 2011 at 8:59 AM, Dave Martin <dave.martin@linaro.org> wrote:
> Because gcc/gas have no sane way to turn on individual CPU
> extensions from the command-line, iwmmxt.S was previously built
> with -mcpu=iwmmxt.  Unfortunately, this also downgrades the CPU to
> v5, with the result that this file fails to build for a Thumb-2
> kernel.
>
> New versions of the tools support -march=<base arch>+iwmmxt, and it
> seems reasonable to require up-to-date tools when building in
> Thumb-2.  So, this patch uses -march=armv7-a+iwmmxt for
> CONFIG_THUMB2_KERNEL=y.
>
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
>  arch/arm/kernel/Makefile |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index f7887dc..8a58339 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -65,7 +65,14 @@ obj-$(CONFIG_CPU_PJ4)                += pj4-cp0.o
>  obj-$(CONFIG_IWMMXT)           += iwmmxt.o
>  obj-$(CONFIG_CPU_HAS_PMU)      += pmu.o
>  obj-$(CONFIG_HW_PERF_EVENTS)   += perf_event.o
> +
> +# When enough people have binutils which support -march=...+iwmmxt, this
> +# should change to something like if __LINUX_ARM_ARCH__ < 7.
> +ifdef CONFIG_THUMB2_KERNEL
> +AFLAGS_iwmmxt.o                        := -Wa,-march=armv7-a+iwmmxt
> +else
>  AFLAGS_iwmmxt.o                        := -Wa,-mcpu=iwmmxt
> +endif

It looks more like the switch should depend on the compiler version.
Unless there is a clear way to decide if gcc supports this switch, I
think it's reasonable to have the change like above.

>
>  ifneq ($(CONFIG_ARCH_EBSA110),y)
>   obj-y                += io.o
> --
> 1.7.4.1
>
>
Nicolas Pitre Sept. 7, 2011, 5:18 p.m.
On Wed, 7 Sep 2011, Eric Miao wrote:

> On Wed, Sep 7, 2011 at 8:59 AM, Dave Martin <dave.martin@linaro.org> wrote:
> > Because gcc/gas have no sane way to turn on individual CPU
> > extensions from the command-line, iwmmxt.S was previously built
> > with -mcpu=iwmmxt.  Unfortunately, this also downgrades the CPU to
> > v5, with the result that this file fails to build for a Thumb-2
> > kernel.
> >
> > New versions of the tools support -march=<base arch>+iwmmxt, and it
> > seems reasonable to require up-to-date tools when building in
> > Thumb-2.  So, this patch uses -march=armv7-a+iwmmxt for
> > CONFIG_THUMB2_KERNEL=y.
> >
> > Signed-off-by: Dave Martin <dave.martin@linaro.org>
> > ---
> >  arch/arm/kernel/Makefile |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> > index f7887dc..8a58339 100644
> > --- a/arch/arm/kernel/Makefile
> > +++ b/arch/arm/kernel/Makefile
> > @@ -65,7 +65,14 @@ obj-$(CONFIG_CPU_PJ4)                += pj4-cp0.o
> >  obj-$(CONFIG_IWMMXT)           += iwmmxt.o
> >  obj-$(CONFIG_CPU_HAS_PMU)      += pmu.o
> >  obj-$(CONFIG_HW_PERF_EVENTS)   += perf_event.o
> > +
> > +# When enough people have binutils which support -march=...+iwmmxt, this
> > +# should change to something like if __LINUX_ARM_ARCH__ < 7.
> > +ifdef CONFIG_THUMB2_KERNEL
> > +AFLAGS_iwmmxt.o                        := -Wa,-march=armv7-a+iwmmxt
> > +else
> >  AFLAGS_iwmmxt.o                        := -Wa,-mcpu=iwmmxt
> > +endif
> 
> It looks more like the switch should depend on the compiler version.
> Unless there is a clear way to decide if gcc supports this switch, I
> think it's reasonable to have the change like above.

Normally the way to go with gcc version dependent alternatives is to use 
something like:

AFLAGS_foo.o := $(call cc-option,<the_new_flag>,<the_fallback_flag>)

This will test if <the_new_flag> is supported by the used gcc, and use 
the fallback otherwise.


Nicolas
Arnd Bergmann Sept. 7, 2011, 8:32 p.m.
On Wednesday 07 September 2011 13:18:21 Nicolas Pitre wrote:
> > > +
> > > +# When enough people have binutils which support -march=...+iwmmxt, this
> > > +# should change to something like if __LINUX_ARM_ARCH__ < 7.
> > > +ifdef CONFIG_THUMB2_KERNEL
> > > +AFLAGS_iwmmxt.o                        := -Wa,-march=armv7-a+iwmmxt
> > > +else
> > >  AFLAGS_iwmmxt.o                        := -Wa,-mcpu=iwmmxt
> > > +endif
> > 
> > It looks more like the switch should depend on the compiler version.
> > Unless there is a clear way to decide if gcc supports this switch, I
> > think it's reasonable to have the change like above.
> 
> Normally the way to go with gcc version dependent alternatives is to use 
> something like:
> 
> AFLAGS_foo.o := $(call cc-option,<the_new_flag>,<the_fallback_flag>)
> 
> This will test if <the_new_flag> is supported by the used gcc, and use 
> the fallback otherwise.

Yes, that's possible here, but it's not actually correct either, because the
CPU core that we are running on is either a v5 XScale with iwmmxt or
a v7 pj4 with iwmmxt. Now, it should not really matter if we build the
code with flags for a different more complex instruction set, but it can
potentially hide bugs.

I think the simple solution that Dave posted is actually more appropriate.
The three possible cases are:

v5+iwmmxt: always use -Wa,-mcpu=iwmmxt as we've always done, and it's correct
v7+iwmmxt+arm: still use -Wa,-mcpu=iwmmxt, not correct but close enough and
               is known to build the file with all existing toolchaings
v7+iwmmxt+thumb2: always use -Wa,-march=armv7-a+iwmmxt, which is correct and
		      the only possible way to build this file anyway. Old toolchains
		      will fail and there is nothing we can do about it.

	Arnd
Dave Martin Sept. 8, 2011, 8:53 a.m.
On Wed, Sep 07, 2011 at 10:32:09PM +0200, Arnd Bergmann wrote:
> On Wednesday 07 September 2011 13:18:21 Nicolas Pitre wrote:
> > > > +
> > > > +# When enough people have binutils which support -march=...+iwmmxt, this
> > > > +# should change to something like if __LINUX_ARM_ARCH__ < 7.
> > > > +ifdef CONFIG_THUMB2_KERNEL
> > > > +AFLAGS_iwmmxt.o                        := -Wa,-march=armv7-a+iwmmxt
> > > > +else
> > > >  AFLAGS_iwmmxt.o                        := -Wa,-mcpu=iwmmxt
> > > > +endif
> > > 
> > > It looks more like the switch should depend on the compiler version.
> > > Unless there is a clear way to decide if gcc supports this switch, I
> > > think it's reasonable to have the change like above.
> > 
> > Normally the way to go with gcc version dependent alternatives is to use 
> > something like:
> > 
> > AFLAGS_foo.o := $(call cc-option,<the_new_flag>,<the_fallback_flag>)
> > 
> > This will test if <the_new_flag> is supported by the used gcc, and use 
> > the fallback otherwise.
> 
> Yes, that's possible here, but it's not actually correct either, because the
> CPU core that we are running on is either a v5 XScale with iwmmxt or
> a v7 pj4 with iwmmxt. Now, it should not really matter if we build the
> code with flags for a different more complex instruction set, but it can
> potentially hide bugs.
> 
> I think the simple solution that Dave posted is actually more appropriate.
> The three possible cases are:
> 
> v5+iwmmxt: always use -Wa,-mcpu=iwmmxt as we've always done, and it's correct
> v7+iwmmxt+arm: still use -Wa,-mcpu=iwmmxt, not correct but close enough and
>                is known to build the file with all existing toolchaings
> v7+iwmmxt+thumb2: always use -Wa,-march=armv7-a+iwmmxt, which is correct and
> 		      the only possible way to build this file anyway. Old toolchains
> 		      will fail and there is nothing we can do about it.

There is another option, which is to use cc-option and then check the
result in AFLAGS_iwmmxt.o, throwing an error from the Makefile as
necessary.

I'll have a go at that if nobody has any objections.

There doesn't seem to be any cleaner way to catch this error -- compiler
version issues are invisible to Kconfig.

Cheers
---Dave

Patch hide | download patch | download mbox

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index f7887dc..8a58339 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -65,7 +65,14 @@  obj-$(CONFIG_CPU_PJ4)		+= pj4-cp0.o
 obj-$(CONFIG_IWMMXT)		+= iwmmxt.o
 obj-$(CONFIG_CPU_HAS_PMU)	+= pmu.o
 obj-$(CONFIG_HW_PERF_EVENTS)	+= perf_event.o
+
+# When enough people have binutils which support -march=...+iwmmxt, this
+# should change to something like if __LINUX_ARM_ARCH__ < 7.
+ifdef CONFIG_THUMB2_KERNEL
+AFLAGS_iwmmxt.o			:= -Wa,-march=armv7-a+iwmmxt
+else
 AFLAGS_iwmmxt.o			:= -Wa,-mcpu=iwmmxt
+endif
 
 ifneq ($(CONFIG_ARCH_EBSA110),y)
   obj-y		+= io.o