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

["Heiko Stübner" <heiko@sntech.de>]

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