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

[Tom Rini]

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.

> > > - 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.

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.

> > > - 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?

> > > - 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.

> > - 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.

> > 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?

> > > 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.

> > > 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.

> > > > > > 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.

> > > > 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.

> > > > > > 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20211202/53661e52/attachment.sig>