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@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/
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@xxxxxxxxx/
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.
