[PATCH 0/2] Add support for phandle in symbols

[PATCH 0/2] Add support for phandle in symbols

[Ayush Singh]

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

Patch 2 containing tests demonstrates how this looks in practice.

Open Items:

1. Phandle vs String identification:
   I am not sure how to distinguish between symbols that are paths and
   phandles. Currently I am checking the proplen and if the first byte
   is '/'. However, this can cause problems if the number of phandles in
   dt exceed 0x2f000000 or 788,529,152.

   Alternatively, if we can be sure that there will be no node whose
   path is of length = 4, i.e. a node name of 2 characters ('\' and NULL
   already take up 2 characters), we can avoid the '/' check.

[1]: https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/

Signed-off-by: Ayush Singh <ayush@beagleboard.org>

---
Ayush Singh (2):
      libfdt: overlay: Allow resolving phandle symbols
      tests: Add test for symbol resolution

 libfdt/fdt_overlay.c                   | 16 ++++++++++------
 tests/overlay_overlay_symbols1.dts     | 12 ++++++++++++
 tests/overlay_overlay_symbols2.dts     |  9 +++++++++
 tests/overlay_overlay_symbols_user.dts | 26 ++++++++++++++++++++++++++
 tests/run_tests.sh                     | 19 +++++++++++++++++++
 5 files changed, 76 insertions(+), 6 deletions(-)
---
base-commit: 99031e3a4a6e479466ae795790b44727434ca27d
change-id: 20240829-symbol-phandle-9d1e0489c32a

Best regards,
-- 
Ayush Singh <ayush@beagleboard.org>

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

[Ayush Singh]

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

[1] https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 libfdt/fdt_overlay.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index 28b667f..10127be 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -395,12 +395,16 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
 	symbol_path = fdt_getprop(fdt, symbols_off, label, &prop_len);
 	if (!symbol_path)
 		return prop_len;
-	
-	symbol_off = fdt_path_offset(fdt, symbol_path);
-	if (symbol_off < 0)
-		return symbol_off;
-	
-	phandle = fdt_get_phandle(fdt, symbol_off);
+
+	if (prop_len == sizeof(uint32_t) && symbol_path[0] != '/') {
+		phandle = fdt32_ld((const fdt32_t *)symbol_path);
+	} else {
+		symbol_off = fdt_path_offset(fdt, symbol_path);
+		if (symbol_off < 0)
+			return symbol_off;
+		phandle = fdt_get_phandle(fdt, symbol_off);
+	}
+
 	if (!phandle)
 		return -FDT_ERR_NOTFOUND;
 

-- 
2.46.0

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

[David Gibson]

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

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.

> 
> [1] https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  libfdt/fdt_overlay.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 28b667f..10127be 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -395,12 +395,16 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>  	symbol_path = fdt_getprop(fdt, symbols_off, label, &prop_len);
>  	if (!symbol_path)
>  		return prop_len;
> -	
> -	symbol_off = fdt_path_offset(fdt, symbol_path);
> -	if (symbol_off < 0)
> -		return symbol_off;
> -	
> -	phandle = fdt_get_phandle(fdt, symbol_off);
> +
> +	if (prop_len == sizeof(uint32_t) && symbol_path[0] != '/') {
> +		phandle = fdt32_ld((const fdt32_t *)symbol_path);
> +	} else {
> +		symbol_off = fdt_path_offset(fdt, symbol_path);
> +		if (symbol_off < 0)
> +			return symbol_off;
> +		phandle = fdt_get_phandle(fdt, symbol_off);
> +	}
> +
>  	if (!phandle)
>  		return -FDT_ERR_NOTFOUND;
>  
> 

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

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

[Ayush Singh]

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? It should be fairly 
straightforward to do. I am not sure how old devicetree compilers will 
react to it though?

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 also am not in the favor of doing something bespoke that does not play 
nice with the existing __fixups__ infra since that has already been 
thoroughly tested, and already creates __fixups__ for symbols.


Ayush Singh

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

[David Gibson]

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

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.

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

> 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 also am not in the favor of doing something bespoke that does not play
> nice with the existing __fixups__ infra since that has already been
> thoroughly tested, and already creates __fixups__ for symbols.

Hmm.  Honestly, the (runtime) overlay format was pretty a hack that's
already trying to do rather more than it was really designed for.  I'm
a bit sceptical of any attempt to extend it further without
redesigning the whole thing with a bit more care and forethought.  I
believe Rob Herring has some thoughts along these lines too.

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

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

[Ayush Singh]

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.

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

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

>> I also am not in the favor of doing something bespoke that does not play
>> nice with the existing __fixups__ infra since that has already been
>> thoroughly tested, and already creates __fixups__ for symbols.
> Hmm.  Honestly, the (runtime) overlay format was pretty a hack that's
> already trying to do rather more than it was really designed for.  I'm
> a bit sceptical of any attempt to extend it further without
> redesigning the whole thing with a bit more care and forethought.  I
> believe Rob Herring has some thoughts along these lines too.


Well, if we are really redesigning stuff, does that mean something like 
dts-v2, or everything should still be backward compatible?

The problem with guessing type can probably be fixed with a tagged union 
for the type (so an extra byte for every prop).

With more and more emphasis on runtime devicetree [0], it would be great 
to have a design that allows tackling more complex use cases.

[0]: https://lpc.events/event/18/contributions/1696/

Ayush Singh

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

[David Gibson]

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

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.

> > > I also am not in the favor of doing something bespoke that does not play
> > > nice with the existing __fixups__ infra since that has already been
> > > thoroughly tested, and already creates __fixups__ for symbols.
> > Hmm.  Honestly, the (runtime) overlay format was pretty a hack that's
> > already trying to do rather more than it was really designed for.  I'm
> > a bit sceptical of any attempt to extend it further without
> > redesigning the whole thing with a bit more care and forethought.  I
> > believe Rob Herring has some thoughts along these lines too.
> 
> Well, if we are really redesigning stuff, does that mean something like
> dts-v2, or everything should still be backward compatible?

Well.. it depends.  There are actually 3 or 4 different layers to consider:

1) The basic data model of the device tree as consumed by the kernel

This is layer which "properties are just bytestrings" comes from.

