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 <[email protected]> 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/[email protected]?part=5
