| Message ID | 20250407033744.4025-2-ziyao@disroot.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Andes |
| Headers | show |
| Series | Fix binman_sym functionality on RISC-V port | expand |
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
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
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
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
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,
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(+)