dts: flatten video's device tree endpoints definition #72950

josuah · 2024-05-17T14:28:27Z

This is part of a series of proposals for incremental changes for the Video API:

drivers: video: API enhancement #72959

Summary:

Remove port@0 from video devicetree and only keep endpoint@0

Before:

videodev { ports { port@0 { endpoint@0 { remote-endpoint; }; }; }; };
videodev { port { endpoint { remote-endpoint; }; }; };

After:

videodev { ports { endpoint@0 { remote-endpoint; }; }; };

Details:

Currently, remote-endpoint is not used (#72311). This allows us to move definitions without breaking anything.

In Linux, the ports{}; block allows to have #address-cells in ports{} applied to port@0{}; port@1{}; ... only, and not i.e. display-timings.

Complex configurations are still possible with this scheme:

videodev@10380000 {
	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;
			#address-cells = <1>;
			#size-cells = <0>;

			endpoint@0 {
				reg = <0>;
				remote-endpoint = <&other>;
			};

			endpoint@1 {
				reg = <1>;
				remote-endpoint = <&other>;
			};
		};

		port@1 {
			reg = <1>;
			#address-cells = <1>;
			#size-cells = <0>;

			endpoint@0 {
				reg = <0>;
				remote-endpoint = <&other>;
			};
		};
	};
};

Is turned into something like this:

videodev@10380000 {
	ports {
		#address-cells = <2>;
		#size-cells = <0>;

		endpoint@00 {
			reg = <0 0>;
			data-lanes = <0 1 2 3>;
			remote-endpoint = <&other>;
		};

		endpoint@01 {
			reg = <0 1>;
			data-lanes = <4 5 6 7>;
			remote-endpoint = <&other>;
		};

		endpoint@10 {
			reg = <1 0>;
			data-lanes = <0 1 2 3>;
			remote-endpoint = <&other>;
		};
	};
};

This prevents to apply devicetree properties to an entire port, affecting each of its endpoint, but in the Linux source, this was only used once, in imx8mp-tqma8mpql-mba8mpxl-lvds-g133han01.dtso:

References

