Re: [PATCH v3] Add new `export-symbols` node

On 4/14/25 22:04, Andrew Davis wrote:
> On 4/14/25 9:46 AM, Herve Codina wrote:
>> Hi Ayush, David,
>>
>> On Sat, 12 Apr 2025 00:19:16 +0530
>> Ayush Singh <[email protected]> wrote:
>>
>>> On 4/11/25 23:09, Andrew Davis wrote:
>>>
>>>> On 4/11/25 3:00 AM, Ayush Singh wrote:
>>>>> `export-symbols` is designed to be a local replacement of global
>>>>> `__symbols__` allowing nodes to define aliases to nodes in a tree, 
>>>>> which
>>>>> will take precedence over the aliases defined in the global
>>>>> `__symbols__`.
>>>>>
>>>>> Having a way to allow node local aliases helps in usecases such as
>>>>> connectors and addon-boards, by allowing decoupling of
>>>>> overlays/devicetree nodes of addon-board from the base connector.
>>>>>
>>>>> Reviewed-by: Herve Codina <[email protected]>
>>>>> Reviewed-by: Luca Ceresoli <[email protected]>
>>>>> Signed-off-by: Ayush Singh <[email protected]>
>>>>> ---
>>>>> This patch series follows the initial RFC [9] sent a few weeks ago. I
>>>>> will be reiterating the RFC here for anyone who might be seeing 
>>>>> this the
>>>>> first time, since there was not much feedback in that thread.
>>>>
>>>> I think this is a useful tool in the effort to build a complete
>>>> addon-board
>>>> solution. But I'm still missing how it all fits together, do you have
>>>> a real
>>>> working overlay making use of this somewhere I could take a look at?
>>>> Maybe
>>>> an overlay for one of the addon-boards you list below (one of the
>>>> BeagleCapes
>>>> for instance).
>>>>
>>
>> We (me and Luca) have a working device-tree and overlay.
>>
>> Our base device tree is the following (simplified but relevant part for
>> this topic are available):
>> / {
>>     ...
>>
>>     addon_connector0: addon-connector0 {
>>         compatible = "gehc,sunhv1-addon-connector";
>>
>>         /*
>>          * addon-connector node is a nexus node
>>          *  - 2 interupt lines are wired to the connector
>>          *  - 1 gpio line is wired to the connector
>>          *  - 1 PWM is wired to the connector
>>          */
>>         #interrupt-cells = <2>;
>>         #address-cells = <0>;
>>         interrupt-map = <0 IRQ_TYPE_LEVEL_LOW    &gpio4 1 
>> IRQ_TYPE_LEVEL_LOW>,
>>                 <0 IRQ_TYPE_EDGE_FALLING &gpio4 1 
>> IRQ_TYPE_EDGE_FALLING>,
>>                 <1 IRQ_TYPE_LEVEL_LOW    &i2c3_mux 1 1 
>> IRQ_TYPE_LEVEL_LOW>,
>>                 <1 IRQ_TYPE_EDGE_FALLING &i2c3_mux 1 1 
>> IRQ_TYPE_EDGE_FALLING>;
>>         #gpio-cells = <2>;
>>         gpio-map-mask = <0xf 0x0>;
>>         gpio-map-pass-thru = <0x0 0xf>;
>>         gpio-map = <0 0 &gpio4 1 0>;
>
> This isn't a thing, or do you plan to add nexus nodes / map bindings 
> for all possible
> enumerated items in DT? Not sure this will scale well :/
>
>>         #pwm-cells = <3>;
>>         pwm-map-mask = <0xffffffff 0 0>;
>>         pwm-map-pass-thru = <0 0xffffffff 0xffffffff>;
>>         pwm-map = <0 0 0 &pwm1 0 57000 0>;
>>
>>
>>         devices {
>
> What is this node? I'm assuming this is something the "connector 
> driver" for
> this connector will look for and populate subnodes? What about simple 
> connectors
> where no driver should be needed?

Well, I don't think there are many connectors that do not need a driver. 
Or well, as long as the connector wants to use nexus nodes, it needs a 
driver. At least for GPIOs and PWMs, nexus nodes work pretty well.

The reason nexus nodes cannot be used without driver is well they appear 
as gpio-controller, which means without a driver, any device using them 
enters differed probing.

>
>>             /*
>>              * 'no bus' devices such as fixed-regulators or
>>              * fixed-clocks will be added in this node by the
>>              * overlay.
>>              */
>>             #address-cells = <0>;
>>             #size-cells = <0>;
>>         };
>>
>>         /*
>>          * This is the i2c bus wired at the connector. It is
>>          * handled by the i2c5 adapter available in the SoC.
>>          * The overlay will add devices in this node. Those
>>          * devices are devices available on the addon-board and
>>          * connected to this i2c bus
>>          */
>>         i2c-addon {
>>             i2c-parent = <&i2c5>;
>
> What if this I2C instance is not enabled (maybe not an issue for I2C,
> but some devices should have their status left disabled unless something
> is connected, SPI for instance).
>
> And this (I2C) only works because there is this `i2c-parent` thing, 
> but that
> isn't the case for most (e.g. there is no spi-parent, mdio-parent, etc..)
> My proposal[0] handles that by giving standard names to the provider 
> phandles
> that conforming add-on board overlays can reference directly.
>
>>             #address-cells = <1>;
>>             #size-cells = <0>;
>>         };
>>
>>         export-symbols {
>>             /*
>>              * The 'addon_connector' symbol can be used from the
>>              * overlay to reference this connector
>>              */
>>             addon_connector = <&addon_connector0>;
>>         };
>>     };
>> };
>>
>>
>> Then following overlay is applied at the addon-connector0 node and 
>> described
>
> *How* is this "applied at the addon-connector0 node"?
>
> Again I'm going to guess this is if we are able to modify the 
> `fdtapply` tool
> to accept connection points as a new command line parameter. And then 
> somehow
> do the same for all other projects that apply DT overlays (U-Boot, 
> Zephyr, etc.).

