Message ID | 20211104165619.2684533-5-peter.hoyes@arm.com |
---|---|
State | Superseded |
Delegated to: | Tom Rini |
Headers | show |
Series | VExpress64 board family improvements | expand |
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 >
Hi, On 09/11/2021 15:06, Andre Przywara wrote: > 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? BASE_FVP does not have OF_BOARD defined in its defconfig for now. I have done some basic testing with OF_BOARD enabled on the BASE_FVP and it seems OK at first glance, but I'm concerned about changes in behavior for existing users (e.g. if extra drivers are being automatically instantiated by nodes in the fdt). The other changes in this patch series (e.g. changing the env vars) are easier to reason about. > >> + >> +#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 >> Your other suggestions make sense to me. Peter
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 +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 + +#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 + +/* 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 + if (fdt_magic(VEXPRESS_FDT_ADDR) == FDT_MAGIC) { + *err = 0; + return (void *)VEXPRESS_FDT_ADDR; + } else if (fdt_magic(prior_stage_fdt_address) == FDT_MAGIC) { + *err = 0; + return (void *)prior_stage_fdt_address; + } else { + *err = -ENXIO; + return NULL; + } +#endif } #endif