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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The `rknn_core_1` DT node is missing the CBUF clocks (`aclk_cbuf` and `hclk_cbuf`), causing a fatal probe failure.
- [High] Providing only a single power domain per core triggers the driver-core auto-attach, conflicting with the driver's explicit attach logic and causing a fatal `-EEXIST` error during probe.
- [High] The `rknn_mmu_1` node has an incorrect `reg` base address (`0x2770a000` instead of `0x27712000`), which maps the wrong physical memory for the NPU1 IOMMU.
- [High] Race condition between the IRQ handler and poll_work causes premature task completion and memory corruption.
- [High] poll_work is not cancelled during a job reset, leaving a stale work item that corrupts subsequent jobs.
- [Low] The `rknn_core_0` and `rknn_core_1` nodes violate the `rockchip,rk3588-rknn-core` binding schema constraints.
- [Low] `rknn_mmu_0` and `rknn_mmu_1` nodes violate the `rockchip,iommu` binding schema.
--

--- Patch [5]: [RFC PATCH v3 5/6] arm64: dts: rockchip: rk3576: add NPU (RKNN) nodes ---
commit 844bc539be73e9227d1af3ee91d59e16d7988bfd
Author: Jiaxing Hu <gahing@gahingwoo.com>

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

This commit adds the two RKNN cores and their IOMMUs, alongside the NPU
power-domain resets. Both cores are disabled by default and can be enabled
by boards as needed.

> --- a/arch/arm64/boot/dts/rockchip/rk3576.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
[ ... ]
> @@ -1832,6 +1840,72 @@ qos_npu_m1ro: qos@27f22100 {
>  			reg = <0x0 0x27f22100 0x0 0x20>;
>  		};
>  
> +		rknn_core_0: npu@27700000 {
> +			compatible = "rockchip,rk3576-rknn-core";
> +			reg = <0x0 0x27700000 0x0 0x1000>,
> +			      <0x0 0x27701000 0x0 0x1000>,
> +			      <0x0 0x27703000 0x0 0x1000>,
> +			      <0x0 0x27704000 0x0 0x1000>,
> +			      <0x0 0x27705000 0x0 0x1000>;
> +			reg-names = "pc", "cna", "core", "dpu", "dpu_rdma";

[Severity: Low]
Does this exceed the maxItems allowed by the schema?

The schema for rockchip,rk3588-rknn-core enforces maxItems: 3 for the reg
property and maxItems: 4 for clocks. It also requires npu-supply
unconditionally and demands two resets (srst_a and srst_h), which appear to
be omitted here.

Could this trigger unconditional dtbs_check validation failures?

> +			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>;

[Severity: High]
Will specifying only a single power domain here conflict with the driver's
explicit attach logic?

Looking at rocket_core_init() in drivers/accel/rocket/rocket_core.c, the
driver sets multi_power_domain = true and explicitly calls
devm_pm_domain_attach_list().

Because only a single power-domains entry is specified, the driver-core will
automatically attach the domain during platform_probe(), populating
dev->pm_domain before the driver's probe runs.

This would cause devm_pm_domain_attach_list() to return -EEXIST, aborting
the probe for both NPU cores.

> +			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 violate the rockchip,iommu binding schema?

The rockchip,iommu.yaml schema requires exactly two clocks (aclk and iface)
and strictly mandates the clock-names property.

Here, 5 unnamed clocks are defined and the clock-names property is omitted,
which might cause dtbs_check validation failures.

> +			#iommu-cells = <0>;
> +			power-domains = <&power RK3576_PD_NPU0>;
> +			status = "disabled";
> +		};
> +
> +		rknn_core_1: npu@27710000 {
[ ... ]
> +			interrupts = <GIC_SPI 248 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cru ACLK_RKNN1>, <&cru HCLK_RKNN_ROOT>,
> +				 <&cru CLK_RKNN_DSU0>, <&cru PCLK_NPUTOP_ROOT>;
> +			clock-names = "aclk", "hclk", "npu", "pclk";

[Severity: High]
Are the CBUF clocks (aclk_cbuf and hclk_cbuf) missing from this node?

The rocket driver explicitly requests 6 clocks via devm_clk_bulk_get() (as
defined by .num_clks = 6 in rk3576_soc_data).

Since devm_clk_bulk_get() is not optional, it will fail with -ENOENT when it
cannot find the aclk_cbuf and hclk_cbuf clocks for NPU1, preventing the
device from probing.

> +			resets = <&cru SRST_A_RKNN1>;
> +			reset-names = "srst_a";
> +			power-domains = <&power RK3576_PD_NPU1>;
> +			iommus = <&rknn_mmu_1>;
> +			status = "disabled";
> +		};
> +
> +		rknn_mmu_1: iommu@2770a000 {
> +			compatible = "rockchip,rk3576-iommu", "rockchip,rk3568-iommu";
> +			reg = <0x0 0x2770a000 0x0 0x100>,
> +			      <0x0 0x2770a100 0x0 0x100>;

[Severity: High]
Is this reg base address correct for the NPU1 IOMMU?

NPU1 starts at 0x27710000, so the corresponding IOMMU hole is typically at
offset +0x2000 (0x27712000).

Mapping this to 0x2770a000 places it inside the 0x2770xxxx memory space
belonging to NPU0. When the IOMMU driver interacts with this wrong base
address, it might configure incorrect hardware registers or trigger faults.


Additionally, looking at the related driver code in
drivers/accel/rocket/rocket_job.c:

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
	rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
	rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);

	scoped_guard(mutex, &core->job_lock)
		if (core->in_flight_job) {
			if (core->in_flight_job->next_task_idx < core->in_flight_job->task_count) {
				rocket_job_hw_submit(core, core->in_flight_job);
				return;
			}
...

[Severity: High]
Can a race condition occur here between the IRQ handler and poll_work?

rocket_job_handle_irq() can execute concurrently from both
rocket_job_irq_handler_thread and rocket_poll_work_fn. Because there is no
mutual exclusion protecting the initial register writes (like
OPERATION_ENABLE), one thread could complete a task, submit the next task to
the hardware via rocket_job_hw_submit(), and release the lock.

Another thread might then immediately acquire the lock, see
core->in_flight_job with next_task_idx advanced, and erroneously signal the
newly running task as complete.

Does this risk detaching the IOMMU while the NPU is actively executing and
writing to memory?


Also in drivers/accel/rocket/rocket_job.c:

drivers/accel/rocket/rocket_job.c:rocket_reset() {
static void
rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
{
	if (!atomic_read(&core->reset.pending))
		return;

	drm_sched_stop(&core->sched, bad);

	scoped_guard(mutex, &core->job_lock) {
...

[Severity: High]
Is poll_work properly cancelled during a job reset here?

When an NPU job times out, rocket_reset() resets the hardware and restarts
the DRM scheduler, but it appears to miss cancelling core->poll_timer and
flushing core->poll_work.

If a timeout occurs just as the polling timer fires, the unhandled poll_work
could persist. When the next valid job is submitted by the restarted
scheduler, the stale work item might execute, improperly cancel the new
job's timer, stop the hardware, and erroneously signal completion for the
brand-new job.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731043507.1832277-1-gahing@gahingwoo.com?part=5