| Message ID | 20210924131021.814662-1-ilias.apalodimas@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1,RFC] treewide: Deprecate OF_PRIOR_STAGE | expand |
Forgot to include Mark, which showed some interest for MBPs On Fri, 24 Sept 2021 at 16:10, 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 boot > loader. However we have another option in the Kconfig (OF_BOARD) which has > identical semantics. > > A good example of this is RISC-V boards which during their startup, > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > they also copy it to prior_stage_fdt_address, if the Kconfig option is > selected, which seems unnecessary(??). > > This is mostly an RFC, trying to figure out if I am missing some subtle > functionality, which would justify having 2 Kconfig options doing similar > things present. > > - Should we do this? > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > going to pass me my DTB". Why should we care if that someone is a prior > bootloader or runtime memory generated on the fly by U-Boot? It all > boils down to having a *board* specific callback for that. > - RISC-V binman should get rid of the option as well if we decide to go > though with this (but I have no idea what RISC-V expects there). > - Can we apply similar logic to OF_HOSTFILE? It seems like we could just > have a board_fdt_blob_setup() for the sandbox that reads the file we > want and get rid of another Kconfig option. > > Note that the original board which introduced CONFIG_OF_PRIOR_STAGE is still > there. If someone cares enough I guess he could fix that as well, but I don't > have the board around, so I prefer keeping it as is and mark the option as > deprecated. For that board we could also keep the prior_stage_fdt_address > without the Kconfig option and simply copy the location there, but the > board must define it's own board_fdt_blob_setup(). That would get rid of > the Kconfig option entirely instead of limiting it to that board only. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > arch/riscv/cpu/cpu.c | 3 --- > arch/riscv/cpu/start.S | 5 ----- > board/emulation/qemu-riscv/qemu-riscv.c | 8 ++++++++ > 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/bcm7260_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 | 3 ++- > lib/fdtdec.c | 4 ++++ > 18 files changed, 33 insertions(+), 32 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/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c > index dcfd3f20bee6..7dfe471dee15 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,9 @@ 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..9aa2b3e6f43a 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 (ulong *)gd->arch.firmware_fdt_addr; > + else > + return (ulong *)&_end; > } > > int board_init(void) > diff --git a/board/sifive/unmatched/unmatched.c b/board/sifive/unmatched/unmatched.c > index d90b252baef7..b703f9c3efc3 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 (ulong *)gd->arch.firmware_fdt_addr; > + else > + return (ulong *)&_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..b12a8810a221 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/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/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..2c5b2ec2d1f8 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 > @@ -124,6 +124,7 @@ config OF_HOSTFILE > > config OF_PRIOR_STAGE > bool "Prior stage bootloader DTB for DT control" > + depends on ARCH_BCMSTB > help > If this option is enabled, the device tree used for DT > control will be read from a device tree binary, at a memory > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > index 7358cb6dd168..8d0db5ac6173 100644 > --- a/lib/fdtdec.c > +++ b/lib/fdtdec.c > @@ -1581,6 +1581,10 @@ int fdtdec_setup(void) > return -1; > } > # elif defined(CONFIG_OF_PRIOR_STAGE) > + /* > + * obsolete don't use this on newer boards. Prefer CONFIG_OF_BOARD > + * instead > + */ > gd->fdt_blob = (void *)(uintptr_t)prior_stage_fdt_address; > # endif > # ifndef CONFIG_SPL_BUILD > -- > 2.33.0 >
Hi Ilias, On Fri, 24 Sept 2021 at 07:10, 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 boot > loader. However we have another option in the Kconfig (OF_BOARD) which has > identical semantics. > > A good example of this is RISC-V boards which during their startup, > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > they also copy it to prior_stage_fdt_address, if the Kconfig option is > selected, which seems unnecessary(??). > > This is mostly an RFC, trying to figure out if I am missing some subtle > functionality, which would justify having 2 Kconfig options doing similar > things present. > > - Should we do this? I think one option is better than two. I have a slight preference for OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it matters, since some of these boards are doing strange things anyway and cannot use OF_PRIOR_STAGE. So let's go with this. > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > going to pass me my DTB". Why should we care if that someone is a prior > bootloader or runtime memory generated on the fly by U-Boot? It all > boils down to having a *board* specific callback for that. More generally, I think OF_BOARD is basically 'opt out of the normal flow and do something special'. So at some point I would like to define what 'normal' is. At present, normal is OF_SEPARATE which means that the devicetree is packed with U-Boot. Really we want to add a second 'normal', to permit a devicetree (and perhaps other stuff) to be passed in. I think this should be that a bloblist is passed in, which can contain a devicetree. If it does, then the one in U-Boot is ignored. There should be a standard way to do this with U-Boot. Apart from the arch-specific selection of machine registers, the standard way should work for all boards, at some indeterminate point in the future. > - RISC-V binman should get rid of the option as well if we decide to go > though with this (but I have no idea what RISC-V expects there). > - Can we apply similar logic to OF_HOSTFILE? It seems like we could just > have a board_fdt_blob_setup() for the sandbox that reads the file we > want and get rid of another Kconfig option. May as well. I cannot see a down side but see how it goes. > > Note that the original board which introduced CONFIG_OF_PRIOR_STAGE is still > there. If someone cares enough I guess he could fix that as well, but I don't > have the board around, so I prefer keeping it as is and mark the option as > deprecated. For that board we could also keep the prior_stage_fdt_address > without the Kconfig option and simply copy the location there, but the > board must define it's own board_fdt_blob_setup(). That would get rid of > the Kconfig option entirely instead of limiting it to that board only. Just remove it. That's why we have maintainers and we cannot keep this around for one board. It really should not have got in anyway IMO. The next step (after removing OF_PRIOR_STAGE) is to make OF_BOARD a bool, i.e. taking it out of the choice. Then these boards use OF_SEPARATE, have an in-try devicetree and use OF_BOARD to override that at runtime. Step 3 is to define the second normal, as above. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > arch/riscv/cpu/cpu.c | 3 --- > arch/riscv/cpu/start.S | 5 ----- > board/emulation/qemu-riscv/qemu-riscv.c | 8 ++++++++ > 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/bcm7260_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 | 3 ++- > lib/fdtdec.c | 4 ++++ > 18 files changed, 33 insertions(+), 32 deletions(-) > Regards, Simon
On 9/24/21 3:10 PM, Ilias Apalodimas 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 boot > loader. However we have another option in the Kconfig (OF_BOARD) which has > identical semantics. > > A good example of this is RISC-V boards which during their startup, > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > they also copy it to prior_stage_fdt_address, if the Kconfig option is > selected, which seems unnecessary(??). > > This is mostly an RFC, trying to figure out if I am missing some subtle > functionality, which would justify having 2 Kconfig options doing similar > things present. > > - Should we do this? > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > going to pass me my DTB". Why should we care if that someone is a prior > bootloader or runtime memory generated on the fly by U-Boot? It all > boils down to having a *board* specific callback for that. > - RISC-V binman should get rid of the option as well if we decide to go > though with this (but I have no idea what RISC-V expects there). Just replace CONFIG_OF_PRIOR_STAGE by CONFIG_OF_BOARD. > - Can we apply similar logic to OF_HOSTFILE? It seems like we could just > have a board_fdt_blob_setup() for the sandbox that reads the file we > want and get rid of another Kconfig option. > > Note that the original board which introduced CONFIG_OF_PRIOR_STAGE is still > there. If someone cares enough I guess he could fix that as well, but I don't > have the board around, so I prefer keeping it as is and mark the option as > deprecated. For that board we could also keep the prior_stage_fdt_address > without the Kconfig option and simply copy the location there, but the > board must define it's own board_fdt_blob_setup(). That would get rid of > the Kconfig option entirely instead of limiting it to that board only. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > arch/riscv/cpu/cpu.c | 3 --- > arch/riscv/cpu/start.S | 5 ----- > board/emulation/qemu-riscv/qemu-riscv.c | 8 ++++++++ > 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/bcm7260_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 | 3 ++- > lib/fdtdec.c | 4 ++++ > 18 files changed, 33 insertions(+), 32 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/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c > index dcfd3f20bee6..7dfe471dee15 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,9 @@ 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..9aa2b3e6f43a 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 (ulong *)gd->arch.firmware_fdt_addr; (ulong *) makes no sense here. (void *) would be more adequate. > + else > + return (ulong *)&_end; ditto > } > > int board_init(void) > diff --git a/board/sifive/unmatched/unmatched.c b/board/sifive/unmatched/unmatched.c > index d90b252baef7..b703f9c3efc3 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 (ulong *)gd->arch.firmware_fdt_addr; ditto > + else > + return (ulong *)&_end; ditto > } > > 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..b12a8810a221 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/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/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..2c5b2ec2d1f8 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 > @@ -124,6 +124,7 @@ config OF_HOSTFILE > > config OF_PRIOR_STAGE > bool "Prior stage bootloader DTB for DT control" > + depends on ARCH_BCMSTB > help > If this option is enabled, the device tree used for DT > control will be read from a device tree binary, at a memory > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > index 7358cb6dd168..8d0db5ac6173 100644 > --- a/lib/fdtdec.c > +++ b/lib/fdtdec.c > @@ -1581,6 +1581,10 @@ int fdtdec_setup(void) > return -1; > } > # elif defined(CONFIG_OF_PRIOR_STAGE) > + /* > + * obsolete don't use this on newer boards. Prefer CONFIG_OF_BOARD > + * instead > + */ This comment should be in Kconfig. Best regards Heinrich > gd->fdt_blob = (void *)(uintptr_t)prior_stage_fdt_address; > # endif > # ifndef CONFIG_SPL_BUILD >
Hi Simon, On Fri, Sep 24, 2021 at 07:57:00AM -0600, Simon Glass wrote: > Hi Ilias, > > On Fri, 24 Sept 2021 at 07:10, 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 boot > > loader. However we have another option in the Kconfig (OF_BOARD) which has > > identical semantics. > > > > A good example of this is RISC-V boards which during their startup, > > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > > they also copy it to prior_stage_fdt_address, if the Kconfig option is > > selected, which seems unnecessary(??). > > > > This is mostly an RFC, trying to figure out if I am missing some subtle > > functionality, which would justify having 2 Kconfig options doing similar > > things present. > > > > - Should we do this? > > I think one option is better than two. I have a slight preference for > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it > matters, since some of these boards are doing strange things anyway > and cannot use OF_PRIOR_STAGE. So let's go with this. For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD. Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then I can send a patch on top of that, which removes the board_fdt_blob_setup() and just stores the address in a similar fashion to the removed 'prior_stage_fdt_address'. That way we can get rid of architecture specific constructs wrt to DT in gd. The callback is a bit more of a pain to maintain for multiple boards but is more flexible than an address in a register. In any case we can do something along the lines of: Check register (or blob list or whatever) if (valid dtb) fixup/amend/use (depending on what we decide) else arch specific callback That should give us enough flexibility to deal with future boards (famous last words). > > > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > > going to pass me my DTB". Why should we care if that someone is a prior > > bootloader or runtime memory generated on the fly by U-Boot? It all > > boils down to having a *board* specific callback for that. > > More generally, I think OF_BOARD is basically 'opt out of the normal > flow and do something special'. > > So at some point I would like to define what 'normal' is. At present, > normal is OF_SEPARATE which means that the devicetree is packed with > U-Boot. > > Really we want to add a second 'normal', to permit a devicetree (and > perhaps other stuff) to be passed in. I think this should be that a > bloblist is passed in, which can contain a devicetree. If it does, > then the one in U-Boot is ignored. In which case we'll have to somehow inject U-boot's control DTB(s) which I personally prefer (rather than asking the previous stage loader to be aware of U-Boot internals). > > There should be a standard way to do this with U-Boot. Apart from the > arch-specific selection of machine registers, the standard way should > work for all boards, at some indeterminate point in the future. Well that would imply that all the existing prior boot loaders agree on something with U-Boot. The bloblist is one of the best options I can think of, but I'll keep thinking about it. > > > - RISC-V binman should get rid of the option as well if we decide to go > > though with this (but I have no idea what RISC-V expects there). > > - Can we apply similar logic to OF_HOSTFILE? It seems like we could just > > have a board_fdt_blob_setup() for the sandbox that reads the file we > > want and get rid of another Kconfig option. > > May as well. I cannot see a down side but see how it goes. > Yea same here. I'll fix that as well and repost once we get some general consensus. > > > > Note that the original board which introduced CONFIG_OF_PRIOR_STAGE is still > > there. If someone cares enough I guess he could fix that as well, but I don't > > have the board around, so I prefer keeping it as is and mark the option as > > deprecated. For that board we could also keep the prior_stage_fdt_address > > without the Kconfig option and simply copy the location there, but the > > board must define it's own board_fdt_blob_setup(). That would get rid of > > the Kconfig option entirely instead of limiting it to that board only. > > Just remove it. That's why we have maintainers and we cannot keep this > around for one board. It really should not have got in anyway IMO. > > The next step (after removing OF_PRIOR_STAGE) is to make OF_BOARD a > bool, i.e. taking it out of the choice. Then these boards use > OF_SEPARATE, have an in-try devicetree and use OF_BOARD to override > that at runtime. I can give it a shot and fix it similarly. If I break it people will yell. If no one yells we don't care ? :) > > Step 3 is to define the second normal, as above. > That sounds reasonable to me. [...] Thanks /Ilias
Hi Ilias, On Fri, 24 Sept 2021 at 08:49, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Fri, Sep 24, 2021 at 07:57:00AM -0600, Simon Glass wrote: > > Hi Ilias, > > > > On Fri, 24 Sept 2021 at 07:10, 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 boot > > > loader. However we have another option in the Kconfig (OF_BOARD) which has > > > identical semantics. > > > > > > A good example of this is RISC-V boards which during their startup, > > > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > > > they also copy it to prior_stage_fdt_address, if the Kconfig option is > > > selected, which seems unnecessary(??). > > > > > > This is mostly an RFC, trying to figure out if I am missing some subtle > > > functionality, which would justify having 2 Kconfig options doing similar > > > things present. > > > > > > - Should we do this? > > > > I think one option is better than two. I have a slight preference for > > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it > > matters, since some of these boards are doing strange things anyway > > and cannot use OF_PRIOR_STAGE. So let's go with this. > > For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD. > Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then > I can send a patch on top of that, which removes the board_fdt_blob_setup() > and just stores the address in a similar fashion to the removed > 'prior_stage_fdt_address'. That way we can get rid of architecture > specific constructs wrt to DT in gd. The callback is a bit more of a pain to > maintain for multiple boards but is more flexible than an address in a > register. In any case we can do something along the lines of: > > Check register (or blob list or whatever) > if (valid dtb) > fixup/amend/use (depending on what we decide) > else > arch specific callback > > That should give us enough flexibility to deal with future boards (famous > last words). SGTM > > > > > > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > > > going to pass me my DTB". Why should we care if that someone is a prior > > > bootloader or runtime memory generated on the fly by U-Boot? It all > > > boils down to having a *board* specific callback for that. > > > > More generally, I think OF_BOARD is basically 'opt out of the normal > > flow and do something special'. > > > > So at some point I would like to define what 'normal' is. At present, > > normal is OF_SEPARATE which means that the devicetree is packed with > > U-Boot. > > > > Really we want to add a second 'normal', to permit a devicetree (and > > perhaps other stuff) to be passed in. I think this should be that a > > bloblist is passed in, which can contain a devicetree. If it does, > > then the one in U-Boot is ignored. > > In which case we'll have to somehow inject U-boot's control DTB(s) which I > personally prefer (rather than asking the previous stage loader to be aware > of U-Boot internals). We don't agree on this, as you know. > > > > > There should be a standard way to do this with U-Boot. Apart from the > > arch-specific selection of machine registers, the standard way should > > work for all boards, at some indeterminate point in the future. > > Well that would imply that all the existing prior boot loaders agree on > something with U-Boot. The bloblist is one of the best options I can think > of, but I'll keep thinking about it. > > > > > > - RISC-V binman should get rid of the option as well if we decide to go > > > though with this (but I have no idea what RISC-V expects there). > > > - Can we apply similar logic to OF_HOSTFILE? It seems like we could just > > > have a board_fdt_blob_setup() for the sandbox that reads the file we > > > want and get rid of another Kconfig option. > > > > May as well. I cannot see a down side but see how it goes. > > > > Yea same here. > I'll fix that as well and repost once we get some general consensus. > > > > > > > Note that the original board which introduced CONFIG_OF_PRIOR_STAGE is still > > > there. If someone cares enough I guess he could fix that as well, but I don't > > > have the board around, so I prefer keeping it as is and mark the option as > > > deprecated. For that board we could also keep the prior_stage_fdt_address > > > without the Kconfig option and simply copy the location there, but the > > > board must define it's own board_fdt_blob_setup(). That would get rid of > > > the Kconfig option entirely instead of limiting it to that board only. > > > > Just remove it. That's why we have maintainers and we cannot keep this > > around for one board. It really should not have got in anyway IMO. > > > > The next step (after removing OF_PRIOR_STAGE) is to make OF_BOARD a > > bool, i.e. taking it out of the choice. Then these boards use > > OF_SEPARATE, have an in-try devicetree and use OF_BOARD to override > > that at runtime. > > I can give it a shot and fix it similarly. If I break it people will yell. > If no one yells we don't care ? :) That's my feeling too. Clean-ups like this are hard enough without trying to care about boards more than the maintainers do :-) > > > > > Step 3 is to define the second normal, as above. > > > > That sounds reasonable to me. > > [...] > Regards, SImon
On Fri, Sep 24, 2021 at 04:46:58PM +0200, Heinrich Schuchardt wrote: > > > On 9/24/21 3:10 PM, Ilias Apalodimas 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 boot > > loader. However we have another option in the Kconfig (OF_BOARD) which has > > identical semantics. > > > > A good example of this is RISC-V boards which during their startup, > > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > > they also copy it to prior_stage_fdt_address, if the Kconfig option is > > selected, which seems unnecessary(??). > > > > This is mostly an RFC, trying to figure out if I am missing some subtle > > functionality, which would justify having 2 Kconfig options doing similar > > things present. > > > > - Should we do this? > > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > > going to pass me my DTB". Why should we care if that someone is a prior > > bootloader or runtime memory generated on the fly by U-Boot? It all > > boils down to having a *board* specific callback for that. > > - RISC-V binman should get rid of the option as well if we decide to go > > though with this (but I have no idea what RISC-V expects there). > > Just replace CONFIG_OF_PRIOR_STAGE by CONFIG_OF_BOARD. Ah thanks! > > > - return (ulong *)gd->arch.firmware_fdt_addr; [...] > > - else > > - return (ulong *)&_end; > > - } > > + if (gd->arch.firmware_fdt_addr) > > + return (ulong *)gd->arch.firmware_fdt_addr; > > (ulong *) makes no sense here. (void *) would be more adequate. > Yea I preserved what was already in there, since I thought that was gonna require a different patch to fix. But Since I'll be moving these lines away I might as well fix it. > > # elif defined(CONFIG_OF_PRIOR_STAGE) [...] > > + /* > > + * obsolete don't use this on newer boards. Prefer CONFIG_OF_BOARD > > + * instead > > + */ > > This comment should be in Kconfig. I'll add it on both. The point is prevent people from doing a similar thing again, even if I miss the mail on the list! Cheers /Ilias > > Best regards > > Heinrich > > > gd->fdt_blob = (void *)(uintptr_t)prior_stage_fdt_address; > > # endif > > # ifndef CONFIG_SPL_BUILD > >
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Date: Fri, 24 Sep 2021 16:12:51 +0300 > > Forgot to include Mark, which showed some interest for MBPs Well, I am currently using OF_BOARD, so this doesn't affect me. I was merely pointing out that having OF_PRIOR_STAGE makes some sense as it clearly indicates that the DT is coming from an earlier firmware stage. OF_BOARD is more flexible and could be used to read the DT from an onboard ROM or something like that. > On Fri, 24 Sept 2021 at 16:10, 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 boot > > loader. However we have another option in the Kconfig (OF_BOARD) which has > > identical semantics. > > > > A good example of this is RISC-V boards which during their startup, > > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > > they also copy it to prior_stage_fdt_address, if the Kconfig option is > > selected, which seems unnecessary(??). > > > > This is mostly an RFC, trying to figure out if I am missing some subtle > > functionality, which would justify having 2 Kconfig options doing similar > > things present. > > > > - Should we do this? > > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > > going to pass me my DTB". Why should we care if that someone is a prior > > bootloader or runtime memory generated on the fly by U-Boot? It all > > boils down to having a *board* specific callback for that. > > - RISC-V binman should get rid of the option as well if we decide to go > > though with this (but I have no idea what RISC-V expects there). > > - Can we apply similar logic to OF_HOSTFILE? It seems like we could just > > have a board_fdt_blob_setup() for the sandbox that reads the file we > > want and get rid of another Kconfig option. > > > > Note that the original board which introduced CONFIG_OF_PRIOR_STAGE is still > > there. If someone cares enough I guess he could fix that as well, but I don't > > have the board around, so I prefer keeping it as is and mark the option as > > deprecated. For that board we could also keep the prior_stage_fdt_address > > without the Kconfig option and simply copy the location there, but the > > board must define it's own board_fdt_blob_setup(). That would get rid of > > the Kconfig option entirely instead of limiting it to that board only. > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > arch/riscv/cpu/cpu.c | 3 --- > > arch/riscv/cpu/start.S | 5 ----- > > board/emulation/qemu-riscv/qemu-riscv.c | 8 ++++++++ > > 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/bcm7260_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 | 3 ++- > > lib/fdtdec.c | 4 ++++ > > 18 files changed, 33 insertions(+), 32 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/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c > > index dcfd3f20bee6..7dfe471dee15 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,9 @@ 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..9aa2b3e6f43a 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 (ulong *)gd->arch.firmware_fdt_addr; > > + else > > + return (ulong *)&_end; > > } > > > > int board_init(void) > > diff --git a/board/sifive/unmatched/unmatched.c b/board/sifive/unmatched/unmatched.c > > index d90b252baef7..b703f9c3efc3 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 (ulong *)gd->arch.firmware_fdt_addr; > > + else > > + return (ulong *)&_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..b12a8810a221 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/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/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..2c5b2ec2d1f8 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 > > @@ -124,6 +124,7 @@ config OF_HOSTFILE > > > > config OF_PRIOR_STAGE > > bool "Prior stage bootloader DTB for DT control" > > + depends on ARCH_BCMSTB > > help > > If this option is enabled, the device tree used for DT > > control will be read from a device tree binary, at a memory > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > index 7358cb6dd168..8d0db5ac6173 100644 > > --- a/lib/fdtdec.c > > +++ b/lib/fdtdec.c > > @@ -1581,6 +1581,10 @@ int fdtdec_setup(void) > > return -1; > > } > > # elif defined(CONFIG_OF_PRIOR_STAGE) > > + /* > > + * obsolete don't use this on newer boards. Prefer CONFIG_OF_BOARD > > + * instead > > + */ > > gd->fdt_blob = (void *)(uintptr_t)prior_stage_fdt_address; > > # endif > > # ifndef CONFIG_SPL_BUILD > > -- > > 2.33.0 > > >
> From: Simon Glass <sjg@chromium.org> > Date: Fri, 24 Sep 2021 07:57:00 -0600 > > Hi Ilias, > > On Fri, 24 Sept 2021 at 07:10, 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 boot > > loader. However we have another option in the Kconfig (OF_BOARD) which has > > identical semantics. > > > > A good example of this is RISC-V boards which during their startup, > > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > > they also copy it to prior_stage_fdt_address, if the Kconfig option is > > selected, which seems unnecessary(??). > > > > This is mostly an RFC, trying to figure out if I am missing some subtle > > functionality, which would justify having 2 Kconfig options doing similar > > things present. > > > > - Should we do this? > > I think one option is better than two. I have a slight preference for > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it > matters, since some of these boards are doing strange things anyway > and cannot use OF_PRIOR_STAGE. So let's go with this. > > > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > > going to pass me my DTB". Why should we care if that someone is a prior > > bootloader or runtime memory generated on the fly by U-Boot? It all > > boils down to having a *board* specific callback for that. > > More generally, I think OF_BOARD is basically 'opt out of the normal > flow and do something special'. > > So at some point I would like to define what 'normal' is. At present, > normal is OF_SEPARATE which means that the devicetree is packed with > U-Boot. > > Really we want to add a second 'normal', to permit a devicetree (and > perhaps other stuff) to be passed in. I think this should be that a > bloblist is passed in, which can contain a devicetree. If it does, > then the one in U-Boot is ignored. > > There should be a standard way to do this with U-Boot. Apart from the > arch-specific selection of machine registers, the standard way should > work for all boards, at some indeterminate point in the future. There are well-established ABIs here that you can't really change. One of those ABIs is how the Linux kernel expects to be called. On both riscv and arm64 Linux expects to find a pointer to the DTB in a register. Several U-Boot platforms take advantage of this by pretending to be a Linux kernel. This way they can be loaded by prior stage firmware that was designed to directly load a Linux kernel. This is what I do for the Apple M1, but this is also how chainloading works on some chromebooks, and there are a few platforms where a proprietary closed source first stage bootloader is used. So once again, U-Boot should be flexible here. We can certainly recommend a certain approach to folks that are building a firmware stack for new platforms, but we can't really enforce it.
Hi Mark, On Sat, Sep 25, 2021 at 07:01:07PM +0200, Mark Kettenis wrote: > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Date: Fri, 24 Sep 2021 16:12:51 +0300 > > > > Forgot to include Mark, which showed some interest for MBPs > > Well, I am currently using OF_BOARD, so this doesn't affect me. I was > merely pointing out that having OF_PRIOR_STAGE makes some sense as it > clearly indicates that the DT is coming from an earlier firmware > stage. OF_BOARD is more flexible and could be used to read the DT > from an onboard ROM or something like that. Sure, the naming is something we'd loose. But it doesn't truly offer us any advantage does it? Unless we know of a board that can read the DT from an onboard ROM *or* a register. That would make some sense in having 2 config options, so that a user could specify which DT he wants. But even in that case, I'd prefer the config to be easier and hide the details under the hood. E.g have a callback that reads the register and if you don't find any valid DTB go read a ROM (which is close too what I suggested on a following mail). The obvious disadvantage of this approach would be not allowing someone to explicitly request which DT to read, but I can live with that. Regards /Ilias > > > On Fri, 24 Sept 2021 at 16:10, 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 boot > > > loader. However we have another option in the Kconfig (OF_BOARD) which has > > > identical semantics. > > > > > > A good example of this is RISC-V boards which during their startup, > > > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > > > they also copy it to prior_stage_fdt_address, if the Kconfig option is > > > selected, which seems unnecessary(??). > > > > > > This is mostly an RFC, trying to figure out if I am missing some subtle > > > functionality, which would justify having 2 Kconfig options doing similar > > > things present. > > > > > > - Should we do this? > > > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > > > going to pass me my DTB". Why should we care if that someone is a prior > > > bootloader or runtime memory generated on the fly by U-Boot? It all > > > boils down to having a *board* specific callback for that. > > > - RISC-V binman should get rid of the option as well if we decide to go > > > though with this (but I have no idea what RISC-V expects there). > > > - Can we apply similar logic to OF_HOSTFILE? It seems like we could just > > > have a board_fdt_blob_setup() for the sandbox that reads the file we > > > want and get rid of another Kconfig option. > > > > > > Note that the original board which introduced CONFIG_OF_PRIOR_STAGE is still > > > there. If someone cares enough I guess he could fix that as well, but I don't > > > have the board around, so I prefer keeping it as is and mark the option as > > > deprecated. For that board we could also keep the prior_stage_fdt_address > > > without the Kconfig option and simply copy the location there, but the > > > board must define it's own board_fdt_blob_setup(). That would get rid of > > > the Kconfig option entirely instead of limiting it to that board only. > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > --- > > > arch/riscv/cpu/cpu.c | 3 --- > > > arch/riscv/cpu/start.S | 5 ----- > > > board/emulation/qemu-riscv/qemu-riscv.c | 8 ++++++++ > > > 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/bcm7260_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 | 3 ++- > > > lib/fdtdec.c | 4 ++++ > > > 18 files changed, 33 insertions(+), 32 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/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c > > > index dcfd3f20bee6..7dfe471dee15 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,9 @@ 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..9aa2b3e6f43a 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 (ulong *)gd->arch.firmware_fdt_addr; > > > + else > > > + return (ulong *)&_end; > > > } > > > > > > int board_init(void) > > > diff --git a/board/sifive/unmatched/unmatched.c b/board/sifive/unmatched/unmatched.c > > > index d90b252baef7..b703f9c3efc3 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 (ulong *)gd->arch.firmware_fdt_addr; > > > + else > > > + return (ulong *)&_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..b12a8810a221 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/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/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..2c5b2ec2d1f8 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 > > > @@ -124,6 +124,7 @@ config OF_HOSTFILE > > > > > > config OF_PRIOR_STAGE > > > bool "Prior stage bootloader DTB for DT control" > > > + depends on ARCH_BCMSTB > > > help > > > If this option is enabled, the device tree used for DT > > > control will be read from a device tree binary, at a memory > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > > index 7358cb6dd168..8d0db5ac6173 100644 > > > --- a/lib/fdtdec.c > > > +++ b/lib/fdtdec.c > > > @@ -1581,6 +1581,10 @@ int fdtdec_setup(void) > > > return -1; > > > } > > > # elif defined(CONFIG_OF_PRIOR_STAGE) > > > + /* > > > + * obsolete don't use this on newer boards. Prefer CONFIG_OF_BOARD > > > + * instead > > > + */ > > > gd->fdt_blob = (void *)(uintptr_t)prior_stage_fdt_address; > > > # endif > > > # ifndef CONFIG_SPL_BUILD > > > -- > > > 2.33.0 > > > > >
Hi Mark, On Sat, 25 Sept 2021 at 11:27, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Simon Glass <sjg@chromium.org> > > Date: Fri, 24 Sep 2021 07:57:00 -0600 > > > > Hi Ilias, > > > > On Fri, 24 Sept 2021 at 07:10, 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 boot > > > loader. However we have another option in the Kconfig (OF_BOARD) which has > > > identical semantics. > > > > > > A good example of this is RISC-V boards which during their startup, > > > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > > > they also copy it to prior_stage_fdt_address, if the Kconfig option is > > > selected, which seems unnecessary(??). > > > > > > This is mostly an RFC, trying to figure out if I am missing some subtle > > > functionality, which would justify having 2 Kconfig options doing similar > > > things present. > > > > > > - Should we do this? > > > > I think one option is better than two. I have a slight preference for > > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it > > matters, since some of these boards are doing strange things anyway > > and cannot use OF_PRIOR_STAGE. So let's go with this. > > > > > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > > > going to pass me my DTB". Why should we care if that someone is a prior > > > bootloader or runtime memory generated on the fly by U-Boot? It all > > > boils down to having a *board* specific callback for that. > > > > More generally, I think OF_BOARD is basically 'opt out of the normal > > flow and do something special'. > > > > So at some point I would like to define what 'normal' is. At present, > > normal is OF_SEPARATE which means that the devicetree is packed with > > U-Boot. > > > > Really we want to add a second 'normal', to permit a devicetree (and > > perhaps other stuff) to be passed in. I think this should be that a > > bloblist is passed in, which can contain a devicetree. If it does, > > then the one in U-Boot is ignored. > > > > There should be a standard way to do this with U-Boot. Apart from the > > arch-specific selection of machine registers, the standard way should > > work for all boards, at some indeterminate point in the future. > > There are well-established ABIs here that you can't really change. > One of those ABIs is how the Linux kernel expects to be called. On > both riscv and arm64 Linux expects to find a pointer to the DTB in a > register. > > Several U-Boot platforms take advantage of this by pretending to be a > Linux kernel. This way they can be loaded by prior stage firmware > that was designed to directly load a Linux kernel. This is what I do > for the Apple M1, but this is also how chainloading works on some > chromebooks, and there are a few platforms where a proprietary closed > source first stage bootloader is used. > > So once again, U-Boot should be flexible here. We can certainly > recommend a certain approach to folks that are building a firmware > stack for new platforms, but we can't really enforce it. Indeed. We can nudge people along by providing useful features. Private firmware does not seem to be going away. Regards, Simon
Hi Mark, On Sat, 25 Sept 2021 at 11:01, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Date: Fri, 24 Sep 2021 16:12:51 +0300 > > > > Forgot to include Mark, which showed some interest for MBPs > > Well, I am currently using OF_BOARD, so this doesn't affect me. I was > merely pointing out that having OF_PRIOR_STAGE makes some sense as it > clearly indicates that the DT is coming from an earlier firmware > stage. OF_BOARD is more flexible and could be used to read the DT > from an onboard ROM or something like that. Yes I totally agree and would prefer something more specific. To me, OF_BOARD is just crazy-town, every board for himself. But I think we need to get there in stages, so perhaps it doesn't matter exactly what path we take. Regards, Simon
Hi Simon, Simon Glass <sjg@chromium.org> writes: > Hi Mark, > > On Sat, 25 Sept 2021 at 11:27, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: >> >> > From: Simon Glass <sjg@chromium.org> >> > Date: Fri, 24 Sep 2021 07:57:00 -0600 >> > >> > Hi Ilias, >> > >> > On Fri, 24 Sept 2021 at 07:10, 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 boot >> > > loader. However we have another option in the Kconfig (OF_BOARD) which has >> > > identical semantics. >> > > >> > > A good example of this is RISC-V boards which during their startup, >> > > pick up the DTB from a1 and copy it in their private gd_t. Apart from that >> > > they also copy it to prior_stage_fdt_address, if the Kconfig option is >> > > selected, which seems unnecessary(??). >> > > >> > > This is mostly an RFC, trying to figure out if I am missing some subtle >> > > functionality, which would justify having 2 Kconfig options doing similar >> > > things present. >> > > >> > > - Should we do this? >> > >> > I think one option is better than two. I have a slight preference for >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it >> > matters, since some of these boards are doing strange things anyway >> > and cannot use OF_PRIOR_STAGE. So let's go with this. >> > >> > > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is >> > > going to pass me my DTB". Why should we care if that someone is a prior >> > > bootloader or runtime memory generated on the fly by U-Boot? It all >> > > boils down to having a *board* specific callback for that. >> > >> > More generally, I think OF_BOARD is basically 'opt out of the normal >> > flow and do something special'. >> > >> > So at some point I would like to define what 'normal' is. At present, >> > normal is OF_SEPARATE which means that the devicetree is packed with >> > U-Boot. >> > >> > Really we want to add a second 'normal', to permit a devicetree (and >> > perhaps other stuff) to be passed in. I think this should be that a >> > bloblist is passed in, which can contain a devicetree. If it does, >> > then the one in U-Boot is ignored. >> > >> > There should be a standard way to do this with U-Boot. Apart from the >> > arch-specific selection of machine registers, the standard way should >> > work for all boards, at some indeterminate point in the future. >> >> There are well-established ABIs here that you can't really change. >> One of those ABIs is how the Linux kernel expects to be called. On >> both riscv and arm64 Linux expects to find a pointer to the DTB in a >> register. >> >> Several U-Boot platforms take advantage of this by pretending to be a >> Linux kernel. This way they can be loaded by prior stage firmware >> that was designed to directly load a Linux kernel. This is what I do >> for the Apple M1, but this is also how chainloading works on some >> chromebooks, and there are a few platforms where a proprietary closed >> source first stage bootloader is used. >> >> So once again, U-Boot should be flexible here. We can certainly >> recommend a certain approach to folks that are building a firmware >> stack for new platforms, but we can't really enforce it. > > Indeed. > > We can nudge people along by providing useful features. Private > firmware does not seem to be going away. The situation Mark described is the same as the one I was addressing by introducing CONFIG_OF_PRIOR_STAGE for these BOLT-using Broadcom boards. BOLT is a Broadcom proprietary first- and second-stage bootloader. On these boards, the DTB that BOLT generates in-memory and provides to the Linux kernel is meant to be authoritative. I would much prefer if Broadcom provided native U-Boot support as an alternative to BOLT, including maintaining free software device trees. But in the absence of that, given that I wanted U-Boot features on these boards, I made U-Boot an intermediate (third) stage and used the BOLT-provided DTB. One reason I had for contributing the changes is that I was faintly hoping to nudge Broadcom to support these and future boards in U-Boot. My understanding is that the DTB design intent does allow things like loading a DTB from ROM, so I'm sort of treating the BOLT-provided DTB that way. But I also understand that not having U-Boot and Linux in full control of device trees for boards they support is annoying. That said, I'm glad the consensus here seems to be that it's preferable to have U-Boot accommodate/still be usable on no-DTS boards. Thomas
Simon Glass <sjg@chromium.org> writes: [...] >> > I think one option is better than two. I have a slight preference for >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it >> > matters, since some of these boards are doing strange things anyway >> > and cannot use OF_PRIOR_STAGE. So let's go with this. >> >> For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD. >> Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then >> I can send a patch on top of that, which removes the board_fdt_blob_setup() >> and just stores the address in a similar fashion to the removed >> 'prior_stage_fdt_address'. That way we can get rid of architecture >> specific constructs wrt to DT in gd. The callback is a bit more of a pain to >> maintain for multiple boards but is more flexible than an address in a >> register. In any case we can do something along the lines of: >> >> Check register (or blob list or whatever) >> if (valid dtb) >> fixup/amend/use (depending on what we decide) >> else >> arch specific callback >> >> That should give us enough flexibility to deal with future boards (famous >> last words). > > SGTM This sounds like a good generalization that would still work for the bcm7445 and bcm7260 boards. I'll test this approach on the evaluation boards I have. For the BCM7445 I may be able to import the evaluation board device tree that Broadcom publishes as part of stblinux. At runtime I may need to merge some of the in-memory items generated by BOLT, but I'll try to make this work. The BCM7260 DTS is not publicly available though, as far as I know. Thomas
Hi Thomas, On Wed, 13 Oct 2021 at 19:26, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote: > > Simon Glass <sjg@chromium.org> writes: > > [...] > > >> > I think one option is better than two. I have a slight preference for > >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it > >> > matters, since some of these boards are doing strange things anyway > >> > and cannot use OF_PRIOR_STAGE. So let's go with this. > >> > >> For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD. > >> Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then > >> I can send a patch on top of that, which removes the board_fdt_blob_setup() > >> and just stores the address in a similar fashion to the removed > >> 'prior_stage_fdt_address'. That way we can get rid of architecture > >> specific constructs wrt to DT in gd. The callback is a bit more of a pain to > >> maintain for multiple boards but is more flexible than an address in a > >> register. In any case we can do something along the lines of: > >> > >> Check register (or blob list or whatever) > >> if (valid dtb) > >> fixup/amend/use (depending on what we decide) > >> else > >> arch specific callback > >> > >> That should give us enough flexibility to deal with future boards (famous > >> last words). > > > > SGTM > > This sounds like a good generalization that would still work for the > bcm7445 and bcm7260 boards. I'll test this approach on the evaluation > boards I have. > Excellent, keep in mind there's a newer version of this [1] > For the BCM7445 I may be able to import the evaluation board device tree > that Broadcom publishes as part of stblinux. At runtime I may need to > merge some of the in-memory items generated by BOLT, but I'll try to > make this work. > > The BCM7260 DTS is not publicly available though, as far as I know. > > Thomas [1] https://lore.kernel.org/u-boot/YWVSrYe0zdg4Qi6R@ubuntu02/ p.s note I forgot to add v4 on patches 2/3 and 3/3, but this is the correct patchset Thanks! /Ilias
Hi Thomas, On Wed, 13 Oct 2021 at 10:22, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote: > > Hi Simon, > > Simon Glass <sjg@chromium.org> writes: > > > Hi Mark, > > > > On Sat, 25 Sept 2021 at 11:27, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > >> > >> > From: Simon Glass <sjg@chromium.org> > >> > Date: Fri, 24 Sep 2021 07:57:00 -0600 > >> > > >> > Hi Ilias, > >> > > >> > On Fri, 24 Sept 2021 at 07:10, 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 boot > >> > > loader. However we have another option in the Kconfig (OF_BOARD) which has > >> > > identical semantics. > >> > > > >> > > A good example of this is RISC-V boards which during their startup, > >> > > pick up the DTB from a1 and copy it in their private gd_t. Apart from that > >> > > they also copy it to prior_stage_fdt_address, if the Kconfig option is > >> > > selected, which seems unnecessary(??). > >> > > > >> > > This is mostly an RFC, trying to figure out if I am missing some subtle > >> > > functionality, which would justify having 2 Kconfig options doing similar > >> > > things present. > >> > > > >> > > - Should we do this? > >> > > >> > I think one option is better than two. I have a slight preference for > >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it > >> > matters, since some of these boards are doing strange things anyway > >> > and cannot use OF_PRIOR_STAGE. So let's go with this. > >> > > >> > > - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is > >> > > going to pass me my DTB". Why should we care if that someone is a prior > >> > > bootloader or runtime memory generated on the fly by U-Boot? It all > >> > > boils down to having a *board* specific callback for that. > >> > > >> > More generally, I think OF_BOARD is basically 'opt out of the normal > >> > flow and do something special'. > >> > > >> > So at some point I would like to define what 'normal' is. At present, > >> > normal is OF_SEPARATE which means that the devicetree is packed with > >> > U-Boot. > >> > > >> > Really we want to add a second 'normal', to permit a devicetree (and > >> > perhaps other stuff) to be passed in. I think this should be that a > >> > bloblist is passed in, which can contain a devicetree. If it does, > >> > then the one in U-Boot is ignored. > >> > > >> > There should be a standard way to do this with U-Boot. Apart from the > >> > arch-specific selection of machine registers, the standard way should > >> > work for all boards, at some indeterminate point in the future. > >> > >> There are well-established ABIs here that you can't really change. > >> One of those ABIs is how the Linux kernel expects to be called. On > >> both riscv and arm64 Linux expects to find a pointer to the DTB in a > >> register. > >> > >> Several U-Boot platforms take advantage of this by pretending to be a > >> Linux kernel. This way they can be loaded by prior stage firmware > >> that was designed to directly load a Linux kernel. This is what I do > >> for the Apple M1, but this is also how chainloading works on some > >> chromebooks, and there are a few platforms where a proprietary closed > >> source first stage bootloader is used. > >> > >> So once again, U-Boot should be flexible here. We can certainly > >> recommend a certain approach to folks that are building a firmware > >> stack for new platforms, but we can't really enforce it. > > > > Indeed. > > > > We can nudge people along by providing useful features. Private > > firmware does not seem to be going away. > > The situation Mark described is the same as the one I was addressing by > introducing CONFIG_OF_PRIOR_STAGE for these BOLT-using Broadcom boards. > BOLT is a Broadcom proprietary first- and second-stage bootloader. On > these boards, the DTB that BOLT generates in-memory and provides to the > Linux kernel is meant to be authoritative. > > I would much prefer if Broadcom provided native U-Boot support as an > alternative to BOLT, including maintaining free software device trees. > But in the absence of that, given that I wanted U-Boot features on these > boards, I made U-Boot an intermediate (third) stage and used the > BOLT-provided DTB. One reason I had for contributing the changes is > that I was faintly hoping to nudge Broadcom to support these and future > boards in U-Boot. There seems to be some genetic quirk in humans that makes them want to invent their own bootloader. I have seen it a lot in my life. > > My understanding is that the DTB design intent does allow things like > loading a DTB from ROM, so I'm sort of treating the BOLT-provided DTB > that way. But I also understand that not having U-Boot and Linux in > full control of device trees for boards they support is annoying. That > said, I'm glad the consensus here seems to be that it's preferable to > have U-Boot accommodate/still be usable on no-DTS boards. Yes, that seems clear. Regards, Simon
Hi Thomas, On Wed, 13 Oct 2021 at 10:26, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote: > > Simon Glass <sjg@chromium.org> writes: > > [...] > > >> > I think one option is better than two. I have a slight preference for > >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it > >> > matters, since some of these boards are doing strange things anyway > >> > and cannot use OF_PRIOR_STAGE. So let's go with this. > >> > >> For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD. > >> Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then > >> I can send a patch on top of that, which removes the board_fdt_blob_setup() > >> and just stores the address in a similar fashion to the removed > >> 'prior_stage_fdt_address'. That way we can get rid of architecture > >> specific constructs wrt to DT in gd. The callback is a bit more of a pain to > >> maintain for multiple boards but is more flexible than an address in a > >> register. In any case we can do something along the lines of: > >> > >> Check register (or blob list or whatever) > >> if (valid dtb) > >> fixup/amend/use (depending on what we decide) > >> else > >> arch specific callback > >> > >> That should give us enough flexibility to deal with future boards (famous > >> last words). > > > > SGTM > > This sounds like a good generalization that would still work for the > bcm7445 and bcm7260 boards. I'll test this approach on the evaluation > boards I have. > > For the BCM7445 I may be able to import the evaluation board device tree > that Broadcom publishes as part of stblinux. At runtime I may need to > merge some of the in-memory items generated by BOLT, but I'll try to > make this work. That would be good. > > The BCM7260 DTS is not publicly available though, as far as I know. Presumably it can be dumped from U-Boot? Regards, Simon
Simon Glass <sjg@chromium.org> writes: [...] > On Wed, 13 Oct 2021 at 10:26, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote: >> >> Simon Glass <sjg@chromium.org> writes: >> >> [...] >> >> >> > I think one option is better than two. I have a slight preference for >> >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it >> >> > matters, since some of these boards are doing strange things anyway >> >> > and cannot use OF_PRIOR_STAGE. So let's go with this. >> >> >> >> For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD. >> >> Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then >> >> I can send a patch on top of that, which removes the board_fdt_blob_setup() >> >> and just stores the address in a similar fashion to the removed >> >> 'prior_stage_fdt_address'. That way we can get rid of architecture >> >> specific constructs wrt to DT in gd. The callback is a bit more of a pain to >> >> maintain for multiple boards but is more flexible than an address in a >> >> register. In any case we can do something along the lines of: >> >> >> >> Check register (or blob list or whatever) >> >> if (valid dtb) >> >> fixup/amend/use (depending on what we decide) >> >> else >> >> arch specific callback >> >> >> >> That should give us enough flexibility to deal with future boards (famous >> >> last words). >> > >> > SGTM >> >> This sounds like a good generalization that would still work for the >> bcm7445 and bcm7260 boards. I'll test this approach on the evaluation >> boards I have. >> >> For the BCM7445 I may be able to import the evaluation board device tree >> that Broadcom publishes as part of stblinux. At runtime I may need to >> merge some of the in-memory items generated by BOLT, but I'll try to >> make this work. > > That would be good. > >> The BCM7260 DTS is not publicly available though, as far as I know. > > Presumably it can be dumped from U-Boot? Technically, yes, but I wouldn't want to publish the result for various reasons; e.g., it would be specific to the evaluation boards I have, and it may contain vendor-specific fields. I'd much rather this one remain a stub, until/unless Broadcom publishes a generic BCM7260 DTS under a free license. Thomas
On Wed, Oct 13, 2021 at 01:36:00PM -0400, Thomas Fitzsimmons wrote: > Simon Glass <sjg@chromium.org> writes: > > [...] > > > On Wed, 13 Oct 2021 at 10:26, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote: > >> > >> Simon Glass <sjg@chromium.org> writes: > >> > >> [...] > >> > >> >> > I think one option is better than two. I have a slight preference for > >> >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it > >> >> > matters, since some of these boards are doing strange things anyway > >> >> > and cannot use OF_PRIOR_STAGE. So let's go with this. > >> >> > >> >> For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD. > >> >> Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then > >> >> I can send a patch on top of that, which removes the board_fdt_blob_setup() > >> >> and just stores the address in a similar fashion to the removed > >> >> 'prior_stage_fdt_address'. That way we can get rid of architecture > >> >> specific constructs wrt to DT in gd. The callback is a bit more of a pain to > >> >> maintain for multiple boards but is more flexible than an address in a > >> >> register. In any case we can do something along the lines of: > >> >> > >> >> Check register (or blob list or whatever) > >> >> if (valid dtb) > >> >> fixup/amend/use (depending on what we decide) > >> >> else > >> >> arch specific callback > >> >> > >> >> That should give us enough flexibility to deal with future boards (famous > >> >> last words). > >> > > >> > SGTM > >> > >> This sounds like a good generalization that would still work for the > >> bcm7445 and bcm7260 boards. I'll test this approach on the evaluation > >> boards I have. > >> > >> For the BCM7445 I may be able to import the evaluation board device tree > >> that Broadcom publishes as part of stblinux. At runtime I may need to > >> merge some of the in-memory items generated by BOLT, but I'll try to > >> make this work. > > > > That would be good. > > > >> The BCM7260 DTS is not publicly available though, as far as I know. > > > > Presumably it can be dumped from U-Boot? > > Technically, yes, but I wouldn't want to publish the result for various > reasons; e.g., it would be specific to the evaluation boards I have, and > it may contain vendor-specific fields. I'd much rather this one remain > a stub, until/unless Broadcom publishes a generic BCM7260 DTS under a > free license. Also note that the notion that the U-Boot source tree _must_ contain a dts for a given board is something we're very much debating still, in another thread, if you're inclined to read and chime in there as well with more details about the broadcom use case and technical/legal limitations. Thanks!
Hi Thomas, On Wed, 13 Oct 2021 at 11:36, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote: > > Simon Glass <sjg@chromium.org> writes: > > [...] > > > On Wed, 13 Oct 2021 at 10:26, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote: > >> > >> Simon Glass <sjg@chromium.org> writes: > >> > >> [...] > >> > >> >> > I think one option is better than two. I have a slight preference for > >> >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it > >> >> > matters, since some of these boards are doing strange things anyway > >> >> > and cannot use OF_PRIOR_STAGE. So let's go with this. > >> >> > >> >> For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD. > >> >> Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then > >> >> I can send a patch on top of that, which removes the board_fdt_blob_setup() > >> >> and just stores the address in a similar fashion to the removed > >> >> 'prior_stage_fdt_address'. That way we can get rid of architecture > >> >> specific constructs wrt to DT in gd. The callback is a bit more of a pain to > >> >> maintain for multiple boards but is more flexible than an address in a > >> >> register. In any case we can do something along the lines of: > >> >> > >> >> Check register (or blob list or whatever) > >> >> if (valid dtb) > >> >> fixup/amend/use (depending on what we decide) > >> >> else > >> >> arch specific callback > >> >> > >> >> That should give us enough flexibility to deal with future boards (famous > >> >> last words). > >> > > >> > SGTM > >> > >> This sounds like a good generalization that would still work for the > >> bcm7445 and bcm7260 boards. I'll test this approach on the evaluation > >> boards I have. > >> > >> For the BCM7445 I may be able to import the evaluation board device tree > >> that Broadcom publishes as part of stblinux. At runtime I may need to > >> merge some of the in-memory items generated by BOLT, but I'll try to > >> make this work. > > > > That would be good. > > > >> The BCM7260 DTS is not publicly available though, as far as I know. > > > > Presumably it can be dumped from U-Boot? > > Technically, yes, but I wouldn't want to publish the result for various > reasons; e.g., it would be specific to the evaluation boards I have, and > it may contain vendor-specific fields. I'd much rather this one remain > a stub, until/unless Broadcom publishes a generic BCM7260 DTS under a > free license. OK. Do you think you could submit a patch to do all this, including some docs about the current situation? Regards, Simon > Thomas
Hi Tom, Tom Rini <trini@konsulko.com> writes: > On Wed, Oct 13, 2021 at 01:36:00PM -0400, Thomas Fitzsimmons wrote: >> Simon Glass <sjg@chromium.org> writes: >> >> [...] >> >> > On Wed, 13 Oct 2021 at 10:26, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote: >> >> >> >> Simon Glass <sjg@chromium.org> writes: >> >> >> >> [...] >> >> >> >> >> > I think one option is better than two. I have a slight preference for >> >> >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it >> >> >> > matters, since some of these boards are doing strange things anyway >> >> >> > and cannot use OF_PRIOR_STAGE. So let's go with this. >> >> >> >> >> >> For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD. >> >> >> Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then >> >> >> I can send a patch on top of that, which removes the board_fdt_blob_setup() >> >> >> and just stores the address in a similar fashion to the removed >> >> >> 'prior_stage_fdt_address'. That way we can get rid of architecture >> >> >> specific constructs wrt to DT in gd. The callback is a bit more of a pain to >> >> >> maintain for multiple boards but is more flexible than an address in a >> >> >> register. In any case we can do something along the lines of: >> >> >> >> >> >> Check register (or blob list or whatever) >> >> >> if (valid dtb) >> >> >> fixup/amend/use (depending on what we decide) >> >> >> else >> >> >> arch specific callback >> >> >> >> >> >> That should give us enough flexibility to deal with future boards (famous >> >> >> last words). >> >> > >> >> > SGTM >> >> >> >> This sounds like a good generalization that would still work for the >> >> bcm7445 and bcm7260 boards. I'll test this approach on the evaluation >> >> boards I have. >> >> >> >> For the BCM7445 I may be able to import the evaluation board device tree >> >> that Broadcom publishes as part of stblinux. At runtime I may need to >> >> merge some of the in-memory items generated by BOLT, but I'll try to >> >> make this work. >> > >> > That would be good. >> > >> >> The BCM7260 DTS is not publicly available though, as far as I know. >> > >> > Presumably it can be dumped from U-Boot? >> >> Technically, yes, but I wouldn't want to publish the result for various >> reasons; e.g., it would be specific to the evaluation boards I have, and >> it may contain vendor-specific fields. I'd much rather this one remain >> a stub, until/unless Broadcom publishes a generic BCM7260 DTS under a >> free license. > > Also note that the notion that the U-Boot source tree _must_ contain a > dts for a given board is something we're very much debating still, in > another thread, if you're inclined to read and chime in there as well > with more details about the broadcom use case and technical/legal > limitations. Thanks! Sure. I read through [1]; here are my thoughts from the BCM7445/BCM7260 perspective. First of all, for background, BCM7445 and BCM7260 are partial ports of U-Boot. They're meant to allow using nice U-Boot features like hush and FIT image loading on these platforms. But they do not handle low-level initialization -- that's done by BOLT, the proprietary first-and-second-stage bootloader -- and they don't support configuring all of the hardware on these boards. Instead these ports include a small set of drivers (e.g., SPI, eMMC, serial) and configuration that is needed to make use of the higher level features. At the time I contributed the BCM7445 support, README called OF_CONTROL an experimental feature, and device driver configuration was alternatively allowed to live in board-specific header files. My initial local implementation did that, but then I switched to OF_CONTROL before submitting the patches, since then I could get some of U-Boot's driver configuration from the prior stage (BOLT) dynamically, instead of hard-coding addresses in U-Boot source code. The proposed new policy would require me to (re-)add these hard-coded values, albeit in a DTS, not a header file. IMO that's probably a good/fair requirement for the non-technical reasons in [1]. The second section of [1] says: "Every board in U-Boot must include a devicetree sufficient to build and boot that board on suitable hardware (or emulation)." I initially read that as "boot to Linux", and so I was thinking the device tree checked into the U-Boot tree had to be sufficient to boot Linux and configure every device that Linux supports. One of Simon's responses [2] clarified for me that the policy proposal was about the control DTB for U-Boot. Now I believe the intent of the proposed policy (stated in the "Devicetree source" section of [1]) is something like "each port in U-Boot must have an in-tree device tree that is sufficient to boot/run *U-Boot itself* on at least one representative board designed around that SoC". That would make sense to me; it would permit not-full-Linux device trees that configure only the device drivers that the port needs to support a subset of U-Boot features. This would allow boards like BCM7260, which have no publicly available Linux DTS, to have a small, generic device tree just for configuring reused, GPL'd U-Boot drivers. This is in contrast to the policy mandating or encouraging dumping to DTS binary-only proprietary Linux DTBs from prior stage bootloaders or ROMs, as a precondition to the port being included in U-Boot. The policy proposal (assuming I'm understanding it correctly now) would have been clearer if one of the first two sections in devicetree.rst explicitly mentioned "control DTB for U-Boot", i.e., the fact that the policy is about U-Boot's own much simpler DTB usage, not Linux's, even though the two projects largely share the same DTSs. Thomas 1. http://patchwork.ozlabs.org/project/uboot/patch/20210919215111.3830278-3-sjg@chromium.org/ 2. https://lists.denx.de/pipermail/u-boot/2021-October/463675.html
Hi Thomas, On Fri, 15 Oct 2021 at 10:19, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote: > > Hi Tom, > > Tom Rini <trini@konsulko.com> writes: > > > On Wed, Oct 13, 2021 at 01:36:00PM -0400, Thomas Fitzsimmons wrote: > >> Simon Glass <sjg@chromium.org> writes: > >> > >> [...] > >> > >> > On Wed, 13 Oct 2021 at 10:26, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote: > >> >> > >> >> Simon Glass <sjg@chromium.org> writes: > >> >> > >> >> [...] > >> >> > >> >> >> > I think one option is better than two. I have a slight preference for > >> >> >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it > >> >> >> > matters, since some of these boards are doing strange things anyway > >> >> >> > and cannot use OF_PRIOR_STAGE. So let's go with this. > >> >> >> > >> >> >> For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD. > >> >> >> Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then > >> >> >> I can send a patch on top of that, which removes the board_fdt_blob_setup() > >> >> >> and just stores the address in a similar fashion to the removed > >> >> >> 'prior_stage_fdt_address'. That way we can get rid of architecture > >> >> >> specific constructs wrt to DT in gd. The callback is a bit more of a pain to > >> >> >> maintain for multiple boards but is more flexible than an address in a > >> >> >> register. In any case we can do something along the lines of: > >> >> >> > >> >> >> Check register (or blob list or whatever) > >> >> >> if (valid dtb) > >> >> >> fixup/amend/use (depending on what we decide) > >> >> >> else > >> >> >> arch specific callback > >> >> >> > >> >> >> That should give us enough flexibility to deal with future boards (famous > >> >> >> last words). > >> >> > > >> >> > SGTM > >> >> > >> >> This sounds like a good generalization that would still work for the > >> >> bcm7445 and bcm7260 boards. I'll test this approach on the evaluation > >> >> boards I have. > >> >> > >> >> For the BCM7445 I may be able to import the evaluation board device tree > >> >> that Broadcom publishes as part of stblinux. At runtime I may need to > >> >> merge some of the in-memory items generated by BOLT, but I'll try to > >> >> make this work. > >> > > >> > That would be good. > >> > > >> >> The BCM7260 DTS is not publicly available though, as far as I know. > >> > > >> > Presumably it can be dumped from U-Boot? > >> > >> Technically, yes, but I wouldn't want to publish the result for various > >> reasons; e.g., it would be specific to the evaluation boards I have, and > >> it may contain vendor-specific fields. I'd much rather this one remain > >> a stub, until/unless Broadcom publishes a generic BCM7260 DTS under a > >> free license. > > > > Also note that the notion that the U-Boot source tree _must_ contain a > > dts for a given board is something we're very much debating still, in > > another thread, if you're inclined to read and chime in there as well > > with more details about the broadcom use case and technical/legal > > limitations. Thanks! > > Sure. I read through [1]; here are my thoughts from the BCM7445/BCM7260 > perspective. > > First of all, for background, BCM7445 and BCM7260 are partial ports of > U-Boot. They're meant to allow using nice U-Boot features like hush and > FIT image loading on these platforms. But they do not handle low-level > initialization -- that's done by BOLT, the proprietary > first-and-second-stage bootloader -- and they don't support configuring > all of the hardware on these boards. Instead these ports include a > small set of drivers (e.g., SPI, eMMC, serial) and configuration that is > needed to make use of the higher level features. > > At the time I contributed the BCM7445 support, README called OF_CONTROL > an experimental feature, and device driver configuration was > alternatively allowed to live in board-specific header files. My > initial local implementation did that, but then I switched to OF_CONTROL > before submitting the patches, since then I could get some of U-Boot's > driver configuration from the prior stage (BOLT) dynamically, instead of > hard-coding addresses in U-Boot source code. The proposed new policy > would require me to (re-)add these hard-coded values, albeit in a DTS, > not a header file. IMO that's probably a good/fair requirement for the > non-technical reasons in [1]. > > The second section of [1] says: "Every board in U-Boot must include a > devicetree sufficient to build and boot that board on suitable hardware > (or emulation)." I initially read that as "boot to Linux", and so I was > thinking the device tree checked into the U-Boot tree had to be > sufficient to boot Linux and configure every device that Linux supports. > One of Simon's responses [2] clarified for me that the policy proposal > was about the control DTB for U-Boot. > > Now I believe the intent of the proposed policy (stated in the > "Devicetree source" section of [1]) is something like "each port in > U-Boot must have an in-tree device tree that is sufficient to boot/run > *U-Boot itself* on at least one representative board designed around > that SoC". That would make sense to me; it would permit not-full-Linux > device trees that configure only the device drivers that the port needs > to support a subset of U-Boot features. This would allow boards like > BCM7260, which have no publicly available Linux DTS, to have a small, > generic device tree just for configuring reused, GPL'd U-Boot drivers. > This is in contrast to the policy mandating or encouraging dumping to > DTS binary-only proprietary Linux DTBs from prior stage bootloaders or > ROMs, as a precondition to the port being included in U-Boot. > > The policy proposal (assuming I'm understanding it correctly now) would > have been clearer if one of the first two sections in devicetree.rst > explicitly mentioned "control DTB for U-Boot", i.e., the fact that the > policy is about U-Boot's own much simpler DTB usage, not Linux's, even > though the two projects largely share the same DTSs. OK thanks for your comments. I have updated the patch to mention the control devicetree explicitly at the top. Regards, Simon > > Thomas > > 1. http://patchwork.ozlabs.org/project/uboot/patch/20210919215111.3830278-3-sjg@chromium.org/ > > 2. https://lists.denx.de/pipermail/u-boot/2021-October/463675.html
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/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c index dcfd3f20bee6..7dfe471dee15 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,9 @@ 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..9aa2b3e6f43a 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 (ulong *)gd->arch.firmware_fdt_addr; + else + return (ulong *)&_end; } int board_init(void) diff --git a/board/sifive/unmatched/unmatched.c b/board/sifive/unmatched/unmatched.c index d90b252baef7..b703f9c3efc3 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 (ulong *)gd->arch.firmware_fdt_addr; + else + return (ulong *)&_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..b12a8810a221 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/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/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..2c5b2ec2d1f8 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 @@ -124,6 +124,7 @@ config OF_HOSTFILE config OF_PRIOR_STAGE bool "Prior stage bootloader DTB for DT control" + depends on ARCH_BCMSTB help If this option is enabled, the device tree used for DT control will be read from a device tree binary, at a memory diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 7358cb6dd168..8d0db5ac6173 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1581,6 +1581,10 @@ int fdtdec_setup(void) return -1; } # elif defined(CONFIG_OF_PRIOR_STAGE) + /* + * obsolete don't use this on newer boards. Prefer CONFIG_OF_BOARD + * instead + */ gd->fdt_blob = (void *)(uintptr_t)prior_stage_fdt_address; # endif # ifndef CONFIG_SPL_BUILD
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 boot loader. However we have another option in the Kconfig (OF_BOARD) which has identical semantics. A good example of this is RISC-V boards which during their startup, pick up the DTB from a1 and copy it in their private gd_t. Apart from that they also copy it to prior_stage_fdt_address, if the Kconfig option is selected, which seems unnecessary(??). This is mostly an RFC, trying to figure out if I am missing some subtle functionality, which would justify having 2 Kconfig options doing similar things present. - Should we do this? - Doesn't OF_BOARD and OF_PRIOR_STAGE practically mean "Someone else is going to pass me my DTB". Why should we care if that someone is a prior bootloader or runtime memory generated on the fly by U-Boot? It all boils down to having a *board* specific callback for that. - RISC-V binman should get rid of the option as well if we decide to go though with this (but I have no idea what RISC-V expects there). - Can we apply similar logic to OF_HOSTFILE? It seems like we could just have a board_fdt_blob_setup() for the sandbox that reads the file we want and get rid of another Kconfig option. Note that the original board which introduced CONFIG_OF_PRIOR_STAGE is still there. If someone cares enough I guess he could fix that as well, but I don't have the board around, so I prefer keeping it as is and mark the option as deprecated. For that board we could also keep the prior_stage_fdt_address without the Kconfig option and simply copy the location there, but the board must define it's own board_fdt_blob_setup(). That would get rid of the Kconfig option entirely instead of limiting it to that board only. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- arch/riscv/cpu/cpu.c | 3 --- arch/riscv/cpu/start.S | 5 ----- board/emulation/qemu-riscv/qemu-riscv.c | 8 ++++++++ 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/bcm7260_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 | 3 ++- lib/fdtdec.c | 4 ++++ 18 files changed, 33 insertions(+), 32 deletions(-)