[PATCH v12 00/14] accel/rocket: RK3576 NPU (RKNN) enablement

[PATCH v12 00/14] accel/rocket: RK3576 NPU (RKNN) enablement

[Jiaxing Hu]

This adds the RK3576 NPU to accel/rocket, which today supports RK3588
only. The RK3576 carries two cores of the same RKNN block, wired up
differently. Two extra convolution buffer clocks, two power domains per
core, one reset rather than two, no NPU SRAM rail, and a PC_TASK_CON
that packs the task number into sixteen bits rather than twelve.

1/14 is Igor Paunovic's "[PATCH v2] accel/rocket: request the core clocks
by name", carried in the series since v11 so it applies to a plain
next-20260911 with nothing outside it to follow. It keeps his authorship
and its four tags. Tested on a Radxa ROCK 4D.

Five things changed since v11:

  3/14   clears the raw interrupt status beside the mask. Masking alone
         left the DPU bit latched until rocket_core_reset(), and the
         hardirq decides on raw status alone, so a fault from the IOMMU
         sharing this core's line would wake the threaded handler again
         and "No handler is running now" would stop holding partway
         through the function.

         It still tests "> 0". An earlier draft of this version changed
         that to "!= 0", on the reasoning that -EINVAL means runtime PM
         is not managing the device and the block is therefore powered.
         That is wrong and the change is withdrawn.
         pm_runtime_get_conditional() tests power.disable_depth before
         power.runtime_status, so -EINVAL masks a suspended device
         rather than excluding one, and this driver makes that state
         twice: in pm_runtime_force_suspend(), its own system sleep
         callback, which disables runtime PM before turning the clocks
         off, and in rocket_core_fini(), which suspends then disables
         before cancelling the timeout worker. Either would have put a
         register write on a powered-down block.

  10/14  cycles the resets before the settle delay 9/14 adds, not after.
         PD_NPU0 and PD_NPU1 ask for both, and the old order spent the
         delay and then deasserted a reset with nothing between it and
         rockchip_pmu_restore_qos(). The reset is SRST_A_RKNN0/1_BIU,
         the bus interface those writes go through.

         This is a change to code Abel Vesa reviewed. His Reviewed-by
         is kept because the patch still does what he read, in a
         different order; say so if it should go. 9/14 is untouched.

  11/14  checks the match data at the top of rocket_probe(), before
         anything is allocated. of_device_get_match_data() returns NULL
         for a device bound by name rather than by compatible, and such
         a device has no of_node, so the walk of matching nodes that
         sizes rdev->cores[] never counted it. Storing into that array
         and checking afterwards writes past the end.

  13/14  gives CLK_RKNN_DSU0 a rate. Nothing in mainline sets it and the
         NPU comes up at 786 MHz, while Rockchip's OPP table asks 800 mV
         of its 800 MHz step and nothing sets the rail either. On the
         ROCK 4D at the 750 mV its PMIC boots with, two jobs at once
         make the second core write single words wrong, thirteen to
         twenty rows of a 5400 row pass, where either core alone is
         exact; at 594 MHz, or at 786 with the rail at 800 mV, four such
         passes are clean. 594 MHz is a divider off GPLL and sits
         between that table's 500 and 600 MHz steps, both of which ask
         725 mV, so it is inside the voltage a board that describes no
         NPU rail already provides.

  14/14  enables both cores and both IOMMUs. v11 enabled rknn_core_0
         alone, "left to whoever can test it". It has been tested.

Sashiko raised one thing on 4/14 that an argument does not settle.
pm_runtime_put_autosuspend() is asynchronous with a 50 ms delay, so on a
workload whose submit gap is shorter the domain does not cycle after a
reset, 10/14's power-on pulse never fires, and rocket_core_reset()'s own
resets are all that run.

Measured on the domain rather than on the device: over one 60.8 s decode
of Phi-3.5-mini on a ROCK 4D, genpd's npu domain took 46.8 s active and
14.2 s idle, which sum to the wall clock, and its idle-state usage count
rose by 202. npu0 and npu1 rose by 210 and 217, and current_state reads
off-0 either side. The domain cycles about two hundred times a minute
under a real decode, and the pulse fires.

Whether it cycles after a timed-out job is the half Sashiko is asking
about, and that depends on the gap to the next submit. Inducing one needs
a kernel with JOB_TIMEOUT_MS lowered and a flash, so that half is still
an argument rather than a measurement.

Findings Sashiko labelled pre-existing, for Tomeu. These five recur; it
raised thirteen across the nine mails, and the one not listed worth
naming is a 64-bit DMA address truncated to 32 bits, which faults the
IOMMU above 4 GB.

  - the job completion path takes iommu_group_get(core->dev) and never
    puts it, one group reference a job, flagged on seven of the nine.
    5/14 moves this call rather than adding or removing it, so the leak
    is pre-existing but not untouched;
  - the shared IRQ handler touches registers without checking the PM
    state;
  - runtime suspend has no synchronize_irq();
  - num_cores is used both as an array length and as the probe index;
  - rdev is leaked through devres on probe deferral.

5/14 moves the iommu_detach_group(NULL, iommu_group_get(core->dev)) line
out of the IRQ handler and into rocket_job_next_locked(). ZhaoJinming's
"accel/rocket: Fix iommu_group leak and unsafe IRQ register access"
changes the same line, is marked for stable, and carries the same Fixes
tag as ours. Whichever lands first the other conflicts; say which you
would rather take.

Still open from v9, no reply since. 9/14 does three things: it adds the
settle delay, renames a macro, and gives RK3576_PD_NPU a regulator, which
also makes every RK3576 board force that domain off at probe. I asked
whether it wants splitting; Abel Vesa's Reviewed-by may be the answer,
and I would rather split it than have it merged on my silence. 13/14 also
gives each core both NPU domains, which is the description that has been
tested here rather than the topology; if it should be one domain per
core, 6/14's minItems has to change with it.

The bindings are unchanged since v9, where dt_binding_check was clean on
all three with dtschema 2026.6 and yamllint 1.38.0, and CHECK_DTBS was
clean on all 13 rk3576 and all 48 rk3588 dtbs. 13/14 adds a property to a
dts rather than to a binding, and CHECK_DTBS is clean on all 13 rk3576
dtbs with it.

A correction to an earlier posting. On 11 September I replied to the v10
cover saying that the 102 induced resets and the all-0x80 buffer it
described were not Igor Paunovic's. They are his: he reported both on
25 August in the v9 02/13 thread, and the 45 resets of 19 August and the
102 of 25 August are two different runs of his. I had read only the
19 August thread, and should have searched before saying in public that
he had not measured it. That mail is withdrawn, and both of his
Tested-by lines stand as he sent them.

Link to v11: https://lore.kernel.org/all/20260831081956.84871-1-gahing@gahingwoo.com/

The tags:

  2/14   Tested-by: Igor Paunovic   # RK3588, three cores
  3/14   Tested-by: Igor Paunovic   # RK3588, three cores, induced
                                    # reset, differential base,
                                    # JOB_TIMEOUT_MS=2
  4/14   Tested-by: Igor Paunovic   # RK3588, three cores, induced
                                    # reset, JOB_TIMEOUT_MS=2
  5/14   Reviewed-by: Igor Paunovic
  6/14   Reviewed-by: Krzysztof Kozlowski
  7/14   Acked-by: Conor Dooley
  8/14   Acked-by: Conor Dooley
  9/14   Reviewed-by: Abel Vesa
  10/14  Reviewed-by: Abel Vesa

Igor Paunovic (1):
  accel/rocket: request the core clocks by name

Jiaxing Hu (13):
  accel/rocket: take the completion register writes under job_lock
  accel/rocket: wait for a running IRQ handler before resetting a core
  accel/rocket: let the core suspend after a reset
  accel/rocket: factor the completion tail out of the IRQ handler
  dt-bindings: npu: rockchip: add rockchip,rk3576-rknn-core
  dt-bindings: power: rockchip: allow resets in a power domain node
  dt-bindings: iommu: rockchip: describe the RK3576 NPU MMU
  pmdomain: rockchip: add optional per-domain power-on settle delay
  pmdomain: rockchip: cycle optional power-domain resets on power-on
  accel/rocket: select the per-core clock and reset counts from match
    data
  accel/rocket: add RK3576 NPU (RKNN) support
  arm64: dts: rockchip: add NPU (RKNN) nodes to rk3576
  arm64: dts: rockchip: enable the NPU on rk3576-rock-4d

 .../bindings/iommu/rockchip,iommu.yaml        |  28 ++++
 .../npu/rockchip,rk3588-rknn-core.yaml        |  47 +++++-
 .../power/rockchip,power-controller.yaml      |   8 ++
 .../boot/dts/rockchip/rk3576-rock-4d.dts      |  22 +++
 arch/arm64/boot/dts/rockchip/rk3576.dtsi      |  86 ++++++++++-
 drivers/accel/rocket/rocket_core.c            |  32 ++++-
 drivers/accel/rocket/rocket_core.h            |  11 +-
 drivers/accel/rocket/rocket_device.c          |   7 +-
 drivers/accel/rocket/rocket_drv.c             |  42 +++++-
 drivers/accel/rocket/rocket_drv.h             |   2 +
 drivers/accel/rocket/rocket_job.c             | 135 +++++++++++++++---
 drivers/pmdomain/rockchip/pm-domains.c        |  83 ++++++++---
 12 files changed, 440 insertions(+), 63 deletions(-)


base-commit: 68142f986ff04b2b70b31db00f719bf690f64a9a
-- 
2.43.0

[PATCH v12 01/14] accel/rocket: request the core clocks by name

[Jiaxing Hu]

From: Igor Paunovic <royalnet026@gmail.com>

rocket_core_init() hands core->clks to devm_clk_bulk_get() without ever
setting the .id members. The rocket_core array is allocated with
devm_kcalloc() in rocket_device_init(), and rocket_probe() only fills in
.rdev, .dev and .index, so all four clk_bulk_data entries are requested
with a NULL con_id (unlike core->resets, whose ids are set a few lines
above).

clk_get(dev, NULL) ends up in of_clk_get_hw(np, 0, NULL), and
of_parse_clkspec() only consults "clock-names" when a name was passed, so
the index stays 0 for all four entries. Every entry therefore ends up
holding a handle to the *first* clock of the DT "clocks" property, i.e.
ACLK_NPUn. Nothing fails: probe succeeds and the driver believes it owns
four different clocks.

The consequence is that rocket_device_runtime_resume() prepares and enables
the AXI clock four times, while hclk, pclk and - most importantly - the NPU
compute clock ("npu", SCMI_CLK_NPU on RK3588) are never prepared or enabled
by this driver at all. The NPU still works only because the Rockchip
power-domain driver sets GENPD_FLAG_PM_CLK and its attach_dev() callback
walks the device node with of_clk_get() and adds every clock to the pm_clk
list, so genpd happens to keep the remaining clocks running. The bug is
therefore latent today, but it means the driver holds no reference to the
clock that actually feeds the NPU, which stands in the way of any future
frequency scaling (OPP/devfreq) work.

Found on an Orange Pi 5 Plus (RK3588) by reading the live clock tree:
/sys/kernel/debug/clk/clk_summary shows four "fdab0000.npu" consumer
handles on aclk_npu0 (and likewise on aclk_npu1/aclk_npu2 for the other two
cores), while hclk_npu0, pclk_npu_root and scmi_clk_npu have no
"fdab0000.npu" consumer at all - their only consumers are the
"npu@fdab0000" handles created by the power-domain driver via of_clk_get().

Set the ids explicitly, in the order mandated by the binding
(Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml):
aclk, hclk, npu, pclk. After the change the driver holds one handle per
distinct clock and clk_bulk_prepare_enable() covers all four.

Note that this is a user-visible tightening for out-of-tree DTs: the old
NULL-id requests resolved by index and succeeded no matter what
"clock-names" contained, while the named requests fail probe with -ENOENT
when one of the four names is missing. That is the right outcome for
in-tree users - the binding requires exactly these four clock-names and
rk3588-base.dtsi carries them on all three cores - but a DT that relied on
the permissive lookup goes from silently running on the wrong clock handles
to not probing at all, so record the change here where git log will find
it.

Fixes: ed98261b4168 ("accel/rocket: Add a new driver for Rockchip's NPU")
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Tested-by: Sidong Yang <sidong.yang@furiosa.ai>
Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, NanoPC-T6 Plus
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Reviewed-by: Jiaxing Hu <gahing@gahingwoo.com>
Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
---
 drivers/accel/rocket/rocket_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
index b3b2fa9ba..5dd260bac 100644
--- a/drivers/accel/rocket/rocket_core.c
+++ b/drivers/accel/rocket/rocket_core.c
@@ -28,6 +28,10 @@ int rocket_core_init(struct rocket_core *core)
 	if (err)
 		return dev_err_probe(dev, err, "failed to get resets for core %d\n", core->index);
 
+	core->clks[0].id = "aclk";
+	core->clks[1].id = "hclk";
+	core->clks[2].id = "npu";
+	core->clks[3].id = "pclk";
 	err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks);
 	if (err)
 		return dev_err_probe(dev, err, "failed to get clocks for core %d\n", core->index);
-- 
2.43.0

[PATCH v12 02/14] accel/rocket: take the completion register writes under job_lock

[Jiaxing Hu]

rocket_job_handle_irq() writes OPERATION_ENABLE and INTERRUPT_CLEAR before
taking job_lock, while rocket_job_hw_submit() writes OPERATION_ENABLE from
inside it. The two can therefore race: a completion being handled on one
core can write its zero after a submit on the same core has written its
one, and stop a task that has only just started.

Nothing in tree hits this often, because the interrupt is the only
completion path and it does not overlap its own submit, but the ordering is
wrong on its own terms.

To be exact about what the lock does and does not buy: a mutex gives mutual
exclusion, not ordering, so it does not by itself stop a zero from landing
after a one. What keeps the ordinary path safe is that the handler signals
the job's done fence before the scheduler can issue the next one. The
reason the writes belong inside the guard is that stopping the block and
deciding what to start next have to be one step, which they were not.

Move both writes inside the existing scoped_guard() rather than adding a
second critical section, so stopping the block and deciding what to start
next are one atomic step.

Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Tested-by: Igor Paunovic <royalnet026@gmail.com> # RK3588, three cores
---
 drivers/accel/rocket/rocket_job.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index f40435505..575945015 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -345,10 +345,15 @@ static void rocket_job_handle_irq(struct rocket_core *core)
 {
 	pm_runtime_mark_last_busy(core->dev);
 
-	rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
-	rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
+	scoped_guard(mutex, &core->job_lock) {
+		/*
+		 * Stopping the block belongs under the lock. hw_submit() writes
+		 * OPERATION_ENABLE too, and outside the lock this zero can land
+		 * after that one and stop a task that has only just started.
+		 */
+		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);
@@ -360,6 +365,7 @@ static void rocket_job_handle_irq(struct rocket_core *core)
 			pm_runtime_put_autosuspend(core->dev);
 			core->in_flight_job = NULL;
 		}
+	}
 }
 
 static void
-- 
2.43.0

[PATCH v12 03/14] accel/rocket: wait for a running IRQ handler before resetting a core

[Jiaxing Hu]

rocket_reset() calls drm_sched_stop(), which stops the scheduler and
returns. It does not wait for a threaded handler that is already running,
so the comment that follows, "Remaining interrupts have been handled",
states an assumption rather than something the code arranges.

