[PATCH V3 3/3] arm64: dts: rockchip: Add DSI and DSI-DPHY nodes to rk356x

[Michael Riesch]

Hi Chris,

Thanks for your efforts, nice work!

A few minor comments below:

On 9/12/22 22:56, Chris Morgan wrote:
> From: Chris Morgan <macromorgan at hotmail.com>
> 
> This adds the DSI controller nodes and DSI-DPHY controller nodes to the
> rk356x device tree.
> 
> Signed-off-by: Chris Morgan <macromorgan at hotmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 72 ++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> index 319981c3e9f7..d150568fde82 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> @@ -699,6 +699,54 @@ vop_mmu: iommu at fe043e00 {
>  		status = "disabled";
>  	};
>  
> +	dsi0: dsi at fe060000 {
> +		compatible = "rockchip,rk3568-mipi-dsi", "snps,dw-mipi-dsi";
> +		reg = <0x00 0xfe060000 0x00 0x10000>;
> +		interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
> +		clock-names = "pclk", "hclk";
> +		clocks = <&cru PCLK_DSITX_0>, <&cru HCLK_VO>;
> +		phy-names = "dphy";
> +		phys = <&mipi_dphy0>;
> +		power-domains = <&power RK3568_PD_VO>;
> +		reset-names = "apb";
> +		resets = <&cru SRST_P_DSITX_0>;
> +		rockchip,grf = <&grf>;
> +		status = "disabled";
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port at 0 {

We'll have to reference that port in the board dts, right? A label would
be helpful, e.g., dsi0_in.

> +				reg = <0>;
> +			};

Would it make sense to add the dsi0_out port at this point?

dsi0_out: port at 1 {
	reg = <1>;
};

> +		};
> +	};
> +
> +	dsi1: dsi at fe070000 {
> +		compatible = "rockchip,rk3568-mipi-dsi", "snps,dw-mipi-dsi";
> +		reg = <0x0 0xfe070000 0x0 0x10000>;
> +		interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
> +		clock-names = "pclk", "hclk";
> +		clocks = <&cru PCLK_DSITX_1>, <&cru HCLK_VO>;
> +		phy-names = "dphy";
> +		phys = <&mipi_dphy1>;
> +		power-domains = <&power RK3568_PD_VO>;
> +		reset-names = "apb";
> +		resets = <&cru SRST_P_DSITX_1>;
> +		rockchip,grf = <&grf>;
> +		status = "disabled";
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port at 0 {

Ditto.

> +				reg = <0>;
> +			};
> +		};
> +	};
> +
>  	hdmi: hdmi at fe0a0000 {
>  		compatible = "rockchip,rk3568-dw-hdmi";
>  		reg = <0x0 0xfe0a0000 0x0 0x20000>;
> @@ -1594,6 +1642,30 @@ combphy2: phy at fe840000 {
>  		status = "disabled";
>  	};
>  
> +	mipi_dphy0: mipi-dphy at fe850000 {

May I suggest to call this one "dsi_dphy0" (analogous to "csi_dphy")?

> +		compatible = "rockchip,rk3568-dsi-dphy";
> +		reg = <0x0 0xfe850000 0x0 0x10000>;
> +		clock-names = "ref", "pclk";
> +		clocks = <&pmucru CLK_MIPIDSIPHY0_REF>, <&cru PCLK_MIPIDSIPHY0>;
> +		#phy-cells = <0>;
> +		power-domains = <&power RK3568_PD_VO>;
> +		reset-names = "apb";
> +		resets = <&cru SRST_P_MIPIDSIPHY0>;
> +		status = "disabled";
> +	};
> +
> +	mipi_dphy1: mipi-dphy at fe860000 {

Ditto (well, "dsi_dphy1" obviously).

Best regards,
Michael

> +		compatible = "rockchip,rk3568-dsi-dphy";
> +		reg = <0x0 0xfe860000 0x0 0x10000>;
> +		clock-names = "ref", "pclk";
> +		clocks = <&pmucru CLK_MIPIDSIPHY1_REF>, <&cru PCLK_MIPIDSIPHY1>;
> +		#phy-cells = <0>;
> +		power-domains = <&power RK3568_PD_VO>;
> +		reset-names = "apb";
> +		resets = <&cru SRST_P_MIPIDSIPHY1>;
> +		status = "disabled";
> +	};
> +
>  	usb2phy0: usb2phy at fe8a0000 {
>  		compatible = "rockchip,rk3568-usb2phy";
>  		reg = <0x0 0xfe8a0000 0x0 0x10000>;