[U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init

[Albert ARIBAUD]

Hi Albert,

On Sun, 7 Oct 2012 19:21:27 +0200, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:

> Hi Albert,
> 
> On Sun, 7 Oct 2012 19:19:37 +0200, Albert ARIBAUD
> <albert.u.boot at aribaud.net> wrote:
> 
> > Hi Jeroen,
> > 
> > On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee
> > <dasuboot at myspectrum.nl> wrote:
> > 
> > > Hello All,
> > > 
> > > On 10/07/2012 01:34 PM, Enric Balletbò i Serra wrote:
> > > > Hi Albert,
> > > >
> > > > 2012/10/5 Albert ARIBAUD <albert.u.boot at aribaud.net>:
> > > >> Hi Tetsuyuki,
> > > >>
> > > >> On Fri,  5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi
> > > >> <koba at kmckk.co.jp> wrote:
> > > >>
> > > >>> lowlevel_init() of rmobile badly assumed that ip register holds return address.
> > > >>> The commit "63ee53a7 armv7 cpu_init_crit: Simplify code" breaks this assumption.
> > > >>> This patch removes this bad assumption and simplify code.
> > > >>>
> > > >>> Signed-off-by: Tetsuyuki Kobayashi <koba at kmckk.co.jp>
> > > >>> ---
> > > >>>
> > > >> ...
> > > > Note that the patch that Tetsuyuki says also breaks SPL support for
> > > > OMAP3 boards, at least my IGEP boards doesn't boot and hangs at SPL
> > > > level.
> > > >
> > > >    U-Boot SPL 2012.10-rc1-00244-g28e5ac2 (Oct 07 2012 - 13:11:29)
> > > >
> > > > Bisecting the problem I encountered the problem is the commit
> > > > "63ee53a7 armv7 cpu_init_crit: Simplify code".
> > > >
> > > > Cheers,
> > > >      Enric
> > > >
> > > I can confirm above. Also the tam3517 som (omap3) fails to boot due to
> > > mentioned commit. The patch from Tetsuyuki is arch specific (rmobile) so
> > > that won't fix the omap case. Reverting the patch, 63ee53a, does help.
> > > 
> > > Is there anything against reverting the patch (at least for the release...)?
> > 
> > Here is my opinion:
> > 
> > 1) I think patch 63ee53a7 is right in considering there is no need for
> > cpu_init_crit to save lr in ip before calling lowlevel_init especially
> > considering this is a tail call.
> > 
> > Only lowlevel_init can tell if it uses ip or lr for its own purposes,
> > thus any saving of ip and/or lr due to the workings of lowlevel_init
> > should be performed in lowlevel_init.
> > 
> > 2) I am not sure that the patch in this discussion depends on 63ee53a7,
> > because IIUC, the patch simply saves ip "on a stack" then lr into ip,
> > and after running s_init, restores from ip and ip from the stack; it
> > never assumes ip contains a return address.
> > 
> > I know we're that close to the release, but I want to be sure we
> > understand what needs fixing. Kobayashi, Jeroen, can you indicate
> > precisely how the issues you encounter are related to 63ee53a7?
> 
> (adding back Tetsuyuki's mail in the Cc: list -- why had it
> disappeared?)
> 
> > > Regards,
> > > Jeroen
> > 
> > Amicalement,
> 
> Amicalement,

Hmm... I notice only now that I had mentally 'fixed' the order of the
restoring lines removed by the patch. Had they been in the right order
(mov lr, ip then ldr ip, [sp]) the original code would have worked,
albeit probably useless.

I suspect that the bad ordering was actually a mistake unseen, and
that the dependence on ip being a return address is only due to this
mistake.

In any case, this makes me *more* determined that 63ee53a7 is right,as
well as this patch.

Jeroen, I suspect that your problem comes from the fact that the same
bug that this patch uncovers and fixes exists also in

arch/arm/cpu/armv7/omap3/lowlevel_init.S (lines 216-218 and 228-229)

... and would be better fixed there than by reverting 63ee53a7.

Amicalement,
-- 
Albert.