Patchwork [U-Boot] arm: add 8-byte alignment for ABI compliance before board_init_f

login
register
mail settings
Submitter Heiko Schocher
Date Nov. 12, 2010, 6:53 a.m.
Message ID <1289544835-24425-1-git-send-email-hs@denx.de>
Download mbox | patch
Permalink /patch/70931/
State Accepted
Commit 296cae732b0dbe374abc9b26fed6f73588b9d1e2
Delegated to: Wolfgang Denk
Headers show

Comments

Heiko Schocher - Nov. 12, 2010, 6:53 a.m.
suggested from Daniel Hobi<daniel.hobi@schmid-telecom.ch>

Tested on following boards:
arm1136: qong
armv7: omap3_beagle
arm926ejs: magnesium, tx25

Signed-off-by: Heiko Schocher <hs@denx.de>
cc: Daniel Hobi <daniel.hobi@schmid-telecom.ch>
cc: Albert ARIBAUD <albert.aribaud@free.fr>
---
 arch/arm/cpu/arm1136/start.S   |    1 +
 arch/arm/cpu/arm1176/start.S   |    1 +
 arch/arm/cpu/arm720t/start.S   |    1 +
 arch/arm/cpu/arm920t/start.S   |    1 +
 arch/arm/cpu/arm925t/start.S   |    1 +
 arch/arm/cpu/arm926ejs/start.S |    1 +
 arch/arm/cpu/arm946es/start.S  |    1 +
 arch/arm/cpu/arm_intcm/start.S |    1 +
 arch/arm/cpu/armv7/start.S     |    1 +
 arch/arm/cpu/ixp/start.S       |    1 +
 arch/arm/cpu/lh7a40x/start.S   |    1 +
 arch/arm/cpu/pxa/start.S       |    1 +
 arch/arm/cpu/s3c44b0/start.S   |    1 +
 arch/arm/cpu/sa1100/start.S    |    1 +
 arch/arm/lib/board.c           |    2 +-
 15 files changed, 15 insertions(+), 1 deletions(-)
Reinhard Meyer - Nov. 12, 2010, 7:06 a.m.
Dear Heiko Schocher,
> diff --git a/arch/arm/cpu/sa1100/start.S b/arch/arm/cpu/sa1100/start.S
> index ace0c07..91cdd72 100644
> --- a/arch/arm/cpu/sa1100/start.S
> +++ b/arch/arm/cpu/sa1100/start.S
> @@ -152,6 +152,7 @@ reset:
>   /* Set stackpointer in internal RAM to call board_init_f */
>   call_board_init_f:
>   	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
> +	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
>   	ldr	r0,=0x00000000
>   	bl	board_init_f
>
> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index 1fd5f83..96c0e30 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -276,7 +276,7 @@ void board_init_f (ulong bootflag)
>   	ulong addr, addr_sp;
>
>   	/* Pointer is writable since we allocated a register for it */
> -	gd = (gd_t *) (CONFIG_SYS_INIT_SP_ADDR);
> +	gd = (gd_t *) ((CONFIG_SYS_INIT_SP_ADDR)&  ~0x07);
>   	/* compiler optimization barrier needed for GCC>= 3.4 */
>   	__asm__ __volatile__("": : :"memory");
>

Is bootflag ever used? If not, why not change the parameter to
give the gd address to board_init_f?

	ld r0, sp (whatever the exact assembly syntax for that would be)

void board_init_f (gd_t *gd_addr)
...
	gd = gd_addr;

One further thought, why not init the reserved register in assembly and
remove the gd relevant code in C? But that bears some risk if the register
is changed and the assembly is forgotten to adapt..

Best regards,
Reinhard
Heiko Schocher - Nov. 12, 2010, 7:19 a.m.
Hello Reinhard,

