Re: [PATCH 1/2] spl: riscv: Disable SPL_BINMAN_UBOOT_SYMBOLS by default

[Yao Zi <ziyao@disroot.org>]

On Mon, Apr 07, 2025 at 01:22:37PM +0200, Jonas Karlman wrote:
> Hi,
> 
> On 2025-04-07 05:37, Yao Zi wrote:
> > The default binman configuration of RISC-V wraps proper U-Boot into a
> > FIT image instead of shipping a plain image, thus there's no
> > "u_boot_any" entry by default. Let's disable the option to prevent
> > binman from looking for a plain proper U-Boot image, failing the build
> > with message like
> > 
> >   Section '/binman/spl-img': Symbol '_binman_u_boot_any_prop_size'
> >      in entry '/binman/spl-img/mkimage/u-boot-spl/u-boot-spl-nodtb':
> >        Entry 'u-boot-any' not found in list (u-boot-spl-nodtb,
> >        u-boot-spl-dtb,u-boot-spl,mkimage,spl-img)
> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  common/spl/Kconfig | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > index 7d6780936d1..356ddab38de 100644
> > --- a/common/spl/Kconfig
> > +++ b/common/spl/Kconfig
> > @@ -214,6 +214,8 @@ config SPL_BINMAN_UBOOT_SYMBOLS
> >  	bool "Declare binman symbols for U-Boot phases in SPL"
> >  	depends on SPL_BINMAN_SYMBOLS
> >  	default n if ARCH_IMX8M || ARCH_IMX8ULP || ARCH_IMX9
> > +	# A FIT image is created with default binman configuration of RISC-V
> 
> Use of a FIT does not really matter, Rockchip use a FIT for U-Boot
> proper and symbols can be located.

You're right. binman is able to match a U-Boot blob in FIT image as long
as its entry name is correct. We should replace blob-ext with
u-boot-nodtb here.

> 
> > +	default n if RISCV
> >  	default y
> >  	help
> >  	  This enables use of symbols in SPL which refer to U-Boot phases,
> 
> Maybe something like this is what you instead want to use:
> 
> diff --git a/arch/riscv/dts/binman.dtsi b/arch/riscv/dts/binman.dtsi
> index ceb916b74a73..e1a5ad573bbe 100644
> --- a/arch/riscv/dts/binman.dtsi
> +++ b/arch/riscv/dts/binman.dtsi
> @@ -30,20 +30,19 @@
>  				uboot {
>  					description = "U-Boot";
>  					type = "standalone";
> -					os = "U-Boot";
> +					os = "u-boot";
>  					arch = "riscv";
>  					compression = "none";
>  					load = /bits/ 64 <CONFIG_TEXT_BASE>;
>  
> -					uboot_blob: blob-ext {
> -						filename = "u-boot-nodtb.bin";
> +					uboot_blob: u-boot-nodtb {

I think the change to uboot_blob node is enough to fix the binman
issue, right?

The value of property "os" is probably wrong since spl_fit.c compares it
to "u-boot" case-sensitively to look for a U-Boot image. But this should
go in another patch IMHO.

>  					};
>  				};
>  #else
>  				linux {
>  					description = "Linux";
>  					type = "standalone";
> -					os = "Linux";
> +					os = "linux";
>  					arch = "riscv";
>  					compression = "none";
>  					load = /bits/ 64 <CONFIG_TEXT_BASE>;
> 
> diff --git a/arch/riscv/dts/starfive-visionfive2-binman.dtsi b/arch/riscv/dts/starfive-visionfive2-binman.dtsi
> index 05787bdb92db..6e083bf0537a 100644
> --- a/arch/riscv/dts/starfive-visionfive2-binman.dtsi
> +++ b/arch/riscv/dts/starfive-visionfive2-binman.dtsi
> @@ -20,6 +20,7 @@
>  			args = "-T sfspl";
>  
>  			u-boot-spl {
> +				no-write-symbols;
>  			};
>  		};
>  	};
> 

At the very first sight I wrongly thought "no-write-symbols" would
prevent binman to function and should be avoided. But visionfive2's case
seems actually special as its spl and proper U-Boot are split into
different images, where binman cannot help much.

I've applied the fix of uboot_blob node to my WIP branch of TH1520 SPL
support and verified it does fix the binman error. Thanks for your hint
and review! I'll include the fix and drop this patch in v2.

> Regards,
> Jonas

Thanks,
Yao Zi