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