There is a case for redesigning this, since it comes from IEEE1275 and
shows its age.  A modern format would likely be self-describing, so
type information would be preserved.  A json-derivative would be the
obvious choice here, except that json can't safely transport 64-bit
integers, which is kind of a fatal flaw for this application.

This would be a fundamentally incompatible change for all current
consumers of the device tree, though.  And when I say consumers, I
don't just mean the kernel base platform code, I mean all the
individual drivers which actually need the device tree information.
I'm not suggesting this layer be changed.

2) The "dtb" format: The linearized encoding of the data model above

Changing this is much more tractable.  The dtb header includes version
numbers, allowing some degree of backwards compatibility.  There have
been, IIRC, 5 versions in total (v1, v2, v3, v16 & v17), though we've
been on v17 for a long time (10+ years) now.

There's not a heap you that can be changed here - at least neatly -
without requiring an incompatible version bump.  However dealing with
even an incompatible change at this layer is *much* easier.  As long
as the base data model remains the same you can mechanically convert
between versions, at least as long as you're not actively using new
features.  In particular it's pretty feasible to have a whole set of
tooling using a newer version that as a last step converts a final
tree to an old version for consumption by the kernel or whatever.

3) The "dts" format: the human readable / writable format

Being parsed text, it's relatively easy to extend this in backwards
compatible ways.  Note that this is more influenced by (1) than the
details of (2).  To this day, dtc can input and output the
long-obsolete v1 dtb format, and that doesn't add a lot of complexity
to it.

Any change to the other layers would likely require extensions here as
well, but I don't think there would be a need for backwards
incompatible changes.  Using certain features in the source might
impose a minimum dtb output version though.

Note that dts isn't in one to one correspondance with either (1) or
(2): there are multiple ways of specifying identical output, as there
would be in most languages.  That is a recurring source of confusion,
but I can't see a reasonable way of changing it short of a complete
redesign of (1), in which case "dts" would likely simply be obsoleted.

4) The overlay specific encoding

Overlays first existed purely as a dts construct: a different way of
arranging things in the source that would compile to the same final
tree.  Runtime overlays (a.k.a. dtbo) came later.  This is by far the
most poorly designed layer, IMO.

Basically when dtbos were invented, it was done as a quick and dirty
encoding of the dts level overlay features into the data model of (1),
which was then just encoded using (2), thereby blurring the layers a
bit.  No real thought was given to versioning or backwards
compatibility.

This is where I think a redesign would make most sense.  However, the
most sensible (IMO) ways of doing so would also require some redesign
to (2).  Basically rather than encoding the overlay specific
information "in band" as specially named and encoded properties, it
would be carried "out of band" as new special tags in the updated dtb
format.  You can think of this as a bit like relocation information
that a C compiler emits alongside the actual instruction stream.

> The problem with guessing type can probably be fixed with a tagged union for
> the type (so an extra byte for every prop).

Yeah, it's not so simple.  As things stand this would imply a change
to (1), which as noted probably isn't really feasible.  Plus, just a
one-byte type per property wouldn't cut it: some bindings have complex
encoded values in properties that are a mixture of integers and
strings and so forth.

> 
> With more and more emphasis on runtime devicetree [0], it would be great to
> have a design that allows tackling more complex use cases.
> 
> [0]: https://lpc.events/event/18/contributions/1696/

Oof.   I don't think overlays as currently designed are a great choice
for hotplug: overlays essentially allow rewriting any part of the
whole device tree, which isn't a great model for a hotplug bus.  A
long time ago some ideas were floated to define "connectors" in the
device tree that more specifically described a way things could be
plugged in that could cover several busses / interrupt controllers /
etc.  That would provide a more structured way of describing plug in
devices that can actually validate what they can do.

Of course, such a scheme is a lot more work to design and implement,
which is why the simple hack of overlays have become dominant instead.

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

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

[Ayush Singh]

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

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

     };

};


grove-addon-board.dtso


&MIKROBUS_SCL_I2C {

     dev {

         ...

     };

};

>>>> I also am not in the favor of doing something bespoke that does not play
>>>> nice with the existing __fixups__ infra since that has already been
>>>> thoroughly tested, and already creates __fixups__ for symbols.
>>> Hmm.  Honestly, the (runtime) overlay format was pretty a hack that's
>>> already trying to do rather more than it was really designed for.  I'm
>>> a bit sceptical of any attempt to extend it further without
>>> redesigning the whole thing with a bit more care and forethought.  I
>>> believe Rob Herring has some thoughts along these lines too.
>> Well, if we are really redesigning stuff, does that mean something like
>> dts-v2, or everything should still be backward compatible?
> Well.. it depends.  There are actually 3 or 4 different layers to consider:
>
> 1) The basic data model of the device tree as consumed by the kernel
>
> This is layer which "properties are just bytestrings" comes from.
>
> There is a case for redesigning this, since it comes from IEEE1275 and
> shows its age.  A modern format would likely be self-describing, so
> type information would be preserved.  A json-derivative would be the
> obvious choice here, except that json can't safely transport 64-bit
> integers, which is kind of a fatal flaw for this application.
>
> This would be a fundamentally incompatible change for all current
> consumers of the device tree, though.  And when I say consumers, I
> don't just mean the kernel base platform code, I mean all the
> individual drivers which actually need the device tree information.
> I'm not suggesting this layer be changed.
>
> 2) The "dtb" format: The linearized encoding of the data model above
>
> Changing this is much more tractable.  The dtb header includes version
> numbers, allowing some degree of backwards compatibility.  There have
> been, IIRC, 5 versions in total (v1, v2, v3, v16 & v17), though we've
> been on v17 for a long time (10+ years) now.
>
> There's not a heap you that can be changed here - at least neatly -
> without requiring an incompatible version bump.  However dealing with
> even an incompatible change at this layer is *much* easier.  As long
> as the base data model remains the same you can mechanically convert
> between versions, at least as long as you're not actively using new
> features.  In particular it's pretty feasible to have a whole set of
> tooling using a newer version that as a last step converts a final
> tree to an old version for consumption by the kernel or whatever.
>
> 3) The "dts" format: the human readable / writable format
>
> Being parsed text, it's relatively easy to extend this in backwards
> compatible ways.  Note that this is more influenced by (1) than the
> details of (2).  To this day, dtc can input and output the
> long-obsolete v1 dtb format, and that doesn't add a lot of complexity
> to it.
>
> Any change to the other layers would likely require extensions here as
> well, but I don't think there would be a need for backwards
> incompatible changes.  Using certain features in the source might
> impose a minimum dtb output version though.
>
> Note that dts isn't in one to one correspondance with either (1) or
> (2): there are multiple ways of specifying identical output, as there
> would be in most languages.  That is a recurring source of confusion,
> but I can't see a reasonable way of changing it short of a complete
> redesign of (1), in which case "dts" would likely simply be obsoleted.
>
> 4) The overlay specific encoding
>
> Overlays first existed purely as a dts construct: a different way of
> arranging things in the source that would compile to the same final
> tree.  Runtime overlays (a.k.a. dtbo) came later.  This is by far the
> most poorly designed layer, IMO.
>
> Basically when dtbos were invented, it was done as a quick and dirty
> encoding of the dts level overlay features into the data model of (1),
> which was then just encoded using (2), thereby blurring the layers a
> bit.  No real thought was given to versioning or backwards
> compatibility.
>
> This is where I think a redesign would make most sense.  However, the
> most sensible (IMO) ways of doing so would also require some redesign
> to (2).  Basically rather than encoding the overlay specific
> information "in band" as specially named and encoded properties, it
> would be carried "out of band" as new special tags in the updated dtb
> format.  You can think of this as a bit like relocation information
> that a C compiler emits alongside the actual instruction stream.
>
>> The problem with guessing type can probably be fixed with a tagged union for
>> the type (so an extra byte for every prop).
> Yeah, it's not so simple.  As things stand this would imply a change
> to (1), which as noted probably isn't really feasible.  Plus, just a
> one-byte type per property wouldn't cut it: some bindings have complex
> encoded values in properties that are a mixture of integers and
> strings and so forth.
>
>> With more and more emphasis on runtime devicetree [0], it would be great to
>> have a design that allows tackling more complex use cases.
>>
>> [0]: https://lpc.events/event/18/contributions/1696/
> Oof.   I don't think overlays as currently designed are a great choice
> for hotplug: overlays essentially allow rewriting any part of the
> whole device tree, which isn't a great model for a hotplug bus.  A
> long time ago some ideas were floated to define "connectors" in the
> device tree that more specifically described a way things could be
> plugged in that could cover several busses / interrupt controllers /
> etc.  That would provide a more structured way of describing plug in
> devices that can actually validate what they can do.
>
> Of course, such a scheme is a lot more work to design and implement,
> which is why the simple hack of overlays have become dominant instead.

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

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.


