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

[Johan Jonker <jbx6244@yandex.com>]

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/

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

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/

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

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