[U-Boot] [PATCH] arm: add 8-byte alignment for ABI compliance before board_init_f
suggested from Daniel Hobidaniel.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(-)
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");
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
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
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,
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
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
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
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
Le 12/11/2010 07:53, Heiko Schocher a écrit :
suggested from Daniel Hobidaniel.hobi@schmid-telecom.ch
Tested on following boards: arm1136: qong armv7: omap3_beagle arm926ejs: magnesium, tx25
Signed-off-by: Heiko Schocherhs@denx.de cc: Daniel Hobidaniel.hobi@schmid-telecom.ch cc: Albert ARIBAUDalbert.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,
Hello Albert,
Albert ARIBAUD wrote:
Le 12/11/2010 07:53, Heiko Schocher a écrit :
suggested from Daniel Hobidaniel.hobi@schmid-telecom.ch
Tested on following boards: arm1136: qong armv7: omap3_beagle arm926ejs: magnesium, tx25
Signed-off-by: Heiko Schocherhs@denx.de cc: Daniel Hobidaniel.hobi@schmid-telecom.ch cc: Albert ARIBAUDalbert.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
Dear Heiko Schocher,
In message 1289544835-24425-1-git-send-email-hs@denx.de you wrote:
suggested from Daniel Hobidaniel.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
participants (5)
-
Albert ARIBAUD -
Graeme Russ -
Heiko Schocher -
Reinhard Meyer -
Wolfgang Denk