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

Hi Johan,

On Tuesday, 20 August 2024 09:34:58 EDT Johan Jonker wrote:

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

quoted

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)

Ok, I'll fix the consistency in the file then, thanks for the clarification.

quoted

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

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/dri
vers/gpio/gpio-rockchip.c#n719

Just above that, it tries to get the pinctrl_dev from the parent node. So the 
rockchip,gpio-bank nodes have to be children of the pinctrl node or the gpio 
driver will just return -EPROBE_DEFER. 

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

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-d5d1597343
c2@gmail.com/

Indeed, the driver could use the gpio-ranges to determine the node's bank 
number. Currently it uses the aliases or falls back to the order of parsing 
nodes.
 

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

My opinion on this is that the rockchip driver needs to be adapted if we want 
the gpio nodes out of the pinctrl. I'm willing to have a look at this, but I 
don't think it is in the scope for this patch set and don't think it should be 
a blocker.

Johan

Regards,
Detlev.