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

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

Hi Johan,

On Tuesday, 20 August 2024 09:34:58 EDT Johan Jonker wrote:
> On 8/19/24 22:06, Detlev Casanova wrote:
> > 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 ?
> 
> 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/

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

> >>> +		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.
> 
> Could the people from Rockchip disclose the USB PD location?
> 
> > [...]
> > 
> >>> +
> >>> +	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.