Re: [PATCH 09/10] arm64: dts: rockchip: Add rk3576 SoC base DT

[Johan Jonker <jbx6244@yandex.com>]

Some comments below. Whenever useful.

On 8/2/24 23:45, Detlev Casanova wrote:
> This device tree contains all devices necessary for booting from network
> or SD Card.
> 
> It supports CPU, CRU, PM domains, dma, interrupts, timers, UART and
> SDHCI (everything necessary to boot Linux on this system on chip) as
> well as Ethernet, I2C, SPI and OTP.
> 
> Also add the necessary DT bindings for the SoC.
> 
> Signed-off-by: Liang Chen <cl@rock-chips.com>
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> [rebase, squash and reword commit message]
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---

[..]

> diff --git a/arch/arm64/boot/dts/rockchip/rk3576.dtsi b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
> new file mode 100644
> index 0000000000000..00c4d2a153ced
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
> @@ -0,0 +1,1635 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2023 Rockchip Electronics Co., Ltd.
> + */
> +

> +#include <dt-bindings/clock/rockchip,rk3576-cru.h>
> +#include <dt-bindings/reset/rockchip,rk3576-cru.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/phy/phy.h>
> +#include <dt-bindings/power/rk3576-power.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +#include <dt-bindings/soc/rockchip,boot-mode.h>

Sort includes.