I do have patches that add it to fdtoverlay. But most driver based 
solutions will probably rely on some EEPROM or sysfs entry as well.


>
> And in that case, we would have to pass in the name of the connector 
> anyway,
> so having that name be the only item in `export-symbols` doesn't get 
> us anything
> new, we could have just passed the connector name directly.
>
> BTW, this is solved in my proposal[0] with adapter/shim overlays. 
> Which allow for
> this to work without any modifying the overlay tooling. Maybe there is 
> a way we
> can put this new `export-symbols` node in the adapter overlay to avoid 
> passing
> in the connector name directly to the tooling.. I have to think on this.

The problem with the __symbols__ proposal is the following:

1. Path references are not supported in overlays.

I have tried to add them as well, but full support for path references 
is not possible in overlays, at least right now. The reason for this can 
be found here [0].

There is a possibility of adding a new node: `__symbols_phandle__`, but 
that seems too linux specific.

Note: It is not possible to use aliases due to the reasons discussed 
here [1].


2. Global modification

The __symbols__ based approach directly adds nodes to different parts of 
the devicetree, outside of the connector. That is a security problem. We 
need a way to isolate any devicetree modification to a single node.

I had a discussion here [2] regarding why the sysfs based interface for 
overlays is not merged in mainline, and at least to me, it seems for 
mainline support, the modifications need to be strictly isolated as much 
as possible. But of course, maybe I misunderstood something.


3. Pollutes the global symbols

Export-symbols are local to the connector node, which brings a lot of 
benefits since it is not possible to accidentally refer to the wrong 
phandle. Can be avoided with careful naming.

Additionally, I am not completely sure how connector versioning would be 
handled here. Maybe the symbols should have naming convention that 
accounts for the version, but that seems like it can get difficult to 
scale pretty fast.


4. It requires all symbols to be generated

I don't personally have a problem with this, but it has come up in some 
other discussions.


>
>> the addon board connected to the connector:
>>
>> / {
>>     fragment@0 {
>>         target-path = "";
>>
>>         __overlay__ {
>>             devices {
>>                 reg_addon_3v3: regulator-addon-3v3 {
>>                     compatible = "regulator-fixed";
>
> Is this really a fixed regulator on the add-on board or is this being 
> feed
> from the main board's PMIC but you have no good way to model that 
> connection
> with this scheme? This breaks the regulator/power dependency graph. If a
> device on the add-on board powers down, that information is no longer 
> sent
> back to the parent PMIC and the power rail will be needlessly left 
> enabled.
>
> This is why I was looking for a full complete example, not a 
> simplified one.
> I'm having to make too many assumptions here to give this 
> `export-symbols`
> thing a proper review.


I will try sending patches for PocketBeagle 2 connector since that is 
probably complex enough to catch the corner cases. But well, it will be 
at least a bit simplified (SPI devices will be missing). The reason 
being there are other open items outside of export-symbols:

1. SPI chipselect.

- That is a required property for SPI devices in dt, but there is no way 
(that I know of) to decouple that in a connector addon-board setup


TechLab cape also exposes a MikroBUS connector on it, so I guess I can 
test how board chaining looks like as well.


>
> Andrew
>
> [0] https://lore.kernel.org/lkml/[email protected]/
>
>>                     regulator-name = "3V3_ADDON";
>>                     regulator-min-microvolt = <15000000>;
>>                     regulator-max-microvolt = <15000000>;
>>                     regulator-always-on;
>>                 };
>>
>>                 reg_addon_12v0: regulator-addon-12v0 {
>>                     compatible = "regulator-fixed";
>>                     regulator-name = "12V0_ADDON";
>>                     vin-supply = <&reg_addon_3v3>;
>>                     regulator-min-microvolt = <12000000>;
>>                     regulator-max-microvolt = <12000000>;
>>                     gpios = <&tca6424_addon 12 GPIO_ACTIVE_HIGH>;
>>                     enable-active-high;
>>                 };
>>
>>                 /*
>>                  * This backligh is a PWM driven backlight.
>>                  * It uses the PWM #0 available at the connector
>>                  */
>>                 backlight_addon: backlight-addon {
>>                     compatible = "pwm-backlight";
>>                     power-supply = <&reg_addon_12v0>;
>>                     pwms = <&addon_connector 0 57000 0>;
>>                     brightness-levels = <0 255>;
>>                     num-interpolated-steps = <255>;
>>                     default-brightness-level = <255>;
>>                 };
>>             };
>>
>>             i2c-addon {
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>
>>                 /*
>>                  * This IO expander uses the interrupt #0
>>                  * available at the connector.
>>                  * It is a device connected to the i2c-addon bus
>>                  * available at the connector.
>>                  */
>>                 tca6424_addon: gpio@23 {
>>                     compatible = "ti,tca6424";
>>                     status = "okay";
>>                     reg = <0x23>;
>>                     gpio-controller;
>>                     #gpio-cells = <2>;
>>                     interrupt-parent = <&addon_connector>;
>>                     interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
>>                     interrupt-controller;
>>                     #interrupt-cells = <2>;
>>                     vcc-supply = <&reg_addon_3v3>;
>>                 };
>>             };
>>         };
>>     };
>> };
>>
>>
>> Best regards,
>> Hervé
>>

Best Regards

Ayush Singh


[0]: 
https://lore.kernel.org/devicetree-compiler/[email protected]/T/#m8259c8754f680b9da7b91f7b7dd89f10da91d8ed

[1]: 
https://lore.kernel.org/all/[email protected]/T/#t

[2]: 
https://lore.kernel.org/all/[email protected]/