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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Lizhi,

On Thu, Aug 24, 2023 at 8:40 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
> On 8/24/23 01:31, Geert Uytterhoeven wrote:
> > 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.
> >
> > 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:

Oh, I had missed that the "fragment target path" is appended,
and thought it was just overridden.

> /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.

Is there a need for that? Both c and d can be handled as subnodes
in a single fragment if the target path is empty (and see below).

> >   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.

That makes sense.
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds