[PATCH] kbuild: dtc: Allow adding device tree fragments via config

[Ahmad Fatoum]

Hello Trent,

On 19.09.21 23:51, Trent Piepho wrote:
> On Wed, Sep 15, 2021 at 4:42 AM Ahmad Fatoum <a.fatoum at pengutronix.de> wrote:
>> On 08.09.21 20:59, Trent Piepho wrote:
>>> This introduces config variables that allow adding additional fragments
>>> to the Barbox device tree(s).
>>>
>>> Example uses are adjusting the flash partition layout, adding barebox
>>> state varibles, or adding an I2C device.  These can be now be done with
>>> build configuration only, without needing to patch the existing dts
>>> files in the Barebox source.
>>>
>>
>> I don't see the utility of using this new mechanism for in-tree boards.
>> I suggest dropping EXTRA_DTS_FRAGMENTS.
> 
> I wasn't sure if this was useful or not.  I didn't have a use case,
> but thought somebody might think of one.

I think we should actively discourage use of this for in-tree configs,
so I prefer removing it.

>>> Preprocessing the dts file gains another layer, where a generated dts
>>> source consisting of an include directive for the original dts source is
>>> followed by more includes for each fragment.  This is piped to the
>>> existing preprocessor call on stdin to avoid another temporary file.
>>> cpp/dtc will correctly identify errors in the source files they occur
>>> in.  The -MT option is used so the cpp auto-dependencies reference the
>>> original dts source and not the generated code passed on stdin.
>>
>> If you go this route wouldn't you want to apply device tree overlays?
> 
> Can the Barebox apply overlays to the *Barebox dtb* when it starts?

I meant overlay application at compile time. I have no experience with
that and was interested to hear your opinion on it.

> of_register_overlay() applies the overlay to the Linux dtb if I'm not
> mistaken.  The Barebox dtb is passed to the entry function from the
> pbl.  By the time board code runs in the main Barebox it's too late to
> modify the Barebox device tree.

Early runtime application is possible as well, see
of_overlay_apply_tree(). But that's not what I want to advocate here.

>> Blindly applying fragments doesn't mesh well with multi-image.
> 
> It is working well for me, but it was not done blindly.  I think the
> use case was not clear.  This is for Barebox when integrated into a
> build system using something like Yocto or Buildroot for someone
> creating a complete finished FW for a real product..  If one was only
> working on Barebox alone, then you would probably just modify existing
> dts files or add a new image build for your board+dts combo.
> 
> Since one is configuring Barebox for their specific product, they do
> not want to build every board Barebox supports.

In my bubble, barebox is occasionally configured to support multiple
board variants via multi-image when it's not feasible to differentiate
at runtime.

> They just want their
> board built.  So one will not create specific NAND flash partitions
> for their exact firmware design and then want those partitions in
> every board Barebox supports.  That doesn't make sense.
> 
> I have one product I am writing FW for, but I still use multi-image
> build because there are different Barebox images for mutually
> exclusive choices on how to configure the hardware.

So the bubbles do overlap :-)

> I want to use RAUC and for that I need to create barebox-state
> variables in the Barebox device tree.
> 
> With this patch, I can create a dts fragment with a barebox-state
> node.  Then I configure buildroot to add this to Barebox.  All my
> different Barebox images now have the barebox-state node, since the
> fragment works for all them.

New features shouldn't break multi-image. And adding a new mechanism
that applies a fragment to all device trees can introduce such breakage.

There is an easy way out, define the (sanitized) board name as a macro
and let users worry about it.

> I commit to my FW repository (which would be like a product specific
> Yocto meta-layer) the barebox-state dts fragment and the RAUC system
> config that references those state variables.  They are both in the
> same repository and if the variables change, both the dts fragment and
> the RAUC config can be updated in the same atomic commit.
> 
> So now I have added rauc support entirely from the Buildroot
> configuration menu with added dts fragment and rauc config files in my
> FW repository.  *I did not need to apply any patches to the Barebox
> code*
> 
> That is the use case here. So I can use rauc with just the standard
> Barebox source without needing to patch it.

I see the utility when using e.g. evaluation kits barebox already
supports. Could be useful for DistroKit if RAUC support were to
be added.

> Of course there are many other things one would probably also do this
> way, like changing an expansion header from I2C to UART or
> partitioning NAND flash.
> 
> I do not see how one could do this with overlays.  Even if it was
> possible to apply an overlay in the pbl, one would still need to patch
> the lowlevel board code to find and apply those overlays, which
> defeats the whole point of not needing to patch the Barebox source.
> 
> I'll also mention that Linux can do this too in a way.  If you need to
> modify your board dts for Linux, what you do is add an external dts
> file in Buildroot/Yocto and have it make a dtb from that.  You don't
> need to patch the dts files in the kernel source.  But the Barebox dtb
> is not supplied to Barebox by a bootloader like Linux.  The way dtb
> generation is integrated into the image Makefile and even the lowlevel
> code makes it not practical to add new images to Barebox with only a
> make command line argument.
> 
>> I assume with overlays, you could skip an overlay if it has a differing
>> compatible. If we don't use overlays, you should at least define
>> a symbol with the name of the device tree file, so fragments have
>> a chance of being multi-image compatible via preprocessor logic.
> 
> Certainly one could use the preprocessor.  It would be much easier to
> just define what you need in the dts itself.  But one would need to
> patch the Barebox dts and the point was to avoid that.  I suppose the
> makefile could construct a valid C macro name from the filename and
> define it.
> 
> But I wonder if one needs this?

Yes, upstream code should take multi-image into account.
I guess sticking some

  #define $(subst -,_,$<) into the echo'ed string should be enough

> If I was building two different
> boards under yocto, I would have a machine specific override in my
> barebox bbappend to add the only dts fragment for board I was building
> for.  Yocto builds a different copy of barebox for each
> target/machine.  I do not need barebox to use the preprocessor to turn
> off the fragment I am not using.  I would not have yocto give it the
> unused fragment in the first place.

I usually avoid Yocto MACHINEs for board variants. Build same rootfs
for both variants, ship two device trees and reference them either
from FIT configurations or bootloader specs and let the bootloader worry
about selecting the correct DT to boot.

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |