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

[Detlev Casanova <detlev.casanova@collabora.com>]

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:
> > 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>
> > ---
> 
> [..]
> 
> > 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:
> > +
> > +	uart1: serial@27310000 {
> > +		compatible = "rockchip,rk3576-uart", "snps,dw-apb-
uart";
> > +		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 ?

> > +		clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>;
> > +		clock-names = "baudclk", "apb_pclk";
> > 
> > +		reg-shift = <2>;
> > +		reg-io-width = <4>;
> 
> Move below "reg".
> 
> > +		dmas = <&dmac0 8>, <&dmac0 9>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&uart1m0_xfer>;
> > +		status = "disabled";
> > +	};
> > +
> > +	pmu: power-management@27380000 {

[...]

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

[...]

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

> > +			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 ??
> 
> > +			reg = <0x0 0x27320000 0x0 0x200>;
> > +			interrupts = <GIC_SPI 153 
IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&cru PCLK_GPIO0>, <&cru 
DBCLK_GPIO0>;
> > +
> > +			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