Re: [U-Boot] [PATCH] arm: rmobile: bugfix: wrong register saving in lowlevel_init
Hi Albert, 2012/10/5 Albert ARIBAUD albert.u.b...@aribaud.net: Hi Tetsuyuki, On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi k...@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 k...@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 ___ 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 albert.u.b...@aribaud.net: Hi Tetsuyuki, On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi k...@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 k...@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 ___ 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 dasub...@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.b...@aribaud.net: Hi Tetsuyuki, On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi k...@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 k...@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, -- 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 albert.u.b...@aribaud.net wrote: Hi Jeroen, On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee dasub...@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.b...@aribaud.net: Hi Tetsuyuki, On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi k...@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 k...@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, -- 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:21:27 +0200, Albert ARIBAUD albert.u.b...@aribaud.net wrote: Hi Albert, On Sun, 7 Oct 2012 19:19:37 +0200, Albert ARIBAUD albert.u.b...@aribaud.net wrote: Hi Jeroen, On Sun, 07 Oct 2012 17:18:27 +0200, Jeroen Hofstee dasub...@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.b...@aribaud.net: Hi Tetsuyuki, On Fri, 5 Oct 2012 13:39:22 +0900, Tetsuyuki Kobayashi k...@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 k...@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. ___ 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 k...@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 k...@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