[PATCH] rockchip: spl: Cache boot source id for later use

[Jonas Karlman]

Hi Dragan,

On 2024-03-19 10:44, Dragan Simic wrote:
> Hello Jonas,
> 
> Please see a few comments below.
> 
> On 2024-03-15 18:34, Jonas Karlman wrote:
>> Rockchip BROM write a boot source id at CFG_IRAM_BASE + 0x10, the id
>> indicate from what storage media TPL/SPL was loaded from.
> 
> s/write/writes/
> s/indicate/indicates/
> 
> There are also a few more similar small grammar issues in the rest
> of the patch descroption below.

Thanks, will try to incorporate the grammatical fixes in a v2.

> 
>> SPL use this value to determine what device "same-as-spl" represent 
>> when
>> determining from where FIT should be loaded. This works as long as the
>> boot_devices array contain a matching id <-> node path entry.
> 
> s/use/uses/
> etc.
> 
>> However, SPL typically load a small part of TF-A into SRAM and on 
>> RK3399
>> this overwrites the CFG_IRAM_BASE + 0x10 addr used for boot source id.
>>
>> Here boot source id is 3 before FIT images is loaded, and 0 after:
>>
>>   U-Boot SPL 2024.04-rc4 (Mar 15 2024 - 17:26:19 +0000)
>>   board_spl_was_booted_from: brom_bootdevice_id 3 maps to
>> '/spi at ff1d0000/flash at 0'
>>   Trying to boot from SPI
>>   ## Checking hash(es) for config config-1 ... OK
>>   ## Checking hash(es) for Image atf-1 ... sha256+ OK
>>   ## Checking hash(es) for Image u-boot ... sha256+ OK
>>   ## Checking hash(es) for Image fdt-1 ... sha256+ OK
>>   ## Checking hash(es) for Image atf-2 ... sha256+ OK
>>   ## Checking hash(es) for Image atf-3 ... sha256+ OK
>>   board_spl_was_booted_from: failed to resolve brom_bootdevice_id 0
>>   spl_decode_boot_device: could not find udevice for /mmc at fe330000
>>   spl_decode_boot_device: could not find udevice for /mmc at fe320000
>>   spl_perform_fixups: could not map boot_device to ofpath: -19
>>
>> Use a static bootdevice_brom_id to cache the boot source id after an
>> initial read from SRAM to fix this, this allow spl_perform_fixups() to
>> resolve correct boot source path for "same-as-spl" after SPL have 
>> loaded
>> TF-A related FIT images into memory.
>>
>> With this the spl-boot-device prop can correctly be resolved to the
>> SPI flash node in the control FDT:
>>
>>   => fdt addr ${fdtcontroladdr}
>>   Working FDT set to f1ee6710
>>   => fdt list /chosen
>>   chosen {
>>       u-boot,spl-boot-device = "/spi at ff1d0000/flash at 0";
>>       stdout-path = "serial2:1500000n8";
>>       u-boot,spl-boot-order = "same-as-spl", "/mmc at fe330000", 
>> "/mmc at fe320000";
>>   };
>>
>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>> ---
>>  arch/arm/mach-rockchip/spl.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-rockchip/spl.c 
>> b/arch/arm/mach-rockchip/spl.c
>> index 1586a093fc37..27e996b504e7 100644
>> --- a/arch/arm/mach-rockchip/spl.c
>> +++ b/arch/arm/mach-rockchip/spl.c
>> @@ -32,9 +32,17 @@ __weak const char * const
>> boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
>>
>>  const char *board_spl_was_booted_from(void)
>>  {
>> -	u32  bootdevice_brom_id = readl(BROM_BOOTSOURCE_ID_ADDR);
>> +	static u32 bootdevice_brom_id;
>>  	const char *bootdevice_ofpath = NULL;
>>
>> +	if (!bootdevice_brom_id)
>> +		bootdevice_brom_id = readl(BROM_BOOTSOURCE_ID_ADDR);
>> +	if (!bootdevice_brom_id) {
>> +		debug("%s: unknown brom_bootdevice_id %x\n",
>> +		      __func__, bootdevice_brom_id);
>> +		return NULL;
>> +	}
>> +
> 
> Maybe it would be better to execute readl(BROM_BOOTSOURCE_ID_ADDR)
> only once, i.e. to have something like this instead:
> 
>      +	static u32 bootdevice_brom_id = -1;
> 
>      +	if (bootdevice_brom_id == -1) {
>      +		bootdevice_brom_id = readl(BROM_BOOTSOURCE_ID_ADDR);
>      +		if (!bootdevice_brom_id)
>      +			debug("%s: unknown brom_bootdevice_id %x\n",
>      +			      __func__, bootdevice_brom_id);
>      +	}
>      +
>      +	if (!bootdevice_brom_id)	/* fail on subsequent tries */
>      +		return NULL;
>      +
> 
> The logic behind such an approach would be to try only once and fail
> on subsequent (re)tries.  That way, it would also serve as some kind of
> a runtime canary test, because the first try should succeed, which may
> prove useful in the field.

If we initialize the static variable to a value it will be stored in the
.data section and takes up binary size. With a static variable like in
this patch this is instead stored in .bss section and gets initialized
to 0 by runtime. 0 is also not a valid boot source id value and the
reset value for the reg.

I do not see any point in making the code more complex than it has to be.
The reg will contain a known boot source id, 1 - 10, set by BROM or
something has gone wrong and the value is not usable any way.

Regards,
Jonas

> 
>>  	if (bootdevice_brom_id < ARRAY_SIZE(boot_devices))
>>  		bootdevice_ofpath = boot_devices[bootdevice_brom_id];