2022-04-22 22:29:13

by Robin Murphy

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

On 2022-04-22 18:09, Sebastian Reichel wrote:
> From: Kever Yang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Elaine Zhang <[email protected]>
> Signed-off-by: Sugar Zhang <[email protected]>
> Signed-off-by: Kever Yang <[email protected]>
> [rebase, squash and reword commit message]
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> 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.


2022-04-26 01:55:33

by Sebastian Reichel

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

Hi,

Thanks for having a look.

On Fri, Apr 22, 2022 at 07:16:13PM +0100, Robin Murphy wrote:
> On 2022-04-22 18:09, Sebastian Reichel wrote:
> > ...
> > + 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?

Yes, see below.

> > + 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?

The GIC in RK3588 is has the same shareability limitation as the RK356x,
but fixed the 32bit limitation. That's why I just added the boot cpu core
for now; adding any other cpu core breaks the boot without the downstream
shareability patch and I'm still investigating.

-- Sebastian


Attachments:
(No filename) (1.41 kB)
signature.asc (849.00 B)
Download all attachments

2022-04-26 03:30:53

by Peter Geis

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

On Mon, Apr 25, 2022 at 2:14 PM Sebastian Reichel
<[email protected]> wrote:
>
> Hi,
>
> Thanks for having a look.
>
> On Fri, Apr 22, 2022 at 07:16:13PM +0100, Robin Murphy wrote:
> > On 2022-04-22 18:09, Sebastian Reichel wrote:
> > > ...
> > > + 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?
>
> Yes, see below.
>
> > > + 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?
>
> The GIC in RK3588 is has the same shareability limitation as the RK356x,
> but fixed the 32bit limitation. That's why I just added the boot cpu core
> for now; adding any other cpu core breaks the boot without the downstream
> shareability patch and I'm still investigating.

There's no way to avoid this issue unfortunately.
See my awful hacked together patch:
https://gitlab.com/pine64-org/quartz-bsp/linux-next/-/commit/8b34fd2a74321f8f5d7731b63eee0f9e03d1393b

Considering the ITS exists pretty much just for MSIs, and my PCIe
series introduces support for legacy interrupts, you may get away with
doing the mbi-alias currently implemented in rk356x.
Note, there are *some* compatibility issues with mbi-alias MSIs,
particularly with high IRQ cards like the Intel x520.

>
> -- Sebastian
> _______________________________________________
> Linux-rockchip mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip