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

Dragan Simic
2024-08-17T03:11:34Z

Hello Detlev,

Please see a few comments below.

On 2024-08-02 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>
> ---

[snip]

> 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>
> +
> +/ {
> + compatible = "rockchip,rk3576";
> +
> + interrupt-parent = <&gic>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + aliases {
> + ethernet0 = &gmac0;
> + ethernet1 = &gmac1;

Please remove ethernetX aliases from the SoC dtsi. The consensus
is that those aliases need to be defined at the board level instead.

See the commit 5d90cb1edcf7 (arm64: dts: rockchip: Remove ethernet0
alias from the SoC dtsi for RK3399, 2023-12-12), for example, for
more details.

> + 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;
> + };
> +
> + xin32k: clock-32k {

Please use "xin32k: clock-xin32k { ... }" instead, because that follows
the recently established revised pattern for clock names. We should
have
come consistency in the new SoC dtsi additions.

> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <32768>;
> + clock-output-names = "xin32k";
> + };
> +
> + xin24m: clock-24m {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <24000000>;
> + clock-output-names = "xin24m";
> + };

Please use "xin24m: clock-xin24m { ... }" instead, for the same reasons
as already described above.

> + spll: clock-702m {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <702000000>;
> + clock-output-names = "spll";
> + };

Perhaps using "spll: clock-spll { ... }" instead would also be a good
idea, because it would improve the overall consistency.