[PATCH 0/2] Fix binman_sym functionality on RISC-V port

[PATCH 0/2] Fix binman_sym functionality on RISC-V port

[Yao Zi]

It's found that BINMAN_SYMBOLS_OK always evaluates to false on RISC-V,
because our linkerscripts don't define symbol __image_copy_start, on
which binman depends for determining the base address of an entry.
Binman simply bails out in case of missing the symbol.

This series first defaults SPL_BINMAN_UBOOT_SYMBOLS to N on RISC-V to
prevent binman from looking for a plain proper U-Boot image, which
isn't desired since binman configuration on RISC-V wraps proper U-Boot
in a FIT image. Then we define __image_copy_start and the paired
__image_copy_end to really fix binman_sym's functionality.

I've tested building with
  - ae350_rv64_defconfig
  - ae350_rv64_spl_defconfig
  - bananapi-f3_defconfig
  - k230_canmv_defconfig
  - microchip_mpfs_icicle_defconfig

and booting successfully on
  - milkv_duo_defconfig
  - qemu-riscv64_smode_defconfig
  - qemu-riscv64_defconfig
  - sifive_unleashed_defconfig
  - starfive_visionfive2_defconfig
with either real hardware or QEMU.

Thanks for your time and review.

Yao Zi (2):
  spl: riscv: Disable SPL_BINMAN_UBOOT_SYMBOLS by default
  riscv: Provide __image_copy_{start_end} symbols in linkerscript

 arch/riscv/cpu/u-boot-spl.lds | 2 ++
 arch/riscv/cpu/u-boot.lds     | 3 +++
 common/spl/Kconfig            | 2 ++
 3 files changed, 7 insertions(+)

-- 
2.49.0

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

[Yao Zi]

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
+	default n if RISCV
 	default y
 	help
 	  This enables use of symbols in SPL which refer to U-Boot phases,
-- 
2.49.0

[PATCH 2/2] riscv: Provide __image_copy_{start_end} symbols in linkerscript

[Yao Zi]

Binman looks for __image_copy_start to determine the base address of an
entry if elf-base-sym isn't specified, which is missing in RISC-V port.
This causes binman skips RISC-V SPL entries without filling addresses
into its .binman_sym_table section.

This patch defines __image_copy_start in linkerscript of both SPL and
proper U-Boot to ensure binman_sym functions correctly with the default
binman.dtsi. The paired symbol, __image_copy_end, is introduced as well
for completeness.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 arch/riscv/cpu/u-boot-spl.lds | 2 ++
 arch/riscv/cpu/u-boot.lds     | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/arch/riscv/cpu/u-boot-spl.lds b/arch/riscv/cpu/u-boot-spl.lds
