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

[Tetsuyuki Kobayashi]

Hello.

(2012/10/08 2:35), Albert ARIBAUD wrote:
> 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.
>
I have the same opinion as Albert.
63ee53a7 is right. It should not be reverted.
lowlevel_init.S in rmobile and omap have mistake, it should be fixed.

I checked 2012.10-rc3. It has already done.
Thank you very much.