Re: [RFC PATCH v4 7/9] arm64: dts: rockchip: rk356x: Add the NPU and its IOMMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect IOMMU compatible string 'rockchip,iommu' is used for the RK3568 NPU IOMMU, which will cause translation faults.
- [High] The assigned clock rates for the NPU AXI bus and compute core are swapped, severely overclocking the AXI bus.
--

commit a304190b5b1e8ffa602c7c5d7dbdfed3a2c3dcae
Author: Midgy BALON <midgy971@gmail.com>
Subject: arm64: dts: rockchip: rk356x: Add the NPU and its IOMMU

This patch adds the device tree nodes for the NVDLA-derived NPU and its
associated IOMMU on the RK3568 SoC. It configures the clocks, resets, power
domains, and assigns the PMU syscon to manage the NPU NoC bus-idle. The
configuration also intends to bring up the NPU AXI bus clock at 200 MHz.

> diff --git a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> index 64bdd8b7754b5..313db59c1aed8 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
[ ... ]
> @@ -572,6 +579,37 @@ power-domain@RK3568_PD_RKVENC {
>  		};
>  	};
>  
> +	rknn_core_0: npu@fde40000 {
> +		compatible = "rockchip,rk3568-rknn-core";
> +		reg = <0x0 0xfde40000 0x0 0x1000>,
> +		      <0x0 0xfde41000 0x0 0x1000>,
> +		      <0x0 0xfde43000 0x0 0x1000>;
> +		reg-names = "pc", "cna", "core";
> +		interrupts = <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru ACLK_NPU>, <&cru HCLK_NPU>,
> +			 <&scmi_clk SCMI_CLK_NPU>, <&cru PCLK_NPU_PRE>;
> +		clock-names = "aclk", "hclk", "npu", "pclk";
> +		assigned-clocks = <&scmi_clk SCMI_CLK_NPU>, <&cru CLK_NPU>;
> +		assigned-clock-rates = <200000000>, <600000000>;

[Severity: High]
Will this configuration inadvertently overclock the AXI bus?

The commit message states the intent to assign the CRU CLK_NPU so the NPU
AXI bus clock comes up at 200 MHz. However, based on the positional mapping
of assigned-clocks to assigned-clock-rates, this assigns 200 MHz to the SCMI
compute core and 600 MHz to CLK_NPU.

Should the values in assigned-clock-rates be swapped to match the order
in assigned-clocks to prevent bus hangs?

> +		resets = <&cru SRST_A_NPU>, <&cru SRST_H_NPU>;
> +		reset-names = "srst_a", "srst_h";
> +		power-domains = <&power RK3568_PD_NPU>;
> +		rockchip,pmu = <&pmu>;
> +		iommus = <&rknn_mmu_0>;
> +		status = "disabled";
> +	};
> +
> +	rknn_mmu_0: iommu@fde4b000 {
> +		compatible = "rockchip,iommu";

[Severity: High]
Does this compatible string correctly identify the v2 IOMMU hardware
present on the RK3568?

Using rockchip,iommu binds to the v1 hardware operations in the Rockchip
IOMMU driver. Because the RK3568 features a v2 IOMMU, configuring it with
v1 driver operations could result in incorrect page table programming and
translation faults when the NPU attempts DMA.

Other IOMMU nodes in rk356x-base.dtsi use rockchip,rk3568-iommu. Should
this node use that compatible string instead?

> +		reg = <0x0 0xfde4b000 0x0 0x40>;
> +		interrupts = <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>;
> +		clock-names = "aclk", "iface";

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613070116.438906-1-midgy971@gmail.com?part=7