Re: [U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init
Hello. (2012/10/08 2:35), Albert ARIBAUD wrote: Hi Albert, On Sun, 7 Oct 2012 19:21:27 +0200, Albert ARIBAUD wrote: Hi Albert, On Sun, 7 Oct 2012 19:19:37 +0200, Albert ARIBAUD wrote: Hi Jeroen, On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee wrote: Hello All, On 10/07/2012 01:34 PM, Enric Balletbò i Serra wrote: Hi Albert, 2012/10/5 Albert ARIBAUD : Hi Tetsuyuki, On Fri, 5 Oct 2012 13:39:22 +0900, 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 --- ... 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init
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 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 --- 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init
Hi Albert, On Sun, 7 Oct 2012 19:21:27 +0200, Albert ARIBAUD wrote: > Hi Albert, > > On Sun, 7 Oct 2012 19:19:37 +0200, Albert ARIBAUD > wrote: > > > Hi Jeroen, > > > > On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee > > wrote: > > > > > Hello All, > > > > > > On 10/07/2012 01:34 PM, Enric Balletbò i Serra wrote: > > > > Hi Albert, > > > > > > > > 2012/10/5 Albert ARIBAUD : > > > >> Hi Tetsuyuki, > > > >> > > > >> On Fri, 5 Oct 2012 13:39:22 +0900, 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 > > > >>> --- > > > >>> > > > >> ... > > > > 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init
Hi Albert, On Sun, 7 Oct 2012 19:19:37 +0200, Albert ARIBAUD wrote: > Hi Jeroen, > > On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee > wrote: > > > Hello All, > > > > On 10/07/2012 01:34 PM, Enric Balletbò i Serra wrote: > > > Hi Albert, > > > > > > 2012/10/5 Albert ARIBAUD : > > >> Hi Tetsuyuki, > > >> > > >> On Fri, 5 Oct 2012 13:39:22 +0900, 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 > > >>> --- > > >>> > > >> ... > > > 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, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init
Hi Jeroen, On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee wrote: > Hello All, > > On 10/07/2012 01:34 PM, Enric Balletbò i Serra wrote: > > Hi Albert, > > > > 2012/10/5 Albert ARIBAUD : > >> Hi Tetsuyuki, > >> > >> On Fri, 5 Oct 2012 13:39:22 +0900, 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 > >>> --- > >>> > >> ... > > 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, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init
Hello All, On 10/07/2012 01:34 PM, Enric Balletbò i Serra wrote: Hi Albert, 2012/10/5 Albert ARIBAUD : Hi Tetsuyuki, On Fri, 5 Oct 2012 13:39:22 +0900, 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 --- ... 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init
Hi Albert, 2012/10/5 Albert ARIBAUD : > Hi Tetsuyuki, > > On Fri, 5 Oct 2012 13:39:22 +0900, 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 >> --- >> 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init
Hi Tetsuyuki, On Fri, 5 Oct 2012 13:39:22 +0900, 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 > --- > 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
[U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init
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 --- 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 - - 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 -- 1.7.9.5 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot