Re: [PATCH 1/4] riscv: spl: Introduce SPL_OPENSBI_OS_BOOT

[Sean Anderson <seanga2@gmail.com>]

On 12/7/22 01:23, Rick Chen wrote:
> In RISC-V, it only provide normal mode booting currently.
> To speed up the booting process, here provide SPL_OPENSBI_OS_BOOT
> to achieve this feature which will be call Fast-Boot mode. By

Can you name this something different. We already have something called
fastboot in-tree (the Android-derived protocol) and there's a Microsoft
technology called fastboot (some kind of hibernation). "OS Boot" isn't
very specific either, since we (almost always) boot an OS. Maybe "Eagle
mode" by analogy to Falcon mode, which lets SPL directly boot an OS.

(Is this substantially different from falcon mode anyway?)

> enabling SPL_OPENSBI_OS_BOOT, it will generate linux.itb instead
> of default u-boot.itb after compiling. It initializes memory with
> the U-Boot SPL at the first stage, just like what a regular booting
> process (i.e. Normal Boot) does in the beginning. Instead of jumping
> to the U-Boot proper from OpenSBI before booting Linux Kernel, the
> Fast Boot process jumps directly to Linux Kernel to gain shorter
> booting time.
> 
> Signed-off-by: Rick Chen <rick@andestech.com>
> ---
>   common/spl/Kconfig       | 14 ++++++++++++++
>   common/spl/spl_fit.c     |  3 ++-
>   common/spl/spl_opensbi.c | 25 ++++++++++++-------------
>   3 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index 05181bdba3..8805aba1b7 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -1509,6 +1509,20 @@ config SPL_OPENSBI_SCRATCH_OPTIONS
>   	  Options passed to fw_dynamic, for example SBI_SCRATCH_NO_BOOT_PRINTS or
>   	  SBI_SCRATCH_DEBUG_PRINTS.
>   
> +config SPL_OPENSBI_OS_BOOT

Please use the same name for the config as for the description.

> +	bool "openSBI Fast Boot"
> +	depends on SPL_OPENSBI
> +	help
> +	  Enable this openSBI can jump to Linux Kernel directly.

Can you put some of the explanation from the commit message here?

> +
> +config SPL_OPENSBI_FIT_NAME
> +	string "SPL openSBI fit image name"
> +	depends on SPL_OPENSBI
> +	default "linux.itb" if SPL_OPENSBI_OS_BOOT
> +	default "u-boot.itb"
> +	help
> +	  This will help to generate different fit name accordingly.

Why not SPL_FS_LOAD_PAYLOAD_NAME?

It looks like the code changes below do not use these configs. Can you
move them to the next patch so it is clearer that they are for binman?

--Sean

>   config SPL_TARGET
>   	string "Addtional build targets for 'make'"
>   	default "spl/u-boot-spl.srec" if RCAR_GEN2
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index c1ed31e367..c5b1dfb3ba 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -363,7 +363,8 @@ static bool os_takes_devicetree(uint8_t os)
>   	case IH_OS_U_BOOT:
>   		return true;
>   	case IH_OS_LINUX:
> -		return IS_ENABLED(CONFIG_SPL_OS_BOOT);
> +		return IS_ENABLED(CONFIG_SPL_OS_BOOT) ||
> +			IS_ENABLED(CONFIG_SPL_OPENSBI_OS_BOOT);
>   	default:
>   		return false;
>   	}
> diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
> index b0f40076c3..83869c6b18 100644
> --- a/common/spl/spl_opensbi.c
> +++ b/common/spl/spl_opensbi.c
> @@ -20,7 +20,7 @@ DECLARE_GLOBAL_DATA_PTR;
>   
>   struct fw_dynamic_info opensbi_info;
>   
> -static int spl_opensbi_find_uboot_node(void *blob, int *uboot_node)
> +static int spl_opensbi_find_os_node(void *blob, int *os_node)
>   {
>   	int fit_images_node, node;
>   	const char *fit_os;
> @@ -34,10 +34,9 @@ static int spl_opensbi_find_uboot_node(void *blob, int *uboot_node)
>   		if (!fit_os)
>   			continue;
>   
> -		if (genimg_get_os_id(fit_os) == IH_OS_U_BOOT) {
> -			*uboot_node = node;
> -			return 0;
> -		}
> +		*os_node = node;
> +
> +		return 0;
>   	}
>   
>   	return -ENODEV;
> @@ -45,8 +44,8 @@ static int spl_opensbi_find_uboot_node(void *blob, int *uboot_node)
>   
>   void spl_invoke_opensbi(struct spl_image_info *spl_image)
>   {
> -	int ret, uboot_node;
> -	ulong uboot_entry;
> +	int ret, os_node;
> +	ulong os_entry;
>   	void (*opensbi_entry)(ulong hartid, ulong dtb, ulong info);
>   
>   	if (!spl_image->fdt_addr) {
> @@ -54,22 +53,22 @@ void spl_invoke_opensbi(struct spl_image_info *spl_image)
>   		hang();
>   	}
>   
> -	/* Find U-Boot image in /fit-images */
> -	ret = spl_opensbi_find_uboot_node(spl_image->fdt_addr, &uboot_node);
> +	/* Find U-Boot or Linux image in /fit-images */
> +	ret = spl_opensbi_find_os_node(spl_image->fdt_addr, &os_node);
>   	if (ret) {
>   		pr_err("Can't find U-Boot node, %d\n", ret);
>   		hang();
>   	}
>   
> -	/* Get U-Boot entry point */
> -	ret = fit_image_get_entry(spl_image->fdt_addr, uboot_node, &uboot_entry);
> +	/* Get os entry point */
> +	ret = fit_image_get_entry(spl_image->fdt_addr, os_node, &os_entry);
>   	if (ret)
> -		ret = fit_image_get_load(spl_image->fdt_addr, uboot_node, &uboot_entry);
> +		ret = fit_image_get_load(spl_image->fdt_addr, os_node, &os_entry);
>   
>   	/* Prepare opensbi_info object */
>   	opensbi_info.magic = FW_DYNAMIC_INFO_MAGIC_VALUE;
>   	opensbi_info.version = FW_DYNAMIC_INFO_VERSION;
> -	opensbi_info.next_addr = uboot_entry;
> +	opensbi_info.next_addr = os_entry;
>   	opensbi_info.next_mode = FW_DYNAMIC_INFO_NEXT_MODE_S;
>   	opensbi_info.options = CONFIG_SPL_OPENSBI_SCRATCH_OPTIONS;
>   	opensbi_info.boot_hart = gd->arch.boot_hart;