Re: [PATCH 00/16] fdt: Make OF_BOARD a boolean option

[Simon Glass]

Hi François,

On Thu, 28 Oct 2021 at 10:26, François Ozog <francois.ozog@linaro.org> wrote:
>
> Hi Simon
>
> Le jeu. 28 oct. 2021 à 17:44, Simon Glass <sjg@chromium.org> a écrit :
>>
>> Hi François,
>>
>> On Thu, 28 Oct 2021 at 08:50, François Ozog <francois.ozog@linaro.org> wrote:
>> >
>> > Hi Simon
>> >
>> > Le jeu. 28 oct. 2021 à 16:30, Simon Glass <sjg@chromium.org> a écrit :
>> >>
>> >> Hi François,
>> >>
>> >> On Thu, 28 Oct 2021 at 02:21, François Ozog <francois.ozog@linaro.org> 
>> >> wrote:
>> >> >
>> >> > Hi Simon,
>> >> >
>> >> > Le jeu. 28 oct. 2021 à 04:51, Simon Glass <sjg@chromium.org> a écrit :
>> >> >>
>> >> >> Hi Ilias,
>> >> >>
>> >> >> On Tue, 26 Oct 2021 at 00:46, Ilias Apalodimas
>> >> >> <ilias.apalodimas@linaro.org> wrote:
>> >> >> >
>> >> >> > Hi Simon,
>> >> >> >
>> >> >> > A bit late to the party, sorry!
>> >> >>
>> >> >> (Did you remember the beer? I am replying to this but I don't think it
>> >> >> is all that helpful for me to reply to a lot of things on this thread,
>> >> >> since I would not be adding much to my cover letter and patches)
>> >> >>
>> >> >> >
>> >> >> > [...]
>> >> >> >
>> >> >> > > >
>> >> >> > > > I really want to see what the binary case looks like since we 
>> >> >> > > > could then
>> >> >> > > > kill off rpi_{3,3_b,4}_defconfig and I would need to see if we 
>> >> >> > > > could
>> >> >> > > > then also do a rpi_arm32_defconfig too.
>> >> >> > > >
>> >> >> > > > I want to see less device trees in U-Boot sources, if they can 
>> >> >> > > > come
>> >> >> > > > functionally correct from the hardware/our caller.
>> >> >> > > >
>> >> >> > > > And I'm not seeing how we make use of "U-Boot /config" if we 
>> >> >> > > > also don't
>> >> >> > > > use the device tree from build time at run time, ignoring the 
>> >> >> > > > device
>> >> >> > > > tree provided to us at run time by the caller.
>> >> >> > >
>> >> >> > > Firstly I should say that I find building firmware very messy and
>> >> >> > > confusing these days. Lots of things to build and it's hard to find
>> >> >> > > the instructions. It doesn't have to be that way, but if we carry 
>> >> >> > > on
>> >> >> > > as we are, it will continue to be messy and in five years you will
>> >> >> > > need a Ph.D and a lucky charm to boot on any modern board. My
>> >> >> > > objective here is to simplify things, bringing some consistency to 
>> >> >> > > the
>> >> >> > > different components. Binman was one effort there. I feel that 
>> >> >> > > putting
>> >> >> > > at least the U-Boot house in order, in my role as devicetree
>> >> >> > > maintainer (and as author of devicetree support in U-Boot back in
>> >> >> > > 2011), is the next step.
>> >> >> > >
>> >> >> > > If we set things up correctly and agree on the bindings, devicetree
>> >> >> > > can be the unifying configuration mechanism through the whole of
>> >> >> > > firmware (except for very early bits) and into the OS, this will 
>> >> >> > > set
>> >> >> > > us up very well to deal with the complexity that is coming.
>> >> >> > >
>> >> >> > > Anyway, here are the mental steps that I've gone through over the 
>> >> >> > > past
>> >> >> > > two months:
>> >> >> > >
>> >> >> > > Step 1: At present, some people think U-Boot is not even allowed to
>> >> >> > > have its own nodes/properties in the DT. It is an abuse of the
>> >> >> > > devicetree standard, like the /chosen node but with less history. 
>> >> >> > > We
>> >> >> > > should sacrifice efficiency, expedience and expandability on the 
>> >> >> > > altar
>> >> >> > > of 'devicetree is a hardware description'. How do we get over that
>> >> >> > > one? Wel, I just think we need to accept that U-Boot uses 
>> >> >> > > devicetree
>> >> >> > > for its own purposes, as well as for booting the OS. I am not 
>> >> >> > > saying
>> >> >> > > it always has to have those properties, but with existing features
>> >> >> > > like verified boot, SPL as well as complex firmware images where
>> >> >> > > U-Boot needs to be able to find things in the image, it is 
>> >> >> > > essential.
>> >> >> > > So let's just assume that we need this everywhere, since we 
>> >> >> > > certainly
>> >> >> > > need it in at least some places.
>> >> >> > >
>> >> >> > > (stop reading here if you disagree, because nothing below will make
>> >> >> > > any sense...you can still use U-Boot v2011.06 which doesn't have
>> >> >> > > OF_CONTROL :-)
>> >> >> >
>> >> >> > Having U-Boot keep it's *internal* config state in DTs is fine.  
>> >> >> > Adding
>> >> >> > that to the DTs that are copied over from linux isn't imho.  There 
>> >> >> > are
>> >> >> > various reasons for that.  First of all syncing device trees is a 
>> >> >> > huge pain
>> >> >> > and that's probably one of the main reasons our DTs are out of sync 
>> >> >> > for a
>> >> >> > large number of boards.
>> >> >> > The point is this was fine in 2011 were we had SPL only,  but the 
>> >> >> > reality
>> >> >> > today is completely different.  There's previous stage boot loaders 
>> >> >> > (and
>> >> >> > enough cases were vendors prefer those over SPL).  If that 
>> >> >> > bootloader needs
>> >> >> > to use it's own device tree for whatever reason,  imposing 
>> >> >> > restrictions on
>> >> >> > it wrt to the device tree it has to include,  and require them to 
>> >> >> > have
>> >> >> > knowledge of U-Boot and it's internal config mechanism makes no 
>> >> >> > sense not
>> >> >> > to mention it doesn't scale at all.
>> >> >>
>> >> >> I think the solution here may be the binman image packer. It works
>> >> >> from a description of the image (i.e. is data-driver) and can collect
>> >> >> all the pieces together. The U-Boot properties (and the ones required
>> >> >> by TF-A, etc.) can be added at package time.
>> >> >>
>> >> >> If you think about it, it doesn't matter what properties are in the DT
>> >> >> that is put into the firmware image. TF-A, for example, is presumably
>> >> >> reading a devicetree from flash, so what does it care if it has some
>> >> >> U-Boot properties in it?
>> >> >
>> >> >
>> >> > I am going to change my position in all mail threads I participate.
>> >> > I was trying to make patches relevant in the future and conceptually 
>> >> > clean. That may not be the most effective position: I should just care 
>> >> > about Linaro and its members being able to implement SystemReady 
>> >> > concepts.
>> >> >
>> >> >
>> >> > If you mandate U-Boot has nodes in the device tree passed to the OS, we 
>> >> > can put DT fragment in  the nt_fw_config section of the fip and merge 
>> >> > it at boot time. So there is a solution compatible with SystemReady.
>> >> >
>> >> > If you want to put fake, non future proof, DT sources in the dts for 
>> >> > platforms that are organized to provide the authoritative DT to U-Boot 
>> >> > at runtime, that's kind of your choice (hopefully representing the rest 
>> >> > of U-Boot community). There will be quirk code in U-Boot to redo the 
>> >> > adaptations on its non authoritative DT that the platform previous 
>> >> > stage firmware does (already saw one in the past month); as Mark said 
>> >> > there will be issues over time; and it will confuse people about the 
>> >> > role of the DT. But I am fine with it as it does not impair Linaro and 
>> >> > its members ability to implement SystemReady way of handling DT.
>> >>
>> >> OK thank you. It doesn't sound like you are very on-board though.
>> >> Also, you mischaracterise my intent with in-tree devicetrees.
>> >>
>> >> I would be happy enough for now if you could accept that U-Boot has
>> >> nodes/properties of its own in the devicetree. It has been a feature
>> >> of U-Boot for 10 years now.
>> >
>> > On SystemReady systems the DT passed to U-Boot for the OS will be 
>> > assembled from the board DT and a U-Boot fragment/overlay. The board DT is 
>> > free from any software/firmware aspects, just contains hardware 
>> > description. The U-Boot fragment/overlay can contain any nodes it wants. 
>> > The location of the bindings specification is essentially irrelevant: it 
>> > could be devicetree.org, U-Boot doc or Linux kernel. Both DTs will be 
>> > stored in the FIP. OEMs making their firmware will just put whatever is 
>> > needed in this “dynamic config” DT. On SystemReady platforms U-Boot will 
>> > always be given a DT, like on the RPI4. U-Boot will be able to ignore it 
>> > obviously. That said, doing so, a platform may end-up failing compliance 
>> > tests.
>> > I think we need to document the above in U-Boot and refer to relevant 
>> > specifications. I’ll let Ilias propose something.
>>
>> Hmm. So long as OF_BOARD is enabled, the devicetree will not be 'ignored'.
>>
>> Are you talking here about what TF-A will do? I assume so, since you
>> mention FIP and I believe that is a TF-A invention.
>
> Yes
>>
>>
>> Of course the image is all packaged together in fact, so binman could
>> presumably merge the DTs at build time, if desired.
>
> Practically I don’t think so. The passed device tree will contain all 
> authoritative information such as discovered normal memory (excluded the 
> secure memory ranges), architectural nodes such as PSCI and other nodes 
> coming from TEE-OS or secure partitions such as SCMI interface or firmwareTPM.
> If you combine the two static parts at build time you will have to extract 
> the runtime pieces from the DT passed to U-Boot.

