[PATCH] rockchip: spl: Cache boot source id for later use
Jonas Karlman
jonas at kwiboo.se
Tue Mar 19 17:52:21 CET 2024
Hi Quentin,
On 2024-03-19 11:19, Quentin Schulz wrote:
> Hi Jonas,
>
> 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 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";
>> };
>>
>
> 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?
As you figured out below, this is currently only an issue with SPI flash
because it was only the SPI flash code path that re-used boot source id
after FIT images have been loaded into memory.
>
> """
> 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 at 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 at fe320000";
> stdout-path = "serial0:115200n8";
> u-boot,spl-boot-order = "same-as-spl", "/spi at ff1d0000/flash at 0",
> "/mmc at fe330000", "/mmc at 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 at ff1d0000/flash at 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 at ff1d0000/flash at 0";
> stdout-path = "serial0:115200n8";
> u-boot,spl-boot-order = "same-as-spl", "/spi at ff1d0000/flash at 0",
> "/mmc at fe330000", "/mmc at 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 :) ).
Correct, as long as the spi flash node was added to the spl-boot-order
there has not been any noticeable issue.
TF-A overwriting the boot source id reg is something I have only seen on
RK3399 so far. I think this is not an issue on RK3328 and RK35xx, but I
could be wrong.
It was only when testing the SPI flash patches for ROCK Pi 4 and my
suggestion to drop the explicit listing of the flash node that I noticed
that there was something not working correctly with the same-as-spl code
path.
>
> 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?
Sure, I will include a fixes tag in a v2.
>
>> 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;
>> + }
>
> 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.
Correct, it is technically not needed, it was mostly added there was an
effort to clean up boot source related debug messages in spl-boot-order.c
and this adds a different grepable debug message.
>
> 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.
Agree, I will add a BROM_BOOTSOURCE_INVALID and change debug message to
invalid instead of unknown in v2.
>
> 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.
1 - 10 is the only expected valid boot source id, anything else would be
considered invalid and is ignored.
The intent of this patch was to cache the first valid boot source id
that is read back from the reg, technically it currently also cache
invalid values, will fix that in a v2.
I have only seen the reg being overwritten to 0 for both open source and
RK TF-A on RK3399. And with current intended use of the function the
value is always read at least once before FIT images is loaded so I am
not that concerned that we will end up with a wrong or bad value.
Regards,
Jonas
>
> Cheers,
> Quentin
More information about the U-Boot
mailing list