[U-Boot,v3,1/4] rpi: push fw_dtb_pointer in the .data section
diff mbox series

Message ID 20191112110029.847-2-matthias.bgg@kernel.org
State Superseded
Delegated to: Matthias Brugger
Headers show
Series
  • RPi one binary for RPi3/4 and RPi1/2
Related show

Commit Message

Matthias Brugger Nov. 12, 2019, 11 a.m. UTC
From: Matthias Brugger <mbrugger@suse.com>

The fw_dtb_pointer was defined in the assembly code, which makes him
live in section .text_rest
Put that's not necessary, we can push the variable in the .data section.

This will prevent relocation errors like:
board/raspberrypi/rpi/rpi.c:317:(.text.board_get_usable_ram_top+0x8):
relocation truncated to fit: R_AARCH64_LDST64_ABS_LO12_NC against symbol
`fw_dtb_pointer' defined in .text section in board/raspberrypi/rpi/built-in.o

Signed-off-by: Matthias Brugger <mbrugger@suse.com>

---

Changes in v3:
- fix armv7 build

Changes in v2:
- push fw_dtb_pointer into the .data section

 board/raspberrypi/rpi/lowlevel_init.S | 12 ++----------
 board/raspberrypi/rpi/rpi.c           |  4 ++--
 2 files changed, 4 insertions(+), 12 deletions(-)

Comments

Alexander Graf Nov. 12, 2019, 12:20 p.m. UTC | #1
On 12.11.19 13:00, matthias.bgg@kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>
>
> The fw_dtb_pointer was defined in the assembly code, which makes him
> live in section .text_rest
> Put that's not necessary, we can push the variable in the .data section.
>
> This will prevent relocation errors like:
> board/raspberrypi/rpi/rpi.c:317:(.text.board_get_usable_ram_top+0x8):
> relocation truncated to fit: R_AARCH64_LDST64_ABS_LO12_NC against symbol
> `fw_dtb_pointer' defined in .text section in board/raspberrypi/rpi/built-in.o
>
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>
> ---
>
> Changes in v3:
> - fix armv7 build
>
> Changes in v2:
> - push fw_dtb_pointer into the .data section
>
>   board/raspberrypi/rpi/lowlevel_init.S | 12 ++----------
>   board/raspberrypi/rpi/rpi.c           |  4 ++--
>   2 files changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/board/raspberrypi/rpi/lowlevel_init.S b/board/raspberrypi/rpi/lowlevel_init.S
> index 435eed521f..8c39b3e12e 100644
> --- a/board/raspberrypi/rpi/lowlevel_init.S
> +++ b/board/raspberrypi/rpi/lowlevel_init.S
> @@ -6,15 +6,6 @@
>   
>   #include <config.h>
>   
> -.align 8
> -.global fw_dtb_pointer
> -fw_dtb_pointer:
> -#ifdef CONFIG_ARM64
> -	.dword 0x0
> -#else
> -	.word 0x0
> -#endif
> -
>   /*
>    * Routine: save_boot_params (called after reset from start.S)
>    * Description: save ATAG/FDT address provided by the firmware at boot time
> @@ -28,7 +19,8 @@ save_boot_params:
>   	adr	x8, fw_dtb_pointer
>   	str	x0, [x8]
>   #else
> -	str	r2, fw_dtb_pointer
> +	ldr	r8, =fw_dtb_pointer
> +	str	r2, [r8]
>   #endif
>   
>   	/* Returns */
> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
> index 9e0abdda31..0e05d59e1f 100644
> --- a/board/raspberrypi/rpi/rpi.c
> +++ b/board/raspberrypi/rpi/rpi.c
> @@ -27,8 +27,8 @@
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> -/* From lowlevel_init.S */
> -extern unsigned long fw_dtb_pointer;
> +/* Assigned in lowlevel_init.S */
> +unsigned long fw_dtb_pointer = 0x1;


I assume you assign the 0x1 here so that it doesn't land in .bss which 
may get cleared after you wrote it? If so, please document that in the 
comment. Also even better yet, document it in a comment and just 
manually assign the variable to the ".data" section using

   __attribute__((section(".data")))



