* [PATCH] bootstash: Do not provide a default address for all
@ 2024-07-03 21:23 Tom Rini
2024-07-05 8:36 ` Simon Glass
2024-07-11 21:27 ` [v2] " Tom Rini
0 siblings, 2 replies; 6+ messages in thread
From: Tom Rini @ 2024-07-03 21:23 UTC (permalink / raw)
To: u-boot; +Cc: Simon Glass
A valid memory location to stash bootstage information at will be
architecture dependent. Move the existing defaults to the main Kconfig
file for this option and set 0x0 as the default only for sandbox.
Signed-off-by: Tom Rini <trini@konsulko.com>
---
Cc: Simon Glass <sjg@chromium.org>
---
arch/arm/mach-stm32mp/Kconfig.13x | 3 ---
arch/arm/mach-stm32mp/Kconfig.15x | 3 ---
arch/arm/mach-stm32mp/Kconfig.25x | 3 ---
boot/Kconfig | 4 +++-
4 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/arch/arm/mach-stm32mp/Kconfig.13x b/arch/arm/mach-stm32mp/Kconfig.13x
index 4d74b35055b8..bc8b3f8cf77f 100644
--- a/arch/arm/mach-stm32mp/Kconfig.13x
+++ b/arch/arm/mach-stm32mp/Kconfig.13x
@@ -28,9 +28,6 @@ config PRE_CON_BUF_ADDR
config PRE_CON_BUF_SZ
default 4096
-config BOOTSTAGE_STASH_ADDR
- default 0xC3000000
-
if BOOTCOUNT_GENERIC
config SYS_BOOTCOUNT_SINGLEWORD
default y
diff --git a/arch/arm/mach-stm32mp/Kconfig.15x b/arch/arm/mach-stm32mp/Kconfig.15x
index d99aa9fd694a..42da36a73e85 100644
--- a/arch/arm/mach-stm32mp/Kconfig.15x
+++ b/arch/arm/mach-stm32mp/Kconfig.15x
@@ -86,9 +86,6 @@ config PRE_CON_BUF_ADDR
config PRE_CON_BUF_SZ
default 4096
-config BOOTSTAGE_STASH_ADDR
- default 0xC3000000
-
if BOOTCOUNT_GENERIC
config SYS_BOOTCOUNT_SINGLEWORD
default y
diff --git a/arch/arm/mach-stm32mp/Kconfig.25x b/arch/arm/mach-stm32mp/Kconfig.25x
index 2c0f691f8b54..7d2d8171845b 100644
--- a/arch/arm/mach-stm32mp/Kconfig.25x
+++ b/arch/arm/mach-stm32mp/Kconfig.25x
@@ -24,9 +24,6 @@ config PRE_CON_BUF_ADDR
config PRE_CON_BUF_SZ
default 4096
-config BOOTSTAGE_STASH_ADDR
- default 0x87000000
-
if DEBUG_UART
config DEBUG_UART_BOARD_INIT
diff --git a/boot/Kconfig b/boot/Kconfig
index 11175fb7bb26..6fb77b23f3c8 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -1002,7 +1002,9 @@ config BOOTSTAGE_STASH
config BOOTSTAGE_STASH_ADDR
hex "Address to stash boot timing information"
- default 0x0
+ default 0xC3000000 if STM32MP13X || STM32MP15X
+ default 0x87000000 if STM32MP25X
+ default 0x0 if SANDBOX
help
Provide an address which will not be overwritten by the OS when it
starts, so that it can read this information when ready.
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bootstash: Do not provide a default address for all
2024-07-03 21:23 [PATCH] bootstash: Do not provide a default address for all Tom Rini
@ 2024-07-05 8:36 ` Simon Glass
2024-07-11 21:27 ` [v2] " Tom Rini
1 sibling, 0 replies; 6+ messages in thread
From: Simon Glass @ 2024-07-05 8:36 UTC (permalink / raw)
To: Tom Rini; +Cc: u-boot
Hi Tom,
On Wed, 3 Jul 2024 at 22:23, Tom Rini <trini@konsulko.com> wrote:
>
> A valid memory location to stash bootstage information at will be
> architecture dependent. Move the existing defaults to the main Kconfig
> file for this option and set 0x0 as the default only for sandbox.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Cc: Simon Glass <sjg@chromium.org>
> ---
> arch/arm/mach-stm32mp/Kconfig.13x | 3 ---
> arch/arm/mach-stm32mp/Kconfig.15x | 3 ---
> arch/arm/mach-stm32mp/Kconfig.25x | 3 ---
> boot/Kconfig | 4 +++-
> 4 files changed, 3 insertions(+), 10 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
But from my previous investigations, having no default for a hex
option can cause the build to hang unless the user is presentt, since
the build keeps asking for the value. I added support in buildman to
try to detect this, so we'll see how it goes.
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v2] bootstash: Do not provide a default address for all
2024-07-03 21:23 [PATCH] bootstash: Do not provide a default address for all Tom Rini
2024-07-05 8:36 ` Simon Glass
@ 2024-07-11 21:27 ` Tom Rini
2024-07-13 15:13 ` Simon Glass
1 sibling, 1 reply; 6+ messages in thread
From: Tom Rini @ 2024-07-11 21:27 UTC (permalink / raw)
To: u-boot
A valid memory location to stash bootstage information at will be
architecture dependent. Move the existing defaults to the main Kconfig
file for this option and set 0x0 as the default only for sandbox.
Signed-off-by: Tom Rini <trini@konsulko.com>
---
Changes in v2:
- Seeing that BOOTSTAGE_STASH_ADDR did not depend on BOOTSTAGE_STASH in
turn lead to discovering that (minor) BOOTSTAGE_STASH_SIZE was also
missing a depends on line and then that a number of places built code
with BOOTSTAGE_STASH_ADDR=0x0 as a compiles-but-likely-fails option.
Rework a number of spots to guard usage around BOOTSTAGE_STASH being
enabled.
---
arch/arm/mach-rockchip/tpl.c | 4 ++--
arch/arm/mach-stm32mp/Kconfig.13x | 3 ---
arch/arm/mach-stm32mp/Kconfig.15x | 3 ---
arch/arm/mach-stm32mp/Kconfig.25x | 3 ---
arch/x86/cpu/cpu.c | 2 ++
boot/Kconfig | 6 +++++-
cmd/bootstage.c | 8 +++++++-
common/board_f.c | 2 ++
common/bootstage.c | 2 ++
common/spl/spl.c | 2 ++
include/bootstage.h | 14 ++++++++------
11 files changed, 30 insertions(+), 19 deletions(-)
diff --git a/arch/arm/mach-rockchip/tpl.c b/arch/arm/mach-rockchip/tpl.c
index 50f04f9474a0..a47cba5163ab 100644
--- a/arch/arm/mach-rockchip/tpl.c
+++ b/arch/arm/mach-rockchip/tpl.c
@@ -92,10 +92,10 @@ void board_init_f(ulong dummy)
int board_return_to_bootrom(struct spl_image_info *spl_image,
struct spl_boot_device *bootdev)
{
-#ifdef CONFIG_BOOTSTAGE_STASH
- int ret;
+ int __maybe_unused ret;
bootstage_mark_name(BOOTSTAGE_ID_END_TPL, "end tpl");
+#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
ret = bootstage_stash((void *)CONFIG_BOOTSTAGE_STASH_ADDR,
CONFIG_BOOTSTAGE_STASH_SIZE);
if (ret)
diff --git a/arch/arm/mach-stm32mp/Kconfig.13x b/arch/arm/mach-stm32mp/Kconfig.13x
index 4d74b35055b8..bc8b3f8cf77f 100644
--- a/arch/arm/mach-stm32mp/Kconfig.13x
+++ b/arch/arm/mach-stm32mp/Kconfig.13x
@@ -28,9 +28,6 @@ config PRE_CON_BUF_ADDR
config PRE_CON_BUF_SZ
default 4096
-config BOOTSTAGE_STASH_ADDR
- default 0xC3000000
-
if BOOTCOUNT_GENERIC
config SYS_BOOTCOUNT_SINGLEWORD
default y
diff --git a/arch/arm/mach-stm32mp/Kconfig.15x b/arch/arm/mach-stm32mp/Kconfig.15x
index d99aa9fd694a..42da36a73e85 100644
--- a/arch/arm/mach-stm32mp/Kconfig.15x
+++ b/arch/arm/mach-stm32mp/Kconfig.15x
@@ -86,9 +86,6 @@ config PRE_CON_BUF_ADDR
config PRE_CON_BUF_SZ
default 4096
-config BOOTSTAGE_STASH_ADDR
- default 0xC3000000
-
if BOOTCOUNT_GENERIC
config SYS_BOOTCOUNT_SINGLEWORD
default y
diff --git a/arch/arm/mach-stm32mp/Kconfig.25x b/arch/arm/mach-stm32mp/Kconfig.25x
index 2c0f691f8b54..7d2d8171845b 100644
--- a/arch/arm/mach-stm32mp/Kconfig.25x
+++ b/arch/arm/mach-stm32mp/Kconfig.25x
@@ -24,9 +24,6 @@ config PRE_CON_BUF_ADDR
config PRE_CON_BUF_SZ
default 4096
-config BOOTSTAGE_STASH_ADDR
- default 0x87000000
-
if DEBUG_UART
config DEBUG_UART_BOARD_INIT
diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
index c8433360f28e..7ce6443cc77e 100644
--- a/arch/x86/cpu/cpu.c
+++ b/arch/x86/cpu/cpu.c
@@ -75,8 +75,10 @@ int __weak x86_cleanup_before_linux(void)
ret = mp_park_aps();
if (ret)
return log_msg_ret("park", ret);
+#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
bootstage_stash((void *)CONFIG_BOOTSTAGE_STASH_ADDR,
CONFIG_BOOTSTAGE_STASH_SIZE);
+#endif
return 0;
}
diff --git a/boot/Kconfig b/boot/Kconfig
index ffcae840a506..ba8bddd14219 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -1002,13 +1002,17 @@ config BOOTSTAGE_STASH
config BOOTSTAGE_STASH_ADDR
hex "Address to stash boot timing information"
- default 0x0
+ depends on BOOTSTAGE_STASH
+ default 0xC3000000 if STM32MP13X || STM32MP15X
+ default 0x87000000 if STM32MP25X
+ default 0x0 if SANDBOX
help
Provide an address which will not be overwritten by the OS when it
starts, so that it can read this information when ready.
config BOOTSTAGE_STASH_SIZE
hex "Size of boot timing stash region"
+ depends on BOOTSTAGE_STASH
default 0x1000
help
This should be large enough to hold the bootstage stash. A value of
diff --git a/cmd/bootstage.c b/cmd/bootstage.c
index 5246924f39a4..4ae09a6fd03d 100644
--- a/cmd/bootstage.c
+++ b/cmd/bootstage.c
@@ -15,6 +15,7 @@ static int do_bootstage_report(struct cmd_tbl *cmdtp, int flag, int argc,
return 0;
}
+#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
static int get_base_size(int argc, char *const argv[], ulong *basep,
ulong *sizep)
{
@@ -58,11 +59,14 @@ static int do_bootstage_stash(struct cmd_tbl *cmdtp, int flag, int argc,
return 0;
}
+#endif
static struct cmd_tbl cmd_bootstage_sub[] = {
U_BOOT_CMD_MKENT(report, 2, 1, do_bootstage_report, "", ""),
+#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
U_BOOT_CMD_MKENT(stash, 4, 0, do_bootstage_stash, "", ""),
U_BOOT_CMD_MKENT(unstash, 4, 0, do_bootstage_stash, "", ""),
+#endif
};
/*
@@ -91,6 +95,8 @@ U_BOOT_CMD(bootstage, 4, 1, do_boostage,
"Boot stage command",
" - check boot progress and timing\n"
"report - Print a report\n"
+#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
"stash [<start> [<size>]] - Stash data into memory\n"
- "unstash [<start> [<size>]] - Unstash data from memory"
+ "unstash [<start> [<size>]] - Unstash data from memory\n"
+#endif
);
diff --git a/common/board_f.c b/common/board_f.c
index 212ffb3090b2..37ae1641f1f0 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -809,6 +809,7 @@ static int initf_bootstage(void)
ret = bootstage_init(!from_spl);
if (ret)
return ret;
+#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
if (from_spl) {
const void *stash = map_sysmem(CONFIG_BOOTSTAGE_STASH_ADDR,
CONFIG_BOOTSTAGE_STASH_SIZE);
@@ -819,6 +820,7 @@ static int initf_bootstage(void)
return ret;
}
}
+#endif
bootstage_mark_name(BOOTSTAGE_ID_START_UBOOT_F, "board_init_f");
diff --git a/common/bootstage.c b/common/bootstage.c
index fb6befcbc4a8..5e6462cd0a34 100644
--- a/common/bootstage.c
+++ b/common/bootstage.c
@@ -501,6 +501,7 @@ int bootstage_unstash(const void *base, int size)
return 0;
}
+#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
int _bootstage_stash_default(void)
{
return bootstage_stash(map_sysmem(CONFIG_BOOTSTAGE_STASH_ADDR, 0),
@@ -514,6 +515,7 @@ int _bootstage_unstash_default(void)
return bootstage_unstash(stash, CONFIG_BOOTSTAGE_STASH_SIZE);
}
+#endif
int bootstage_get_size(void)
{
diff --git a/common/spl/spl.c b/common/spl/spl.c
index 7794ddccade1..47db4ead5050 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -472,11 +472,13 @@ static int spl_common_init(bool setup_malloc)
ret);
return ret;
}
+#if CONFIG_IS_ENABLED(BOOTSTAGE)
if (!u_boot_first_phase()) {
ret = bootstage_unstash_default();
if (ret)
log_debug("Failed to unstash bootstage: ret=%d\n", ret);
}
+#endif
bootstage_mark_name(get_bootstage_id(true),
spl_phase_name(spl_phase()));
#if CONFIG_IS_ENABLED(LOG)
diff --git a/include/bootstage.h b/include/bootstage.h
index f4e77b09d747..2d4987f31414 100644
--- a/include/bootstage.h
+++ b/include/bootstage.h
@@ -462,18 +462,20 @@ int _bootstage_unstash_default(void);
static inline int bootstage_stash_default(void)
{
- if (CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH))
- return _bootstage_stash_default();
-
+#if CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
+ return _bootstage_stash_default();
+#else
return 0;
+#endif
}
static inline int bootstage_unstash_default(void)
{
- if (CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH))
- return _bootstage_unstash_default();
-
+#if CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
+ return _bootstage_unstash_default();
+#else
return 0;
+#endif
}
/* Helper macro for adding a bootstage to a line of code */
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [v2] bootstash: Do not provide a default address for all
2024-07-11 21:27 ` [v2] " Tom Rini
@ 2024-07-13 15:13 ` Simon Glass
2024-07-13 16:50 ` Tom Rini
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2024-07-13 15:13 UTC (permalink / raw)
To: Tom Rini; +Cc: u-boot
Hi Tom,
On Thu, 11 Jul 2024 at 22:27, Tom Rini <trini@konsulko.com> wrote:
>
> A valid memory location to stash bootstage information at will be
> architecture dependent. Move the existing defaults to the main Kconfig
> file for this option and set 0x0 as the default only for sandbox.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Changes in v2:
> - Seeing that BOOTSTAGE_STASH_ADDR did not depend on BOOTSTAGE_STASH in
> turn lead to discovering that (minor) BOOTSTAGE_STASH_SIZE was also
> missing a depends on line and then that a number of places built code
> with BOOTSTAGE_STASH_ADDR=0x0 as a compiles-but-likely-fails option.
> Rework a number of spots to guard usage around BOOTSTAGE_STASH being
> enabled.
> ---
> arch/arm/mach-rockchip/tpl.c | 4 ++--
> arch/arm/mach-stm32mp/Kconfig.13x | 3 ---
> arch/arm/mach-stm32mp/Kconfig.15x | 3 ---
> arch/arm/mach-stm32mp/Kconfig.25x | 3 ---
> arch/x86/cpu/cpu.c | 2 ++
> boot/Kconfig | 6 +++++-
> cmd/bootstage.c | 8 +++++++-
> common/board_f.c | 2 ++
> common/bootstage.c | 2 ++
> common/spl/spl.c | 2 ++
> include/bootstage.h | 14 ++++++++------
> 11 files changed, 30 insertions(+), 19 deletions(-)
There is quite a bit going on in this patch.
>
> diff --git a/arch/arm/mach-rockchip/tpl.c b/arch/arm/mach-rockchip/tpl.c
> index 50f04f9474a0..a47cba5163ab 100644
> --- a/arch/arm/mach-rockchip/tpl.c
> +++ b/arch/arm/mach-rockchip/tpl.c
> @@ -92,10 +92,10 @@ void board_init_f(ulong dummy)
> int board_return_to_bootrom(struct spl_image_info *spl_image,
> struct spl_boot_device *bootdev)
> {
> -#ifdef CONFIG_BOOTSTAGE_STASH
> - int ret;
> + int __maybe_unused ret;
>
> bootstage_mark_name(BOOTSTAGE_ID_END_TPL, "end tpl");
> +#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
It should be enough to just call bootstage_stash_default() here,
unconditionally. It does nothing if not enabled.
> ret = bootstage_stash((void *)CONFIG_BOOTSTAGE_STASH_ADDR,
> CONFIG_BOOTSTAGE_STASH_SIZE);
> if (ret)
> diff --git a/arch/arm/mach-stm32mp/Kconfig.13x b/arch/arm/mach-stm32mp/Kconfig.13x
> index 4d74b35055b8..bc8b3f8cf77f 100644
> --- a/arch/arm/mach-stm32mp/Kconfig.13x
> +++ b/arch/arm/mach-stm32mp/Kconfig.13x
> @@ -28,9 +28,6 @@ config PRE_CON_BUF_ADDR
> config PRE_CON_BUF_SZ
> default 4096
>
> -config BOOTSTAGE_STASH_ADDR
> - default 0xC3000000
> -
> if BOOTCOUNT_GENERIC
> config SYS_BOOTCOUNT_SINGLEWORD
> default y
> diff --git a/arch/arm/mach-stm32mp/Kconfig.15x b/arch/arm/mach-stm32mp/Kconfig.15x
> index d99aa9fd694a..42da36a73e85 100644
> --- a/arch/arm/mach-stm32mp/Kconfig.15x
> +++ b/arch/arm/mach-stm32mp/Kconfig.15x
> @@ -86,9 +86,6 @@ config PRE_CON_BUF_ADDR
> config PRE_CON_BUF_SZ
> default 4096
>
> -config BOOTSTAGE_STASH_ADDR
> - default 0xC3000000
> -
> if BOOTCOUNT_GENERIC
> config SYS_BOOTCOUNT_SINGLEWORD
> default y
> diff --git a/arch/arm/mach-stm32mp/Kconfig.25x b/arch/arm/mach-stm32mp/Kconfig.25x
> index 2c0f691f8b54..7d2d8171845b 100644
> --- a/arch/arm/mach-stm32mp/Kconfig.25x
> +++ b/arch/arm/mach-stm32mp/Kconfig.25x
> @@ -24,9 +24,6 @@ config PRE_CON_BUF_ADDR
> config PRE_CON_BUF_SZ
> default 4096
>
> -config BOOTSTAGE_STASH_ADDR
> - default 0x87000000
> -
> if DEBUG_UART
>
> config DEBUG_UART_BOARD_INIT
> diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
> index c8433360f28e..7ce6443cc77e 100644
> --- a/arch/x86/cpu/cpu.c
> +++ b/arch/x86/cpu/cpu.c
> @@ -75,8 +75,10 @@ int __weak x86_cleanup_before_linux(void)
> ret = mp_park_aps();
> if (ret)
> return log_msg_ret("park", ret);
> +#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> bootstage_stash((void *)CONFIG_BOOTSTAGE_STASH_ADDR,
> CONFIG_BOOTSTAGE_STASH_SIZE);
> +#endif
Same here
>
> return 0;
> }
> diff --git a/boot/Kconfig b/boot/Kconfig
> index ffcae840a506..ba8bddd14219 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -1002,13 +1002,17 @@ config BOOTSTAGE_STASH
>
> config BOOTSTAGE_STASH_ADDR
> hex "Address to stash boot timing information"
> - default 0x0
> + depends on BOOTSTAGE_STASH
> + default 0xC3000000 if STM32MP13X || STM32MP15X
> + default 0x87000000 if STM32MP25X
> + default 0x0 if SANDBOX
> help
> Provide an address which will not be overwritten by the OS when it
> starts, so that it can read this information when ready.
>
> config BOOTSTAGE_STASH_SIZE
> hex "Size of boot timing stash region"
> + depends on BOOTSTAGE_STASH
> default 0x1000
> help
> This should be large enough to hold the bootstage stash. A value of
> diff --git a/cmd/bootstage.c b/cmd/bootstage.c
> index 5246924f39a4..4ae09a6fd03d 100644
> --- a/cmd/bootstage.c
> +++ b/cmd/bootstage.c
> @@ -15,6 +15,7 @@ static int do_bootstage_report(struct cmd_tbl *cmdtp, int flag, int argc,
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> static int get_base_size(int argc, char *const argv[], ulong *basep,
> ulong *sizep)
> {
> @@ -58,11 +59,14 @@ static int do_bootstage_stash(struct cmd_tbl *cmdtp, int flag, int argc,
>
> return 0;
> }
> +#endif
>
> static struct cmd_tbl cmd_bootstage_sub[] = {
> U_BOOT_CMD_MKENT(report, 2, 1, do_bootstage_report, "", ""),
> +#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> U_BOOT_CMD_MKENT(stash, 4, 0, do_bootstage_stash, "", ""),
> U_BOOT_CMD_MKENT(unstash, 4, 0, do_bootstage_stash, "", ""),
> +#endif
> };
>
> /*
> @@ -91,6 +95,8 @@ U_BOOT_CMD(bootstage, 4, 1, do_boostage,
> "Boot stage command",
> " - check boot progress and timing\n"
> "report - Print a report\n"
> +#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> "stash [<start> [<size>]] - Stash data into memory\n"
> - "unstash [<start> [<size>]] - Unstash data from memory"
> + "unstash [<start> [<size>]] - Unstash data from memory\n"
> +#endif
> );
> diff --git a/common/board_f.c b/common/board_f.c
> index 212ffb3090b2..37ae1641f1f0 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -809,6 +809,7 @@ static int initf_bootstage(void)
> ret = bootstage_init(!from_spl);
> if (ret)
> return ret;
> +#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> if (from_spl) {
> const void *stash = map_sysmem(CONFIG_BOOTSTAGE_STASH_ADDR,
> CONFIG_BOOTSTAGE_STASH_SIZE);
> @@ -819,6 +820,7 @@ static int initf_bootstage(void)
> return ret;
> }
> }
> +#endif
>
> bootstage_mark_name(BOOTSTAGE_ID_START_UBOOT_F, "board_init_f");
>
> diff --git a/common/bootstage.c b/common/bootstage.c
> index fb6befcbc4a8..5e6462cd0a34 100644
> --- a/common/bootstage.c
> +++ b/common/bootstage.c
> @@ -501,6 +501,7 @@ int bootstage_unstash(const void *base, int size)
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> int _bootstage_stash_default(void)
> {
> return bootstage_stash(map_sysmem(CONFIG_BOOTSTAGE_STASH_ADDR, 0),
> @@ -514,6 +515,7 @@ int _bootstage_unstash_default(void)
>
> return bootstage_unstash(stash, CONFIG_BOOTSTAGE_STASH_SIZE);
> }
> +#endif
>
> int bootstage_get_size(void)
> {
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 7794ddccade1..47db4ead5050 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -472,11 +472,13 @@ static int spl_common_init(bool setup_malloc)
> ret);
> return ret;
> }
> +#if CONFIG_IS_ENABLED(BOOTSTAGE)
I don't think this #ifdef is needed.
> if (!u_boot_first_phase()) {
> ret = bootstage_unstash_default();
> if (ret)
> log_debug("Failed to unstash bootstage: ret=%d\n", ret);
> }
> +#endif
> bootstage_mark_name(get_bootstage_id(true),
> spl_phase_name(spl_phase()));
> #if CONFIG_IS_ENABLED(LOG)
> diff --git a/include/bootstage.h b/include/bootstage.h
> index f4e77b09d747..2d4987f31414 100644
> --- a/include/bootstage.h
> +++ b/include/bootstage.h
> @@ -462,18 +462,20 @@ int _bootstage_unstash_default(void);
>
> static inline int bootstage_stash_default(void)
> {
> - if (CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH))
> - return _bootstage_stash_default();
> -
> +#if CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> + return _bootstage_stash_default();
> +#else
> return 0;
> +#endif
I believe you can leave this as it is.
> }
>
> static inline int bootstage_unstash_default(void)
> {
> - if (CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH))
> - return _bootstage_unstash_default();
> -
> +#if CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> + return _bootstage_unstash_default();
> +#else
> return 0;
> +#endif
and this
> }
>
> /* Helper macro for adding a bootstage to a line of code */
> --
> 2.34.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v2] bootstash: Do not provide a default address for all
2024-07-13 15:13 ` Simon Glass
@ 2024-07-13 16:50 ` Tom Rini
2024-07-15 11:39 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Tom Rini @ 2024-07-13 16:50 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot
[-- Attachment #1: Type: text/plain, Size: 4544 bytes --]
On Sat, Jul 13, 2024 at 04:13:50PM +0100, Simon Glass wrote:
> Hi Tom,
>
> On Thu, 11 Jul 2024 at 22:27, Tom Rini <trini@konsulko.com> wrote:
> >
> > A valid memory location to stash bootstage information at will be
> > architecture dependent. Move the existing defaults to the main Kconfig
> > file for this option and set 0x0 as the default only for sandbox.
> >
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > ---
> > Changes in v2:
> > - Seeing that BOOTSTAGE_STASH_ADDR did not depend on BOOTSTAGE_STASH in
> > turn lead to discovering that (minor) BOOTSTAGE_STASH_SIZE was also
> > missing a depends on line and then that a number of places built code
> > with BOOTSTAGE_STASH_ADDR=0x0 as a compiles-but-likely-fails option.
> > Rework a number of spots to guard usage around BOOTSTAGE_STASH being
> > enabled.
> > ---
> > arch/arm/mach-rockchip/tpl.c | 4 ++--
> > arch/arm/mach-stm32mp/Kconfig.13x | 3 ---
> > arch/arm/mach-stm32mp/Kconfig.15x | 3 ---
> > arch/arm/mach-stm32mp/Kconfig.25x | 3 ---
> > arch/x86/cpu/cpu.c | 2 ++
> > boot/Kconfig | 6 +++++-
> > cmd/bootstage.c | 8 +++++++-
> > common/board_f.c | 2 ++
> > common/bootstage.c | 2 ++
> > common/spl/spl.c | 2 ++
> > include/bootstage.h | 14 ++++++++------
> > 11 files changed, 30 insertions(+), 19 deletions(-)
>
> There is quite a bit going on in this patch.
Yes, there was unfortunately some underlying bugs.
> > diff --git a/arch/arm/mach-rockchip/tpl.c b/arch/arm/mach-rockchip/tpl.c
> > index 50f04f9474a0..a47cba5163ab 100644
> > --- a/arch/arm/mach-rockchip/tpl.c
> > +++ b/arch/arm/mach-rockchip/tpl.c
> > @@ -92,10 +92,10 @@ void board_init_f(ulong dummy)
> > int board_return_to_bootrom(struct spl_image_info *spl_image,
> > struct spl_boot_device *bootdev)
> > {
> > -#ifdef CONFIG_BOOTSTAGE_STASH
> > - int ret;
> > + int __maybe_unused ret;
> >
> > bootstage_mark_name(BOOTSTAGE_ID_END_TPL, "end tpl");
> > +#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
>
> It should be enough to just call bootstage_stash_default() here,
> unconditionally. It does nothing if not enabled.
Nope, we don't have ADDR/SIZE defined. The current defaulting them to
0 leads to the case today where platforms which don't enable stash have
~700 bytes of stash related code being kept.
[snip]
> > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > index 7794ddccade1..47db4ead5050 100644
> > --- a/common/spl/spl.c
> > +++ b/common/spl/spl.c
> > @@ -472,11 +472,13 @@ static int spl_common_init(bool setup_malloc)
> > ret);
> > return ret;
> > }
> > +#if CONFIG_IS_ENABLED(BOOTSTAGE)
>
> I don't think this #ifdef is needed.
As part of being able to discard the stash functionality, it is.
> > if (!u_boot_first_phase()) {
> > ret = bootstage_unstash_default();
> > if (ret)
> > log_debug("Failed to unstash bootstage: ret=%d\n", ret);
> > }
> > +#endif
> > bootstage_mark_name(get_bootstage_id(true),
> > spl_phase_name(spl_phase()));
> > #if CONFIG_IS_ENABLED(LOG)
> > diff --git a/include/bootstage.h b/include/bootstage.h
> > index f4e77b09d747..2d4987f31414 100644
> > --- a/include/bootstage.h
> > +++ b/include/bootstage.h
> > @@ -462,18 +462,20 @@ int _bootstage_unstash_default(void);
> >
> > static inline int bootstage_stash_default(void)
> > {
> > - if (CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH))
> > - return _bootstage_stash_default();
> > -
> > +#if CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> > + return _bootstage_stash_default();
> > +#else
> > return 0;
> > +#endif
>
> I believe you can leave this as it is.
>
> > }
> >
> > static inline int bootstage_unstash_default(void)
> > {
> > - if (CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH))
> > - return _bootstage_unstash_default();
> > -
> > +#if CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> > + return _bootstage_unstash_default();
> > +#else
> > return 0;
> > +#endif
>
> and this
Nope, this too is required to discard "stash" when not enabled.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v2] bootstash: Do not provide a default address for all
2024-07-13 16:50 ` Tom Rini
@ 2024-07-15 11:39 ` Simon Glass
0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2024-07-15 11:39 UTC (permalink / raw)
To: Tom Rini; +Cc: u-boot
Hi Tom,
On Sat, 13 Jul 2024 at 17:50, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Jul 13, 2024 at 04:13:50PM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 11 Jul 2024 at 22:27, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > A valid memory location to stash bootstage information at will be
> > > architecture dependent. Move the existing defaults to the main Kconfig
> > > file for this option and set 0x0 as the default only for sandbox.
> > >
> > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > ---
> > > Changes in v2:
> > > - Seeing that BOOTSTAGE_STASH_ADDR did not depend on BOOTSTAGE_STASH in
> > > turn lead to discovering that (minor) BOOTSTAGE_STASH_SIZE was also
> > > missing a depends on line and then that a number of places built code
> > > with BOOTSTAGE_STASH_ADDR=0x0 as a compiles-but-likely-fails option.
> > > Rework a number of spots to guard usage around BOOTSTAGE_STASH being
> > > enabled.
> > > ---
> > > arch/arm/mach-rockchip/tpl.c | 4 ++--
> > > arch/arm/mach-stm32mp/Kconfig.13x | 3 ---
> > > arch/arm/mach-stm32mp/Kconfig.15x | 3 ---
> > > arch/arm/mach-stm32mp/Kconfig.25x | 3 ---
> > > arch/x86/cpu/cpu.c | 2 ++
> > > boot/Kconfig | 6 +++++-
> > > cmd/bootstage.c | 8 +++++++-
> > > common/board_f.c | 2 ++
> > > common/bootstage.c | 2 ++
> > > common/spl/spl.c | 2 ++
> > > include/bootstage.h | 14 ++++++++------
> > > 11 files changed, 30 insertions(+), 19 deletions(-)
> >
> > There is quite a bit going on in this patch.
>
> Yes, there was unfortunately some underlying bugs.
>
> > > diff --git a/arch/arm/mach-rockchip/tpl.c b/arch/arm/mach-rockchip/tpl.c
> > > index 50f04f9474a0..a47cba5163ab 100644
> > > --- a/arch/arm/mach-rockchip/tpl.c
> > > +++ b/arch/arm/mach-rockchip/tpl.c
> > > @@ -92,10 +92,10 @@ void board_init_f(ulong dummy)
> > > int board_return_to_bootrom(struct spl_image_info *spl_image,
> > > struct spl_boot_device *bootdev)
> > > {
> > > -#ifdef CONFIG_BOOTSTAGE_STASH
> > > - int ret;
> > > + int __maybe_unused ret;
> > >
> > > bootstage_mark_name(BOOTSTAGE_ID_END_TPL, "end tpl");
> > > +#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> >
> > It should be enough to just call bootstage_stash_default() here,
> > unconditionally. It does nothing if not enabled.
>
> Nope, we don't have ADDR/SIZE defined. The current defaulting them to
> 0 leads to the case today where platforms which don't enable stash have
> ~700 bytes of stash related code being kept.
Yes I see.
>
> [snip]
> > > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > > index 7794ddccade1..47db4ead5050 100644
> > > --- a/common/spl/spl.c
> > > +++ b/common/spl/spl.c
> > > @@ -472,11 +472,13 @@ static int spl_common_init(bool setup_malloc)
> > > ret);
> > > return ret;
> > > }
> > > +#if CONFIG_IS_ENABLED(BOOTSTAGE)
> >
> > I don't think this #ifdef is needed.
>
> As part of being able to discard the stash functionality, it is.
>
> > > if (!u_boot_first_phase()) {
> > > ret = bootstage_unstash_default();
> > > if (ret)
> > > log_debug("Failed to unstash bootstage: ret=%d\n", ret);
> > > }
> > > +#endif
> > > bootstage_mark_name(get_bootstage_id(true),
> > > spl_phase_name(spl_phase()));
> > > #if CONFIG_IS_ENABLED(LOG)
> > > diff --git a/include/bootstage.h b/include/bootstage.h
> > > index f4e77b09d747..2d4987f31414 100644
> > > --- a/include/bootstage.h
> > > +++ b/include/bootstage.h
> > > @@ -462,18 +462,20 @@ int _bootstage_unstash_default(void);
> > >
> > > static inline int bootstage_stash_default(void)
> > > {
> > > - if (CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH))
> > > - return _bootstage_stash_default();
> > > -
> > > +#if CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> > > + return _bootstage_stash_default();
> > > +#else
> > > return 0;
> > > +#endif
> >
> > I believe you can leave this as it is.
> >
> > > }
> > >
> > > static inline int bootstage_unstash_default(void)
> > > {
> > > - if (CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH))
> > > - return _bootstage_unstash_default();
> > > -
> > > +#if CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
> > > + return _bootstage_unstash_default();
> > > +#else
> > > return 0;
> > > +#endif
> >
> > and this
>
> Nope, this too is required to discard "stash" when not enabled.
Yes, got it, but my point was that the header file already has this
logic. I'll send a new version to show what I am talking about...
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-15 11:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-03 21:23 [PATCH] bootstash: Do not provide a default address for all Tom Rini
2024-07-05 8:36 ` Simon Glass
2024-07-11 21:27 ` [v2] " Tom Rini
2024-07-13 15:13 ` Simon Glass
2024-07-13 16:50 ` Tom Rini
2024-07-15 11:39 ` Simon Glass
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox