[PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards
At some point back in 2018 prior_stage_fdt_address and OF_PRIOR_STAGE got introduced, in order to support a DTB handed over by an earlier stage boo loader. However we have another option in the Kconfig (OF_BOARD) which has identical semantics.
On RISC-V boards which during their startup, some of the platforms, pick up the DTB from a1 and copy it in their private gd_t. Apart from that they copy it to prior_stage_fdt_address, if the Kconfig option is selected, which is unnecessary.
So let's switch the config option for those boards to OF_BOARD and define the required board_fdt_blob_setup() for them.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- arch/riscv/cpu/cpu.c | 3 --- arch/riscv/cpu/start.S | 5 ----- arch/riscv/dts/binman.dtsi | 6 +++--- board/AndesTech/ax25-ae350/ax25-ae350.c | 1 - board/emulation/qemu-riscv/qemu-riscv.c | 9 +++++++++ board/sifive/unleashed/unleashed.c | 10 ++++------ board/sifive/unmatched/unmatched.c | 10 ++++------ configs/ae350_rv32_defconfig | 2 +- configs/ae350_rv32_spl_defconfig | 2 +- configs/ae350_rv64_defconfig | 2 +- configs/ae350_rv64_spl_defconfig | 2 +- configs/qemu-riscv32_defconfig | 2 +- configs/qemu-riscv32_smode_defconfig | 2 +- configs/qemu-riscv32_spl_defconfig | 2 +- configs/qemu-riscv64_defconfig | 2 +- configs/qemu-riscv64_smode_defconfig | 2 +- configs/qemu-riscv64_spl_defconfig | 2 +- dts/Kconfig | 2 +- 18 files changed, 31 insertions(+), 35 deletions(-)
diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c index c894ac10b536..e16f1df30254 100644 --- a/arch/riscv/cpu/cpu.c +++ b/arch/riscv/cpu/cpu.c @@ -16,9 +16,6 @@ * The variables here must be stored in the data section since they are used * before the bss section is available. */ -#ifdef CONFIG_OF_PRIOR_STAGE -phys_addr_t prior_stage_fdt_address __section(".data"); -#endif #ifndef CONFIG_XIP u32 hart_lottery __section(".data") = 0;
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S index 308b0a97a58f..76850ec9be2c 100644 --- a/arch/riscv/cpu/start.S +++ b/arch/riscv/cpu/start.S @@ -142,11 +142,6 @@ call_harts_early_init: bnez tp, secondary_hart_loop #endif
-#ifdef CONFIG_OF_PRIOR_STAGE - la t0, prior_stage_fdt_address - SREG s1, 0(t0) -#endif - jal board_init_f_init_reserve
SREG s1, GD_FIRMWARE_FDT_ADDR(gp) diff --git a/arch/riscv/dts/binman.dtsi b/arch/riscv/dts/binman.dtsi index d26cfdb78a9e..5757ef65ea4b 100644 --- a/arch/riscv/dts/binman.dtsi +++ b/arch/riscv/dts/binman.dtsi @@ -48,7 +48,7 @@ }; };
-#ifndef CONFIG_OF_PRIOR_STAGE +#ifndef CONFIG_OF_BOARD @fdt-SEQ { description = "NAME"; type = "flat_dt"; @@ -60,7 +60,7 @@ configurations { default = "conf-1";
-#ifndef CONFIG_OF_PRIOR_STAGE +#ifndef CONFIG_OF_BOARD @conf-SEQ { #else conf-1 { @@ -68,7 +68,7 @@ description = "NAME"; firmware = "opensbi"; loadables = "uboot"; -#ifndef CONFIG_OF_PRIOR_STAGE +#ifndef CONFIG_OF_BOARD fdt = "fdt-SEQ"; #endif }; diff --git a/board/AndesTech/ax25-ae350/ax25-ae350.c b/board/AndesTech/ax25-ae350/ax25-ae350.c index 81b0ee992372..4f03806272df 100644 --- a/board/AndesTech/ax25-ae350/ax25-ae350.c +++ b/board/AndesTech/ax25-ae350/ax25-ae350.c @@ -21,7 +21,6 @@
DECLARE_GLOBAL_DATA_PTR;
-extern phys_addr_t prior_stage_fdt_address; /* * Miscellaneous platform dependent initializations */ diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c index dcfd3f20bee6..aa91ca91325c 100644 --- a/board/emulation/qemu-riscv/qemu-riscv.c +++ b/board/emulation/qemu-riscv/qemu-riscv.c @@ -14,6 +14,8 @@ #include <virtio_types.h> #include <virtio.h>
+DECLARE_GLOBAL_DATA_PTR; + int board_init(void) { /* @@ -69,3 +71,10 @@ int board_fit_config_name_match(const char *name) return 0; } #endif + +void *board_fdt_blob_setup(void) +{ + /* Stored the DTB address there during our init */ + return (void *)gd->arch.firmware_fdt_addr; +} + diff --git a/board/sifive/unleashed/unleashed.c b/board/sifive/unleashed/unleashed.c index 8cd514df3005..7e89c3f740a7 100644 --- a/board/sifive/unleashed/unleashed.c +++ b/board/sifive/unleashed/unleashed.c @@ -116,12 +116,10 @@ int misc_init_r(void)
void *board_fdt_blob_setup(void) { - if (IS_ENABLED(CONFIG_OF_SEPARATE)) { - if (gd->arch.firmware_fdt_addr) - return (ulong *)gd->arch.firmware_fdt_addr; - else - return (ulong *)&_end; - } + if (gd->arch.firmware_fdt_addr) + return (void *)gd->arch.firmware_fdt_addr; + else + return (void *)&_end; }
int board_init(void) diff --git a/board/sifive/unmatched/unmatched.c b/board/sifive/unmatched/unmatched.c index d90b252baef7..2f26f92fcb2b 100644 --- a/board/sifive/unmatched/unmatched.c +++ b/board/sifive/unmatched/unmatched.c @@ -13,12 +13,10 @@
void *board_fdt_blob_setup(void) { - if (IS_ENABLED(CONFIG_OF_SEPARATE)) { - if (gd->arch.firmware_fdt_addr) - return (ulong *)gd->arch.firmware_fdt_addr; - else - return (ulong *)&_end; - } + if (gd->arch.firmware_fdt_addr) + return (void *)gd->arch.firmware_fdt_addr; + else + return (void *)&_end; }
int board_init(void) diff --git a/configs/ae350_rv32_defconfig b/configs/ae350_rv32_defconfig index 4e7a1686a64d..8b6c0b8a4a0a 100644 --- a/configs/ae350_rv32_defconfig +++ b/configs/ae350_rv32_defconfig @@ -15,7 +15,7 @@ CONFIG_CMD_SF_TEST=y # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_PREFER_SERVERIP=y CONFIG_CMD_CACHE=y -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y diff --git a/configs/ae350_rv32_spl_defconfig b/configs/ae350_rv32_spl_defconfig index 34c6af6e7e17..a0fe9b9a71df 100644 --- a/configs/ae350_rv32_spl_defconfig +++ b/configs/ae350_rv32_spl_defconfig @@ -19,7 +19,7 @@ CONFIG_CMD_SF_TEST=y # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_PREFER_SERVERIP=y CONFIG_CMD_CACHE=y -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_BOOTP_SEND_HOSTNAME=y diff --git a/configs/ae350_rv64_defconfig b/configs/ae350_rv64_defconfig index 05eee371ac2f..cb23cbd3d95e 100644 --- a/configs/ae350_rv64_defconfig +++ b/configs/ae350_rv64_defconfig @@ -16,7 +16,7 @@ CONFIG_CMD_SF_TEST=y # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_PREFER_SERVERIP=y CONFIG_CMD_CACHE=y -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y diff --git a/configs/ae350_rv64_spl_defconfig b/configs/ae350_rv64_spl_defconfig index 9cd7848c92eb..9ad312505db3 100644 --- a/configs/ae350_rv64_spl_defconfig +++ b/configs/ae350_rv64_spl_defconfig @@ -20,7 +20,7 @@ CONFIG_CMD_SF_TEST=y # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_PREFER_SERVERIP=y CONFIG_CMD_CACHE=y -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_BOOTP_SEND_HOSTNAME=y diff --git a/configs/qemu-riscv32_defconfig b/configs/qemu-riscv32_defconfig index 8ac16cf4186e..6fe133c268d7 100644 --- a/configs/qemu-riscv32_defconfig +++ b/configs/qemu-riscv32_defconfig @@ -9,6 +9,6 @@ CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_BOOTEFI_SELFTEST=y CONFIG_CMD_NVEDIT_EFI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/configs/qemu-riscv32_smode_defconfig b/configs/qemu-riscv32_smode_defconfig index 05eda439618f..c67e8206d1ab 100644 --- a/configs/qemu-riscv32_smode_defconfig +++ b/configs/qemu-riscv32_smode_defconfig @@ -10,6 +10,6 @@ CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_BOOTEFI_SELFTEST=y CONFIG_CMD_NVEDIT_EFI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/configs/qemu-riscv32_spl_defconfig b/configs/qemu-riscv32_spl_defconfig index ee81e552724d..77e81fac3af7 100644 --- a/configs/qemu-riscv32_spl_defconfig +++ b/configs/qemu-riscv32_spl_defconfig @@ -12,6 +12,6 @@ CONFIG_DISPLAY_CPUINFO=y CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_SBI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig index daf5d655d01f..90e87672aab0 100644 --- a/configs/qemu-riscv64_defconfig +++ b/configs/qemu-riscv64_defconfig @@ -10,6 +10,6 @@ CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_BOOTEFI_SELFTEST=y CONFIG_CMD_NVEDIT_EFI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig index 4a6416e2540b..0a8393903368 100644 --- a/configs/qemu-riscv64_smode_defconfig +++ b/configs/qemu-riscv64_smode_defconfig @@ -13,6 +13,6 @@ CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_BOOTEFI_SELFTEST=y CONFIG_CMD_NVEDIT_EFI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig index 429d4d814e65..a15e82dd3ee1 100644 --- a/configs/qemu-riscv64_spl_defconfig +++ b/configs/qemu-riscv64_spl_defconfig @@ -13,6 +13,6 @@ CONFIG_DISPLAY_CPUINFO=y CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_SBI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/dts/Kconfig b/dts/Kconfig index dabe0080c1ef..39270b47f9f0 100644 --- a/dts/Kconfig +++ b/dts/Kconfig @@ -107,7 +107,7 @@ config OF_EMBED Boards in the mainline U-Boot tree should not use it.
config OF_BOARD - bool "Provided by the board at runtime" + bool "Provided by the board (e.g a previous loader) at runtime" depends on !SANDBOX help If this option is enabled, the device tree will be provided by
At some point back in 2018 prior_stage_fdt_address and OF_PRIOR_STAGE got introduced, in order to support a DTB handed over by an earlier stage boo loader. However we have another option in the Kconfig (OF_BOARD) which has identical semantics.
So let's remove the option in an effort to simplify U-Boot's config and DTB management, and use OF_BOARD instead.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- arch/arm/Kconfig | 1 - board/broadcom/bcmstb/bcmstb.c | 6 ++++++ configs/bcm7260_defconfig | 2 +- configs/bcm7445_defconfig | 2 +- 4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index b5bd3284cd1c..8bc4391fcb27 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -620,7 +620,6 @@ config ARCH_BCMSTB select DM select GPIO_EXTRA_HEADER select OF_CONTROL - select OF_PRIOR_STAGE imply CMD_DM help This enables support for Broadcom ARM-based set-top box diff --git a/board/broadcom/bcmstb/bcmstb.c b/board/broadcom/bcmstb/bcmstb.c index 076ac9414418..40d9def24b7b 100644 --- a/board/broadcom/bcmstb/bcmstb.c +++ b/board/broadcom/bcmstb/bcmstb.c @@ -135,3 +135,9 @@ int board_late_init(void)
return 0; } + +void *board_fdt_blob_setup(void) +{ + /* Stored the DTB address there during our init */ + return (void *)prior_stage_fdt_address; +} diff --git a/configs/bcm7260_defconfig b/configs/bcm7260_defconfig index a42a6acb06d5..be0c945dc811 100644 --- a/configs/bcm7260_defconfig +++ b/configs/bcm7260_defconfig @@ -22,7 +22,7 @@ CONFIG_CMD_CACHE=y CONFIG_CMD_EXT2=y CONFIG_CMD_EXT4=y CONFIG_CMD_FS_GENERIC=y -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_MMC=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y diff --git a/configs/bcm7445_defconfig b/configs/bcm7445_defconfig index 96e8da0748ae..9ab72dcf37c7 100644 --- a/configs/bcm7445_defconfig +++ b/configs/bcm7445_defconfig @@ -23,7 +23,7 @@ CONFIG_CMD_CACHE=y CONFIG_CMD_EXT2=y CONFIG_CMD_EXT4=y CONFIG_CMD_FS_GENERIC=y -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
On Mon, 27 Sept 2021 at 00:48, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
At some point back in 2018 prior_stage_fdt_address and OF_PRIOR_STAGE got introduced, in order to support a DTB handed over by an earlier stage boo loader. However we have another option in the Kconfig (OF_BOARD) which has identical semantics.
So let's remove the option in an effort to simplify U-Boot's config and DTB management, and use OF_BOARD instead.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
arch/arm/Kconfig | 1 - board/broadcom/bcmstb/bcmstb.c | 6 ++++++ configs/bcm7260_defconfig | 2 +- configs/bcm7445_defconfig | 2 +- 4 files changed, 8 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
The previous patches removed OF_PRIOR_STAGE from the last consumers of the Kconfig option. Cleanup any references to it in documentation, code and configuration options.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- dts/Kconfig | 11 ++--------- include/fdtdec.h | 4 ---- lib/fdtdec.c | 2 -- tools/binman/binman.rst | 16 ++++++---------- 4 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/dts/Kconfig b/dts/Kconfig index 39270b47f9f0..100769017e12 100644 --- a/dts/Kconfig +++ b/dts/Kconfig @@ -22,7 +22,7 @@ config BINMAN config BINMAN_STANDALONE_FDT bool depends on BINMAN - default y if OF_BOARD || OF_PRIOR_STAGE + default y if OF_BOARD help This option tells U-Boot build system that a standalone device tree source is explicitly required when using binman to package U-Boot. @@ -32,7 +32,7 @@ config BINMAN_STANDALONE_FDT directory for a specific board. Such device tree sources are built for OF_SEPARATE or OF_EMBED. However for a scenario like the board device tree blob is not provided in the U-Boot build tree, but fed to U-Boot - in the runtime, e.g.: in the OF_PRIOR_STAGE case that it is passed by + in the runtime, e.g.: in the OF_BOARD case that it is passed by a prior stage bootloader. For such scenario, a standalone device tree blob containing binman node to describe how to package U-Boot should be provided explicitly. @@ -122,13 +122,6 @@ config OF_HOSTFILE This is only useful for Sandbox. Use the -d flag to U-Boot to specify the file to read.
-config OF_PRIOR_STAGE - bool "Prior stage bootloader DTB for DT control" - help - If this option is enabled, the device tree used for DT - control will be read from a device tree binary, at a memory - location passed to U-Boot by the prior stage bootloader. - endchoice
config DEFAULT_DEVICE_TREE diff --git a/include/fdtdec.h b/include/fdtdec.h index 8ac20c9a64f7..d0e13fc18313 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -55,10 +55,6 @@ struct bd_info; #define SPL_BUILD 0 #endif
-#ifdef CONFIG_OF_PRIOR_STAGE -extern phys_addr_t prior_stage_fdt_address; -#endif - /* * Information about a resource. start is the first address of the resource * and end is the last address (inclusive). The length of the resource will diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 7358cb6dd168..7b379564600d 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1580,8 +1580,6 @@ int fdtdec_setup(void) puts("Failed to read control FDT\n"); return -1; } -# elif defined(CONFIG_OF_PRIOR_STAGE) - gd->fdt_blob = (void *)(uintptr_t)prior_stage_fdt_address; # endif # ifndef CONFIG_SPL_BUILD /* Allow the early environment to override the fdt address */ diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 09e7b5719825..614df541c5ac 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -232,18 +232,18 @@ You can use other, more specific CONFIG options - see 'Automatic .dtsi inclusion' below.
-Using binman with OF_BOARD or OF_PRIOR_STAGE +Using binman with OF_BOARD --------------------------------------------
Normally binman is used with a board configured with OF_SEPARATE or OF_EMBED. This is a typical scenario where a device tree source that contains the binman node is provided in the arch/<arch>/dts directory for a specific board.
-However for a board configured with OF_BOARD or OF_PRIOR_STAGE, no device tree -blob is provided in the U-Boot build phase hence the binman node information -is not available. In order to support such use case, a new Kconfig option -BINMAN_STANDALONE_FDT is introduced, to tell the build system that a standalone -device tree blob containing binman node is explicitly required. +However for a board configured with OF_BOARD, no device tree blob is provided +in the U-Boot build phase hence the binman node information is not available. +In order to support such use case, a new Kconfig option BINMAN_STANDALONE_FDT +is introduced, to tell the build system that a standalone device tree blob +containing binman node is explicitly required.
Note there is a Kconfig option BINMAN_FDT which enables U-Boot run time to access information about binman entries, stored in the device tree in a binman @@ -252,10 +252,6 @@ For the other OF_CONTROL methods, it's quite possible binman node is not available as binman is invoked during the build phase, thus this option is not turned on by default for these OF_CONTROL methods.
-See qemu-riscv64_spl_defconfig for an example of how binman is used with -OF_PRIOR_STAGE to generate u-boot.itb image. - - Access to binman entry offsets at run time (symbols) ----------------------------------------------------
On Mon, 27 Sept 2021 at 00:48, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
The previous patches removed OF_PRIOR_STAGE from the last consumers of the Kconfig option. Cleanup any references to it in documentation, code and configuration options.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
dts/Kconfig | 11 ++--------- include/fdtdec.h | 4 ---- lib/fdtdec.c | 2 -- tools/binman/binman.rst | 16 ++++++---------- 4 files changed, 8 insertions(+), 25 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Mon, 27 Sept 2021 at 00:48, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
At some point back in 2018 prior_stage_fdt_address and OF_PRIOR_STAGE got introduced, in order to support a DTB handed over by an earlier stage boo loader. However we have another option in the Kconfig (OF_BOARD) which has identical semantics.
On RISC-V boards which during their startup, some of the platforms, pick up the DTB from a1 and copy it in their private gd_t. Apart from that they copy it to prior_stage_fdt_address, if the Kconfig option is selected, which is unnecessary.
So let's switch the config option for those boards to OF_BOARD and define the required board_fdt_blob_setup() for them.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
arch/riscv/cpu/cpu.c | 3 --- arch/riscv/cpu/start.S | 5 ----- arch/riscv/dts/binman.dtsi | 6 +++--- board/AndesTech/ax25-ae350/ax25-ae350.c | 1 - board/emulation/qemu-riscv/qemu-riscv.c | 9 +++++++++ board/sifive/unleashed/unleashed.c | 10 ++++------ board/sifive/unmatched/unmatched.c | 10 ++++------ configs/ae350_rv32_defconfig | 2 +- configs/ae350_rv32_spl_defconfig | 2 +- configs/ae350_rv64_defconfig | 2 +- configs/ae350_rv64_spl_defconfig | 2 +- configs/qemu-riscv32_defconfig | 2 +- configs/qemu-riscv32_smode_defconfig | 2 +- configs/qemu-riscv32_spl_defconfig | 2 +- configs/qemu-riscv64_defconfig | 2 +- configs/qemu-riscv64_smode_defconfig | 2 +- configs/qemu-riscv64_spl_defconfig | 2 +- dts/Kconfig | 2 +- 18 files changed, 31 insertions(+), 35 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Mon, Sep 27, 2021 at 2:48 PM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
At some point back in 2018 prior_stage_fdt_address and OF_PRIOR_STAGE got introduced, in order to support a DTB handed over by an earlier stage boo loader. However we have another option in the Kconfig (OF_BOARD) which has identical semantics.
On RISC-V boards which during their startup, some of the platforms, pick up the DTB from a1 and copy it in their private gd_t. Apart from that they copy it to prior_stage_fdt_address, if the Kconfig option is selected, which is unnecessary.
So let's switch the config option for those boards to OF_BOARD and define the required board_fdt_blob_setup() for them.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
arch/riscv/cpu/cpu.c | 3 --- arch/riscv/cpu/start.S | 5 ----- arch/riscv/dts/binman.dtsi | 6 +++--- board/AndesTech/ax25-ae350/ax25-ae350.c | 1 - board/emulation/qemu-riscv/qemu-riscv.c | 9 +++++++++ board/sifive/unleashed/unleashed.c | 10 ++++------ board/sifive/unmatched/unmatched.c | 10 ++++------ configs/ae350_rv32_defconfig | 2 +- configs/ae350_rv32_spl_defconfig | 2 +- configs/ae350_rv64_defconfig | 2 +- configs/ae350_rv64_spl_defconfig | 2 +- configs/qemu-riscv32_defconfig | 2 +- configs/qemu-riscv32_smode_defconfig | 2 +- configs/qemu-riscv32_spl_defconfig | 2 +- configs/qemu-riscv64_defconfig | 2 +- configs/qemu-riscv64_smode_defconfig | 2 +- configs/qemu-riscv64_spl_defconfig | 2 +- dts/Kconfig | 2 +- 18 files changed, 31 insertions(+), 35 deletions(-)
diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c index c894ac10b536..e16f1df30254 100644 --- a/arch/riscv/cpu/cpu.c +++ b/arch/riscv/cpu/cpu.c @@ -16,9 +16,6 @@
- The variables here must be stored in the data section since they are used
- before the bss section is available.
*/ -#ifdef CONFIG_OF_PRIOR_STAGE -phys_addr_t prior_stage_fdt_address __section(".data"); -#endif #ifndef CONFIG_XIP u32 hart_lottery __section(".data") = 0;
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S index 308b0a97a58f..76850ec9be2c 100644 --- a/arch/riscv/cpu/start.S +++ b/arch/riscv/cpu/start.S @@ -142,11 +142,6 @@ call_harts_early_init: bnez tp, secondary_hart_loop #endif
-#ifdef CONFIG_OF_PRIOR_STAGE
la t0, prior_stage_fdt_addressSREG s1, 0(t0)-#endif
jal board_init_f_init_reserve SREG s1, GD_FIRMWARE_FDT_ADDR(gp)diff --git a/arch/riscv/dts/binman.dtsi b/arch/riscv/dts/binman.dtsi index d26cfdb78a9e..5757ef65ea4b 100644 --- a/arch/riscv/dts/binman.dtsi +++ b/arch/riscv/dts/binman.dtsi @@ -48,7 +48,7 @@ }; };
-#ifndef CONFIG_OF_PRIOR_STAGE +#ifndef CONFIG_OF_BOARD @fdt-SEQ { description = "NAME"; type = "flat_dt"; @@ -60,7 +60,7 @@ configurations { default = "conf-1";
-#ifndef CONFIG_OF_PRIOR_STAGE +#ifndef CONFIG_OF_BOARD @conf-SEQ { #else conf-1 { @@ -68,7 +68,7 @@ description = "NAME"; firmware = "opensbi"; loadables = "uboot"; -#ifndef CONFIG_OF_PRIOR_STAGE +#ifndef CONFIG_OF_BOARD fdt = "fdt-SEQ"; #endif }; diff --git a/board/AndesTech/ax25-ae350/ax25-ae350.c b/board/AndesTech/ax25-ae350/ax25-ae350.c index 81b0ee992372..4f03806272df 100644 --- a/board/AndesTech/ax25-ae350/ax25-ae350.c +++ b/board/AndesTech/ax25-ae350/ax25-ae350.c @@ -21,7 +21,6 @@
DECLARE_GLOBAL_DATA_PTR;
-extern phys_addr_t prior_stage_fdt_address; /*
- Miscellaneous platform dependent initializations
*/ diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c index dcfd3f20bee6..aa91ca91325c 100644 --- a/board/emulation/qemu-riscv/qemu-riscv.c +++ b/board/emulation/qemu-riscv/qemu-riscv.c @@ -14,6 +14,8 @@ #include <virtio_types.h> #include <virtio.h>
+DECLARE_GLOBAL_DATA_PTR;
int board_init(void) { /* @@ -69,3 +71,10 @@ int board_fit_config_name_match(const char *name) return 0; } #endif
+void *board_fdt_blob_setup(void) +{
/* Stored the DTB address there during our init */return (void *)gd->arch.firmware_fdt_addr;+}
diff --git a/board/sifive/unleashed/unleashed.c b/board/sifive/unleashed/unleashed.c index 8cd514df3005..7e89c3f740a7 100644 --- a/board/sifive/unleashed/unleashed.c +++ b/board/sifive/unleashed/unleashed.c @@ -116,12 +116,10 @@ int misc_init_r(void)
void *board_fdt_blob_setup(void) {
if (IS_ENABLED(CONFIG_OF_SEPARATE)) {if (gd->arch.firmware_fdt_addr)return (ulong *)gd->arch.firmware_fdt_addr;elsereturn (ulong *)&_end;}
if (gd->arch.firmware_fdt_addr)return (void *)gd->arch.firmware_fdt_addr;elsereturn (void *)&_end;}
I was wondering if we need to check CONFIG_OF_BOARD here? I'm not sure whether we should distinguish the value of a1 register which is meaningless. It means that if we don't expect the device tree to be passed by prior stage, then the a1 register might be a trash value at the beginning, so it would still return the arch.firmware_fdt_addr here, rather than _end. And do you think that we should enable the CONFIG_OF_BOARD for unmatched and unleashed? Because it seems to me that we actually pass the device tree by prior stage (i.e. OpenSBI).
int board_init(void) diff --git a/board/sifive/unmatched/unmatched.c b/board/sifive/unmatched/unmatched.c index d90b252baef7..2f26f92fcb2b 100644 --- a/board/sifive/unmatched/unmatched.c +++ b/board/sifive/unmatched/unmatched.c @@ -13,12 +13,10 @@
void *board_fdt_blob_setup(void) {
if (IS_ENABLED(CONFIG_OF_SEPARATE)) {if (gd->arch.firmware_fdt_addr)return (ulong *)gd->arch.firmware_fdt_addr;elsereturn (ulong *)&_end;}
if (gd->arch.firmware_fdt_addr)return (void *)gd->arch.firmware_fdt_addr;elsereturn (void *)&_end;}
int board_init(void) diff --git a/configs/ae350_rv32_defconfig b/configs/ae350_rv32_defconfig index 4e7a1686a64d..8b6c0b8a4a0a 100644 --- a/configs/ae350_rv32_defconfig +++ b/configs/ae350_rv32_defconfig @@ -15,7 +15,7 @@ CONFIG_CMD_SF_TEST=y # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_PREFER_SERVERIP=y CONFIG_CMD_CACHE=y -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y diff --git a/configs/ae350_rv32_spl_defconfig b/configs/ae350_rv32_spl_defconfig index 34c6af6e7e17..a0fe9b9a71df 100644 --- a/configs/ae350_rv32_spl_defconfig +++ b/configs/ae350_rv32_spl_defconfig @@ -19,7 +19,7 @@ CONFIG_CMD_SF_TEST=y # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_PREFER_SERVERIP=y CONFIG_CMD_CACHE=y -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_BOOTP_SEND_HOSTNAME=y diff --git a/configs/ae350_rv64_defconfig b/configs/ae350_rv64_defconfig index 05eee371ac2f..cb23cbd3d95e 100644 --- a/configs/ae350_rv64_defconfig +++ b/configs/ae350_rv64_defconfig @@ -16,7 +16,7 @@ CONFIG_CMD_SF_TEST=y # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_PREFER_SERVERIP=y CONFIG_CMD_CACHE=y -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y diff --git a/configs/ae350_rv64_spl_defconfig b/configs/ae350_rv64_spl_defconfig index 9cd7848c92eb..9ad312505db3 100644 --- a/configs/ae350_rv64_spl_defconfig +++ b/configs/ae350_rv64_spl_defconfig @@ -20,7 +20,7 @@ CONFIG_CMD_SF_TEST=y # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_PREFER_SERVERIP=y CONFIG_CMD_CACHE=y -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_BOOTP_SEND_HOSTNAME=y diff --git a/configs/qemu-riscv32_defconfig b/configs/qemu-riscv32_defconfig index 8ac16cf4186e..6fe133c268d7 100644 --- a/configs/qemu-riscv32_defconfig +++ b/configs/qemu-riscv32_defconfig @@ -9,6 +9,6 @@ CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_BOOTEFI_SELFTEST=y CONFIG_CMD_NVEDIT_EFI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/configs/qemu-riscv32_smode_defconfig b/configs/qemu-riscv32_smode_defconfig index 05eda439618f..c67e8206d1ab 100644 --- a/configs/qemu-riscv32_smode_defconfig +++ b/configs/qemu-riscv32_smode_defconfig @@ -10,6 +10,6 @@ CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_BOOTEFI_SELFTEST=y CONFIG_CMD_NVEDIT_EFI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/configs/qemu-riscv32_spl_defconfig b/configs/qemu-riscv32_spl_defconfig index ee81e552724d..77e81fac3af7 100644 --- a/configs/qemu-riscv32_spl_defconfig +++ b/configs/qemu-riscv32_spl_defconfig @@ -12,6 +12,6 @@ CONFIG_DISPLAY_CPUINFO=y CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_SBI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig index daf5d655d01f..90e87672aab0 100644 --- a/configs/qemu-riscv64_defconfig +++ b/configs/qemu-riscv64_defconfig @@ -10,6 +10,6 @@ CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_BOOTEFI_SELFTEST=y CONFIG_CMD_NVEDIT_EFI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig index 4a6416e2540b..0a8393903368 100644 --- a/configs/qemu-riscv64_smode_defconfig +++ b/configs/qemu-riscv64_smode_defconfig @@ -13,6 +13,6 @@ CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_BOOTEFI_SELFTEST=y CONFIG_CMD_NVEDIT_EFI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig index 429d4d814e65..a15e82dd3ee1 100644 --- a/configs/qemu-riscv64_spl_defconfig +++ b/configs/qemu-riscv64_spl_defconfig @@ -13,6 +13,6 @@ CONFIG_DISPLAY_CPUINFO=y CONFIG_DISPLAY_BOARDINFO=y CONFIG_CMD_SBI=y # CONFIG_CMD_MII is not set -CONFIG_OF_PRIOR_STAGE=y +CONFIG_OF_BOARD=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_DM_MTD=y diff --git a/dts/Kconfig b/dts/Kconfig index dabe0080c1ef..39270b47f9f0 100644 --- a/dts/Kconfig +++ b/dts/Kconfig @@ -107,7 +107,7 @@ config OF_EMBED Boards in the mainline U-Boot tree should not use it.
config OF_BOARD
bool "Provided by the board at runtime"
bool "Provided by the board (e.g a previous loader) at runtime" depends on !SANDBOX help If this option is enabled, the device tree will be provided by-- 2.33.0
Hi Zong,
[...]
diff --git a/board/sifive/unleashed/unleashed.c b/board/sifive/unleashed/unleashed.c index 8cd514df3005..7e89c3f740a7 100644 --- a/board/sifive/unleashed/unleashed.c +++ b/board/sifive/unleashed/unleashed.c @@ -116,12 +116,10 @@ int misc_init_r(void)
void *board_fdt_blob_setup(void) {
if (IS_ENABLED(CONFIG_OF_SEPARATE)) {if (gd->arch.firmware_fdt_addr)return (ulong *)gd->arch.firmware_fdt_addr;elsereturn (ulong *)&_end;}
if (gd->arch.firmware_fdt_addr)return (void *)gd->arch.firmware_fdt_addr;elsereturn (void *)&_end;}
I was wondering if we need to check CONFIG_OF_BOARD here? I'm not sure whether we should distinguish the value of a1 register which is meaningless. It means that if we don't expect the device tree to be passed by prior stage, then the a1 register might be a trash value at the beginning, so it would still return the arch.firmware_fdt_addr here, rather than _end.
I thought about it as well. Those boards were configured up to now with 'CONFIG_OF_SEPARATE'. Which means we are looking at an existing issue? IOW the device tree was passed as part of U-Boot, which would mean a1 would have had thrash as well. Maybe a1 always has a valid DT on those boards so we never noticed?
And do you think that we should enable the CONFIG_OF_BOARD for unmatched and unleashed? Because it seems to me that we actually pass the device tree by prior stage (i.e. OpenSBI).
Yes in that case what you request makes sense for unmatched/unleashed. Return gd->arch.firmware_fdt_addr in OF_BOARD is selected otherwise return _end (instead of the current check). If that sounds good to you I'll send a v2
[...]
Regards /Ilias
On Wed, Sep 29, 2021 at 12:02:16PM +0300, Ilias Apalodimas wrote:
Hi Zong,
[...]
diff --git a/board/sifive/unleashed/unleashed.c b/board/sifive/unleashed/unleashed.c index 8cd514df3005..7e89c3f740a7 100644 --- a/board/sifive/unleashed/unleashed.c +++ b/board/sifive/unleashed/unleashed.c @@ -116,12 +116,10 @@ int misc_init_r(void)
void *board_fdt_blob_setup(void) {
if (IS_ENABLED(CONFIG_OF_SEPARATE)) {if (gd->arch.firmware_fdt_addr)return (ulong *)gd->arch.firmware_fdt_addr;elsereturn (ulong *)&_end;}
if (gd->arch.firmware_fdt_addr)return (void *)gd->arch.firmware_fdt_addr;elsereturn (void *)&_end;}
I was wondering if we need to check CONFIG_OF_BOARD here? I'm not sure whether we should distinguish the value of a1 register which is meaningless. It means that if we don't expect the device tree to be passed by prior stage, then the a1 register might be a trash value at the beginning, so it would still return the arch.firmware_fdt_addr here, rather than _end.
I thought about it as well. Those boards were configured up to now with 'CONFIG_OF_SEPARATE'. Which means we are looking at an existing issue? IOW the device tree was passed as part of U-Boot, which would mean a1 would have had thrash as well. Maybe a1 always has a valid DT on those boards so we never noticed?
And do you think that we should enable the CONFIG_OF_BOARD for unmatched and unleashed? Because it seems to me that we actually pass the device tree by prior stage (i.e. OpenSBI).
Yes in that case what you request makes sense for unmatched/unleashed. Return gd->arch.firmware_fdt_addr in OF_BOARD is selected otherwise return _end (instead of the current check). If that sounds good to you I'll send a v2
Looking a bit more at it... Apparently those boards boot from SPL. So it's SPL->OpenSBI->U-Boot. By having the config as OF_SEPARATE the *U-Boot* DTB is used. SPL passes it to OpenSBI and OpenSBI passes it on a1 to U-Boot proper. That's why the register reading works for that config.
In that case the pre-existing code is 'wrong' as well, since the DTB is not at _end, but the bogus path is never taken... (check the __weak board_fdt_blob_setup for details).
So I think I'll send a v2, keeping the config as-is and fixing the return address of the DTB in case OF_BOARD is ever selected.
Cheers /Ilias
[...]
Regards /Ilias
On Wed, Sep 29, 2021 at 6:17 PM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, Sep 29, 2021 at 12:02:16PM +0300, Ilias Apalodimas wrote:
Hi Zong,
[...]
diff --git a/board/sifive/unleashed/unleashed.c b/board/sifive/unleashed/unleashed.c index 8cd514df3005..7e89c3f740a7 100644 --- a/board/sifive/unleashed/unleashed.c +++ b/board/sifive/unleashed/unleashed.c @@ -116,12 +116,10 @@ int misc_init_r(void)
void *board_fdt_blob_setup(void) {
if (IS_ENABLED(CONFIG_OF_SEPARATE)) {if (gd->arch.firmware_fdt_addr)return (ulong *)gd->arch.firmware_fdt_addr;elsereturn (ulong *)&_end;}
if (gd->arch.firmware_fdt_addr)return (void *)gd->arch.firmware_fdt_addr;elsereturn (void *)&_end;}
I was wondering if we need to check CONFIG_OF_BOARD here? I'm not sure whether we should distinguish the value of a1 register which is meaningless. It means that if we don't expect the device tree to be passed by prior stage, then the a1 register might be a trash value at the beginning, so it would still return the arch.firmware_fdt_addr here, rather than _end.
I thought about it as well. Those boards were configured up to now with 'CONFIG_OF_SEPARATE'. Which means we are looking at an existing issue? IOW the device tree was passed as part of U-Boot, which would mean a1 would have had thrash as well. Maybe a1 always has a valid DT on those boards so we never noticed?
And do you think that we should enable the CONFIG_OF_BOARD for unmatched and unleashed? Because it seems to me that we actually pass the device tree by prior stage (i.e. OpenSBI).
Yes in that case what you request makes sense for unmatched/unleashed. Return gd->arch.firmware_fdt_addr in OF_BOARD is selected otherwise return _end (instead of the current check). If that sounds good to you I'll send a v2
Looking a bit more at it... Apparently those boards boot from SPL. So it's SPL->OpenSBI->U-Boot. By having the config as OF_SEPARATE the *U-Boot* DTB is used. SPL passes it to OpenSBI and OpenSBI passes it on a1 to U-Boot proper. That's why the register reading works for that config.
In that case the pre-existing code is 'wrong' as well, since the DTB is not at _end, but the bogus path is never taken... (check the __weak board_fdt_blob_setup for details).
If I remember correctly, the SPL would calculate the size of u-boot proper, and then put the DTB at the end of u-boot proper, so the DTB would fortuitously be put at the _end location.
So I think I'll send a v2, keeping the config as-is and fixing the return address of the DTB in case OF_BOARD is ever selected.
Yes, it seems to me that we could use a config to separate the case between the prior stage and the _end. Just note that, there is a patch on the fly, it modifies the same snippet of code, you might need to update your code based on top of it. https://lists.denx.de/pipermail/u-boot/2021-September/460378.html
Cheers /Ilias
[...]
Regards /Ilias
if (IS_ENABLED(CONFIG_OF_SEPARATE)) {if (gd->arch.firmware_fdt_addr)return (ulong *)gd->arch.firmware_fdt_addr;elsereturn (ulong *)&_end;}
if (gd->arch.firmware_fdt_addr)return (void *)gd->arch.firmware_fdt_addr;elsereturn (void *)&_end;}
I was wondering if we need to check CONFIG_OF_BOARD here? I'm not sure whether we should distinguish the value of a1 register which is meaningless. It means that if we don't expect the device tree to be passed by prior stage, then the a1 register might be a trash value at the beginning, so it would still return the arch.firmware_fdt_addr here, rather than _end.
I thought about it as well. Those boards were configured up to now with 'CONFIG_OF_SEPARATE'. Which means we are looking at an existing issue? IOW the device tree was passed as part of U-Boot, which would mean a1 would have had thrash as well. Maybe a1 always has a valid DT on those boards so we never noticed?
And do you think that we should enable the CONFIG_OF_BOARD for unmatched and unleashed? Because it seems to me that we actually pass the device tree by prior stage (i.e. OpenSBI).
Yes in that case what you request makes sense for unmatched/unleashed. Return gd->arch.firmware_fdt_addr in OF_BOARD is selected otherwise return _end (instead of the current check). If that sounds good to you I'll send a v2
Looking a bit more at it... Apparently those boards boot from SPL. So it's SPL->OpenSBI->U-Boot. By having the config as OF_SEPARATE the *U-Boot* DTB is used. SPL passes it to OpenSBI and OpenSBI passes it on a1 to U-Boot proper. That's why the register reading works for that config.
In that case the pre-existing code is 'wrong' as well, since the DTB is not at _end, but the bogus path is never taken... (check the __weak board_fdt_blob_setup for details).
If I remember correctly, the SPL would calculate the size of u-boot proper, and then put the DTB at the end of u-boot proper, so the DTB would fortuitously be put at the _end location.
I haven't yet seen the creation part, but looking into the default board_fdt_blob_setup() the location seems to vary depending on CONFIG_SPL_BUILD. If that's selected (which is the case for those boards), then it depends on yet another SPL config for a separate .bsdd section.
I don't have a board to verify my suspicion but I think reading the DTB without looking into a1 is broken for these boards.
So I think I'll send a v2, keeping the config as-is and fixing the return address of the DTB in case OF_BOARD is ever selected.
Yes, it seems to me that we could use a config to separate the case between the prior stage and the _end.
Untangling OF_SEPARATE and OF_BOARD is part of a bigger revamp I wanted to do on the handover of a device tree from previous bootloaders, since we do have similar 'problems' in Arm and TF-A. But in principle OF_SEPARATE shouldn't have per board code to overwrite it. OF_BOARD should be used for that. OF_SEPARATE should merely mean "The dtb is concatenated to my U-Boot binary.
Right now RISC-V uses OF_SEPARATE reads the DTB on SPL and then goes back to using the a1 register for U-Boot proper. We could instead read the U-Boot concatenated DTB always in that case. OF_BOARD would then be used in case OpenSBI is compiled with a *different* DTB and you'd want to use that. Any idea if OpenSBI performs fixups before handing over the dtb in a1?
Unfortunately I don't have a board to test apart from QEMU. Let me respin this, with a potential fix I have in mind and we can discuss further.
Just note that, there is a patch on the fly, it modifies the same snippet of code, you might need to update your code based on top of it. https://lists.denx.de/pipermail/u-boot/2021-September/460378.html
I'll reply to that and see if the _end is indeed a problem.
Thanks /Ilias
Date: Wed, 29 Sep 2021 15:55:48 +0300 From: Ilias Apalodimas ilias.apalodimas@linaro.org
if (IS_ENABLED(CONFIG_OF_SEPARATE)) {if (gd->arch.firmware_fdt_addr)return (ulong *)gd->arch.firmware_fdt_addr;elsereturn (ulong *)&_end;}
if (gd->arch.firmware_fdt_addr)return (void *)gd->arch.firmware_fdt_addr;elsereturn (void *)&_end;}
I was wondering if we need to check CONFIG_OF_BOARD here? I'm not sure whether we should distinguish the value of a1 register which is meaningless. It means that if we don't expect the device tree to be passed by prior stage, then the a1 register might be a trash value at the beginning, so it would still return the arch.firmware_fdt_addr here, rather than _end.
I thought about it as well. Those boards were configured up to now with 'CONFIG_OF_SEPARATE'. Which means we are looking at an existing issue? IOW the device tree was passed as part of U-Boot, which would mean a1 would have had thrash as well. Maybe a1 always has a valid DT on those boards so we never noticed?
And do you think that we should enable the CONFIG_OF_BOARD for unmatched and unleashed? Because it seems to me that we actually pass the device tree by prior stage (i.e. OpenSBI).
Yes in that case what you request makes sense for unmatched/unleashed. Return gd->arch.firmware_fdt_addr in OF_BOARD is selected otherwise return _end (instead of the current check). If that sounds good to you I'll send a v2
Looking a bit more at it... Apparently those boards boot from SPL. So it's SPL->OpenSBI->U-Boot. By having the config as OF_SEPARATE the *U-Boot* DTB is used. SPL passes it to OpenSBI and OpenSBI passes it on a1 to U-Boot proper. That's why the register reading works for that config.
In that case the pre-existing code is 'wrong' as well, since the DTB is not at _end, but the bogus path is never taken... (check the __weak board_fdt_blob_setup for details).
If I remember correctly, the SPL would calculate the size of u-boot proper, and then put the DTB at the end of u-boot proper, so the DTB would fortuitously be put at the _end location.
I haven't yet seen the creation part, but looking into the default board_fdt_blob_setup() the location seems to vary depending on CONFIG_SPL_BUILD. If that's selected (which is the case for those boards), then it depends on yet another SPL config for a separate .bsdd section.
I don't have a board to verify my suspicion but I think reading the DTB without looking into a1 is broken for these boards.
So I think I'll send a v2, keeping the config as-is and fixing the return address of the DTB in case OF_BOARD is ever selected.
Yes, it seems to me that we could use a config to separate the case between the prior stage and the _end.
Untangling OF_SEPARATE and OF_BOARD is part of a bigger revamp I wanted to do on the handover of a device tree from previous bootloaders, since we do have similar 'problems' in Arm and TF-A. But in principle OF_SEPARATE shouldn't have per board code to overwrite it. OF_BOARD should be used for that. OF_SEPARATE should merely mean "The dtb is concatenated to my U-Boot binary.
Right now RISC-V uses OF_SEPARATE reads the DTB on SPL and then goes back to using the a1 register for U-Boot proper. We could instead read the U-Boot concatenated DTB always in that case. OF_BOARD would then be used in case OpenSBI is compiled with a *different* DTB and you'd want to use that. Any idea if OpenSBI performs fixups before handing over the dtb in a1?
It does. One of the things it does is add a reserved memory entry for itself.
Unfortunately I don't have a board to test apart from QEMU. Let me respin this, with a potential fix I have in mind and we can discuss further.
Just note that, there is a patch on the fly, it modifies the same snippet of code, you might need to update your code based on top of it. https://lists.denx.de/pipermail/u-boot/2021-September/460378.html
I'll reply to that and see if the _end is indeed a problem.
Thanks /Ilias
Hi Mark,
On Wed, Sep 29, 2021 at 02:59:10PM +0200, Mark Kettenis wrote:
[...]
I was wondering if we need to check CONFIG_OF_BOARD here? I'm not sure whether we should distinguish the value of a1 register which is
Yes, it seems to me that we could use a config to separate the case between the prior stage and the _end.
Untangling OF_SEPARATE and OF_BOARD is part of a bigger revamp I wanted to do on the handover of a device tree from previous bootloaders, since we do have similar 'problems' in Arm and TF-A. But in principle OF_SEPARATE shouldn't have per board code to overwrite it. OF_BOARD should be used for that. OF_SEPARATE should merely mean "The dtb is concatenated to my U-Boot binary.
Right now RISC-V uses OF_SEPARATE reads the DTB on SPL and then goes back to using the a1 register for U-Boot proper. We could instead read the U-Boot concatenated DTB always in that case. OF_BOARD would then be used in case OpenSBI is compiled with a *different* DTB and you'd want to use that. Any idea if OpenSBI performs fixups before handing over the dtb in a1?
It does. One of the things it does is add a reserved memory entry for itself.
Ah lovely :(, then untangling that is not an option atm :(. We still have to keep board_fdt_blob_setup() a __weak symbol for those boards, even if OF_SEPARATE is selected.
Unfortunately I don't have a board to test apart from QEMU. Let me respin this, with a potential fix I have in mind and we can discuss further.
Just note that, there is a patch on the fly, it modifies the same snippet of code, you might need to update your code based on top of it. https://lists.denx.de/pipermail/u-boot/2021-September/460378.html
I'll reply to that and see if the _end is indeed a problem.
Thanks /Ilias
Thanks /Ilias
participants (4)
-
Ilias Apalodimas -
Mark Kettenis -
Simon Glass -
Zong Li