| Message ID | 1376504746-15173-4-git-send-email-jeroen@myspectrum.nl |
|---|---|
| State | Changes Requested |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
On 08/14/2013 08:25 PM, Jeroen Hofstee wrote: > To be more EABI compliant and as a preparation for building > with clang, use the platform-specific r9 register for gd > instead of r8. > > note: The FIQ is not updated since it is not used in u-boot, > and under discussion for the time being. > > The following checkpatch warning is ignored: > WARNING: Use of volatile is usually wrong: see > Documentation/volatile-considered-harmful.txt > > cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> > --- > arch/arm/config.mk | 2 +- > arch/arm/include/asm/global_data.h | 2 +- > arch/arm/lib/crt0.S | 16 ++++++++-------- > 3 files changed, 10 insertions(+), 10 deletions(-) > This patch assumes only crt0.S sets the register used for gd in asm. I just noticed cpu/armv7/lowlevel_init.S does set gd manually, so all users of the common board.c are likely bricked with the patch as is. Looking into it.... Regards, Jeroen
On 08/17/2013 11:40 AM, Jeroen Hofstee wrote: > On 08/14/2013 08:25 PM, Jeroen Hofstee wrote: >> To be more EABI compliant and as a preparation for building >> with clang, use the platform-specific r9 register for gd >> instead of r8. >> >> note: The FIQ is not updated since it is not used in u-boot, >> and under discussion for the time being. >> >> The following checkpatch warning is ignored: >> WARNING: Use of volatile is usually wrong: see >> Documentation/volatile-considered-harmful.txt >> >> cc: Albert ARIBAUD <albert.u.boot@aribaud.net> >> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> >> --- >> arch/arm/config.mk | 2 +- >> arch/arm/include/asm/global_data.h | 2 +- >> arch/arm/lib/crt0.S | 16 ++++++++-------- >> 3 files changed, 10 insertions(+), 10 deletions(-) >> > > This patch assumes only crt0.S sets the register used for > gd in asm. I just noticed cpu/armv7/lowlevel_init.S does set gd > manually, so all users of the common board.c are likely > bricked with the patch as is. Looking into it.... Looking into this, it seems lowlevel_init.S could simply be removed by placing the call to s_init at a better spot. As Tom pointed out, this is already being discussed at [1]. As that is a topic of its own I intend to simply change lowlevel_init.S to us r9 in this patchset. Regards, Jeroen [1] http://u-boot.10912.n7.nabble.com/PATCH-1-3-arm-spl-Fix-SPL-booting-for-OMAP3-td156990.html
Hi Jeroen, On Sat, Aug 17, 2013 at 3:40 AM, Jeroen Hofstee <jeroen@myspectrum.nl> wrote: > On 08/14/2013 08:25 PM, Jeroen Hofstee wrote: >> >> To be more EABI compliant and as a preparation for building >> with clang, use the platform-specific r9 register for gd >> instead of r8. >> >> note: The FIQ is not updated since it is not used in u-boot, >> and under discussion for the time being. >> >> The following checkpatch warning is ignored: >> WARNING: Use of volatile is usually wrong: see >> Documentation/volatile-considered-harmful.txt >> >> cc: Albert ARIBAUD <albert.u.boot@aribaud.net> >> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> >> --- >> arch/arm/config.mk | 2 +- >> arch/arm/include/asm/global_data.h | 2 +- >> arch/arm/lib/crt0.S | 16 ++++++++-------- >> 3 files changed, 10 insertions(+), 10 deletions(-) >> > > This patch assumes only crt0.S sets the register used for > gd in asm. I just noticed cpu/armv7/lowlevel_init.S does set gd > manually, so all users of the common board.c are likely > bricked with the patch as is. Looking into it.... I may misunderstood what you are saying here, but I believe that the code in common/board_f.c which creates a global_data on the stack can be removed for ARM now that Albert has tidied all this up with the crt0.S changes, etc. So in common/board_f.c something like: void board_init_f(ulong boot_flags) { /* These two archs set up the global_data before board_init_f() */ #if !defined(CONFIG_X86) && !defined(CONFIG_ARM) gd_t data; gd = &data; #endif gd->flags = boot_flags; Regards, Simon
Hello Simon, On 08/19/2013 05:08 AM, Simon Glass wrote: > On Sat, Aug 17, 2013 at 3:40 AM, Jeroen Hofstee <jeroen@myspectrum.nl> wrote: >> >> This patch assumes only crt0.S sets the register used for >> gd in asm. I just noticed cpu/armv7/lowlevel_init.S does set gd >> manually, so all users of the common board.c are likely >> bricked with the patch as is. Looking into it.... > I may misunderstood what you are saying here, Likely, this is not about how to set reserve gd, but why gd is setup twice. The answer is because some more cleanup is needed (which deserves its own patch). > but I believe that the > code in common/board_f.c which creates a global_data on the stack can > be removed for ARM now that Albert has tidied all this up with the > crt0.S changes, etc. So in common/board_f.c something like: > > void board_init_f(ulong boot_flags) > { > /* These two archs set up the global_data before board_init_f() */ > #if !defined(CONFIG_X86) && !defined(CONFIG_ARM) > gd_t data; > > gd = &data; > #endif > > gd->flags = boot_flags; > This won't work in general for ARM as board_init_f returns in the ARM specific board (and will be rather ugly for the clang case). Regards, Jeroen
Hello Simon, On 08/19/2013 07:32 PM, Jeroen Hofstee wrote: > > On 08/19/2013 05:08 AM, Simon Glass wrote: >> On Sat, Aug 17, 2013 at 3:40 AM, Jeroen Hofstee >> <jeroen@myspectrum.nl> wrote: >>> >>> This patch assumes only crt0.S sets the register used for >>> gd in asm. I just noticed cpu/armv7/lowlevel_init.S does set gd >>> manually, so all users of the common board.c are likely >>> bricked with the patch as is. Looking into it.... >> I may misunderstood what you are saying here, > > Likely, this is not about how to set reserve gd, but why gd > is setup twice. The answer is because some more cleanup > is needed (which deserves its own patch). > Right... Now I understand what you were talking about. Gd is actually setup _three_ times in a row: 1) cpu/armv7/lowlevel_init.S 2) ./arch/arm/lib/crt0.S 3) common/board_f.c >> but I believe that the >> code in common/board_f.c which creates a global_data on the stack can >> be removed for ARM now that Albert has tidied all this up with the >> crt0.S changes, etc. So in common/board_f.c something like: >> >> void board_init_f(ulong boot_flags) >> { >> /* These two archs set up the global_data before board_init_f() */ >> #if !defined(CONFIG_X86) && !defined(CONFIG_ARM) >> gd_t data; >> >> gd = &data; >> #endif >> >> gd->flags = boot_flags; >> > Yup understood now, this makes sense (but as part of a cleanup patchset) Thanks, Regards, Jeroen
Hi Jeroen, On Wed, Aug 21, 2013 at 10:22 AM, Jeroen Hofstee <jeroen@myspectrum.nl> wrote: > Hello Simon, > > > On 08/19/2013 07:32 PM, Jeroen Hofstee wrote: >> >> >> On 08/19/2013 05:08 AM, Simon Glass wrote: >>> >>> On Sat, Aug 17, 2013 at 3:40 AM, Jeroen Hofstee <jeroen@myspectrum.nl> >>> wrote: >>>> >>>> >>>> This patch assumes only crt0.S sets the register used for >>>> gd in asm. I just noticed cpu/armv7/lowlevel_init.S does set gd >>>> manually, so all users of the common board.c are likely >>>> bricked with the patch as is. Looking into it.... >>> >>> I may misunderstood what you are saying here, >> >> >> Likely, this is not about how to set reserve gd, but why gd >> is setup twice. The answer is because some more cleanup >> is needed (which deserves its own patch). >> > > Right... Now I understand what you were talking about. Gd is actually > setup _three_ times in a row: Yes. > > 1) cpu/armv7/lowlevel_init.S > 2) ./arch/arm/lib/crt0.S > 3) common/board_f.c > > >>> but I believe that the >>> code in common/board_f.c which creates a global_data on the stack can >>> be removed for ARM now that Albert has tidied all this up with the >>> crt0.S changes, etc. So in common/board_f.c something like: >>> >>> void board_init_f(ulong boot_flags) >>> { >>> /* These two archs set up the global_data before board_init_f() */ >>> #if !defined(CONFIG_X86) && !defined(CONFIG_ARM) >>> gd_t data; >>> >>> gd = &data; >>> #endif >>> >>> gd->flags = boot_flags; >>> >> > > Yup understood now, this makes sense (but as part of a cleanup patchset) Agreed, no suggestion it should be done here - I just wanted to make sure you are not working hard to preserve unnecessary code. Regards, Simon
diff --git a/arch/arm/config.mk b/arch/arm/config.mk index 5e382ab..5f6e032 100644 --- a/arch/arm/config.mk +++ b/arch/arm/config.mk @@ -99,4 +99,4 @@ ifneq ($(CONFIG_SPL_BUILD),y) ALL-y += checkarmreloc endif -OPTION_FIXED_GD=$(call cc-option, -ffixed-r8) +OPTION_FIXED_GD=$(call cc-option, -ffixed-r9) diff --git a/arch/arm/include/asm/global_data.h b/arch/arm/include/asm/global_data.h index 79a9597..e126436 100644 --- a/arch/arm/include/asm/global_data.h +++ b/arch/arm/include/asm/global_data.h @@ -47,6 +47,6 @@ struct arch_global_data { #include <asm-generic/global_data.h> -#define DECLARE_GLOBAL_DATA_PTR register volatile gd_t *gd asm ("r8") +#define DECLARE_GLOBAL_DATA_PTR register volatile gd_t *gd asm ("r9") #endif /* __ASM_GBL_DATA_H */ diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S index 960d12e..ac54b93 100644 --- a/arch/arm/lib/crt0.S +++ b/arch/arm/lib/crt0.S @@ -69,7 +69,7 @@ ENTRY(_main) bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ sub sp, #GD_SIZE /* allocate one GD above SP */ bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ - mov r8, sp /* GD is above SP */ + mov r9, sp /* GD is above SP */ mov r0, #0 bl board_init_f @@ -81,15 +81,15 @@ ENTRY(_main) * 'here' but relocated. */ - ldr sp, [r8, #GD_START_ADDR_SP] /* sp = gd->start_addr_sp */ + ldr sp, [r9, #GD_START_ADDR_SP] /* sp = gd->start_addr_sp */ bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ - ldr r8, [r8, #GD_BD] /* r8 = gd->bd */ - sub r8, r8, #GD_SIZE /* new GD is below bd */ + ldr r9, [r9, #GD_BD] /* r9 = gd->bd */ + sub r9, r9, #GD_SIZE /* new GD is below bd */ adr lr, here - ldr r0, [r8, #GD_RELOC_OFF] /* r0 = gd->reloc_off */ + ldr r0, [r9, #GD_RELOC_OFF] /* r0 = gd->reloc_off */ add lr, lr, r0 - ldr r0, [r8, #GD_RELOCADDR] /* r0 = gd->relocaddr */ + ldr r0, [r9, #GD_RELOCADDR] /* r0 = gd->relocaddr */ b relocate_code here: @@ -111,8 +111,8 @@ clbss_l:cmp r0, r1 /* while not at end of BSS */ bl red_led_on /* call board_init_r(gd_t *id, ulong dest_addr) */ - mov r0, r8 /* gd_t */ - ldr r1, [r8, #GD_RELOCADDR] /* dest_addr */ + mov r0, r9 /* gd_t */ + ldr r1, [r9, #GD_RELOCADDR] /* dest_addr */ /* call board_init_r */ ldr pc, =board_init_r /* this is auto-relocated! */
To be more EABI compliant and as a preparation for building with clang, use the platform-specific r9 register for gd instead of r8. note: The FIQ is not updated since it is not used in u-boot, and under discussion for the time being. The following checkpatch warning is ignored: WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt cc: Albert ARIBAUD <albert.u.boot@aribaud.net> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl> --- arch/arm/config.mk | 2 +- arch/arm/include/asm/global_data.h | 2 +- arch/arm/lib/crt0.S | 16 ++++++++-------- 3 files changed, 10 insertions(+), 10 deletions(-)