Reinhard Meyer wrote:
> Dear Heiko Schocher,
>> diff --git a/arch/arm/cpu/sa1100/start.S b/arch/arm/cpu/sa1100/start.S
>> index ace0c07..91cdd72 100644
>> --- a/arch/arm/cpu/sa1100/start.S
>> +++ b/arch/arm/cpu/sa1100/start.S
>> @@ -152,6 +152,7 @@ reset:
>>   /* Set stackpointer in internal RAM to call board_init_f */
>>   call_board_init_f:
>>       ldr    sp, =(CONFIG_SYS_INIT_SP_ADDR)
>> +    bic    sp, sp, #7 /* 8-byte alignment for ABI compliance */
>>       ldr    r0,=0x00000000
>>       bl    board_init_f
>>
>> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
>> index 1fd5f83..96c0e30 100644
>> --- a/arch/arm/lib/board.c
>> +++ b/arch/arm/lib/board.c
>> @@ -276,7 +276,7 @@ void board_init_f (ulong bootflag)
>>       ulong addr, addr_sp;
>>
>>       /* Pointer is writable since we allocated a register for it */
>> -    gd = (gd_t *) (CONFIG_SYS_INIT_SP_ADDR);
>> +    gd = (gd_t *) ((CONFIG_SYS_INIT_SP_ADDR)&  ~0x07);
>>       /* compiler optimization barrier needed for GCC>= 3.4 */
>>       __asm__ __volatile__("": : :"memory");
>>
> 
> Is bootflag ever used? If not, why not change the parameter to

No.

> give the gd address to board_init_f?
> 
>     ld r0, sp (whatever the exact assembly syntax for that would be)
> 
> void board_init_f (gd_t *gd_addr)
> ...
>     gd = gd_addr;

I thought this too, but in arch/powerpc/lib/board.c it is used as bootflag,
so I didn;t want to touch this ... but looking in arch/*/lib/board.c
this first parameter is not always used as bootflag ... so I think
that would be good ... opinions?

> One further thought, why not init the reserved register in assembly and
> remove the gd relevant code in C? But that bears some risk if the register
> is changed and the assembly is forgotten to adapt..

No, I think this is to risky ...

bye,
Heiko
Albert ARIBAUD - Nov. 12, 2010, 7:53 a.m.
Le 12/11/2010 07:53, Heiko Schocher a écrit :
> suggested from Daniel Hobi<daniel.hobi@schmid-telecom.ch>
>
> Tested on following boards:
> arm1136: qong
> armv7: omap3_beagle
> arm926ejs: magnesium, tx25
>
> Signed-off-by: Heiko Schocher<hs@denx.de>
> cc: Daniel Hobi<daniel.hobi@schmid-telecom.ch>
> cc: Albert ARIBAUD<albert.aribaud@free.fr>

I'm a bit uneasy about having the symbol unaligned and the aligning done 
by the code (and in different places):

>   	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
> +	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */

> -	gd = (gd_t *) (CONFIG_SYS_INIT_SP_ADDR);
> +	gd = (gd_t *) ((CONFIG_SYS_INIT_SP_ADDR)&  ~0x07);
>

There is always a risk that overhauls of the code, or new uses elsewhere 
in the code, forget about the alignment constraint and use the symbol 
straight away, which could cause all sorts of hard to debug issues.

Could we not align the symbol value itself so that the code simply uses 
the symbol?

Amicalement,
Albert ARIBAUD - Nov. 12, 2010, 8:04 a.m.
Le 12/11/2010 08:19, Heiko Schocher a écrit :

>> One further thought, why not init the reserved register in assembly and
>> remove the gd relevant code in C? But that bears some risk if the register
>> is changed and the assembly is forgotten to adapt..
>
> No, I think this is to risky ...

It is absolutely not risky.

It is simply infeasible. :)

Different ARM toolchains use registers in different ways. Go from one 
toolchain to another, or from one *version* of a toolchain to another, 
and your "reserved" register is not any more.

Amicalement,
Heiko Schocher - Nov. 12, 2010, 8:05 a.m.
Hello Albert,

