* [PATCH 2/3] board: arm: Remove OF_PRIOR_STAGE
2021-09-27 6:47 [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards Ilias Apalodimas
@ 2021-09-27 6:47 ` Ilias Apalodimas
2021-09-27 20:15 ` Simon Glass
2021-09-27 6:47 ` [PATCH 3/3] treewide: " Ilias Apalodimas
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Ilias Apalodimas @ 2021-09-27 6:47 UTC (permalink / raw)
To: trini
Cc: mark.kettenis, Ilias Apalodimas, Bharat Gooty,
Rayagonda Kokatanur, Rick Chen, Leo, Thomas Fitzsimmons,
Simon Glass, Bin Meng, Sean Anderson, Green Wan,
Marek Behún, Brad Kim, Zong Li, Heinrich Schuchardt,
David Abdurachmanov, Dimitri John Ledkov, u-boot
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
--
2.33.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] board: arm: Remove OF_PRIOR_STAGE
2021-09-27 6:47 ` [PATCH 2/3] board: arm: Remove OF_PRIOR_STAGE Ilias Apalodimas
@ 2021-09-27 20:15 ` Simon Glass
0 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2021-09-27 20:15 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Tom Rini, Mark Kettenis, Bharat Gooty, Rayagonda Kokatanur,
Rick Chen, Leo, Thomas Fitzsimmons, Bin Meng, Sean Anderson,
Green Wan, Marek Behún, Brad Kim, Zong Li,
Heinrich Schuchardt, David Abdurachmanov, Dimitri John Ledkov,
U-Boot Mailing List
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] treewide: Remove OF_PRIOR_STAGE
2021-09-27 6:47 [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards Ilias Apalodimas
2021-09-27 6:47 ` [PATCH 2/3] board: arm: Remove OF_PRIOR_STAGE Ilias Apalodimas
@ 2021-09-27 6:47 ` Ilias Apalodimas
2021-09-27 20:15 ` Simon Glass
2021-09-27 20:14 ` [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards Simon Glass
2021-09-29 8:33 ` Zong Li
3 siblings, 1 reply; 13+ messages in thread
From: Ilias Apalodimas @ 2021-09-27 6:47 UTC (permalink / raw)
To: trini
Cc: mark.kettenis, Ilias Apalodimas, Bharat Gooty,
Rayagonda Kokatanur, Rick Chen, Leo, Thomas Fitzsimmons,
Simon Glass, Bin Meng, Green Wan, Marek Behún,
Sean Anderson, Lukas Auer, Brad Kim, Zong Li,
Heinrich Schuchardt, Dimitri John Ledkov, David Abdurachmanov,
u-boot
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)
----------------------------------------------------
--
2.33.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] treewide: Remove OF_PRIOR_STAGE
2021-09-27 6:47 ` [PATCH 3/3] treewide: " Ilias Apalodimas
@ 2021-09-27 20:15 ` Simon Glass
0 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2021-09-27 20:15 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Tom Rini, Mark Kettenis, Bharat Gooty, Rayagonda Kokatanur,
Rick Chen, Leo, Thomas Fitzsimmons, Bin Meng, Green Wan,
Marek Behún, Sean Anderson, Lukas Auer, Brad Kim, Zong Li,
Heinrich Schuchardt, Dimitri John Ledkov, David Abdurachmanov,
U-Boot Mailing List
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards
2021-09-27 6:47 [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards Ilias Apalodimas
2021-09-27 6:47 ` [PATCH 2/3] board: arm: Remove OF_PRIOR_STAGE Ilias Apalodimas
2021-09-27 6:47 ` [PATCH 3/3] treewide: " Ilias Apalodimas
@ 2021-09-27 20:14 ` Simon Glass
2021-09-29 8:33 ` Zong Li
3 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2021-09-27 20:14 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Tom Rini, Mark Kettenis, Bharat Gooty, Rayagonda Kokatanur,
Rick Chen, Leo, Thomas Fitzsimmons, Bin Meng, Marek Behún,
Green Wan, Sean Anderson, Lukas Auer, Brad Kim, Zong Li,
Heinrich Schuchardt, David Abdurachmanov, Dimitri John Ledkov,
U-Boot Mailing List
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards
2021-09-27 6:47 [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards Ilias Apalodimas
` (2 preceding siblings ...)
2021-09-27 20:14 ` [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards Simon Glass
@ 2021-09-29 8:33 ` Zong Li
2021-09-29 9:02 ` Ilias Apalodimas
3 siblings, 1 reply; 13+ messages in thread
From: Zong Li @ 2021-09-29 8:33 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: trini, mark.kettenis, Bharat Gooty, Rayagonda Kokatanur,
Rick Chen, Leo, Thomas Fitzsimmons, Simon Glass, Bin Meng,
Marek Behún, Green Wan, Sean Anderson, Lukas Auer, Brad Kim,
Heinrich Schuchardt, David Abdurachmanov, Dimitri John Ledkov,
U-Boot Mailing List
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_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;
> }
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;
> - 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
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards
2021-09-29 8:33 ` Zong Li
@ 2021-09-29 9:02 ` Ilias Apalodimas
2021-09-29 10:17 ` Ilias Apalodimas
0 siblings, 1 reply; 13+ messages in thread
From: Ilias Apalodimas @ 2021-09-29 9:02 UTC (permalink / raw)
To: Zong Li
Cc: trini, mark.kettenis, Bharat Gooty, Rayagonda Kokatanur,
Rick Chen, Leo, Thomas Fitzsimmons, Simon Glass, Bin Meng,
Marek Behún, Green Wan, Sean Anderson, Lukas Auer, Brad Kim,
Heinrich Schuchardt, David Abdurachmanov, Dimitri John Ledkov,
U-Boot Mailing List
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;
> > - else
> > - return (ulong *)&_end;
> > - }
> > + if (gd->arch.firmware_fdt_addr)
> > + return (void *)gd->arch.firmware_fdt_addr;
> > + else
> > + return (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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards
2021-09-29 9:02 ` Ilias Apalodimas
@ 2021-09-29 10:17 ` Ilias Apalodimas
2021-09-29 11:51 ` Zong Li
0 siblings, 1 reply; 13+ messages in thread
From: Ilias Apalodimas @ 2021-09-29 10:17 UTC (permalink / raw)
To: Zong Li
Cc: trini, mark.kettenis, Bharat Gooty, Rayagonda Kokatanur,
Rick Chen, Leo, Thomas Fitzsimmons, Simon Glass, Bin Meng,
Marek Behún, Green Wan, Sean Anderson, Lukas Auer, Brad Kim,
Heinrich Schuchardt, David Abdurachmanov, Dimitri John Ledkov,
U-Boot Mailing List
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;
> > > - else
> > > - return (ulong *)&_end;
> > > - }
> > > + if (gd->arch.firmware_fdt_addr)
> > > + return (void *)gd->arch.firmware_fdt_addr;
> > > + else
> > > + return (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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards
2021-09-29 10:17 ` Ilias Apalodimas
@ 2021-09-29 11:51 ` Zong Li
2021-09-29 12:55 ` Ilias Apalodimas
0 siblings, 1 reply; 13+ messages in thread
From: Zong Li @ 2021-09-29 11:51 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: trini, mark.kettenis, Bharat Gooty, Rayagonda Kokatanur,
Rick Chen, Leo, Thomas Fitzsimmons, Simon Glass, Bin Meng,
Marek Behún, Green Wan, Sean Anderson, Lukas Auer, Brad Kim,
Heinrich Schuchardt, David Abdurachmanov, Dimitri John Ledkov,
U-Boot Mailing List
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;
> > > > - else
> > > > - return (ulong *)&_end;
> > > > - }
> > > > + if (gd->arch.firmware_fdt_addr)
> > > > + return (void *)gd->arch.firmware_fdt_addr;
> > > > + else
> > > > + return (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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards
2021-09-29 11:51 ` Zong Li
@ 2021-09-29 12:55 ` Ilias Apalodimas
2021-09-29 12:59 ` Mark Kettenis
0 siblings, 1 reply; 13+ messages in thread
From: Ilias Apalodimas @ 2021-09-29 12:55 UTC (permalink / raw)
To: Zong Li
Cc: trini, mark.kettenis, Bharat Gooty, Rayagonda Kokatanur,
Rick Chen, Leo, Thomas Fitzsimmons, Simon Glass, Bin Meng,
Marek Behún, Green Wan, Sean Anderson, Lukas Auer, Brad Kim,
Heinrich Schuchardt, David Abdurachmanov, Dimitri John Ledkov,
U-Boot Mailing List
> > > > > - 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;
> > > > > }
> > > >
> > > > 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards
2021-09-29 12:55 ` Ilias Apalodimas
@ 2021-09-29 12:59 ` Mark Kettenis
2021-09-29 13:11 ` Ilias Apalodimas
0 siblings, 1 reply; 13+ messages in thread
From: Mark Kettenis @ 2021-09-29 12:59 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: zong.li, trini, bharat.gooty, rayagonda.kokatanur, rick, ycliang,
fitzsim, sjg, bmeng.cn, marek.behun, green.wan, seanga2, lukas,
brad.kim, xypron.glpk, david.abdurachmanov, dimitri.ledkov,
u-boot
> 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;
> > > > > > - else
> > > > > > - return (ulong *)&_end;
> > > > > > - }
> > > > > > + if (gd->arch.firmware_fdt_addr)
> > > > > > + return (void *)gd->arch.firmware_fdt_addr;
> > > > > > + else
> > > > > > + return (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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] treewide: Remove OF_PRIOR_STAGE from RISC-V boards
2021-09-29 12:59 ` Mark Kettenis
@ 2021-09-29 13:11 ` Ilias Apalodimas
0 siblings, 0 replies; 13+ messages in thread
From: Ilias Apalodimas @ 2021-09-29 13:11 UTC (permalink / raw)
To: Mark Kettenis
Cc: zong.li, trini, bharat.gooty, rayagonda.kokatanur, rick, ycliang,
fitzsim, sjg, bmeng.cn, marek.behun, green.wan, seanga2, lukas,
brad.kim, xypron.glpk, david.abdurachmanov, dimitri.ledkov,
u-boot
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
^ permalink raw reply [flat|nested] 13+ messages in thread