[0]: 
https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/

[1]: https://lpc.events/event/18/contributions/1696/


Ayush Singh

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

[David Gibson]

[-- 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 --]

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

[Geert Uytterhoeven]

Hi David,

On Mon, Sep 23, 2024 at 5:41 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
> 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 2: /aliases

Does the (Linux) DT overlay code support updating aliases?
Last time I needed that (almost a decade ago), it did not.

"[PATCH/RFC 0/3] of/overlay: Update aliases when added or removed"[1]
was never applied, due to me never getting to all of the requested changes.

[1] https://lore.kernel.org/all/1435675876-2159-1-git-send-email-geert+renesas@glider.be/

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

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

[David Gibson]

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

On Mon, Sep 23, 2024 at 10:22:03AM +0200, Geert Uytterhoeven wrote:
> Hi David,
> 
> On Mon, Sep 23, 2024 at 5:41 AM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > 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 2: /aliases
> 
> Does the (Linux) DT overlay code support updating aliases?
> Last time I needed that (almost a decade ago), it did not.

Huh.  I hadn't realised the kernel kept a separate cache of aliases
that wasn't updated.  Assuming that's still the case, that would
complicate matters a bit.

> "[PATCH/RFC 0/3] of/overlay: Update aliases when added or removed"[1]
> was never applied, due to me never getting to all of the requested changes.
> 
> [1] https://lore.kernel.org/all/1435675876-2159-1-git-send-email-geert+renesas@glider.be/
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

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

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

[Geert Uytterhoeven]

Hi David,

On Mon, Sep 23, 2024 at 10:41 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Mon, Sep 23, 2024 at 10:22:03AM +0200, Geert Uytterhoeven wrote:
> > On Mon, Sep 23, 2024 at 5:41 AM David Gibson
> > <david@gibson.dropbear.id.au> wrote:
> > > 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 2: /aliases
> >
> > Does the (Linux) DT overlay code support updating aliases?
> > Last time I needed that (almost a decade ago), it did not.
>
> Huh.  I hadn't realised the kernel kept a separate cache of aliases
> that wasn't updated.  Assuming that's still the case, that would
> complicate matters a bit.

Indeed.

> > "[PATCH/RFC 0/3] of/overlay: Update aliases when added or removed"[1]
> > was never applied, due to me never getting to all of the requested changes.
> >
> > [1] https://lore.kernel.org/all/1435675876-2159-1-git-send-email-geert+renesas@glider.be/

FTR, if anyone is interested in this, IIRC I have an updated version
somewhere that did implement some of the requested changes. Just ask.

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

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

[David Gibson]

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

On Mon, Sep 23, 2024 at 11:12:02AM +0200, Geert Uytterhoeven wrote:
> Hi David,
> 
> On Mon, Sep 23, 2024 at 10:41 AM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > On Mon, Sep 23, 2024 at 10:22:03AM +0200, Geert Uytterhoeven wrote:
> > > On Mon, Sep 23, 2024 at 5:41 AM David Gibson
> > > <david@gibson.dropbear.id.au> wrote:
> > > > 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 2: /aliases
> > >
> > > Does the (Linux) DT overlay code support updating aliases?
> > > Last time I needed that (almost a decade ago), it did not.
> >
> > Huh.  I hadn't realised the kernel kept a separate cache of aliases
> > that wasn't updated.  Assuming that's still the case, that would
> > complicate matters a bit.
> 
> Indeed.

Actually, in a sense this is just an aspect of a more general thing:
libfdt's is not the only relevant implementation of overlays.  If you
want to extend what overlays can do, you need to consider the kernel
implementation too.

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

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

[Ayush Singh]

On 9/23/24 15:18, David Gibson wrote:

