Patchwork [U-Boot,v2,3/4] ARM: use r9 for gd

login
register
mail settings
Submitter Jeroen Hofstee
Date Aug. 14, 2013, 6:25 p.m.
Message ID <1376504746-15173-4-git-send-email-jeroen@myspectrum.nl>
Download mbox | patch
Permalink /patch/267179/
State Changes Requested
Delegated to: Albert ARIBAUD
Headers show

Comments

Jeroen Hofstee - Aug. 14, 2013, 6:25 p.m.
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(-)
Jeroen Hofstee - Aug. 17, 2013, 9:40 a.m.
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
Jeroen Hofstee - Aug. 17, 2013, 1:51 p.m.
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
Simon Glass - Aug. 19, 2013, 3:08 a.m.
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
Jeroen Hofstee - Aug. 19, 2013, 5:32 p.m.
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
Jeroen Hofstee - Aug. 21, 2013, 4:22 p.m.
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
Simon Glass - Aug. 21, 2013, 11:44 p.m.
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

Patch

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! */