[U-Boot] [PATCH v2 1/2] arm: make arm926ejs startup code thumb compatible

[Christoph Müllner]

> On 21 Apr 2018, at 15:10, Måns Rullgård <mans at mansr.com> wrote:
> 
> Klaus Goger <klaus.goger at theobroma-systems.com <mailto:klaus.goger at theobroma-systems.com>> writes:
> 
>> 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>
> 
> This problem isn't specific to Thumb mode.  An ARM build would also
> break if the lowlevel_init function happened to clobber r12, which it is
> permitted to do.  It's just dumb luck that this hasn't happened yet.

True. Since we are in low-level code and are not called by any C-functions
(we are startup code), using sl/r10 is ok.

> 
>> ---
>> 
>> Changes in v2:
>> - use bl instead of blx to call lowlevel_init
>> - remove mxs tag as it apply to all arm926ejs platforms
>> 
>> arch/arm/cpu/arm926ejs/start.S | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
>> index 959d1ed86d..317df5c401 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 */
>> +	mov	sl, lr		/* perserve link reg across call */
>> 	bl	lowlevel_init	/* go setup pll,mux,memory */
>> -	mov	lr, ip		/* restore link */
>> +	mov	lr, sl		/* restore link */
> 
> I prefer to use plain register names (r10) rather than the aliases (sl)
> when not using them for the special functions indicated by the latter.

Ok, will change that.

> 
>> #endif
>> -	mov	pc, lr		/* back to my caller */
>> +	bx	lr		/* back to my caller */
> 
> This change seems unrelated.  Yes, bx is the preferred instruction, but
> using mov here isn't breaking anything.  If it bothers you, feel free to
> make a separate patch fixing all the instances of mov to the pc
> register, not just this one.

We’ll remove it from the patch.

Thanks,
Christoph
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 874 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180421/acafbc3b/attachment.sig>