Albert ARIBAUD wrote:
> Le 12/11/2010 07:53, Heiko Schocher a écrit :
>> suggested from Daniel Hobi<daniel.hobi@schmid-telecom.ch>
>>
>> Tested on following boards:
>> arm1136: qong
>> armv7: omap3_beagle
>> arm926ejs: magnesium, tx25
>>
>> Signed-off-by: Heiko Schocher<hs@denx.de>
>> cc: Daniel Hobi<daniel.hobi@schmid-telecom.ch>
>> cc: Albert ARIBAUD<albert.aribaud@free.fr>
> 
> I'm a bit uneasy about having the symbol unaligned and the aligning done
> by the code (and in different places):

ok.

>>       ldr    sp, =(CONFIG_SYS_INIT_SP_ADDR)
>> +    bic    sp, sp, #7 /* 8-byte alignment for ABI compliance */
> 
>> -    gd = (gd_t *) (CONFIG_SYS_INIT_SP_ADDR);
>> +    gd = (gd_t *) ((CONFIG_SYS_INIT_SP_ADDR)&  ~0x07);
>>
> 
> There is always a risk that overhauls of the code, or new uses elsewhere
> in the code, forget about the alignment constraint and use the symbol
> straight away, which could cause all sorts of hard to debug issues.

Ok.

> Could we not align the symbol value itself so that the code simply uses
> the symbol?

Hmm.. but users can forgot to align the symbol ... and I think, code
should not change that often in this place ...

bye,
Heiko
Wolfgang Denk - Nov. 12, 2010, 8:50 a.m.
Dear Reinhard Meyer,

In message <4CDCE769.8080209@emk-elektronik.de> you wrote:
>
> Is bootflag ever used? If not, why not change the parameter to
> give the gd address to board_init_f?

No, bootflag is never used and could / should be removed.

Passing gd as parameter makes no sense, thoug, as it's global data and
we reserve a register to store it's address, so it can always be used
with minimal overhead.

> One further thought, why not init the reserved register in assembly and
> remove the gd relevant code in C? But that bears some risk if the register
> is changed and the assembly is forgotten to adapt..

We try to do as much as pssible in C, and only what really cannot be
avoided in assembly.

Best regards,

Wolfgang Denk
Graeme Russ - Nov. 12, 2010, 8:50 a.m.
On 12/11/10 18:19, Heiko Schocher wrote:
> Hello Reinhard,
> 
> Reinhard Meyer wrote:
>> Dear Heiko Schocher,
>>> diff --git a/arch/arm/cpu/sa1100/start.S b/arch/arm/cpu/sa1100/start.S

>> Is bootflag ever used? If not, why not change the parameter to
> 
> No.
> 
>> give the gd address to board_init_f?
>>
>>     ld r0, sp (whatever the exact assembly syntax for that would be)
>>
>> void board_init_f (gd_t *gd_addr)
>> ...
>>     gd = gd_addr;
> 
> I thought this too, but in arch/powerpc/lib/board.c it is used as bootflag,
> so I didn;t want to touch this ... but looking in arch/*/lib/board.c
> this first parameter is not always used as bootflag ... so I think
> that would be good ... opinions?
> 

For x86, I use the parameter as a pointer to gd and put boot flags in gd:

void board_init_f (ulong gdp)
{

The memory layout for x86 is:

+--Top of Memory-+
|                |
|     Stack      |
|                |
+----------------+
|                |
|  Global Data   |
|                |
+----------------+
|                |
|  U-Boot Code   |
|    + Data      |
|                |
+----------------+ <-- dest_addr (see below)
|                |
|      BSS       |
|                |
+----------------+
|                |
|  malloc area   |
|                |
+----------------+
|                |
|  Free Memory   |
/                /

So passing the pointer to gd also serves to calculate the relocation address:

	/* Calculate destination RAM Address and relocation offset */
	dest_addr  = (void *)gdp - (bss_end - text_start);
	rel_offset = dest_addr - text_start;


