Re: [PATCH 1/2] libfdt: overlay: Allow resolving phandle symbols

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8390 bytes --]

On Fri, Sep 20, 2024 at 10:04:56PM +0530, Ayush Singh wrote:
> On 9/18/24 08:06, David Gibson wrote:
> 
> > On Mon, Sep 16, 2024 at 03:10:52PM +0530, Ayush Singh wrote:
> > > On 9/12/24 09:08, David Gibson wrote:
> > > 
> > > > On Mon, Sep 09, 2024 at 12:54:34PM +0530, Ayush Singh wrote:
> > > > > On 9/9/24 10:33, David Gibson wrote:
> > > > > 
> > > > > > On Mon, Sep 02, 2024 at 05:47:55PM +0530, Ayush Singh wrote:
> > > > > > > Add ability to resolve symbols pointing to phandles instead of strings.
> > > > > > > 
> > > > > > > Combining this with existing fixups infrastructure allows creating
> > > > > > > symbols in overlays that refer to undefined phandles. This is planned to
> > > > > > > be used for addon board chaining [1].
> > > > > > I don't think this "autodetection" of whether the value is a phandle
> > > > > > or path is a good idea.  Yes, it's probably unlikely to get it wrong
> > > > > > in practice, but sloppy cases like this have a habit of coming back to
> > > > > > bite you later on.  If you want this, I think you need to design a new
> > > > > > way of encoding the new options.
> > > > > Would something like `__symbols_phandle__` work?
> > > > Preferable to the overloaded values in the original version, certainly.
> > > I can whip it up if that would be sufficient. But if we are talking about
> > > any big rewrite, well, I would like to get the details for that sorted out
> > > first.
> > Fair enough.
> > 
> > > > > It should be fairly
> > > > > straightforward to do. I am not sure how old devicetree compilers will react
> > > > > to it though?
> > > > Well, the devicetree compiler itself never actually looks at the
> > > > overlay encoding stuff.  The relevant thing is libfdt's overlay
> > > > application logic... and any other implementations of overlay handling
> > > > that are out there.
> > > > 
> > > > At which I should probably emphasize that changes to the overlay
> > > > format aren't really mine to approve or not.  It's more or less an
> > > > open standard, although not one with a particularly formal definition.
> > > > Of course, libfdt's implementation - and therefore I - do have a fair
> > > > bit of influence on what's considered the standard.
> > > So do I need to start a discussion for this somewhere other than the
> > > devicetree-compiler mailing list? Since ZephyrRTOS is also using devicetree,
> > > maybe things need to be discussed with them as well?
> > <devicetree-spec@vger.kernel.org> and <devicetree@vger.kernel.org> are
> > the obvious candidate places.  There will be substantial overlap with
> > devicetree-compiler of course, but not total probably.
> > 
> > > > > I really do not think having a different syntax for phandle symbols would be
> > > > > good since that would mean we will have 2 ways to represent phandles:
> > > > > 
> > > > > 1. For __symbols__
> > > > > 
> > > > > 2. For every other node.
> > > > I'm really not sure what you're getting at here.
> > > I just meant that I would like to keep the syntax the same:
> > > 
> > > sym = <&abc>;
> > Ok, the syntax for creating them in dts, rather than the encoding
> > within the dtb.  Why are you manually creating symbols?
> > 
> > So.. aside from all the rest, I don't really understand why you want
> > to encode the symbols as phandles rather than paths.
> 
> It's for connector stacking using the approach described here [0].

Thanks for the link.

