[PATCH 1/1 RFC] treewide: Deprecate OF_PRIOR_STAGE

[Simon Glass]

Hi Ilias,

On Fri, 24 Sept 2021 at 08:49, Ilias Apalodimas
<ilias.apalodimas at 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 at 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