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

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

[Jonas Karlman]

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";
  };

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;
+	}
+
 	if (bootdevice_brom_id < ARRAY_SIZE(boot_devices))
 		bootdevice_ofpath = boot_devices[bootdevice_brom_id];
 
-- 
2.43.2

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

[Kever Yang]

On 2024/3/16 01: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";
>    };
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   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;
> +	}
> +
>   	if (bootdevice_brom_id < ARRAY_SIZE(boot_devices))
>   		bootdevice_ofpath = boot_devices[bootdevice_brom_id];
>   

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

[Dragan Simic]

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.

> 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@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";
>   };
> 
> 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;
> +	}
> +

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 (bootdevice_brom_id < ARRAY_SIZE(boot_devices))
>  		bootdevice_ofpath = boot_devices[bootdevice_brom_id];

Re: [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@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";
>>   };
>>
>> 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;
>> +	}
>> +
> 
> 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];

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

[Dragan Simic]

On 2024-03-19 16:59, Jonas Karlman wrote:
> On 2024-03-19 10:44, Dragan Simic wrote:
>> On 2024-03-15 18:34, Jonas Karlman wrote:
>>> 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.

Is it mandatory that this static variable ends up in the .bss section?

> 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.

As I already described, making the code more complex would intentionally
introduce a failure point, which would be triggered in case obtaining
valid value from readl() fails the first time.  Something like that is
usually known as a canary, which I'm sure you already know about.

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

[Quentin Schulz]

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@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 :) ).

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.

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.

Cheers,
Quentin

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

[Dragan Simic]

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.

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

[Jonas Karlman]

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@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?

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@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 :) ).

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@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