Regards,

Graeme
Graeme Russ - Nov. 12, 2010, 9:47 a.m.
On 12/11/10 19:50, Wolfgang Denk wrote:
> Dear Reinhard Meyer,
> 
> In message <4CDCE769.8080209@emk-elektronik.de> you wrote:
>>
>> Is bootflag ever used? If not, why not change the parameter to
>> give the gd address to board_init_f?
> 
> No, bootflag is never used and could / should be removed.
> 
> Passing gd as parameter makes no sense, thoug, as it's global data and
> we reserve a register to store it's address, so it can always be used
> with minimal overhead.

Which is a really nice idea if you have a register to spare. Unfortunately,
x86 does not have this luxury, and the only way I can get gd (and as a
consequence upper memory bound for relocation) is to allocate it in
assembler and pass to C as a function argument

Regards,

Graeme
Wolfgang Denk - Nov. 12, 2010, 9:56 a.m.
Dear Graeme Russ,

In message <4CDD0D1F.7010805@gmail.com> you wrote:
>
> > Passing gd as parameter makes no sense, thoug, as it's global data and
> > we reserve a register to store it's address, so it can always be used
> > with minimal overhead.
> 
> Which is a really nice idea if you have a register to spare. Unfortunately,
> x86 does not have this luxury, and the only way I can get gd (and as a
> consequence upper memory bound for relocation) is to allocate it in
> assembler and pass to C as a function argument

Agreed - x86 has always it's own set of limitations.

Best regards,

Wolfgang Denk
Wolfgang Denk - Dec. 8, 2010, 10:50 p.m.
Dear Heiko Schocher,

In message <1289544835-24425-1-git-send-email-hs@denx.de> you wrote:
> suggested from Daniel Hobi<daniel.hobi@schmid-telecom.ch>
> 
> Tested on following boards:
> arm1136: qong
> armv7: omap3_beagle
> arm926ejs: magnesium, tx25
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> cc: Daniel Hobi <daniel.hobi@schmid-telecom.ch>
> cc: Albert ARIBAUD <albert.aribaud@free.fr>
> ---
>  arch/arm/cpu/arm1136/start.S   |    1 +
>  arch/arm/cpu/arm1176/start.S   |    1 +
>  arch/arm/cpu/arm720t/start.S   |    1 +
>  arch/arm/cpu/arm920t/start.S   |    1 +
>  arch/arm/cpu/arm925t/start.S   |    1 +
>  arch/arm/cpu/arm926ejs/start.S |    1 +
>  arch/arm/cpu/arm946es/start.S  |    1 +
>  arch/arm/cpu/arm_intcm/start.S |    1 +
>  arch/arm/cpu/armv7/start.S     |    1 +
>  arch/arm/cpu/ixp/start.S       |    1 +
>  arch/arm/cpu/lh7a40x/start.S   |    1 +
>  arch/arm/cpu/pxa/start.S       |    1 +
>  arch/arm/cpu/s3c44b0/start.S   |    1 +
>  arch/arm/cpu/sa1100/start.S    |    1 +
>  arch/arm/lib/board.c           |    2 +-
>  15 files changed, 15 insertions(+), 1 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

Patch

diff --git a/arch/arm/cpu/arm1136/start.S b/arch/arm/cpu/arm1136/start.S
index aecc943..f8558c1 100644
--- a/arch/arm/cpu/arm1136/start.S
+++ b/arch/arm/cpu/arm1136/start.S
@@ -176,6 +176,7 @@  next:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 
 #ifdef CONFIG_NAND_SPL
