Re: [PATCH 08/12] arm64: dts: rockchip: rk356x: Add VOP2 nodes

[Johan Jonker <jbx6244@gmail.com>]

Hi Sascha,


On 11/17/21 3:33 PM, Sascha Hauer wrote:
> The VOP2 is the display output controller on the RK3568. Add the node
> for it to the dtsi file along with the required display-subsystem node
> and the iommu node.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 52 ++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> index 46d9552f60284..6ebf7c14e096a 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> @@ -447,6 +447,58 @@ gmac1_mtl_tx_setup: tx-queues-config {
>  		};
>  	};
>  

> +	display_subsystem: display-subsystem {
> +		compatible = "rockchip,display-subsystem";
> +		ports = <&vop_out>;
> +	};

Some DT sort rules:

For nodes:
Sort things without reg alphabetical first,
then sort the rest by reg address.

> +
> +	vop: vop@fe040000 {

> +		compatible = "rockchip,rk3568-vop";

From rockchip-vop2.yaml:
+properties:
+  compatible:
+    enum:

+      - rockchip,rk3568-vop
+      - rockchip,rk3566-vop

Maybe sort yaml compatibles in alphabetical order.

rockchip,rk3566-vop is not used in the dtsi I think.

Comment by Andy Yan:
> 
> But take care that the vop on rk3566 has a special limitation: there are 
> three
> 
> windows(Cluster1/Esmart1/Smart1) that have a mirror lock, that means they
> 
> can't be programed framebuffer address independently , they can only
> 
> share framebuffer address with Cluster0/Esmart0/Smart0.

Question:
Given Andy's comment could someone explain weather the vop and hdmi
nodes should be in rk3566.dtsi and rk3568.dtsi with an extra
rockchip,rk3566-dw-hdmi compatible?

> +		reg = <0x0 0xfe040000 0x0 0x3000>, <0x0 0xfe044000 0x0 0x1000>;
> +		reg-names = "regs", "gamma_lut";

> +		rockchip,grf = <&grf>;
Heiko's sort rules:

compatible
reg
interrupts
[alphabetical]
status [if needed]

> +		interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>, <&cru DCLK_VOP0>, <&cru DCLK_VOP1>, <&cru DCLK_VOP2>;
> +		clock-names = "aclk_vop", "hclk_vop", "dclk_vp0", "dclk_vp1", "dclk_vp2";
> +		iommus = <&vop_mmu>;
> +		power-domains = <&power RK3568_PD_VO>;
> +		status = "disabled";
> +
> +		vop_out: ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			vp0: port@0 {

> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <0>;

My incomplete list:

Inside nodes:
If exists on top: compatible, reg and interrupts.
In alphabetical order the required properties.
Then in alphabetical order the other properties.
And as last things that start with '#' in alphabetical order.
Add status below all other properties for soc internal components with
any board-specifics.

> +			};
> +
> +			vp1: port@1 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <1>;
> +			};
> +
> +			vp2: port@2 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <2>;
> +			};
> +		};
> +	};
> +
> +	vop_mmu: iommu@fe043e00 {
> +		compatible = "rockchip,rk3568-iommu";
> +		reg = <0x0 0xfe043e00 0x0 0x100>, <0x0 0xfe043f00 0x0 0x100>;
> +		interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;

> +		interrupt-names = "vop_mmu";

ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml

arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dt.yaml: iommu@fe043e00:
'interrupt-names' does not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml

> +		clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>;
> +		clock-names = "aclk", "iface";
> +		#iommu-cells = <0>;
> +		status = "disabled";
> +	};
> +
>  	qos_gpu: qos@fe128000 {
>  		compatible = "rockchip,rk3568-qos", "syscon";
>  		reg = <0x0 0xfe128000 0x0 0x20>;
>