Hi Ilias, On Tue, 26 Sept 2023 at 07:13, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > Hi Simon, > > [...] > > > > > > > > > > > So, instead of adding OF_BLOBLIST, just move this code under OF_BOARD, > > > > > inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required and > > > > > the previous stage loader is supposed to provide a DT we can just > > > > > throw an error and stop booting > > > > > > > > This is the bit I don't get. > > > > > > > > The OF_BOARD thing is a hack, in that the board can do what it likes. > > > > It is our way of handling board-specific mechanisms. > > > > > > > > But I am wanting a standard mechanism, i.e. like 'standard passage', a > > > > way of passing the DT through the phases. > > > > > > > > If I put this under OF_BOARD, then the board gets to override the > > > > mechanism, so which is it? > > > > > > No, it's the other way around in my head. OF_BOARD description is 'a > > > previous stage loader hands me over the DT', which is a superset of > > > the bloblist. > > > Whether it comes in a firmware handoff format, or a hacky register the > > > previous bootloader filled in is a detail we have to deal with and we > > > need to keep backwards compatibility. > > > > > > Maybe adding a coding snip would help > > > if (IS_ENABLED(CONFIG_OF_BOARD)) { > > > if (CONFIG_IS_ENABLED(BLOBLIST)) { <- This instead of OF_BLOBLIST > > > ret = bloblist_maybe_init(); > > > if (ret) > > > return ret; > > > /* Dynamically scan for a DT in the bloblist. */ > > > gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0); > > > if (!gd->fdt_blob) { > > > printf("Not FDT found in bloblist\n"); > > > bloblist_show_list(); > > > // We can choose to not return an error here and keep > > > scanning in case the DT is in a register, but I am fine with both > > > return -ENOENT; > > > } > > > gd->fdt_src = FDTSRC_BLOBLIST; > > > bloblist_show_list(); > > > log_debug("Devicetree is in bloblist at %p\n", gd->fdt_blob); > > > // We can also bail out of this entirely if we do find a DT via > > > a bloblist. > > > } else { > > > gd->fdt_blob = board_fdt_blob_setup(&ret); > > > if (ret) > > > return ret; > > > gd->fdt_src = FDTSRC_BOARD; > > > } > > > } > > > > > > I haven't even compiled the code above, but it should give you a > > > better idea of what I am suggesting > > > > OK I see...yes that is along the lines of what I thought you meant. > > > > But OF_BOARD does not mean 'previous stage loader hands me over the > > DT'. I means call board_fdt_blob_setup() which could do anything. If > > some boards use that function to implement getting a DT from the prior > > stage, that's fine, but it isn't limited to that. > > I think it is limited. The help message says 'Provider of DTB for DT > control' and OF_BOARD help message is 'Provided by the board (e.g a > previous loader) at runtime '. > In fact, that's exactly what I tried to clean up with commit > e7fb789612e39. Sandbox had its special OF_HOSTFILE for the DT. We > cleaned that up and then reintroduced it with a different name in > commit 275b4832f6bf91c.
That commit adds HAS_PRIOR_STATE, though, not anything to do with sandbox. OK. Perhaps it is just the OF_BOARD name that I object to? > > In any case, if people want to do 'more' we have > CONFIG_OF_BOARD_SETUP/FIXUP which should be used instead for other > platform-specific stuff. While at it OF_PRIOR_STAGE is a much better > name that OF_BOARD. So we could get rid of OF_PRIOR_STAGE and rename > OF_BOARD to that. To do that, we have to work out what to do with board-specific setup. > > The function name that is called in the setup phase is > board_fdt_blob_setup(), so it's explicitly targeting the fdt setup, or > at least that's what the name suggets. Grepping for CONFIG_OF_BOARD > matches in > board/AndesTech/ae350/ae350.c > board/armltd/vexpress64/vexpress64.c > board/raspberrypi/rpi/rpi.c > board/sifive/unleashed/unleashed.c > board/sifive/unmatched/unmatched.c > board/starfive/visionfive2/starfive_visionfive2.c > board/xilinx/common/board.c: > All these just use it to setup the DT, apart from the rpi which > additionally sets up some memory for the DT. > > Apart from that it's used on a few platform drivers to setup so DM > flags (most of them set DM_FLAG_PRE_RELOC), but we can easily change > that and use The only affected files are > drivers/pinctrl/broadcom/pinctrl-bcm283x.c > drivers/serial/serial_bcm283x_mu.c > drivers/serial/serial_bcm283x_pl011.c > > So making OF_BOARD do the right thing is trivial and I can work with > you on that. I don't think it's too much effort anyway What is the right thing you refer to here? > > > > > There is an HAS_PRIOR_STAGE option which sets OF_BOARD at present. But > > that doesn't seem right in the long term. > > > > The goal here is to standardise the passing of DT from a prior stage, > > so that all boards (except perhaps the dark child rpi) use standard > > passage (bloblist) to do so. That should not depend on OF_BOARD and in > > fact I would like to make OF_BOARD the exception, used when > > OF_BLOBLIST doesn't suit. > > We are on the same page regarding the standardization. But I think > shoehorning an OF_BLOBLIST just papers over the problem. > The board should ideally have 2 options > 1. a previous loader gives me the DT in X random ways (one of them is > the bloblist) > 2. U-Boot provides it because it was built with it > A user can always load his own special DT if he wants to, but that's > done at a later stage. OK > > But if we clean it up properly we can > 1. Do Not allow people to sneak in random options in OF_BOARD or > whatever and keep the options sane and clean I am certainly keen to remove OF_BOARD, or print a warning when it is used. > 2. enable BLOBLIST by default in all supported platforms and push to > make it the default. We can even be stricter and crash on boot if > BLOBLIST is selected and we can't find the DT in there. That would > have a similar effect to selecting it as a Kconfig option Yes, that's what I am doing with this patch. So we end up with: OF_SEPARATE - packed with U-Boot OF_BLOBLIST - comes in a bloblist OF_BOARD - escape hatch, please don't use Regards, Simon > > Thanks > /Ilias > > > > > > > > > > Hope that helps > > > /Ilias > > > > > > > > You say 'we can just throw and error' but what is the error? If the DT > > > > is provided via the bloblist, there is no error. If it is not, I > > > > already show an error and halt as you can see in the code below. > > > > > > > > I guess I'm just confused about what you are saying here. > > > > > > > > > > > > > > > > > > > > + gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, > > > > > > 0); > > > > > > + if (!gd->fdt_blob) { > > > > > > + printf("Not FDT found in bloblist\n"); > > > > > > + bloblist_show_list(); > > > > > > + return -ENOENT; > > > > > > + } > > > > > > > > > > [...] > > > > > > > > > Regards, > > Simon