index 907094620bd..18eb62c01c6 100644
--- a/arch/riscv/cpu/u-boot-spl.lds
+++ b/arch/riscv/cpu/u-boot-spl.lds
@@ -16,6 +16,7 @@ ENTRY(_start)
 SECTIONS
 {
 	. = ALIGN(4);
+	__image_copy_start = ADDR(.text);
 	.text : {
 		arch/riscv/cpu/start.o	(.text)
 		*(.text*)
@@ -46,6 +47,7 @@ SECTIONS
 
 	_end = .;
 	_image_binary_end = .;
+	_image_copy_end = .;
 
 	.bss : {
 		__bss_start = .;
diff --git a/arch/riscv/cpu/u-boot.lds b/arch/riscv/cpu/u-boot.lds
index 2ffe6ba3c8f..b11ea8b56d2 100644
--- a/arch/riscv/cpu/u-boot.lds
+++ b/arch/riscv/cpu/u-boot.lds
@@ -10,6 +10,7 @@ ENTRY(_start)
 SECTIONS
 {
 	. = ALIGN(4);
+	__image_copy_start = ADDR(.text);
 	.text : {
 		arch/riscv/cpu/start.o	(.text)
 	}
@@ -57,6 +58,8 @@ SECTIONS
 		__efi_runtime_rel_stop = .;
 	}
 
+	__image_copy_end = .;
+
 	/DISCARD/ : { *(.rela.plt*) }
 	.rela.dyn : {
 		__rel_dyn_start = .;
-- 
2.49.0

Re: [PATCH 2/2] riscv: Provide __image_copy_{start_end} symbols in linkerscript

[Simon Glass]

+Heinrich Schuchardt

On Mon, 7 Apr 2025 at 15:38, Yao Zi <ziyao@disroot.org> wrote:
>
> Binman looks for __image_copy_start to determine the base address of an
> entry if elf-base-sym isn't specified, which is missing in RISC-V port.
> This causes binman skips RISC-V SPL entries without filling addresses
> into its .binman_sym_table section.
>
> This patch defines __image_copy_start in linkerscript of both SPL and
> proper U-Boot to ensure binman_sym functions correctly with the default
> binman.dtsi. The paired symbol, __image_copy_end, is introduced as well
> for completeness.
>
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  arch/riscv/cpu/u-boot-spl.lds | 2 ++
>  arch/riscv/cpu/u-boot.lds     | 3 +++
>  2 files changed, 5 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>


>
> diff --git a/arch/riscv/cpu/u-boot-spl.lds b/arch/riscv/cpu/u-boot-spl.lds
> index 907094620bd..18eb62c01c6 100644
> --- a/arch/riscv/cpu/u-boot-spl.lds
> +++ b/arch/riscv/cpu/u-boot-spl.lds
> @@ -16,6 +16,7 @@ ENTRY(_start)
>  SECTIONS
>  {
>         . = ALIGN(4);
> +       __image_copy_start = ADDR(.text);
>         .text : {
>                 arch/riscv/cpu/start.o  (.text)
>                 *(.text*)
> @@ -46,6 +47,7 @@ SECTIONS
>
>         _end = .;
>         _image_binary_end = .;
> +       _image_copy_end = .;
>
>         .bss : {
>                 __bss_start = .;
> diff --git a/arch/riscv/cpu/u-boot.lds b/arch/riscv/cpu/u-boot.lds
> index 2ffe6ba3c8f..b11ea8b56d2 100644
> --- a/arch/riscv/cpu/u-boot.lds
> +++ b/arch/riscv/cpu/u-boot.lds
> @@ -10,6 +10,7 @@ ENTRY(_start)
>  SECTIONS
>  {
>         . = ALIGN(4);
> +       __image_copy_start = ADDR(.text);
>         .text : {
>                 arch/riscv/cpu/start.o  (.text)
>         }
> @@ -57,6 +58,8 @@ SECTIONS
>                 __efi_runtime_rel_stop = .;
>         }
>
> +       __image_copy_end = .;
> +
>         /DISCARD/ : { *(.rela.plt*) }
>         .rela.dyn : {
>                 __rel_dyn_start = .;
> --
> 2.49.0
>

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

[Simon Glass]

Hi Yao,

On Mon, 7 Apr 2025 at 15:38, Yao Zi <ziyao@disroot.org> 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)

Do you know where the symbol is being referenced? This sounds like a bug to me.

It is likely to be spl_get_image_pos(), perhaps called from
spl_set_header_raw_uboot().

If you are using FIT that function should not be called.

>
> 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
> +       default n if RISCV
>         default y
>         help
>           This enables use of symbols in SPL which refer to U-Boot phases,
> --
> 2.49.0
>

Regards,
Simon

Re: [PATCH 2/2] riscv: Provide __image_copy_{start_end} symbols in linkerscript

[Jonas Karlman]

Hi,

On 2025-04-07 05:37, Yao Zi wrote:
> Binman looks for __image_copy_start to determine the base address of an
> entry if elf-base-sym isn't specified, which is missing in RISC-V port.
> This causes binman skips RISC-V SPL entries without filling addresses
> into its .binman_sym_table section.
> 
> This patch defines __image_copy_start in linkerscript of both SPL and
> proper U-Boot to ensure binman_sym functions correctly with the default
> binman.dtsi. The paired symbol, __image_copy_end, is introduced as well
> for completeness.
> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  arch/riscv/cpu/u-boot-spl.lds | 2 ++
>  arch/riscv/cpu/u-boot.lds     | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/arch/riscv/cpu/u-boot-spl.lds b/arch/riscv/cpu/u-boot-spl.lds
> index 907094620bd..18eb62c01c6 100644
> --- a/arch/riscv/cpu/u-boot-spl.lds
> +++ b/arch/riscv/cpu/u-boot-spl.lds
> @@ -16,6 +16,7 @@ ENTRY(_start)
>  SECTIONS
>  {
>  	. = ALIGN(4);
> +	__image_copy_start = ADDR(.text);
>  	.text : {
>  		arch/riscv/cpu/start.o	(.text)
>  		*(.text*)
> @@ -46,6 +47,7 @@ SECTIONS
>  
>  	_end = .;
>  	_image_binary_end = .;
> +	_image_copy_end = .;

This looks like a typo, should probably be with two _ ?

Regards,
Jonas

>  
>  	.bss : {
>  		__bss_start = .;
> diff --git a/arch/riscv/cpu/u-boot.lds b/arch/riscv/cpu/u-boot.lds
> index 2ffe6ba3c8f..b11ea8b56d2 100644
> --- a/arch/riscv/cpu/u-boot.lds
> +++ b/arch/riscv/cpu/u-boot.lds
> @@ -10,6 +10,7 @@ ENTRY(_start)
>  SECTIONS
>  {
>  	. = ALIGN(4);
> +	__image_copy_start = ADDR(.text);
>  	.text : {
>  		arch/riscv/cpu/start.o	(.text)
>  	}
> @@ -57,6 +58,8 @@ SECTIONS
>  		__efi_runtime_rel_stop = .;
>  	}
>  
> +	__image_copy_end = .;
> +
>  	/DISCARD/ : { *(.rela.plt*) }
>  	.rela.dyn : {
>  		__rel_dyn_start = .;

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

[Jonas Karlman]

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.

> +	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 {
 					};
 				};
 #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;
 			};
 		};
 	};

Regards,
Jonas

Re: [PATCH 2/2] riscv: Provide __image_copy_{start_end} symbols in linkerscript

[Yao Zi]

On Mon, Apr 07, 2025 at 01:10:32PM +0200, Jonas Karlman wrote:
> Hi,
> 
> On 2025-04-07 05:37, Yao Zi wrote:
> > Binman looks for __image_copy_start to determine the base address of an
> > entry if elf-base-sym isn't specified, which is missing in RISC-V port.
> > This causes binman skips RISC-V SPL entries without filling addresses
> > into its .binman_sym_table section.
> > 
> > This patch defines __image_copy_start in linkerscript of both SPL and
> > proper U-Boot to ensure binman_sym functions correctly with the default
> > binman.dtsi. The paired symbol, __image_copy_end, is introduced as well
> > for completeness.
> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  arch/riscv/cpu/u-boot-spl.lds | 2 ++
> >  arch/riscv/cpu/u-boot.lds     | 3 +++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/arch/riscv/cpu/u-boot-spl.lds b/arch/riscv/cpu/u-boot-spl.lds
> > index 907094620bd..18eb62c01c6 100644
> > --- a/arch/riscv/cpu/u-boot-spl.lds
> > +++ b/arch/riscv/cpu/u-boot-spl.lds
> > @@ -16,6 +16,7 @@ ENTRY(_start)
> >  SECTIONS
> >  {
> >  	. = ALIGN(4);
> > +	__image_copy_start = ADDR(.text);
> >  	.text : {
> >  		arch/riscv/cpu/start.o	(.text)
> >  		*(.text*)
> > @@ -46,6 +47,7 @@ SECTIONS
> >  
> >  	_end = .;
> >  	_image_binary_end = .;
> > +	_image_copy_end = .;
> 
> This looks like a typo, should probably be with two _ ?

Yes, thanks for catching this. I'll correct it and apply Simon's tag in
v2. Simon, is it okay for you?

Thanks,
Yao Zi

> Regards,
> Jonas
> 
> >  
> >  	.bss : {
> >  		__bss_start = .;
> > diff --git a/arch/riscv/cpu/u-boot.lds b/arch/riscv/cpu/u-boot.lds
> > index 2ffe6ba3c8f..b11ea8b56d2 100644
> > --- a/arch/riscv/cpu/u-boot.lds
> > +++ b/arch/riscv/cpu/u-boot.lds
> > @@ -10,6 +10,7 @@ ENTRY(_start)
> >  SECTIONS
> >  {
> >  	. = ALIGN(4);
> > +	__image_copy_start = ADDR(.text);
> >  	.text : {
> >  		arch/riscv/cpu/start.o	(.text)
> >  	}
> > @@ -57,6 +58,8 @@ SECTIONS
> >  		__efi_runtime_rel_stop = .;
> >  	}
> >  
> > +	__image_copy_end = .;
> > +
> >  	/DISCARD/ : { *(.rela.plt*) }
> >  	.rela.dyn : {
> >  		__rel_dyn_start = .;
> 

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

[Yao Zi]

On Mon, Apr 07, 2025 at 10:50:07PM +1200, Simon Glass wrote:
> Hi Yao,
> 
> On Mon, 7 Apr 2025 at 15:38, Yao Zi <ziyao@disroot.org> 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)
> 
> Do you know where the symbol is being referenced? This sounds like a bug to me.
> 
> It is likely to be spl_get_image_pos(), perhaps called from
> spl_set_header_raw_uboot().

The symbol is referenced in common/spl/spl.c:54,

	#if CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS)
	/* See spl.h for information about this */
	#if defined(CONFIG_SPL_BUILD)
	binman_sym_declare(ulong, u_boot_any, image_pos);
	binman_sym_declare(ulong, u_boot_any, size);
	#endif

> If you are using FIT that function should not be called.

The place where binman_sym() is used isn't where an entry in .binman_sym
section is introduced. Instead, binman_sym_declare() actually defines
the entry.

So whether spl_get_image_pos() is invoked doesn't affect the failure,
since in both cases binman_sym_declare() puts an entry into .binman_sym,
confusing binman.

imho SPL_BINMAN_UBOOT_SYMBOLS should depend on !SPL_LOAD_FIT in an ideal
world, but I'm not sure the impact on current codebase thus limited the
change to RISC-V only.

Best regards,
Yao Zi

> >
> > 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
> > +       default n if RISCV
> >         default y
> >         help
> >           This enables use of symbols in SPL which refer to U-Boot phases,
> > --
> > 2.49.0
> >
> 
> Regards,
> Simon

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

[Yao Zi]

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

Re: [PATCH 2/2] riscv: Provide __image_copy_{start_end} symbols in linkerscript

[Simon Glass]

Hi Yao,

On Tue, 8 Apr 2025 at 03:32, Yao Zi <ziyao@disroot.org> wrote:
>
> On Mon, Apr 07, 2025 at 01:10:32PM +0200, Jonas Karlman wrote:
> > Hi,
> >
> > On 2025-04-07 05:37, Yao Zi wrote:
> > > Binman looks for __image_copy_start to determine the base address of an
> > > entry if elf-base-sym isn't specified, which is missing in RISC-V port.
> > > This causes binman skips RISC-V SPL entries without filling addresses
> > > into its .binman_sym_table section.
> > >
> > > This patch defines __image_copy_start in linkerscript of both SPL and
> > > proper U-Boot to ensure binman_sym functions correctly with the default
> > > binman.dtsi. The paired symbol, __image_copy_end, is introduced as well
> > > for completeness.
> > >
> > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > ---
> > >  arch/riscv/cpu/u-boot-spl.lds | 2 ++
> > >  arch/riscv/cpu/u-boot.lds     | 3 +++
> > >  2 files changed, 5 insertions(+)
> > >
> > > diff --git a/arch/riscv/cpu/u-boot-spl.lds b/arch/riscv/cpu/u-boot-spl.lds
> > > index 907094620bd..18eb62c01c6 100644
> > > --- a/arch/riscv/cpu/u-boot-spl.lds
> > > +++ b/arch/riscv/cpu/u-boot-spl.lds
> > > @@ -16,6 +16,7 @@ ENTRY(_start)
> > >  SECTIONS
> > >  {
> > >     . = ALIGN(4);
> > > +   __image_copy_start = ADDR(.text);
> > >     .text : {
> > >             arch/riscv/cpu/start.o  (.text)
> > >             *(.text*)
> > > @@ -46,6 +47,7 @@ SECTIONS
> > >
> > >     _end = .;
> > >     _image_binary_end = .;
> > > +   _image_copy_end = .;
> >
> > This looks like a typo, should probably be with two _ ?
>
> Yes, thanks for catching this. I'll correct it and apply Simon's tag in
> v2. Simon, is it okay for you?

Yes.

Regards,
Simon