Call synchronize_irq(core->irq) after drm_sched_stop() and reword the
comment to say what holds afterwards.

It has to go before the scoped_guard(mutex, &core->job_lock) rather than
inside it. rocket_job_handle_irq() takes job_lock, so waiting for the
handler while holding that lock would be waiting for a handler that is
waiting for us. Nothing is held at that point: drm_sched_job_timedout()
drops job_list_lock before calling ->timedout_job(), and the only live
caller, rocket_job_timedout(), runs in process context, so sleeping there
is allowed.

This does not stop a handler that has already read in_flight_job from
finishing its work on the job the reset is about to drop. That window needs
the check and the register writes to be one step under the lock, which is
what the previous patch does; the two are complementary.

Mask the block before the sync as well. INTERRUPT_MASK is armed by
hw_submit() on every submit and cleared only by the hardirq, so on an
ordinary timeout it is still live and a completion can arrive after
synchronize_irq() returns. Nothing is lost by clearing it, since the next
submit arms it again.

That write is the first register access this function has ever made, and it
is guarded, because the function holds no runtime PM reference of its own.
The only reference in the window belongs to in_flight_job, and the
completion path can have put it and cleared the pointer before the timeout
worker arrives: drm_sched_stop() sits in between and can block on
cancel_work_sync() and on a dma_fence_wait(), and it subtracts every
pending job's credits, so rocket_job_is_idle() is true and
rocket_device_runtime_suspend() will not refuse. With the autosuspend delay
elapsed the clocks are off and both NPU domains are down. A register access
in that state takes an async SError on this hardware, which is the failure
two later patches in this series describe from the power-on side.

pm_runtime_get_if_active() resumes nothing and allocates nothing; if the
core is already down there is no live interrupt to mask and the following
synchronize_irq() is all that is needed. Only a POSITIVE answer says the
device is active, and that distinction is not cosmetic: the helper tests
power.disable_depth before power.runtime_status, so -EINVAL masks a
suspended device rather than excluding one. pm_runtime_force_suspend(),
which is this driver's own system sleep callback, disables runtime PM first
and turns the clocks off second; rocket_core_fini() suspends the core and
disables before cancelling the timeout worker. Both offer -EINVAL with the
domain down, which is the SError this patch exists to avoid causing.

The mask is written with a clear of the raw status, paired the way the
completion path writes them. Masking alone leaves the DPU bit latched until
rocket_core_reset(), and the hardirq decides on raw status alone, so a
fault from the IOMMU sharing this core's line would wake the thread again
and the guarantee this patch is about would stop holding partway through
the function. Igor Paunovic asked the general form of this on v8 -- whether
rocket_reset() should hold a reference -- and it was deferred then because
nothing in the path touched a register. This patch is what makes it matter.

The deadlock this placement avoids would not have been reported. The wait
is on desc->wait_for_threads rather than on a lock, so lockdep does not
model it and it would have hung silently.

Igor also ran a differential on RK3588 with this patch and the previous one
removed together, so what it shows bounds the pair rather than either one
of them. On 19 August, 45 induced resets across both arms: no
manifestation, oracle 48/48 throughout. On 25 August, the same protocol on
v9 as posted, one of five runs on the arm without the two patches returned
all 48 output channels at 0x80 from an inference that reported success,
with nothing in dmesg, in lockdep or on the serial console, out of 102
resets across nine runs.

His own bound on it is the right one: one event in 53 differential resets
against zero in 49 with the patches, timing-dependent, and his protocol
cannot tell a genuinely hung block from a lost completion. It bounds; it
does not prove.

Link: https://lore.kernel.org/all/20260819073530.6087-1-royalnet026@gmail.com/
Link: https://lore.kernel.org/all/CAEWPSH5mxTbUkNouxm6yecMZYvDowquhvYvhaXQ8HoMtHD5U1g@mail.gmail.com/

Suggested-by: Igor Paunovic <royalnet026@gmail.com>
Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Tested-by: Igor Paunovic <royalnet026@gmail.com> # RK3588, three cores, induced reset, differential base, JOB_TIMEOUT_MS=2
---
 drivers/accel/rocket/rocket_job.c | 49 +++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index 575945015..0be8db391 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -377,9 +377,52 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
 	drm_sched_stop(&core->sched, bad);
 
 	/*
-	 * Remaining interrupts have been handled, but we might still have
-	 * stuck jobs. Let's make sure the PM counters stay balanced by
-	 * manually calling pm_runtime_put_noidle().
+	 * Mask the block before waiting. hw_submit() arms INTERRUPT_MASK on
+	 * every submit and only the hardirq clears it, so on an ordinary
+	 * timeout it is still live and a completion can arrive after the sync
+	 * returns. The next submit re-arms it, so nothing is lost here.
+	 *
+	 * Only when the device is already awake, though. This function holds no
+	 * runtime PM reference of its own: the only one in the window belongs to
+	 * in_flight_job, and the completion path may have put it and cleared the
+	 * pointer before the timeout worker got here. drm_sched_stop() above can
+	 * block for a long time, and it drops every pending job's credits, so
+	 * rocket_job_is_idle() is true and nothing keeps the core resumed. On
+	 * this hardware a register access with the domain down takes an async
+	 * SError, so a reset must not be the thing that causes one.
+	 *
+	 * Only a positive answer will do. pm_runtime_get_if_active() tests
+	 * power.disable_depth before power.runtime_status, so -EINVAL MASKS a
+	 * suspended device rather than excluding one: pm_runtime_force_suspend(),
+	 * which is this driver's own system suspend callback, disables runtime PM
+	 * first and turns the clocks off second, and rocket_core_fini() suspends
+	 * the core and disables before it cancels the timeout worker. Both leave
+	 * the domain down with -EINVAL on offer.
+	 *
+	 * Clear the raw status along with the mask, the way the completion path
+	 * does. Masking alone leaves the DPU bit latched until
+	 * rocket_core_reset(), and the hardirq decides on raw status alone, so a
+	 * fault from the IOMMU that shares this line would wake the thread again
+	 * and what the comment below asserts would stop being true.
+	 */
+	if (pm_runtime_get_if_active(core->dev) > 0) {
+		rocket_pc_writel(core, INTERRUPT_MASK, 0x0);
+		rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
+		pm_runtime_put_autosuspend(core->dev);
+	}
+
+	/*
+	 * drm_sched_stop() returns without waiting for a threaded handler that
+	 * is already running, so wait for one here. This has to stay outside
+	 * job_lock: the handler takes that lock, so waiting for it while
+	 * holding it would deadlock instead of fencing anything.
+	 */
+	synchronize_irq(core->irq);
+
+	/*
+	 * No handler is running now, but we might still have stuck jobs. Let's
+	 * make sure the PM counters stay balanced by manually calling
+	 * pm_runtime_put_noidle().
 	 */
 	scoped_guard(mutex, &core->job_lock) {
 		if (core->in_flight_job)
-- 
2.43.0

[PATCH v12 04/14] accel/rocket: let the core suspend after a reset

[Jiaxing Hu]

rocket_reset() drops the in-flight job's runtime PM reference with
pm_runtime_put_noidle(), a bare decrement that requests nothing. The core
is left at usage_count 0 but still runtime-active with no idle request
pending, so it does not suspend until something else asks, and on a
platform whose power domain does work on power-on that work never happens.

On RK3576 that work is a bus interface reset the domain cycles when it
comes up. Without it the NPU's IOMMU stops answering, and the job after a
timeout returns a surface of the output zero point with rk_iommu reporting
that MMU_DTE_ADDR is not functioning.

Measured on a ROCK 4D in one boot, three runs, one variable between them.
With the bare put the core reads runtime-active with its rail still up
after the reset, the IOMMU reports the failure on the next attach and the
inference returns 0 of 128 channels. With the reference put back through
pm_runtime_put_autosuspend() the core reads suspended with the rail down,
there is no IOMMU message, and the same inference returns 128 of 128. A
third run repeating the first failed the same way.

It also matches the put in the completion path a few lines away, so the
reset path no longer leaves the device in a state the rest of the driver
never produces. The remaining put, on the error path in rocket_job_run(),
is a plain pm_runtime_put() and is left alone here: it unwinds a
pm_runtime_resume_and_get() that never reached the hardware, and changing
it belongs in its own patch.

Igor Paunovic ran the differential on RK3588: 45 induced resets across
three cores, with and without the two preceding patches, and the domain
dropped every single time with no MMU message on either kernel. So this is
not rocket-wide. His conditions cross a healthy block with a lowered
timeout rather than a hung one, which he was careful to say his protocol
cannot settle, but it is what scopes the change to RK3576.

Link: https://lore.kernel.org/all/20260819073530.6087-1-royalnet026@gmail.com/
Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Tested-by: Igor Paunovic <royalnet026@gmail.com> # RK3588, three cores, induced reset, JOB_TIMEOUT_MS=2
---
 drivers/accel/rocket/rocket_job.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index 0be8db391..b588049aa 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -421,12 +421,12 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
 
 	/*
 	 * No handler is running now, but we might still have stuck jobs. Let's
-	 * make sure the PM counters stay balanced by manually calling
-	 * pm_runtime_put_noidle().
+	 * make sure the PM counters stay balanced by putting the reference the
+	 * job took, and request idle while doing it so the core can suspend.
 	 */
 	scoped_guard(mutex, &core->job_lock) {
 		if (core->in_flight_job)
-			pm_runtime_put_noidle(core->dev);
+			pm_runtime_put_autosuspend(core->dev);
 
 		iommu_detach_group(NULL, core->iommu_group);
 
-- 
2.43.0

[PATCH v12 05/14] accel/rocket: factor the completion tail out of the IRQ handler

[Jiaxing Hu]

rocket_job_handle_irq() stops the block and then either starts the job's
next task or retires the job. The second half is a step of its own and
reads better with a name, now that taking the register writes under
job_lock has moved it a level deeper inside the scoped guard.

Move it to rocket_job_next_locked(). The early return that used to leave
the handler now leaves the helper, which is the same thing here: the scoped
guard drops job_lock either way and nothing follows it.

Doing it as its own patch keeps the locking fix at the head of the series
minimal, so a bisect that stops before this one gets that fix and nothing
else. There is one caller, and no functional change.

Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Reviewed-by: Igor Paunovic <royalnet026@gmail.com>
---
 drivers/accel/rocket/rocket_job.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index b588049aa..8cffe93f6 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -341,6 +341,25 @@ static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job)
 	return ERR_PTR(ret);
 }
 
+/* Start the job's next task, or retire it. Caller holds job_lock. */
+static void rocket_job_next_locked(struct rocket_core *core)
+{
+	lockdep_assert_held(&core->job_lock);
+
+	if (!core->in_flight_job)
+		return;
+
+	if (core->in_flight_job->next_task_idx < core->in_flight_job->task_count) {
+		rocket_job_hw_submit(core, core->in_flight_job);
+		return;
+	}
+
+	iommu_detach_group(NULL, iommu_group_get(core->dev));
+	dma_fence_signal(core->in_flight_job->done_fence);
+	pm_runtime_put_autosuspend(core->dev);
+	core->in_flight_job = NULL;
+}
+
 static void rocket_job_handle_irq(struct rocket_core *core)
 {
 	pm_runtime_mark_last_busy(core->dev);
@@ -354,17 +373,7 @@ static void rocket_job_handle_irq(struct rocket_core *core)
 		rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
 		rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
 
-		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;
-			}
-
-			iommu_detach_group(NULL, iommu_group_get(core->dev));
-			dma_fence_signal(core->in_flight_job->done_fence);
-			pm_runtime_put_autosuspend(core->dev);
-			core->in_flight_job = NULL;
-		}
+		rocket_job_next_locked(core);
 	}
 }
 
-- 
2.43.0

[PATCH v12 06/14] dt-bindings: npu: rockchip: add rockchip,rk3576-rknn-core

[Jiaxing Hu]

The RK3576 NPU has two cores of the same RKNN block the RK3588 binding
already describes, but it wires them up differently: two extra CBUF clocks,
two power domains per core, and a single reset instead of two. It also has
no NPU SRAM supply.

Widen the property ranges to cover both, then pin each SoC back to its own
shape in allOf so nothing loosens for RK3588, and keep sram-supply required
for rockchip,rk3588-rknn-core only.

Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 .../npu/rockchip,rk3588-rknn-core.yaml        | 47 +++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml b/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml
index caca2a490..3b611b64c 100644
--- a/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml
+++ b/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml
@@ -21,6 +21,7 @@ properties:
 
   compatible:
     enum:
+      - rockchip,rk3576-rknn-core
       - rockchip,rk3588-rknn-core
 
   reg:
@@ -33,14 +34,18 @@ properties:
       - const: core # Main NPU core processing unit registers
 
   clocks:
-    maxItems: 4
+    minItems: 4
+    maxItems: 6
 
   clock-names:
+    minItems: 4
     items:
       - const: aclk
       - const: hclk
       - const: npu
       - const: pclk
+      - const: aclk_cbuf
+      - const: hclk_cbuf
 
   interrupts:
     maxItems: 1
@@ -51,12 +56,15 @@ properties:
   npu-supply: true
 
   power-domains:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
 
   resets:
+    minItems: 1
     maxItems: 2
 
   reset-names:
+    minItems: 1
     items:
       - const: srst_a
       - const: srst_h
@@ -75,7 +83,40 @@ required:
   - resets
   - reset-names
   - npu-supply
-  - sram-supply
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: rockchip,rk3588-rknn-core
+    then:
+      properties:
+        clocks:
+          maxItems: 4
+        clock-names:
+          maxItems: 4
+        power-domains:
+          maxItems: 1
+        resets:
+          minItems: 2
+        reset-names:
+          minItems: 2
+      required:
+        - sram-supply
+    else:
+      properties:
+        clocks:
+          minItems: 6
+        clock-names:
+          minItems: 6
+        power-domains:
+          minItems: 2
+        resets:
+          maxItems: 1
+        reset-names:
+          maxItems: 1
+        sram-supply: false
 
 additionalProperties: false
 
-- 
2.43.0

[PATCH v12 07/14] dt-bindings: power: rockchip: allow resets in a power domain node

[Jiaxing Hu]

Some domains do not come up in a usable state on their own and need their
resets cycled once power is on. The RK3576 NPU domains are one
case: without it the first access after power-on takes an async SError.

Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/power/rockchip,power-controller.yaml         | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml b/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml
index b41db576f..83741f048 100644
--- a/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml
+++ b/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml
@@ -136,6 +136,13 @@ $defs:
           A number of phandles to clocks that need to be enabled
           while power domain switches state.
 
+      resets:
+        maxItems: 1
+        description:
+          A phandle to a reset that needs to be cycled once the power domain has
+          been switched on, for domains whose logic does not come up in a usable
+          state by itself.
+
       domain-supply:
         description: domain regulator supply.
 
@@ -216,6 +223,7 @@ examples:
                     reg = <RK3399_PD_IEP>;
                     clocks = <&cru ACLK_IEP>,
                              <&cru HCLK_IEP>;
+                    resets = <&cru SRST_A_IEP>;
                     pm_qos = <&qos_iep>;
                     #power-domain-cells = <0>;
                 };
-- 
2.43.0

[PATCH v12 08/14] dt-bindings: iommu: rockchip: describe the RK3576 NPU MMU

[Jiaxing Hu]

The RK3576 NPU MMUs are rk3568-iommu compatible but take five clocks where
every other Rockchip MMU takes two, the extra three being the compute clock
and the two convolution buffer clocks.

