Re: [PATCHv1 18/19] arm64: dts: rockchip: Add base DT for rk3588 SoC

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 22/04/2022 19:09, Sebastian Reichel wrote:

Thank you for your patch. There is something to discuss/improve.

> +
> +	clocks {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;

There are no children with unit addresses... this should not be a simple
bus.

> +
> +		spll: spll {

Generic node names please, so either "clock-0" or "spll-clock" etc.

> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <702000000>;
> +			clock-output-names = "spll";
> +		};
> +
> +		xin24m: xin24m {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <24000000>;
> +			clock-output-names = "xin24m";
> +		};
> +
> +		xin32k: xin32k {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <32768>;
> +			clock-output-names = "xin32k";
> +		};
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu-map {
> +			cluster0 {
> +				core0 {
> +					cpu = <&cpu_l0>;
> +				};
> +			};
> +		};
> +
> +		cpu_l0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +			capacity-dmips-mhz = <530>;
> +			clocks = <&scmi_clk SCMI_CLK_CPUL>;
> +			i-cache-size = <32768>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <128>;
> +			d-cache-size = <32768>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <128>;
> +			next-level-cache = <&l2_cache_l0>;
> +			#cooling-cells = <2>;
> +			dynamic-power-coefficient = <228>;
> +		};
> +
> +		l2_cache_l0: l2-cache-l0 {
> +			compatible = "cache";
> +			cache-size = <131072>;
> +			cache-line-size = <64>;
> +			cache-sets = <512>;
> +			next-level-cache = <&l3_cache>;
> +		};
> +
> +		l3_cache: l3-cache {
> +			compatible = "cache";
> +			cache-size = <3145728>;
> +			cache-line-size = <64>;
> +			cache-sets = <4096>;
> +		};
> +	};
> +
> +	arm-pmu {

Generic node name, so just "pmu" unless there is goign to be a another
PMU node?

> +		compatible = "arm,armv8-pmuv3";
> +		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
> +		interrupt-affinity = <&cpu_l0>;
> +	};
> +
> +	firmware {
> +		optee: optee {
> +			compatible = "linaro,optee-tz";
> +			method = "smc";
> +		};
> +
> +		scmi: scmi {
> +			compatible = "arm,scmi-smc";
> +			shmem = <&scmi_shmem>;
> +			arm,smc-id = <0x82000010>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			scmi_clk: protocol@14 {
> +				reg = <0x14>;
> +				#clock-cells = <1>;
> +
> +				assigned-clocks = <&scmi_clk SCMI_SPLL>;
> +				assigned-clock-rates = <700000000>;
> +			};
> +
> +			scmi_reset: protocol@16 {
> +				reg = <0x16>;
> +				#reset-cells = <1>;
> +			};
> +		};
> +
> +		sdei: sdei {
> +			compatible = "arm,sdei-1.0";
> +			method = "smc";
> +		};
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> +	};
> +
> +	sram@10f000 {
> +		compatible = "mmio-sram";
> +		reg = <0x0 0x0010f000 0x0 0x100>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0x0 0x0010f000 0x100>;
> +
> +		scmi_shmem: sram@0 {
> +			compatible = "arm,scmi-shmem";
> +			reg = <0x0 0x100>;
> +		};
> +	};
> +
> +	php_grf: syscon@fd5b0000 {
> +		compatible = "rockchip,rk3588-php-grf", "syscon";
> +		reg = <0x0 0xfd5b0000 0x0 0x1000>;
> +	};
> +
> +	ioc: syscon@fd5f0000 {
> +		compatible = "rockchip,rk3588-ioc", "syscon";
> +		reg = <0x0 0xfd5f0000 0x0 0x10000>;
> +	};
> +
> +	syssram: sram@fd600000 {
> +		compatible = "mmio-sram";
> +		reg = <0x0 0xfd600000 0x0 0x100000>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x0 0xfd600000 0x100000>;

No children here, so why do you need it?

> +	};
> +

Best regards,
Krzysztof