diff mbox

[RFC,2/3] ARM: iwmmxt: Port problematic iwmmxt support code to v7/Thumb-2

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

Commit Message

Dave Martin Sept. 7, 2011, 3:59 p.m. UTC
The iwmmxt code contains some code to implement a pseudo-ISB, but
this is not buildable for Thumb-2.

This patch replaces the pseudo-ISB with a real one for Thumb-2
kernels.

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

Comments

Eric Miao Sept. 7, 2011, 4:13 p.m. UTC | #1
On Wed, Sep 7, 2011 at 8:59 AM, Dave Martin <dave.martin@linaro.org> wrote:
> The iwmmxt code contains some code to implement a pseudo-ISB, but
> this is not buildable for Thumb-2.
>
> This patch replaces the pseudo-ISB with a real one for Thumb-2
> kernels.
>
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
>  arch/arm/kernel/iwmmxt.S |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/kernel/iwmmxt.S b/arch/arm/kernel/iwmmxt.S
> index a087838..bee8a74 100644
> --- a/arch/arm/kernel/iwmmxt.S
> +++ b/arch/arm/kernel/iwmmxt.S
> @@ -319,8 +319,17 @@ ENTRY(iwmmxt_task_switch)
>        PJ4(eor r1, r1, #0xf)
>        PJ4(mcr p15, 0, r1, c1, c0, 2)
>
> +/*
> + * When enough people have binutils which support -march=...+iwmmxt, this
> + * should change to #if __LINUX_ARM_ARCH__ < 7.
> + */
> +#ifndef CONFIG_THUMB2_KERNEL
>        mrc     p15, 0, r1, c2, c0, 0
>        sub     pc, lr, r1, lsr #32             @ cpwait and return
> +#else
> +       isb                                     @ ISB needed instead on ARMv7
> +       mov     pc, lr
> +#endif

Or maybe using ARM() and THUMB() will be a better fit here?
Arnd Bergmann Sept. 7, 2011, 8:35 p.m. UTC | #2
On Wednesday 07 September 2011 09:13:38 Eric Miao wrote:
> >
> > +/*
> > + * When enough people have binutils which support -march=...+iwmmxt, this
> > + * should change to #if __LINUX_ARM_ARCH__ < 7.
> > + */
> > +#ifndef CONFIG_THUMB2_KERNEL
> >        mrc     p15, 0, r1, c2, c0, 0
> >        sub     pc, lr, r1, lsr #32             @ cpwait and return
> > +#else
> > +       isb                                     @ ISB needed instead on ARMv7
> > +       mov     pc, lr
> > +#endif
> 
> Or maybe using ARM() and THUMB() will be a better fit here?

I think it would be logical to use XSC() and PJ4() instead, as those are used
elsehere in this file. However, that doesn't work with the only binutils.

If the !THUMB code above is actually correct for v7 in ARM mode (i.e. it performs
the equivalent of an ISB), then I think we should leave it.

It does look like it's whitespace broken though.

	Arnd
Dave Martin Sept. 8, 2011, 8:58 a.m. UTC | #3
On Wed, Sep 07, 2011 at 10:35:46PM +0200, Arnd Bergmann wrote:
> On Wednesday 07 September 2011 09:13:38 Eric Miao wrote:
> > >
> > > +/*
> > > + * When enough people have binutils which support -march=...+iwmmxt, this
> > > + * should change to #if __LINUX_ARM_ARCH__ < 7.
> > > + */
> > > +#ifndef CONFIG_THUMB2_KERNEL
> > >        mrc     p15, 0, r1, c2, c0, 0
> > >        sub     pc, lr, r1, lsr #32             @ cpwait and return
> > > +#else
> > > +       isb                                     @ ISB needed instead on ARMv7
> > > +       mov     pc, lr
> > > +#endif
> > 
> > Or maybe using ARM() and THUMB() will be a better fit here?
> 
> I think it would be logical to use XSC() and PJ4() instead, as those are used
> elsehere in this file. However, that doesn't work with the only binutils.

I take pj4 and older platforms can never be built in the same kernel?
Otherwise, this approach is totally broken anyway-- we'll generate code
which won't run on some of the kernel's supported platforms.

> If the !THUMB code above is actually correct for v7 in ARM mode (i.e. it performs
> the equivalent of an ISB), then I think we should leave it.

With the proposed change to use cc-option in the Makefile,
we have the option of using the v7 code whenever binutils supports
it, and printing a warning otherwise.

> It does look like it's whitespace broken though.

I'll double-check that.

Cheers
---Dave
Russell King - ARM Linux Sept. 8, 2011, 9:01 a.m. UTC | #4
On Thu, Sep 08, 2011 at 09:58:00AM +0100, Dave Martin wrote:
> I take pj4 and older platforms can never be built in the same kernel?
> Otherwise, this approach is totally broken anyway-- we'll generate code
> which won't run on some of the kernel's supported platforms.

We have a well enforced split between ARMv5 and older CPUs, and ARMv6
and later CPUs.
Dave Martin Sept. 8, 2011, 11:33 a.m. UTC | #5
On Thu, Sep 08, 2011 at 10:01:22AM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 08, 2011 at 09:58:00AM +0100, Dave Martin wrote:
> > I take pj4 and older platforms can never be built in the same kernel?
> > Otherwise, this approach is totally broken anyway-- we'll generate code
> > which won't run on some of the kernel's supported platforms.
> 
> We have a well enforced split between ARMv5 and older CPUs, and ARMv6
> and later CPUs.

Good point -- OK, I guess we don't need to worry about that scenario.

Cheers
---Dave
Arnd Bergmann Sept. 8, 2011, 2:40 p.m. UTC | #6
On Thursday 08 September 2011, Dave Martin wrote:
> On Thu, Sep 08, 2011 at 10:01:22AM +0100, Russell King - ARM Linux wrote:
> > On Thu, Sep 08, 2011 at 09:58:00AM +0100, Dave Martin wrote:
> > > I take pj4 and older platforms can never be built in the same kernel?
> > > Otherwise, this approach is totally broken anyway-- we'll generate code
> > > which won't run on some of the kernel's supported platforms.
> > 
> > We have a well enforced split between ARMv5 and older CPUs, and ARMv6
> > and later CPUs.
> 
> Good point -- OK, I guess we don't need to worry about that scenario.

I actually have a patch that enforces this rule in Kconfig for pxa.
Right now, you can happily select any combination of boards, which
blows up very early during the build phase if you combine v5 and v7.

	Arnd
diff mbox

Patch

diff --git a/arch/arm/kernel/iwmmxt.S b/arch/arm/kernel/iwmmxt.S
index a087838..bee8a74 100644
--- a/arch/arm/kernel/iwmmxt.S
+++ b/arch/arm/kernel/iwmmxt.S
@@ -319,8 +319,17 @@  ENTRY(iwmmxt_task_switch)
 	PJ4(eor r1, r1, #0xf)
 	PJ4(mcr	p15, 0, r1, c1, c0, 2)
 
+/*
+ * When enough people have binutils which support -march=...+iwmmxt, this
+ * should change to #if __LINUX_ARM_ARCH__ < 7.
+ */
+#ifndef CONFIG_THUMB2_KERNEL
 	mrc	p15, 0, r1, c2, c0, 0
 	sub	pc, lr, r1, lsr #32		@ cpwait and return
+#else
+	isb					@ ISB needed instead on ARMv7
+	mov	pc, lr
+#endif
 
 /*
  * Remove Concan ownership of given task