diff --git a/arch/arm/cpu/arm1176/start.S b/arch/arm/cpu/arm1176/start.S
index f04d268..03365dc 100644
--- a/arch/arm/cpu/arm1176/start.S
+++ b/arch/arm/cpu/arm1176/start.S
@@ -251,6 +251,7 @@  skip_tcmdisable:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/arm720t/start.S b/arch/arm/cpu/arm720t/start.S
index 8cd267b..21b2aca 100644
--- a/arch/arm/cpu/arm720t/start.S
+++ b/arch/arm/cpu/arm720t/start.S
@@ -159,6 +159,7 @@  reset:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/arm920t/start.S b/arch/arm/cpu/arm920t/start.S
index d4edde7..57430f8 100644
--- a/arch/arm/cpu/arm920t/start.S
+++ b/arch/arm/cpu/arm920t/start.S
@@ -205,6 +205,7 @@  copyex:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/arm925t/start.S b/arch/arm/cpu/arm925t/start.S
index 51229c6..3e6dd90 100644
--- a/arch/arm/cpu/arm925t/start.S
+++ b/arch/arm/cpu/arm925t/start.S
@@ -196,6 +196,7 @@  poll1:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
index 6dcc9b4..bf4988d 100644
--- a/arch/arm/cpu/arm926ejs/start.S
+++ b/arch/arm/cpu/arm926ejs/start.S
@@ -174,6 +174,7 @@  reset:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/arm946es/start.S b/arch/arm/cpu/arm946es/start.S
index cad43ba..1d9ba30 100644
--- a/arch/arm/cpu/arm946es/start.S
+++ b/arch/arm/cpu/arm946es/start.S
@@ -165,6 +165,7 @@  reset:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/arm_intcm/start.S b/arch/arm/cpu/arm_intcm/start.S
index 957ca34..c481de7 100644
--- a/arch/arm/cpu/arm_intcm/start.S
+++ b/arch/arm/cpu/arm_intcm/start.S
@@ -163,6 +163,7 @@  reset:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index bb3948d..691eab0 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -166,6 +166,7 @@  next:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/ixp/start.S b/arch/arm/cpu/ixp/start.S
index 8d1aebc..f561c8f 100644
--- a/arch/arm/cpu/ixp/start.S
+++ b/arch/arm/cpu/ixp/start.S
@@ -289,6 +289,7 @@  reset:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/lh7a40x/start.S b/arch/arm/cpu/lh7a40x/start.S
index fd8a40b..5015211 100644
--- a/arch/arm/cpu/lh7a40x/start.S
+++ b/arch/arm/cpu/lh7a40x/start.S
@@ -176,6 +176,7 @@  reset:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/pxa/start.S b/arch/arm/cpu/pxa/start.S
index ae358a5..66479cb 100644
--- a/arch/arm/cpu/pxa/start.S
+++ b/arch/arm/cpu/pxa/start.S
@@ -220,6 +220,7 @@  zerojmp:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/s3c44b0/start.S b/arch/arm/cpu/s3c44b0/start.S
index 67b2c6a..5563572 100644
--- a/arch/arm/cpu/s3c44b0/start.S
+++ b/arch/arm/cpu/s3c44b0/start.S
@@ -148,6 +148,7 @@  reset:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/cpu/sa1100/start.S b/arch/arm/cpu/sa1100/start.S
index ace0c07..91cdd72 100644
--- a/arch/arm/cpu/sa1100/start.S
+++ b/arch/arm/cpu/sa1100/start.S
@@ -152,6 +152,7 @@  reset:
 /* Set stackpointer in internal RAM to call board_init_f */
 call_board_init_f:
 	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	bic	sp, sp, #7 /* 8-byte alignment for ABI compliance */
 	ldr	r0,=0x00000000
 	bl	board_init_f
 
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index 1fd5f83..96c0e30 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -276,7 +276,7 @@  void board_init_f (ulong bootflag)
 	ulong addr, addr_sp;
 
 	/* Pointer is writable since we allocated a register for it */
-	gd = (gd_t *) (CONFIG_SYS_INIT_SP_ADDR);
+	gd = (gd_t *) ((CONFIG_SYS_INIT_SP_ADDR) & ~0x07);
 	/* compiler optimization barrier needed for GCC >= 3.4 */
 	__asm__ __volatile__("": : :"memory");