[PATCH v6 00/25] fdt: Make OF_BOARD a boolean option

[Ilias Apalodimas]

Hi Simon, 

On Sat, Dec 04, 2021 at 10:25:29AM -0700, Simon Glass wrote:
> Hi Ilias,
> 
> On Sat, 4 Dec 2021 at 08:58, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Simon,
> >
> > [...]
> > > > [huge snip]
> > > > > > There's things that need to be cleaned up because we have some small
> > > > > > number of platforms that went off and did their own thing.  But largely
> > > > > > yes, things make sense to me.  We have:
> > > > > > - We embedded the device tree that will configure U-Boot, because there
> > > > > >   is no way for the hardware to have provided us one.
> > > > > > - We do not embed the device tree that will configure U-Boot, because
> > > > > >   there is already one present in memory for us to use.
> > > > > >
> > > > > > Then we have the developer option of:
> > > > > > - We embedded the device tree that will configure U-Boot, because we're
> > > > > >   developing something.
> > > > >
> > > > > Yes, agreed those are the cases. To me this needs to be a run-time choice.
> > > >
> >
> > I am not convinced the case is "we are developing something".  The
> > arguments for this are something along the lines of:
> > 1. U-Boot will fail to compile if OF_SEPARATE is selected and you provide
> > no DT so we need to include it.  With which I disagree.  The config option
> > says "You must provide an external device tree" for a reason.
> 
> Where are you reading that? I'm lost.

In the XEN patch commit message [1]

> 
> OF_SEPARATE means it is not embedded in U-Boot but is attached to the end of it.
> 
> > 2. The DT's are  hard to find.  The 'hard to find' RPI DTs are in fact
> > committed in the kernel. I am not sure about the rest, but honestly this
> > isn't a convincing argument for me.
> 
> Great, so you've solved that one but even that was confusing for me,
> as my patch should make clear. Every single board would be its own
> special snowflake if we went that way.
> 
> So if they are in the kernel, we just need to put them in U-Boot too,
> so problem solved. The auto-sync thing that I believe Rob is working
> on will make things easy.

My problem is not usable DTs like the PRI4.  My problem is the OF_BOARD
being a runtime option and always building that DT, while at the same time
introduce a third option to not include it in the binary.  Another problem
is 'empty' DTs.

> 
> >
> > What else are we gaining with it being a run time choice?  One of the
> 
> We are requiring a DT to be present in the U-Boot tree for
> development, documentation and discoverability purposes. Devs can turn
> OF_BOARD on and off as it suits them when iterating on a platform.

They can also do the same thing without tangling the 2 options.  The *real*
problem is that in a month from now we'll get a patch saying "I need to
change X on RPI4 DTB because I have this special reason and I want a
specific u-boot binding" and then we are back at the start.  We also have
OF_EMBED for the 'developer' option which is also mentioned explicitly in
the Kconfig.

> 
> > things this approach does address, but we keep conveniently ignoring, is
> > that it tries to preserve the current status quo.  You can go and add the
> > special missing U-Boot nodes in those DT's.  So that would bypass problems
> > if the bindings get NAK'ed.  But this is going to work against the
> > fragmentation we are trying to resolve.
> 
> Well that's another reason why we need in-tree DTs at the moment. That
> reason goes away once we have our bindings upstream and are able to do
> what we need with DT there.
> 

Again, that's a huge if.  I am honestly not saying this in bad faith, but I
have my concerns on if and what gets upstreamed.  So what this really does
in my head is preserve the current functionality.  So what I am trying to
avoid here is, in case the bindings get NAK'ed, we go back and say "that's
fine we got this covered, look it's in docs and commit messages!" 

> >
> > > > But it's not possible.  That's the problem we keep going around and
> > > > around about.  People keep raising real life examples where you cannot
> > > > make a run time choice between "device tree we're passed at run time"
> > > > and "device tree we're compiled with".
> > >
> > > I haven't seen one. The most extreme case is QEMU and it works fine. I
> > > even added a test with it. What am I missing?
> 
> (I think my point there is made)
> 
> >
> > IMHO 3b595da441cf is the commit that started the problem and tangled those
> > 2 options.  Note that this support has been removed from the dragonboard
> > later in 0204d1b56b2f ....
> 
> Yes it is one of them. There may be cases where we want to patch up
> the DT that U-Boot builds. In fact OF_BOARD does not mean it came from
> a prior stage. All sorts of things could be going on. We should not
> conflate building a DT with OF_BOARD.

