Re: [PATCH 0/7] of: overlay: Add support for export-symbols node feature

[Rob Herring]

+dtc list and David G.

On Tue, Dec 10, 2024 at 2:16 AM Herve Codina <herve.codina@xxxxxxxxxxx> wrote:
>
> Hi Rob,
>
> On Mon, 9 Dec 2024 14:11:09 -0600
> Rob Herring <robh@xxxxxxxxxx> wrote:
>
> ...
> > >
> > > Our overlay using the nexus node can contains:
> > >    node {
> > >       foo-gpio = <&connector 0 GPIO_ACTIVE_HIGH>;
> > >    };
> >
> > Couldn't we make something like this work:
> >
> > connector: __overlay__ {
> >
> >    node {
> >       foo-gpio = <&connector 0 GPIO_ACTIVE_HIGH>;
> >    };
> > };
> >
> > We already have to process all the phandles in the overlay. So this
> > just changes handling of 'connector' from being a local phandle which
> > we just renumber to an unresolved phandle which we have to lookup and
> > replace the phandle uses with.
> >
>
> I have tried what you suggested but I've got some issues with dtc.
>
> If a label is not used as a phandle in a dts, dtc doesn't create the phandle
> property in the pointed node (except if we use '-@' option but I don't want
> to add all symbols in my dtb just for one or two connector symbols).

Sorry, but that's the cost of using overlays, and that's pretty
orthogonal to the issue of how the overlay references the connector
node.

However, I agree '-@' is a pretty big switch and an issue that's been
discussed before. I also don't like that all labels become part of the
ABI nor the fact that overlays can make any random modification
anywhere in the DT. I would rather see some sort of explicit opt-in
mechanism of nodes we can apply overlays to. Perhaps we could do
something like this:

/export/ label: node {
};

And then __symbols__ can be only those exported labels (unless -@ is used).

> The way to make sure that the phandle property will be created in the base
> DT node by dtc is to reference the label as a phandle in the base DT.
> The export-symbols node references this label as a phandle in the base DT
> and so, with that, dtc creates the phandle property.
>
> Also, using 'connector: __overlay__' allows to have only one label from
> the base DT to be referenced by the overlay.
>
> I don't know if use cases exist where more than one label need to be
> referenced but this 'one label' constraint is not present with the
> export-symbols node.
>
> The use case where more than one label would be needed is the need for a
> phandle from the overlay that couldn't be translated by the connector nexus
> node. Maybe pinctrl because it uses of_find_node_by_phandle().

Labels are an ABI. I can't see that we need to remap them when we can
just say the name must be X. We can have multiple labels on a node as
well. So I think the problem space is purely mapping 1 name to
multiple possible names.

The connector handling has to be addressed binding by binding at least
for each pattern of binding. Pinctrl binding is pretty unique, so we
should make sure we can handle it in this case.

> Last point, having export-symbols node makes some nodes explicitly
> candidates for an overlay and defines the label to be used on the base DT
> node side. This specific label can be described in the node binding as well
> as the nexus node properties.

Both David (IIRC) and I feel that putting the overlay info
(__symbols__, __fixups__, etc.) within the DT data rather than in the
DTB format was a mistake. The export-symbols node expands on that, so
I'm not sure that's the right direction.

(We should have rev'ed the DTB format to store type information for
(at a minimum) phandles.)

> With 'connector: __overlay__', the overlay can be applied on any nodes, at
> least from the needed label point of view without any restrictions.

Certainly that is something I'd like to have some control over. An
/export/ tag would accomplish that.

One idea I have there is that the overlay could have the compatible of
the connector and we use that for matching. That would give us a way
to know what base DTs overlays apply to. Then you could load an
overlay and dispatch it to the correct driver to handle. It would have
to be handled as a special case as the compatible may match, but
wouldn't necessarily be equal values.


I'll throw out another idea. What if we make resolving phandle errors
something that can be handled by the connector driver? The driver
knows 'connector' resolves to the connector node it is applying the
overlay to.

Rob