Hi Alex, On 2025-02-05 20:02, Alex Bee wrote: > Hi Jonas, > >> Hi Alex, >> >> On 2025-01-27 19:59, Alex Bee wrote: >>> Hi Jonas, >>> >>> thanks for working on this. Please see one comment below. >>> >>> Am 27.01.25 um 00:48 schrieb Jonas Karlman: >>>> A few Rockchip ARMv7 SoCs use 0x60000000 as DRAM base address instead of >>>> the more common 0x0 DRAM base address used on AArch64 SoCs. >>>> >>>> Add default options that should work for these ARMv7 SoCs. Same offsets >>>> as before are used, just below 64 MiB. Hex values have also been padded >>>> to improve alignment. >>>> >>>> Signed-off-by: Jonas Karlman <jo...@kwiboo.se> >>>> --- >>>> arch/arm/mach-rockchip/Kconfig | 35 +++++++++++++++++++++++----------- >>>> 1 file changed, 24 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-rockchip/Kconfig >>>> b/arch/arm/mach-rockchip/Kconfig >>>> index 5aa9e0f1af3b..da20d3233fe3 100644 >>>> --- a/arch/arm/mach-rockchip/Kconfig >>>> +++ b/arch/arm/mach-rockchip/Kconfig >>>> @@ -637,40 +637,53 @@ source "arch/arm/mach-rockchip/rv1126/Kconfig" >>>> if ROCKCHIP_COMMON_STACK_ADDR && SPL_SHARES_INIT_SP_ADDR >>>> >>>> config CUSTOM_SYS_INIT_SP_ADDR >>>> - default 0x3f00000 >>>> + default 0x63f00000 if SPL_TEXT_BASE = 0x60000000 >>>> + default 0x03f00000 if SPL_TEXT_BASE = 0x00000000 >>>> >>>> config SYS_MALLOC_F_LEN >>>> - default 0x10000 if CUSTOM_SYS_INIT_SP_ADDR = 0x3f00000 >>>> + default 0x10000 if CUSTOM_SYS_INIT_SP_ADDR = 0x63f00000 >>>> + default 0x10000 if CUSTOM_SYS_INIT_SP_ADDR = 0x03f00000 >>>> >>>> config SPL_SYS_MALLOC_F_LEN >>>> - default 0x8000 if CUSTOM_SYS_INIT_SP_ADDR = 0x3f00000 >>>> + default 0x8000 if CUSTOM_SYS_INIT_SP_ADDR = 0x63f00000 >>>> + default 0x8000 if CUSTOM_SYS_INIT_SP_ADDR = 0x03f00000 >>>> >>>> config TPL_SYS_MALLOC_F_LEN >>>> - default 0x4000 if CUSTOM_SYS_INIT_SP_ADDR = 0x3f00000 >>>> + default 0x0800 if CUSTOM_SYS_INIT_SP_ADDR = 0x63f00000 >>>> + default 0x4000 if CUSTOM_SYS_INIT_SP_ADDR = 0x03f00000 >>>> >>>> config TEXT_BASE >>>> - default 0x00200000 if ARM64 >>>> + default 0x60200000 if SPL_TEXT_BASE = 0x60000000 >>>> + default 0x00200000 if SPL_TEXT_BASE = 0x00000000 >>>> >>>> config SPL_TEXT_BASE >>>> - default 0x0 if ARM64 >>>> + default 0x60000000 if ROCKCHIP_RK3036 || ROCKCHIP_RK3066 || \ >>>> + ROCKCHIP_RK3128 || ROCKCHIP_RK3188 || \ >>> I'm not sure if adding RK3036 and RK3188 is correct at this point. Both use >>> SPL (without TPL) currently for DRAM initialization and thus need their >>> CFG_IRAM_BASE as SPL_TEXT_BASE. >> I think it should be safe to add these default values here, any value >> defined in board defconfig or soc Kconfig will take precedence. >> >> The RK3036 and RK3188 boards use SPL back-to-brom to load U-Boot proper, >> if someone wants to move these board to use similar TPL+SPL+FIT this >> will prepare for such occasion. >> >> Having them included here also prevents a possible issue in case someone >> in the future where to add a select/imply ROCKCHIP_COMMON_STACK_ADDR for >> these SoCs, else they may fallback to wrongly use 0x0. >> >> Do you want me to drop them in a v2? >> >> Regards, >> Jonas > it is currently "safe" as it would of course be overwritten by the > currently existing defconfigs. But if someone dares to use this symbol for > a downstream/new board for either of these two SoCs, things go south. > > How about adding some "extra security" by making it > depends on TPL || ROCKCHIP_EXTERNAL_TPL > > As I mentioned off-list: I'm not sure if any of these > XXX_ROCKCHIP_COMMON_BORD belong here: I understand the goal of syncing > configurations for the different SoCs, but I'm not sure if a symbol called > ROCKCHIP_COMMON_STACK_ADDR is the right place for it (plus the > XXX_ROCKCHIP_COMMON_BORD symbols (should) have no dependency on using > ROCKCHIP_COMMON_STACK_ADDR, neither code-wise nor from semantics).
In principle I fully agree, and TPL_ROCKCHIP_COMMON_BOARD was moved away from being implied in first patch in this series. However, I left the implied SPL symbol as-is. Adding the "depends on TPL ..." could be something for a v2. The original intent of this symbol was to help move boards and SoCs that use common TPL+SPL to use safer addresses without having to affect all boards/SoCs all at once. With this series it should be simple to move the remaining ARMv7 SoCs to use similar TPL/ROCKCHIP_EXTERNAL_TPL + SPL separation without having to worry about picking new SPL stack addresses in DRAM. The Kconfig symbol is hidden from menuconfig so I do not expect end users to easily break anything by accidentally enabling this symbol. I was hoping to be able to send a migration series for RK322x this weekend but unfortunately, I managed to brick the new RK3228A box I picked up after only a few hours, the box uses an eMCP and I have not found an exposed mmc clk pin yet, a new box should arrive soon :-) Regards, Jonas > > Regards, > Alex > >>> Regards, >>> >>> Alex >>> >>>> + ROCKCHIP_RK322X || ROCKCHIP_RV1108 >>>> + default 0x00000000 >>>> >>>> config SPL_HAS_BSS_LINKER_SECTION >>>> default y if ARM64 >>>> >>>> config SPL_BSS_START_ADDR >>>> - default 0x3f80000 >>>> + default 0x63f80000 if SPL_TEXT_BASE = 0x60000000 >>>> + default 0x03f80000 if SPL_TEXT_BASE = 0x00000000 >>>> >>>> config SPL_BSS_MAX_SIZE >>>> - default 0x8000 if SPL_BSS_START_ADDR = 0x3f80000 >>>> + default 0x8000 if SPL_BSS_START_ADDR = 0x63f80000 >>>> + default 0x8000 if SPL_BSS_START_ADDR = 0x03f80000 >>>> >>>> config SPL_STACK_R >>>> - default y if CUSTOM_SYS_INIT_SP_ADDR = 0x3f00000 >>>> + default y if CUSTOM_SYS_INIT_SP_ADDR = 0x63f00000 >>>> + default y if CUSTOM_SYS_INIT_SP_ADDR = 0x03f00000 >>>> >>>> config SPL_STACK_R_ADDR >>>> - default 0x3e00000 if CUSTOM_SYS_INIT_SP_ADDR = 0x3f00000 >>>> + default 0x63e00000 if CUSTOM_SYS_INIT_SP_ADDR = 0x63f00000 >>>> + default 0x03e00000 if CUSTOM_SYS_INIT_SP_ADDR = 0x03f00000 >>>> >>>> config SPL_STACK_R_MALLOC_SIMPLE_LEN >>>> - default 0x200000 if SPL_STACK_R_ADDR = 0x3e00000 >>>> + default 0x200000 if SPL_STACK_R_ADDR = 0x63e00000 >>>> + default 0x200000 if SPL_STACK_R_ADDR = 0x03e00000 >>>> >>>> endif >>>> endif
