[PATCH v2] spl: bootstage: move bootstage_stash before jumping to image

[PATCH v2] spl: bootstage: move bootstage_stash before jumping to image

[Chanho Park]

Regarding IH_OS_OPENSBI, IH_OS_LINUX and IH_OS_TEE, there is no chance
to stash bootstage record because they do not return to SPL after
jumping to the image.
Hence, this patch separates the final stage bootstage code into
spl_bootstage_finish and call the function before jumping to the image.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
Changes from v1
- Separate the final stage bootstage code into spl_bootstage_finish.
- As Simon suggests, call the function before jumping to the image.

 common/spl/spl.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index 0062f3f45d9a..eaf2f076ddd1 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -738,6 +738,19 @@ void board_init_f(ulong dummy)
 }
 #endif
 
+static void spl_bootstage_finish(void)
+{
+	int ret;
+
+	bootstage_mark_name(get_bootstage_id(false), "end phase");
+#ifdef CONFIG_BOOTSTAGE_STASH
+	ret = bootstage_stash((void *)CONFIG_BOOTSTAGE_STASH_ADDR,
+			      CONFIG_BOOTSTAGE_STASH_SIZE);
+	if (ret)
+		debug("Failed to stash bootstage: err=%d\n", ret);
+#endif
+}
+
 void board_init_r(gd_t *dummy1, ulong dummy2)
 {
 	u32 spl_boot_list[] = {
@@ -865,12 +878,14 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 	case IH_OS_TEE:
 		debug("Jumping to U-Boot via OP-TEE\n");
 		spl_board_prepare_for_optee(spl_image.fdt_addr);
+		spl_bootstage_finish();
 		jump_to_image_optee(&spl_image);
 		break;
 #endif
 #if CONFIG_IS_ENABLED(OPENSBI)
 	case IH_OS_OPENSBI:
 		debug("Jumping to U-Boot via RISC-V OpenSBI\n");
+		spl_bootstage_finish();
 		spl_invoke_opensbi(&spl_image);
 		break;
 #endif
@@ -881,6 +896,7 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 		spl_fixup_fdt((void *)CONFIG_SYS_SPL_ARGS_ADDR);
 #endif
 		spl_board_prepare_for_linux();
+		spl_bootstage_finish();
 		jump_to_image_linux(&spl_image);
 #endif
 	default:
@@ -890,13 +906,7 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 	debug("SPL malloc() used 0x%lx bytes (%ld KB)\n", gd->malloc_ptr,
 	      gd->malloc_ptr / 1024);
 #endif
-	bootstage_mark_name(get_bootstage_id(false), "end phase");
-#ifdef CONFIG_BOOTSTAGE_STASH
-	ret = bootstage_stash((void *)CONFIG_BOOTSTAGE_STASH_ADDR,
-			      CONFIG_BOOTSTAGE_STASH_SIZE);
-	if (ret)
-		debug("Failed to stash bootstage: err=%d\n", ret);
-#endif
+	spl_bootstage_finish();
 
 	if (IS_ENABLED(CONFIG_SPL_VIDEO_REMOVE)) {
 		struct udevice *dev;
-- 
2.39.2

Re: [PATCH v2] spl: bootstage: move bootstage_stash before jumping to image

[Simon Glass]

Hi Chanho,

On Mon, 28 Aug 2023 at 22:28, Chanho Park <chanho61.park@samsung.com> wrote:
>
> Regarding IH_OS_OPENSBI, IH_OS_LINUX and IH_OS_TEE, there is no chance
> to stash bootstage record because they do not return to SPL after
> jumping to the image.
> Hence, this patch separates the final stage bootstage code into
> spl_bootstage_finish and call the function before jumping to the image.
>
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
> Changes from v1
> - Separate the final stage bootstage code into spl_bootstage_finish.
> - As Simon suggests, call the function before jumping to the image.

I think you misunderstood me here. I mean, you cannot jump off
somewhere in your board code. You must change it so it returns
correctly, and the jump happens from spl.c's board_init_r() function.
The way it works is you set up the spl_image structure, then it SPL
jumps to it at the end of the functions.

Regards,
Simon

RE: [PATCH v2] spl: bootstage: move bootstage_stash before jumping to image

[Chanho Park]

Hi Simon,

> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: Wednesday, August 30, 2023 1:38 AM
> To: Chanho Park <chanho61.park@samsung.com>
> Cc: Nikhil M Jain <n-jain1@ti.com>; Marek Vasut <marex@denx.de>; u-
> boot@lists.denx.de
> Subject: Re: [PATCH v2] spl: bootstage: move bootstage_stash before
> jumping to image
> 
> Hi Chanho,
> 
> On Mon, 28 Aug 2023 at 22:28, Chanho Park <chanho61.park@samsung.com>
> wrote:
> >
> > Regarding IH_OS_OPENSBI, IH_OS_LINUX and IH_OS_TEE, there is no chance
> > to stash bootstage record because they do not return to SPL after
> > jumping to the image.
> > Hence, this patch separates the final stage bootstage code into
> > spl_bootstage_finish and call the function before jumping to the image.
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> > ---
> > Changes from v1
> > - Separate the final stage bootstage code into spl_bootstage_finish.
> > - As Simon suggests, call the function before jumping to the image.
> 
> I think you misunderstood me here. I mean, you cannot jump off somewhere
> in your board code. You must change it so it returns correctly, and the
> jump happens from spl.c's board_init_r() function.
> The way it works is you set up the spl_image structure, then it SPL jumps
> to it at the end of the functions.

I feel like I'm still not clear on what you mean. Sorry.

switch (spl_image.os) {
	case IH_OS_U_BOOT:
	case IH_OS_ARM_TRUSTED_FIRMWARE:
	case IH_OS_TEE:
	case IH_OS_OPENSBI:
	case IH_OS_LINUX:
}

Regarding ATF/TEE/OPENSBI and Linux, they need different number of arguments and formats to jump to the image, respectively.
I think that's why they can't go to the final stage and can't use jump_to_image_no_args.

Do you want to move jump codes at the end of the board_init_r function?
The easiest way is that we just move the whole switch statements to the final stage of the function.
Otherwise, the arguments can be prepared from switch statement and make jump_to_image function to support variable length of arguments.
(Or we can put switch statement there to support various jump of the image)

Can you elaborate a bit more?

Best Regards,
Chanho Park

Re: [PATCH v2] spl: bootstage: move bootstage_stash before jumping to image

[Simon Glass]

Hi Chanho,

On Tue, 29 Aug 2023 at 23:10, Chanho Park <chanho61.park@samsung.com> wrote:
>
> Hi Simon,
>
> > -----Original Message-----
> > From: Simon Glass <sjg@chromium.org>
> > Sent: Wednesday, August 30, 2023 1:38 AM
> > To: Chanho Park <chanho61.park@samsung.com>
> > Cc: Nikhil M Jain <n-jain1@ti.com>; Marek Vasut <marex@denx.de>; u-
> > boot@lists.denx.de
> > Subject: Re: [PATCH v2] spl: bootstage: move bootstage_stash before
> > jumping to image
> >
> > Hi Chanho,
> >
> > On Mon, 28 Aug 2023 at 22:28, Chanho Park <chanho61.park@samsung.com>
> > wrote:
> > >
> > > Regarding IH_OS_OPENSBI, IH_OS_LINUX and IH_OS_TEE, there is no chance
> > > to stash bootstage record because they do not return to SPL after
> > > jumping to the image.
> > > Hence, this patch separates the final stage bootstage code into
> > > spl_bootstage_finish and call the function before jumping to the image.
> > >
> > > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> > > ---
> > > Changes from v1
> > > - Separate the final stage bootstage code into spl_bootstage_finish.
> > > - As Simon suggests, call the function before jumping to the image.
> >
> > I think you misunderstood me here. I mean, you cannot jump off somewhere
> > in your board code. You must change it so it returns correctly, and the
> > jump happens from spl.c's board_init_r() function.
> > The way it works is you set up the spl_image structure, then it SPL jumps
> > to it at the end of the functions.
>
> I feel like I'm still not clear on what you mean. Sorry.
>
> switch (spl_image.os) {
>         case IH_OS_U_BOOT:
>         case IH_OS_ARM_TRUSTED_FIRMWARE:
>         case IH_OS_TEE:
>         case IH_OS_OPENSBI:
>         case IH_OS_LINUX:
> }
>
> Regarding ATF/TEE/OPENSBI and Linux, they need different number of arguments and formats to jump to the image, respectively.
> I think that's why they can't go to the final stage and can't use jump_to_image_no_args.

OK, so let's move that code into spl.c and have it do the right thing...

>
> Do you want to move jump codes at the end of the board_init_r function?
> The easiest way is that we just move the whole switch statements to the final stage of the function.
> Otherwise, the arguments can be prepared from switch statement and make jump_to_image function to support variable length of arguments.
> (Or we can put switch statement there to support various jump of the image)
>
> Can you elaborate a bit more?

Basically SPL should have one place where it jumps to the next phase.
If you do it willy nilly, then generic features like bloblist and
bootstage cannot work, as you have found.

The way SPL board_init_r() is set up is something like this:

- do some init
- work through the boot devices until one is found that can boot
- prepare to jump (thjis is where the bloblist and bootstage are finalised)
- jump

So we should keep this approach, even if it means putting a switch at
the end like:

switch (how_to_jump) {
case way1: ...
case way2: ...
}


Regards,
Simon