| Message ID | 1349411962-29202-1-git-send-email-koba@kmckk.co.jp |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Tetsuyuki, On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi <koba@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@kmckk.co.jp> > --- > Hello, Albert > Hello, Tom > Thank you for merging my patches. > > I checked current u-boot-master and found kzm9g board fails to boot because bug. > This bugfix patch solves it. > I think 2012.10 release should have this bugfix. > > Iwamatsu-san, could you double check this patch? > > > arch/arm/cpu/armv7/rmobile/lowlevel_init.S | 12 +----------- > 1 file changed, 1 insertion(+), 11 deletions(-) > > diff --git a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S > index 1bd391e..4fdca06 100644 > --- a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S > +++ b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S > @@ -76,17 +76,7 @@ loop0: > bne loop0 > > ldr sp, MERAM_STACK Is this load of sp still needed? > - str ip, [sp] /* stash old link register */ > - mov ip, lr /* save link reg across call */ > - > - bl s_init > - > - ldr ip, [sp] /* restore save ip */ > - mov lr, ip /* restore link reg */ > - > - /* back to arch calling code */ > - mov pc, lr > + b s_init > > .pool > .align 4 Amicalement,
Hi Albert, 2012/10/5 Albert ARIBAUD <albert.u.boot@aribaud.net>: > Hi Tetsuyuki, > > On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi > <koba@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@kmckk.co.jp> >> --- >> Hello, Albert >> Hello, Tom >> Thank you for merging my patches. >> >> I checked current u-boot-master and found kzm9g board fails to boot because bug. >> This bugfix patch solves it. >> I think 2012.10 release should have this bugfix. >> >> Iwamatsu-san, could you double check this patch? >> >> >> arch/arm/cpu/armv7/rmobile/lowlevel_init.S | 12 +----------- >> 1 file changed, 1 insertion(+), 11 deletions(-) >> >> diff --git a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S >> index 1bd391e..4fdca06 100644 >> --- a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S >> +++ b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S >> @@ -76,17 +76,7 @@ loop0: >> bne loop0 >> >> ldr sp, MERAM_STACK > > Is this load of sp still needed? > >> - str ip, [sp] /* stash old link register */ >> - mov ip, lr /* save link reg across call */ >> - >> - bl s_init >> - >> - ldr ip, [sp] /* restore save ip */ >> - mov lr, ip /* restore link reg */ >> - >> - /* back to arch calling code */ >> - mov pc, lr >> + b s_init >> >> .pool >> .align 4 > > Amicalement, > -- > Albert. > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot 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
Hello All, On 10/07/2012 01:34 PM, Enric Balletbò i Serra wrote: > Hi Albert, > > 2012/10/5 Albert ARIBAUD <albert.u.boot@aribaud.net>: >> Hi Tetsuyuki, >> >> On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi >> <koba@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@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...)? Regards, Jeroen
Hi Jeroen, On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee <dasuboot@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@aribaud.net>: > >> Hi Tetsuyuki, > >> > >> On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi > >> <koba@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@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? > Regards, > Jeroen Amicalement,
Hi Albert, On Sun, 7 Oct 2012 19:19:37 +0200, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hi Jeroen, > > On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee > <dasuboot@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@aribaud.net>: > > >> Hi Tetsuyuki, > > >> > > >> On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi > > >> <koba@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@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,
Hi Albert, On Sun, 7 Oct 2012 19:21:27 +0200, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hi Albert, > > On Sun, 7 Oct 2012 19:19:37 +0200, Albert ARIBAUD > <albert.u.boot@aribaud.net> wrote: > > > Hi Jeroen, > > > > On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee > > <dasuboot@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@aribaud.net>: > > > >> Hi Tetsuyuki, > > > >> > > > >> On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi > > > >> <koba@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@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,
On Thu, Oct 04, 2012 at 06:39:22PM -0000, Tetsuyuki Kobayashi 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@kmckk.co.jp> Applied to u-boot/master, thanks!
Hi Albert, sorry for late response. (2012/10/06 1:23), Albert ARIBAUD wrote: > Hi Tetsuyuki, > > On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi > <koba@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@kmckk.co.jp> >> --- >> Hello, Albert >> Hello, Tom >> Thank you for merging my patches. >> >> I checked current u-boot-master and found kzm9g board fails to boot because bug. >> This bugfix patch solves it. >> I think 2012.10 release should have this bugfix. >> >> Iwamatsu-san, could you double check this patch? >> >> >> arch/arm/cpu/armv7/rmobile/lowlevel_init.S | 12 +----------- >> 1 file changed, 1 insertion(+), 11 deletions(-) >> >> diff --git a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S >> index 1bd391e..4fdca06 100644 >> --- a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S >> +++ b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S >> @@ -76,17 +76,7 @@ loop0: >> bne loop0 >> >> ldr sp, MERAM_STACK > > Is this load of sp still needed? Yes. At this point sp is not yet initialized and s_init() uses stack area. > >> - str ip, [sp] /* stash old link register */ >> - mov ip, lr /* save link reg across call */ >> - >> - bl s_init >> - >> - ldr ip, [sp] /* restore save ip */ >> - mov lr, ip /* restore link reg */ >> - >> - /* back to arch calling code */ >> - mov pc, lr >> + b s_init >> >> .pool >> .align 4
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@aribaud.net> wrote: > >> Hi Albert, >> >> On Sun, 7 Oct 2012 19:19:37 +0200, Albert ARIBAUD >> <albert.u.boot@aribaud.net> wrote: >> >>> Hi Jeroen, >>> >>> On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee >>> <dasuboot@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@aribaud.net>: >>>>>> Hi Tetsuyuki, >>>>>> >>>>>> On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi >>>>>> <koba@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@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.
diff --git a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S index 1bd391e..4fdca06 100644 --- a/arch/arm/cpu/armv7/rmobile/lowlevel_init.S +++ b/arch/arm/cpu/armv7/rmobile/lowlevel_init.S @@ -76,17 +76,7 @@ loop0: bne loop0 ldr sp, MERAM_STACK - - str ip, [sp] /* stash old link register */ - mov ip, lr /* save link reg across call */ - - bl s_init - - ldr ip, [sp] /* restore save ip */ - mov lr, ip /* restore link reg */ - - /* back to arch calling code */ - mov pc, lr + b s_init .pool .align 4
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@kmckk.co.jp> --- Hello, Albert Hello, Tom Thank you for merging my patches. I checked current u-boot-master and found kzm9g board fails to boot because bug. This bugfix patch solves it. I think 2012.10 release should have this bugfix. Iwamatsu-san, could you double check this patch? arch/arm/cpu/armv7/rmobile/lowlevel_init.S | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-)