ftdoverlay overwrites phandle

ftdoverlay overwrites phandle

[Uwe Kleine-König]

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

Hello,

I want to apply an overlay to a device tree that is compiled without -@
because that's how dtbs are shipped by the Debian kernel package.
I'm using dtc 1.6.1-1 as provided by Debian.

The machine dtb is bcm2836-rpi-2-b.dtb[1]. The source for the dtbo looks
as follows:

	uwe@taurus:~/tmp$ cat dht11-overlay.dts 
	/*
	 * Overlay for the DHT11/21/22 humidity/temperature sensor modules.
	 */
	/dts-v1/;
	/plugin/;

	/ {
		compatible = "brcm,bcm2708";

		fragment@0 {
			target-path = "/";

			__overlay__ {
				dht11: dht11@0 {
					compatible = "dht11";
					pinctrl-names = "default";
					pinctrl-0 = <&dht11_pins>;
					gpios = <&gpio 4 0>;
					status = "okay";
				};
			};
		};

		fragment@1 {
			target-path = "/soc/";
			__overlay__ {
				gpio: gpio@7e200000 {
					dht11_pins: dht11_pins {
						brcm,pins = <4>;
						brcm,function = <0>; // in
						brcm,pull = <0>; // off
					};
				};
			};
		};

		__overrides__ {
			gpiopin = <&dht11_pins>,"brcm,pins:0",
				<&dht11>,"gpios:4";
		};
	};

which is more or less the overlay with the same name provided by the
RaspberryPi Foundation[2].

I can compile and apply the overlay just fine (well there are warnings,
but ...):

	uwe@taurus:~/tmp$ dtc -I dts -O dtb dht11-overlay.dts > dht11-overlay.dtbo
	dht11-overlay.dts:14.19-20.6: Warning (unit_address_vs_reg): /fragment@0/__overlay__/dht11@0: node has a unit name, but no reg or ranges property
	dht11-overlay.dts:27.24-33.6: Warning (unit_address_vs_reg): /fragment@1/__overlay__/gpio@7e200000: node has a unit name, but no reg or ranges property
	dht11-overlay.dts:14.19-20.6: Warning (gpios_property): /fragment@0/__overlay__/dht11@0: Missing property '#gpio-cells' in node /fragment@1/__overlay__/gpio@7e200000 or bad phandle (referred from gpios[0])
	uwe@taurus:~/tmp$ fdtoverlay -i bcm2836-rpi-2-b.dtb -o bcm2836-rpi-2-b+dht11.dtb dht11-overlay.dtbo