This could be the subject of some future discussion, perhaps, as I
don't think we are talking about the same thing. I am talking about
the DT packaged in the firmware, but I think you are talking about the
things detected at runtime. Binman is for the first problem, not the
second.

Regards,
Simon


>>
>>
>>
>> Regards,
>> Simon
>>
>> >> >>
>> >> >>
>> >> >> As to syncing, we have solved this using u-boot.dtsi files in U-Boot,
>> >> >> so I think this can be dealt with.
>> >> >>
>> >> >> >
>> >> >> > >
>> >> >> > > Step 2: Assume U-Boot has its own nodes/properties. How do they get
>> >> >> > > there? Well, we have u-boot.dtsi files for that (the 2016 patch
>> >> >> > > "6d427c6b1fa binman: Automatically include a U-Boot .dtsi file"), 
>> >> >> > > we
>> >> >> > > have binman definitions, etc. So we need a way to overlay those 
>> >> >> > > things
>> >> >> > > into the DT. We already support this for in-tree DTs, so IMO this 
>> >> >> > > is
>> >> >> > > easy. Just require every board to have an in-tree DT. It helps with
>> >> >> > > discoverability and documentation, anyway. That is this series.
>> >> >> > >
>> >> >> >
>> >> >> > Again, the board might decide for it's own reason to provide it's 
>> >> >> > own DT.
>> >> >> > IMHO U-Boot must be able to cope with that and asking DTs to be 
>> >> >> > included in
>> >> >> > U-Boot source is not the right way to do that,  not to mention cases 
>> >> >> > were
>> >> >> > that's completely unrealistic (e.g QEMU or a board that reads the 
>> >> >> > DTB from
>> >> >> > it's flash).
>> >> >>
>> >> >> I think you are at step 2. See above for my response.
>> >> >>
>> >> >> >
>> >> >> > > (I think most of us are at the beginning of step 2, unsure about it
>> >> >> > > and worried about step 3)
>> >> >> > >
>> >> >> > > Step 3: Ah, but there are flows (i.e. boards that use a particular
>> >> >> > > flow only, or boards that sometimes use a flow) which need the DT 
>> >> >> > > to
>> >> >> > > come from a prior stage. How to handle that? IMO that is only 
>> >> >> > > going to
>> >> >> > > grow as every man and his dog get into the write-a-bootloader
>> >> >> > > business.
>> >> >> >
>> >> >> > And that's exactly why we have to come up with something that 
>> >> >> > scales,  without
>> >> >> > having to add a bunch of unusable DTs in U-Boot.
>> >> >>
>> >> >> In what way does this not scale? How are the DTs unusable? If there is
>> >> >> a standard binding, we should be fine.
>> >> >>
>> >> >> >
>> >> >> > > We need a way to provide the U-Boot nodes/properties in a
>> >> >> > > form that the prior stage can consume and integrate with its build
>> >> >> > > system. Is TF-A the only thing being discussed here? If so, let's 
>> >> >> > > just
>> >> >> > > do it. We have the u-boot.dtsi and we can use binman to put the 
>> >> >> > > image
>> >> >> > > together, for example. Or we can get clever and create some sort of
>> >> >> > > overlay dtb.
>> >> >> > >
>> >> >> > > Step 3a. But I don't want to do that. a) If U-Boot needs this stuff
>> >> >> > > then it will need to build it in and use two devicetrees, one 
>> >> >> > > internal
>> >> >> > > and one from the prior stage....well that is not very efficient 
>> >> >> > > and it
>> >> >> > > is going to be confusing for people to figure out what U-Boot is
>> >> >> > > actually doing. But we actually already do that in a lot of cases
>> >> >> > > where U-Boot passes a DT to the kernel which is different to the 
>> >> >> > > one
>> >> >> > > it uses. So perhaps we have three devicetrees? OMG.
>> >> >> >
>> >> >> > No we don't. That's a moot point. If you separate the DTs U-Boot
>> >> >> > provides the internal one and inherits one 'generic'.  Linux will be 
>> >> >> > able to use
>> >> >> > that.  So the only case were you'll need 3 DTs is if the *vendor* 
>> >> >> > breaks the
>> >> >> > DT across kernel versions,  In which case there's not much you can 
>> >> >> > do to
>> >> >> > begin with and that's already a case we have to deal with.
>> >> >>
>> >> >> Linux actually doesn't care if the U-Boot properties are in the tree,
>> >> >> so long as we have proper bindings. My point here is we only need
>> >> >> either:
>> >> >>
>> >> >> a. one devicetree, shared with Linux and U-Boot (and TF-A?)
>> >> >> b. two devicetrees, one for use in firmware and one for passing to 
>> >> >> Linux
>> >> >>
>> >> >> We don't need to separate out the U-Boot properties into a second (or
>> >> >> third) devicetree. There just isn't any point.
>> >> >>
>> >> >> >
>> >> >> > > b) Well then
>> >> >> > > U-Boot can have its own small devicetree with its bits and then 
>> >> >> > > U-Boot
>> >> >> > > can merge the two when it starts. Again that is not very 
>> >> >> > > efficient. It
>> >> >> > > means that U-Boot cannot be controlled by the prior stage (e.g. to 
>> >> >> > > get
>> >> >> > > its public key from there or to enable/disable the console), so
>> >> >> > > unified firmware config is not possible. It will get very 
>> >> >> > > confusing,
>> >> >> > > particularly for debugging U-Boot. c) Some other scheme to avoid
>> >> >> > > accepting step 3...please stop!
>> >> >> > >
>> >> >> > > Step 4: Yes, but there is QEMU, which makes the devicetree up out 
>> >> >> > > of
>> >> >> > > whole cloth. What about that? Well, we are just going to have to 
>> >> >> > > deal
>> >> >> > > with that. We can easily merge in the U-Boot nodes/properties and
>> >> >> > > update the U-Boot CI scripts to do this, as needed, e.g. with
>> >> >> > > qemu-riscv64_spl. It's only one use case, although Xen might do
>> >> >> > > something similar.
>> >> >> > >
>> >> >> > > To my mind, that deals with both the build-time and run-time 
>> >> >> > > issues.
>> >> >> > > We have a discoverable DT in U-Boot, which should be considered the
>> >> >> > > source of truth for most boards. We can sync it with Linux
>> >> >> > > automatically with the tooling that I hope Rob Herring will come up
>> >> >> > > with. We can use an empty one where there really is no default,
>> >> >> > > although I'd argue that is making perfect an enemy of the good.
>> >> >> > >
>> >> >> > > Step 5: If we get clever and want to remove them from the U-Boot 
>> >> >> > > tree
>> >> >> > > and pick them up from somewhere else, we can do that with 
>> >> >> > > sufficient
>> >> >> > > tooling. Perhaps we should set a timeline for that? A year? Two? 
>> >> >> > > Six?
>> >> >> >
>> >> >> > We can start slowly migrating boards and see how that works out.
>> >> >> > We could either use 2 device trees as you proposed, or have u-boot 
>> >> >> > merge
>> >> >> > the 'u-boot' DTB and the inherited DTB before DM comes up.  OTOH I'd 
>> >> >> > prefer
>> >> >> > if linux gets handed a clean device tree without the u-boot 
>> >> >> > internals in
>> >> >> > it, so I think 2 discrete DTs is cleaner overall.
>> >> >>
>> >> >> I know you would prefer that, but does it really matter in practice?
>> >> >> What is the objection, actually?
>> >> >>
>> >> >> As I mentioned on the call, I think the prior stage should do any
>> >> >> merging or fixing up. Trying to do that sort of thing in 'early' code
>> >> >> in U-Boot (or any other program, including Linux) is such a pain. With
>> >> >> U-Boot, for example, we don't even have any RAM available to do it
>> >> >> with half the time and it would dramatically increase the amount of
>> >> >> memory needed prior to relocation. It just isn't a very good idea to
>> >> >> try to do this in early code. It is also completely unnecessary, once
>> >> >> you get past the philosophical objections.
>> >> >>
>> >> >> If TF-A wants to be in the picture, let it deal with the implications
>> >> >> and responsibility thus incurred. TF-A has no right to tell U-Boot how
>> >> >> to handle its config. TF-A is 0.5m LOC, i.e. a lot, almost a quarter
>> >> >> of the size of U-Boot. It duplicates loads of things in there. No one
>> >> >> will even *notice* an FDT merge function, which is actually only 70
>> >> >> LOC:
>> >> >>
>> >> >> /**
>> >> >>  * overlay_apply_node - Merges a node into the base device tree
>> >> >>  * @fdt: Base Device Tree blob
>> >> >>  * @target: Node offset in the base device tree to apply the fragment 
>> >> >> to
>> >> >>  * @fdto: Device tree overlay blob
>> >> >>  * @node: Node offset in the overlay holding the changes to merge
>> >> >>  *
>> >> >>  * overlay_apply_node() merges a node into a target base device tree
>> >> >>  * node pointed.
>> >> >>  *
>> >> >>  * This is part of the final step in the device tree overlay
>> >> >>  * application process, when all the phandles have been adjusted and
>> >> >>  * resolved and you just have to merge overlay into the base device
>> >> >>  * tree.
>> >> >>  *
>> >> >>  * returns:
>> >> >>  *      0 on success
>> >> >>  *      Negative error code on failure
>> >> >>  */
>> >> >> static int overlay_apply_node(void *fdt, int target,
>> >> >>                void *fdto, int node)
>> >> >> {
>> >> >>    int property;
>> >> >>    int subnode;
>> >> >>
>> >> >>    fdt_for_each_property_offset(property, fdto, node) {
>> >> >>       const char *name;
>> >> >>       const void *prop;
>> >> >>       int prop_len;
>> >> >>       int ret;
>> >> >>
>> >> >>       prop = fdt_getprop_by_offset(fdto, property, &name,
>> >> >>                     &prop_len);
>> >> >>       if (prop_len == -FDT_ERR_NOTFOUND)
>> >> >>          return -FDT_ERR_INTERNAL;
>> >> >>       if (prop_len < 0)
>> >> >>          return prop_len;
>> >> >>
>> >> >>       ret = fdt_setprop(fdt, target, name, prop, prop_len);
>> >> >>       if (ret)
>> >> >>          return ret;
>> >> >>    }
>> >> >>
>> >> >>    fdt_for_each_subnode(subnode, fdto, node) {
>> >> >>       const char *name = fdt_get_name(fdto, subnode, NULL);
>> >> >>       int nnode;
>> >> >>       int ret;
>> >> >>
>> >> >>       nnode = fdt_add_subnode(fdt, target, name);
>> >> >>       if (nnode == -FDT_ERR_EXISTS) {
>> >> >>          nnode = fdt_subnode_offset(fdt, target, name);
>> >> >>          if (nnode == -FDT_ERR_NOTFOUND)
>> >> >>             return -FDT_ERR_INTERNAL;
>> >> >>       }
>> >> >>
>> >> >>       if (nnode < 0)
>> >> >>          return nnode;
>> >> >>
>> >> >>       ret = overlay_apply_node(fdt, nnode, fdto, subnode);
>> >> >>       if (ret)
>> >> >>          return ret;
>> >> >>    }
>> >> >>
>> >> >>    return 0;
>> >> >>
>> >> >>
>> >> >>
>> >> >> }
>> >> >>
>> >> >>
>> >> >> >
>> >> >> > Regards
>> >> >> > /Ilias
>> >> >> > >
>> >> >> > > To repeat, if we set things up correctly and agree on the bindings,
>> >> >> > > devicetree can be the unifying configuration mechanism through the
>> >> >> > > whole of firmware (except for very early bits) and into the OS. I 
>> >> >> > > feel
>> >> >> > > this will set us up very well to deal with the complexity that is
>> >> >> > > coming.
>> >> >> > >
>> >> >>
>> >> >> Regards,
>> >> >> Simon
>> >
>> > --
>> > François-Frédéric Ozog | Director Business Development
>> > T: +33.67221.6485
>> > francois.ozog@linaro.org | Skype: ffozog
>> >
>
> --
> François-Frédéric Ozog | Director Business Development
> T: +33.67221.6485
> francois.ozog@linaro.org | Skype: ffozog
>