> On 19.04.2018, at 10:33, Marek Vasut <ma...@denx.de> wrote: > > On 04/19/2018 10:02 AM, klaus.go...@theobroma-systems.com wrote: >> >>> On 18.04.2018, at 22:02, Marek Vasut <ma...@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.go...@theobroma-systems.com> >>>> Signed-off-by: Christoph Muellner >>>> <christoph.muell...@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
signature.asc
Description: Message signed with OpenPGP
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot