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

[Simon Glass]

Hi Tom,

On Thu, 2 Dec 2021 at 15:47, Tom Rini <trini at konsulko.com> wrote:
>
> On Thu, Dec 02, 2021 at 12:12:16PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 2 Dec 2021 at 11:34, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Thu, Dec 02, 2021 at 11:17:38AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 2 Dec 2021 at 11:03, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Thu, Dec 02, 2021 at 10:07:13AM -0700, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Thu, 2 Dec 2021 at 09:59, Tom Rini <trini at konsulko.com> wrote:
> > > > > > >
> > > > > > > On Thu, Dec 02, 2021 at 09:49:51AM -0700, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Thu, 2 Dec 2021 at 09:38, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Dec 02, 2021 at 05:33:53PM +0100, François Ozog wrote:
> > > > > > > > > > Hi Simon
> > > > > > > > > >
> > > > > > > > > > Le jeu. 2 déc. 2021 à 17:00, Simon Glass <sjg at chromium.org> a écrit :
> > > > > > > > > >
> > > > > > > > > > > With Ilias' efforts we have dropped OF_PRIOR_STAGE and OF_HOSTFILE so
> > > > > > > > > > > there are only three ways to obtain a devicetree:
> > > > > > > > > > >
> > > > > > > > > > >    - OF_SEPARATE - the normal way, where the devicetree is built and
> > > > > > > > > > >       appended to U-Boot
> > > > > > > > > > >    - OF_EMBED - for development purposes, the devicetree is embedded in
> > > > > > > > > > >       the ELF file (also used for EFI)
> > > > > > > > > > >    - OF_BOARD - the board figures it out on its own
> > > > > > > > > > >
> > > > > > > > > > > The last one is currently set up so that no devicetree is needed at all
> > > > > > > > > > > in the U-Boot tree. Most boards do provide one, but some don't. Some
> > > > > > > > > > > don't even provide instructions on how to boot on the board.
> > > > > > > > > > >
> > > > > > > > > > > The problems with this approach are documented in another patch in this
> > > > > > > > > > > series: "doc: Add documentation about devicetree usage"
> > > > > > > > > > >
> > > > > > > > > > > In practice, OF_BOARD is not really distinct from OF_SEPARATE. Any board
> > > > > > > > > > > can obtain its devicetree at runtime, even it is has a devicetree built
> > > > > > > > > > > in U-Boot. This is because U-Boot may be a second-stage bootloader and its
> > > > > > > > > > > caller may have a better idea about the hardware available in the machine.
> > > > > > > > > > > This is the case with a few QEMU boards, for example.
> > > > > > > > > > >
> > > > > > > > > > > So it makes no sense to have OF_BOARD as a 'choice'. It should be an
> > > > > > > > > > > option, available with either OF_SEPARATE or OF_EMBED.
> > > > > > > > > > >
> > > > > > > > > > > This series makes this change, adding various missing devicetree files
> > > > > > > > > > > (and placeholders) to make the build work.
> > > > > > > > > > >
> > > > > > > > > > > Note: If board maintainers are able to add their own patch to add the
> > > > > > > > > > > files, some patches in this series can be dropped.
> > > > > > > > > > >
> > > > > > > > > > > It also provides a few qemu clean-ups discovered along the way. The
> > > > > > > > > > > qemu-riscv64_spl problem is fixed.
> > > > > > > > > > >
> > > > > > > > > > > [1]
> > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20210919215111.3830278-3-sjg@chromium.org/
> > > > > > > > > > >
> > > > > > > > > > > Changes in v6:
> > > > > > > > > > > - Fix description of OF_BOARD so it refers just to the current state
> > > > > > > > > > > - Explain that the 'two devicetrees' refers to two *control* devicetrees
> > > > > > > > > > > - Expand the commit message based on comments
> > > > > > > > > > > - Expand the commit message based on comments
> > > > > > > > > >
> > > > > > > > > > You haven’t addressed any concerns expressed on the mailing list.so I am
> > > > > > > > > > not in favor of this new version either.
> > > > > > > > > > If you make a version without « fake DTs » as you name them, there are good
> > > > > > > > > > advances in the documentation and other areas that would be better in
> > > > > > > > > > mainline….
> > > > > > > > > > If I am the only one thinking this way and the patch can be accepted, I
> > > > > > > > > > would love there is a warning in capital letters at the top of the DTS fake
> > > > > > > > > > files that explains the intent of this fake DT, the possible outcomes of
> > > > > > > > > > not using the one provided by the platform and the right way of dealing
> > > > > > > > > > with DTs for the platform.
> > > > > > > > >
> > > > > > > > > This is the part that I too am still unhappy about.  I do not want
> > > > > > > > > reference or fake or whatever device trees in the U-Boot source tree.
> > > > > > > > > We should be able to _remove_ the ones we have, that are not required,
> > > > > > > > > with doc/board/...rst explaining how to get / view one.  Not adding
> > > > > > > > > more.
> > > > > > > >
> > > > > > > > I understand you don't like it and that others don't as well. I wish
> > > > > > > > it had not come to this.
> > > > > > > >
> > > > > > > > However we are only talking about 10 boards, three of which don't even
> > > > > > > > have a devicetree anywhere I can find.
> > > > > > > >
> > > > > > > > I think on balance this is a substantial clean-up. I am happy to add
> > > > > > > > whatever caveats and documentation people want to clarify what is
> > > > > > > > going on here. I'm happy to look at future options where the
> > > > > > > > devicetree is hosted elsewhere, so long as it is trivial to build it
> > > > > > > > within U-Boot for development purposes.
> > > > > > > >
> > > > > > > > I'll also note that the bootstd series shows the devicetree source:
> > > > > > > >
> > > > > > > > Core:  246 devices, 88 uclasses, devicetree: board
> > > > > > > >
> > > > > > > > But for now, I still feel this is the best path forward.
> > > > > > >
> > > > > > > I'm not sure how to proceed here.  The reviews are rather strongly
> > > > > > > against the "include a device tree that won't be used".  The use case of
> > > > > > > "but for development someone might need to modify the device tree" is
> > > > > > > handled by platforms documenting where / how to get the real one.  We
> > > > > > > should even update the Kconfig help to note that if you enable this your
> > > > > > > board docs MUST explain where the device tree can be seen (or have some
> > > > > > > legal reason you think it's OK to not...).
> > > > > >
> > > > > > Right, we can do lots of things as we have discussed. I am very
> > > > > > willing to work on these and make sure it is hard to do the thing. But
> > > > > > this series is long enough already.
> > > > >
> > > > > Yes, I think the rest of us had hoped you would come around to all of
> > > > > our reasoning by this point, is why this is taking so long.
> > > > >
> > > >
> > > > Look, if I thought this was all wrong I would not be doing it. We have
> > > > a range of opinions:
> > >
> > > And the rest of us wouldn't keep trying to argue otherwise if we didn't
> > > see problems with it, still.
> >
> > I don't claim it is perfect, just that it is better than what we have.
> > We have lots of ideas on how to improve things, e.g.
> >
> > - upstreaming the bindings
> > - having a way to obtain the correct DT through a script
> > - syncing the DT automatically with an external source
> > - applying validation to the in-tree DT
> > - using binman to make it easier to build images
> >
> > That will take a lot of effort and I don't see anyone else taking it
> > on. All we have is a lot of people with competing ideas, many
> > influenced by other projects (TF-A, QEMU, Xen, m1n1, etc.). We could
> > ask the coreboot people what they think, except that they banned
> > devicetree years ago. Sometimes I wish...
>
> Maybe part of the problem here is disagreement on the problems with what
> we have.  The problems I see are:
> - We have some dts files out of sync with upstream, but this ISN'T a
>   problem for them as the dtb in U-Boot never leaves the U-Boot world.
>   It could, but it doesn't, so no one notices, and no one works to
>   update it.  This is typically 32bit ARM.
> - We have some dts files out of sync with upstream, AND this IS a
>   problem for them as the dtb can be passed along to the OS, typically
>   via bootefi and so 64bit ARM platforms.
> - We have a number of u-boot prefixed properties and they need to be
>   both upstreamed and perhaps re-evaluated because it's sometimes very
>   much not clear when / why people need them, as can be seen by some of
>   the copy/paste that's in the -u-boot.dtsi files now, including the
>   typod ones we talked about on IRC.
> - We're very very inconsistent with the case of "a valid device tree for
>   the OS and/or us exists in memory".
>
> It's this last case that's causing all of the headaches in this series
> because on the one hand, there's only a few platforms here, today.  On
> the other hand, there's a very very strong push for 64bit ARM platforms
> to all join in this group because given the nature of the architecture,
> some other firmware runs before us and in theory could / should be
> responsible.

I don't think it will matter much, though, in the end. If what you say
comes to pass then we will have a consistent DT everywhere and it
won't matter where it comes from.

After all, in the end we need to produce a single firmware image. If
binman is used for that, the DT will be placed there.

For development, we may have a situation where all the other bits are
blobs and just U-Boot is being iterated, to get the higher-level
features working properly. We need a way to update the DT easily, e.g.
just building it with U-Boot. That is something I do a *lot* and that
is what I'm not giving up. Not until it is clearer how things are
going to land over the next few years.

>
> > > > - U-Boot should not have its own nodes/properties
> > >
> > > The caveat there is that aren't documented upstream bindings.  I think
> > > at this point the lack of screaming and otherwise "wait, no no no
> > > don't!" that your current patch has gotten means it's time for a pull
> > > request, and for that to go in, and so this line of argument would be
> > > simply removed.
> >
> > That is going to be a long process. I have >100 patches pending and
> > only so much energy. We need to make incremental progress and we need
> > a development setup that works for U-Boot developers.
>
> Right, it will take a bit of time.  But it's also the case that you're
> the person that most deeply understands a good number of them.
>
> > > > - U-Boot should not have DTs in-tree
> > >
> > > ... for the cases where the DTs are not used at run time, yes.
> >
> > Except when they are. E.g. for rpi3 I do actually use the in-tree .dtb
> > for development. I don't what that to be a hassle. U-Boot is a
> > development project and it needs to be easy.
>
> But it's also a user platform.  And it's causing us problems.  This is
> the thread I was referring to earlier:
> https://lore.kernel.org/u-boot/20211125194223.1094066-1-sjoerd@collabora.com/
> and it's not the first, or possibly even only outstanding problem right
> now.

That is dealing with fixing up a loaded DT, as I understand it. We
have that problem anyway when we use a loaded DT to pass to Linux. If
so, it isn't really related to what we are talking about here.

>
> I very much get that part of the overall device tree development cycle
> is painful.  One thought I had today was that one issue here is that you
> see a problem of "I have 10 boards and each board wants me to supply an
> updated device tree in its own way, let me unify that".  Others of us
> (at least myself) see a problem of "10 vendors have officially declared
> you update the device tree for their platform like $this, which is
> unfortunate but pretty well set in stone, with things well outside our
> control depending on this".
>
> So, to me, the root cause problem with Pi support is that we have rpi_4
> and rpi_3 and so forth defconfigs when they should all be supported via
> rpi_arm64 which uses the device tree passed from the firmware blob and
> ready for Linux to consume.  And if you need to modify the dtb, you do
> it the supported way via config.txt.  Which means that yes, we're back
> at my point about 10 vendors and so 10 ways to do the same concept.
> Which is a huge sink when you're working with 10 different platforms.

OK.

>
> > > > - U-Boot should have DTs only when essential
> > >
> > > I don't understand this point.  Can you please elaborate?
> >
> > That it should only be in U-Boot if there is no way it can be anywhere else.
>
> Because of say u-boot,dm-pre-reloc properties, or do you mean the whole
> device tree itself?

No I mean this is a position people are taking. I think it
destructive. U-Boot should be able to have the DT, just like any other
project. They should all be identical (one day), but there is no
reason to strip things out of U-Boot just because some other random
project decides it wants to pass one along at runtime.

>
> > > > - U-Boot should have DTs in-tree for all boards
> > >
> > > This is the line you're pushing and almost every other reviewer
> > > disagrees with.
> >
> > Yes.
> >
> > >
> > > > What's the downside here anyway?
> > >
> > > - A lack of clarity.  We have dts files, you modify those dts files,
> > >   they aren't used.  What's the point?  Oh, you forgot to tweak
> > >   something else.  Wait, now nothing works.  Oh, it's a mismatch between
> > >   what this dts was at one point, and what it needs to be now to
> > >   actually work.
> >
> > That's not my experience with this series. It seems to work fine.
> > Ilias and Heinrich have both pointed to things that 'don't work', but
> > actually I believe things work just as they do now.
>
> I'm talking about for average developers.  I just answered a question on
> StackOverflow earlier this week about what all the device trees that are
> everywhere are even used for, and what needs to be modified to have a
> change actually show up.  The DTS situation in general has a very steep
> learning curve and this will make things harder I believe.

OK so I have added:
- a way to detect where the DT comes from
- showing that on the console
- OF_BOARD Is still the default for boards that use it

I have offered later:
- EXPERT mode to override turning OF_BOARD for boards where it should be enabled
- build-time warnings
- a build-time environment var/flag to enable this

What more do you want? :-) Honestly, let's just go with it.

>
> > > - We're adding more ongoing sync-up work.  While I loudly applaud the
> > >   custodians that are keeping their dts files in sync very regularly,
> > >   and I sympathize with the custodians that want to do it more, but are
> > >   unable to find the time, I do not want to add more of this work.  Even
> > >   more so when it's unclear who would be doing it.  Or what the use is.
> >
> > Well we can make this better, but it is a small number of boards. It
> > is in the noise.
>
> I'm basing my concern here on the number of SoCs that regularly resync
> vs the ones that do not.

This is not going to be solved this week or next. But we have a plan
to resolve it and the current state actually makes all this harder.

>
> > > There's probably more if I think about it harder, but those are the
> > > first to spring to mind.
> > >
> > > > > > It is more than just development. A devicetree is needed for binman to
> > > > > > work, even if it is empty. The documentation idea doesn't really work,
> > > > > > as I think I have proven by the difficulty in getting this series
> > > > > > together. An automated mechanism that runs in CI might be acceptable,
> > > > > > but that is in the future. For now, I believe it just HAS to be
> > > > > > in-tree.
> > > > >
> > > > > I still don't see any reason why we need these incorrect and not
> > > > > functionally used device trees in-tree when a dummy invalid tree is
> > > > > enough to make things link.  We're dealing with real "we must have X.bin
> > > > > in the output for things to function" issues on other platforms with
> > > > > binman right now.  Using a dummy dts should be fine.
> > > >
> > > > Incorrect in what way?
> > >
> > > Well, in the QEMU instance, they're only as correct as the parameters
> > > passed to qemu-system-foo when you did -dumpdtb to start with.  Lets
> > > take TPM as that now should show up in the device tree, or not,
> > > depending on if we have the backend side of it?  Or all of the examples
> > > of how to arbitrarily configure a system as Heinrich noted.
> > >
> > > Or the Pi examples where we need to use the device tree passed to us
> > > because config.txt is the official way to modify things in the device
> > > tree on that platform.
> >
> > OK, so when you add extra parameters to the ARM and RISC-V QEMU
> > command line, it doesn't add those extra parameters and those extra
> > devices are not present.That does not sound surprising to me. It
> > should be obvious to anyone doing development on these boards.
>
> Why is it going to be obvious that we're ignoring the supplied device
> tree?  Why is it expected that U-Boot works like this, when nothing else
> does?

Because U-Boot will print that out on boot:

   U-Boot 2021.10-00190 (Oct 30 2021 - 09:01:29 -0600)

   DRAM:  128 MiB
>  Core:  42 devices, 11 uclasses, devicetree: board   [or passage, or separate]
   Flash: 64 MiB

https://patchwork.ozlabs.org/project/uboot/patch/20211101011734.1614781-23-sjg@chromium.org/

>
> > > > How do I get a real one for development? How do I turn off OF_BOARD
> > > > and use the in-tree one?
> > >
> > > How do you turn off the run-time device tree and instead use the in-tree
> > > one for development, with your series?
> >
> > Disable OF_BOARD manually. As we discussed, we can put that behind an
> > EXPERT flag or a build flag, or other things. There is a message
> > printed when U-Boot starts, too (with a later series).
>
> OK, so what I'm saying is that for rpi_arm64, qemu_arm*, etc, etc, the
> default builds that dummy empty device tree, and when you want to do
> your developer use case you disable OF_BOARD and set it to point at what
> you want.

Where does that 'what you want' come from?

>
> > > > The documentation approach is not good enough.
> > >
> > > Why?  But maybe I can better explain things in Mark's part of the thread
> > > about power domains and serial on M1..
> >
> > Because I can't find the DT and it takes manual effort to locate it
> > and set it up for development. It is just too hard.
>
> You're optimizing things for the "I'm developing on 10 arbitrary
> different SoCs" which isn't the common use case.  Mark for example
> doesn't have a problem figuring out where the M1 dts files live, no one
> working on Pi has problems finding the Pi dts files, the Xen folks know
> what's up in their case, and so on.  Yes, the developing broadly use
> case is hard and needs better documentation so that it's not too hard to
> come up to speed on a new platform.  That's true for everyone.

It's not remotely feasible for me, at least. I'm pleased that it works
for you, but a bit surprised.

I don't want to give up the easy building that we have now, until we
have something just as easy in place.

>
> > > > > > > And yes, we're "only" talking about 10 platforms, which include things
> > > > > > > like the "everyone" has one Pi family, the extraordinarily flexible (and
> > > > > > > so easy for the reference device tree to be very wrong) QEMU families
> > > > > > > and then platforms that are including a dts in-tree now because they
> > > > > > > were told that was required.
> > > > > >
> > > > > > Actually it is only rpi4 that doesn't have an in-tree DT. There is
> > > > > > actually no reason for it not to, e.g. Linux has it. Why not U-Boot?
> > > > > > The argument is the same.
> > > > >
> > > > > Because we don't need it!  It serves no purpose!  It exists in Linux
> > > > > as that's the primary device tree source repository.  We could _copy_ it
> > > > > in, if it was useful.  But then we need to re-sync it every so often,
> > > > > and for less clear reasons than all of the platforms that we need to
> > > > > sync with the kernel for, AND we use the tree.
> > > >
> > > > So some people don't need it and it serves no purpose for them. But
> > > > why do they care? It is not hurting anyone. This is all overblown.
> > >
> > > Because it's adding ongoing maintenance work, and reducing clarity of
> > > the codebase, to summarize my concerns above.
> >
> > Yes, but can we just accept that as a cost? We have plans and ideas to
> > reduce it over time, right?
>
> In that there's lessons to learn from other projects about source layout
> so it's easier to re-sync files, sure, there's ideas.  But it also
> continues to just ignore / set aside the problem of how frequently how
> much of device trees change.  It's getting better over time and
> validation being non-optional in Linux moving forward will really help.
> But it's still work, and still makes things unclear where / why we're
> doing what we would be doing.  If we're given a perfectly functional and
> correct device tree in memory for a Pi for example, why do we not use
> it by default and instead pick one that was built in to U-Boot and
> misses the changes the user may have made on purpose.  That is part of
> clarity as well.

But I'm not saying we should not use it by default. This is what I
find so confusing, that people are imputing some strange change here.
I just want to be able to (as a dev) use the in-tree DT, if I want to,
if I know what I am doing.

When we have a better system for getting the DT for development
purposes, let's use it. But we don't.

Again, this series changes *nothing* about how rpi works by default.

>
> > > > > There's even an argument to be made that it IS in Linux because when you
> > > > > build that dtb, it's what the firmware then ships and uses and provides
> > > > > to everyone at run time, possibly along with whatever other
> > > > > modifications the binary firmware did (see the assorted threads,
> > > > > including one this week about the problems we have because we don't just
> > > > > always use the dtb provided to us at run time).
> > > > >
> > > > > > Most QEMU boards have an in-tree devicetree. It is only ARM (now
> > > > > > copied by RISC-V) which doesn't.
> > > > >
> > > > > Yes, these are more examples of "someone said we need to copy it in, so
> > > > > we copy it in".
> > > >
> > > > No that's not correct. With x86, ppc, integrator, ast2500 and many
> > > > others we *need* the DT and *it is not* created by QEMU.
> > >
> > > You're listing both QEMU virtual machines and QEMU-as-hardware-emulators
> > > above, which confuses things.  The integrator code runs on real
> > > hardware.  Real hardware which does not have the device tree on it.  I
> > > assume that QEMU + Linux - U-Boot for integrator requires you to pass
> > > the .dtb in, just like real the real hardware does.  That's true of all
> > > of the cases I believe where we use QEMU-as-hardware-emulator because
> > > QEMU is also authentic to the hardware.
> >
> > Yes, I get it. But the common case just works and I want to use binman
> > images in CI as well as development with these things, without hacking
> > QEMU itself. It is just not sensible to have to upgrade QEMU just to
> > get an updated DT for trying something out.
>
> I don't follow you.  There's nothing special about the integrator build
> in CI, which is the whole point.
>
> And then I keep saying that if you're modifying the dtb from QEMU, and
> it's not just a proof of concept or something being developed and
> upstreamed, something is wrong with our flow.  We have something
> backwards and need to evaluate it.

