[PATCH 4/5] vexpress64: Enable OF_CONTROL and OF_BOARD for VExpress64

[Andre Przywara]

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
>