| Message ID | 1344599604-9623-1-git-send-email-dave.martin@linaro.org |
|---|---|
| State | Accepted |
| Commit | 7095b95654302ceb8a011ee883d1500851061b0d |
| Headers | show |
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 > >
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? :)
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
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.
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
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:
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(-)