dtoverlay: cannot add/remove fragments alongside non-integer parameters

[compholio]

It is convenient to not require more parameters than necessary when using dtmerge/dtoverlay, toward that end it would be nice to support enabling a fragment while also turning on other parameters. For example, this snippet could allow an optional PHY handle to be applied to an ethernet overlay:

/ {
	phy: fragment@3 {
		phy_handle: __dormant__ { };
	};
	__overrides__ {
		phy =
			<0>,"+3"
		,
			<&phy>,"target-path"
		,
			<&phy_handle>,"dtoverlay,phandle:0=",<&phy_handle>
		,
			<&overlay>,"phy-handle:0=",<&phy_handle>
		;
};

Note: I have separately patched dtmerge to allow it to override a phandle at the toplevel when the parameter is named "dtoverlay,phandle".

So, in this example "phy=alias-for-phy" or "phy=/a-full-path/to/a-phy@0" would permit enabling the "phy" fragment along with setting the target path. I've attached a patch that allows this behavior (+/- with a string override):

override-strings.patch

I'd like to have thoughts on this, if something like this is acceptable I'm happy to open a merge request.

[pelwell]

I'm open to the idea of supporting the use of overlay overrides with string parameters - the previous omission was an oversight. I understand the caution behind only supporting unconditional enable/disable (+/-). However, the implementation is a bit ugly - I'm not keen on the magic illegal value. If instead you were to treat non-empty strings as true/1 and empty strings as false/0 you would avoid the need to disallow the conditionals (=/!) and conceivably allow some additional uses.

Can you say a bit more about your other patch? What is the use case?

[compholio]

You're thinking something like this?:

0001-dtoverlay-allow-fragment-enable-disable-to-work-with.patch

I have four devkits from three different manufacturers that are all very "swiss army knife" systems. The worst case is the QSGMII PHYs, as there are two possible configurations for them (controlled by dip switches).
Example config 1 (separated SGMII lanes):

dtmerge dtb dtb qoriq-ethernet enet=fm1-mac6 phy-connection-type=sgmii phy=qsgmii-s1-p1 in-band-status
dtmerge dtb dtb qoriq-ethernet enet=fm1-mac9 phy-connection-type=sgmii phy=qsgmii-s2-p2 in-band-status

Example config 2 (single lane in QSGMII):

dtmerge dtb dtb qoriq-ethernet enet=fm1-mac1 phy-connection-type=qsgmii phy=qsgmii-s2-p4 in-band-status
dtmerge dtb dtb qoriq-ethernet enet=fm1-mac5 phy-connection-type=qsgmii phy=qsgmii-s2-p2 in-band-status
dtmerge dtb dtb qoriq-ethernet enet=fm1-mac6 phy-connection-type=qsgmii phy=qsgmii-s2-p1 in-band-status
dtmerge dtb dtb qoriq-ethernet enet=fm1-mac8 phy-connection-type=qsgmii phy=qsgmii-s2-p3 in-band-status

If the systems were "same enough" then I could live with it, but the PHYs are on different mdio buses for different kits (and multiple buses on one kit). So, that makes it really challenging to use handles (hence using the target-path above instead) since then the handles would need to be valid for every system (I would need to make a bunch of extra dummy handles). The way I was dealing with this previously was using a lookup table, but now that I am adding another system (not added below) this was getting a bit too unwieldy:

	/* give the overlay a place to find the PHY node */
	phy: fragment@3 {
		__overlay__ {
			phy_handle: ethernet-phy@0 {
				reg = <0x00>;
			};
		};
	};
	__overrides__ {
		phy =
			<&overlay>,"phy-handle:0=",<&phy_handle>
		,
			<&phy>,
			"target-path{invalid=/invalid
				,qsgmii-s1-p1=/soc/fman@1a00000/mdio@fc000,
				,qsgmii-s1-p2=/soc/fman@1a00000/mdio@fc000,
				,qsgmii-s1-p3=/soc/fman@1a00000/mdio@fc000,
				,qsgmii-s1-p4=/soc/fman@1a00000/mdio@fc000,
				,qsgmii-s2-p1=/soc/fman@1a00000/mdio@fc000,
				,qsgmii-s2-p2=/soc/fman@1a00000/mdio@fc000,
				,qsgmii-s2-p3=/soc/fman@1a00000/mdio@fc000,
				,qsgmii-s2-p4=/soc/fman@1a00000/mdio@fc000,
				,qsgmii-p1=/soc/fman@1a00000/mdio@fd000,
				,qsgmii-p2=/soc/fman@1a00000/mdio@fd000,
				,qsgmii-p3=/soc/fman@1a00000/mdio@fd000,
				,qsgmii-p4=/soc/fman@1a00000/mdio@fd000,
			}"
		,
			<&phy_handle>,
			"reg:0{invalid=",<0xffffffff>,
				"qsgmii-s1-p1=",<0x1c>,
				"qsgmii-s1-p2=",<0x1d>,
				"qsgmii-s1-p3=",<0x1e>,
				"qsgmii-s1-p4=",<0x1f>,
				"qsgmii-s2-p1=",<0x00>,
				"qsgmii-s2-p2=",<0x01>,
				"qsgmii-s2-p3=",<0x02>,
				"qsgmii-s2-p4=",<0x03>,
				"qsgmii-p1=",<0x1c>,
				"qsgmii-p2=",<0x1d>,
				"qsgmii-p3=",<0x1e>,
				"qsgmii-p4=",<0x1f>,
			"}"
		;

So, by adding a patch to allow "dtoverlay,phandle" to set the phandle on the toplevel node I can just use the aliases (already present in the original device trees) to refer to the PHYs by name and then stick in the appropriate phy-handle for the ethernet MAC. I feel like this is very useful, but I would guess it requires a little more refining to be acceptable upstream:

toplevel-phandle.patch

[pelwell]

Yes, your patch looks like what I had in mind, assuming that it works as expected.

I'll read your use case when I get a spare moment.

[compholio]

...
I'll read your use case when I get a spare moment.

Great, if it's of interest to you then let me know. If so then I can open a pull request to work on making something better, as I can imagine a few ways that the version above ( https://github.com/user-attachments/files/28106874/toplevel-phandle.patch ) could be improved.

[pelwell]

4dec650 is the equivalent change for ovmerge.

[pelwell]

Well, this is unexpected - the string parameter change is now available in a Pi 5 EEPROM: raspberrypi/rpi-eeprom@9c7eb94
Other platforms will follow.

By the way, I would find it helpful to have a self-contained (but possibly cut down) real-world example that relies on the toplevel phandle patch. Obviously I understand that it won't work without the patch.

[compholio]

Happy to help, here is a stripped down version (minimal number of elements) of the vendor device tree (with noted modifications for aliases, to keep consistent with what you have already seen):

/ {
	aliases {
		qsgmii-s2-p2 = &qsgmii2_phy2;
		fm1-mac5 = &enet4; /* original vendor version is ethernet4, added for my convenience */
		fm1-mac9 = &enet6; /* original vendor version is ethernet6, added for my convenience */
	};
};
&enet4 {
	status = "disabled";
};
&enet6 {
	status = "disabled";
};
&mdio0 {
	status = "okay";
	qsgmii2_phy2: ethernet-phy@1 {
		compatible = "ethernet-phy-ieee802.3-c22";
		reg = <0x01>;
	};
};

The overlay with the phandle modification (stripped to minimal elements):

/ {
	enet: fragment@1 {
		overlay: __overlay__ {
			status = "okay";
		};
	};
	phy: fragment@3 {
		phy_handle: __overlay__ { };
	};
	__overrides__ {
		enet =
			<&enet>,"target-path",
		;
		phy =
			<&phy>,"target-path",
			<&phy_handle>,"dtoverlay,phandle:0=",<&phy_handle>,
			<&overlay>,"phy-handle:0=",<&phy_handle>
		;
};

And the two ways this element can be configured (by dip switches):

dtmerge dtb dtb qoriq-ethernet enet=fm1-mac9 phy=qsgmii-s2-p2
dtmerge dtb dtb qoriq-ethernet enet=fm1-mac5 phy=qsgmii-s2-p2

So, in the first example the phy-handle for the specified PHY (qsgmii-s2-p2/&qsgmii2_phy2) is applied to the ninth MAC (fm1-mac9/&enet6) and in the second example the phy-handle is applied to the fifth MAC (fm1-mac5/&enet5). Other settings also require changing (phy-connection-type, etc.), but this stripped down example just focuses on the phandle that needs to by placed in the ethernet node phy-handle property.

It's worth noting that the current patch I have for this replaces the original phandle in the tree prior to application, keeping the original handle (if possible) is one of the improvements I was thinking you would want (though it does not matter for my use case).

[pelwell]

I've been chewing over the issue of top-level phandles for a while, and I can't think of a better solution. One could imagine a scheme where a top-level phandle that was referenced by a part of the applied overlay would automatically be preserved, but there's quite a bit of processing required compared to a simple strcmp.

Detecting that an existing phandle is about to be overridden is fairly easy (at least in the firmware and dtmerge), but replicating that base phandle value back to the other instances is more painful - perhaps a quick first pass to spot the collisions before the renumbering and merging would be simplest.

[compholio]

Would you like me to open an issue for "need a way to keep toplevel phandles"? It's also possible there could be boolean "dtoverlay,preserve-phandle" or something for this.

[pelwell]

The advantage of using a separate boolean is that it can just exist in the __overlay__ block without needing to be patched in any way. You might as well make it a PR.

[compholio]

Okay, take a look at #198 and see what you think.