Re: [RFC PATCH v4 5/6] arm64: dts: rockchip: rk3576: add NPU (RKNN) nodes

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] The newly added NPU core nodes violate the `rockchip,rk3588-rknn-core` binding schema across multiple properties.
- [Low] The newly added IOMMU nodes violate the `rockchip,iommu` binding schema by declaring excessive clocks and missing required properties.
- [Low] Adding a `resets` property to the NPU power domains violates the generic Rockchip power controller schema.
--

--- Patch [5]: [RFC PATCH v4 5/6] arm64: dts: rockchip: rk3576: add NPU (RKNN) nodes ---
commit aae8c26737d67b11f41b924b5322997c922df364
Author: Jiaxing Hu <[email protected]>

arm64: dts: rockchip: rk3576: add NPU (RKNN) nodes

Add the two RKNN cores and their IOMMUs, plus the NPU power-domain
resets the pmdomain driver now cycles on power-on. Both cores are
disabled by default; boards enable what they wire up.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3576.dtsi b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
> --- a/arch/arm64/boot/dts/rockchip/rk3576.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
> @@ -1070,14 +1070,22 @@ power-domain@RK3576_PD_NPUTOP {
>  						power-domain@RK3576_PD_NPU0 {
>  							reg = <RK3576_PD_NPU0>;
>  							clocks = <&cru HCLK_RKNN_ROOT>,
> -								 <&cru ACLK_RKNN0>;
> +								 <&cru ACLK_RKNN0>,
> +								 <&cru CLK_RKNN_DSU0>,
> +								 <&cru ACLK_RKNN_CBUF>,
> +								 <&cru HCLK_RKNN_CBUF>;
> +							resets = <&cru SRST_A_RKNN0_BIU>;

[Severity: Low]
Does adding the resets property here violate the generic Rockchip power
controller schema?

The rockchip,power-controller.yaml binding restricts properties for pd-nodes
and it doesn't appear to list resets as an allowable property, which could
cause a dtbs_check failure.

>  							pm_qos = <&qos_npu_m0>;
>  							#power-domain-cells = <0>;
>  						};
>  						power-domain@RK3576_PD_NPU1 {
>  							reg = <RK3576_PD_NPU1>;
>  							clocks = <&cru HCLK_RKNN_ROOT>,
> -								 <&cru ACLK_RKNN1>;
> +								 <&cru ACLK_RKNN1>,
> +								 <&cru CLK_RKNN_DSU0>,
> +								 <&cru ACLK_RKNN_CBUF>,
> +								 <&cru HCLK_RKNN_CBUF>;
> +							resets = <&cru SRST_A_RKNN1_BIU>;

[Severity: Low]
This shares the same resets property schema issue mentioned for RK3576_PD_NPU0
above.

[ ... ]

> +		rknn_core_0: npu@27700000 {
> +			compatible = "rockchip,rk3576-rknn-core";
> +			reg = <0x0 0x27700000 0x0 0x1000>,
> +			      <0x0 0x27701000 0x0 0x1000>,
> +			      <0x0 0x27703000 0x0 0x1000>;
> +			reg-names = "pc", "cna", "core";
> +			interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cru ACLK_RKNN0>, <&cru HCLK_RKNN_ROOT>,
> +				 <&cru CLK_RKNN_DSU0>, <&cru PCLK_NPUTOP_ROOT>,
> +				 <&cru ACLK_RKNN_CBUF>, <&cru HCLK_RKNN_CBUF>;
> +			clock-names = "aclk", "hclk", "npu", "pclk",
> +				      "aclk_cbuf", "hclk_cbuf";
> +			resets = <&cru SRST_A_RKNN0>;
> +			reset-names = "srst_a";
> +			power-domains = <&power RK3576_PD_NPU0>, <&power RK3576_PD_NPU1>;

[Severity: Low]
Do the power-domains, clocks, and missing npu-supply properties violate the
rockchip,rk3588-rknn-core binding schema? 

The current schema seems to require exactly 1 power-domain, up to 4 clocks,
and explicitly requires an npu-supply.

> +			iommus = <&rknn_mmu_0>;
> +			status = "disabled";
> +		};
> +
> +		rknn_mmu_0: iommu@27702000 {
> +			compatible = "rockchip,rk3576-iommu", "rockchip,rk3568-iommu";
> +			reg = <0x0 0x27702000 0x0 0x100>,
> +			      <0x0 0x27702100 0x0 0x100>;
> +			interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cru ACLK_RKNN0>, <&cru HCLK_RKNN_ROOT>,
> +				 <&cru CLK_RKNN_DSU0>, <&cru ACLK_RKNN_CBUF>,
> +				 <&cru HCLK_RKNN_CBUF>;

[Severity: Low]
Does this node violate the rockchip,iommu binding schema by declaring 5 clocks
without a clock-names mapping? 

The rockchip,iommu.yaml schema restricts clocks to exactly 2 items and
requires the clock-names property to be defined.

[ ... ]

> +		rknn_core_1: npu@27708000 {
> +			compatible = "rockchip,rk3576-rknn-core";
[ ... ]
> +			power-domains = <&power RK3576_PD_NPU1>, <&power RK3576_PD_NPU0>;

[Severity: Low]
This shares the same rockchip,rk3588-rknn-core schema violation mentioned for
rknn_core_0.

> +			iommus = <&rknn_mmu_1>;
> +			status = "disabled";
> +		};
> +
> +		rknn_mmu_1: iommu@2770a000 {
> +			compatible = "rockchip,rk3576-iommu", "rockchip,rk3568-iommu";
[ ... ]
> +			clocks = <&cru ACLK_RKNN1>, <&cru HCLK_RKNN_ROOT>,
> +				 <&cru CLK_RKNN_DSU0>, <&cru ACLK_RKNN_CBUF>,
> +				 <&cru HCLK_RKNN_CBUF>;

[Severity: Low]
This shares the same rockchip,iommu schema violation mentioned for rknn_mmu_0
regarding extra clocks and missing clock-names.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5