On 04/26/2018 09:33 PM, Dr. Philipp Tomsich wrote: > >> On 26 Apr 2018, at 20:18, Klaus Goger <klaus.go...@theobroma-systems.com> >> 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 r12 to store the link register will fail if the called >> function is using r12 and not preserving it as allowed. > > Maybe you could rephrase this one, as the “not preserving it as allowed” > made me read this twice until I realised that it is equivalent to “does not > preserve (and is not required by the PCS to preserve)”? > >> This can happen in ARM and thumb mode but will definitely be triggered >> by building it in thumb. >> Using r10 should be safe in any case as we are startup code and not >> called by any C-function and no stack is set up. >> >> Signed-off-by: Klaus Goger <klaus.go...@theobroma-systems.com> >> Signed-off-by: Christoph Muellner <christoph.muell...@theobroma-systems.com> > > Reviewed-by: Philipp Tomsich <philipp.toms...@theobroma-systems.com>
I'd prefer to see a review from someone not from the same company :) >> --- >> >> Changes in v3: >> - reword commit message as it isn't thumb specific >> - use r10 instead of sl alias as we don't use it as stack limit pointer >> - revert return to a pc mov as it is a unnecessary change for this patch >> >> 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 | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S >> index 959d1ed86d..a625b39787 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 r10, lr /* perserve link reg across call */ >> bl lowlevel_init /* go setup pll,mux,memory */ >> - mov lr, ip /* restore link */ >> + mov lr, r10 /* restore link */ >> #endif >> mov pc, lr /* back to my caller */ >> #endif /* CONFIG_SKIP_LOWLEVEL_INIT */ >> -- >> 2.11.0 >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> https://lists.denx.de/listinfo/u-boot > -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
