Message-ID: <20241210155833.76357d50@bootlin.com>
Date: Tue, 10 Dec 2024 15:58:33 +0100
From: Herve Codina <herve.codina@...tlin.com>
To: Rob Herring <robh@...nel.org>
Cc: David Gibson <david@...son.dropbear.id.au>, Andrew Davis <afd@...com>,
 Ayush Singh <ayush@...gleboard.org>, Geert Uytterhoeven
 <geert@...ux-m68k.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Arnd Bergmann <arnd@...db.de>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, Saravana Kannan
 <saravanak@...gle.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Luca Ceresoli <luca.ceresoli@...tlin.com>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>, Devicetree Compiler
 <devicetree-compiler@...r.kernel.org>
Subject: Re: [PATCH 0/7] of: overlay: Add support for export-symbols node
 feature

Hi Rob,

On Tue, 10 Dec 2024 07:46:02 -0600
Rob Herring <robh@...nel.org> wrote:

> +dtc list and David G.
> 
> On Tue, Dec 10, 2024 at 2:16 AM Herve Codina <herve.codina@...tlin.com> wrote:
> >
> > Hi Rob,
> >
> > On Mon, 9 Dec 2024 14:11:09 -0600
> > Rob Herring <robh@...nel.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?

Best regards,
Hervé

Powered by blists - more mailing lists