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

[Simon Glass]

Hi Mark,

On Thu, 2 Dec 2021 at 12:22, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > From: Simon Glass <sjg at chromium.org>
> > Date: Thu, 2 Dec 2021 11:53:53 -0700
> >
> > Hi Mark,
> >
> > On Thu, 2 Dec 2021 at 11:47, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> > >
> > > > From: Simon Glass <sjg at chromium.org>
> > > > Date: Thu, 2 Dec 2021 11:21:22 -0700
> > > >
> > > > Hi Mark,
> > > >
> > > > On Thu, 2 Dec 2021 at 11:15, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> > > > >
> > > > > > From: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > > > > Date: Thu, 2 Dec 2021 19:03:46 +0200
> > > > > >
> > > > > > On Thu, 2 Dec 2021 at 18: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.
> > > > > >
> > > > > > So this is a key point for me and the reason I completely disagree
> > > > > > with this approach.  This proposal is working in the *exact* opposite
> > > > > > direction and we'll never be able to get rid of device trees from
> > > > > > U-Boot, even if at some point they move out of the kernel to a
> > > > > > 'common' repo'.  I'll just repeat what I've been saying since v1.
> > > > > > Personally I'd be way happier if we could figure out were the specific
> > > > > > U-Boot config nodes are needed and when are they needed.  Based on
> > > > > > what we figure out we could, pick up the device tree from a previous
> > > > > > state bootloader and fix it up with our special nodes before we start
> > > > > > using it, using internal DTS files (compiled to .dtbos or similar)
> > > > > > that indeed belong in the u-boot tree.
> > > > >
> > > > > I don't think it makes sense to put stuff in the DT that is specific
> > > > > for U-Boot only to pull it out moments later.  Maybe it does make some
> > > > > sense to do this to pass information between TPL/SPL and U-Boot
> > > > > proper.  But otherwise you can just use global variables...
> > > > >
> > > > > Now I just ran into an issue on Apple M1 that may have some relevance
> > > > > here.  I'm adding support for power domains and the serial port
> > > > > requires certain power domains to be on.  Since the serial port is
> > > > > initialized in the pre-relocation phase this means that the device
> > > > > tree nodes for the power domain controllers need to have the
> > > > > "u-boot,dm-pre-reloc" property on them.  Otherwise the DM code won't
> > > > > be able to bind the power domain controller driver in this phase and
> > > > > binding the serial port driver itself will fail.  Which makes U-Boot
> > > > > hang without any visible output on the serial console.
> > > > >
> > > > > Within the Asahi Linux group we're currently discussing how to solve
> > > > > this.  We could just add the "u-boot,dm-pre-reloc" properties in the
> > > > > device trees that we're going to distribute as part of m1n1 (the
> > > > > "bootloader" than embeds U-Boot).  Or we can write some code that adds
> > > > > those properties to the device tree nodes that are dependencies for
> > > > > the serial port.
> > > > >
> > > > > I don't think the suggestion of applying an overlay embedded in U-Boot
> > > > > would work here.  The code applying the overlay would need to run very
> > > > > early on in the pre-relocation phase.  We'd also have to include
> > > > > overlays for all the models that Apple offers and pick the right one.
> > > > > And if a new model appears we can no longer just add a new device tree
> > > > > to m1n1.
> > > >
> > > > Well put.
> > > >
> > > > >
> > > > > But maybe there is a case where the overlay approach would make sense...
> > > >
> > > > There might be, but I haven't found it yet.
> > > >
> > > > BTW I suggest we figure out how to upstream the binding for this. I
> > > > will see if I can send a patch to start the process. The last patch
> > > > didn't get any comments though.
> > > >
> > > > In the interim there is probably no choice but to add the properties
> > > > into the m1n1 project. Of course, for development, you could just turn
> > > > off OF_BOARD, with my series applied. Which is a lot of the point of
> > > > all of this discussion...
> > >
> > > And that is exactly why I don't see the point of this series.  There
> > > is no chance of that working.  The device tree that is currently in
> > > the U-Boot tree (essentially because you made me add one) is only a
> > > template that gets modified by m1n1 to account for hardware
> > > characteristics (amount of memory, cores that are actually enabled,
> > > MAC addresses, WiFi antenna configuration, etc. etc.).  If I flip
> > > OF_BOARD U-Boot would simply not work.
> > >
> > > The way I do development is that I simply build a device tree, build
> > > U-boot and upload them over serial to m1n1 running on the device.  I
> > > currently do build my device trees out of the U-Boot tree, but the
> > > main reason for that is because the Linux development model means that
> > > too many of the device tree patches are still in flight.  I often use
> > > the device tree from a different U-Boot repository clone than the one
> > > I do the actual U-Boot development in anyway.
> >
> > Yes and I think you have made my point. That is all I am saying. I am
> > not claiming that U-Boot needs to be the golden repo for the .dts,
> > just that it should have something suitable that works well enough for
> > development, and can be modified and updated as development
> > progresses.
>
> No you are missing my point.  I'm only doing it this way because the
> Linux development model is so broken that it takes months for patches
> posted on the mailing list to end up in a somewhat official source
> tree and I would go bat-shit crazy having to deal with all the merges
> and rebasing going on.
>
> If there was a separate device tree project, I would very much prefer
> to use that and make device tree changes there such that I could
> submit any additions to the canonical sources.  I'm currently spending
> too much of my development time synching the device trees in my U-Boot
> tree with what's going on on the Linux side.

Right, exactly. I have exactly the same problem and so do other people
working with U-Boot. At least we have a work-around.

Regards,
Simon