| Message ID | 1289544835-24425-1-git-send-email-hs@denx.de |
|---|---|
| State | Accepted |
| Commit | 296cae732b0dbe374abc9b26fed6f73588b9d1e2 |
| Delegated to: | Wolfgang Denk |
| Headers | show |
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 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,
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,
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
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 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
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
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
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");
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(-)