Re: [PATCH 1/1 RFC] treewide: Deprecate OF_PRIOR_STAGE

[Thomas Fitzsimmons <fitzsim@fitzsim.org>]

Hi Tom,

Tom Rini <trini@konsulko.com> writes:

> On Wed, Oct 13, 2021 at 01:36:00PM -0400, Thomas Fitzsimmons wrote:
>> Simon Glass <sjg@chromium.org> writes:
>> 
>> [...]
>> 
>> > On Wed, 13 Oct 2021 at 10:26, Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote:
>> >>
>> >> Simon Glass <sjg@chromium.org> writes:
>> >>
>> >> [...]
>> >>
>> >> >> > I think one option is better than two. I have a slight preference for
>> >> >> > OF_PRIOR_STAGE because it is board-agnostic, but I'm not sure it
>> >> >> > matters, since some of these boards are doing strange things anyway
>> >> >> > and cannot use OF_PRIOR_STAGE. So let's go with this.
>> >> >>
>> >> >> For now it's easier getting rid of OF_PRIOR_STAGE than OF_BOARD.
>> >> >> Once we unify OF_PRIOR_STAGE/OF_BOARD and OF_HOSTFILE, then
>> >> >> I can send a patch on top of that, which removes the board_fdt_blob_setup()
>> >> >> and just stores the address in a similar fashion to the removed
>> >> >> 'prior_stage_fdt_address'.  That way we can get rid of architecture
>> >> >> specific constructs wrt to DT in gd.  The callback is a bit more of a pain to
>> >> >> maintain for multiple boards but is more flexible than an address in a
>> >> >> register.  In any case we can do something along the lines of:
>> >> >>
>> >> >> Check register (or blob list or whatever)
>> >> >> if (valid dtb)
>> >> >>     fixup/amend/use (depending on what we decide)
>> >> >> else
>> >> >>    arch specific callback
>> >> >>
>> >> >> That should give us enough flexibility to deal with future boards (famous
>> >> >> last words).
>> >> >
>> >> > SGTM
>> >>
>> >> This sounds like a good generalization that would still work for the
>> >> bcm7445 and bcm7260 boards.  I'll test this approach on the evaluation
>> >> boards I have.
>> >>
>> >> For the BCM7445 I may be able to import the evaluation board device tree
>> >> that Broadcom publishes as part of stblinux.  At runtime I may need to
>> >> merge some of the in-memory items generated by BOLT, but I'll try to
>> >> make this work.
>> >
>> > That would be good.
>> >
>> >> The BCM7260 DTS is not publicly available though, as far as I know.
>> >
>> > Presumably it can be dumped from U-Boot?
>> 
>> Technically, yes, but I wouldn't want to publish the result for various
>> reasons; e.g., it would be specific to the evaluation boards I have, and
>> it may contain vendor-specific fields.  I'd much rather this one remain
>> a stub, until/unless Broadcom publishes a generic BCM7260 DTS under a
>> free license.
>
> Also note that the notion that the U-Boot source tree _must_ contain a
> dts for a given board is something we're very much debating still, in
> another thread, if you're inclined to read and chime in there as well
> with more details about the broadcom use case and technical/legal
> limitations.  Thanks!

Sure.  I read through [1]; here are my thoughts from the BCM7445/BCM7260
perspective.

First of all, for background, BCM7445 and BCM7260 are partial ports of
U-Boot.  They're meant to allow using nice U-Boot features like hush and
FIT image loading on these platforms.  But they do not handle low-level
initialization -- that's done by BOLT, the proprietary
first-and-second-stage bootloader -- and they don't support configuring
all of the hardware on these boards.  Instead these ports include a
small set of drivers (e.g., SPI, eMMC, serial) and configuration that is
needed to make use of the higher level features.

At the time I contributed the BCM7445 support, README called OF_CONTROL
an experimental feature, and device driver configuration was
alternatively allowed to live in board-specific header files.  My
initial local implementation did that, but then I switched to OF_CONTROL
before submitting the patches, since then I could get some of U-Boot's
driver configuration from the prior stage (BOLT) dynamically, instead of
hard-coding addresses in U-Boot source code.  The proposed new policy
would require me to (re-)add these hard-coded values, albeit in a DTS,
not a header file.  IMO that's probably a good/fair requirement for the
non-technical reasons in [1].

The second section of [1] says: "Every board in U-Boot must include a
devicetree sufficient to build and boot that board on suitable hardware
(or emulation)."  I initially read that as "boot to Linux", and so I was
thinking the device tree checked into the U-Boot tree had to be
sufficient to boot Linux and configure every device that Linux supports.
One of Simon's responses [2] clarified for me that the policy proposal
was about the control DTB for U-Boot.

Now I believe the intent of the proposed policy (stated in the
"Devicetree source" section of [1]) is something like "each port in
U-Boot must have an in-tree device tree that is sufficient to boot/run
*U-Boot itself* on at least one representative board designed around
that SoC".  That would make sense to me; it would permit not-full-Linux
device trees that configure only the device drivers that the port needs
to support a subset of U-Boot features.  This would allow boards like
BCM7260, which have no publicly available Linux DTS, to have a small,
generic device tree just for configuring reused, GPL'd U-Boot drivers.
This is in contrast to the policy mandating or encouraging dumping to
DTS binary-only proprietary Linux DTBs from prior stage bootloaders or
ROMs, as a precondition to the port being included in U-Boot.

The policy proposal (assuming I'm understanding it correctly now) would
have been clearer if one of the first two sections in devicetree.rst
explicitly mentioned "control DTB for U-Boot", i.e., the fact that the
policy is about U-Boot's own much simpler DTB usage, not Linux's, even
though the two projects largely share the same DTSs.

Thomas

1. http://patchwork.ozlabs.org/project/uboot/patch/20210919215111.3830278-3-sjg@chromium.org/

2. https://lists.denx.de/pipermail/u-boot/2021-October/463675.html