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

From: Robin Murphy
Date: Fri Apr 22 2022 - 14:26:11 EST


On 2022-04-22 18:09, Sebastian Reichel wrote:
From: Kever Yang <kever.yang@xxxxxxxxxxxxxx>

This initial version supports (single core) CPU, dma, interrupts, timers,
UART and SDHCI. In short - everything necessary to boot Linux on this
system on chip.

The DT is split into rk3588 and rk3588s, which is a reduced version
(i.e. with less peripherals) of the former.

Signed-off-by: Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx>
Signed-off-by: Elaine Zhang <zhangqing@xxxxxxxxxxxxxx>
Signed-off-by: Sugar Zhang <sugar.zhang@xxxxxxxxxxxxxx>
Signed-off-by: Kever Yang <kever.yang@xxxxxxxxxxxxxx>
[rebase, squash and reword commit message]
Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
---
arch/arm64/boot/dts/rockchip/rk3588.dtsi | 6 +
arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 501 ++++++++++++++++++++++
include/dt-bindings/clock/rk3588-cru.h | 1 +
3 files changed, 508 insertions(+)
create mode 100644 arch/arm64/boot/dts/rockchip/rk3588.dtsi
create mode 100644 arch/arm64/boot/dts/rockchip/rk3588s.dtsi

diff --git a/arch/arm64/boot/dts/rockchip/rk3588.dtsi b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
new file mode 100644
index 000000000000..ddb3ccff1299
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2021 Rockchip Electronics Co., Ltd.
+ */
+
+#include "rk3588s.dtsi"
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
new file mode 100644
index 000000000000..f7d3ad4384b3
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -0,0 +1,501 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2021 Rockchip Electronics Co., Ltd.
+ */
+
+#include <dt-bindings/clock/rk3588-cru.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+
+/ {
+ compatible = "rockchip,rk3588";
+
+ interrupt-parent = <&gic>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ aliases {
+ serial0 = &uart0;
+ serial1 = &uart1;
+ serial2 = &uart2;
+ serial3 = &uart3;
+ serial4 = &uart4;
+ serial5 = &uart5;
+ serial6 = &uart6;
+ serial7 = &uart7;
+ serial8 = &uart8;
+ serial9 = &uart9;
+ };
+
+ clocks {
+ compatible = "simple-bus";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;

I'm pretty sure that doing clocks as fake buses fell out of favour long ago.

+
+ spll: spll {
+ 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";
+ };

Do those two really belong in the SoC DTSI? On previous SoCs they're typically external inputs, and while the 24MHz is usually a crystal which can be largely taken for granted, the 32KHz is often provided by an RTC chip or similar which might need proper modelling.

+ };
+
+ 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>;
+ };

Is there any particular reason for not including more of the CPUs?

+
+ 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 {
+ compatible = "arm,armv8-pmuv3";

Please use the correct Cortex-A55 compatible.

+ interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-affinity = <&cpu_l0>;

Is affinity meaningful for a single CPU? If this is going to need to be a partitioned PPI once the Cortex-A76 PMU shows up as well, start as you mean to go on.

+ };
+
+ 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)>;

A mask representing all 4 of one (of 8) CPUs, for a GICv2 which we don't have? I doubt it ;)

+ };
+
+ 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>;
+ };
+
+ cru: clock-controller@fd7c0000 {
+ compatible = "rockchip,rk3588-cru";
+ rockchip,grf = <&php_grf>;
+ reg = <0x0 0xfd7c0000 0x0 0x5c000>;
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+
+ assigned-clocks =
+ <&cru PLL_PPLL>, <&cru PLL_AUPLL>,
+ <&cru PLL_NPLL>, <&cru PLL_GPLL>,
+ <&cru ACLK_CENTER_ROOT>,
+ <&cru HCLK_CENTER_ROOT>, <&cru ACLK_CENTER_LOW_ROOT>,
+ <&cru ACLK_TOP_ROOT>, <&cru PCLK_TOP_ROOT>,
+ <&cru ACLK_LOW_TOP_ROOT>, <&cru PCLK_PMU0_ROOT>,
+ <&cru HCLK_PMU_CM0_ROOT>, <&cru ACLK_VOP>,
+ <&cru ACLK_BUS_ROOT>, <&cru CLK_150M_SRC>,
+ <&cru CLK_GPU>;
+ assigned-clock-rates =
+ <100000000>, <786432000>,
+ <850000000>, <1188000000>,
+ <702000000>,
+ <400000000>, <500000000>,
+ <800000000>, <100000000>,
+ <400000000>, <100000000>,
+ <200000000>, <500000000>,
+ <375000000>, <150000000>,
+ <200000000>;
+ };
+
+ sdhci: mmc@fe2e0000 {
+ compatible = "rockchip,rk3588-dwcmshc", "snps,dwcmshc-sdhci";
+ reg = <0x0 0xfe2e0000 0x0 0x10000>;
+ interrupts = <GIC_SPI 205 IRQ_TYPE_LEVEL_HIGH>;
+ assigned-clocks = <&cru BCLK_EMMC>, <&cru TMCLK_EMMC>, <&cru CCLK_EMMC>;
+ assigned-clock-rates = <200000000>, <24000000>, <200000000>;
+ clocks = <&cru CCLK_EMMC>, <&cru HCLK_EMMC>,
+ <&cru ACLK_EMMC>, <&cru BCLK_EMMC>,
+ <&cru TMCLK_EMMC>;
+ clock-names = "core", "bus", "axi", "block", "timer";
+ resets = <&cru SRST_C_EMMC>, <&cru SRST_H_EMMC>,
+ <&cru SRST_A_EMMC>, <&cru SRST_B_EMMC>,
+ <&cru SRST_T_EMMC>;
+ reset-names = "core", "bus", "axi", "block", "timer";
+ max-frequency = <200000000>;
+ status = "disabled";
+ };
+
+ gic: interrupt-controller@fe600000 {
+ compatible = "arm,gic-v3";
+ reg = <0x0 0xfe600000 0 0x10000>, /* GICD */
+ <0x0 0xfe680000 0 0x100000>; /* GICR */
+ interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-controller;
+ #interrupt-cells = <3>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ its: interrupt-controller@fe640000 {
+ compatible = "arm,gic-v3-its";
+ msi-controller;
+ #msi-cells = <1>;
+ reg = <0x0 0xfe640000 0x0 0x20000>;
+ };
+ };

Does the ITS (and other bits related to GIC memory accesses) actually work, or will we have more of the same issues as RK356x?

Thanks,
Robin.