diff mbox series

[1/1,RFC] treewide: Deprecate OF_PRIOR_STAGE

Message ID 20210924131021.814662-1-ilias.apalodimas@linaro.org
State New
Headers show
Series [1/1,RFC] treewide: Deprecate OF_PRIOR_STAGE | expand

Commit Message

Ilias Apalodimas Sept. 24, 2021, 1:10 p.m. UTC
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(-)

-- 
2.33.0

Comments

Ilias Apalodimas Sept. 24, 2021, 1:12 p.m. UTC | #1
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

>
Simon Glass Sept. 24, 2021, 1:57 p.m. UTC | #2
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
Heinrich Schuchardt Sept. 24, 2021, 2:46 p.m. UTC | #3
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

>
Ilias Apalodimas Sept. 24, 2021, 2:49 p.m. UTC | #4
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
Simon Glass Sept. 24, 2021, 4:07 p.m. UTC | #5
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
Ilias Apalodimas Sept. 24, 2021, 4:46 p.m. UTC | #6
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

> >
Mark Kettenis Sept. 25, 2021, 5:01 p.m. UTC | #7
> 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

> >

>
Mark Kettenis Sept. 25, 2021, 5:27 p.m. UTC | #8
> 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.
Ilias Apalodimas Sept. 25, 2021, 5:49 p.m. UTC | #9
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

> > >

> >
Simon Glass Sept. 26, 2021, 3:53 p.m. UTC | #10
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
Simon Glass Sept. 26, 2021, 3:54 p.m. UTC | #11
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
Thomas Fitzsimmons Oct. 13, 2021, 4:22 p.m. UTC | #12
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
Thomas Fitzsimmons Oct. 13, 2021, 4:26 p.m. UTC | #13
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
Ilias Apalodimas Oct. 13, 2021, 4:53 p.m. UTC | #14
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
Simon Glass Oct. 13, 2021, 4:58 p.m. UTC | #15
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
Simon Glass Oct. 13, 2021, 4:58 p.m. UTC | #16
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
Thomas Fitzsimmons Oct. 13, 2021, 5:36 p.m. UTC | #17
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
Tom Rini Oct. 13, 2021, 5:58 p.m. UTC | #18
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!

-- 
Tom
Simon Glass Oct. 13, 2021, 6:05 p.m. UTC | #19
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
Thomas Fitzsimmons Oct. 15, 2021, 4:19 p.m. UTC | #20
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
Simon Glass Oct. 24, 2021, 7:54 p.m. UTC | #21
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 mbox series

Patch

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