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