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@xxxxxxxxxxxxxx>
>>> Signed-off-by: Finley Xiao <finley.xiao@xxxxxxxxxxxxxx>
>>> Signed-off-by: Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx>
>>> Signed-off-by: Elaine Zhang <zhangqing@xxxxxxxxxxxxxx>
>>> [rebase, squash and reword commit message]
>>> Signed-off-by: Detlev Casanova <detlev.casanova@xxxxxxxxxxxxx>
>>> ---
>>
>> [..]
>>
>>> 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@xxxxxxxxxx/
>
>>> + 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@xxxxxxxxxx/
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@xxxxxxxxx/
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
>
>
>
