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

On 8/19/24 22:06, Detlev Casanova wrote:

Hi Johan,

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

quoted

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

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 ?

The dts-coding-style.rst does not say much about things with "#",
so below a property they refer to or at the end looks nicer.
No strict rule, but do it in a consistent style in file. 

Original comment by robh for things with "reg":
"It makes more sense to keep reg-io-width together with reg."
https://lore.kernel.org/all/20240131135955.GA966672-robh@kernel.org/ (local)

quoted

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

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.

Could the people from Rockchip disclose the USB PD location?

[...]

quoted

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 ?

The aliases that we added to the DT files are a work around to prevent damage when we moved to generic gpio node names.
There just happened to be some code for it in the driver...
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpio/gpio-rockchip.c#n719

Comment by robh:
"GPIO shouldn't really have an alias either IMO."
https://lore.kernel.org/linux-arm-kernel/20230118153236.GA33699-robh@kernel.org/ (local)

Mainline Rockchip gpio driver is not so to the standard.
The file gpio-rockchip.c currently does nothing with "gpio-ranges" vs. bank and node relation.
My simple patch was acked, but never applied. There's no public maintainer response of what to improve.
Guess, probably something more complicated idiot prove "gpio-ranges" parsing/bank linking is needed?
https://lore.kernel.org/linux-arm-kernel/890be9a0-8e82-a8f4-bc15-d5d1597343c2@gmail.com/ (local)

I leave this subject up to the experts to find out what is needed to improve.
Don't ask me.

Johan

quoted

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

quoted

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

DBCLK_GPIO0>;

quoted

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