On Thu, 4 Nov 2021 16:56:18 +0000 Peter Hoyes <peter.ho...@arm.com> wrote:
Hi, > From: Peter Hoyes <peter.ho...@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.ho...@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 >