Patchwork ARM: mxc: ssi-fiq: Make ssi-fiq.S Thumb-2 compatible

login
register
mail settings
Submitter Dave Martin
Date Aug. 10, 2012, 11:53 a.m.
Message ID <1344599604-9623-1-git-send-email-dave.martin@linaro.org>
Download mbox | patch
Permalink /patch/176470/
State New
Headers show

Comments

Dave Martin - Aug. 10, 2012, 11:53 a.m.
Because FIQ handlers get copied straight into the vectors page to
the FIQ vector entry point, FIQ handlers in a Thumb-2 kernel must
start in Thumb-2.  A Thumb-2 kernel enters all exception vectors in
Thumb-2.

This patch adapts the mxc SSI FIQ code suitable for a Thumb-2
kernel.

The code contained use of r13 (sp) which isn't allowed in Thumb-2.
r11 and r13 have been swapped throughout the file to work around
this.

Currently, the way that the function to be copied is located using
labels is a bit ugly: we cannot annotate the FIQ handler properly
as a Thumb-2 function, because this would set bit 0 of the label
address seen by the linker, causing off-by-one errors when copying
the function.  Ideally, the copy would be done with fncpy(), but
this would require changes to the common set_fiq_handler()
function.  For now, we don't touch this.

References to locally-defined global symbols with adr and ldr may
not be accepted by the assembler in Thumb-2.  Local shadow symbols
are added to work around this.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---

Note that while this code builds, I don't know the hardware well enough 
to be confident testing it.  Review and testing from anyone with
experience of imx SoC audio would be appreciated.

 arch/arm/plat-mxc/ssi-fiq.S |   89 ++++++++++++++++++++++++-------------------
 1 files changed, 50 insertions(+), 39 deletions(-)
Sascha Hauer - Aug. 13, 2012, 7:35 p.m.
Hi Dave,

On Fri, Aug 10, 2012 at 12:53:24PM +0100, Dave Martin wrote:
> Because FIQ handlers get copied straight into the vectors page to
> the FIQ vector entry point, FIQ handlers in a Thumb-2 kernel must
> start in Thumb-2.  A Thumb-2 kernel enters all exception vectors in
> Thumb-2.

I finally came along testing this. I have no Thumb2 capable hardware
to test if it works in thumb2 mode, but at least in Arm mode it works.
This is enough to not introduce a regression, so we can go for this.
I'll add it to my tree with a

Tested-by: Sascha Hauer <s.hauer@pengutronix.de>

Thanks
 Sascha

