Hi Jonas, On Mon, 17 Feb 2025 at 13:28, Jonas Karlman <jo...@kwiboo.se> wrote: > > Hi Simon, > > On 2025-02-09 22:14, Simon Glass wrote: > > The most common word for features that make a platform work is to use > > 'HAVE_xxx'. Rename this option to match. > > We already have HAS_CUSTOM_SYS_INIT_SP_ADDR related to the init stack > pointer address, maybe the new symbol should be named something similar > to that? e.g. HAS_xPL_INIT_SP_ADDR
That would be quite confusing, since we still need SYS_INIT_SP_ADDR that option to work. The name would have to be xPL_HAS_SP_INIT_SP_ADDR to fit with the naming convention and allow CONFIG_IS_ENABLED() and avoid more confusion. Plus, SYS_INIT_SP_ADDR is actually a fallback for when the xPL builds don't provide their own address. So I think this needs to be separate, particularly as Tom is talking of splitting the deconfig files, at which point we wouldn't be able to make xPL use SYS_INIT_SP_ADDR as a default. > > > > > Update the help to use the word 'phase' rather than 'stage', since > > that is the current terminology. Also clarify that, absent this setting, > > the stack pointer generally comes from the value used by U-Boot proper, > > rather than SPL. > > The last statement is not fully correct, see below. > > > > > Move the option just above TPL_STACK which depends on it. > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > --- > > > > Changes in v2: > > - Add new patch to rename TPL_NEEDS_SEPARATE_STACK > > > > arch/arm/lib/crt0.S | 2 +- > > arch/arm/lib/crt0_64.S | 2 +- > > arch/arm/mach-rockchip/Kconfig | 14 +++++++------- > > common/spl/Kconfig.tpl | 19 ++++++++++--------- > > configs/mk808_defconfig | 2 +- > > 5 files changed, 20 insertions(+), 19 deletions(-) > > > > diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S > > index a691d844847..a7e6f50d39a 100644 > > --- a/arch/arm/lib/crt0.S > > +++ b/arch/arm/lib/crt0.S > > @@ -100,7 +100,7 @@ ENTRY(_main) > > * Set up initial C runtime environment and call board_init_f(0). > > */ > > > > -#if defined(CONFIG_TPL_BUILD) && defined(CONFIG_TPL_NEEDS_SEPARATE_STACK) > > +#if defined(CONFIG_TPL_BUILD) && defined(CONFIG_TPL_HAVE_INIT_STACK) > > ldr r0, =(CONFIG_TPL_STACK) > > #elif defined(CONFIG_XPL_BUILD) && defined(CONFIG_SPL_STACK) > > ldr r0, =(CONFIG_SPL_STACK) > > Here we can see that SPL_STACK is used as the first fallback for > XPL_BUILD's. Oh dear. I believe that is unintended, but I'm not sure which way to take it. For now I'll update the comment. > > > diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S > > index 32401f544a7..62a0abe1fed 100644 > > --- a/arch/arm/lib/crt0_64.S > > +++ b/arch/arm/lib/crt0_64.S > > @@ -69,7 +69,7 @@ ENTRY(_main) > > /* > > * Set up initial C runtime environment and call board_init_f(0). > > */ > > -#if defined(CONFIG_TPL_BUILD) && defined(CONFIG_TPL_NEEDS_SEPARATE_STACK) > > +#if defined(CONFIG_TPL_BUILD) && defined(CONFIG_TPL_HAVE_INIT_STACK) > > ldr x0, =(CONFIG_TPL_STACK) > > #elif defined(CONFIG_XPL_BUILD) && defined(CONFIG_SPL_STACK) > > ldr x0, =(CONFIG_SPL_STACK) > > diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig > > index 4d3157b2edd..4c515593718 100644 > > --- a/arch/arm/mach-rockchip/Kconfig > > +++ b/arch/arm/mach-rockchip/Kconfig > > @@ -9,7 +9,7 @@ config ROCKCHIP_PX30 > > select SPL > > select TPL > > select TPL_TINY_FRAMEWORK if TPL > > - select TPL_NEEDS_SEPARATE_STACK if TPL > > + select TPL_HAVE_INIT_STACK if TPL > > imply SPL_SEPARATE_BSS > > select SPL_SERIAL > > select TPL_SERIAL > > @@ -106,7 +106,7 @@ config ROCKCHIP_RK322X > > select TPL > > select TPL_DM > > select TPL_OF_LIBFDT > > - select TPL_NEEDS_SEPARATE_STACK if TPL > > + select TPL_HAVE_INIT_STACK if TPL > > select SPL_DRIVERS_MISC > > imply ROCKCHIP_COMMON_BOARD > > imply SPL_SERIAL > > @@ -139,7 +139,7 @@ config ROCKCHIP_RK3288 > > imply TPL_DRIVERS_MISC > > imply TPL_LIBCOMMON_SUPPORT > > imply TPL_LIBGENERIC_SUPPORT > > - imply TPL_NEEDS_SEPARATE_STACK > > + imply TPL_HAVE_INIT_STACK > > imply TPL_OF_CONTROL > > imply TPL_OF_PLATDATA > > imply TPL_RAM > > @@ -197,7 +197,7 @@ config ROCKCHIP_RK3328 > > select SPL > > select SUPPORT_TPL > > select TPL > > - select TPL_NEEDS_SEPARATE_STACK if TPL > > + select TPL_HAVE_INIT_STACK if TPL > > imply ARMV8_CRYPTO > > imply ARMV8_SET_SMPEN > > imply MISC > > @@ -225,7 +225,7 @@ config ROCKCHIP_RK3368 > > select ARM64 > > select SUPPORT_SPL > > select SUPPORT_TPL > > - select TPL_NEEDS_SEPARATE_STACK if TPL > > + select TPL_HAVE_INIT_STACK if TPL > > imply ROCKCHIP_COMMON_BOARD > > imply SPL_ROCKCHIP_COMMON_BOARD > > imply SPL_SEPARATE_BSS > > @@ -257,7 +257,7 @@ config ROCKCHIP_RK3399 > > select SPL_RAM if SPL > > select SPL_REGMAP if SPL > > select SPL_SYSCON if SPL > > - select TPL_NEEDS_SEPARATE_STACK if TPL > > + select TPL_HAVE_INIT_STACK if TPL > > select SPL_SEPARATE_BSS > > select CLK > > select FIT > > @@ -392,7 +392,7 @@ config ROCKCHIP_RV1126 > > select SKIP_LOWLEVEL_INIT_ONLY > > select TPL > > select SUPPORT_TPL > > - select TPL_NEEDS_SEPARATE_STACK > > + select TPL_HAVE_INIT_STACK > > select TPL_ROCKCHIP_BACK_TO_BROM > > select SPL > > select SUPPORT_SPL > > diff --git a/common/spl/Kconfig.tpl b/common/spl/Kconfig.tpl > > index 22ca7016453..980d6da202a 100644 > > --- a/common/spl/Kconfig.tpl > > +++ b/common/spl/Kconfig.tpl > > @@ -106,12 +106,6 @@ config TPL_LDSCRIPT > > May be left empty to trigger the Makefile infrastructure to > > fall back to the linker-script used for the SPL stage. > > > > -config TPL_NEEDS_SEPARATE_STACK > > - bool "TPL needs a separate initial stack-pointer" > > - help > > - Enable, if the TPL stage should not inherit its initial > > - stack-pointer from the settings for the SPL stage. > > - > > config TPL_POWER > > bool "Support power drivers" > > help > > @@ -140,11 +134,18 @@ config TPL_MAX_SIZE > > help > > The maximum size (in bytes) of the TPL stage. > > > > +config TPL_HAVE_INIT_STACK > > + bool "TPL requires a initial, fixed, stack-pointer location" > > + help > > + Enable if the TPL phase should not use inherit its initial > > "should not use inherit its initial" does not sound correct. Fixed > > > + stack-pointer from the settings for U-Boot proper, but should set > > Here you are changing from SPL to proper, however the SPL address will > be used as the fallback (if it is defined) and then SYS_INIT_SP_ADDR. OK, I'll update that. > > > + its own value. > > + > > config TPL_STACK > > - hex "Address of the initial stack-pointer for the TPL stage" > > - depends on TPL_NEEDS_SEPARATE_STACK > > + hex "Address of the initial stack-pointer for the TPL phase" > > + depends on TPL_HAVE_INIT_STACK > > help > > - The address of the initial stack-pointer for the TPL stage. > > + The address of the initial stack-pointer for the TPL phase > > Did you remove the . (period) on purpose? Yes, as it is not a sentence > > Regards, > Jonas > > > Usually this will be the (aligned) top-of-stack. > > > > config TPL_READ_ONLY > > diff --git a/configs/mk808_defconfig b/configs/mk808_defconfig > > index e47d0b594f3..a1f089a23ea 100644 > > --- a/configs/mk808_defconfig > > +++ b/configs/mk808_defconfig > > @@ -48,7 +48,7 @@ CONFIG_SPL_NO_BSS_LIMIT=y > > CONFIG_SPL_SEPARATE_BSS=y > > CONFIG_SPL_FS_EXT4=y > > CONFIG_SYS_MMCSD_FS_BOOT_PARTITION=2 > > -CONFIG_TPL_NEEDS_SEPARATE_STACK=y > > +CONFIG_TPL_HAVE_INIT_STACK=y > > # CONFIG_BOOTM_PLAN9 is not set > > # CONFIG_BOOTM_RTEMS is not set > > # CONFIG_BOOTM_VXWORKS is not set > Regards, Simon