It says 'board specific way'.  To means that means we either create it on
the fly or inherit it.  In the case you want to fixup the DTB provided by
OF_SEPARATE, there are _weak platform functions to patch that up. 
You don't really need OF_BOARD for that, you can do that fine with OF_SEPARATE
and check fdtdec_board_setup().

> 
> >
> > >
> > > >
> > > > And it's not helpful.  It is ALWAYS the case that we know that we want
> > > > to override the run time device tree with our own, because it's a
> > > > developer developing things or it's a user / production case where we
> > > > must use the provided tree.  NOT doing that is what leads to madness
> > > > like we see for example on Pi where if we don't use the passed tree we
> > > > still need to copy X/Y/Z out of it.
> > >
> > > Aren't you talking about the distro DT there, rather than the the one
> > > on the boot disk? That is my reading of that patch. If we need to do
> > > that sort of thing, it doesn't matter where the the cointrol DT comes
> > > from. You are still going to have to do that sort of thing.
> > >
> > > It is not ALWAYS the case. I've shown you how easy it is to disable
> > > OF_BOARD and still boot / iterate.
> > >
> > > >
> > > > > > > Are you looking to have an empty DT in u-boot.bin? Perhaps we should
> > > > > > > provide a way to do that? But what is driving that desire?
> > > > > >
> > > > > > I'm looking for ways to convince you that we do not need to include a
> > > > > > device tree in the binary.  There's a growing set of devices where the
> > > > > > device tree exists with the device.  If it's missing, that's a huge
> > > > > > fatal error we can't do all that much about.  If we need to do something
> > > > > > to that device tree for U-Boot, yes, fine, we should make it straight
> > > > > > forward for the developer to do that.  But that's not the common case!
> > > > >
> > > > > Well we could add another Kconfig which tells U-Boot not to include a
> > > > > devicetree in u-boot.bin, if that would resolve this?
> > > > >
> > > > > I just want to make sure that we always build the devicetrees and that
> > > > > it is easy for a knowledgeable dev to switch over to use them, without
> > > > > spelunking through dozens of other projects to discover the secret DT
> > > > > that no one will tell us about.
> >
> > So the translation of this for me is:
> > We have 2 discrete options (that can be cleaned up further) which choose
> > either to embed or receive the DTB.  Let's tangle them and introduce a
> > *new* third option to separate them if someone needs to.  Which makes no
> > sense to me.
> 
> Embed is confusing because OF_EMBED means to link it into U-Boot (just
> used for debugging)
> 

Which as I said, it's what development should use?

> I think this is the core of the problem. There are just lots of ideas
> about how all of this should work. Please try out the series and see
> if you find any problems. Then we can talk about actual issues rather
> than things that might happen.

I am pretty sure the patches work. I never doubted that.  What I don't love
is the architecture and the way it entangles the existing options.

> 
> The two options you refer to are OF_SEPARATE and OF_BOARD. We will
> perhaps have OF_PASSAGE at some point. We already have OF_EMBED.
> 
> To me, OF_BOARD and OF_PASSAGE are run-time things because they
> indicate what we expect to happen at runtime. If something goes wrong,
> we might still be able to print an error message, if we have a backup
> DT.

I don't see a point saving a developer from a mistake.  I'd much rather
crash as loudly as possible to indicate "HEY YOU MESSED UP", instead of 
hiding a problem he might miss under the mat.

Maybe I am missing something, but OF_PASSAGE seems superfluous to me.
OF_SEPARATE and OF_BOARD cover all the cases I can think of.

> 
> But OF_BOARD and OF_PASSAGE are not related to the build itself. IMO
> OF_BOARD is underspecified. One day I would like to move towards
> defining these cases better, e.g.
> 
> - DT is passed in a register (rpi and apple_m1, stm32mp, RISC-V, qemu
> RISC-V, bcmstb, Octeon, Xen)
> - DT is at a fixed address (qemu, highbank, socrates, qemu ppc)
> - DT is in a file (sandbox)

Again I might be missing something but to me:
1. OF_SEPARATE = provided by u-boot.  If someone wants to fix that up we
got a number of ways already.
2. OF_BOARD providing with magic somehow at runtime (prior loader,
constructed at runtime, read from a flash, whatever). 
3. OF_EMBED whatever special developer case we want to use it for.

> 
> IMO many of these could all be handled with standard passage, i.e. in
> a standard way.
> 

[...]

> 
> Regards,
> Simon

[1] https://lore.kernel.org/u-boot/20211204180318.GV1220664@bill-the-cat/T/#m543a10a4b558ccd540fa425d61106ea515393105

Regards
/Ilias