Re: [PATCH v6 9/9] arm64: dts: rockchip: add the dcphy mipi csi-2 receiver nodes to rk3588

[Michael Riesch <michael.riesch@collabora.com>]

Hi Jason,

On 11/10/26 09:18, Jason Yang wrote:
> The MIPI0 and MIPI1 receiver units are connected to MIPI DCPHY0 and
> MIPI DCPHY1, whose receiver side is now supported. Add the two nodes and
> wire them to the VICAP ports.
> 
> The dsi nodes keep <&mipidcphy0 PHY_TYPE_DPHY>, the only value the
> binding had before the PHY receiver was added. PHY_TYPE_DPHY still
> selects the PHY transmitter in D-PHY mode. The csi nodes use the new
> PHY_DIRECTION_RX from the dt-bindings header. Neither phy node needs
> phy-type: the DSI host and the CSI-2 host both run the block in D-PHY
> mode, the default.
> 
> There is an ABI break if the dsi and csi nodes use the new cell values:
> an older kernel ignores phy-type and takes any cell value as the mode of
> the block. A DSI node with PHY_DIRECTION_TX then gets no display, and a
> CSI-2 node with PHY_DIRECTION_RX gets no camera.
> 
> To keep the ABI break small, the dsi nodes keep PHY_TYPE_DPHY, which
> older and newer kernels both accept. With the csi nodes disabled, the
> display therefore keeps working on either kernel.
> 
> The csi nodes have no older value to keep and have to use
> PHY_DIRECTION_RX. Enabling csi0 or csi1 therefore needs a PHY driver
> with receiver support. Since v7.0, a CSI-2 host driver probes the csi
> nodes. An older PHY driver then gives the CSI-2 host the PHY transmitter
> that drives the display. The camera gets no frames, and the display can
> stop working as well.

I think you can restrict the patch description to what the patch
actually does.

This patch modifies the device tree in a perfectly valid way as set out
in the DT bindings. Any breakage or disadvantage of the approach therein
should be discussed there.

> Signed-off-by: Jason Yang <jason98166@gmail.com>

But apart from that this LGTM!

Reviewed-by: Michael Riesch <michael.riesch@collabora.com>

Best regards,
Michael

> ---
>  arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 68 +++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> index aa69fd3793fd..c7174c2f15c3 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> @@ -1516,10 +1516,18 @@ vicap_dvp: port@0 {
>  
>  			vicap_mipi0: port@1 {
>  				reg = <0x1>;
> +
> +				vicap_mipi0_input: endpoint {
> +					remote-endpoint = <&csi0_output>;
> +				};
>  			};
>  
>  			vicap_mipi1: port@2 {
>  				reg = <0x2>;
> +
> +				vicap_mipi1_input: endpoint {
> +					remote-endpoint = <&csi1_output>;
> +				};
>  			};
>  
>  			vicap_mipi2: port@3 {
> @@ -1568,6 +1576,66 @@ vicap_mmu: iommu@fdce0800 {
>  		status = "disabled";
>  	};
>  
> +	csi0: csi@fdd10000 {
> +		compatible = "rockchip,rk3588-mipi-csi2", "rockchip,rk3568-mipi-csi2";
> +		reg = <0x0 0xfdd10000 0x0 0x10000>;
> +		interrupts = <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH 0>;
> +		interrupt-names = "err1", "err2";
> +		clocks = <&cru PCLK_CSI_HOST_0>;
> +		phys = <&mipidcphy0 PHY_DIRECTION_RX>;
> +		power-domains = <&power RK3588_PD_VI>;
> +		resets = <&cru SRST_P_CSI_HOST_0>;
> +		status = "disabled";
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			csi0_in: port@0 {
> +				reg = <0>;
> +			};
> +
> +			csi0_out: port@1 {
> +				reg = <1>;
> +
> +				csi0_output: endpoint {
> +					remote-endpoint = <&vicap_mipi0_input>;
> +				};
> +			};
> +		};
> +	};
> +
> +	csi1: csi@fdd20000 {
> +		compatible = "rockchip,rk3588-mipi-csi2", "rockchip,rk3568-mipi-csi2";
> +		reg = <0x0 0xfdd20000 0x0 0x10000>;
> +		interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH 0>;
> +		interrupt-names = "err1", "err2";
> +		clocks = <&cru PCLK_CSI_HOST_1>;
> +		phys = <&mipidcphy1 PHY_DIRECTION_RX>;
> +		power-domains = <&power RK3588_PD_VI>;
> +		resets = <&cru SRST_P_CSI_HOST_1>;
> +		status = "disabled";
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			csi1_in: port@0 {
> +				reg = <0>;
> +			};
> +
> +			csi1_out: port@1 {
> +				reg = <1>;
> +
> +				csi1_output: endpoint {
> +					remote-endpoint = <&vicap_mipi1_input>;
> +				};
> +			};
> +		};
> +	};
> +
>  	csi2: csi@fdd30000 {
>  		compatible = "rockchip,rk3588-mipi-csi2", "rockchip,rk3568-mipi-csi2";
>  		reg = <0x0 0xfdd30000 0x0 0x10000>;
>