> On Mon, Sep 23, 2024 at 11:12:02AM +0200, Geert Uytterhoeven wrote:
>> Hi David,
>>
>> On Mon, Sep 23, 2024 at 10:41 AM David Gibson
>> <david@gibson.dropbear.id.au> wrote:
>>> On Mon, Sep 23, 2024 at 10:22:03AM +0200, Geert Uytterhoeven wrote:
>>>> On Mon, Sep 23, 2024 at 5:41 AM David Gibson
>>>> <david@gibson.dropbear.id.au> wrote:
>>>>> 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 2: /aliases
>>>> Does the (Linux) DT overlay code support updating aliases?
>>>> Last time I needed that (almost a decade ago), it did not.
>>> Huh.  I hadn't realised the kernel kept a separate cache of aliases
>>> that wasn't updated.  Assuming that's still the case, that would
>>> complicate matters a bit.
>> Indeed.
> Actually, in a sense this is just an aspect of a more general thing:
> libfdt's is not the only relevant implementation of overlays.  If you
> want to extend what overlays can do, you need to consider the kernel
> implementation too.
>

So, I don't think we can go the aliases route. I posted an updated  
of_alias patch [0], but as Rob pointed out:

```

Drivers use the non-existent alias numbers for instances without an
alias. So what happens if an index is already in use and then an
overlay uses the same index.

I don't see how this can work reliably unless the alias name doesn't
exist in the base DT.

```


Not really sure how alias overloading can be supported in kernel without 
breaking existing drivers.

Overlays are starting to feel more like a hack the longer I work on this.


I guess I can try out implementing `foo = &bar;` approach and see how 
that goes.


[0]: 
https://lore.kernel.org/all/20241110-of-alias-v2-0-16da9844a93e@beagleboard.org/T/#t


Ayush Singh

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

[Ayush Singh]

On 9/23/24 14:42, Geert Uytterhoeven wrote:

> Hi David,
>
> On Mon, Sep 23, 2024 at 10:41 AM David Gibson
> <david@gibson.dropbear.id.au> wrote:
>> On Mon, Sep 23, 2024 at 10:22:03AM +0200, Geert Uytterhoeven wrote:
>>> On Mon, Sep 23, 2024 at 5:41 AM David Gibson
>>> <david@gibson.dropbear.id.au> wrote:
>>>> 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 2: /aliases
>>> Does the (Linux) DT overlay code support updating aliases?
>>> Last time I needed that (almost a decade ago), it did not.
>> Huh.  I hadn't realised the kernel kept a separate cache of aliases
>> that wasn't updated.  Assuming that's still the case, that would
>> complicate matters a bit.
> Indeed.
>
>>> "[PATCH/RFC 0/3] of/overlay: Update aliases when added or removed"[1]
>>> was never applied, due to me never getting to all of the requested changes.
>>>
>>> [1] https://lore.kernel.org/all/1435675876-2159-1-git-send-email-geert+renesas@glider.be/
> FTR, if anyone is interested in this, IIRC I have an updated version
> somewhere that did implement some of the requested changes. Just ask.
>
> Thanks!
>
>
> Gr{oetje,eeting}s,
>
>                          Geert
>
Hi, I would be interested in picking up that patch series and getting it 
merged. Regardless of whether we use alias for mikroBUS, I think it is 
high time the devicetree support is improved in Linux kernel.


Ayush Singh

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

[Ayush Singh]

On 9/23/24 09:11, David Gibson wrote:

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

Well, my previous attempts at not using devicetree for the addon boards 
was met with 2 main arguments:

1. Hardware should be described with devicetree.

2. There can exist a fairly complicated addon board which will not work 
without devicetree.

Additionally, it was mentioned that if something is missing from the 
devicetree, I should look at extending device trees instead of trying to 
bypass it. So, here we are.

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


Do you have a link to the "connector binding" proposal you are 
mentioning here? I really believe having a better way to support such 
connectors is really important for embedded systems. And I am okay with 
adding any missing bits to make it a reality.

With `PREEMPT_RT` patches being merged, it is probably a good time to 
improve embedded linux.


Ayush Singh

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

[David Gibson]

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

