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

Hi Johan,

On Thursday, 15 August 2024 05:30:25 EDT Johan Jonker wrote:

Some comments below. Whenever useful.

On 8/2/24 23:45, Detlev Casanova wrote:

quoted

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 <redacted>
Signed-off-by: Finley Xiao <redacted>
Signed-off-by: Yifeng Zhao <redacted>
Signed-off-by: Elaine Zhang <redacted>
[rebase, squash and reword commit message]
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---

[..]

quoted

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

[..]

For uart0..uart11:

quoted

+
+	uart1: serial@27310000 {
+		compatible = "rockchip,rk3576-uart", "snps,dw-apb-

uart";

quoted

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

Ok, so it should be sorted alphabetically with the following exceptions:
- 'compatible' and 'reg.*' on top
- "#.*" at the end, sorted
- "status" last.

Is that right ?

quoted

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

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

Move below "reg".

quoted

+		dmas = <&dmac0 8>, <&dmac0 9>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&uart1m0_xfer>;
+		status = "disabled";
+	};
+
+	pmu: power-management@27380000 {

[...]

quoted

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

The TRM doesn't tell me anything, but If I don't put it as a child of VOP, it 
just hangs when the kernel tries to shut it down.

[...]

quoted

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

I tried putting the gpio nodes out of the pinctrl node, they should work 
because they already have a gpio-ranges field.
But unfortunately, that seem to break the pinctrl driver which hangs at some 
point. Maybe some adaptations are needed to support this, or am I missing 
something ?

quoted

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

quoted

+			reg = <0x0 0x27320000 0x0 0x200>;
+			interrupts = <GIC_SPI 153 

IRQ_TYPE_LEVEL_HIGH>;

quoted

+			clocks = <&cru PCLK_GPIO0>, <&cru 

DBCLK_GPIO0>;

quoted

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

Regards,

Detlev