Reviewing a few contexts where remote-endpoint are invoked in Linux (audio, mipi, hdmi, usb) and have extra properties (outside reg, #address-cells =, #size-cells and nested blocks):

cd Linux
find . -name '*.dts*' -exec grep -C10 'remote-endpoint' {} +

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts

link-frequencies in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi

data-lanes in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8186-corsola-steelix.dtsi

data-lanes in endoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi

dai-format in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000.dtsi

convert-rate in endoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/nvidia/tegra194-p3509-0000.dtsi

data-lanes in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/px30-evb.dts

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/renesas/hihope-rzg2-ex-aistarvision-mipi-adapter-2.1.dtsi

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dts

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/renesas/rz-smarc-cru-csi-ov5645.dtsi

sevral in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/renesas/draak.dtsi

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/qrb5165-rb5-vision-mezzanine.dts

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts

data-lanes in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi

data-lanes in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi

data-lanes in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sm8350-hdk.dts

data-lanes in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi

several in endpoints:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mm-venice-gw72xx-0x-imx219.dtso

dual-lvds-odd-pixels in port:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl-lvds-g133han01.dtso

dual-lvds-even-pixels in port:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl-lvds-g133han01.dtso

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi

data-lanes in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mp-evk.dts

bus-width in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/renesas/r8a7791-koelsch.dts

arm,pl11x,tft-r0g0b0-pads in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm/arm-realview-eb.dtsi

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/st/stm32mp157c-ev1.dts

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/st/stm32429i-eval.dts

several in endpoint:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ti/omap/omap3-n900.dts

josuah · 2024-05-17T14:48:40Z

Interesting consequence: it becomes possible for any driver to list all ports using DT_FOREACH_CHILD() on ports{};, convenient for a driver to add a struct device * reference to each of its connected devices in an array.

Similar to this:

zephyr/drivers/interrupt_controller/intc_gic.c

Lines 29 to 31 in 5409c7c

    
           static const uint64_t cpu_mpid_list[] = { 
        
           	DT_FOREACH_CHILD_STATUS_OKAY_SEP(DT_PATH(cpus), DT_REG_ADDR, (,)) 
        
           };

For instance, allowing video_endpoint_id in video_enqueue to be converted to an actual struct device *.

This change removes the "port@0 { ... };" block, making the devicetree less nested while systematically including the "ports { ... };" root block. In practice, this looks like only adding an "s" to "port" blocks, but the changes are also semantic. Complex configurations are still possible with this scheme: video@10380000 { ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; #address-cells = <1>; #size-cells = <0>; endpoint@0 { reg = <0>; remote-endpoint = <&other>; }; endpoint@1 { reg = <1>; remote-endpoint = <&other>; }; }; port@1 { reg = <1>; #address-cells = <1>; #size-cells = <0>; endpoint@0 { reg = <0>; remote-endpoint = <&other>; }; }; }; }; Is turned into something like this: video@10380000 { ports { #address-cells = <2>; #size-cells = <0>; endpoint@00 { reg = <0 0>; data-lanes = <0 1 2 3>; remote-endpoint = <&other>; }; endpoint@01 { reg = <0 1>; data-lanes = <4 5 6 7>; remote-endpoint = <&other>; }; endpoint@10 { reg = <1 0>; data-lanes = <0 1 2 3>; remote-endpoint = <&other>; }; }; }; Which helps simpler device to be simpler, and allows more complex device with arbitrary number of nesting to remain simple. Both very-nested and completely flat (1 port 1 endpoint) look exactly the same way, which helps with use of generic macros to traverse the devicetree across all video devices. The use of "ports { ... };" is required so that the "#address-cells" property can be different between "ports" and i.e. "display-timings". This prevents to apply devicetree properties to an entire port, affecting each of its endpoint, but in the Linux source, this was only used once, in imx8mp-tqma8mpql-mba8mpxl-lvds-g133han01.dtso. Signed-off-by: Josuah Demangeon <me@josuah.net>

josuah · 2024-05-20T09:17:15Z

Some other issue there is with a nested ports -> port@0 -> endpoint@ is that the current video API does not support specifying one port of one endpoint, only an individual number is passed to select a particular endpoint.

drivers: video: introduce semantics for enum endpoint_id #73009

loicpoulain · 2024-05-20T16:24:53Z

Looks great to me, but someone from dts to approve.

josuah · 2024-05-22T14:31:00Z

One issue is that the ports { port { endpoint {}; }; }; hierarchy is encoded in dtc, which Zephyr uses:
https://github.com/torvalds/linux/blob/dccb07f2914cdab2ac3a5b6c98406f765acab803/scripts/dtc/checks.c#L1770

So we would end-up with warnings like this:

/home/tinyvision/zephyrproject/build/zephyr/zephyr.dts:234.10-254.6:
Warning (graph_port): /soc/usb@b0000000/ports: graph port node name should be 'port'

Likewise, Port 0 Endpoint 0 In would become: endpoint@001 and Zephyr would complain of the leading zeros:

/home/tinyvision/zephyrproject/build/zephyr/zephyr.dts:238.17-241.7: Warning (unit_address_format): /soc/usb@b0000000/ports/endpoint@000:

So keeping the ports / port / endpoint Hierarchy could simplify things, but not sure yet how to deal with this without raising complexity too much... Maybe this is where having a video_dt_spec would help: ad-hoc encoding over reg = <...>; seems to have limitations.

See also the suggestion to include the endpoint chain as global devicetree macros.
#73023 (comment)

josuah · 2024-05-28T00:03:58Z

One easy workaround is to simply use a single port{} as the root:

this requires no change: fewer things to convert.
and play along better with upstream.
there is still a solution for handling endpoints with devicetree macros: assuming a single port at first, then introduce more complex macros later if needed.
postpone the choice between endpoint@ab { reg = <a, b>; }; or port@a { reg = <a>; endpoint@b { reg = <b>; }; }; for later.

josuah · 2024-05-28T00:05:37Z

Since doing nothing turns out better, may you allow me to close my pull request.
Sorry for the needless bother!

josuah requested a review from erwango as a code owner May 17, 2024 14:28

zephyrbot added platform: NXP NXP platform: STM32 ST Micro STM32 area: Devicetree Binding PR modifies or adds a Device Tree binding labels May 17, 2024

zephyrbot requested review from ajarmouni-st, danieldegrasse, decsny, DerekSnell, dleach02, EmilioCBen, FRASTM, galak, gautierg-st, GeorgeCGV, mmahadevan108 and yvanderv May 17, 2024 14:29

zephyrbot assigned erwango May 17, 2024

josuah marked this pull request as draft May 17, 2024 14:32

josuah mentioned this pull request May 17, 2024

drivers: video: API enhancement #72959

Open

josuah marked this pull request as ready for review May 20, 2024 09:17

josuah force-pushed the pr-dts-flatten-video-port-endpoint branch from e4b132b to 045a59c Compare May 20, 2024 09:53

josuah mentioned this pull request May 20, 2024

drivers: video: Introduce macro to deal with endpoints #73023

Closed

erwango approved these changes May 21, 2024

View reviewed changes

josuah closed this May 28, 2024

josuah deleted the pr-dts-flatten-video-port-endpoint branch July 27, 2024 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dts: flatten video's device tree endpoints definition #72950

dts: flatten video's device tree endpoints definition #72950

josuah commented May 17, 2024 •

edited

Loading

josuah commented May 17, 2024

josuah commented May 20, 2024

loicpoulain commented May 20, 2024

josuah commented May 22, 2024

josuah commented May 28, 2024

josuah commented May 28, 2024

dts: flatten video's device tree endpoints definition #72950

dts: flatten video's device tree endpoints definition #72950

Conversation

josuah commented May 17, 2024 • edited Loading

Summary:

Details:

References

josuah commented May 17, 2024

josuah commented May 20, 2024

loicpoulain commented May 20, 2024

josuah commented May 22, 2024

josuah commented May 28, 2024

josuah commented May 28, 2024

josuah commented May 17, 2024 •

edited

Loading