> +
> +/ {
> +	compatible = "rockchip,rk3576";
> +
> +	interrupt-parent = <&gic>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	aliases {
> +		ethernet0 = &gmac0;
> +		ethernet1 = &gmac1;
> +		gpio0 = &gpio0;
> +		gpio1 = &gpio1;
> +		gpio2 = &gpio2;
> +		gpio3 = &gpio3;
> +		gpio4 = &gpio4;
> +		i2c0 = &i2c0;
> +		i2c1 = &i2c1;
> +		i2c2 = &i2c2;
> +		i2c3 = &i2c3;
> +		i2c4 = &i2c4;
> +		i2c5 = &i2c5;
> +		i2c6 = &i2c6;
> +		i2c7 = &i2c7;
> +		i2c8 = &i2c8;
> +		i2c9 = &i2c9;
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;
> +		serial3 = &uart3;
> +		serial4 = &uart4;
> +		serial5 = &uart5;
> +		serial6 = &uart6;
> +		serial7 = &uart7;
> +		serial8 = &uart8;
> +		serial9 = &uart9;
> +		serial10 = &uart10;
> +		serial11 = &uart11;
> +		spi0 = &spi0;
> +		spi1 = &spi1;
> +		spi2 = &spi2;
> +		spi3 = &spi3;
> +		spi4 = &spi4;
> +	};
> +

[..]

For uart0..uart11:

> +
> +	uart1: serial@27310000 {
> +		compatible = "rockchip,rk3576-uart", "snps,dw-apb-uart";
> +		reg = <0x0 0x27310000 0x0 0x100>;

> +		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;

"interrupts" are sort just like other properties. A mix of sort styles exists, so check all nodes.

> +		clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>;
> +		clock-names = "baudclk", "apb_pclk";

> +		reg-shift = <2>;
> +		reg-io-width = <4>;

Move below "reg". 

> +		dmas = <&dmac0 8>, <&dmac0 9>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&uart1m0_xfer>;
> +		status = "disabled";
> +	};
> +
> +	pmu: power-management@27380000 {
> +		compatible = "rockchip,rk3576-pmu", "syscon", "simple-mfd";
> +		reg = <0x0 0x27380000 0x0 0x800>;
> +
> +		power: power-controller {
> +			compatible = "rockchip,rk3576-power-controller";
> +			#power-domain-cells = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;

> +			status = "okay";

Remove.

> +
> +			power-domain@RK3576_PD_NPU {
> +				reg = <RK3576_PD_NPU>;

> +				#power-domain-cells = <0>;

				#power-domain-cells = <1>;

      "#power-domain-cells":
        enum: [0, 1]
        description:
          Must be 0 for nodes representing a single PM domain and 1 for nodes
          providing multiple PM domains.

> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				power-domain@RK3576_PD_NPUTOP {
> +					reg = <RK3576_PD_NPUTOP>;

> +					#power-domain-cells = <0>;


					#power-domain-cells = <1>;
      "#power-domain-cells":
        enum: [0, 1]
        description:
          Must be 0 for nodes representing a single PM domain and 1 for nodes
          providing multiple PM domains.

> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					clocks = <&cru ACLK_RKNN0>,
> +						 <&cru ACLK_RKNN1>,
> +						 <&cru ACLK_RKNN_CBUF>,
> +						 <&cru CLK_RKNN_DSU0>,
> +						 <&cru HCLK_RKNN_CBUF>,
> +						 <&cru HCLK_RKNN_ROOT>,
> +						 <&cru HCLK_NPU_CM0_ROOT>,
> +						 <&cru PCLK_NPUTOP_ROOT>;
> +					pm_qos = <&qos_npu_mcu>,
> +						 <&qos_npu_nsp0>,
> +						 <&qos_npu_nsp1>,
> +						 <&qos_npu_m0ro>,
> +						 <&qos_npu_m1ro>;
> +
> +					power-domain@RK3576_PD_NPU0 {
> +						reg = <RK3576_PD_NPU0>;
> +						#power-domain-cells = <0>;
> +						clocks = <&cru HCLK_RKNN_ROOT>,
> +							 <&cru ACLK_RKNN0>;
> +						pm_qos = <&qos_npu_m0>;
> +					};
> +					power-domain@RK3576_PD_NPU1 {
> +						reg = <RK3576_PD_NPU1>;
> +						#power-domain-cells = <0>;
> +						clocks = <&cru HCLK_RKNN_ROOT>,
> +							 <&cru ACLK_RKNN1>;
> +						pm_qos = <&qos_npu_m1>;
> +					};
> +				};
> +			};
> +
> +			power-domain@RK3576_PD_GPU {
> +				reg = <RK3576_PD_GPU>;
> +				#power-domain-cells = <0>;
> +				clocks = <&cru CLK_GPU>;
> +				pm_qos = <&qos_gpu>;
> +			};
> +
> +			power-domain@RK3576_PD_NVM {
> +				reg = <RK3576_PD_NVM>;

> +				#power-domain-cells = <0>;

				#power-domain-cells = <1>;
      "#power-domain-cells":
        enum: [0, 1]
        description:
          Must be 0 for nodes representing a single PM domain and 1 for nodes
          providing multiple PM domains.

> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				clocks = <&cru ACLK_EMMC>, <&cru HCLK_EMMC>;
> +				pm_qos = <&qos_emmc>,
> +					 <&qos_fspi0>;
> +
> +				power-domain@RK3576_PD_SDGMAC {
> +					reg = <RK3576_PD_SDGMAC>;

> +				#power-domain-cells = <0>;

TAB ??

> +					clocks = <&cru ACLK_HSGPIO>,
> +						 <&cru ACLK_GMAC0>,
> +						 <&cru ACLK_GMAC1>,
> +						 <&cru CCLK_SRC_SDIO>,
> +						 <&cru CCLK_SRC_SDMMC0>,
> +						 <&cru HCLK_HSGPIO>,
> +						 <&cru HCLK_SDIO>,
> +						 <&cru HCLK_SDMMC0>;
> +					pm_qos = <&qos_fspi1>,
> +						 <&qos_gmac0>,
> +						 <&qos_gmac1>,
> +						 <&qos_sdio>,
> +						 <&qos_sdmmc>,
> +						 <&qos_flexbus>;
> +				};
> +			};
> +			power-domain@RK3576_PD_PHP {
> +				reg = <RK3576_PD_PHP>;

> +				#power-domain-cells = <0>;

				#power-domain-cells = <1>;
      "#power-domain-cells":
        enum: [0, 1]
        description:
          Must be 0 for nodes representing a single PM domain and 1 for nodes
          providing multiple PM domains.

> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				clocks = <&cru ACLK_PHP_ROOT>,
> +					 <&cru PCLK_PHP_ROOT>,
> +					 <&cru ACLK_MMU0>,
> +					 <&cru ACLK_MMU1>;
> +				pm_qos = <&qos_mmu0>,
> +					 <&qos_mmu1>;
> +
> +				power-domain@RK3576_PD_SUBPHP {
> +					reg = <RK3576_PD_SUBPHP>;

> +				#power-domain-cells = <0>;

TAB??

> +				};
> +			};
> +			power-domain@RK3576_PD_AUDIO {
> +				reg = <RK3576_PD_AUDIO>;
> +				#power-domain-cells = <0>;
> +			};
> +			power-domain@RK3576_PD_VEPU1 {

> +				clocks = <&cru ACLK_VEPU1>,
> +					 <&cru HCLK_VEPU1>;
> +				reg = <RK3576_PD_VEPU1>;

sort

> +				#power-domain-cells = <0>;
> +				pm_qos = <&qos_vepu1>;
> +			};
> +			power-domain@RK3576_PD_VPU {
> +				reg = <RK3576_PD_VPU>;
> +				#power-domain-cells = <0>;
> +				clocks = <&cru ACLK_EBC>,
> +					 <&cru HCLK_EBC>,
> +					 <&cru ACLK_RGA2E_0>,
> +					 <&cru HCLK_RGA2E_0>,
> +					 <&cru ACLK_RGA2E_1>,
> +					 <&cru HCLK_RGA2E_1>,
> +					 <&cru ACLK_JPEG>,
> +					 <&cru HCLK_JPEG>,
> +					 <&cru ACLK_VDPP>,
> +					 <&cru HCLK_VDPP>;
> +				pm_qos = <&qos_ebc>,
> +					 <&qos_rga0>,
> +					 <&qos_rga1>,
> +					 <&qos_jpeg>,
> +					 <&qos_vdpp>;
> +			};
> +			power-domain@RK3576_PD_VDEC {
> +				reg = <RK3576_PD_VDEC>;
> +				#power-domain-cells = <0>;
> +				clocks = <&cru ACLK_RKVDEC_ROOT>,
> +					 <&cru HCLK_RKVDEC>;
> +				pm_qos = <&qos_rkvdec>;
> +			};
> +			power-domain@RK3576_PD_VI {
> +				reg = <RK3576_PD_VI>;

> +				#power-domain-cells = <0>;

				#power-domain-cells = <1>;
      "#power-domain-cells":
        enum: [0, 1]
        description:
          Must be 0 for nodes representing a single PM domain and 1 for nodes

> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				clocks = <&cru ACLK_VICAP>,
> +					 <&cru HCLK_VICAP>,
> +					 <&cru DCLK_VICAP>,
> +					 <&cru ACLK_VI_ROOT>,
> +					 <&cru HCLK_VI_ROOT>,
> +					 <&cru PCLK_VI_ROOT>,
> +					 <&cru CLK_ISP_CORE>,
> +					 <&cru ACLK_ISP>,
> +					 <&cru HCLK_ISP>,
> +					 <&cru CLK_CORE_VPSS>,
> +					 <&cru ACLK_VPSS>,
> +					 <&cru HCLK_VPSS>;
> +				pm_qos = <&qos_isp_mro>,
> +					 <&qos_isp_mwo>,
> +					 <&qos_vicap_m0>,
> +					 <&qos_vpss_mro>,
> +					 <&qos_vpss_mwo>;
> +
> +				power-domain@RK3576_PD_VEPU0 {
> +					reg = <RK3576_PD_VEPU0>;
> +					#power-domain-cells = <0>;
> +					clocks = <&cru ACLK_VEPU0>,
> +						 <&cru HCLK_VEPU0>;
> +					pm_qos = <&qos_vepu0>;
> +				};
> +			};
> +			power-domain@RK3576_PD_VOP {
> +				reg = <RK3576_PD_VOP>;

> +				#power-domain-cells = <0>;

				#power-domain-cells = <1>;
      "#power-domain-cells":
        enum: [0, 1]
        description:
          Must be 0 for nodes representing a single PM domain and 1 for nodes
          providing multiple PM domains.

> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				clocks = <&cru ACLK_VOP>,
> +					 <&cru HCLK_VOP>,
> +					 <&cru HCLK_VOP_ROOT>;
> +				pm_qos = <&qos_vop_m0>,
> +					 <&qos_vop_m1ro>;
> +
> +				power-domain@RK3576_PD_USB {

Since when is USB part of VOP?
Recheck?

> +					reg = <RK3576_PD_USB>;
> +					#power-domain-cells = <0>;
> +					clocks = <&cru PCLK_PHP_ROOT>,
> +						 <&cru ACLK_USB_ROOT>,
> +						 <&cru ACLK_MMU2>,
> +						 <&cru ACLK_SLV_MMU2>,
> +						 <&cru ACLK_UFS_SYS>;
> +					pm_qos = <&qos_mmu2>,
> +						 <&qos_ufshc>;
> +				};
> +				power-domain@RK3576_PD_VO0 {
> +					reg = <RK3576_PD_VO0>;
> +					#power-domain-cells = <0>;
> +					clocks = <&cru ACLK_HDCP0>,
> +						 <&cru HCLK_HDCP0>,
> +						 <&cru ACLK_VO0_ROOT>,
> +						 <&cru PCLK_VO0_ROOT>,
> +						 <&cru HCLK_VOP_ROOT>;
> +					pm_qos = <&qos_hdcp0>;
> +				};
> +				power-domain@RK3576_PD_VO1 {
> +					reg = <RK3576_PD_VO1>;
> +					#power-domain-cells = <0>;
> +					clocks = <&cru ACLK_HDCP1>,
> +						 <&cru HCLK_HDCP1>,
> +						 <&cru ACLK_VO1_ROOT>,
> +						 <&cru PCLK_VO1_ROOT>,
> +						 <&cru HCLK_VOP_ROOT>;
> +					pm_qos = <&qos_hdcp1>;
> +				};
> +			};
> +		};
> +	};
> +

[..]

> +

> +	rktimer: timer@2acc0000 {

timer0: .....

> +		compatible = "rockchip,rk3576-timer", "rockchip,rk3288-timer";
> +		reg = <0x0 0x2acc0000 0x0 0x20>;

> +		interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;

sort

> +		clocks = <&cru PCLK_BUSTIMER0>, <&cru CLK_TIMER0>;
> +		clock-names = "pclk", "timer";
> +	};
> +
> +	wdt: watchdog@2ace0000 {

> +		compatible = "snps,dw-wdt";

Must be SoC related.
Add something to snps,dw-wdt.yaml

> +		reg = <0x0 0x2ace0000 0x0 0x100>;
> +		clocks = <&cru TCLK_WDT0>, <&cru PCLK_WDT0>;
> +		clock-names = "tclk", "pclk";
> +		interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> +		status = "disabled";
> +	};
> +

For spi0..spi4:


> +	spi0: spi@2acf0000 {

> +		compatible = "rockchip,rk3066-spi";

Must be SoC related.
Add something to spi-rockchip.yaml

> +		reg = <0x0 0x2acf0000 0x0 0x1000>;

> +		interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>;

sort

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

Move things that start with # down the list as posible.

> +		clocks = <&cru CLK_SPI0>, <&cru PCLK_SPI0>;
> +		clock-names = "spiclk", "apb_pclk";
> +		dmas = <&dmac0 14>, <&dmac0 15>;
> +		dma-names = "tx", "rx";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&spi0m0_csn0 &spi0m0_csn1 &spi0m0_pins>;
> +		num-cs = <2>;
> +		status = "disabled";
> +	};
> +
> +	spi1: spi@2ad00000 {

> +	spi2: spi@2ad10000 {

> +	spi3: spi@2ad20000 {

> +	spi4: spi@2ad30000 {

> +	uart0: serial@2ad40000 {

> +	uart2: serial@2ad50000 {

> +	uart3: serial@2ad60000 {

> +	uart4: serial@2ad70000 {

> +	uart5: serial@2ad80000 {

> +	uart6: serial@2ad90000 {

> +	uart7: serial@2ada0000 {

> +	uart8: serial@2adb0000 {

> +	uart9: serial@2adc0000 {

> +	saradc: adc@2ae00000 {
> +		compatible = "rockchip,rk3576-saradc", "rockchip,rk3588-saradc";
> +		reg = <0x0 0x2ae00000 0x0 0x10000>;

> +		interrupts = <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>;

sort

> +		#io-channel-cells = <1>;

Things that start with "#" are used to interpreted the DT, so down the list as possible.

> +		clocks = <&cru CLK_SARADC>, <&cru PCLK_SARADC>;
> +		clock-names = "saradc", "apb_pclk";
> +		resets = <&cru SRST_P_SARADC>;
> +		reset-names = "saradc-apb";
> +		status = "disabled";
> +	};
> +

[..]

> +
> +	uart10: serial@2afc0000 {
> +
> +	uart11: serial@2afd0000 {
> +

[..]

> +
> +	pinctrl: pinctrl {
> +		compatible = "rockchip,rk3576-pinctrl";
> +		rockchip,grf = <&ioc_grf>;
> +		rockchip,sys-grf = <&sys_grf>;
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +

> +		gpio0: gpio@27320000 {

The use of gpio nodes as subnode of pinctrl is deprecated.

patternProperties:
  "gpio@[0-9a-f]+$":
    type: object

    $ref: /schemas/gpio/rockchip,gpio-bank.yaml#
    deprecated: true

    unevaluatedProperties: false

> +			compatible = "rockchip,gpio-bank";

When in use as separate node the compatible must be SoC related.

Question for the maintainers: Extra entry to rockchip,gpio-bank.yaml ??

> +			reg = <0x0 0x27320000 0x0 0x200>;
> +			interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cru PCLK_GPIO0>, <&cru DBCLK_GPIO0>;
> +
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-ranges = <&pinctrl 0 0 32>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		gpio1: gpio@2ae10000 {
> +
> +		gpio2: gpio@2ae20000 {
> +
> +		gpio3: gpio@2ae30000 {
> +
> +		gpio4: gpio@2ae40000 {
> +	};
> +};
> +
> +#include "rk3576-pinctrl.dtsi"