However there is a problem: The original bcm2836-rpi-2-b.dtb has:

	uwe@taurus:~/tmp$ fdtdump bcm2836-rpi-2-b.dtb 
	...
	        gpio@7e200000 {
	            ....
	            phandle = <0x00000006>;
	        ....
	        hdmi@7e902000 {
	            ...
	            hpd-gpios = <0x00000006 0x0000002e 0x00000001>;
	            ...

the patched dtb has:

	...
	    dht11@0 {
	        phandle = <0x0000001d>;
	        status = "okay";
	        gpios = <0x0000001c 0x00000004 0x00000000>;
	        pinctrl-0 = <0x0000001b>;
	        pinctrl-names = "default";
	        compatible = "dht11";
	    };
	    ...
	        gpio@7e200000 {
	            phandle = <0x0000001c>;
	        ....
	        hdmi@7e902000 {
	            ...
	            hpd-gpios = <0x00000006 0x0000002e 0x00000001>;
	            ...

So the gpios property of the dht11 node points to the gpio node as
expected, however the hpd-gpios property of the hdmi node is broken
because the phandle of gpio@7e200000 changed.

I think it should be possible to not overwrite the phandle of
gpio@7e200000 and just reuse the value 6.

Is this a bug, or did I miss a feature that allows me to create a dtbo
that does the right thing?

@Corsac: Maybe it's worth to test if the rpi firmware does a better job
at applying the dtbo? If you test this, please report here.

Best regards
Uwe

[1] https://www.kleine-koenig.org/tmp/bcm2836-rpi-2-b.dtb
[2] https://raw.githubusercontent.com/raspberrypi/linux/5cc390759ed0461bd35744e817470869e99e0484/arch/arm/boot/dts/overlays/dht11-overlay.dts

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: ftdoverlay overwrites phandle

[Uwe Kleine-König]

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

Hello,

[Cc += Maxime who seems to have done some overlay work in dtc]

On Wed, Jun 29, 2022 at 10:59:27PM +0200, Uwe Kleine-König wrote:
> I want to apply an overlay to a device tree that is compiled without -@
> because that's how dtbs are shipped by the Debian kernel package.
> I'm using dtc 1.6.1-1 as provided by Debian.
> 
> The machine dtb is bcm2836-rpi-2-b.dtb[1]. The source for the dtbo looks
> as follows:
> 
> 	uwe@taurus:~/tmp$ cat dht11-overlay.dts 
> 	/*
> 	 * Overlay for the DHT11/21/22 humidity/temperature sensor modules.
> 	 */
> 	/dts-v1/;
> 	/plugin/;
> 
> 	/ {
> 		compatible = "brcm,bcm2708";
> 
> 		fragment@0 {
> 			target-path = "/";
> 
> 			__overlay__ {
> 				dht11: dht11@0 {
> 					compatible = "dht11";
> 					pinctrl-names = "default";
> 					pinctrl-0 = <&dht11_pins>;
> 					gpios = <&gpio 4 0>;
> 					status = "okay";
> 				};
> 			};
> 		};
> 
> 		fragment@1 {
> 			target-path = "/soc/";
> 			__overlay__ {
> 				gpio: gpio@7e200000 {
> 					dht11_pins: dht11_pins {
> 						brcm,pins = <4>;
> 						brcm,function = <0>; // in
> 						brcm,pull = <0>; // off
> 					};
> 				};
> 			};
> 		};
> 
> 		__overrides__ {
> 			gpiopin = <&dht11_pins>,"brcm,pins:0",
> 				<&dht11>,"gpios:4";
> 		};
> 	};
> 
> which is more or less the overlay with the same name provided by the
> RaspberryPi Foundation[2].
> 
> I can compile and apply the overlay just fine (well there are warnings,
> but ...):
> 
> 	uwe@taurus:~/tmp$ dtc -I dts -O dtb dht11-overlay.dts > dht11-overlay.dtbo
> 	dht11-overlay.dts:14.19-20.6: Warning (unit_address_vs_reg): /fragment@0/__overlay__/dht11@0: node has a unit name, but no reg or ranges property
> 	dht11-overlay.dts:27.24-33.6: Warning (unit_address_vs_reg): /fragment@1/__overlay__/gpio@7e200000: node has a unit name, but no reg or ranges property
> 	dht11-overlay.dts:14.19-20.6: Warning (gpios_property): /fragment@0/__overlay__/dht11@0: Missing property '#gpio-cells' in node /fragment@1/__overlay__/gpio@7e200000 or bad phandle (referred from gpios[0])
> 	uwe@taurus:~/tmp$ fdtoverlay -i bcm2836-rpi-2-b.dtb -o bcm2836-rpi-2-b+dht11.dtb dht11-overlay.dtbo
> 
> However there is a problem: The original bcm2836-rpi-2-b.dtb has:
> 
> 	uwe@taurus:~/tmp$ fdtdump bcm2836-rpi-2-b.dtb 
> 	...
> 	        gpio@7e200000 {
> 	            ....
> 	            phandle = <0x00000006>;
> 	        ....
> 	        hdmi@7e902000 {
> 	            ...
> 	            hpd-gpios = <0x00000006 0x0000002e 0x00000001>;
> 	            ...
> 
> the patched dtb has:
> 
> 	...
> 	    dht11@0 {
> 	        phandle = <0x0000001d>;
> 	        status = "okay";
> 	        gpios = <0x0000001c 0x00000004 0x00000000>;
> 	        pinctrl-0 = <0x0000001b>;
> 	        pinctrl-names = "default";
> 	        compatible = "dht11";
> 	    };
> 	    ...
> 	        gpio@7e200000 {
> 	            phandle = <0x0000001c>;
> 	        ....
> 	        hdmi@7e902000 {
> 	            ...
> 	            hpd-gpios = <0x00000006 0x0000002e 0x00000001>;
> 	            ...
> 
> So the gpios property of the dht11 node points to the gpio node as
> expected, however the hpd-gpios property of the hdmi node is broken
> because the phandle of gpio@7e200000 changed.
> 
> I think it should be possible to not overwrite the phandle of
> gpio@7e200000 and just reuse the value 6.

I thought a bit more about that and with the given design of fdtoverlay
this is hard because there are two types of (local) references: One
where you just have to add $delta (which is the only case that is
currently supported) and the other where the already existing number
from the dtb is reused.

An alternative would be to use a new node (say) __base_symbols__ that is
used to lookup symbols in the base dtb. E.g.:

	/ {
	    compatible = "brcm,bcm2708";
	    fragment@0 {
		target-path = "/";
		__overlay__ {
		    dht11 {
			compatible = "dht11";
			pinctrl-names = "default";
			pinctrl-0 = <0x00000001>;
			gpios = <0xffffffff 0x00000004 0x00000000>;
			status = "okay";
			phandle = <0x00000002>;
		    };
		};
	    };
	    __fixups__ {
		gpio = "/fragment@0/__overlay__/dht11:gpios:0";
	    };
	    __base_symbols__ {
	        gpio = "/soc/gpio@7e200000";
	    };
	};

then if the dtb that is supposed to be patched doesn't have gpio in its
__symbols__ node, this is looked up using __base_symbols__ in the dtbo.

Does this sound sensible? The downside compared to the first possibility
is that we have to extend the overlay device tree format.

A workaround I just noticed is: I could create two overlays and apply
them im a row. The first adds a __symbols__ node to the dtb and the
second uses the newly created node to properly fixup the local
reference.

What do you think?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: ftdoverlay overwrites phandle

[Rob Herring]

On Wed, Jun 29, 2022 at 3:20 PM Uwe Kleine-König <ukleinek-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org> wrote:
>
> Hello,
>
> I want to apply an overlay to a device tree that is compiled without -@
> because that's how dtbs are shipped by the Debian kernel package.

Does applying this overlay work as expected if -@ is used? I would
imagine so as there is an assumption the base dtb was compiled with
-@. The fix is to fix Debian dtbs (or don't use them as dtbs should
come from firmware rather than distros).

There might be sufficient information to make your case work because
IIRC all the (resolved) overlay phandle values get adjusted when
applied as they could collide with the base tree phandle values.
However, doing so would mean users may start working around base DTs
compiled without -@. That would remove labels being an ABI which I
don't love, but it would also remove the abstraction that labels
provide.

Certainly, the base phandle value shouldn't just be silently overwritten.

Rob

Re: ftdoverlay overwrites phandle

[Uwe Kleine-König]

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

Hello Rob,

On Tue, Jul 12, 2022 at 09:53:57AM -0600, Rob Herring wrote:
> On Wed, Jun 29, 2022 at 3:20 PM Uwe Kleine-König <ukleinek-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org> wrote:
> >
> > Hello,
> >
> > I want to apply an overlay to a device tree that is compiled without -@
> > because that's how dtbs are shipped by the Debian kernel package.
> 
> Does applying this overlay work as expected if -@ is used? I would
> imagine so as there is an assumption the base dtb was compiled with
> -@.

No it doesn't. I created a minimal (oh well, at least small) reproducer:

	uwe@taurus:~/gsrc/oftreemerge$ cat base.dts
	/dts-v1/;

	/ {
		#address-cells = <1>;
		#size-cells = <1>;

		node_a: a {
			prop = "blub";
		};

		node_b: b {
			a = <&node_a>;
		};
	};

	uwe@taurus:~/gsrc/oftreemerge$ cat overlay.dts
	/dts-v1/;
	/plugin/;

	/ {
		fragment@0 {
			target-path = "/";

			__overlay__ {
				node_a: a {
				};

				c {
					a = <&node_a>;
				};
			};
		};
	};

When compiling without -@ (as reported initially) I get:

	uwe@taurus:~/gsrc/oftreemerge$ dtc -I dts -O dtb -o base.dtb base.dts 
	uwe@taurus:~/gsrc/oftreemerge$ dtc -I dts -O dtb -o overlay.dtbo overlay.dts 
	uwe@taurus:~/gsrc/oftreemerge$ fdtoverlay -i base.dtb -o patched.dtb overlay.dtbo 
	uwe@taurus:~/gsrc/oftreemerge$ fdtdump patched.dtb 
	...
	/ {
	    #address-cells = <0x00000001>;
	    #size-cells = <0x00000001>;
	    c {
		a = <0x00000002>;
	    };
	    a {
		prop = "blub";
		phandle = <0x00000002>;
	    };
	    b {
		a = <0x00000001>;
	    };
	};

So b.a gets broken as the phandle for a got updated.

When doing the same with -@ the result is identical (apart from the
expected changes: more and different phandles, __symbols__ node etc):

	uwe@taurus:~/gsrc/oftreemerge$ dtc -@ -I dts -O dtb -o base.dtb base.dts
	uwe@taurus:~/gsrc/oftreemerge$ dtc -@ -I dts -O dtb -o overlay.dtbo overlay.dts
	uwe@taurus:~/gsrc/oftreemerge$ fdtoverlay -i base.dtb -o patched.dtb overlay.dtbo
	uwe@taurus:~/gsrc/oftreemerge$ fdtdump patched.dtb
	...
	/ {
	    #address-cells = <0x00000001>;
	    #size-cells = <0x00000001>;
	    c {
		a = <0x00000003>;
	    };
	    a {
		prop = "blub";
		phandle = <0x00000003>;
	    };
	    b {
		a = <0x00000001>;
		phandle = <0x00000002>;
	    };
	    __symbols__ {
		node_a = "/a";
		node_b = "/b";
	    };
	};

b.a still points to a non-existing node.

> The fix is to fix Debian dtbs

One of the blockers for that is that adding -@ to the kernel default
rules was rejected more than once.
(https://lore.kernel.org/linux-arm-kernel/CAK7LNAS5t1wew0MMFjdB5HGCAMerhU7pAGiFhcTtCRUAAjGLpw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/)

> (or don't use them as dtbs should come from firmware rather than
> distros).

In this case the distro provides the firmware, too. So well, you could
argue the dtbs should (in this case) be provided by the raspi-firmware
package and not the kernel image package, but that only makes things
harder to handle, because the kernel is effectively the upstream for the
device tree files.

> There might be sufficient information to make your case work because
> IIRC all the (resolved) overlay phandle values get adjusted when
> applied as they could collide with the base tree phandle values.
> However, doing so would mean users may start working around base DTs
> compiled without -@. That would remove labels being an ABI which I
> don't love, but it would also remove the abstraction that labels
> provide.

My point of view is a bit different. It would allow to practically apply
overlays to device trees without -@. It doesn't limit in any way the
semantic of labels in the case where -@ is used. I can accept we're
differing in that point and I don't think working on reaching an
agreement here is time spend well.

> Certainly, the base phandle value shouldn't just be silently overwritten.

We agree here, which is good enough for me.

Fixing that is a bit complicated, because currently fdtoverlay just
applies a fixed offset to the phandle values defined in the overlay. I
think it would need another pass over the device tree to determine
already existing phandles and maintaining a mapping for these.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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