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

[Peter Hoyes]

Hi,

On 09/11/2021 15:06, Andre Przywara wrote:
> 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?

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