Message ID | 4D504CAA.7000302@gmail.com |
---|---|
State | Superseded |
Headers | show |
Hi Stefano, On 2/7/2011 5:48 PM, Magnus Lilja wrote: ... > > Here's a somewhat cleaner version of my patch. Hope the mail looks ok, I'm having internet connectivity issues this evening so I'm using a different installation of Thunderbird than usual. > > Regards, Magnus > > diff --git a/arch/arm/cpu/arm1136/start.S b/arch/arm/cpu/arm1136/start.S > index 12545c2..bab2868 100644 > --- a/arch/arm/cpu/arm1136/start.S > +++ b/arch/arm/cpu/arm1136/start.S > @@ -163,15 +163,7 @@ call_board_init_f: > bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ > ldr r0,=0x00000000 > > -#ifdef CONFIG_NAND_SPL > - bl nand_boot > -#else > -#ifdef CONFIG_ONENAND_IPL > - bl start_oneboot > -#else > bl board_init_f > -#endif /* CONFIG_ONENAND_IPL */ > -#endif /* CONFIG_NAND_SPL */ > > > /*------------------------------------------------------------------------------*/ > > @@ -267,10 +259,10 @@ clbss_l:str r2, [r0] /* clear loop... */ > */ > #ifdef CONFIG_NAND_SPL > ldr r0, _nand_boot_ofs > - adr r1, _start > - add pc, r0, r1 > -_nand_boot_ofs: > - .word nand_boot - _start > + mov pc, r0 > + > +_nand_boot_ofs: > + .word nand_boot > #else > jump_2_ram: > ldr r0, _board_init_r_ofs I confirmed that by applying my original patch of this thread plus Magnus´ patch above I can get MX31PDK to boot. Please let me know how you want me to proceed. Thanks, Fabio Estevam
Am 08.02.2011 18:09, schrieb Fabio Estevam: > I confirmed that by applying my original patch of this thread plus > Magnus´ patch above I can get MX31PDK to boot. Fine. > > Please let me know how you want me to proceed. I think the correct way is that Magnus adds his Signed-off-by to his patch, repushing the patch to the list. Please put Albert in CC, as this file is competence of the ARM maintainer (well, we tested only on i.MX, I see...). I will take your patch for the mx31pdk and I will merge it on u-boot-imx (and including it in the next pull request as well). Stefano
Hi >> Please let me know how you want me to proceed. > > I think the correct way is that Magnus adds his Signed-off-by to his > patch, repushing the patch to the list. Please put Albert in CC, as this > file is competence of the ARM maintainer (well, we tested only on i.MX, > I see...). I will take your patch for the mx31pdk and I will merge it on > u-boot-imx (and including it in the next pull request as well). Patch reposted as a separate mail a couple of minutes ago. As I mention in the patch I think Fabio's patch has to be applied first. Another solution would be to change my patch somewhat to apply it first and then update Fabios patch to only touch the i.MX31-PDK specific files. I cannot work on that before Thursday or Friday though. Regards, Magnus Lilja
Am 08.02.2011 20:26, schrieb Magnus Lilja: > Patch reposted as a separate mail a couple of minutes ago. > > As I mention in the patch I think Fabio's patch has to be applied first. I think your patch is ok - Fabio fixed the syntax error as you do. We need only one of them. > Another solution would be to change my patch somewhat to apply it first > and then update Fabios patch to only touch the i.MX31-PDK specific > files. IMHO this is the preferred way, because the two issues are orthogonal. Your patch fixes booting from NAND for ARM11, and Fabio's patch fix the mx31pdk board only. Regards, Stefano
Le 08/02/2011 21:18, stefano babic a écrit : > Am 08.02.2011 20:26, schrieb Magnus Lilja: >> Patch reposted as a separate mail a couple of minutes ago. >> >> As I mention in the patch I think Fabio's patch has to be applied first. > > I think your patch is ok - Fabio fixed the syntax error as you do. We > need only one of them. > >> Another solution would be to change my patch somewhat to apply it first >> and then update Fabios patch to only touch the i.MX31-PDK specific >> files. > > IMHO this is the preferred way, because the two issues are orthogonal. > Your patch fixes booting from NAND for ARM11, and Fabio's patch fix the > mx31pdk board only. Agreed. Note also that there was a recent patch to ARM926's start.S (replacing 'adr r1, _start' with 'ldr r1, _TEXT_BASE' at line 284). The same should be done on arm1136. > Regards, > Stefano Amicalement,
Hi Albert, On 2/8/2011 6:50 PM, Albert ARIBAUD wrote: > Le 08/02/2011 21:18, stefano babic a écrit : >> Am 08.02.2011 20:26, schrieb Magnus Lilja: >>> Patch reposted as a separate mail a couple of minutes ago. >>> >>> As I mention in the patch I think Fabio's patch has to be applied first. >> >> I think your patch is ok - Fabio fixed the syntax error as you do. We >> need only one of them. >> >>> Another solution would be to change my patch somewhat to apply it first >>> and then update Fabios patch to only touch the i.MX31-PDK specific >>> files. >> >> IMHO this is the preferred way, because the two issues are orthogonal. >> Your patch fixes booting from NAND for ARM11, and Fabio's patch fix the >> mx31pdk board only. > > Agreed. Ok, I have just submitted the patch series treating one issue at the time. > Note also that there was a recent patch to ARM926's start.S (replacing 'adr r1, _start' with 'ldr r1, _TEXT_BASE' at line 284). The same should be done on arm1136. Ok, I haven´t added this change to my patch series yet, but I can do it on a separate patch after my original patch series is applied, if this is OK with you. I have tested this recommended change and it worked fine on my mx31pdk board. Thanks, Fabio Estevam
Hi Albert, On Wednesday 09 February 2011 02:20 AM, Albert ARIBAUD wrote: > Le 08/02/2011 21:18, stefano babic a écrit : >> Am 08.02.2011 20:26, schrieb Magnus Lilja: >>> Patch reposted as a separate mail a couple of minutes ago. >>> >>> As I mention in the patch I think Fabio's patch has to be applied first. >> >> I think your patch is ok - Fabio fixed the syntax error as you do. We >> need only one of them. >> >>> Another solution would be to change my patch somewhat to apply it first >>> and then update Fabios patch to only touch the i.MX31-PDK specific >>> files. >> >> IMHO this is the preferred way, because the two issues are orthogonal. >> Your patch fixes booting from NAND for ARM11, and Fabio's patch fix the >> mx31pdk board only. > > Agreed. > > Note also that there was a recent patch to ARM926's start.S (replacing > 'adr r1, _start' with 'ldr r1, _TEXT_BASE' at line 284). The same should > be done on arm1136. Is this going to happen for armv7 too? What is the real reason behind this proposal. What is the case when _start is not same as _TEXT_BASE(I looked at the archives but couldn't filter out the original discussion on this) I see a problem with that. _TEXT_BASE is based on CONFIG_SYS_TEXT_BASE. In our SPL's case CONFIG_SYS_TEXT_BASE indicates the TEXT_BASE for u-boot and *CONFIG_SYS_SPL_TEXT_BASE* indicates the TEXT_BASE for SPL. Both are defined and useful in SPL because one is used for linking SPL while the other is used while loading u-boot from MMC. So, CONFIG_SYS_TEXT_BASE used in the start.S of SPL will not be correct. In the worst case we need to define yet another label in the linker scripts like __text_base. But I was wondering if we could maintain the status quo for armv7: that is 'adr r1, _start' Best regards, Aneesh
Le 11/02/2011 11:51, Aneesh V a écrit : > Hi Albert, > > On Wednesday 09 February 2011 02:20 AM, Albert ARIBAUD wrote: >> Le 08/02/2011 21:18, stefano babic a écrit : >>> Am 08.02.2011 20:26, schrieb Magnus Lilja: >>>> Patch reposted as a separate mail a couple of minutes ago. >>>> >>>> As I mention in the patch I think Fabio's patch has to be applied >>>> first. >>> >>> I think your patch is ok - Fabio fixed the syntax error as you do. We >>> need only one of them. >>> >>>> Another solution would be to change my patch somewhat to apply it first >>>> and then update Fabios patch to only touch the i.MX31-PDK specific >>>> files. >>> >>> IMHO this is the preferred way, because the two issues are orthogonal. >>> Your patch fixes booting from NAND for ARM11, and Fabio's patch fix the >>> mx31pdk board only. >> >> Agreed. >> >> Note also that there was a recent patch to ARM926's start.S (replacing >> 'adr r1, _start' with 'ldr r1, _TEXT_BASE' at line 284). The same should >> be done on arm1136. > > Is this going to happen for armv7 too? What is the real reason behind > this proposal. What is the case when _start is not same as _TEXT_BASE(I > looked at the archives but couldn't filter out the original discussion > on this) The difference is that _TEXT_BASE always contains the link-time address of _start, whereas references to _start may contain a different value if the code is executed somewhere else than at the link-time address. /Normally/, u-boot should always execute first at the link-time address -- that's a base constraint. /But/ this change makes it more resilient to out-of-link-time-address execution, and I want, at some time in the future, to find a way for u-boot to be able to start anywhere -- within reasonable limits: anywhere in NOR for a NOR-based U-boot, anywhere in RAM for a RAM-based U-boot, but I am not talking about a generic, run-in-RAM-or-NOR-or-anywhere, binary. Yet. :) > I see a problem with that. _TEXT_BASE is based on > CONFIG_SYS_TEXT_BASE. In our SPL's case CONFIG_SYS_TEXT_BASE indicates > the TEXT_BASE for u-boot and *CONFIG_SYS_SPL_TEXT_BASE* indicates the > TEXT_BASE for SPL. Both are defined and useful in SPL because one is > used for linking SPL while the other is used while loading u-boot from > MMC. So, CONFIG_SYS_TEXT_BASE used in the start.S of SPL will not be > correct. The change I indicate is under the #else of a #ifdef CONFIG_NAND_SPL, so it will not apply to SPL. Does that still cause an issue with armv7? > In the worst case we need to define yet another label in the linker > scripts like __text_base. But I was wondering if we could maintain the > status quo for armv7: that is 'adr r1, _start' As long as you run the u-boot start code at the link-time address, there will be no difference except the code is more correct with respect to what it should do; and if you run it elsewhere, which you should not, you have slightly better chances that it manages to survive. > Best regards, > Aneesh Amicalement,
On Friday 11 February 2011 06:16 PM, Albert ARIBAUD wrote: [snip...] >>> >>> Note also that there was a recent patch to ARM926's start.S (replacing >>> 'adr r1, _start' with 'ldr r1, _TEXT_BASE' at line 284). The same should >>> be done on arm1136. >> >> Is this going to happen for armv7 too? What is the real reason behind >> this proposal. What is the case when _start is not same as _TEXT_BASE(I >> looked at the archives but couldn't filter out the original discussion >> on this) > > The difference is that _TEXT_BASE always contains the link-time address > of _start, whereas references to _start may contain a different value if > the code is executed somewhere else than at the link-time address. > > /Normally/, u-boot should always execute first at the link-time address > -- that's a base constraint. > > /But/ this change makes it more resilient to out-of-link-time-address > execution, and I want, at some time in the future, to find a way for > u-boot to be able to start anywhere -- within reasonable limits: > anywhere in NOR for a NOR-based U-boot, anywhere in RAM for a RAM-based > U-boot, but I am not talking about a generic, > run-in-RAM-or-NOR-or-anywhere, binary. > > Yet. :) > >> I see a problem with that. _TEXT_BASE is based on >> CONFIG_SYS_TEXT_BASE. In our SPL's case CONFIG_SYS_TEXT_BASE indicates >> the TEXT_BASE for u-boot and *CONFIG_SYS_SPL_TEXT_BASE* indicates the >> TEXT_BASE for SPL. Both are defined and useful in SPL because one is >> used for linking SPL while the other is used while loading u-boot from >> MMC. So, CONFIG_SYS_TEXT_BASE used in the start.S of SPL will not be >> correct. > > The change I indicate is under the #else of a #ifdef CONFIG_NAND_SPL, so > it will not apply to SPL. Does that still cause an issue with armv7? No. It doesn't. I am fine with this change if it applies only to u-boot. br, Aneesh
different installation of Thunderbird than usual. Regards, Magnus diff --git a/arch/arm/cpu/arm1136/start.S b/arch/arm/cpu/arm1136/start.S index 12545c2..bab2868 100644 --- a/arch/arm/cpu/arm1136/start.S +++ b/arch/arm/cpu/arm1136/start.S @@ -163,15 +163,7 @@ call_board_init_f: bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ ldr r0,=0x00000000 -#ifdef CONFIG_NAND_SPL - bl nand_boot -#else -#ifdef CONFIG_ONENAND_IPL - bl start_oneboot -#else bl board_init_f -#endif /* CONFIG_ONENAND_IPL */ -#endif /* CONFIG_NAND_SPL */ /*------------------------------------------------------------------------------*/