Give them a compatible of their own and pin both sides with an allOf, so
that an rk3568-iommu cannot carry five clocks and an NPU MMU cannot carry
two. Describing the extra clocks as belonging to one SoC without saying so
in the schema, which is what a comment on a description does, leaves both
of those spellings valid.

Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/iommu/rockchip,iommu.yaml        | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml b/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml
index 6ce41d11f..83d7e7c8e 100644
--- a/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml
+++ b/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml
@@ -26,6 +26,7 @@ properties:
       - items:
           - enum:
               - rockchip,rk3576-iommu
+              - rockchip,rk3576-npu-iommu
               - rockchip,rk3588-iommu
           - const: rockchip,rk3568-iommu
 
@@ -42,14 +43,22 @@ properties:
     minItems: 1
 
   clocks:
+    minItems: 2
     items:
       - description: Core clock
       - description: Interface clock
+      - description: Compute clock
+      - description: Convolution buffer core clock
+      - description: Convolution buffer interface clock
 
   clock-names:
+    minItems: 2
     items:
       - const: aclk
       - const: iface
+      - const: npu
+      - const: aclk_cbuf
+      - const: hclk_cbuf
 
   "#iommu-cells":
     const: 0
@@ -72,6 +81,25 @@ required:
   - clock-names
   - "#iommu-cells"
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: rockchip,rk3576-npu-iommu
+    then:
+      properties:
+        clocks:
+          minItems: 5
+        clock-names:
+          minItems: 5
+    else:
+      properties:
+        clocks:
+          maxItems: 2
+        clock-names:
+          maxItems: 2
+
 additionalProperties: false
 
 examples:
-- 
2.43.0

[PATCH v12 09/14] pmdomain: rockchip: add optional per-domain power-on settle delay

[Jiaxing Hu]

The RK3576 NPU domains need a short settle time after the idle request is
released before the registers behind the domain answer. Without it the QoS
writes that rockchip_pmu_restore_qos() issues land while the domain is
still coming up, and the NPU throws an async SError on the first cold
power-on.

Give rockchip_domain_info an optional delay_us and wait for it between
releasing idle and restoring QoS. Rename DOMAIN_M_O_R_G to
DOMAIN_M_O_R_G_W, since the suffixes name the fields the macro sets and
this one now also carries a wakeup delay; RK3576 is its only user, so the
old spelling is not kept around.

While the macro is being rewritten, give it the regulator argument that
DOMAIN_M_O_R and DOMAIN_M_R already take. Without .need_regulator set,
rockchip_pd_regulator_enable() returns early for every RK3576 domain, so a
domain-supply in the device tree is never looked up and never enabled. Add
a DOMAIN_RK3576_R spelling that passes true and use it for RK3576_PD_NPU,
which is the one RK3576 domain with a rail of its own; every other domain
passes false and is unchanged.

Two things beyond the delay, both worth saying out loud because neither is
what the subject describes.

RK3576_PD_NPU also gets need_regulator here, which makes
rockchip_pm_add_one_domain() call rockchip_pd_power(pd, false) at probe on
all thirteen in-tree rk3576 boards. Twelve of them describe no
domain-supply, so the first power-on takes a dummy regulator and a dev_warn
rather than failing. It is deliberate, and it is what the delay is for:
without the domain being forced off at probe, a bootloader that leaves the
NPU powered means ->power_on is never called and neither the delay nor
10/14's reset pulse ever runs.

If that is too much for one patch, say so and it splits.

Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Reviewed-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
 drivers/pmdomain/rockchip/pm-domains.c | 56 ++++++++++++++++----------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index ba66ae719..39988efd8 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -18,6 +18,7 @@
 #include <linux/of_address.h>
 #include <linux/of_clk.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/mfd/syscon.h>
