Re: [PATCH 3/4] arm64: dts: mediatek: mt8167: Add DRM nodes

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 15/02/26 09:53, Luca Leonardo Scorcia ha scritto:
> Add all the DRM nodes required to get DSI to work on MT8167 SoC.
> 
> Signed-off-by: Luca Leonardo Scorcia <l.scorcia@gmail.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8167.dtsi | 386 +++++++++++++++++++++++
>   1 file changed, 386 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8167.dtsi b/arch/arm64/boot/dts/mediatek/mt8167.dtsi
> index 27cf32d7ae35..c6306234e592 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8167.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8167.dtsi
> @@ -16,6 +16,20 @@
>   / {
>   	compatible = "mediatek,mt8167";
>   
> +	aliases {
> +		aal0 = &aal;
> +		ccorr0 = &ccorr;
> +		color0 = &color;
> +		dither0 = &dither;
> +		dsi0 = &dsi;
> +		gamma0 = &gamma;
> +		ovl0 = &ovl0;
> +		pwm0 = &disp_pwm;
> +		rdma0 = &rdma0;
> +		rdma1 = &rdma1;
> +		wdma0 = &wdma;
> +	};
> +
>   	soc {
>   		topckgen: topckgen@10000000 {
>   			compatible = "mediatek,mt8167-topckgen", "syscon";
> @@ -120,10 +134,371 @@ iommu: m4u@10203000 {
>   			#iommu-cells = <1>;
>   		};
>   
> +		disp_pwm: pwm@1100f000 {
> +			compatible = "mediatek,mt8167-disp-pwm",
> +				     "mediatek,mt8173-disp-pwm";

The compatible strings fit in a single line. Up to 100 columns it's fine: for every
node additions that you're doing, please use a single line wherever you can.

> +			reg = <0 0x1100f000 0 0x1000>;
> +			clocks = <&mmsys CLK_MM_DISP_PWM_26M>,
> +				 <&mmsys CLK_MM_DISP_PWM_MM>;
> +			clock-names = "main",
> +				      "mm";

That also includes clock-names and, where possible, also clocks and anything
else really.

> +			power-domains = <&spm MT8167_POWER_DOMAIN_MM>;
> +			#pwm-cells = <2>;
> +			status = "disabled";
> +		};
> +
>   		mmsys: syscon@14000000 {
>   			compatible = "mediatek,mt8167-mmsys", "syscon";
>   			reg = <0 0x14000000 0 0x1000>;
> +			power-domains = <&spm MT8167_POWER_DOMAIN_MM>;
>   			#clock-cells = <1>;
> +
> +			port {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				mmsys_main: endpoint@0 {
> +					reg = <0>;
> +					remote-endpoint = <&ovl0_in>;
> +				};
> +
> +				mmsys_ext: endpoint@1 {
> +					reg = <1>;
> +					remote-endpoint = <&rdma1_in>;
> +				};

Nice! Thanks for expressing those connections with an OF Graph. That makes things
simpler for other devices based on the same SoC.

> +			};
> +		};
> +
> +		ovl0: ovl0@14007000 {
> +			compatible = "mediatek,mt8167-disp-ovl";
> +			reg = <0 0x14007000 0 0x1000>;
> +			clocks = <&mmsys CLK_MM_DISP_OVL0>;
> +			interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_LOW>;
> +			iommus = <&iommu M4U_PORT_DISP_OVL0>;
> +			power-domains = <&spm MT8167_POWER_DOMAIN_MM>;
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					reg = <0>;

For OVL, RDMA, COLOR, CCORR, and others, you have only one endpoint.

Since there is only one, you can compress the nodes and avoid the text bloat
(and please do), by declaring the nodes like:

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

				port@0 {
					reg = <0>;
					ovl0_in: endpoint {
						remote-endpoint = <&mmsys_main>;
					};
				};

				port@1 {
					reg = <1>;
					ovl0_out: endpoint {
						remote-endpoint = <&color_in>;
					};
				};
			};

Please do this for all nodes where you have only a single endpoint (so, for
everything but mmsys).


> +					ovl0_in: endpoint@0 {
> +						reg = <0>;
> +						remote-endpoint = <&mmsys_main>;
> +					};
> +				};
> +
> +				port@1 {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					reg = <1>;
> +					ovl0_out: endpoint@0 {
> +						reg = <0>;
> +						remote-endpoint = <&color_in>;
> +					};
> +				};
> +			};
> +		};

...snip...

> +
> +		dsi: dsi@14012000 {
> +			compatible = "mediatek,mt8167-dsi",
> +				     "mediatek,mt2701-dsi";
> +			reg = <0 0x14012000 0 0x1000>;
> +			clocks = <&mmsys CLK_MM_DSI_ENGINE>,
> +				 <&mmsys CLK_MM_DSI_DIGITAL>,
> +				 <&mipi_tx>;
> +			clock-names = "engine", "digital", "hs";
> +			interrupts = <GIC_SPI 171 IRQ_TYPE_LEVEL_LOW>;
> +			phys = <&mipi_tx>;
> +			phy-names = "dphy";
> +			power-domains = <&spm MT8167_POWER_DOMAIN_MM>;
> +			status = "disabled";
> +
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;

..snip..

> +
> +				port@1 {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					reg = <1>;
> +					dsi_out: endpoint@0 {
> +						reg = <0>;
> +					};
> +				};

That's shorter, and simpler:
				port@1 {
					reg = <1>;
					dsi_out: endpoint { };
				};

> +			};
> +		};
> +
> +		mutex: mutex@14015000 {
> +			compatible = "mediatek,mt8167-disp-mutex";
> +			reg = <0 0x14015000 0 0x1000>;
> +			interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_LOW>;
> +			power-domains = <&spm MT8167_POWER_DOMAIN_MM>;
>   		};
>   
>   		larb0: larb@14016000 {
> @@ -145,6 +520,17 @@ smi_common: smi@14017000 {
>   			power-domains = <&spm MT8167_POWER_DOMAIN_MM>;
>   		};
>   
> +		mipi_tx: dsi-phy@14018000 {
> +			compatible = "mediatek,mt8167-mipi-tx",
> +				     "mediatek,mt2701-mipi-tx";

Fits in a single line :-)

> +			reg = <0 0x14018000 0 0x90>;
> +			clocks = <&topckgen CLK_TOP_MIPI_26M_DBG>;
> +			clock-output-names = "mipi_tx0_pll";
> +			#clock-cells = <0>;
> +			#phy-cells = <0>;
> +			status = "disabled";
> +		};
> +
>   		imgsys: syscon@15000000 {
>   			compatible = "mediatek,mt8167-imgsys", "syscon";
>   			reg = <0 0x15000000 0 0x1000>;
Nice job, btw.

Cheers,
Angelo