On Thu, 4 Nov 2021 16:56:18 +0000 Peter Hoyes peter.hoyes@arm.com wrote:
Hi,
From: Peter Hoyes Peter.Hoyes@arm.com
Capture x0 in lowlevel_init.S as potential fdt address. Modify board_fdt_blob_setup to use fdt address from either vexpress_aemv8.h or lowlevel_init.S.
Signed-off-by: Peter Hoyes Peter.Hoyes@arm.com
board/armltd/vexpress64/Makefile | 5 +++++ board/armltd/vexpress64/lowlevel_init.S | 12 ++++++++++++ board/armltd/vexpress64/vexpress64.c | 24 ++++++++++++++++++++++++ 3 files changed, 41 insertions(+) create mode 100644 board/armltd/vexpress64/lowlevel_init.S
diff --git a/board/armltd/vexpress64/Makefile b/board/armltd/vexpress64/Makefile index 868dc4f629..5703e75967 100644 --- a/board/armltd/vexpress64/Makefile +++ b/board/armltd/vexpress64/Makefile @@ -5,3 +5,8 @@
obj-y := vexpress64.o obj-$(CONFIG_TARGET_VEXPRESS64_JUNO) += pcie.o +ifdef CONFIG_OF_BOARD +ifndef CONFIG_TARGET_VEXPRESS64_JUNO +obj-y += lowlevel_init.o
I wonder if it hurts to avoid all these confusing conditionals and just always include this, even for Juno? Maybe we can use x0 even on the Juno at some day?
+endif +endif diff --git a/board/armltd/vexpress64/lowlevel_init.S b/board/armltd/vexpress64/lowlevel_init.S new file mode 100644 index 0000000000..3dcfb85d0e --- /dev/null +++ b/board/armltd/vexpress64/lowlevel_init.S @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- (C) Copyright 2021 Arm Limited
- */
+.global save_boot_params +save_boot_params:
- adr x8, prior_stage_fdt_address
- str x0, [x8]
- b save_boot_params_ret
diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c index d2f307cca5..bde6ef1ba3 100644 --- a/board/armltd/vexpress64/vexpress64.c +++ b/board/armltd/vexpress64/vexpress64.c @@ -86,6 +86,8 @@ int dram_init_banksize(void) }
#ifdef CONFIG_OF_BOARD
Do we still need this? Every vexpress64 platform should now rely on OF_BOARD?
+#ifdef CONFIG_TARGET_VEXPRESS64_JUNO #define JUNO_FLASH_SEC_SIZE (256 * 1024) static phys_addr_t find_dtb_in_nor_flash(const char *partname) { @@ -130,9 +132,19 @@ static phys_addr_t find_dtb_in_nor_flash(const char *partname)
return ~0; } +#else
As suggested above, we probably should always include this variable below, so turn the #else into an #endif?
+/* Assigned in lowlevel_init.S
- Push the variable into the .data section so that it
- does not get cleared later.
- */
+unsigned long __section(".data") prior_stage_fdt_address;
+#endif
void *board_fdt_blob_setup(int *err) { +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO phys_addr_t fdt_rom_addr = find_dtb_in_nor_flash(CONFIG_JUNO_DTB_PART);
*err = 0; @@ -142,6 +154,18 @@ void *board_fdt_blob_setup(int *err) }
return (void *)fdt_rom_addr; +#else
Can you turn this into an #endif?
- if (fdt_magic(VEXPRESS_FDT_ADDR) == FDT_MAGIC) {
*err = 0;return (void *)VEXPRESS_FDT_ADDR;
"else" after an unconditional return is somewhat frowned upon, since it gives a wrong impression about the code flow.
What about: #ifdef VEXPRESS_FDT_ADDR if (fdt_magic(VEXPRESS_FDT_ADDR) ... { ... return ...; } #endif
- } else if (fdt_magic(prior_stage_fdt_address) == FDT_MAGIC) {
*err = 0;return (void *)prior_stage_fdt_address;- } else {
If we always include prior_stage_fdt_address (even though it may be unused), you can just always include this piece. And lose the else here, since we return inside the if branch.
Cheers, Andre
*err = -ENXIO;return NULL;- }
+#endif } #endif