Alex
Matthias Brugger Nov. 12, 2019, 12:50 p.m. UTC | #2
On 12/11/2019 13:20, Alexander Graf wrote:
> 
> On 12.11.19 13:00, matthias.bgg@kernel.org wrote:
>> From: Matthias Brugger <mbrugger@suse.com>
>>
>> The fw_dtb_pointer was defined in the assembly code, which makes him
>> live in section .text_rest
>> Put that's not necessary, we can push the variable in the .data section.
>>
>> This will prevent relocation errors like:
>> board/raspberrypi/rpi/rpi.c:317:(.text.board_get_usable_ram_top+0x8):
>> relocation truncated to fit: R_AARCH64_LDST64_ABS_LO12_NC against symbol
>> `fw_dtb_pointer' defined in .text section in board/raspberrypi/rpi/built-in.o
>>
>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>>
>> ---
>>
>> Changes in v3:
>> - fix armv7 build
>>
>> Changes in v2:
>> - push fw_dtb_pointer into the .data section
>>
>>   board/raspberrypi/rpi/lowlevel_init.S | 12 ++----------
>>   board/raspberrypi/rpi/rpi.c           |  4 ++--
>>   2 files changed, 4 insertions(+), 12 deletions(-)
>>
>> diff --git a/board/raspberrypi/rpi/lowlevel_init.S
>> b/board/raspberrypi/rpi/lowlevel_init.S
>> index 435eed521f..8c39b3e12e 100644
>> --- a/board/raspberrypi/rpi/lowlevel_init.S
>> +++ b/board/raspberrypi/rpi/lowlevel_init.S
>> @@ -6,15 +6,6 @@
>>     #include <config.h>
>>   -.align 8
>> -.global fw_dtb_pointer
>> -fw_dtb_pointer:
>> -#ifdef CONFIG_ARM64
>> -    .dword 0x0
>> -#else
>> -    .word 0x0
>> -#endif
>> -
>>   /*
>>    * Routine: save_boot_params (called after reset from start.S)
>>    * Description: save ATAG/FDT address provided by the firmware at boot time
>> @@ -28,7 +19,8 @@ save_boot_params:
>>       adr    x8, fw_dtb_pointer
>>       str    x0, [x8]
>>   #else
>> -    str    r2, fw_dtb_pointer
>> +    ldr    r8, =fw_dtb_pointer
>> +    str    r2, [r8]
>>   #endif
>>         /* Returns */
>> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
>> index 9e0abdda31..0e05d59e1f 100644
>> --- a/board/raspberrypi/rpi/rpi.c
>> +++ b/board/raspberrypi/rpi/rpi.c
>> @@ -27,8 +27,8 @@
>>     DECLARE_GLOBAL_DATA_PTR;
>>   -/* From lowlevel_init.S */
>> -extern unsigned long fw_dtb_pointer;
>> +/* Assigned in lowlevel_init.S */
>> +unsigned long fw_dtb_pointer = 0x1;
> 
> 
> I assume you assign the 0x1 here so that it doesn't land in .bss which may get
> cleared after you wrote it? If so, please document that in the comment. Also

Yes exactly.

> even better yet, document it in a comment and just manually assign the variable
> to the ".data" section using
> 
>   __attribute__((section(".data")))
> 

Agree that's a cleaner approach then adding any random value to the pointer.

Regards,
Matthias

Patch
diff mbox series

diff --git a/board/raspberrypi/rpi/lowlevel_init.S b/board/raspberrypi/rpi/lowlevel_init.S
index 435eed521f..8c39b3e12e 100644
--- a/board/raspberrypi/rpi/lowlevel_init.S
+++ b/board/raspberrypi/rpi/lowlevel_init.S
@@ -6,15 +6,6 @@ 
 
 #include <config.h>
 
-.align 8
-.global fw_dtb_pointer
-fw_dtb_pointer:
-#ifdef CONFIG_ARM64
-	.dword 0x0
-#else
-	.word 0x0
-#endif
-
 /*
  * Routine: save_boot_params (called after reset from start.S)
  * Description: save ATAG/FDT address provided by the firmware at boot time
@@ -28,7 +19,8 @@  save_boot_params:
 	adr	x8, fw_dtb_pointer
 	str	x0, [x8]
 #else
-	str	r2, fw_dtb_pointer
+	ldr	r8, =fw_dtb_pointer
+	str	r2, [r8]
 #endif
 
 	/* Returns */
diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
index 9e0abdda31..0e05d59e1f 100644
--- a/board/raspberrypi/rpi/rpi.c
+++ b/board/raspberrypi/rpi/rpi.c
@@ -27,8 +27,8 @@ 
 
 DECLARE_GLOBAL_DATA_PTR;
 
-/* From lowlevel_init.S */
-extern unsigned long fw_dtb_pointer;
+/* Assigned in lowlevel_init.S */
+unsigned long fw_dtb_pointer = 0x1;
 
 /* TODO(sjg@chromium.org): Move these to the msg.c file */
 struct msg_get_arm_mem {