> 
> This patch adapts the mxc SSI FIQ code suitable for a Thumb-2
> kernel.
> 
> The code contained use of r13 (sp) which isn't allowed in Thumb-2.
> r11 and r13 have been swapped throughout the file to work around
> this.
> 
> Currently, the way that the function to be copied is located using
> labels is a bit ugly: we cannot annotate the FIQ handler properly
> as a Thumb-2 function, because this would set bit 0 of the label
> address seen by the linker, causing off-by-one errors when copying
> the function.  Ideally, the copy would be done with fncpy(), but
> this would require changes to the common set_fiq_handler()
> function.  For now, we don't touch this.
> 
> References to locally-defined global symbols with adr and ldr may
> not be accepted by the assembler in Thumb-2.  Local shadow symbols
> are added to work around this.
> 
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
> 
> Note that while this code builds, I don't know the hardware well enough 
> to be confident testing it.  Review and testing from anyone with
> experience of imx SoC audio would be appreciated.
> 
>  arch/arm/plat-mxc/ssi-fiq.S |   89 ++++++++++++++++++++++++-------------------
>  1 files changed, 50 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/ssi-fiq.S b/arch/arm/plat-mxc/ssi-fiq.S
> index 8397a2d..a8b93c5 100644
> --- a/arch/arm/plat-mxc/ssi-fiq.S
> +++ b/arch/arm/plat-mxc/ssi-fiq.S
> @@ -34,91 +34,98 @@
>  		.global imx_ssi_fiq_rx_buffer
>  		.global imx_ssi_fiq_tx_buffer
>  
> +/*
> + * imx_ssi_fiq_start is _intentionally_ not marked as a function symbol
> + * using ENDPROC().  imx_ssi_fiq_start and imx_ssi_fiq_end are used to
> + * mark the function body so that it can be copied to the FIQ vector in
> + * the vectors page.  imx_ssi_fiq_start should only be called as the result
> + * of an FIQ: calling it directly will not work.
> + */
>  imx_ssi_fiq_start:
> -		ldr r12, imx_ssi_fiq_base
> +		ldr r12, .L_imx_ssi_fiq_base
>  
>  		/* TX */
> -		ldr r11, imx_ssi_fiq_tx_buffer
> +		ldr r13, .L_imx_ssi_fiq_tx_buffer
>  
>  		/* shall we send? */
> -		ldr r13, [r12, #SSI_SIER]
> -		tst r13, #SSI_SIER_TFE0_EN
> +		ldr r11, [r12, #SSI_SIER]
> +		tst r11, #SSI_SIER_TFE0_EN
>  		beq 1f
>  
>  		/* TX FIFO empty? */
> -		ldr r13, [r12, #SSI_SISR]
> -		tst r13, #SSI_SISR_TFE0
> +		ldr r11, [r12, #SSI_SISR]
> +		tst r11, #SSI_SISR_TFE0
>  		beq 1f
>  
>  		mov r10, #0x10000
>  		sub r10, #1
>  		and r10, r10, r8	/* r10: current buffer offset */
>  
> -		add r11, r11, r10
> +		add r13, r13, r10
>  
> -		ldrh r13, [r11]
> -		strh r13, [r12, #SSI_STX0]
> +		ldrh r11, [r13]
> +		strh r11, [r12, #SSI_STX0]
>  
> -		ldrh r13, [r11, #2]
> -		strh r13, [r12, #SSI_STX0]
> +		ldrh r11, [r13, #2]
> +		strh r11, [r12, #SSI_STX0]
>  
> -		ldrh r13, [r11, #4]
> -		strh r13, [r12, #SSI_STX0]
> +		ldrh r11, [r13, #4]
> +		strh r11, [r12, #SSI_STX0]
>  
> -		ldrh r13, [r11, #6]
> -		strh r13, [r12, #SSI_STX0]
> +		ldrh r11, [r13, #6]
> +		strh r11, [r12, #SSI_STX0]
>  
>  		add r10, #8
> -		lsr r13, r8, #16	/* r13: buffer size */
> -		cmp r10, r13
> -		lslgt r8, r13, #16
> +		lsr r11, r8, #16	/* r11: buffer size */
> +		cmp r10, r11
> +		lslgt r8, r11, #16
>  		addle r8, #8
>  1:
>  		/* RX */
>  
>  		/* shall we receive? */
> -		ldr r13, [r12, #SSI_SIER]
> -		tst r13, #SSI_SIER_RFF0_EN
> +		ldr r11, [r12, #SSI_SIER]
> +		tst r11, #SSI_SIER_RFF0_EN
>  		beq 1f
>  
>  		/* RX FIFO full? */
> -		ldr r13, [r12, #SSI_SISR]
> -		tst r13, #SSI_SISR_RFF0
> +		ldr r11, [r12, #SSI_SISR]
> +		tst r11, #SSI_SISR_RFF0
>  		beq 1f
>  
> -		ldr r11, imx_ssi_fiq_rx_buffer
> +		ldr r13, .L_imx_ssi_fiq_rx_buffer
>  
>  		mov r10, #0x10000
>  		sub r10, #1
>  		and r10, r10, r9	/* r10: current buffer offset */
>  
> -		add r11, r11, r10
> +		add r13, r13, r10
>  
> -		ldr r13, [r12, #SSI_SACNT]
> -		tst r13, #SSI_SACNT_AC97EN
> +		ldr r11, [r12, #SSI_SACNT]
> +		tst r11, #SSI_SACNT_AC97EN
>  
> -		ldr r13, [r12, #SSI_SRX0]
> -		strh r13, [r11]
> +		ldr r11, [r12, #SSI_SRX0]
> +		strh r11, [r13]
>  
> -		ldr r13, [r12, #SSI_SRX0]
> -		strh r13, [r11, #2]
> +		ldr r11, [r12, #SSI_SRX0]
> +		strh r11, [r13, #2]
>  
>  		/* dummy read to skip slot 12 */
> -		ldrne r13, [r12, #SSI_SRX0]
> +		ldrne r11, [r12, #SSI_SRX0]
>  
> -		ldr r13, [r12, #SSI_SRX0]
> -		strh r13, [r11, #4]
> +		ldr r11, [r12, #SSI_SRX0]
> +		strh r11, [r13, #4]
>  
> -		ldr r13, [r12, #SSI_SRX0]
> -		strh r13, [r11, #6]
> +		ldr r11, [r12, #SSI_SRX0]
> +		strh r11, [r13, #6]
>  
>  		/* dummy read to skip slot 12 */
> -		ldrne r13, [r12, #SSI_SRX0]
> +		ldrne r11, [r12, #SSI_SRX0]
>  
>  		add r10, #8
> -		lsr r13, r9, #16	/* r13: buffer size */
> -		cmp r10, r13
> -		lslgt r9, r13, #16
> +		lsr r11, r9, #16	/* r11: buffer size */
> +		cmp r10, r11
> +		lslgt r9, r11, #16
>  		addle r9, #8
>  
>  1:
> @@ -126,11 +133,15 @@ imx_ssi_fiq_start:
>  		subs	pc, lr, #4
>  
>  		.align
> +.L_imx_ssi_fiq_base:
>  imx_ssi_fiq_base:
>  		.word 0x0
> +.L_imx_ssi_fiq_rx_buffer:
>  imx_ssi_fiq_rx_buffer:
>  		.word 0x0
> +.L_imx_ssi_fiq_tx_buffer:
>  imx_ssi_fiq_tx_buffer:
>  		.word 0x0
> +.L_imx_ssi_fiq_end:
>  imx_ssi_fiq_end:
>  
> -- 
> 1.7.4.1
> 
>
Matt Sealey - Aug. 13, 2012, 10:28 p.m.
IMX_PIN_REG(MX51_PAD_EIM_DA15, NO_PAD, 0x058, 0, 0x000, 0), /*
MX51_PAD_EIM_DA15__EIM_DA15 */



On Mon, Aug 13, 2012 at 2:35 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Hi Dave,
>
> On Fri, Aug 10, 2012 at 12:53:24PM +0100, Dave Martin wrote:
>> Because FIQ handlers get copied straight into the vectors page to
>> the FIQ vector entry point, FIQ handlers in a Thumb-2 kernel must
>> start in Thumb-2.  A Thumb-2 kernel enters all exception vectors in
>> Thumb-2.
>
> I finally came along testing this. I have no Thumb2 capable hardware
> to test if it works in thumb2 mode, but at least in Arm mode it works.
> This is enough to not introduce a regression, so we can go for this.

This is the interesting dichotomy; the code is only required truly on
devices where Thumb2 isn't available, right?

What we're after here is a build fix more than anything due to an
overzealous, over-inclusive configuration that can't be easily split.
If the ARM code works on the devices it's intended for, excellent. We
can fix the weird inclusions of the code later (and maybe if we're
splitting v6_v7 into just v6 and v7 configs for some other reason, and
all the users go away, maybe we should bump v7 to default to Thumb2?
:)
Sascha Hauer - Aug. 14, 2012, 11:40 a.m.
On Mon, Aug 13, 2012 at 05:28:17PM -0500, Matt Sealey wrote:
>  IMX_PIN_REG(MX51_PAD_EIM_DA15, NO_PAD, 0x058, 0, 0x000, 0), /*
> MX51_PAD_EIM_DA15__EIM_DA15 */
> 
> 
> 
> On Mon, Aug 13, 2012 at 2:35 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > Hi Dave,
> >
> > On Fri, Aug 10, 2012 at 12:53:24PM +0100, Dave Martin wrote:
> >> Because FIQ handlers get copied straight into the vectors page to
> >> the FIQ vector entry point, FIQ handlers in a Thumb-2 kernel must
> >> start in Thumb-2.  A Thumb-2 kernel enters all exception vectors in
> >> Thumb-2.
> >
> > I finally came along testing this. I have no Thumb2 capable hardware
> > to test if it works in thumb2 mode, but at least in Arm mode it works.
> > This is enough to not introduce a regression, so we can go for this.
> 
> This is the interesting dichotomy; the code is only required truly on
> devices where Thumb2 isn't available, right?

It is also used on the i.MX51 Eukrea board. I asked Eric (Cced) to test
it in thumb2 mode, no response so far. I don't know the reason why he
uses FIQ mode instead of SDMA, maybe simply historical reasons.

Sascha
Matt Sealey - Aug. 14, 2012, 5:49 p.m.
On Tue, Aug 14, 2012 at 6:40 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Mon, Aug 13, 2012 at 05:28:17PM -0500, Matt Sealey wrote:
>>  IMX_PIN_REG(MX51_PAD_EIM_DA15, NO_PAD, 0x058, 0, 0x000, 0), /*
>> MX51_PAD_EIM_DA15__EIM_DA15 */
>>
>>
>>
>> On Mon, Aug 13, 2012 at 2:35 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > Hi Dave,
>> >
>> > On Fri, Aug 10, 2012 at 12:53:24PM +0100, Dave Martin wrote:
>> >> Because FIQ handlers get copied straight into the vectors page to
>> >> the FIQ vector entry point, FIQ handlers in a Thumb-2 kernel must
>> >> start in Thumb-2.  A Thumb-2 kernel enters all exception vectors in
>> >> Thumb-2.
>> >
>> > I finally came along testing this. I have no Thumb2 capable hardware
>> > to test if it works in thumb2 mode, but at least in Arm mode it works.
>> > This is enough to not introduce a regression, so we can go for this.
>>
>> This is the interesting dichotomy; the code is only required truly on
>> devices where Thumb2 isn't available, right?
>
> It is also used on the i.MX51 Eukrea board. I asked Eric (Cced) to test
> it in thumb2 mode, no response so far. I don't know the reason why he
> uses FIQ mode instead of SDMA, maybe simply historical reasons.

If we can lose that requirement then the only thing in the way is the
phyCORE support, which goes away on a "pure" v7 config anyway,
correct? I was just thinking, why not split v6_v7_defconfig into a
v7_defconfig losing those boards anyway and enable Thumb2 by default
while we're at it? It would mean more testing gets done and this error
would crop up both more often, but be less surprising and won't take a
year to revisit ;)

Pure v7 config would still include Tegra, OMAP3/4, i.MX5 onwards and
the newer chips. One zImage for v6_v7 is still possible but I don't
know if anyone really has the idea in their heads that you'd want to
boot the same kernel on i.MX3 as well as i.MX6Q even if they do share
drivers to some degree.
Sascha Hauer - Aug. 14, 2012, 8:32 p.m.
On Tue, Aug 14, 2012 at 12:49:43PM -0500, Matt Sealey wrote:
> On Tue, Aug 14, 2012 at 6:40 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Mon, Aug 13, 2012 at 05:28:17PM -0500, Matt Sealey wrote:
> >>  IMX_PIN_REG(MX51_PAD_EIM_DA15, NO_PAD, 0x058, 0, 0x000, 0), /*
> >> MX51_PAD_EIM_DA15__EIM_DA15 */
> >>
> >>
> >>
> >> On Mon, Aug 13, 2012 at 2:35 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > Hi Dave,
> >> >
> >> > On Fri, Aug 10, 2012 at 12:53:24PM +0100, Dave Martin wrote:
> >> >> Because FIQ handlers get copied straight into the vectors page to
> >> >> the FIQ vector entry point, FIQ handlers in a Thumb-2 kernel must
> >> >> start in Thumb-2.  A Thumb-2 kernel enters all exception vectors in
> >> >> Thumb-2.
> >> >
> >> > I finally came along testing this. I have no Thumb2 capable hardware
> >> > to test if it works in thumb2 mode, but at least in Arm mode it works.
> >> > This is enough to not introduce a regression, so we can go for this.
> >>
> >> This is the interesting dichotomy; the code is only required truly on
> >> devices where Thumb2 isn't available, right?
> >
> > It is also used on the i.MX51 Eukrea board. I asked Eric (Cced) to test
> > it in thumb2 mode, no response so far. I don't know the reason why he
> > uses FIQ mode instead of SDMA, maybe simply historical reasons.
> 
> If we can lose that requirement then the only thing in the way is the
> phyCORE support, which goes away on a "pure" v7 config anyway,
> correct? I was just thinking, why not split v6_v7_defconfig into a
> v7_defconfig losing those boards anyway and enable Thumb2 by default
> while we're at it? It would mean more testing gets done and this error
> would crop up both more often, but be less surprising and won't take a
> year to revisit ;)

It took a long time until all people have realized that their code may
not only run on the SoC they are currently working on, but also on other
SoCs. Having many SoCs in a single defconfig makes this fact more
obvious and often breaks compilation if their code is not multi SoC
safe. I don't really want to lose this.

Hopefully we will soon only have a handfull of ARM defconfigs anyway.
Until this is the case I vote for not splitting up defconfigs. Then we
can have things like thumb2_defconfig, armv6_defconfig,...

Sascha

Patch

diff --git a/arch/arm/plat-mxc/ssi-fiq.S b/arch/arm/plat-mxc/ssi-fiq.S
index 8397a2d..a8b93c5 100644
--- a/arch/arm/plat-mxc/ssi-fiq.S
+++ b/arch/arm/plat-mxc/ssi-fiq.S
@@ -34,91 +34,98 @@ 
 		.global imx_ssi_fiq_rx_buffer
 		.global imx_ssi_fiq_tx_buffer
 
+/*
+ * imx_ssi_fiq_start is _intentionally_ not marked as a function symbol
+ * using ENDPROC().  imx_ssi_fiq_start and imx_ssi_fiq_end are used to
+ * mark the function body so that it can be copied to the FIQ vector in
+ * the vectors page.  imx_ssi_fiq_start should only be called as the result
+ * of an FIQ: calling it directly will not work.
+ */
 imx_ssi_fiq_start:
-		ldr r12, imx_ssi_fiq_base
+		ldr r12, .L_imx_ssi_fiq_base
 
 		/* TX */
-		ldr r11, imx_ssi_fiq_tx_buffer
+		ldr r13, .L_imx_ssi_fiq_tx_buffer
 
 		/* shall we send? */
-		ldr r13, [r12, #SSI_SIER]
-		tst r13, #SSI_SIER_TFE0_EN
+		ldr r11, [r12, #SSI_SIER]
+		tst r11, #SSI_SIER_TFE0_EN
 		beq 1f
 
 		/* TX FIFO empty? */
-		ldr r13, [r12, #SSI_SISR]
-		tst r13, #SSI_SISR_TFE0
+		ldr r11, [r12, #SSI_SISR]
+		tst r11, #SSI_SISR_TFE0
 		beq 1f
 
 		mov r10, #0x10000
 		sub r10, #1
 		and r10, r10, r8	/* r10: current buffer offset */
 
-		add r11, r11, r10
+		add r13, r13, r10
 
-		ldrh r13, [r11]
-		strh r13, [r12, #SSI_STX0]
+		ldrh r11, [r13]
+		strh r11, [r12, #SSI_STX0]
 
-		ldrh r13, [r11, #2]
-		strh r13, [r12, #SSI_STX0]
+		ldrh r11, [r13, #2]
+		strh r11, [r12, #SSI_STX0]
 
-		ldrh r13, [r11, #4]
-		strh r13, [r12, #SSI_STX0]
+		ldrh r11, [r13, #4]
+		strh r11, [r12, #SSI_STX0]
 
-		ldrh r13, [r11, #6]
-		strh r13, [r12, #SSI_STX0]
+		ldrh r11, [r13, #6]
+		strh r11, [r12, #SSI_STX0]
 
 		add r10, #8
-		lsr r13, r8, #16	/* r13: buffer size */
-		cmp r10, r13
-		lslgt r8, r13, #16
+		lsr r11, r8, #16	/* r11: buffer size */
+		cmp r10, r11
+		lslgt r8, r11, #16
 		addle r8, #8
 1:
 		/* RX */
 
 		/* shall we receive? */
-		ldr r13, [r12, #SSI_SIER]
-		tst r13, #SSI_SIER_RFF0_EN
+		ldr r11, [r12, #SSI_SIER]
+		tst r11, #SSI_SIER_RFF0_EN
 		beq 1f
 
 		/* RX FIFO full? */
-		ldr r13, [r12, #SSI_SISR]
-		tst r13, #SSI_SISR_RFF0
+		ldr r11, [r12, #SSI_SISR]
+		tst r11, #SSI_SISR_RFF0
 		beq 1f
 
-		ldr r11, imx_ssi_fiq_rx_buffer
+		ldr r13, .L_imx_ssi_fiq_rx_buffer
 
 		mov r10, #0x10000
 		sub r10, #1
 		and r10, r10, r9	/* r10: current buffer offset */
 
-		add r11, r11, r10
+		add r13, r13, r10
 
-		ldr r13, [r12, #SSI_SACNT]
-		tst r13, #SSI_SACNT_AC97EN
+		ldr r11, [r12, #SSI_SACNT]
+		tst r11, #SSI_SACNT_AC97EN
 
-		ldr r13, [r12, #SSI_SRX0]
-		strh r13, [r11]
+		ldr r11, [r12, #SSI_SRX0]
+		strh r11, [r13]
 
-		ldr r13, [r12, #SSI_SRX0]
-		strh r13, [r11, #2]
+		ldr r11, [r12, #SSI_SRX0]
+		strh r11, [r13, #2]
 
 		/* dummy read to skip slot 12 */
-		ldrne r13, [r12, #SSI_SRX0]
+		ldrne r11, [r12, #SSI_SRX0]
 
-		ldr r13, [r12, #SSI_SRX0]
-		strh r13, [r11, #4]
+		ldr r11, [r12, #SSI_SRX0]
+		strh r11, [r13, #4]
 
-		ldr r13, [r12, #SSI_SRX0]
-		strh r13, [r11, #6]
+		ldr r11, [r12, #SSI_SRX0]
+		strh r11, [r13, #6]
 
 		/* dummy read to skip slot 12 */
-		ldrne r13, [r12, #SSI_SRX0]
+		ldrne r11, [r12, #SSI_SRX0]
 
 		add r10, #8
-		lsr r13, r9, #16	/* r13: buffer size */
-		cmp r10, r13
-		lslgt r9, r13, #16
+		lsr r11, r9, #16	/* r11: buffer size */
+		cmp r10, r11
+		lslgt r9, r11, #16
 		addle r9, #8
 
 1:
@@ -126,11 +133,15 @@  imx_ssi_fiq_start:
 		subs	pc, lr, #4
 
 		.align
+.L_imx_ssi_fiq_base:
 imx_ssi_fiq_base:
 		.word 0x0
+.L_imx_ssi_fiq_rx_buffer:
 imx_ssi_fiq_rx_buffer:
 		.word 0x0
+.L_imx_ssi_fiq_tx_buffer:
 imx_ssi_fiq_tx_buffer:
 		.word 0x0
+.L_imx_ssi_fiq_end:
 imx_ssi_fiq_end: