[U-Boot,RFC] ARM: mx31pdk: Use the new relocation scheme

Hi

On 2011-02-07 15:13, Stefano Babic wrote:
>>> I have no idea if this breaks other boards but at least it's start
>>> for further work. I tried it on the imx31 litekit (which boots from
>>> NOR) but that board doesn't seem to boot even before applying
>>> Fabios patch, I'll see if I can figure out why that board is broken
>>> now. Don't know when though.
>>
>> I will let others comment about the changes you did in
>> arch/arm/cpu/arm1136/start.S Will also try it later today.
>
> Anyway, arch/arm/cpu/arm1136/start.S cannot be compiled without the
> changes suggested by Magnus due to the following lines:
>
> _nand_boot_ofs
>         : .word nand_boot - _start
>
> ":" seems to me wrong in any case and should be fixed. Is this syntax
> accepted by newer compiler releases ?
>
> IMHO board_init_f must be called absolutely before nand_boot. I have
> tested the changes suggested by Magnus on a i.MX35 and the board boots
> from NAND. However, I do not see the problems with the console, and I
> can think they are not related to this issue. >

I agree on that last statement, the console thing must be something else.

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 

@@ -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

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