Re: [PATCH v7 4/5] arm64: dts: rockchip: add core dtsi for RK3562 SoC

Heiko Stübner
2025-05-11T10:14:53Z

Hi,

Am Sonntag, 11. Mai 2025, 12:00:22 Mitteleuropäische Sommerzeit schrieb Chukun Pan:
> > <snip>
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3562.dtsi
> > @@ -0,0 +1,1187 @@
> > <snip>
> > +#include <dt-bindings/phy/phy.h>
> > +#include <dt-bindings/power/rockchip,rk3562-power.h>
> > +#include <dt-bindings/pinctrl/rockchip.h>
>
> Does this need to be sorted alphabetically?

the rk3562-power header is gone temporarily anyway (in another tree),
so I moved things to numbers temporarily and created
https://lore.kernel.org/r/20250510161531.2086706-1-heiko@sntech.de
to be applied after the merge-window.

>
> > <snip>
> > + idle-states {
> > + entry-method = "psci";
>
> It would be better to leave a blank line here.

added

> > + CPU_SLEEP: cpu-sleep {
>
> > <snip>
> > + pwm0: pwm@ff230000 {
> > + compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > + reg = <0x0 0xff230000 0x0 0x10>;
> > + #pwm-cells = <3>;
> > + pinctrl-names = "active";
>
> It should be `pinctrl-names = "default";` for pwm, see also [1]

fixed the pinctrl

> > + pinctrl-0 = <&pwm0m0_pins>;
> > + clocks = <&cru CLK_PMU1_PWM0>, <&cru PCLK_PMU1_PWM0>;
> > + clock-names = "pwm", "pclk";
> > + status = "disabled";
> > + };
>
> The pinctrl should be placed below the clock.

fixed ordering

>
> > <snip>
> > + power: power-controller {
> > + compatible = "rockchip,rk3562-power-controller";
> > + #power-domain-cells = <1>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + status = "okay";
> > +
>
> `status = "okay"` is not needed here.
> Also remove extra blank lines.

dropped okay ... extra blank was already removing when applying

>
> > +
> > + power-domain@RK3562_PD_GPU {
> > <snip>
> > + power-domain@RK3562_PD_VI {
> > + reg = <RK3562_PD_VI>;
> > + #power-domain-cells = <1>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + pm_qos = <&qos_isp>,
> > + <&qos_vicap>;
> > +
> > + power-domain@RK3562_PD_VEPU {
> > + reg = <RK3562_PD_VEPU>;
> > + pm_qos = <&qos_vepu>;
> > + #power-domain-cells = <0>;
>
> This line is missing a tab, resulting in an indentation error.

fixed the indentation

> > + };
> > + };
> > <snip>
>
> > + pcie2x1: pcie@ff500000 {
> > + compatible = "rockchip,rk3562-pcie", "rockchip,rk3568-pcie";
> > + #address-cells = <3>;
> > + #size-cells = <2>;
>
> #address-cells/#size-cells should be placed above `status = "disabled";`
> I think other nodes also need to change this. (Some for #pwm-cells)

moved them (and the #pwm-cells above)

> > + interrupt-names = "sys", "pmc", "msg", "legacy", "err", "msi";
>
> I noticed that the bsp 5.10 kernel said that pcie only has 8 MSI vectors,
> [2][3] but in the bsp 6.1 kernel it changed to 32 MSI vectors [4].
>
> The rockchip documentation also says there are only 8 MSI vectors:
>
> [5] Page37 8.8 "RK3528/RK3562/RK3576可分配的MSI或者MSI-X总数是8个"
> Translate into English: "The total number of MSI or MSI-X that
> can be allocated by RK3528/RK3562/RK3576 is 8"
>
> We noticed this when supporting rk3528, so which one is correct?

I'll leave that for a fixup patch or further discussion ;-)

> > + phys = <&combphy_pu PHY_TYPE_PCIE>;
>
> s/combphy_pu/combphy
>
> > <snip>
> + combphy_pu: phy@ff750000 {
>
> Please change it to `combphy` like rk3568.

changed the naming.

> Heiko I know you just merged this, it's a bit offensive but I think
> it would be better to drop these patches and fix them further.
> Or kever would you like to send a fix patch? Although most of
> them are typo issues, it will take a few patches to fix it.

First of all, thanks for noticing all the bits and pieces to improve.
I di think I have now fixed up all the "regular" pieces you mentioned
and amended the commit accordingly:

https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?id=1d2f65fa98ddcafdfd1ebcdb87105141861b584a

Heiko