| Message ID | 20180421151110.20423-2-mans@mansr.com |
|---|---|
| State | Accepted |
| Commit | da372af6e40d84e8a47baa41d22dd3e953469b74 |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [U-Boot,1/5] ARM: mxs: make lowlevel_init() weak | expand |
On 21/04/2018 17:11, Mans Rullgard wrote: > The code attempts to preserve the value of LR by storing it in R12/IP > across the lowevel_init() call. However, this register is not saved > by the callee. Use a register that guaranteed to be preserved instead. > > Signed-off-by: Mans Rullgard <mans@mansr.com> > --- > 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 959d1ed86d8a..a6f0bdb70345 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 r4, lr /* perserve link reg across call */ > bl lowlevel_init /* go setup pll,mux,memory */ > - mov lr, ip /* restore link */ > + mov lr, r4 /* restore link */ > #endif > mov pc, lr /* back to my caller */ > #endif /* CONFIG_SKIP_LOWLEVEL_INIT */ > Applied to u-boot-imx, thanks ! Best regards, Stefano Babic
Hi Mans, Stefano, On Fri, Apr 27, 2018 at 9:00 PM Stefano Babic <sbabic@denx.de> wrote: > On 21/04/2018 17:11, Mans Rullgard wrote: > > The code attempts to preserve the value of LR by storing it in R12/IP > > across the lowevel_init() call. However, this register is not saved > > by the callee. Use a register that guaranteed to be preserved instead. > > > > Signed-off-by: Mans Rullgard <mans@mansr.com> > > --- > > 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 959d1ed86d8a..a6f0bdb70345 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 r4, lr /* perserve link reg across call */ > > bl lowlevel_init /* go setup pll,mux,memory */ > > - mov lr, ip /* restore link */ > > + mov lr, r4 /* restore link */ > > #endif > > mov pc, lr /* back to my caller */ > > #endif /* CONFIG_SKIP_LOWLEVEL_INIT */ > > > Applied to u-boot-imx, thanks ! I think this might be causing me a problem on a Marvell Kirkwood board I'm working on getting into upstream. It may also be problematic for orion5x boards. Both of these use r4 in lowlevel_init. Obviously I can fix the board that I'm working on. Is there some expectation as to which registers can be clobbered?
> On 07.05.2018, at 10:25, Chris Packham <judge.packham@gmail.com> wrote: > > Hi Mans, Stefano, > > On Fri, Apr 27, 2018 at 9:00 PM Stefano Babic <sbabic@denx.de> wrote: > >> On 21/04/2018 17:11, Mans Rullgard wrote: >>> The code attempts to preserve the value of LR by storing it in R12/IP >>> across the lowevel_init() call. However, this register is not saved >>> by the callee. Use a register that guaranteed to be preserved instead. >>> >>> Signed-off-by: Mans Rullgard <mans@mansr.com> >>> --- >>> 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 959d1ed86d8a..a6f0bdb70345 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 r4, lr /* perserve link reg across call */ >>> bl lowlevel_init /* go setup pll,mux,memory */ >>> - mov lr, ip /* restore link */ >>> + mov lr, r4 /* restore link */ >>> #endif >>> mov pc, lr /* back to my caller */ >>> #endif /* CONFIG_SKIP_LOWLEVEL_INIT */ >>> > >> Applied to u-boot-imx, thanks ! > > I think this might be causing me a problem on a Marvell Kirkwood board I'm > working on getting into upstream. It may also be problematic for orion5x > boards. Both of these use r4 in lowlevel_init. > > Obviously I can fix the board that I'm working on. Is there some > expectation as to which registers can be clobbered? The "Procedure Call Standard for the ARM® Architecture” may help: http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf Page 15 A subroutine must preserve the contents of the registers r4-r8, r10, r11 and SP (and r9 in PCS variants that designate r9 as v6). So for thumb r1-r3 and r12 should be usable without taking any care of them. Maybe r13 depending if you already have a stack or not. Regards, Klaus
Chris Packham <judge.packham@gmail.com> writes: > Hi Mans, Stefano, > > On Fri, Apr 27, 2018 at 9:00 PM Stefano Babic <sbabic@denx.de> wrote: > >> On 21/04/2018 17:11, Mans Rullgard wrote: >> > The code attempts to preserve the value of LR by storing it in R12/IP >> > across the lowevel_init() call. However, this register is not saved >> > by the callee. Use a register that guaranteed to be preserved instead. >> > >> > Signed-off-by: Mans Rullgard <mans@mansr.com> >> > --- >> > 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 959d1ed86d8a..a6f0bdb70345 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 r4, lr /* perserve link reg across call */ >> > bl lowlevel_init /* go setup pll,mux,memory */ >> > - mov lr, ip /* restore link */ >> > + mov lr, r4 /* restore link */ >> > #endif >> > mov pc, lr /* back to my caller */ >> > #endif /* CONFIG_SKIP_LOWLEVEL_INIT */ >> > > >> Applied to u-boot-imx, thanks ! > > I think this might be causing me a problem on a Marvell Kirkwood board I'm > working on getting into upstream. It may also be problematic for orion5x > boards. Both of these use r4 in lowlevel_init. I've just sent an untested patch for orion5x.
On Mon, May 7, 2018 at 10:11 PM Måns Rullgård <mans@mansr.com> wrote: > Chris Packham <judge.packham@gmail.com> writes: > > Hi Mans, Stefano, > > > > On Fri, Apr 27, 2018 at 9:00 PM Stefano Babic <sbabic@denx.de> wrote: > > > >> On 21/04/2018 17:11, Mans Rullgard wrote: > >> > The code attempts to preserve the value of LR by storing it in R12/IP > >> > across the lowevel_init() call. However, this register is not saved > >> > by the callee. Use a register that guaranteed to be preserved instead. > >> > > >> > Signed-off-by: Mans Rullgard <mans@mansr.com> > >> > --- > >> > 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 959d1ed86d8a..a6f0bdb70345 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 r4, lr /* perserve link reg across call */ > >> > bl lowlevel_init /* go setup pll,mux,memory */ > >> > - mov lr, ip /* restore link */ > >> > + mov lr, r4 /* restore link */ > >> > #endif > >> > mov pc, lr /* back to my caller */ > >> > #endif /* CONFIG_SKIP_LOWLEVEL_INIT */ > >> > > > > >> Applied to u-boot-imx, thanks ! > > > > I think this might be causing me a problem on a Marvell Kirkwood board I'm > > working on getting into upstream. It may also be problematic for orion5x > > boards. Both of these use r4 in lowlevel_init. > I've just sent an untested patch for orion5x. Thanks for that.
Hi Klaus, On Mon, May 7, 2018 at 8:48 PM <klaus.goger@theobroma-systems.com> wrote: > > On 07.05.2018, at 10:25, Chris Packham <judge.packham@gmail.com> wrote: > > > > Hi Mans, Stefano, > > > > On Fri, Apr 27, 2018 at 9:00 PM Stefano Babic <sbabic@denx.de> wrote: > > > >> On 21/04/2018 17:11, Mans Rullgard wrote: > >>> The code attempts to preserve the value of LR by storing it in R12/IP > >>> across the lowevel_init() call. However, this register is not saved > >>> by the callee. Use a register that guaranteed to be preserved instead. > >>> > >>> Signed-off-by: Mans Rullgard <mans@mansr.com> > >>> --- > >>> 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 959d1ed86d8a..a6f0bdb70345 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 r4, lr /* perserve link reg across call */ > >>> bl lowlevel_init /* go setup pll,mux,memory */ > >>> - mov lr, ip /* restore link */ > >>> + mov lr, r4 /* restore link */ > >>> #endif > >>> mov pc, lr /* back to my caller */ > >>> #endif /* CONFIG_SKIP_LOWLEVEL_INIT */ > >>> > > > >> Applied to u-boot-imx, thanks ! > > > > I think this might be causing me a problem on a Marvell Kirkwood board I'm > > working on getting into upstream. It may also be problematic for orion5x > > boards. Both of these use r4 in lowlevel_init. > > > > Obviously I can fix the board that I'm working on. Is there some > > expectation as to which registers can be clobbered? > The "Procedure Call Standard for the ARM® Architecture” may help: http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf > Page 15 > A subroutine must preserve the contents of the registers r4-r8, r10, r11 and > SP (and r9 in PCS variants that designate r9 as v6). > So for thumb r1-r3 and r12 should be usable without taking any care of them. > Maybe r13 depending if you already have a stack or not. Thanks. I figured there was probably some document that described this. I started looking at EABI docs but didn't find anything last night.
diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S index 959d1ed86d8a..a6f0bdb70345 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 r4, lr /* perserve link reg across call */ bl lowlevel_init /* go setup pll,mux,memory */ - mov lr, ip /* restore link */ + mov lr, r4 /* restore link */ #endif mov pc, lr /* back to my caller */ #endif /* CONFIG_SKIP_LOWLEVEL_INIT */
The code attempts to preserve the value of LR by storing it in R12/IP across the lowevel_init() call. However, this register is not saved by the callee. Use a register that guaranteed to be preserved instead. Signed-off-by: Mans Rullgard <mans@mansr.com> --- arch/arm/cpu/arm926ejs/start.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)