On Tue, Sep 24, 2024 at 12:11:36PM +0530, Ayush Singh wrote:
> On 9/23/24 09:11, David Gibson wrote:
> > On Fri, Sep 20, 2024 at 10:04:56PM +0530, Ayush Singh wrote:
> > > On 9/18/24 08:06, David Gibson wrote:
[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.
> 
> Well, my previous attempts at not using devicetree for the addon boards was
> met with 2 main arguments:
> 
> 1. Hardware should be described with devicetree.
>
> 2. There can exist a fairly complicated addon board which will not work
> without devicetree.
> 
> Additionally, it was mentioned that if something is missing from the
> devicetree, I should look at extending device trees instead of trying to
> bypass it. So, here we are.

Absolutely.  This isn't about not using a devicetree, it's about the
mechanism for updating the devicetree with plug in components.
Overlays, as they're currently specced, are simple and easy... but
also crude, limited and sloppily designed.


> 
> > > 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.
> 
> 
> Do you have a link to the "connector binding" proposal you are mentioning
> here? I really believe having a better way to support such connectors is
> really important for embedded systems. And I am okay with adding any missing
> bits to make it a reality.
> 
> With `PREEMPT_RT` patches being merged, it is probably a good time to
> improve embedded linux.

I don't think there was ever a proposal written up as such.  It was
just an idea floating around the mailing lists.  I did manage to dig
up what were meant to be some illustrative examples of the idea.
Alas, without any explanatory notes.  It was last modified in 2016,
but let's see what I can remember in terms of context.  Note that all
of the below was a quick draft - it would certainly need polish and
all syntax is negotiable.  In particular the use of the /plugin/
keyword might not be compatible with its current use for overlays, so
that would probably need changing.


The idea is that a base board could define specific "connectors",
which could describe what buses / pins / interrupts / whatever were
exposed on that connector.  Each connector instance had some local
aliases referencing the nodes in the base board the connector could
alter.

So, a board with a "widget" socket which exposes two interrupt lines,
an I2C bus and an MMIO bus might look roughly like this:

/dts-v1/;

/ {
	compatible = "foo,oldboard";
	ranges;
	soc@... {
		ranges;
		mmio: mmio-bus@... {
			#address-cells = <2>;
			#size-cells = <2>;
			ranges;
		};
		i2c: i2c@... {
		};
		intc: intc@... {
			#interrupt-cells = <2>;
		};
	};

	connectors {
		widget1 {
			compatible = "foo,widget-socket";
			w1_irqs: irqs {
				interrupt-controller;
				#address-cells = <0>;
				#interrupt-cells = <1>;
				interrupt-map-mask = <0xffffffff>;
				interrupt-map = <
					0 &intc 7 0
					1 &intc 8 0
				>;
			};
			aliases = {
				i2c = &i2c;
				intc = &w1_irqs;
				mmio = &mmio;
			};
		};
	};
};


A later version of the board might expose two widget sockets that are
backwards compatible with the original widget but also add some new
features.  It might look like this:

/dts-v1/;

/ {
	compatible = "foo,newboard";
	ranges;
	soc@... {
		ranges;	
		mmio: mmio-bus@... {
			#address-cells = <2>;
			#size-cells = <2>;
			ranges;
		};
		i2c0: i2c@... {
		};
		i2c1: i2c@... {
		};
		intc: intc@... {
		};
	};

	connectors {
		widget1 {
			compatible = "foo,widget-socket-v2", "foo,widget-socket";
			w1_irqs: irqs {
				interrupt-controller;
				#address-cells = <0>;
				#interrupt-cells = <1>;
				interrupt-map-mask = <0xffffffff>;
				interrupt-map = <
					0 &intc 17 0
					1 &intc 8 0
				>;
			};
			aliases = {
				i2c = &i2c0;
				intc = &w1_irqs;
				mmio = &mmio;
			};
		};
		widget2 {
			compatible = "foo,widget-socket-v2", "foo,widget-socket";
			w2_irqs: irqs {
				interrupt-controller;
				#address-cells = <0>;
				#interrupt-cells = <1>;
				interrupt-map-mask = <0xffffffff>;
				interrupt-map = <
					0 &intc 9 0
					1 &intc 10 0
				>;
			};
			aliases = {
				i2c = &i2c1;
				widget-irqs = &w2_irqs;
				mmio = &mmio;
			};
		};
	};
};


A plugin device tree describing something which plugs into a widget
socket might look like this.  Note that it specifies the *type* of
socket it goes into ("foo,widget-socket"), but not the specific
instance of a socket it goes into.  When plugging this into a
"foo,newboard" you'd have to specify whether this is attaching to the
widget1 or widget2 socket.

/dts-v1/;

/plugin/ foo,widget-socket {
	compatible = "foo,whirligig-widget";
};

&i2c {
	whirligig-controller@... {
		...
		interrupt-parent = <&widget-irqs>;
		interrupts = <0>;
	};
};

A plugin could also expose a secondary connector.  Here's one which
adds a "superbus" controller mapped into the parent's MMIO bus.

/dts-v1/;

/plugin/ foo,widget-socket-v2 {
	compatible = "foo,superduper-widget};

	connectors {
		compatible = "foo,super-socket";
		aliases {
			superbus = &superbus;
		};	
	};
};

&mmio {
	superbus: super-bridge@100000000 {
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0x0  0xabcd0000 0x12345600  0x100>;
	};
};

&i2c {
	super-controller@... {
		...
	};
	duper-controller@... {
	};
};


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

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

[Geert Uytterhoeven]

On Wed, Sep 25, 2024 at 9:28 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Tue, Sep 24, 2024 at 12:11:36PM +0530, Ayush Singh wrote:
> > On 9/23/24 09:11, David Gibson wrote:
> > > 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.
> >
> > Do you have a link to the "connector binding" proposal you are mentioning
> > here? I really believe having a better way to support such connectors is
> > really important for embedded systems. And I am okay with adding any missing
> > bits to make it a reality.
> >
> > With `PREEMPT_RT` patches being merged, it is probably a good time to
> > improve embedded linux.
>
> I don't think there was ever a proposal written up as such.  It was
> just an idea floating around the mailing lists.  I did manage to dig
> up what were meant to be some illustrative examples of the idea.
> Alas, without any explanatory notes.  It was last modified in 2016,
> but let's see what I can remember in terms of context.  Note that all
> of the below was a quick draft - it would certainly need polish and
> all syntax is negotiable.  In particular the use of the /plugin/
> keyword might not be compatible with its current use for overlays, so
> that would probably need changing.
>
>
> The idea is that a base board could define specific "connectors",
> which could describe what buses / pins / interrupts / whatever were
> exposed on that connector.  Each connector instance had some local
> aliases referencing the nodes in the base board the connector could
> alter.

Several people are working on things related to this.
Please have a look at https://lpc.events/event/18/contributions/1696/.
I don't know the video is online yet.

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

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

[David Gibson]

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

On Wed, Sep 25, 2024 at 09:58:06AM +0200, Geert Uytterhoeven wrote:
> On Wed, Sep 25, 2024 at 9:28 AM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > On Tue, Sep 24, 2024 at 12:11:36PM +0530, Ayush Singh wrote:
> > > On 9/23/24 09:11, David Gibson wrote:
> > > > 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.
> > >
> > > Do you have a link to the "connector binding" proposal you are mentioning
> > > here? I really believe having a better way to support such connectors is
> > > really important for embedded systems. And I am okay with adding any missing
> > > bits to make it a reality.
> > >
> > > With `PREEMPT_RT` patches being merged, it is probably a good time to
> > > improve embedded linux.
> >
> > I don't think there was ever a proposal written up as such.  It was
> > just an idea floating around the mailing lists.  I did manage to dig
> > up what were meant to be some illustrative examples of the idea.
> > Alas, without any explanatory notes.  It was last modified in 2016,
> > but let's see what I can remember in terms of context.  Note that all
> > of the below was a quick draft - it would certainly need polish and
> > all syntax is negotiable.  In particular the use of the /plugin/
> > keyword might not be compatible with its current use for overlays, so
> > that would probably need changing.
> >
> >
> > The idea is that a base board could define specific "connectors",
> > which could describe what buses / pins / interrupts / whatever were
> > exposed on that connector.  Each connector instance had some local
> > aliases referencing the nodes in the base board the connector could
> > alter.
> 
> Several people are working on things related to this.
> Please have a look at https://lpc.events/event/18/contributions/1696/.
> I don't know the video is online yet.

Ayush linked to that earlier in the thread.  AFAICT it still seems to
be focussing on using overlays in their existing formulation, rather
than redesigning the plugin mechanism.

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

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

[Ayush Singh]

On 9/25/24 12:58, David Gibson wrote:

> On Tue, Sep 24, 2024 at 12:11:36PM +0530, Ayush Singh wrote:
>> On 9/23/24 09:11, David Gibson wrote:
>>> On Fri, Sep 20, 2024 at 10:04:56PM +0530, Ayush Singh wrote:
>>>> On 9/18/24 08:06, David Gibson wrote:
> [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.
>> Well, my previous attempts at not using devicetree for the addon boards was
>> met with 2 main arguments:
>>
>> 1. Hardware should be described with devicetree.
>>
>> 2. There can exist a fairly complicated addon board which will not work
>> without devicetree.
>>
>> Additionally, it was mentioned that if something is missing from the
>> devicetree, I should look at extending device trees instead of trying to
>> bypass it. So, here we are.
> Absolutely.  This isn't about not using a devicetree, it's about the
> mechanism for updating the devicetree with plug in components.
> Overlays, as they're currently specced, are simple and easy... but
> also crude, limited and sloppily designed.
>
>
>>>> 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.
>>
>> Do you have a link to the "connector binding" proposal you are mentioning
>> here? I really believe having a better way to support such connectors is
>> really important for embedded systems. And I am okay with adding any missing
>> bits to make it a reality.
>>
>> With `PREEMPT_RT` patches being merged, it is probably a good time to
>> improve embedded linux.
> I don't think there was ever a proposal written up as such.  It was
> just an idea floating around the mailing lists.  I did manage to dig
> up what were meant to be some illustrative examples of the idea.
> Alas, without any explanatory notes.  It was last modified in 2016,
> but let's see what I can remember in terms of context.  Note that all
> of the below was a quick draft - it would certainly need polish and
> all syntax is negotiable.  In particular the use of the /plugin/
> keyword might not be compatible with its current use for overlays, so
> that would probably need changing.

Thanks. I also personally would be interested in an approach that can 
avoid symbols. The reason being I would like to be able to use the same 
addon board overlays to support mikroBUS on Zephyr.

I also like the prospect of doing connector versioning.

> The idea is that a base board could define specific "connectors",
> which could describe what buses / pins / interrupts / whatever were
> exposed on that connector.  Each connector instance had some local
> aliases referencing the nodes in the base board the connector could
> alter.
>
> So, a board with a "widget" socket which exposes two interrupt lines,
> an I2C bus and an MMIO bus might look roughly like this:
>
> /dts-v1/;
>
> / {
> 	compatible = "foo,oldboard";
> 	ranges;
> 	soc@... {
> 		ranges;
> 		mmio: mmio-bus@... {
> 			#address-cells = <2>;
> 			#size-cells = <2>;
> 			ranges;
> 		};
> 		i2c: i2c@... {
> 		};
> 		intc: intc@... {
> 			#interrupt-cells = <2>;
> 		};
> 	};
>
> 	connectors {
> 		widget1 {
> 			compatible = "foo,widget-socket";
> 			w1_irqs: irqs {
> 				interrupt-controller;
> 				#address-cells = <0>;
> 				#interrupt-cells = <1>;
> 				interrupt-map-mask = <0xffffffff>;
> 				interrupt-map = <
> 					0 &intc 7 0
> 					1 &intc 8 0
> 				>;
> 			};
> 			aliases = {
> 				i2c = &i2c;
> 				intc = &w1_irqs;
> 				mmio = &mmio;
> 			};
> 		};
> 	};
> };
>
>
> A later version of the board might expose two widget sockets that are
> backwards compatible with the original widget but also add some new
> features.  It might look like this:
>
> /dts-v1/;
>
> / {
> 	compatible = "foo,newboard";
> 	ranges;
> 	soc@... {
> 		ranges;	
> 		mmio: mmio-bus@... {
> 			#address-cells = <2>;
> 			#size-cells = <2>;
> 			ranges;
> 		};
> 		i2c0: i2c@... {
> 		};
> 		i2c1: i2c@... {
> 		};
> 		intc: intc@... {
> 		};
> 	};
>
> 	connectors {
> 		widget1 {
> 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> 			w1_irqs: irqs {
> 				interrupt-controller;
> 				#address-cells = <0>;
> 				#interrupt-cells = <1>;
> 				interrupt-map-mask = <0xffffffff>;
> 				interrupt-map = <
> 					0 &intc 17 0
> 					1 &intc 8 0
> 				>;
> 			};
> 			aliases = {
> 				i2c = &i2c0;
> 				intc = &w1_irqs;
> 				mmio = &mmio;
> 			};
> 		};
> 		widget2 {
> 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> 			w2_irqs: irqs {
> 				interrupt-controller;
> 				#address-cells = <0>;
> 				#interrupt-cells = <1>;
> 				interrupt-map-mask = <0xffffffff>;
> 				interrupt-map = <
> 					0 &intc 9 0
> 					1 &intc 10 0
> 				>;
> 			};
> 			aliases = {
> 				i2c = &i2c1;
> 				widget-irqs = &w2_irqs;
> 				mmio = &mmio;
> 			};
> 		};
> 	};
> };
>
>
> A plugin device tree describing something which plugs into a widget
> socket might look like this.  Note that it specifies the *type* of
> socket it goes into ("foo,widget-socket"), but not the specific
> instance of a socket it goes into.  When plugging this into a
> "foo,newboard" you'd have to specify whether this is attaching to the
> widget1 or widget2 socket.
>
> /dts-v1/;
>
> /plugin/ foo,widget-socket {
> 	compatible = "foo,whirligig-widget";
> };
>
> &i2c {
> 	whirligig-controller@... {
> 		...
> 		interrupt-parent = <&widget-irqs>;
> 		interrupts = <0>;
> 	};
> };

I do have a few questions regarding this approach:

1. How do you propose selecting the connector should be done? We can 
rely on the connector driver for this, but the symbols [0] based 
approach does have an interesting benefit. It can work with applying 
overlays from uboot, or well any other static method where we already 
know which board we want to use. This is quite useful in embedded context.

2. Where would `compatible = "foo,whirligig-widget";` be applied to? 
Will it override the compatible property of the `widget{n}`? If I am 
right, then I guess the connector will have a single driver instead of 
each widget having a unique driver attached to it.

> A plugin could also expose a secondary connector.  Here's one which
> adds a "superbus" controller mapped into the parent's MMIO bus.
>
> /dts-v1/;
>
> /plugin/ foo,widget-socket-v2 {
> 	compatible = "foo,superduper-widget};
>
> 	connectors {
> 		compatible = "foo,super-socket";
> 		aliases {
> 			superbus = &superbus;
> 		};	
> 	};
> };
>
> &mmio {
> 	superbus: super-bridge@100000000 {
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		ranges = <0x0  0xabcd0000 0x12345600  0x100>;
> 	};
> };
>
> &i2c {
> 	super-controller@... {
> 		...
> 	};
> 	duper-controller@... {
> 	};
> };
>
>

[0]: 
https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/


Ayush Singh

[PATCH 2/2] tests: Add test for symbol resolution

[Ayush Singh]

A simple test for both phandle symbols and string symbols. Also tests
chaining symbol overlays.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 tests/overlay_overlay_symbols1.dts     | 12 ++++++++++++
 tests/overlay_overlay_symbols2.dts     |  9 +++++++++
 tests/overlay_overlay_symbols_user.dts | 26 ++++++++++++++++++++++++++
 tests/run_tests.sh                     | 19 +++++++++++++++++++
 4 files changed, 66 insertions(+)

diff --git a/tests/overlay_overlay_symbols1.dts b/tests/overlay_overlay_symbols1.dts
new file mode 100644
index 0000000..bd74242
--- /dev/null
+++ b/tests/overlay_overlay_symbols1.dts
@@ -0,0 +1,12 @@
+/dts-v1/;
+/plugin/;
+
+&{/} {
+	__symbols__ {
+    TEST_STR = "/test-node";
+    TEST_PHANDLE = <&test>;
+
+    SUBTEST_STR = "/test-node/sub-test-node";
+    SUBTEST_PHANDLE = <&subtest>;
+	};
+};
diff --git a/tests/overlay_overlay_symbols2.dts b/tests/overlay_overlay_symbols2.dts
new file mode 100644
index 0000000..b8da96b
--- /dev/null
+++ b/tests/overlay_overlay_symbols2.dts
@@ -0,0 +1,9 @@
+/dts-v1/;
+/plugin/;
+
+&{/} {
+	__symbols__ {
+		TEST_CHAIN = <&TEST_PHANDLE>;
+		SUBTEST_CHAIN = <&SUBTEST_PHANDLE>;
+	};
+};
diff --git a/tests/overlay_overlay_symbols_user.dts b/tests/overlay_overlay_symbols_user.dts
new file mode 100644
index 0000000..4da2136
--- /dev/null
+++ b/tests/overlay_overlay_symbols_user.dts
@@ -0,0 +1,26 @@
+/dts-v1/;
+/plugin/;
+
+&TEST_STR {
+  str-prop = "test-node";
+};
+
+&TEST_PHANDLE {
+  phandle-prop = "test-node";
+};
+
+&TEST_CHAIN {
+  chain-prop = "test-node";
+};
+
+&SUBTEST_STR {
+  str-prop = "subtest-node";
+};
+
+&SUBTEST_PHANDLE {
+  phandle-prop = "subtest-node";
+};
+
+&SUBTEST_CHAIN {
+  chain-prop = "subtest-node";
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 937b128..56f8d0d 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -1018,6 +1018,25 @@ fdtoverlay_tests() {
     # test that the new property is installed
     run_fdtoverlay_test foobar "/test-node" "test-str-property" "-ts" ${basedtb} ${targetdtb} ${overlaydtb}
 
+    symbol1_overlay="$SRCDIR/overlay_overlay_symbols1.dts"
+    symbol1_overlaydtbo=overlay_overlay_symbols1.fdtoverlay.test.dtb
+    symbol2_overlay="$SRCDIR/overlay_overlay_symbols2.dts"
+    symbol2_overlaydtbo=overlay_overlay_symbols2.fdtoverlay.test.dtb
+    symbol_user_overlay="$SRCDIR/overlay_overlay_symbols_user.dts"
+    symbol_user_overlaydtbo=overlay_overlay_symbols_user.fdtoverlay.test.dtb
+
+    # test overlay symbol resolution
+    run_dtc_test -@ -I dts -O dtb -o $symbol1_overlaydtbo $symbol1_overlay
+    run_dtc_test -@ -I dts -O dtb -o $symbol2_overlaydtbo $symbol2_overlay
+    run_dtc_test -@ -I dts -O dtb -o $symbol_user_overlaydtbo $symbol_user_overlay
+
+    run_fdtoverlay_test test-node "/test-node" "str-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
+    run_fdtoverlay_test test-node "/test-node" "phandle-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
+    run_fdtoverlay_test test-node "/test-node" "chain-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
+    run_fdtoverlay_test subtest-node "/test-node/sub-test-node" "str-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
+    run_fdtoverlay_test subtest-node "/test-node/sub-test-node" "phandle-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
+    run_fdtoverlay_test subtest-node "/test-node/sub-test-node" "chain-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
+
     stacked_base="$SRCDIR/stacked_overlay_base.dts"
     stacked_basedtb=stacked_overlay_base.fdtoverlay.test.dtb
     stacked_bar="$SRCDIR/stacked_overlay_bar.dts"

-- 
2.46.0

Re: [PATCH 2/2] tests: Add test for symbol resolution

[Andrew Davis]

On 9/2/24 7:17 AM, Ayush Singh wrote:
> A simple test for both phandle symbols and string symbols. Also tests
> chaining symbol overlays.
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>   tests/overlay_overlay_symbols1.dts     | 12 ++++++++++++
>   tests/overlay_overlay_symbols2.dts     |  9 +++++++++
>   tests/overlay_overlay_symbols_user.dts | 26 ++++++++++++++++++++++++++
>   tests/run_tests.sh                     | 19 +++++++++++++++++++
>   4 files changed, 66 insertions(+)
> 
> diff --git a/tests/overlay_overlay_symbols1.dts b/tests/overlay_overlay_symbols1.dts
> new file mode 100644
> index 0000000..bd74242
> --- /dev/null
> +++ b/tests/overlay_overlay_symbols1.dts
> @@ -0,0 +1,12 @@
> +/dts-v1/;
> +/plugin/;
> +
> +&{/} {
> +	__symbols__ {
> +    TEST_STR = "/test-node";

Looks like you are mixing tabs and spaces. Probably need to fix your
editor to treat .dts files like .c and use tabs for indent.

Andrew

> +    TEST_PHANDLE = <&test>;
> +
> +    SUBTEST_STR = "/test-node/sub-test-node";
> +    SUBTEST_PHANDLE = <&subtest>;
> +	};
> +};
> diff --git a/tests/overlay_overlay_symbols2.dts b/tests/overlay_overlay_symbols2.dts
> new file mode 100644
> index 0000000..b8da96b
> --- /dev/null
> +++ b/tests/overlay_overlay_symbols2.dts
> @@ -0,0 +1,9 @@
> +/dts-v1/;
> +/plugin/;
> +
> +&{/} {
> +	__symbols__ {
> +		TEST_CHAIN = <&TEST_PHANDLE>;
> +		SUBTEST_CHAIN = <&SUBTEST_PHANDLE>;
> +	};
> +};
> diff --git a/tests/overlay_overlay_symbols_user.dts b/tests/overlay_overlay_symbols_user.dts
> new file mode 100644
> index 0000000..4da2136
> --- /dev/null
> +++ b/tests/overlay_overlay_symbols_user.dts
> @@ -0,0 +1,26 @@
> +/dts-v1/;
> +/plugin/;
> +
> +&TEST_STR {
> +  str-prop = "test-node";
> +};
> +
> +&TEST_PHANDLE {
> +  phandle-prop = "test-node";
> +};
> +
> +&TEST_CHAIN {
> +  chain-prop = "test-node";
> +};
> +
> +&SUBTEST_STR {
> +  str-prop = "subtest-node";
> +};
> +
> +&SUBTEST_PHANDLE {
> +  phandle-prop = "subtest-node";
> +};
> +
> +&SUBTEST_CHAIN {
> +  chain-prop = "subtest-node";
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 937b128..56f8d0d 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -1018,6 +1018,25 @@ fdtoverlay_tests() {
>       # test that the new property is installed
>       run_fdtoverlay_test foobar "/test-node" "test-str-property" "-ts" ${basedtb} ${targetdtb} ${overlaydtb}
>   
> +    symbol1_overlay="$SRCDIR/overlay_overlay_symbols1.dts"
> +    symbol1_overlaydtbo=overlay_overlay_symbols1.fdtoverlay.test.dtb
> +    symbol2_overlay="$SRCDIR/overlay_overlay_symbols2.dts"
> +    symbol2_overlaydtbo=overlay_overlay_symbols2.fdtoverlay.test.dtb
> +    symbol_user_overlay="$SRCDIR/overlay_overlay_symbols_user.dts"
> +    symbol_user_overlaydtbo=overlay_overlay_symbols_user.fdtoverlay.test.dtb
> +
> +    # test overlay symbol resolution
> +    run_dtc_test -@ -I dts -O dtb -o $symbol1_overlaydtbo $symbol1_overlay
> +    run_dtc_test -@ -I dts -O dtb -o $symbol2_overlaydtbo $symbol2_overlay
> +    run_dtc_test -@ -I dts -O dtb -o $symbol_user_overlaydtbo $symbol_user_overlay
> +
> +    run_fdtoverlay_test test-node "/test-node" "str-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
> +    run_fdtoverlay_test test-node "/test-node" "phandle-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
> +    run_fdtoverlay_test test-node "/test-node" "chain-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
> +    run_fdtoverlay_test subtest-node "/test-node/sub-test-node" "str-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
> +    run_fdtoverlay_test subtest-node "/test-node/sub-test-node" "phandle-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
> +    run_fdtoverlay_test subtest-node "/test-node/sub-test-node" "chain-prop" "-ts" ${basedtb} ${targetdtb} ${symbol1_overlaydtbo} ${symbol2_overlaydtbo} ${symbol_user_overlaydtbo}
> +
>       stacked_base="$SRCDIR/stacked_overlay_base.dts"
>       stacked_basedtb=stacked_overlay_base.fdtoverlay.test.dtb
>       stacked_bar="$SRCDIR/stacked_overlay_bar.dts"
> 

Re: [PATCH 0/2] Add support for phandle in symbols

[Andrew Davis]

On 9/2/24 7:17 AM, 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].
> 
> Patch 2 containing tests demonstrates how this looks in practice.
> 
> Open Items:
> 
> 1. Phandle vs String identification:
>     I am not sure how to distinguish between symbols that are paths and
>     phandles. Currently I am checking the proplen and if the first byte
>     is '/'. However, this can cause problems if the number of phandles in
>     dt exceed 0x2f000000 or 788,529,152.
> 

Ha, that is clever, never thought about the count ending up in the
ascii space :)

Does the blob format being big-endian affect this? Would mean the
slightly more realistic phandle count of 47 would run into this..

Probably not an issue, the below length check would save us.

>     Alternatively, if we can be sure that there will be no node whose
>     path is of length = 4, i.e. a node name of 2 characters ('\' and NULL
>     already take up 2 characters), we can avoid the '/' check.
> 

I also ran into the issue of the fdt not having type info. My solution
was the same as yours, quick and simple len==4 check.

We can disassemble fdt back into source, I thought about how that
works without type info, scanning the code it seems dtc also just
guesses the same way we do here (see guess_value_type()).

Might have been better if all properties in __symbols__ were simply
phandles from the start. Having full paths string for every symbol
adds most of the bloat when enabling symbols (adds ~10% to each fdt
file on average in kernel last time I ran an audit), phandle-only
would solve that.

Andrew

> [1]: https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> 
> ---
> Ayush Singh (2):
>        libfdt: overlay: Allow resolving phandle symbols
>        tests: Add test for symbol resolution
> 
>   libfdt/fdt_overlay.c                   | 16 ++++++++++------
>   tests/overlay_overlay_symbols1.dts     | 12 ++++++++++++
>   tests/overlay_overlay_symbols2.dts     |  9 +++++++++
>   tests/overlay_overlay_symbols_user.dts | 26 ++++++++++++++++++++++++++
>   tests/run_tests.sh                     | 19 +++++++++++++++++++
>   5 files changed, 76 insertions(+), 6 deletions(-)
> ---
> base-commit: 99031e3a4a6e479466ae795790b44727434ca27d
> change-id: 20240829-symbol-phandle-9d1e0489c32a
> 
> Best regards,