Why? There are going to be dozens of different pre-boot loaders,
right? Everyone loves writing a new pre-boot loader. How else are you
going to deal with whatever weird way each one of them produces a DT
so you can modify it for development purposes.

Do you want to have to rebuild that pre-boot loader from source every
time you want to make a change?

Inserting it with U-Boot is the only sane way to develop U-Boot. Mark
has found that. I know it for a fact.

So we have to make it *impossible* to insert a DT that U-Boot actually
uses. Why?

>
> > > > > > > How about adjusting the make logic so that if a tree isn't found, we use
> > > > > > > a dummy minimal valid dts file?
> > > > > >
> > > > > > This is what I have done for the boards where I could not figure out
> > > > > > how to get any sort of DT, yes. But I don't think that should be the
> > > > > > default.
> > > > >
> > > > > The more I think about this, the more I think dummy minimal valid dts
> > > > > should be the fall-back default.  This then solves the "I'm a developer,
> > > > > I need to modify the dts files" case because you then just provide the
> > > > > dts instead where it should go, and it's used.
> > > >
> > > > How does it solve it? I don't even know how to get it in many cases.
> > > > If it is a dummy then I cannot actually use it for development, right?
> > >
> > > It solves the problem of "we must have this dts file so that the build
> > > will not fail".  The next step of "oh, we actually want to use it, and
> > > not the run-time provided dtb" would be the same as whatever you're
> > > doing with this series.
> >
> > Not in my experience. For example, with QEMU I was able to use an
> > in-tree .dts for the bootstd stuff without any issues. This series
> > also explains how to discover the DT, at least for the two QEMU boards
> > that need it.
>
> Then something is backwards still.  You should be able to flip the
> CONFIG switch to have the arbitrary dtb you want used, but the default
> should be the provided device tree.

Yes, agreed. Have I not been saying that from the start? What are we
still discussing?

I cannot reconcile that statement with what you have said above. Email
is such a nightmare.

Regards,
Simon