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

[François Ozog]

Hi  Simon,Tom

Le jeu. 2 déc. 2021 à 19:34, Tom Rini <trini at konsulko.com> a écrit :

> 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.
>
> > - 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.
>
> > - U-Boot should not have DTs in-tree
>
> ... for the cases where the DTs are not used at run time, yes.
>
> > - U-Boot should have DTs only when essential
>
> I don't understand this point.  Can you please elaborate?
>
> > - U-Boot should have DTs in-tree for all boards
>
> This is the line you're pushing and almost every other reviewer
> disagrees with.

For the sake of getting interesting parts of the patch set in and “move
on”, what about:
 providing an empty (see below) DT for boards for which U-Boot receives the
“source of truth” so that there are no build issues.
The empty DT contains a comment that explains the DT lifecycle for the
board and points to the documentation tree for sample DTs.
The boards would the ones we talked about and SystemReady boards
https://www.arm.com/why-arm/architecture/systems/systemready-certification-program/ir?_ga=2.35159527.79382615.1638484482-159131740.1638484482


>
> > 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.
> - 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.
>
> 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.
>
> > 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?
>
> > 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..
>
> > > > > 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.
>
> > > 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.
>
> > > > > 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.
>
> --
> Tom
>
-- 
François-Frédéric Ozog | *Director Business Development*
T: +33.67221.6485
francois.ozog at linaro.org | Skype: ffozog