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

[Herve Codina <herve.codina@bootlin.com>]

Hi Rob, David,

On Tue, 10 Dec 2024 15:58:33 +0100
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi Rob,
> 
> On Tue, 10 Dec 2024 07:46:02 -0600
> Rob Herring <robh@kernel.org> wrote:
> 
> > +dtc list and David G.
> > 
> > On Tue, Dec 10, 2024 at 2:16 AM Herve Codina <herve.codina@bootlin.com> wrote:  
> > >
> > > Hi Rob,
> > >
> > > On Mon, 9 Dec 2024 14:11:09 -0600
> > > Rob Herring <robh@kernel.org> 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.  
> 
> So, with a base DT having:
>   /export/ label0: node@0 {
>   }
> 
>   /export/ label1: node@1 {
>   }
> 
> the __symbols__ node will contains:
>   __symbols__ {
> 	label0 = ...;
> 	label1 = ...;
>   }
> 
> without export-symbols, the overlay will look like this:
>   connector: __overlay__ {
> 	...
> 	ref = <&connector>;
>   }
> 
> The "connector" label is the only one we can use from the overlay to
> reference the base DT (special label defined on the __overlay__ node).
> As it is defined to point to __overlay__, it has to be resolved to the
> exported symbol that point to the node where the overlay is applied.
> 
> If the overlay is applied on node@0, 'connector' is resolved to node@0.
> 
> This case cannot be handled:
>    connector: __overlay__ {
> 	...
> 	ref1 = <&connector>;
>         ref2 = <&other-label-from-base-dt>;
>    }
> 
> Indeed, only one 'connector' label can be resolved to node@0 or node@1.
> other-label-from-base-dt cannot be resolved based on the node the overlay
> is applied to.
> 
> Again, I am not sure on my side that we have to handle this case of multiple
> labels in the overlay that point to the base DT dependent on node@0 or node@1.
> 
> On my use case, I considered the node@0 or node@1 as nexus nodes and so, all
> GPIOs (soon PWM, I hope, and probably other ressources in the future) can be
> referenced using nexus nodes.
> 
> > 
> > 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.  
> 
> If pinctrl can be handled using nexus node, it should be ok. Otherwise
> things are going to be complicated. Again, with your proposal, we can
> reference only one label from the overlay, the node where the overlay
> is applied to.
> 
> >   
> > > 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.  
> 
> compatible property will not be enough. We can have two exact same
> connectors on the same base board:
>   /export/ label0: node@0 {
>      compatible = 'foo,addon-connector';
>   }
> 
>   /export/ label1: node@1 {
>      compatible = 'foo,addon-connector';
>   }
> 
> To load the overlay, we can match compatible for sure but we need to node
> the overlay is supposed to be applied to.
> 
> Also, it you want to check bindings for node@0 available in the base DT
> and bindings in the overlay, having one compatible string with different
> meaning will introduce some complexity.
> A compatible string can now define the "provider" part (the base DT) and
> the "consumer" part (the overlay).
> 
> > 
> > 
> > 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.  
> 
> Well, my proposal was to give enough information to the resolver instead of
> handling errors.
> 
> I probably miss something but I don't see what could be the benefit to do
> that in the other way. Can you give me more details about your idea?
> 

If I understand correctly, to move forward on the topic, we are waiting from
feedback from David.

Is that correct or do you have in mind an other plan I could explore?

Best regards,
Hervé