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

[Ayush Singh <ayush@beagleboard.org>]

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