[PATCH 4/5] vexpress64: Enable OF_CONTROL and OF_BOARD for VExpress64
Andre Przywara
andre.przywara at arm.com
Tue Nov 9 16:06:52 CET 2021
On Thu, 4 Nov 2021 16:56:18 +0000
Peter Hoyes <peter.hoyes at arm.com> wrote:
Hi,
> From: Peter Hoyes <Peter.Hoyes at 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 at 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
>
More information about the U-Boot
mailing list