@@ -59,6 +60,7 @@ struct rockchip_domain_info {
 	u32 pwr_offset;
 	u32 mem_offset;
 	u32 req_offset;
+	u32 delay_us;
 };
 
 struct rockchip_pmu_info {
@@ -185,7 +187,7 @@ struct rockchip_pmu {
 	.need_regulator = regulator,			\
 }
 
-#define DOMAIN_M_O_R_G(_name, p_offset, pwr, status, m_offset, m_status, r_status, r_offset, req, idle, ack, g_mask, wakeup)	\
+#define DOMAIN_M_O_R_G_W(_name, p_offset, pwr, status, m_offset, m_status, r_status, r_offset, req, idle, ack, g_mask, delay, wakeup, regulator)	\
 {							\
 	.name = _name,					\
 	.pwr_offset = p_offset,				\
@@ -200,8 +202,10 @@ struct rockchip_pmu {
 	.req_mask = (req),				\
 	.idle_mask = (idle),				\
 	.clk_ungate_mask = (g_mask),			\
+	.delay_us = (delay),				\
 	.ack_mask = (ack),				\
 	.active_wakeup = wakeup,			\
+	.need_regulator = regulator,			\
 }
 
 #define DOMAIN_M_R(_name, pwr, status, req, idle, ack, wakeup, regulator)	\
@@ -258,8 +262,11 @@ struct rockchip_pmu {
 #define DOMAIN_RK3568(name, pwr, req, wakeup, regulator)		\
 	DOMAIN_M_R(name, pwr, pwr, req, req, req, wakeup, regulator)
 
-#define DOMAIN_RK3576(name, p_offset, pwr, status, r_status, r_offset, req, idle, g_mask, wakeup)	\
-	DOMAIN_M_O_R_G(name, p_offset, pwr, status, 0, r_status, r_status, r_offset, req, idle, idle, g_mask, wakeup)
+#define DOMAIN_RK3576(name, p_offset, pwr, status, r_status, r_offset, req, idle, g_mask, delay, wakeup)	\
+	DOMAIN_M_O_R_G_W(name, p_offset, pwr, status, 0, r_status, r_status, r_offset, req, idle, idle, g_mask, delay, wakeup, false)
+
+#define DOMAIN_RK3576_R(name, p_offset, pwr, status, r_status, r_offset, req, idle, g_mask, delay, wakeup)	\
+	DOMAIN_M_O_R_G_W(name, p_offset, pwr, status, 0, r_status, r_status, r_offset, req, idle, idle, g_mask, delay, wakeup, true)
 
 /*
  * Dynamic Memory Controller may need to coordinate with us -- see
@@ -681,6 +688,10 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
 		if (ret < 0)
 			goto out;
 
+		/* Some domains need to settle before the QoS registers answer. */
+		if (pd->info->delay_us)
+			udelay(pd->info->delay_us);
+
 		rockchip_pmu_restore_qos(pd);
 	}
 
@@ -1300,25 +1311,26 @@ static const struct rockchip_domain_info rk3568_pm_domains[] = {
 };
 
 static const struct rockchip_domain_info rk3576_pm_domains[] = {
-	[RK3576_PD_NPU]		= DOMAIN_RK3576("npu",    0x0, BIT(0),  BIT(0), 0,       0x0, 0,       0,       0,       false),
-	[RK3576_PD_NVM]		= DOMAIN_RK3576("nvm",    0x0, BIT(6),  0,      BIT(6),  0x4, BIT(2),  BIT(18), BIT(2),  false),
-	[RK3576_PD_SDGMAC]	= DOMAIN_RK3576("sdgmac", 0x0, BIT(7),  0,      BIT(7),  0x4, BIT(1),  BIT(17), 0x6,     false),
-	[RK3576_PD_AUDIO]	= DOMAIN_RK3576("audio",  0x0, BIT(8),  0,      BIT(8),  0x4, BIT(0),  BIT(16), BIT(0),  false),
-	[RK3576_PD_PHP]		= DOMAIN_RK3576("php",    0x0, BIT(9),  0,      BIT(9),  0x0, BIT(15), BIT(15), BIT(15), false),
-	[RK3576_PD_SUBPHP]	= DOMAIN_RK3576("subphp", 0x0, BIT(10), 0,      BIT(10), 0x0, 0,       0,       0,       false),
-	[RK3576_PD_VOP]		= DOMAIN_RK3576("vop",    0x0, BIT(11), 0,      BIT(11), 0x0, 0x6000,  0x6000,  0x6000,  false),
-	[RK3576_PD_VO1]		= DOMAIN_RK3576("vo1",    0x0, BIT(14), 0,      BIT(14), 0x0, BIT(12), BIT(12), 0x7000,  false),
-	[RK3576_PD_VO0]		= DOMAIN_RK3576("vo0",    0x0, BIT(15), 0,      BIT(15), 0x0, BIT(11), BIT(11), 0x6800,  false),
-	[RK3576_PD_USB]		= DOMAIN_RK3576("usb",    0x4, BIT(0),  0,      BIT(16), 0x0, BIT(10), BIT(10), 0x6400,  true),
-	[RK3576_PD_VI]		= DOMAIN_RK3576("vi",     0x4, BIT(1),  0,      BIT(17), 0x0, BIT(9),  BIT(9),  BIT(9),  false),
-	[RK3576_PD_VEPU0]	= DOMAIN_RK3576("vepu0",  0x4, BIT(2),  0,      BIT(18), 0x0, BIT(7),  BIT(7),  0x280,   false),
-	[RK3576_PD_VEPU1]	= DOMAIN_RK3576("vepu1",  0x4, BIT(3),  0,      BIT(19), 0x0, BIT(8),  BIT(8),  BIT(8),  false),
-	[RK3576_PD_VDEC]	= DOMAIN_RK3576("vdec",   0x4, BIT(4),  0,      BIT(20), 0x0, BIT(6),  BIT(6),  BIT(6),  false),
-	[RK3576_PD_VPU]		= DOMAIN_RK3576("vpu",    0x4, BIT(5),  0,      BIT(21), 0x0, BIT(5),  BIT(5),  BIT(5),  false),
-	[RK3576_PD_NPUTOP]	= DOMAIN_RK3576("nputop", 0x4, BIT(6),  0,      BIT(22), 0x0, 0x18,    0x18,    0x18,    false),
-	[RK3576_PD_NPU0]	= DOMAIN_RK3576("npu0",   0x4, BIT(7),  0,      BIT(23), 0x0, BIT(1),  BIT(1),  0x1a,    false),
-	[RK3576_PD_NPU1]	= DOMAIN_RK3576("npu1",   0x4, BIT(8),  0,      BIT(24), 0x0, BIT(2),  BIT(2),  0x1c,    false),
-	[RK3576_PD_GPU]		= DOMAIN_RK3576("gpu",    0x4, BIT(9),  0,      BIT(25), 0x0, BIT(0),  BIT(0),  BIT(0),  false),
+	/*                                            name    p_offset pwr      status  r_status r_offset req      idle     g_mask   delay wakeup */
+	[RK3576_PD_NPU]		= DOMAIN_RK3576_R("npu",  0x0, BIT(0),  BIT(0), 0,       0x0, 0,       0,       0,       0,    false),
+	[RK3576_PD_NVM]		= DOMAIN_RK3576("nvm",    0x0, BIT(6),  0,      BIT(6),  0x4, BIT(2),  BIT(18), BIT(2),  0,    false),
+	[RK3576_PD_SDGMAC]	= DOMAIN_RK3576("sdgmac", 0x0, BIT(7),  0,      BIT(7),  0x4, BIT(1),  BIT(17), 0x6,     0,    false),
+	[RK3576_PD_AUDIO]	= DOMAIN_RK3576("audio",  0x0, BIT(8),  0,      BIT(8),  0x4, BIT(0),  BIT(16), BIT(0),  0,    false),
+	[RK3576_PD_PHP]		= DOMAIN_RK3576("php",    0x0, BIT(9),  0,      BIT(9),  0x0, BIT(15), BIT(15), BIT(15), 0,    false),
+	[RK3576_PD_SUBPHP]	= DOMAIN_RK3576("subphp", 0x0, BIT(10), 0,      BIT(10), 0x0, 0,       0,       0,       0,    false),
+	[RK3576_PD_VOP]		= DOMAIN_RK3576("vop",    0x0, BIT(11), 0,      BIT(11), 0x0, 0x6000,  0x6000,  0x6000,  0,    false),
+	[RK3576_PD_VO1]		= DOMAIN_RK3576("vo1",    0x0, BIT(14), 0,      BIT(14), 0x0, BIT(12), BIT(12), 0x7000,  0,    false),
+	[RK3576_PD_VO0]		= DOMAIN_RK3576("vo0",    0x0, BIT(15), 0,      BIT(15), 0x0, BIT(11), BIT(11), 0x6800,  0,    false),
+	[RK3576_PD_USB]		= DOMAIN_RK3576("usb",    0x4, BIT(0),  0,      BIT(16), 0x0, BIT(10), BIT(10), 0x6400,  0,    true),
+	[RK3576_PD_VI]		= DOMAIN_RK3576("vi",     0x4, BIT(1),  0,      BIT(17), 0x0, BIT(9),  BIT(9),  BIT(9),  0,    false),
+	[RK3576_PD_VEPU0]	= DOMAIN_RK3576("vepu0",  0x4, BIT(2),  0,      BIT(18), 0x0, BIT(7),  BIT(7),  0x280,   0,    false),
+	[RK3576_PD_VEPU1]	= DOMAIN_RK3576("vepu1",  0x4, BIT(3),  0,      BIT(19), 0x0, BIT(8),  BIT(8),  BIT(8),  0,    false),
+	[RK3576_PD_VDEC]	= DOMAIN_RK3576("vdec",   0x4, BIT(4),  0,      BIT(20), 0x0, BIT(6),  BIT(6),  BIT(6),  0,    false),
+	[RK3576_PD_VPU]		= DOMAIN_RK3576("vpu",    0x4, BIT(5),  0,      BIT(21), 0x0, BIT(5),  BIT(5),  BIT(5),  0,    false),
+	[RK3576_PD_NPUTOP]	= DOMAIN_RK3576("nputop", 0x4, BIT(6),  0,      BIT(22), 0x0, 0x18,    0x18,    0x18,    15,   false),
+	[RK3576_PD_NPU0]	= DOMAIN_RK3576("npu0",   0x4, BIT(7),  0,      BIT(23), 0x0, BIT(1),  BIT(1),  0x1a,    15,   false),
+	[RK3576_PD_NPU1]	= DOMAIN_RK3576("npu1",   0x4, BIT(8),  0,      BIT(24), 0x0, BIT(2),  BIT(2),  0x1c,    15,   false),
+	[RK3576_PD_GPU]		= DOMAIN_RK3576("gpu",    0x4, BIT(9),  0,      BIT(25), 0x0, BIT(0),  BIT(0),  BIT(0),  0,    false),
 };
 
 static const struct rockchip_domain_info rk3588_pm_domains[] = {
-- 
2.43.0

[PATCH v12 10/14] pmdomain: rockchip: cycle optional power-domain resets on power-on

[Jiaxing Hu]

Some Rockchip domains come out of power-on with their bus interface in an
undefined state. On the RK3576 NPU this shows up as a hang on the first
register access after the domain is switched on, and pulsing the domain's
resets at this point clears it.

Take the domain node's resets if it has any, and pulse them between
releasing idle and restoring QoS. The resets are optional, so domains that
do not list any are unaffected.

The cycle goes before the settle delay 9/14 adds, not after it. A domain
that asks for both is asking to settle before the QoS registers answer, and
a reset deasserted after the delay would leave nothing between the deassert
and rockchip_pmu_restore_qos(). On RK3576 PD_NPU0 and PD_NPU1 ask for both,
and the reset they cycle is SRST_A_RKNN0/1_BIU, the bus interface those QoS
writes go through.

It only runs when the domain actually changes state: rockchip_pd_power()
returns early when the hardware already reads the state being asked for. A
bootloader that leaves the NPU powered would therefore skip both this and
the delay, which is why 9/14 gives RK3576_PD_NPU need_regulator and forces
the domain off at probe.

No in-tree DTS puts resets in a power-domain node today, so every other
Rockchip SoC takes the optional get's NULL and is unchanged.

Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Reviewed-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
 drivers/pmdomain/rockchip/pm-domains.c | 27 ++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index 39988efd8..6cc8d6efd 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -19,6 +19,7 @@
 #include <linux/of_clk.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/reset.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/mfd/syscon.h>
@@ -103,6 +104,7 @@ struct rockchip_pm_domain {
 	struct clk_bulk_data *clks;
 	struct device_node *node;
 	struct regulator *supply;
+	struct reset_control *resets;
 };
 
 struct rockchip_pmu {
@@ -688,6 +690,21 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
 		if (ret < 0)
 			goto out;
 
+		/*
+		 * Optional: some domains need their resets cycled once power
+		 * is on. This goes BEFORE the settle delay, not after: a
+		 * domain that asks for both is asking to settle before the
+		 * QoS registers answer, and a reset deasserted after the
+		 * delay would leave nothing between it and the QoS writes.
+		 * On RK3576 the reset being cycled is the NPU core's bus
+		 * interface, which is what those writes go through.
+		 */
+		if (pd->resets) {
+			reset_control_assert(pd->resets);
+			usleep_range(10, 20);
+			reset_control_deassert(pd->resets);
+		}
+
 		/* Some domains need to settle before the QoS registers answer. */
 		if (pd->info->delay_us)
 			udelay(pd->info->delay_us);
@@ -861,6 +878,14 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 	if (error)
 		goto err_put_clocks;
 
+	pd->resets = of_reset_control_array_get_optional_exclusive(node);
+	if (IS_ERR(pd->resets)) {
+		error = dev_err_probe(pmu->dev, PTR_ERR(pd->resets),
+				      "%pOFn: failed to get resets\n", node);
+		pd->resets = NULL;
+		goto err_unprepare_clocks;
+	}
+
 	pd->num_qos = of_count_phandle_with_args(node, "pm_qos",
 						 NULL);
 
@@ -931,6 +956,7 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 	clk_bulk_unprepare(pd->num_clks, pd->clks);
 err_put_clocks:
 	clk_bulk_put(pd->num_clks, pd->clks);
+	reset_control_put(pd->resets);
 	return error;
 }
 
@@ -949,6 +975,7 @@ static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
 
 	clk_bulk_unprepare(pd->num_clks, pd->clks);
 	clk_bulk_put(pd->num_clks, pd->clks);
+	reset_control_put(pd->resets);
 
 	/* protect the zeroing of pm->num_clks */
 	mutex_lock(&pd->pmu->mutex);
-- 
2.43.0

[PATCH v12 11/14] accel/rocket: select the per-core clock and reset counts from match data

[Jiaxing Hu]

The RK3576 carries the same RKNN block with a different set of clocks and
resets, so the counts cannot stay compile-time constants. Add a soc_data
struct to the of_device_id match data and take the bulk counts from it.
RK3588 keeps four clocks and two resets, so nothing changes for it, and the
arrays keep their present sizes: the SoC that needs a longer one grows it
in the patch that adds the names.

rocket_core_reset() is switched over as well. It is the same array, and
leaving it on ARRAY_SIZE() would walk entries that were never acquired once
a SoC asks for fewer.

soc is checked at the top of rocket_probe(), before anything is allocated
and before the per-core fields are stored. of_device_get_match_data()
returns NULL for a device that bound by name rather than by compatible, and
such a device has no of_node, so it was never counted by the walk of
matching nodes that sizes rdev->cores[]. Storing into that array first and
checking afterwards would be writing past the end under the check's own
premise, and returning from there is the one error path in this function
that would skip the unwind the path below it does.

Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
---
 drivers/accel/rocket/rocket_core.c |  8 ++++----
 drivers/accel/rocket/rocket_core.h |  7 +++++++
 drivers/accel/rocket/rocket_drv.c  | 26 +++++++++++++++++++++++---
 3 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
index 5dd260bac..b202d1581 100644
--- a/drivers/accel/rocket/rocket_core.c
+++ b/drivers/accel/rocket/rocket_core.c
@@ -23,7 +23,7 @@ int rocket_core_init(struct rocket_core *core)
 
 	core->resets[0].id = "srst_a";
 	core->resets[1].id = "srst_h";
-	err = devm_reset_control_bulk_get_exclusive(&pdev->dev, ARRAY_SIZE(core->resets),
+	err = devm_reset_control_bulk_get_exclusive(&pdev->dev, core->soc->num_resets,
 						    core->resets);
 	if (err)
 		return dev_err_probe(dev, err, "failed to get resets for core %d\n", core->index);
@@ -32,7 +32,7 @@ int rocket_core_init(struct rocket_core *core)
 	core->clks[1].id = "hclk";
 	core->clks[2].id = "npu";
 	core->clks[3].id = "pclk";
-	err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks);
+	err = devm_clk_bulk_get(dev, core->soc->num_clks, core->clks);
 	if (err)
 		return dev_err_probe(dev, err, "failed to get clocks for core %d\n", core->index);
 
@@ -109,9 +109,9 @@ void rocket_core_fini(struct rocket_core *core)
 
 void rocket_core_reset(struct rocket_core *core)
 {
-	reset_control_bulk_assert(ARRAY_SIZE(core->resets), core->resets);
+	reset_control_bulk_assert(core->soc->num_resets, core->resets);
 
 	udelay(10);
 
-	reset_control_bulk_deassert(ARRAY_SIZE(core->resets), core->resets);
+	reset_control_bulk_deassert(core->soc->num_resets, core->resets);
 }
diff --git a/drivers/accel/rocket/rocket_core.h b/drivers/accel/rocket/rocket_core.h
index f6d738285..ba74c5339 100644
--- a/drivers/accel/rocket/rocket_core.h
+++ b/drivers/accel/rocket/rocket_core.h
@@ -27,9 +27,16 @@
 #define rocket_core_writel(core, reg, value) \
 	writel(value, (core)->core_iomem + (REG_CORE_##reg) - REG_CORE_S_STATUS)
 
+/* Per-SoC differences, selected by the of_device_id match data. */
+struct rocket_soc_data {
+	unsigned int num_clks;		/* clk_bulk count */
+	unsigned int num_resets;	/* reset_bulk count */
+};
+
 struct rocket_core {
 	struct device *dev;
 	struct rocket_device *rdev;
+	const struct rocket_soc_data *soc;
 	unsigned int index;
 
 	int irq;
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 8bbbce594..7ed64c131 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -159,8 +159,22 @@ static const struct drm_driver rocket_drm_driver = {
 
 static int rocket_probe(struct platform_device *pdev)
 {
+	const struct rocket_soc_data *soc = of_device_get_match_data(&pdev->dev);
 	int ret;
 
+	/*
+	 * soc is dereferenced without a check by every one of its users, and
+	 * rocket_core_init() below is the first of them. A device that bound
+	 * by name rather than by compatible has no match data, so fail before
+	 * anything is allocated rather than at the first dereference: the
+	 * number of cores comes from a walk of matching DT nodes, and a device
+	 * with no of_node was never counted by it.
+	 */
+	if (!soc) {
+		dev_err(&pdev->dev, "no match data for this device\n");
+		return -ENODEV;
+	}
+
 	if (rdev == NULL) {
 		/* First core probing, initialize DRM device. */
 		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
@@ -176,6 +190,7 @@ static int rocket_probe(struct platform_device *pdev)
 
 	rdev->cores[core].rdev = rdev;
 	rdev->cores[core].dev = &pdev->dev;
+	rdev->cores[core].soc = soc;
 	rdev->cores[core].index = core;
 
 	rdev->num_cores++;
@@ -213,8 +228,13 @@ static void rocket_remove(struct platform_device *pdev)
 	}
 }
 
+static const struct rocket_soc_data rk3588_soc_data = {
+	.num_clks = 4,
+	.num_resets = 2,
+};
+
 static const struct of_device_id dt_match[] = {
-	{ .compatible = "rockchip,rk3588-rknn-core" },
+	{ .compatible = "rockchip,rk3588-rknn-core", .data = &rk3588_soc_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, dt_match);
@@ -240,7 +260,7 @@ static int rocket_device_runtime_resume(struct device *dev)
 	if (core < 0)
 		return -ENODEV;
 
-	err = clk_bulk_prepare_enable(ARRAY_SIZE(rdev->cores[core].clks), rdev->cores[core].clks);
+	err = clk_bulk_prepare_enable(rdev->cores[core].soc->num_clks, rdev->cores[core].clks);
 	if (err) {
 		dev_err(dev, "failed to enable (%d) clocks for core %d\n", err, core);
 		return err;
@@ -260,7 +280,7 @@ static int rocket_device_runtime_suspend(struct device *dev)
 	if (!rocket_job_is_idle(&rdev->cores[core]))
 		return -EBUSY;
 
-	clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks), rdev->cores[core].clks);
+	clk_bulk_disable_unprepare(rdev->cores[core].soc->num_clks, rdev->cores[core].clks);
 
 	return 0;
 }
-- 
2.43.0

[PATCH v12 12/14] accel/rocket: add RK3576 NPU (RKNN) support

[Jiaxing Hu]

The RK3576 has two cores of the same RKNN block and a few platform
differences:

 - the CBUF (convolution buffer) has its own clock domain, so the core
   needs six clocks rather than four;
 - there is no per-core hclk reset. The CRU has SRST_A_RKNN0 and
   SRST_A_RKNN1 but no SRST_H_RKNN0 or SRST_H_RKNN1, so a core takes one
   reset where RK3588 takes two;
 - the NPU spans two power domains, and a device with more than one is
   skipped by the driver-core single-domain auto-attach, so the list has
   to be attached explicitly;
 - PC_TASK_CON packs the task number with sixteen bits rather than
   twelve, moving the two controls above it up by four and adding a third.

That last one is the reason this series has been reporting, since v3, that
the block accepts exactly one task per reset. rocket_registers.h is
generated from the RK3588 description, so writing it unchanged to an RK3576
asks for task_number 0x7001, which is 28673 tasks, and puts
TASK_COUNT_CLEAR inside the sixteen-bit task number, where it inflates the
count rather than clearing it. The counter is then only ever cleared by a
reset.

The layout was confirmed by Chaoyi Chen of Rockchip, including the third
control at BIT(18), task_last_layer_clear, which belongs on every submit
alongside the count clear:

  https://lore.kernel.org/all/4f300b78-d96d-4d98-8819-dc292b0c9b97@rock-chips.com/

With that written correctly a job of several tasks runs to completion, the
completion interrupt arrives, and /proc/interrupts counts up. A convolution
submitted three times with three different inputs is byte exact against the
CPU reference each time, with no reset in between and with nothing retiring
the job but the interrupt.

Counting the cores now walks the driver's own match table instead of a
second, hand-kept list of compatibles. The array sized from that count is
indexed by every core that goes on to probe, so the two lists cannot be
allowed to disagree.

All of it hangs off the soc_data added earlier, so the RK3588 path keeps
its existing counts and behaviour.

The match table moves to rocket_drv.h so rocket_device.c can walk it with
for_each_matching_node() rather than repeating a for_each_compatible_node()
loop per SoC, which also keeps num_cores in step with the table that sizes
the array it counts into. The declaration needs struct of_device_id, taken
from <linux/device-id/of.h> rather than <linux/mod_devicetable.h>, which
carries every subsystem's tables with it.

Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
---
 drivers/accel/rocket/rocket_core.c   | 20 ++++++++++++++
 drivers/accel/rocket/rocket_core.h   |  8 +++---
 drivers/accel/rocket/rocket_device.c |  7 ++++-
 drivers/accel/rocket/rocket_drv.c    | 16 +++++++++---
 drivers/accel/rocket/rocket_drv.h    |  2 ++
 drivers/accel/rocket/rocket_job.c    | 39 +++++++++++++++++++++++++---
 6 files changed, 81 insertions(+), 11 deletions(-)

diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
index b202d1581..91f690176 100644
--- a/drivers/accel/rocket/rocket_core.c
+++ b/drivers/accel/rocket/rocket_core.c
@@ -8,6 +8,7 @@
 #include <linux/err.h>
 #include <linux/iommu.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 
@@ -21,6 +22,7 @@ int rocket_core_init(struct rocket_core *core)
 	u32 version;
 	int err = 0;
 
+	/* RK3576 has no per-core hclk reset, so it takes srst_a alone. */
 	core->resets[0].id = "srst_a";
 	core->resets[1].id = "srst_h";
 	err = devm_reset_control_bulk_get_exclusive(&pdev->dev, core->soc->num_resets,
@@ -32,6 +34,9 @@ int rocket_core_init(struct rocket_core *core)
 	core->clks[1].id = "hclk";
 	core->clks[2].id = "npu";
 	core->clks[3].id = "pclk";
+	/* RK3576 clocks the CBUF separately; the compute path stalls without these. */
+	core->clks[4].id = "aclk_cbuf";
+	core->clks[5].id = "hclk_cbuf";
 	err = devm_clk_bulk_get(dev, core->soc->num_clks, core->clks);
 	if (err)
 		return dev_err_probe(dev, err, "failed to get clocks for core %d\n", core->index);
@@ -60,6 +65,21 @@ int rocket_core_init(struct rocket_core *core)
 	if (err)
 		return err;
 
+	/*
+	 * RK3576 spans two power domains, and a multi-domain device is skipped
+	 * by the driver-core single-domain auto-attach, so attach the list here.
+	 * This goes before the first thing that would have to be unwound, so a
+	 * failure can simply return.
+	 */
+	if (core->soc->multi_power_domain) {
+		struct dev_pm_domain_list *pd_list;
+
+		err = devm_pm_domain_attach_list(dev, NULL, &pd_list);
+		if (err < 0)
+			return dev_err_probe(dev, err,
+					     "failed to attach NPU power domains\n");
+	}
+
 	core->iommu_group = iommu_group_get(dev);
 
 	err = rocket_job_init(core);
diff --git a/drivers/accel/rocket/rocket_core.h b/drivers/accel/rocket/rocket_core.h
index ba74c5339..8c8d1f453 100644
--- a/drivers/accel/rocket/rocket_core.h
+++ b/drivers/accel/rocket/rocket_core.h
@@ -29,8 +29,10 @@
 
 /* Per-SoC differences, selected by the of_device_id match data. */
 struct rocket_soc_data {
-	unsigned int num_clks;		/* clk_bulk count */
-	unsigned int num_resets;	/* reset_bulk count */
+	unsigned int num_clks;		/* clk_bulk count: 4 base, 6 with CBUF */
+	unsigned int num_resets;	/* reset_bulk count: 2 base, 1 on RK3576 */
+	bool multi_power_domain;	/* device spans more than one PM domain */
+	bool task_con_16bit;		/* PC_TASK_CON uses the 16-bit task number */
 };
 
 struct rocket_core {
@@ -43,7 +45,7 @@ struct rocket_core {
 	void __iomem *pc_iomem;
 	void __iomem *cna_iomem;
 	void __iomem *core_iomem;
-	struct clk_bulk_data clks[4];
+	struct clk_bulk_data clks[6];
 	struct reset_control_bulk_data resets[2];
 
 	struct iommu_group *iommu_group;
diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c
index 46e6ee1e7..923add5bd 100644
--- a/drivers/accel/rocket/rocket_device.c
+++ b/drivers/accel/rocket/rocket_device.c
@@ -9,6 +9,7 @@
 #include <linux/of.h>
 
 #include "rocket_device.h"
+#include "rocket_drv.h"
 
 struct rocket_device *rocket_device_init(struct platform_device *pdev,
 					 const struct drm_driver *rocket_drm_driver)
@@ -27,7 +28,11 @@ struct rocket_device *rocket_device_init(struct platform_device *pdev,
 	ddev = &rdev->ddev;
 	dev_set_drvdata(dev, rdev);
 
-	for_each_compatible_node(core_node, NULL, "rockchip,rk3588-rknn-core")
+	/*
+	 * Count over the same match table the platform driver binds with, so
+	 * that a core added there is counted here without a second edit.
+	 */
+	for_each_matching_node(core_node, rocket_dt_match)
 		if (of_device_is_available(core_node))
 			num_cores++;
 
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 7ed64c131..f387b4656 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -231,13 +231,23 @@ static void rocket_remove(struct platform_device *pdev)
 static const struct rocket_soc_data rk3588_soc_data = {
 	.num_clks = 4,
 	.num_resets = 2,
+	.multi_power_domain = false,
+	.task_con_16bit = false,
 };
 
-static const struct of_device_id dt_match[] = {
+static const struct rocket_soc_data rk3576_soc_data = {
+	.num_clks = 6,
+	.num_resets = 1,
+	.multi_power_domain = true,
+	.task_con_16bit = true,
+};
+
+const struct of_device_id rocket_dt_match[] = {
 	{ .compatible = "rockchip,rk3588-rknn-core", .data = &rk3588_soc_data },
+	{ .compatible = "rockchip,rk3576-rknn-core", .data = &rk3576_soc_data },
 	{}
 };
-MODULE_DEVICE_TABLE(of, dt_match);
+MODULE_DEVICE_TABLE(of, rocket_dt_match);
 
 static int find_core_for_dev(struct device *dev)
 {
@@ -296,7 +306,7 @@ static struct platform_driver rocket_driver = {
 	.driver	 = {
 		.name = "rocket",
 		.pm = pm_ptr(&rocket_pm_ops),
-		.of_match_table = dt_match,
+		.of_match_table = rocket_dt_match,
 	},
 };
 
diff --git a/drivers/accel/rocket/rocket_drv.h b/drivers/accel/rocket/rocket_drv.h
index 2c673bb99..0cd692a66 100644
--- a/drivers/accel/rocket/rocket_drv.h
+++ b/drivers/accel/rocket/rocket_drv.h
@@ -6,10 +6,12 @@
 
 #include <drm/drm_mm.h>
 #include <drm/gpu_scheduler.h>
+#include <linux/device-id/of.h>
 
 #include "rocket_device.h"
 
 extern const struct dev_pm_ops rocket_pm_ops;
+extern const struct of_device_id rocket_dt_match[];
 
 struct rocket_iommu_domain {
 	struct iommu_domain *domain;
diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index 8cffe93f6..c5396c62d 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -21,6 +21,30 @@
 
 #define JOB_TIMEOUT_MS 500
 
+/*
+ * PC_TASK_CON packs the task number with control bits above it, and neither
+ * the width of the number nor the count of the controls is the same on every
+ * SoC. rocket_registers.h is generated from the RK3588 description, where the
+ * task number is twelve bits and there are two:
+ *
+ *   RK3588   BIT[11:0] task_number, BIT[12] pp_en, BIT[13] count_clear
+ *   RK3576   BIT[15:0] task_number, BIT[16] pp_en, BIT[17] count_clear,
+ *            BIT[18] last_layer_clear
+ *
+ * The RK3576 layout was confirmed by Chaoyi Chen of Rockchip:
+ * https://lore.kernel.org/all/4f300b78-d96d-4d98-8819-dc292b0c9b97@rock-chips.com/
+ *
+ * Writing the RK3588 layout to an RK3576 therefore asks for task_number
+ * 0x7001, that is 28673 tasks, and lands the count clear on a bit that does
+ * nothing. The task counter is then only ever cleared by a reset, which is
+ * exactly the "one task per reset" behaviour this series has been reporting
+ * since v3.
+ */
+#define RK3576_PC_TASK_CON_TASK_NUMBER(n)	((n) & 0xffff)
+#define RK3576_PC_TASK_CON_PP_EN		BIT(16)
+#define RK3576_PC_TASK_CON_COUNT_CLEAR		BIT(17)
+#define RK3576_PC_TASK_CON_LAST_LAYER_CLEAR	BIT(18)
+
 static struct rocket_job *
 to_rocket_job(struct drm_sched_job *sched_job)
 {
@@ -142,10 +166,17 @@ static void rocket_job_hw_submit(struct rocket_core *core, struct rocket_job *jo
 	rocket_pc_writel(core, INTERRUPT_MASK, PC_INTERRUPT_MASK_DPU_0 | PC_INTERRUPT_MASK_DPU_1);
 	rocket_pc_writel(core, INTERRUPT_CLEAR, PC_INTERRUPT_CLEAR_DPU_0 | PC_INTERRUPT_CLEAR_DPU_1);
 
-	rocket_pc_writel(core, TASK_CON, PC_TASK_CON_RESERVED_0(1) |
-					 PC_TASK_CON_TASK_COUNT_CLEAR(1) |
-					 PC_TASK_CON_TASK_NUMBER(1) |
-					 PC_TASK_CON_TASK_PP_EN(1));
+	if (core->soc->task_con_16bit)
+		rocket_pc_writel(core, TASK_CON,
+				 RK3576_PC_TASK_CON_LAST_LAYER_CLEAR |
+				 RK3576_PC_TASK_CON_COUNT_CLEAR |
+				 RK3576_PC_TASK_CON_PP_EN |
+				 RK3576_PC_TASK_CON_TASK_NUMBER(1));
+	else
+		rocket_pc_writel(core, TASK_CON, PC_TASK_CON_RESERVED_0(1) |
+						 PC_TASK_CON_TASK_COUNT_CLEAR(1) |
+						 PC_TASK_CON_TASK_NUMBER(1) |
+						 PC_TASK_CON_TASK_PP_EN(1));
 
 	rocket_pc_writel(core, TASK_DMA_BASE_ADDR, PC_TASK_DMA_BASE_ADDR_DMA_BASE_ADDR(0x0));
 
-- 
2.43.0

[PATCH v12 13/14] arm64: dts: rockchip: add NPU (RKNN) nodes to rk3576

[Jiaxing Hu]

Add the two RKNN cores and their IOMMUs. Both cores are disabled by
default; boards enable what they wire up.

PD_NPU0 and PD_NPU1 are siblings under PD_NPUTOP and hold one core each,
but the convolution buffer and the DSU sit above them: ACLK_RKNN_CBUF,
HCLK_RKNN_CBUF and CLK_RKNN_DSU0 belong to the block rather than to either
core, and PD_NPUTOP already lists all three. Add them to both core domains
as well, so a core domain switching state has the clocks of the path it
shares running, and give each core domain the BIU reset that the pmdomain
driver now cycles once power is on.

Each core lists both core domains, its own first, so that a core in use has
the whole block powered. Whether a single core can reach the shared path
with the sibling domain off is not something this series establishes;
listing both is the description that has been tested here. The IOMMU in
front of each core lists that core's domain only.

Label the outer PD_NPU node so a board can attach the NPU rail to the
domain that gates the block.

Clock the NPU inside the voltage its rail is given. CLK_RKNN_DSU0 clocks
both cores and the CBUF they share, nothing in mainline sets its rate, and
the block comes up at 786.432 MHz. Rockchip's OPP table for this NPU asks
800 mV of its 800 MHz step at the worst leakage bins, and nothing in
mainline sets the rail either, so a board that follows this DTS runs the
NPU above the step whose voltage it happens to boot with.

On a ROCK 4D with both cores enabled and vdd_npu_s0 at the 750 mV its PMIC
comes up with, two jobs in flight at once make the second core write single
words of its output wrong: the right value with a bit of the accumulator
set, always the same position in the array. Either core alone is exact.
Four device trees, same board, kernel and userspace, four passes of 5400
rows each, every row compared with the same multiply done one row at a
time:

  786 MHz, 750 mV    13 to 20 wrong rows a pass
  594 MHz, 750 mV    0, 0, 0, 0
  786 MHz, 800 mV    0, 0, 0, 0
  786 MHz, 850 mV    0, 0, 0, 0

594 MHz is a divider off GPLL and sits between that table's 500 and 600 MHz
steps, both of which ask 725 mV at every leakage bin, so it is inside the
voltage a board that describes no NPU rail already provides.

The trade it buys is a core against a clock, and both halves are measured.
The rate lives in the device tree, so the two clocks cannot share a boot;
boot-to-boot drift was measured first, by booting 594 twice, and is under
1%. Five runs an arm, the arms alternating inside a boot, one warm-up a
model discarded, medians of five:

  decode tok/s      594 MHz   786 MHz
  Llama-3.2-1B        17.85     18.60   two cores
                      11.17     13.77   one core
  SmolLM2-135M        41.46     43.12   two cores
                      38.26     41.90   one core

Losing 192 MHz costs 4.0 to 4.2% of decode with both cores running. Losing
a core costs 26 to 37% on the 1B model, at either clock. The rate is the
cheaper of the two by seven to nine times.

The two arms cross-check each other: the clock is worth 23% on ONE core
against 4% on two. With both cores running the bottleneck is no longer the
clock, which is why this configuration can afford to give up 192 MHz.

A core is worth much less on a small model, 2.8 to 7.7% on 135M, where the
second core's dispatch overhead is not repaid. TTFT moves by under 2%
either way, so none of this says anything about prefill.

An OPP table with the rail attached is the proper answer, and it wants
driver support this series does not have.

Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
---
 arch/arm64/boot/dts/rockchip/rk3576.dtsi | 86 +++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3576.dtsi b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
index d418bfc04..e9cd11b58 100644
--- a/arch/arm64/boot/dts/rockchip/rk3576.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
@@ -1042,7 +1042,7 @@ power: power-controller {
 				#address-cells = <1>;
 				#size-cells = <0>;
 
-				power-domain@RK3576_PD_NPU {
+				pd_npu: power-domain@RK3576_PD_NPU {
 					reg = <RK3576_PD_NPU>;
 					#power-domain-cells = <1>;
 					#address-cells = <1>;
@@ -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>;
 							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>;
 							pm_qos = <&qos_npu_m1>;
 							#power-domain-cells = <0>;
 						};
@@ -1261,6 +1269,78 @@ power-domain@RK3576_PD_VO1 {
 			};
 		};
 
+		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";
+			assigned-clocks = <&cru CLK_RKNN_DSU0>;
+			assigned-clock-rates = <594000000>;
+			resets = <&cru SRST_A_RKNN0>;
+			reset-names = "srst_a";
+			power-domains = <&power RK3576_PD_NPU0>, <&power RK3576_PD_NPU1>;
+			iommus = <&rknn_mmu_0>;
+			status = "disabled";
+		};
+
+		rknn_mmu_0: iommu@27702000 {
+			compatible = "rockchip,rk3576-npu-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>;
+			clock-names = "aclk", "iface", "npu",
+				      "aclk_cbuf", "hclk_cbuf";
+			#iommu-cells = <0>;
+			power-domains = <&power RK3576_PD_NPU0>;
+			status = "disabled";
+		};
+
+		rknn_core_1: npu@27708000 {
+			compatible = "rockchip,rk3576-rknn-core";
+			reg = <0x0 0x27708000 0x0 0x1000>,
+			      <0x0 0x27709000 0x0 0x1000>,
+			      <0x0 0x2770b000 0x0 0x1000>;
+			reg-names = "pc", "cna", "core";
+			interrupts = <GIC_SPI 248 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cru ACLK_RKNN1>, <&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";
+			assigned-clocks = <&cru CLK_RKNN_DSU0>;
+			assigned-clock-rates = <594000000>;
+			resets = <&cru SRST_A_RKNN1>;
+			reset-names = "srst_a";
+			power-domains = <&power RK3576_PD_NPU1>, <&power RK3576_PD_NPU0>;
+			iommus = <&rknn_mmu_1>;
+			status = "disabled";
+		};
+
+		rknn_mmu_1: iommu@2770a000 {
+			compatible = "rockchip,rk3576-npu-iommu", "rockchip,rk3568-iommu";
+			reg = <0x0 0x2770a000 0x0 0x100>,
+			      <0x0 0x2770a100 0x0 0x100>;
+			interrupts = <GIC_SPI 248 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cru ACLK_RKNN1>, <&cru HCLK_RKNN_ROOT>,
+				 <&cru CLK_RKNN_DSU0>, <&cru ACLK_RKNN_CBUF>,
+				 <&cru HCLK_RKNN_CBUF>;
+			clock-names = "aclk", "iface", "npu",
+				      "aclk_cbuf", "hclk_cbuf";
+			#iommu-cells = <0>;
+			power-domains = <&power RK3576_PD_NPU1>;
+			status = "disabled";
+		};
+
 		gpu: gpu@27800000 {
 			compatible = "rockchip,rk3576-mali", "arm,mali-bifrost";
 			reg = <0x0 0x27800000 0x0 0x20000>;
-- 
2.43.0

[PATCH v12 14/14] arm64: dts: rockchip: enable the NPU on rk3576-rock-4d

[Jiaxing Hu]

Enable both RKNN cores and their IOMMUs on the Radxa ROCK 4D, and hand
vdd_npu_s0 to the NPU power domain as its domain-supply, so the rail is
switched by the domain that gates the block.

Measured on a ROCK 4D with this in place. The rail's regulator debugfs
reports open_count 1, so the domain is the consumer that took it. A sampler
running beside an inference caught use_count at 1, and three reads at rest
report use_count 0 with the rail disabled, so it follows the domain rather
than staying on. Over the same run the genpd active_time of all four NPU
domains rises by roughly 60ms per inference, and the inferences either side
of that are 128 of 128 channels against the CPU reference.

Both cores keep npu-supply on the same rail, which the binding requires.
v11 enabled rknn_core_0 alone and left the second to whoever could test it;
it has been tested since. Both cores probe, each with its IOMMU, and a
runtime that deals its submits across the two returns text identical to the
one-core run over nine language models.

Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
---
 .../boot/dts/rockchip/rk3576-rock-4d.dts      | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts b/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts
index 272af1012..93d59c0a9 100644
--- a/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts
@@ -722,6 +722,10 @@ &pcie0 {
 	status = "okay";
 };
 
+&pd_npu {
+	domain-supply = <&vdd_npu_s0>;
+};
+
 &pinctrl {
 	hdmi {
 		hdmi_tx_on_h: hdmi-tx-on-h {
@@ -779,6 +783,24 @@ wifi_en_h: wifi-en-h {
 	};
 };
 
+&rknn_core_0 {
+	npu-supply = <&vdd_npu_s0>;
+	status = "okay";
+};
+
+&rknn_mmu_0 {
+	status = "okay";
+};
+
+&rknn_core_1 {
+	npu-supply = <&vdd_npu_s0>;
+	status = "okay";
+};
+
+&rknn_mmu_1 {
+	status = "okay";
+};
+
 &sai6 {
 	status = "okay";
 };
-- 
2.43.0

Re: [PATCH v12 08/14] dt-bindings: iommu: rockchip: describe the RK3576 NPU MMU

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `rockchip,rk3568-iommu` fallback compatible string is invalid for `rockchip,rk3576-npu-iommu` because older OS drivers binding to the fallback will only enable 2 clocks, causing device failure.
--

commit 0d5ec29abf5273f66b488bef225aa90e06766258
Author: Jiaxing Hu <gahing@gahingwoo.com>

dt-bindings: iommu: rockchip: describe the RK3576 NPU MMU

The RK3576 NPU MMUs are rk3568-iommu compatible but take five clocks where
every other Rockchip MMU takes two, the extra three being the compute clock
and the two convolution buffer clocks.

> diff --git a/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml b/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml
> index 6ce41d11ff5e5..83d7e7c8e0c22 100644
> --- a/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml
> @@ -26,6 +26,7 @@ properties:
>        - items:
>            - enum:
>                - rockchip,rk3576-iommu
> +              - rockchip,rk3576-npu-iommu
>                - rockchip,rk3588-iommu
>            - const: rockchip,rk3568-iommu

[Severity: High]
Is rockchip,rk3568-iommu a safe fallback compatible string for
rockchip,rk3576-npu-iommu?

If an older kernel (e.g., prior to the July 2026 driver update introducing
devm_clk_bulk_get_all) boots with an updated device tree, it will bind to the
rockchip,rk3568-iommu fallback. The older driver hardcodes requests for only 2
clocks (aclk and iface) and leaves the 3 required NPU clocks disabled during
register access.

On Rockchip SoCs, accessing registers with disabled clocks typically causes a
bus hang or synchronous external abort, panicking the system. Does this
violate device tree backward compatibility guarantees?

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

Re: [PATCH v12 01/14] accel/rocket: request the core clocks by name

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Probe deferral, missing clocks, or device unbind leaks devres allocations attached to the global `drm_dev`.
- [High] Incorrect core tracking logic in `rocket_remove` and `rocket_probe` causes active cores to be orphaned, unreachable, or silently overwritten upon partial unbind.
--

commit 4baecbc1d36cdb2c0321f080e879383800570708
Author: Igor Paunovic <royalnet026@gmail.com>

accel/rocket: request the core clocks by name

This patch fixes an issue where the NPU driver failed to set the .id members
of its clk_bulk_data struct before requesting them. Without IDs, clk_bulk_get
defaulted to returning the first clock in the device tree for all entries. The
clocks are now properly requested by name as required by the DT binding.

> diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
> index b3b2fa9ba645a..5dd260bacbff6 100644
> --- a/drivers/accel/rocket/rocket_core.c
> +++ b/drivers/accel/rocket/rocket_core.c
> @@ -28,6 +28,10 @@ int rocket_core_init(struct rocket_core *core)
>  	if (err)
>  		return dev_err_probe(dev, err, "failed to get resets for core %d\n", core->index);
>  
> +	core->clks[0].id = "aclk";
> +	core->clks[1].id = "hclk";
> +	core->clks[2].id = "npu";
> +	core->clks[3].id = "pclk";
>  	err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks);
>  	if (err)
>  		return dev_err_probe(dev, err, "failed to get clocks for core %d\n", core->index);

[Severity: High]
This is a pre-existing issue, but does this failure path cause an unbounded
resource leak if devm_clk_bulk_get() defers probe or fails?

If rocket_core_init() fails here, rocket_probe() cleans up by calling
rocket_device_fini() and setting the global rdev pointer to NULL:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
    ...
	ret = rocket_core_init(&rdev->cores[core]);
	if (ret) {
		rdev->num_cores--;

		if (rdev->num_cores == 0) {
			rocket_device_fini(rdev);
			rdev = NULL;
		}
	}
    ...
}

However, rdev and its cores array are allocated in rocket_device_init() using
devm_* functions attached to drm_dev, which is a static global device:

drivers/accel/rocket/rocket_device.c:rocket_device_init() {
    ...
	rdev = devm_drm_dev_alloc(dev, rocket_drm_driver, struct rocket_device, ddev);
    ...
	rdev->cores = devm_kcalloc(dev, num_cores, sizeof(*rdev->cores), GFP_KERNEL);
    ...
}

Because drm_dev is global and persists across probe deferrals, its devres
list is never cleared. When rocket_probe() executes again, a new rdev is
allocated on the same global device, permanently leaking the previous one.

[Severity: High]
This is also a pre-existing issue, but if this failure path is hit (or if a
core is unbound via sysfs), does the core tracking logic in rocket_remove()
and rocket_probe() orphan the remaining active cores?

If a core other than the most-recently-probed one fails or unbinds,
rocket_remove() blindly decrements rdev->num_cores:

drivers/accel/rocket/rocket_drv.c:rocket_remove() {
    ...
	rocket_core_fini(&rdev->cores[core]);
	rdev->num_cores--;
    ...
}

Without shifting array elements to compact the list, active cores located
past the shrunken num_cores boundary can no longer be found for teardown or
power management:

drivers/accel/rocket/rocket_drv.c:find_core_for_dev() {
    ...
	for (unsigned int core = 0; core < rdev->num_cores; core++) {
		if (dev == rdev->cores[core].dev)
			return core;
	}
    ...
}

This could cause power management callbacks to fail with -ENODEV while the
hardware is still running. Furthermore, if a new core binds later,
rocket_probe() will insert it at rdev->num_cores, which could silently
overwrite the live context of the stranded core.

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

Re: [PATCH v12 02/14] accel/rocket: take the completion register writes under job_lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The shared hardirq handler accesses hardware registers without checking if the device is active via pm_runtime_get_if_active(), causing spurious interrupts and potential bus faults when suspended.
- [High] The runtime suspend callback disables hardware clocks without first calling `synchronize_irq()`, leading to potential race conditions and hardware lockups with the shared IRQ handler.
- [Medium] Leak of IOMMU group kobject reference in `rocket_job_handle_irq()`.
- [Medium] The module removal path calls `pm_runtime_disable()` which can cancel a pending asynchronous autosuspend, leaving the hardware clocks permanently enabled.
--

commit 628f481e9ee69f13147956ea2124dd232b5d1c38
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: take the completion register writes under job_lock

This patch fixes a race condition by moving the OPERATION_ENABLE and
INTERRUPT_CLEAR register writes inside the job_lock scoped_guard. This ensures
that stopping the block and deciding what to start next are performed as a
single atomic step.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index f404355058185..575945015ca3b 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -345,10 +345,15 @@ static void rocket_job_handle_irq(struct rocket_core *core)
>  {
>  	pm_runtime_mark_last_busy(core->dev);
>  
> -	rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
> -	rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
> +	scoped_guard(mutex, &core->job_lock) {
> +		/*
> +		 * Stopping the block belongs under the lock. hw_submit() writes
> +		 * OPERATION_ENABLE too, and outside the lock this zero can land
> +		 * after that one and stop a task that has only just started.
> +		 */
> +		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);

[Severity: Medium]
This is a pre-existing issue, but does this code leak a kobject
reference every time a job completes?

Just below this hunk in rocket_job_handle_irq(), the completion path has:

			iommu_detach_group(NULL, iommu_group_get(core->dev));

Calling iommu_group_get(core->dev) retrieves the group and increments its
reference count.

However, iommu_detach_group() does not consume this reference, and there is
no matching iommu_group_put() in this path.

Could this be fixed by using the pre-cached core->iommu_group instead?

[ ... ]


[Severity: High]
This is a pre-existing issue, but can the shared IRQ handler cause an
asynchronous bus fault or spurious handling when the device is suspended?

The interrupt is registered as IRQF_SHARED in rocket_job_init():

drivers/accel/rocket/rocket_job.c:rocket_job_init() {
    ...
	ret = devm_request_threaded_irq(core->dev, core->irq,
					rocket_job_irq_handler,
					rocket_job_irq_handler_thread,
					IRQF_SHARED, dev_name(core->dev), core);
    ...
}

If another device on the same shared line triggers an interrupt while the
rocket device is runtime-suspended, the handler blindly reads from the hardware:

drivers/accel/rocket/rocket_job.c:rocket_job_irq_handler() {
	struct rocket_core *core = data;
	u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
    ...
}

This unprotected read will typically return 0xffffffff when clocks are
disabled, which satisfies the bitmask checks and causes the handler to wake
the IRQ thread. The thread then executes on powered-off hardware.

Per subsystem guidelines, should we check if the device is active using
pm_runtime_get_if_active() before accessing registers here?


[Severity: High]
This is a pre-existing issue, but does the runtime suspend callback disable
clocks while the shared IRQ handler might still be executing?

drivers/accel/rocket/rocket_drv.c:rocket_device_runtime_suspend() {
	...
	clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks),
				   rdev->cores[core].clks);

	return 0;
}

Since the interrupt is registered as IRQF_SHARED, the handler could be running
concurrently on another CPU.

If clocks are disabled exactly while the in-flight handler attempts to access
hardware registers, it will cause a hardware lockup.

Per subsystem guidelines, drivers using IRQF_SHARED must call
synchronize_irq() in their runtime suspend callback before powering down.

Should a synchronize_irq() call be added here?


[Severity: Medium]
This is a pre-existing issue, but can unloading the module leave the hardware
powered on permanently?

At the end of rocket_core_init() and during job completions, an autosuspend
is scheduled using pm_runtime_put_autosuspend(). However, the removal path
calls pm_runtime_disable() directly:

drivers/accel/rocket/rocket_core.c:rocket_core_fini() {
	pm_runtime_dont_use_autosuspend(core->dev);
	pm_runtime_disable(core->dev);
	iommu_group_put(core->iommu_group);
    ...
}

Calling pm_runtime_disable() cancels any pending asynchronous autosuspend work.

If the module is removed while an autosuspend is pending, the device is left
powered on with clocks enabled.

Should we ensure the device is fully suspended (e.g., using
pm_runtime_put_sync()) before disabling runtime PM?

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

Re: [PATCH v12 10/14] pmdomain: rockchip: cycle optional power-domain resets on power-on

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Massive Use-After-Free of genpd structures if probe fails and pm_genpd_remove() fails during error cleanup.
--

commit 82ba89727b40b24aa0ac406b6e9c71489c988d78
Author: Jiaxing Hu <gahing@gahingwoo.com>

    pmdomain: rockchip: cycle optional power-domain resets on power-on
    
    Some Rockchip domains come out of power-on with their bus interface in an
    undefined state. On the RK3576 NPU this shows up as a hang on the first
    register access after the domain is switched on, and pulsing the domain's
    resets at this point clears it.

> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c

[ ... ]

> @@ -949,6 +975,7 @@ static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
>  
>  	clk_bulk_unprepare(pd->num_clks, pd->clks);
>  	clk_bulk_put(pd->num_clks, pd->clks);
> +	reset_control_put(pd->resets);
>  
>  	/* protect the zeroing of pm->num_clks */
>  	mutex_lock(&pd->pmu->mutex);

[Severity: High]
This is a pre-existing issue, but does this error cleanup path leave a freed
structure in the global gpd_list if probe fails?

During error cleanup, rockchip_pm_domain_cleanup() iterates forward through
the domains array:

    for (i = 0; i < pmu->genpd_data.num_domains; i++) {
        genpd = pmu->genpd_data.domains[i];
        if (genpd) {
            pd = to_rockchip_pd(genpd);
            rockchip_pm_remove_one_domain(pd);
        }
    }

Since parent domains typically appear before their children in the array, the
parent is visited first. 

When rockchip_pm_remove_one_domain() attempts to remove the parent domain,
pm_genpd_remove() fails with -EBUSY because the child is still attached:

    ret = pm_genpd_remove(&pd->genpd);
    if (ret < 0)
        dev_err(pd->pmu->dev, "failed to remove domain '%s' : %d...",

The domain is left in the global gpd_list and the function continues without
retrying.

Because rockchip_pm_add_one_domain() allocates the domain structure with
devm_kzalloc():

    pd = devm_kzalloc(pmu->dev, sizeof(*pd), GFP_KERNEL);

When probe returns an error, such as -EPROBE_DEFER, devres automatically frees
the memory backing the parent domain. 

Can this leave a dangling pointer in the global gpd_list that could trigger a
use-after-free or kernel panic when the list is next traversed, such as
during pm_genpd_poweroff_unused()? Should rockchip_pm_domain_cleanup() be
updated to iterate backwards to ensure children are removed before their
parents?

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

Re: [PATCH v12 04/14] accel/rocket: let the core suspend after a reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Async vs Synchronous Runtime PM Put race condition violates hardware power cycle constraints.

Pre-existing issues:
- [High] Shared IRQ handler accesses hardware registers without verifying runtime PM state, risking SError crashes.
- [High] Runtime suspend callback disables clocks without calling synchronize_irq().
- [High] Reference count leak of IOMMU group in job completion path.
--

commit 230b2e3fd33edbead6ba7eab402e2a3e06d6ef3a
Author: Jiaxing Hu <gahing@gahingwoo.com>
accel/rocket: let the core suspend after a reset

This commit changes the runtime PM put in rocket_reset() from noidle to
autosuspend. This aims to allow the core to suspend and cycle its power
domain after a reset, which is necessary on platforms like RK3576 to
recover the IOMMU.

Link: https://lore.kernel.org/all/20260819073530.6087-1-royalnet026@gmail.com/

> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -421,12 +421,12 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>  
>  	/*
>  	 * No handler is running now, but we might still have stuck jobs. Let's
> -	 * make sure the PM counters stay balanced by manually calling
> -	 * pm_runtime_put_noidle().
> +	 * make sure the PM counters stay balanced by putting the reference the
> +	 * job took, and request idle while doing it so the core can suspend.
>  	 */
>  	scoped_guard(mutex, &core->job_lock) {
>  		if (core->in_flight_job)
> -			pm_runtime_put_noidle(core->dev);
> +			pm_runtime_put_autosuspend(core->dev);

[Severity: High]
Will this asynchronous suspend actually have time to trigger before the
scheduler starts again?

The RK3576 NPU requires a power domain cycle (suspend and resume) to reset
its bus interface and recover the IOMMU after a job timeout. However,
pm_runtime_put_autosuspend() queues an asynchronous suspend that may not
fire immediately.

[ ... ]

>  		iommu_detach_group(NULL, core->iommu_group);
>  
>  		core->in_flight_job = NULL;
>  	}
>  
>  	/* Proceed with reset now. */
>  	rocket_core_reset(core);
>  
>  	/* NPU has been reset, we can clear the reset pending bit. */
>  	atomic_set(&core->reset.pending, 0);
>  
>  	/* Restart the scheduler */
>  	drm_sched_start(&core->sched, 0);
>  }

[Severity: High]
If there are multiple jobs queued, doesn't restarting the scheduler here
immediately dequeue the next job?

If the next job calls rocket_job_run() and invokes
pm_runtime_resume_and_get(), this would cancel the pending autosuspend
before the timer expires. Would it be safer to use a synchronous suspend
(like pm_runtime_put_sync_suspend()) to enforce the hardware ordering
constraint before restarting the scheduler?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
issue with how the shared IRQ handler accesses hardware registers:

drivers/accel/rocket/rocket_job.c:rocket_job_irq_handler() {
    u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
    ...
}

Since the interrupt is shared (IRQF_SHARED) with the IOMMU, couldn't this
handler be invoked unconditionally while the rocket_core is in an
RPM_SUSPENDED state? Reading memory-mapped registers of a suspended device
could cause an asynchronous SError and crash the system. Should this be
guarded with pm_runtime_get_if_active()?

[Severity: High]
This is a pre-existing issue, but the runtime suspend callback disables
clocks without synchronizing the shared IRQ:

drivers/accel/rocket/rocket_drv.c:rocket_device_runtime_suspend() {
    ...
    clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks),
                               rdev->cores[core].clks);
    ...
}

Because the device uses a shared interrupt, is there a race condition where
the interrupt handler might be actively executing on another CPU while the
runtime suspend callback powers down the hardware? Should this callback use
synchronize_irq() first?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
reference count leak in the job completion path:

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
    ...
    iommu_detach_group(NULL, iommu_group_get(core->dev));
    ...
}

Does iommu_detach_group() consume the reference returned by
iommu_group_get()? If it doesn't, this seems to leak a reference on the
IOMMU group every time a job is completed. Could this use the cached
core->iommu_group or add a corresponding iommu_group_put()?

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

Re: [PATCH v12 05/14] accel/rocket: factor the completion tail out of the IRQ handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Leaking IOMMU group reference on job completion in rocket_job_next_locked().
- [High] Unconditional hardware access in shared IRQ handler while device may be suspended.
- [High] Missing synchronize_irq() in runtime suspend callback allows shared IRQ to race with clock disable.
--

commit d5e7944e1f53bee9ce55d9b7485126ab8b104207
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: factor the completion tail out of the IRQ handler

rocket_job_handle_irq() stops the block and then either starts the job's
next task or retires the job. The second half is a step of its own and
reads better with a name, now that taking the register writes under
job_lock has moved it a level deeper inside the scoped guard.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index b588049aaa6ce..8cffe93f6c5ab 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -341,6 +341,25 @@ static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job)
>  	return ERR_PTR(ret);
>  }
>  
> +/* Start the job's next task, or retire it. Caller holds job_lock. */
> +static void rocket_job_next_locked(struct rocket_core *core)
> +{
> +	lockdep_assert_held(&core->job_lock);
> +
> +	if (!core->in_flight_job)
> +		return;
> +
> +	if (core->in_flight_job->next_task_idx < core->in_flight_job->task_count) {
> +		rocket_job_hw_submit(core, core->in_flight_job);
> +		return;
> +	}
> +
> +	iommu_detach_group(NULL, iommu_group_get(core->dev));

[Severity: High]
This is a pre-existing issue, but does this code leak the iommu_group
reference?

iommu_group_get() explicitly increments the reference count of the IOMMU group,
but iommu_detach_group() only uses the pointer and doesn't take ownership.

Since iommu_group_put() is never called here, the reference leaks on every
job completion. This can eventually saturate the refcount and prevent release
during device teardown.

Would it be better to use the already-cached core->iommu_group directly here?

> +	dma_fence_signal(core->in_flight_job->done_fence);
> +	pm_runtime_put_autosuspend(core->dev);
> +	core->in_flight_job = NULL;
> +}
> +
>  static void rocket_job_handle_irq(struct rocket_core *core)
>  {

[ ... ]

[Severity: High]
This is another pre-existing issue, but can this unconditionally access
hardware registers while the device is suspended?

The IRQ is requested with IRQF_SHARED:

rocket_job_init() {
    ...
    ret = devm_request_threaded_irq(core->dev, core->irq,
                                    rocket_job_irq_handler,
                                    ...
                                    IRQF_SHARED, dev_name(core->dev),
                                    core);
    ...
}

However, the handler immediately reads hardware registers without checking if
the device is active:

rocket_job_irq_handler() {
    ...
    u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
    ...
}

If another device on the same shared line raises an interrupt while the Rocket
device is suspended, reading the suspended device's registers can cause a bus
fault or return 0xffffffff.

If it returns 0xffffffff, bitwise checks will falsely pass, leading to
invalid writes to INTERRUPT_MASK and spuriously waking the threaded handler.

Should this use pm_runtime_get_if_active() before accessing the registers?


[Severity: High]
This is a pre-existing issue, but can the shared IRQ handler race with
clock disable during runtime suspend?

The runtime suspend callback disables clocks without first synchronizing the
IRQ:

rocket_device_runtime_suspend() {
    ...
    clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks),
                               rdev->cores[core].clks);
    ...
}

If another device on the shared IRQ line triggers an interrupt concurrently,
rocket_job_irq_handler() could execute on another CPU while the hardware clocks
are being disabled, potentially leading to a bus fault or system crash.

Does this need a call to synchronize_irq() before powering down the hardware?

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

Re: [PATCH v12 11/14] accel/rocket: select the per-core clock and reset counts from match data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Failure to clear the global `rdev` pointer on an error path leads to an error pointer dereference on subsequent probe attempts.
- [High] Removing and re-probing cores out of order will corrupt the state of active cores and leak resources due to flawed `rdev->num_cores` indexing.
- [High] Resources allocated using `devm_*` APIs on the global dummy `drm_dev` are permanently leaked when cores are unbound or when initialization fails.
- [High] The global `rdev` pointer and `rdev->num_cores` state are accessed and modified without synchronization during concurrent probes.
- [High] The DRM device is exposed to userspace before core initialization completes, leading to a NULL pointer dereference on early open().
- [High] Asynchronous tasks and IRQs are not canceled/disabled before destroying the DRM scheduler.
--

commit 699632a47b8954e3644c77059cfdba18cb1c898b
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: select the per-core clock and reset counts from match data

This patch updates the RKNN block driver to read clock and reset counts from
per-SoC match data instead of using compile-time constants. This accommodates
SoCs like the RK3576 which have different hardware requirements than the RK3588.
The arrays sizing logic is also updated to use the dynamically provided counts.

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 8bbbce594883e..7ed64c131e9cb 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
> @@ -159,8 +159,22 @@ static const struct drm_driver rocket_drm_driver = {
>  
>  static int rocket_probe(struct platform_device *pdev)
>  {
> +	const struct rocket_soc_data *soc = of_device_get_match_data(&pdev->dev);
>  	int ret;
>  
> +	/*
> +	 * soc is dereferenced without a check by every one of its users, and
> +	 * rocket_core_init() below is the first of them. A device that bound
> +	 * by name rather than by compatible has no match data, so fail before
> +	 * anything is allocated rather than at the first dereference: the
> +	 * number of cores comes from a walk of matching DT nodes, and a device
> +	 * with no of_node was never counted by it.
> +	 */
> +	if (!soc) {
> +		dev_err(&pdev->dev, "no match data for this device\n");
> +		return -ENODEV;
> +	}
> +
>  	if (rdev == NULL) {
>  		/* First core probing, initialize DRM device. */
>  		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);

[Severity: High]
This isn't a bug introduced by this patch, but if rocket_device_init() fails
during the first core's probe, it returns an error pointer which is assigned
to the global rdev pointer:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
  ...
  if (IS_ERR(rdev)) {
    dev_err(&pdev->dev, "failed to initialize rocket device\n");
    return PTR_ERR(rdev);
  }
  ...
}

Could a subsequent probe attempt bypass the rdev == NULL check and dereference
the error pointer when accessing rdev->num_cores?

[Severity: High]
This is a pre-existing issue, but do devm_* resources allocated on the global
dummy drm_dev leak when cores are unbound?

drivers/accel/rocket/rocket_device.c:rocket_device_init() {
  ...
  rdev = devm_drm_dev_alloc(dev, rocket_drm_driver, struct rocket_device,
                            ddev);
  ...
}

When all cores are unbound, rocket_device_fini() unregisters the DRM device
and sets the global rdev to NULL. Since the global drm_dev is never destroyed,
does this permanently leak the struct rocket_device, core arrays, and mutexes?

[Severity: High]
This is a pre-existing issue, but are the global rdev pointer and
rdev->num_cores state protected against concurrent probes?

If multiple RKNN cores are probed concurrently via sysfs bind, could multiple
threads observe rdev == NULL simultaneously, leading to multiple
initializations of the DRM device and races when modifying the cores array?

[Severity: High]
This isn't a bug introduced by this patch, but is the DRM device exposed to
userspace before core initialization completes?

rocket_device_init() registers the DRM device via drm_dev_register(), making
it visible to userspace. If userspace opens the device immediately:

drivers/accel/rocket/rocket_drv.c:rocket_open() {
  ...
  rocket_priv->domain = rocket_iommu_domain_create(rdev->cores[0].dev);
  ...
}

Could this lead to a NULL pointer dereference in iommu_paging_domain_alloc()
since rocket_probe() hasn't populated rdev->cores[0].dev yet?

[ ... ]

> @@ -176,6 +190,7 @@ static int rocket_probe(struct platform_device *pdev)
>  
>  	rdev->cores[core].rdev = rdev;
>  	rdev->cores[core].dev = &pdev->dev;
> +	rdev->cores[core].soc = soc;
>  	rdev->cores[core].index = core;
>  
>  	rdev->num_cores++;

[Severity: High]
This is a pre-existing issue, but does using rdev->num_cores as an array
index corrupt the state of active cores when devices are removed and
re-probed out of order?

If core 0 is unbound via sysfs, rocket_remove() decrements rdev->num_cores
without shifting the rdev->cores[] array:

drivers/accel/rocket/rocket_drv.c:rocket_remove() {
  ...
  rdev->num_cores--;
  ...
}

A subsequent unbind of core 1 would fail because find_core_for_dev() shrinks
its search bounds to the new num_cores. If a new core is then probed, it uses
the decremented num_cores as the index:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
  ...
  unsigned int core = rdev->num_cores;
  ...
  rdev->cores[core].rdev = rdev;
  ...
}

Would this overwrite the active state of core 1?

[Severity: High]
This is a pre-existing issue, but are asynchronous tasks and IRQs canceled
before destroying the DRM scheduler?

In rocket_job_fini(), drm_sched_fini() is called to tear down the DRM
scheduler before canceling the background work:

drivers/accel/rocket/rocket_job.c:rocket_job_fini() {
  ...
  drm_sched_fini(&core->sched);
    
  cancel_work_sync(&core->reset.work);
  destroy_workqueue(core->reset.wq);
  ...
}

If the background reset.work executes concurrently, it calls
rocket_reset_work() which issues drm_sched_start() on a scheduler that is
already destroyed. Could this lead to a use-after-free or list corruption?

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

Re: [PATCH v12 03/14] accel/rocket: wait for a running IRQ handler before resetting a core

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardirq handler reads hardware registers on a potentially suspended device without checking if it is active.
- [High] Missing `synchronize_irq()` in runtime suspend callback allows powering down hardware while an IRQ handler is mid-flight.
- [High] The threaded IRQ handler accesses hardware registers without verifying the device is awake.
- [High] Leaking an `iommu_group` reference upon job completion.
- [High] Calling `drm_sched_fini()` on the error path of a failed `drm_sched_init()` causes a NULL pointer dereference.
--

commit f451c258aa3279d254b112c34e76371c476fb507
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: wait for a running IRQ handler before resetting a core

rocket_reset() calls drm_sched_stop(), which stops the scheduler and
returns. It does not wait for a threaded handler that is already running,
so the comment that follows, "Remaining interrupts have been handled",
states an assumption rather than something the code arranges.

Link: https://lore.kernel.org/all/20260819073530.6087-1-royalnet026@gmail.com/
Link: https://lore.kernel.org/all/CAEWPSH5mxTbUkNouxm6yecMZYvDowquhvYvhaXQ8HoMtHD5U1g@mail.gmail.com/

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 575945015ca3b..0be8db391989b 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c

[ ... ]

drivers/accel/rocket/rocket_job.c:rocket_job_irq_handler() {
	struct rocket_core *core = data;
	u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);

[Severity: High]
This is a pre-existing issue, but does this unconditional register read
crash the system if the device is suspended when the shared interrupt fires?

The driver registers its hardware IRQ with IRQF_SHARED (shared with the
IOMMU). If the shared interrupt fires while the rocket device is
runtime-suspended, rocket_job_irq_handler() immediately executes
rocket_pc_readl() without checking if it is active. Accessing registers
while the device's clocks are disabled triggers an SError kernel panic.

drivers/accel/rocket/rocket_drv.c:rocket_device_runtime_suspend() {
	...
	clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks),
				   rdev->cores[core].clks);

	return 0;
}

[Severity: High]
This is a pre-existing issue, but does missing synchronize_irq() allow
powering down hardware while an IRQ handler is mid-flight?

The rocket_device_runtime_suspend() callback disables clocks via
clk_bulk_disable_unprepare() without prior synchronization of the shared IRQ
line. If an interrupt fires just before this callback, the handler may run
concurrently with the suspend sequence, disabling hardware clocks while the
IRQ handler is still executing, leading to SError kernel panics.

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
	pm_runtime_mark_last_busy(core->dev);

	scoped_guard(mutex, &core->job_lock) {
		/*
		 * Stopping the block belongs under the lock. hw_submit() writes
		 * OPERATION_ENABLE too, and outside the lock this zero can land
		 * after that one and stop a task that has only just started.
		 */
		rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
		rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);

[Severity: High]
This is a pre-existing issue, but does the threaded IRQ handler access
hardware registers without verifying the device is awake?

If the threaded handler is awakened (e.g., if the hardirq reads 0xffffffff
from powered-off hardware, which evaluates as true for DPU interrupt bits),
it unconditionally writes to OPERATION_ENABLE and INTERRUPT_CLEAR. Since it
does not call pm_runtime_get_if_active(), these writes will cause an SError
if the device is suspended.

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
			...
			iommu_detach_group(NULL, iommu_group_get(core->dev));

[Severity: High]
This is a pre-existing issue, but does this leak an iommu_group reference
upon job completion?

iommu_group_get() increments the reference count of the group, but
iommu_detach_group() does not consume it. Since there is no corresponding
iommu_group_put() called, this causes a persistent kobject refcount leak
that eventually overflows.

drivers/accel/rocket/rocket_job.c:rocket_job_init() {
	...
err_sched:
	drm_sched_fini(&core->sched);

	destroy_workqueue(core->reset.wq);
	return ret;
}

[Severity: High]
This is a pre-existing issue, but does calling drm_sched_fini() on the error
path of a failed drm_sched_init() cause a NULL pointer dereference?

If drm_sched_init() fails, it cleans up internally, leaving sched->sched_rq
as NULL but sched->num_rqs > 0. The driver's err_sched path unconditionally
calls drm_sched_fini(), which iterates up to num_rqs and attempts to kfree
sched->sched_rq[i], dereferencing the NULL sched_rq pointer array.

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

Re: [PATCH v12 09/14] pmdomain: rockchip: add optional per-domain power-on settle delay

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Forcing the NPU parent domain off at probe bypasses the idle sequence for its child domains, risking AXI bus hangs, and can cause pm_genpd_add_subdomain() to fail.
- [Medium] Hijacking the need_regulator flag to force a hardware reset at probe is an API abuse that generates intentional, false kernel warnings on unaffected boards.

Pre-existing issues:
- [High] Error cleanup path fails to unlink subdomains, causing pm_genpd_remove() to fail and leading to a Use-After-Free of devres-managed memory.
--

commit c3ca2f4bcfcc96e94f4eccdb9b209d7e8e746177
Author: Jiaxing Hu <gahing@gahingwoo.com>

pmdomain: rockchip: add optional per-domain power-on settle delay

This patch introduces an optional delay_us to rockchip_domain_info, allowing
domains like the RK3576 NPU to settle before QoS register restoration. It also
adds a DOMAIN_RK3576_R macro to pass the need_regulator flag, applying it to
RK3576_PD_NPU to force a power-off at probe on boards without a domain-supply.

> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index ba66ae7194289..39988efd86aaa 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c
> @@ -258,8 +262,11 @@ struct rockchip_pmu {
>  #define DOMAIN_RK3568(name, pwr, req, wakeup, regulator)		\
>  	DOMAIN_M_R(name, pwr, pwr, req, req, req, wakeup, regulator)
>  
> -#define DOMAIN_RK3576(name, p_offset, pwr, status, r_status, r_offset, req, idle, g_mask, wakeup)	\
> -	DOMAIN_M_O_R_G(name, p_offset, pwr, status, 0, r_status, r_status, r_offset, req, idle, idle, g_mask, wakeup)
> +#define DOMAIN_RK3576(name, p_offset, pwr, status, r_status, r_offset, req, idle, g_mask, delay, wakeup)	\
> +	DOMAIN_M_O_R_G_W(name, p_offset, pwr, status, 0, r_status, r_status, r_offset, req, idle, idle, g_mask, delay, wakeup, false)
> +
> +#define DOMAIN_RK3576_R(name, p_offset, pwr, status, r_status, r_offset, req, idle, g_mask, delay, wakeup)	\
> +	DOMAIN_M_O_R_G_W(name, p_offset, pwr, status, 0, r_status, r_status, r_offset, req, idle, idle, g_mask, delay, wakeup, true)

[Severity: Medium]
Is hijacking the need_regulator flag here to force a hardware reset at probe
an abuse of the API? 

Setting this flag on a domain that doesn't physically have a regulator forces
the regulator core to issue a warning. As the commit message mentions, 12 out
of 13 boards do not describe a domain-supply, which means they will fallback 
to a dummy regulator and print a dev_warn on otherwise healthy systems. 

Could a dedicated boolean flag be used instead to force a probe-time
power-off?

[ ... ]

> @@ -1300,25 +1311,26 @@ static const struct rockchip_domain_info rk3568_pm_domains[] = {
>  };
>  
>  static const struct rockchip_domain_info rk3576_pm_domains[] = {
> -	[RK3576_PD_NPU]		= DOMAIN_RK3576("npu",    0x0, BIT(0),  BIT(0), 0,       0x0, 0,       0,       0,       false),
[ ... ]
> +	/*                                            name    p_offset pwr      status  r_status r_offset req      idle     g_mask   delay wakeup */
> +	[RK3576_PD_NPU]		= DOMAIN_RK3576_R("npu",  0x0, BIT(0),  BIT(0), 0,       0x0, 0,       0,       0,       0,    false),

[Severity: High]
Does forcing the NPU parent domain off at probe bypass the idle sequence for
its child domains?

If the bootloader leaves the NPU powered on, rockchip_pm_add_one_domain()
forces this parent domain off because need_regulator is true:

    if (pd->info->need_regulator)
        rockchip_pd_power(pd, false);
        
Because the parent RK3576_PD_NPU lacks an idle_mask (set to 0 here), power is
cut abruptly without idling its subdomains (NPUTOP, NPU0, NPU1), which risks
AXI bus hangs. 

Furthermore, if the children's hardware status bits do not instantly clear,
they will be registered as ON, and pm_genpd_add_subdomain() will fail with
-EINVAL because it refuses to link an ON child to an OFF parent.

[Severity: High]
This is a pre-existing issue, but if the probe fails (such as from the
pm_genpd_add_subdomain() failure mentioned above), does the error cleanup
path cause a use-after-free?

In rockchip_pm_domain_cleanup(), the code iteratively attempts to remove
domains but doesn't unlink subdomains first:

    rockchip_pm_remove_one_domain(pd);

Which then calls:

    ret = pm_genpd_remove(&pd->genpd);

pm_genpd_remove() detects the active child links on the parent and returns
-EBUSY, failing to remove the parent domain from the global gpd_list.
Devres then frees the struct rockchip_pm_domain memory, leaving dangling
pointers in the core genpd list.

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

Re: [PATCH v12 12/14] accel/rocket: add RK3576 NPU (RKNN) support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The driver improperly tracks active RKNN cores using a sequential counter (`rdev->num_cores`) to index into the fixed-size `rdev->cores` array. Unbinding a core decrements the counter without shifting array elements, causing subsequent binds to overwrite active core structures and leading to skipped cleanup (use-after-free) and driver state corruption.
- [High] 64-bit IOVA task->regcmd is silently truncated to 32 bits, despite 40-bit DMA mask.
--

commit f9d20327e89caf3b9bf4ada2ba21cb2dc6201353
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: add RK3576 NPU (RKNN) support

This commit adds support for the RK3576 NPU by introducing new SoC-specific
definitions, clock/reset handling, PM domains, and adapting hardware task
control structures.

Link: https://lore.kernel.org/all/4f300b78-d96d-4d98-8819-dc292b0c9b97@rock-chips.com/

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
[ ... ]
> @@ -231,13 +231,23 @@ static void rocket_remove(struct platform_device *pdev)
>  static const struct rocket_soc_data rk3588_soc_data = {
>  	.num_clks = 4,
>  	.num_resets = 2,
> +	.multi_power_domain = false,
> +	.task_con_16bit = false,
>  };
>  
> -static const struct of_device_id dt_match[] = {
> +static const struct rocket_soc_data rk3576_soc_data = {
> +	.num_clks = 6,
> +	.num_resets = 1,
> +	.multi_power_domain = true,
> +	.task_con_16bit = true,
> +};
> +
> +const struct of_device_id rocket_dt_match[] = {
>  	{ .compatible = "rockchip,rk3588-rknn-core", .data = &rk3588_soc_data },
> +	{ .compatible = "rockchip,rk3576-rknn-core", .data = &rk3576_soc_data },
>  	{}
>  };
> -MODULE_DEVICE_TABLE(of, dt_match);
> +MODULE_DEVICE_TABLE(of, rocket_dt_match);

[Severity: Critical]
This isn't a bug introduced by this patch, but the way the driver tracks
active RKNN cores using rdev->num_cores appears to have a flaw that can
cause state corruption and a use-after-free.

If multiple cores are probed, and an intermediate core is unbound,
rocket_remove() decrements the counter without shifting the remaining
elements in the rdev->cores array to fill the gap:

drivers/accel/rocket/rocket_drv.c:rocket_remove() {
    ...
    rocket_core_fini(&rdev->cores[core]);
    rdev->num_cores--;
    ...
}

If another core is then probed, rocket_probe() allocates a slot based on the
decremented num_cores, causing the newly bound core to overwrite the
existing, active core structure:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
    ...
    unsigned int core = rdev->num_cores;
    dev_set_drvdata(&pdev->dev, rdev);
    rdev->cores[core].rdev = rdev;
    ...
}

When find_core_for_dev() is later called to unbind the overwritten core, it
fails to find it, which aborts rocket_remove() and leaks resources like the
running DRM scheduler thread. Eventually, when the driver is unloaded, the
memory containing the active thread is freed, leading to a use-after-free
crash.

Could we fix the core array management to handle unbinding properly?

[ ... ]
> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
[ ... ]
> @@ -142,10 +166,17 @@ static void rocket_job_hw_submit(struct rocket_core *core, struct rocket_job *jo
>  	rocket_pc_writel(core, INTERRUPT_MASK, PC_INTERRUPT_MASK_DPU_0 | PC_INTERRUPT_MASK_DPU_1);
>  	rocket_pc_writel(core, INTERRUPT_CLEAR, PC_INTERRUPT_CLEAR_DPU_0 | PC_INTERRUPT_CLEAR_DPU_1);
>  
> -	rocket_pc_writel(core, TASK_CON, PC_TASK_CON_RESERVED_0(1) |
> -					 PC_TASK_CON_TASK_COUNT_CLEAR(1) |
> -					 PC_TASK_CON_TASK_NUMBER(1) |
> -					 PC_TASK_CON_TASK_PP_EN(1));
> +	if (core->soc->task_con_16bit)
> +		rocket_pc_writel(core, TASK_CON,
> +				 RK3576_PC_TASK_CON_LAST_LAYER_CLEAR |
> +				 RK3576_PC_TASK_CON_COUNT_CLEAR |
> +				 RK3576_PC_TASK_CON_PP_EN |
> +				 RK3576_PC_TASK_CON_TASK_NUMBER(1));
> +	else
> +		rocket_pc_writel(core, TASK_CON, PC_TASK_CON_RESERVED_0(1) |
> +						 PC_TASK_CON_TASK_COUNT_CLEAR(1) |
> +						 PC_TASK_CON_TASK_NUMBER(1) |
> +						 PC_TASK_CON_TASK_PP_EN(1));

[Severity: High]
This is a pre-existing issue, but I noticed there is a potential 32-bit
truncation of a 64-bit IOVA in the hardware submission logic just above this
code in rocket_job_hw_submit().

The driver configures a 40-bit DMA mask in rocket_core_init():

drivers/accel/rocket/rocket_core.c:rocket_core_init() {
    ...
    err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(40));
    ...
}

However, the UAPI struct restricts the IOVA to 32 bits:

include/uapi/drm/rocket_accel.h:drm_rocket_task {
    ...
    __u32 regcmd;
    ...
}

When userspace populates drm_rocket_task.regcmd, the __u32 type silently
truncates addresses greater than 4GB. The kernel then writes this truncated 
address to the 32-bit BASE_ADDRESS hardware register:

drivers/accel/rocket/rocket_job.c:rocket_job_hw_submit() {
    ...
    rocket_pc_writel(core, BASE_ADDRESS, task->regcmd);
    ...
}

Can this lead to IOMMU translation faults, device hangs, or memory corruption
if an unprivileged userspace process allocates enough memory to receive an
IOVA greater than 4GB?

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

Re: [PATCH v12 03/14] accel/rocket: wait for a running IRQ handler before resetting a core

[Igor Paunovic]

Hi Jiaxing,

While re-running the 19 August protocol on v12 as posted today, I
found an error in my own reports that this commit message now
carries. It is mine to correct, and it also corrects yesterday's mail
[1]. What falls from that mail: the 53-versus-49 bound, the sentence
that 19 August showed no manifestation on either arm, and the closing
suggestion that both statements can stand in the commit message. What
stands: the provenance, 45 resets on 19 August on v8 1-2/12 and 102
on 25 August on v9, and the caveat that this protocol bounds and does
not prove.

My script kept the scorer output of every inference per round and
never aggregated it; my summaries scored only the one inference after
the forced autosuspend. Aggregating the per-round files now, the
constant-0x80 result is in the rounds of nearly every run, on every
arm, on all three dates (resets per run, then rounds at 0x80):

  19 Aug  v8 1+2        12 + 8            2 + 1
  19 Aug  base only     12 + 13           2 + 3
  25 Aug  v9 1+2        12 + 11           6 + 1
  25 Aug  base only     8/10/12/8/15      2/0/1/1/2  (+ the one after
                                                     suspend)
  25 Aug  v9 1+2+3      13 + 13           2 + 2
  12 Sep  v12 2+3       10 + 11           2 + 0
  12 Sep  v12 2+3+4     9/5/12/13/14      2/0/2/1/1  (+ the one after
                                                     suspend, twice)

Today's seven runs were two arms only, no unpatched arm, so nothing
today re-tests the differential. Same board and base as before,
PROVE_LOCKING and DEBUG_ATOMIC_SLEEP on, serial console captured on a
second machine for the whole session.

So "no manifestation on either arm, oracle 48/48 throughout" on 19
August and "every inference matched" on 25 August were both wrong
for the in-round inferences, and the 0x80 buffer is not a
differential signal. It is what a job cancelled by the reset looks
like from userspace in this protocol, whatever made the job miss its
deadline, so it cannot tell the races 2/14 and 3/14 close from an
ordinary induced timeout.

The mechanism, from the code: rocket_reset() calls drm_sched_stop(),
which detaches the hardware fence of every pending job that has not
completed; rocket_core_reset() kills the block; drm_sched_start(sched,
0) then completes those jobs through drm_sched_job_done(job,
-ECANCELED). That finished fence is the one on the output BO's
reservation. The only wait the rocket uAPI offers is
DRM_IOCTL_ROCKET_PREP_BO, and rocket_gem.c maps any positive return
of dma_resv_wait_timeout() to 0, error or not, so teflon reads an
output buffer that was never written, which its output conversion
turns into 0x80 (mesa rkt_ml.c, output + 0x80), exactly as you
described for RK3576. With JOB_TIMEOUT_MS=2 the timeout fires on
about half the inferences (74 timeouts over the 147 inferences run
today, seven runs of twenty-one; 14 of the 21 in the traced run
below); whether the reset or the completion wins that race decides
the outcome, on every arm alike.

A direct witness, one run on v12 2+3+4 today with a kprobe on
drm_sched_fence_finished(): exactly two completions in the whole run
carried result -125 (-ECANCELED), and exactly two inferences came
back all-0x80, round 11 and the post-suspend one. The two
cancellations are 5.3062 s apart and the two scorer files 5.3057 s
apart, and across all twenty-one inferences of the run the gap
between a traced completion and the scorer file it produced is
constant to within 2 ms, against a round period of 280 ms, so each
cancellation falls unambiguously in the round that came back at
0x80. In the other runs I have only the scorer output, which took
exactly two values today (all 48 channels within 1 of the CPU
reference, or all-0x80, nothing in between), so there the
identification of the 0x80 results as cancelled jobs is inference,
not observation.

For this commit message I would drop the two paragraphs that cite my
runs as evidence for the race: the one beginning "Igor also ran a
differential on RK3588" and the one beginning "His own bound on it is
the right one", and put this in their place: 45 induced resets on 19
August, 102 on 25 August and 74 today, every reset recovered, no MMU
faults, no lockdep report from rocket or the scheduler in the runs
where lockdep was still armed, and of the 420 inferences scored, 384
matched the CPU reference within 1 on all 48 output channels while 36
returned the all-0x80 buffer of a job the reset had cancelled. Please
keep both Link: lines and add this message as a third, so anyone
following the 19 and 25 August reports lands on the correction too.

Two things I should have said before: all of those resets landed on
core 0 (fdab0000), the other two cores being bound but idle in this
single-client protocol; and two of today's five 2+3+4 runs come from
a boot that had an unrelated lockdep splat in the DP driver at probe
time, before the test, so they carry no PROVE_LOCKING cover. The
Tested-by lines on 2/14, 3/14 and 4/14 stand for that and only that.
On 3/14 please drop "differential base" from my tag comment, which
then reads exactly like the one on 4/14; and on 2/14, where the
comment is only "# RK3588, three cores", please give it the same
comment as 4/14, since the lowered timeout belongs on every tag that
came out of this protocol.

One question this leaves, mostly for Tomeu: there is no out-fence or
status field in the rocket uAPI, and PREP_BO drops the fence error,
so a job cancelled by a reset is indistinguishable from one that ran
(rocket_job_run() reads the error, but only to return NULL instead of
executing the job). Is that intended?

The artifacts of all twenty runs (scorer output per round, runtime
and genpd state, journal, serial log, today's kprobe trace) are
preserved if you or Tomeu want them.

[1] https://lore.kernel.org/all/20260911192833.105634-1-royalnet026@gmail.com/

Regards,
Igor