[U-Boot] [PATCH v1] arm: mxs: make startup code thumb compatible

[Christoph Müllner]

> On 19.04.2018, at 10:33, Marek Vasut <marex at denx.de> wrote:
> 
> On 04/19/2018 10:02 AM, klaus.goger at theobroma-systems.com wrote:
>> 
>>> On 18.04.2018, at 22:02, Marek Vasut <marex at denx.de> wrote:
>>> 
>>> On 04/18/2018 06:25 PM, Klaus Goger wrote:
>>>> When building the mxs platform in thumb mode gcc generates code using
>>>> the intra procedure call scratch register (ip/r12) for the calling the
>>>> lowlevel_init function. This modifies the lr in flush_dcache which
>>>> causes u-boot proper to end in an endless loop.
>>>> 
>>>> 40002334:       e1a0c00e        mov     ip, lr
>>>> 40002338:       eb00df4c        bl      4003a070
>>>> <__lowlevel_init_from_arm>
>>>> 4000233c:       e1a0e00c        mov     lr, ip
>>>> 40002340:       e1a0f00e        mov     pc, lr
>>>> [...]
>>>> 4003a070 <__lowlevel_init_from_arm>:
>>>> 4003a070:       e59fc004        ldr     ip, [pc, #4]    ; 4003a07c
>>>> <__lowlevel_init_from_arm+0xc>
>>>> 4003a074:       e08fc00c        add     ip, pc, ip
>>>> 4003a078:       e12fff1c        bx      ip
>>>> 4003a07c:       fffc86cd        .word   0xfffc86cd
>>>> 
>>>> Instead of using the the ip/r12 register we use sl/r10 to preserve the
>>>> link register.
>>>> 
>>>> According to "Procedure Call Standard for the ARM Architecture" by ARM
>>>> subroutines have to preserve the contents of register r4-r8, r10, r11
>>>> and SP. So using r10 instead of r12 should be save.
>>>> 
>>>> Signed-off-by: Klaus Goger <klaus.goger at theobroma-systems.com>
>>>> Signed-off-by: Christoph Muellner <christoph.muellner at theobroma-systems.com>
>>>> 
>>>> ---
>>>> 
>>>> arch/arm/cpu/arm926ejs/start.S | 8 ++++----
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>> 
>>>> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
>>>> index 959d1ed86d..103bd15914 100644
>>>> --- a/arch/arm/cpu/arm926ejs/start.S
>>>> +++ b/arch/arm/cpu/arm926ejs/start.S
>>>> @@ -105,9 +105,9 @@ flush_dcache:
>>>> 	/*
>>>> 	 * Go setup Memory and board specific bits prior to relocation.
>>>> 	 */
>>>> -	mov	ip, lr		/* perserve link reg across call */
>>>> -	bl	lowlevel_init	/* go setup pll,mux,memory */
>>>> -	mov	lr, ip		/* restore link */
>>>> +	mov	sl, lr		/* perserve link reg across call */
>>>> +	blx	lowlevel_init	/* go setup pll,mux,memory */
>>>> +	mov	lr, sl		/* restore link */
>>>> #endif
>>>> -	mov	pc, lr		/* back to my caller */
>>>> +	bx	lr		/* back to my caller */
>>>> #endif /* CONFIG_SKIP_LOWLEVEL_INIT */
>>> 
>>> Are you sure this doesn't break the non-thumb operation ?
>> 
>> Yes I’m sure. I also tested thumb and non-thumb builds on the same device with
>> that patch. Both give you a proper u-boot shell.
>> 
>> I think there will be more patches down the road fixing other thumb/non-thumb issues
>> (like booting a kernel). But they are not related to that patchset.
> 
> Can lowlevel_init be ARM code if U-Boot is compiled in Thumb mode ? I
> guess it can if lowlevel_init is assembler code, in which case doing
> blx/bx to it would call it in Thumb mode and crash ?

It works, because for the tested mxs lowlevel_init is defined as follows:
arch/arm/cpu/arm926ejs/mxs/mxs.c:void lowlevel_init(void) {}
So the compiler fixes this for us, by generating the following code:

> 40002308 <flush_dcache>:
> ...
> 40002334:       e1a0a00e        mov     sl, lr
> 40002338:       eb014df6        bl      40055b18 <__lowlevel_init_from_arm>
> 4000233c:       e1a0e00a        mov     lr, sl
> 40002340:       e1a0f00e        mov     pc, lr

the compiler generated arm-to-thumb helper:

> 40055b18 <__lowlevel_init_from_arm>:
> 40055b18:       e59fc004        ldr     ip, [pc, #4]    ; 40055b24 <__lowlevel_init_from_arm+0xc>
> 40055b1c:       e08fc00c        add     ip, pc, ip
> 40055b20:       e12fff1c        bx      ip
> 40055b24:       fffacd2d        .word   0xfffacd2d


and the thumb compiled lowlevel_init:

> 40002850 <lowlevel_init>:
> 40002850:       4770            bx      lr


So it works for mxs, but it will break other arm926ejs boards,
which implement lowlevel_init in assembly.

I've just tested and the exactly same code as above it generated if we
use "bl lowlevel_init" and compile it with thumb. So we will update
the patch to exclude that changed line.

Thanks,
Christoph



> 
> I might be wrong though
> 
>>> Also, is this really MXS specific or not ?
>> 
>> My error, shouldn’t have tagged it mxs as it’s for all arm926ejs platforms.
>> 
>> I guess arm920t, arm946es, arm720t, arm1136 and omap3 will have the same issue as
>> it has more or less verbatim copies of these functions.
>> But do the lack of hardware I could not test it.
> 
> 
> --
> Best regards,
> Marek Vasut

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 874 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180419/5cbf6e6a/attachment.sig>