Re: [PATCH V13 4/5] of: overlay: Extend of_overlay_fdt_apply() to specify the target node

[Rob Herring <robh@kernel.org>]

On Thu, Aug 24, 2023 at 1:40 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
>
> Hi Geert,
>
> Thanks for reviewing the patch. I add my comment in-line.
>
> On 8/24/23 01:31, Geert Uytterhoeven wrote:
> >     Hi Lizhi,
> >
> > On Tue, 15 Aug 2023, Lizhi Hou wrote:
> >> Currently, in an overlay fdt fragment, it needs to specify the exact
> >> location in base DT. In another word, when the fdt fragment is
> >> generated,
> >> the base DT location for the fragment is already known.
> >>
> >> There is new use case that the base DT location is unknown when fdt
> >> fragment is generated. For example, the add-on device provide a fdt
> >> overlay with its firmware to describe its downstream devices. Because it
> >> is add-on device which can be plugged to different systems, its firmware
> >> will not be able to know the overlay location in base DT. Instead, the
> >> device driver will load the overlay fdt and apply it to base DT at
> >> runtime.
> >> In this case, of_overlay_fdt_apply() needs to be extended to specify
> >> the target node for device driver to apply overlay fdt.
> >>    int overlay_fdt_apply(..., struct device_node *base);
> >>
> >> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> >
> > Thanks for your patch, which is now commit 47284862bfc7fd56 ("of:
> > overlay: Extend of_overlay_fdt_apply() in dt-rh/for-next.
> >
> >> --- a/drivers/of/overlay.c
> >> +++ b/drivers/of/overlay.c
> >> @@ -715,6 +730,7 @@ static struct device_node *find_target(struct
> >> device_node *info_node)
> >> /**
> >>  * init_overlay_changeset() - initialize overlay changeset from
> >> overlay tree
> >>  * @ovcs:        Overlay changeset to build
> >> + * @target_base:    Point to the target node to apply overlay
> >>  *
> >>  * Initialize @ovcs.  Populate @ovcs->fragments with node information
> >> from
> >>  * the top level of @overlay_root.  The relevant top level nodes are the
> >
> > As an overlay can contain one or more fragments, this means the
> > base (when specified) will be applied to all fragments, and will thus
> > override the target-path properties in all fragments.
> >
> > However, for the use case of an overlay that you can plug into
> > a random location (and of which there can be multiple instances),
> > there can really be only a single fragment.  Even nodes that typically
> > live at the root level (e.g. gpio-leds or gpio-keys) must be inserted
> > below the specified location, to avoid conflicts.

It's not a random location, but a location where the full path and/or
unit-address are not known. What we should know is the node's base
name and compatible.

I think we can assume for this kind of usecase, that adding nodes only
under a defined base node is allowed. This is also just the
restriction I've asked for every time more general support of applying
overlays by the kernel is requested. The add-on card, hat, cape, etc.
usecases should all be applied downstream of some node.

> >
> > Hence:
> >   1. Should init_overlay_changeset() return -EINVAL if target_base is
> >      specified, and there is more than one fragment?
>
> Maybe allowing more than one fragment make the interface more generic?
> For example, it could support the use case that multiple fragments share
> the same base node.
>
> Currently, the fragment overlay path is "base node path" + "fragment
> target path". Thus, for the structure:
>
> /a/b/c/fragment0
>
> /a/b/d/fagment1
>
> It can be two fragments in one fdt by using
>
>    base node path = /a/b
>
>    fragment0 target path = /c
>
>    fragment1 target path = /d
>
> I am not sure if there will be this kind of use case or not. And I think
> it would not be hurt to allow that.
>
> >
> >   2. Should there be a convention about the target-path property's
> >      contents in the original overlay?
> >      drivers/of/unittest-data/overlay_pci_node.dtso in "[PATCH V13 5/5]
> >      of: unittest: Add pci_dt_testdrv pci driver" uses
> >
> >          target-path="";
> >
> >      which cannot be represented when using sugar syntax.
> >      "/" should work fine, though.
>
> Because the fragment overlay path is "base node path" + "fragment target
> path", I may add code to check if "fragment target patch is '/' and
> ignore it. I think that would support sugar syntax with only '/' specified.

Note that "/" is also a valid target path. I think it would be better
to have a form that's obviously not a fixed path. I think what's
needed is to be able to specify just the nodename with or without the
unit-address. I don't know if dtc will accept that.

As labels are part of the ABI with overlays, a target label could also
work. Though the kernel would have to learn to add new labels or get a
label path from another source as a label doesn't exist on a generated
node.

Rob