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

[Dragan Simic <dsimic@manjaro.org>]

Hello Quentin,

On 2024-03-19 11:19, Quentin Schulz wrote:
> On 3/15/24 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.
>> 
>> 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.
>> 
>> 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@ff1d0000/flash@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@fe330000
>>    spl_decode_boot_device: could not find udevice for /mmc@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@ff1d0000/flash@0";
>>        stdout-path = "serial2:1500000n8";
>>        u-boot,spl-boot-order = "same-as-spl", "/mmc@fe330000", 
>> "/mmc@fe320000";
>>    };
>> 
> 
> I'm perplexed. We make use of this spl-boot-device DT property on Puma
> (RK3399) and Ringneck (PX30) and I am pretty sure I tested it does
> what it's supposed to do. So that is a bit surprising this seems to
> not work anymore. Is this related to the BSS/stack memory address
> location changes you made recently by any chance? Or did I manage to
> be very lucky for a very long time for our boards?
> 
> """
> U-Boot SPL 2024.04-rc4-00026-g6ec096a7116-dirty (Mar 19 2024 - 10:50:03 
> +0100)
> board_spl_was_booted_from: brom_bootdevice_id 5 maps to '/mmc@fe320000'
> Trying to boot from MMC2
> load_simple_fit: Skip load 'atf-5': image size is 0!
> NOTICE:  BL31: v2.9(release):v2.9.0
> NOTICE:  BL31: Built : 17:47:58, Jun 21 2023
> 
> 
> U-Boot 2024.04-rc4-00026-g6ec096a7116-dirty (Mar 19 2024 - 10:50:03 
> +0100)
> [...]
> => fdt addr ${fdtcontroladdr}
> Working FDT set to f1f13d10
> => fdt list /chosen
> chosen {
> 	u-boot,spl-boot-device = "/mmc@fe320000";
> 	stdout-path = "serial0:115200n8";
> 	u-boot,spl-boot-order = "same-as-spl", "/spi@ff1d0000/flash@0",
> "/mmc@fe330000", "/mmc@fe320000";
> };
> """
> 
> for Puma when booting from SD card... I don't see
> board_spl_was_booted_from being called a second time after BL31 is
> loaded?
> 
> mmmmmmm
> 
> Very interestingly, when booting from SPI-NOR flash:
> 
> """
> U-Boot SPL 2024.04-rc4-00026-g6ec096a7116-dirty (Mar 19 2024 - 10:50:03 
> +0100)
> board_spl_was_booted_from: brom_bootdevice_id 3 maps to 
> '/spi@ff1d0000/flash@0'
> Trying to boot from SPI
> load_simple_fit: Skip load 'atf-5': image size is 0!
> board_spl_was_booted_from: failed to resolve brom_bootdevice_id 0
> NOTICE:  BL31: v2.9(release):v2.9.0
> NOTICE:  BL31: Built : 17:47:58, Jun 21 2023
> 
> 
> U-Boot 2024.04-rc4-00026-g6ec096a7116-dirty (Mar 19 2024 - 10:50:03 
> +0100)
> [...]
> => fdt addr ${fdtcontroladdr}
> Working FDT set to f1f13d10
> => fdt list /chosen
> chosen {
> 	u-boot,spl-boot-device = "/spi@ff1d0000/flash@0";
> 	stdout-path = "serial0:115200n8";
> 	u-boot,spl-boot-order = "same-as-spl", "/spi@ff1d0000/flash@0",
> "/mmc@fe330000", "/mmc@fe320000";
> };
> """
> 
> but the DT is properly written...
> 
> Ahah! This is because of one of my commits where I added support for
> SPI-NOR flashes to spl_perform_fixups. So I think this worked for me
> because the SPI-NOR flash is explicitly listed in spl-boot-order for
> Puma, so when same-as-spl fails to resolve, the device is still found
> in spl-boot-order DT property which means spl_perform_fixup will still
> be able to write that spl-boot-device DT property. So basically, the
> issue is related to SPI-NOR flash NOT being explicitly listed in
> spl-boot-order or/and that the order isn't actually respected because
> same-as-spl is basically skipped right now (but it works for Puma
> because the next medium in the list is SPI, so skipping same-as-spl
> for SPI, would result in checking SPI again :) ).

This was a very nice read, thanks for writing it down in detail! :)

> Can you please add:
> 
> Fixes: d57e16c7e712 ("rockchip: find U-boot proper boot device by
> inverting the logic that sets it")
> 
> to the commit log regardless of the implementation we'll go for?
> 
>> Signed-off-by: Jonas Karlman <jonas@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;
>> +	}
> 
> I don't think we absolutely need the second if block, as this would be
> handled by the else part of the if block that isn't shown in this git
> context.

Having a separate if + debug() message would be fine if we opt for
the "runtime canary test" approach I suggested a bit earlier.  If we
opt for a different approach, I agree that a separate if + debug()
would be pretty much redundant.

> Also, I would suggest to add a new entry to the BROM_BOOTSOURCE_*
> enum, e.g. BROM_BOOTSOURCE_INVALID/UNKNOWN = 0 so it's a bit more
> explicit and we're also "ready" for the day Rockchip decides to use 0
> as a valid BROM boot source so we know all the places we need to
> modify the logic.
> 
> Moreover, I join Dragan over the use of a "valid" value for deciding
> to read from the RAM... but for another reason. If it actually is 0
> for some reason, we would re-read from that address in RAM until we
> get something different from 0... which may happen to be written with
> something else than 0 when loading that small part of TF-A into SRAM?
> So we would then have something completely unexpected as boot source
> now.

Good point.  I didn't have that in mind, but using a totally invalid
value to determine uninitialized state is pretty much always a good
approach that may also prevent unforeseen issues.