> To go into more detail, let us assume that we have a mikrobus connector on
> the base board. We connect an addon board that exposes a grove connector.
> Now to describe the parent i2c adapter of the grove connector, we cannot
> really specify the full node path. However, having support for phandles
> would make writing the overlay for such an addon board possible.
> 
> In practice it might look as follows:
> 
> 
> mikrobus-connector.dtso
> 
> 
> &{/} {
> 
>    __symbols__ {
> 
>         MIKROBUS_SCL_I2C = "path";
> 
>         ...
> 
>     };
> 
> }
> 
> 
> grove-connector-addon.dtso
> 
> 
> &{/} {
> 
>     __symbols__ {
> 
>         GROVE_PIN1_I2C = <&MIKROBUS_SCL_I2C>;
> 
>     };
> 
> };

So, essentially you're just adding new labels as aliases to existing
labels?

Ok, I can see at least two ways of doing that which I think are a more
natural fit than allowing symbols to be phandles.

# Method 1: Allow path references in overlays

dts allows references both in integer context:
	foo = <&bar>;
in which case it resolves to a phandle, but also in string/bytestring context:
	foo = &bar;
In which case it resolves to a path.

Runtime overlays, only support the former, but could be extended to
support the latter form.  It would be a trickier than phandle
references, because updating a path reference would require expanding
/ shrinking the property including it, but I don't think it's super
difficult.

It might be somewhat harder to imlpement than your original proposal,
but it's conceptually simpler and more versatile.  In fact it removes
a currently existing asymmetry between what dts and overlays can do.

# Method 2: /aliases

/__symbols__ is very similar to the much older IEEE1275 concept of
/aliases.  In fact they're encoded identically except for appearing in
/aliases instead of /__symbols__.  The original use for these was in
interactive Open Firmware, so you could type, say, "dev hd" instead of
"dev /pci@XXXXXXXX/scsi@X,Y/lun@XX/..." or whatever path the primary
hard disk had.  Arguably, __symbols__ should never have been invented,
and we should have just used /aliases.  This is the kind of thing I
mean when I say they overlay encoding wasn't very well thought out.

But, here's the thing:

a) aliases can be defined in terms of other aliases:

	aliases {
		scsi0 = "/pci@XXXXX/pci-bridge@X,Y/scsi@X,Y";
		hd = "scsi0/lun@YY";
	}

b) fdt_path_offset() already resolves aliases

If given a path without a leading '/', it will look up the first
component as an alias, then look up the rest of the path relative to
that.

So, in your example, if the base layer defined MIKROBUS_SCL_I2C as
an alias rather than a symbol, then in the next layer you could have:

&{/} {
     	aliases {
		GROVE_PIN1_I2C = "MIKROBUS_SCL_I2C";
	}
}

libfdt should already resolve this when applying overlays, because it
just calls fdt_path_offset().

So your only remainingly difficulty is /aliases versus /__symbols__.
It would be fairly simple to make overlay application expand
__symbols__ in much the same way as aliases.  Or, you could just have
a copy of everything in __symbols__ in aliases as well (which might be
a path to eventually deprecating /__symbols__).  dtc already has the
-A flag which will but all labels into /aliases, very much like -@
will put them all in /__symbols__.

[snip]
> Well, a lot of work is still going in this direction [1]. I myself
> am trying to use it for mikroBUS connectors.

Sure, and I can see why: it seems tantalizingly close to working
simply.  But that doesn't mean it won't have limitations that will
bite you down the track.

> The reason for using devicetree overlays for such connectors is
> because of their loose nature (mikroBUS is also open
> connector). This means that as long as the sensor/ device can make
> do with the pins provided by mikroBUS connector, it can be an addon
> board. And there is no limitation of staking the boards. So one can
> do rpi hat -> mikrbus connectors -> grove connector -> grove some
> addon board.

For example, the very fact that these are loose exposes you to one
pretty obvious limitation here.  Suppose some future board has a
connector with enough pins that you can hang two independent grove
connectors off it, and someone makes a hat/shield/whatever that does
exactly that.  But now the grove addon dtbs need to be different
depending on whether they attach to grove connector 1 or grove
connector 2.

The old "connector binding" proposals I was describing aimed to
decouple the type of the connector from the instance of the connector
for exactly this sort of case.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]