[PATCH 2/2] rockchip: rk3399: Update stack and bss addresses

[Quentin Schulz]

Hi Jonas,

On 2/12/24 12:08, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2024-02-12 11:37, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 2/12/24 01:59, Jonas Karlman wrote:
>>> With the stack and text base used by U-Boot SPL and proper on RK3399
>>> there is a high likelihood of overlapping when U-Boot proper + FDT nears
>>> 1 MiB in size.
>>>
>>> Currently the following memory layout is used:
>>> - 0x00000000 (@0 MiB): U-Boot SPL text base
>>> - 0x00040000 (@256 KiB): TF-A
>>> - 0x00200000 (@2 MiB): U-Boot proper text base
>>> - 0x00300000 (@3 MiB): U-Boot proper init stack
>>> - 0x00400000 (@4 MiB): U-Boot SPL init stack
>>> - 0x00400000 (@4 MiB): U-Boot SPL bss
>>> - 0x02000000 (@64 MiB): U-Boot SPL reloc stack
>>> - 0x08400000 (@132 MiB): optional OPTEE
>>>
>>> SPL load TF-A, U-Boot proper and optional OPTEE after SPL stack has
>>> relocated, meaning that there is room for close to 2 MiB (@2-4 MiB).
>>> However, the initial stack used by U-Boot proper is limiting the size
>>> of U-Boot proper + FDT to below 1 MiB (@2-3 MiB).
>>>
>>> Instead change to use the following memory layout:
>>> - 0x00000000 (@0 MiB): U-Boot SPL text base
>>> - 0x00040000 (@256 KiB): TF-A
>>> - 0x00200000 (@2 MiB): U-Boot proper text base
>>> - 0x00400000 (@4 MiB): U-Boot SPL init stack
>>> - 0x00800000 (@8 MiB): U-Boot proper init stack (changed)
>>> - 0x02000000 (@32 MiB): U-Boot SPL bss (changed)
>>> - 0x04000000 (@64 MiB): U-Boot SPL reloc stack
>>> - 0x08400000 (@132 MiB): optional OPTEE
>>>
>>> This should leave room for loading and running a U-Boot proper close
>>> to 6 MiB (@2-8 MiB) in size. When U-Boot proper has relocated is should
>>> be possible to use @2-132 MiB and @164 MiB+ for scripts, kernel etc.
>>>
>>> With the moved SPL reloc stack it is also possible to use a much larger
>>> malloc area in SPL, e.g. using default SPL_STACK_R_MALLOC_SIMPLE_LEN.
>>>
>>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>>
>> Do we not need to update memory addresses in the environment as well?
> 
> The addresses in environment is to my knowledge only used in U-Boot
> proper after relocation. And at that time we should be free to use @2+
> MiB up to somewhere close to gd ram_top/relocaddr. The optional Rockchip
> OPTEE blobb may occupy @132-164 MiB, but I do not think loading OPTEE
> blobs works as-is, so that may not be a big issue.
> 

Upstream OP-TEE OS seems to support RK3399, c.f. 
https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-rockchip/platform_rk3399.c

I cannot say whether it works, I have no knowledge of any of our 
customers using it (but that predates my joining Theobroma, so maybe 
long time customers do use it?). However, we do use an upstream-based 
OP-TEE OS for one of our PX30-based boards, so I would say Rockchip's 
OP-TEE OS is not the only option so we shouldn't just disregard this 
location in RAM... especially since ramdisk_addr_r starts at 96MiB. But 
since we're not changing that location.... people who would have this 
issue, would have encountered the issue by now already (provided they 
load a ramdisk bigger than 36MiB). So let's ignore that one actually :)

> So to my knowledge we do not need to update any of the memory addresses
> in the environment as well.
> 
>>
>> scriptaddr=0x00500000
>>
>> This is at 5MiB if I'm not mistaken?
>>
>> Basically everything in include/configs/rk3399_common.h in
>> ENV_MEM_LAYOUT_SETTINGS needs to be updated to be located after 164MiB?
>>
>> Did I misunderstand something? I find it weird the current values seems
>> to be located after U-Boot SPL bss and that e.g.
>>
>> kernel_addr_r=0x02080000
>>
>> is at 32.5MiB, and with U-Boot SPL reloc stack currently at 64MiB... a
>> 31.5+MiB kernel Image would mess up the data there? (what do we need it
>> for BTW once in U-boot proper after relocation?).
> 
> After SPL has jumped to TF-A and U-boot proper any memory used by SPL
> can be overwritten/used for something else.
> 
> My understanding is as follow:
> - SPL always need access to bss
> - SPL need access to init stack until it has relocated to reloc stack
> - SPL will load FIT images using the reloc stack, at this time only
>    SPL code, bss and reloc stack matters
> - U-boot proper will use its init stack until it has relocated to end
>    of memory. Until U-Boot proper has relocated it will use init stack
> - We could use same memory position for init stack for SPL and proper,
>    e.g. SPL_SHARES_INIT_SP_ADDR=y without any issue
> - After U-Boot proper has relocated we should be free to use @2+ MiB
>    up to where U-Boot proper was relocated.
> 

Not sure how Falcon boot fit in all of this and whether it makes use of 
the environment variable (likely in the SPL binary itself).

Was this from experience and/or is there some literature in U-Boot git 
repo already? Couldn't find much in the 5min I looked for it :)

Thanks for the taking the time to explain this, much appreciated.

>>
>> Side question, what about making those values the default for RK3399
>> instead of having it in each defconfig? This could grow quickly out of
>> hands to move this to the Kconfig symbol itself, so not sure it's that
>> interesting to the project, but raising this anyway.
> 
> Agree, was trying to define these as defaults in soc Kconfig, but that
> rearranged some values in other defconfigs so I avoided doing that in
> this series. Also touching each defconfig helped me ping board
> maintainers of the change.
> 
> Hopefully a follow-up series can move these, and other options, into
> soc Kconfig.
> 

I was more suggesting adding:
"""
diff --git a/Kconfig b/Kconfig
index 00ed1ecc173..b816e536674 100644
--- a/Kconfig
+++ b/Kconfig
@@ -249,6 +249,7 @@ config HAS_CUSTOM_SYS_INIT_SP_ADDR
  config CUSTOM_SYS_INIT_SP_ADDR
  	hex "Static location for the initial stack pointer"
  	depends on HAS_CUSTOM_SYS_INIT_SP_ADDR
+	default 0x800000 if ROCKCHIP_RK3399
  	default TEXT_BASE if TFABOOT

  config SYS_MALLOC_F
diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 6405374bcc1..08b71713b79 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -121,6 +121,7 @@ config SPL_BSS_START_ADDR
  	default 0x81f80000 if ARCH_SUNXI && MACH_SUNIV
  	default 0x4ff80000 if ARCH_SUNXI && !(MACH_SUN9I || MACH_SUNIV)
  	default 0x2ff80000 if ARCH_SUNXI && MACH_SUN9I
+	default 0x02000000 if ROCKCHIP_RK3399
  	default 0x1000 if ARCH_ZYNQMP

  choice
"""

So that boards could still override it in defconfigs if they really want 
to. I don't have knowledge of any other way to provide an overridable 
default?

Anyway, a topic for another series, nothing requiring to block this 
series, so:

Reviewed-by: Quentin Schulz <quentin.schulz at theobroma-systems.com>

Thanks,
Quentin