Re: [PATCH v4 19/21] drivers/of: Support adding sub-tree

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2015-05-14 at 09:23 +0300, Pantelis Antoniou wrote:

> > A few things that I don't find in the overlay code (but maybe I haven't
> > looked at it hard enough):
> > 
> > - Can it remove nodes/properties ?
> > 
> 
> Yes.

Ok, I've missed that when looking at the overlay code then, I'll have to
give it a closer look.

> > - Can it "commit" a changeset so it's permanently part of the main DT ?
> > We will never have a concept of "revertable" changesets, if we need a
> > subsequent update, we will get a new overlay from FW that will remove
> > what needs to be removed and add what needs to be added.
> > 
> 
> The overlay when applied is a part of the kernel DT tree.
> It is trivial to add a mechanism that simply commits everything and
> tosses away the revert information.
> 
> Note that in that case you have to make provisions for the unflatten
> blob to not be freed or for the device tree nodes/properties to be
> dynamically allocated.

I think it makes sense to do the dynamic thing anyway...

> > IE, our current mechanism without overlay is fairly simple:
> > 
> >  - On PCI unplug, we remove all nodes below the slot (from linux),
> > the FW does the equivalent internally.
> > 
> 
> If you use an overlay, you just revert it and everything would
> be as it was before, without anything hanging below the slot node.

Except that doesn't work for the boot time content which we get
from the firmware as part of the initial FDT (and we can't change that
without breaking backward compatibility).

> Note that the ‘remove all nodes below the slot’ does not work for my case.
> 
> That is because there are devices being instantiated under the slot
> (i2c busses, i2c devices, FPGAs etc) that need to be removed from the
> system.

Right while in my case, there isn't, it's just the standard OF PCI
representation generated by FW, the main thing is that it might have
some enriched properties for some known cable cards of external drawers
that are good to have.

> >  - On PCI re-plug, the FW internally builds new nodes and sends a
> > new subtree as an FDT that we can expand/attach.
> > 
> 
> You can easily send a DT blob containing an overlay from firmware.

Sending one is easy. Building it is not :-)

> It can be even easy, since you might not have to recreate the full blob
> each time, but instead using flat device tree methods to populate the
> few properties that change each time.

No, we basically have our internal tree in the firmware in a format
similar to Linux, ie, a pointer based tree. We can "flatten" it of
course, but generating an overlay is trickier. We can, it's just more
work and we are running out of time (I basically have to cut that FW in
the next few days, then we'll be stuck with whatever interfaces we
created, I have a big of time to fix bugs after that but that's about
it).

> > Now we could consider that subtree as a changeset that can be undone,
> > but that wouldn't work for boot time. And subsequent updates wouldn't
> > have that concept of "undoing" anyway.
> > 
> 
> I have posted another patch that does boot-time DT quirk which are
> non-revertable.
> 
> https://lkml.org/lkml/2015/2/18/258

Not sure how that applies in my case ... I can't change the
representation of the PCI subtree, this is standard OFW representation,
I can't change the FW to make it an overlay-like thing at boot time,
that would break existing kernels.

> > IE. conceptually, what overlays do today is quite rooted around the idea
> > of having a fixed "base" DT and some pre-compiled DTB overlays that
> > get added/removed. The design completely ignore the idea of a FW that
> > maintains a "live" tree which we want to keep in sync, which is what we
> > want to do here, or what we could do with a "live" open firmware
> > implementation.
> > 
> > Now we might be able to reconcile them, but it feels to me that the
> > overlay/changeset stuff is too rooted in the first concept…
> > 
> 
> The first DT overlays use case (beaglebone capes) is what got the concept
> started.
> 
> Right now is a generic mechanism to apply modifications to the kernel
> live tree, with the possibility to revert them.

Yes but as I said it's not really thought in term of keeping the kernel
tree in sync with an external dynamically generated tree. Maybe we can
fix it, but it's more complex...

Ben.

> > Ben.
> > 
> > 
> 
> Regards
> 
> — Pantelis