* [PATCH v2 01/11] accel/rocket: search every core slot when a core is removed
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 02/11] accel/rocket: number the cores by devicetree position, not bind order Igor Paunovic
` (9 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
rocket_remove() decrements rdev->num_cores for each core it removes,
while find_core_for_dev() searches slots 0 to num_cores - 1. Unbinding
the cores in the order they were bound therefore loses the last one: by
the time it is removed the search range has already shrunk past its
slot, so find_core_for_dev() returns -1 and rocket_remove() gives up
without doing anything.
num_cores never reaches zero, rocket_device_fini() never runs, and the
file-scoped rdev keeps pointing at a device that is going away. Binding
the cores again starts from that stale count, because rocket_probe()
takes rdev->num_cores as the slot to fill. On an RK3588, which describes
three cores, the second round lands on slots 1, 2 and 3 while rdev->cores
was allocated with room for three:
rocket fdab0000.npu: Rockchip NPU core 1 version: 1179210309
rocket fdac0000.npu: Rockchip NPU core 2 version: 1179210309
rocket fdad0000.npu: Rockchip NPU core 3 version: 1179210309
The write to rdev->cores[3] is past the end of the array.
Nothing in tree reads the core array often enough to notice, so the
overrun is silent today. It turned up while testing a devfreq series on
top of this, where a worker walks every core a few times a second, and
UBSAN caught the first bool it read out of the overrun entry:
UBSAN: invalid-load in drivers/accel/rocket/rocket_devfreq.c:47:10
load of value 5 is not a valid value for type '_Bool'
Workqueue: devfreq_wq devfreq_monitor
Record how many slots were allocated and search all of them. Every core
is then found on removal, num_cores reaches zero, the device is torn down
and a later bind starts from a clean rdev.
This does not make unbinding a single core out of several work: probe
still takes num_cores as the slot to fill, and rocket_open() still
reaches for cores[0] whether or not anything is there. The fourth patch
takes care of both.
Found by unbinding and rebinding all three cores on an Orange Pi 5 Plus.
With this applied the cores land in slots 0, 1 and 2 every time, whichever
order they are bound in, and the shared supply goes back to a single user
after each round.
Fixes: ed98261b4168 ("accel/rocket: Add a new driver for Rockchip's NPU")
Cc: stable@vger.kernel.org
Assisted-by: LLM sparse checkpatch
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Tested-by: Jiaxing Hu <gahing@gahingwoo.com> # RK3576, two cores
Tested-by: Sidong Yang <sidong.yang@furiosa.ai> # RK3588, 3 cores
---
Two paragraphs changed from the standalone posting, which this series
supersedes: the one on unbinding a single core now points at patch 4
instead of saying what is left, and the round counts are dropped from
the last one. Posting:
https://lore.kernel.org/r/20260904125936.26234-1-royalnet026@gmail.com
The two Tested-by tags are from that thread. The round counts were
taken on a kernel that carried the use-after-free fixed in 8/11; they
were re-measured with the whole series applied, see the notes on patch
4 and the cover letter. The single-core unbind and rocket_open() cases
in that paragraph are 2/2 of the lifecycle series, now rebased into
this one as patch 4:
https://lore.kernel.org/r/20260731064933.12548-3-royalnet026@gmail.com
drivers/accel/rocket/rocket_device.c | 2 ++
drivers/accel/rocket/rocket_device.h | 1 +
drivers/accel/rocket/rocket_drv.c | 2 +-
3 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c
index 46e6ee1e72c5f..efd004194c1af 100644
--- a/drivers/accel/rocket/rocket_device.c
+++ b/drivers/accel/rocket/rocket_device.c
@@ -31,6 +31,8 @@ struct rocket_device *rocket_device_init(struct platform_device *pdev,
if (of_device_is_available(core_node))
num_cores++;
+ rdev->max_cores = num_cores;
+
rdev->cores = devm_kcalloc(dev, num_cores, sizeof(*rdev->cores), GFP_KERNEL);
if (!rdev->cores)
return ERR_PTR(-ENOMEM);
diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/rocket_device.h
index ce662abc01d3d..c62d567010696 100644
--- a/drivers/accel/rocket/rocket_device.h
+++ b/drivers/accel/rocket/rocket_device.h
@@ -19,6 +19,7 @@ struct rocket_device {
struct rocket_core *cores;
unsigned int num_cores;
+ unsigned int max_cores;
};
struct rocket_device *rocket_device_init(struct platform_device *pdev,
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 8bbbce594883e..2bcfe4ab3c68f 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -223,7 +223,7 @@ static int find_core_for_dev(struct device *dev)
{
struct rocket_device *rdev = dev_get_drvdata(dev);
- for (unsigned int core = 0; core < rdev->num_cores; core++) {
+ for (unsigned int core = 0; core < rdev->max_cores; core++) {
if (dev == rdev->cores[core].dev)
return core;
}
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 02/11] accel/rocket: number the cores by devicetree position, not bind order
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 01/11] accel/rocket: search every core slot when a core is removed Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 03/11] accel/rocket: search every core slot when looking up a scheduler Igor Paunovic
` (8 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
rocket_job_hw_submit() programs the S_POINTER registers of a core with an
extra bit derived from core->index, the way the vendor driver derives it
from the hardware number of the core. rocket_probe() sets core->index to
the slot the core takes in rdev->cores[], which is the order the cores
bind in.
The two agree only while the cores that bind are a prefix of the core
nodes in the devicetree, in devicetree order. Unbind them and bind them
back with a different core first, have one core's probe deferred behind a
sibling's, or disable a core other than the last one, and every task
submitted to a core whose slot is not its hardware number times out after
500 ms. The reset that follows does not help, and the inference finishes
with wrong output.
Observed on an Orange Pi 5 Plus with a KASAN build, over all six bind
orders of the three cores: only the devicetree order ran clean. The other
five produced 27 to 141 "NPU job timed out". In four of them no output
tensor changed with the input, and the harness gave up before its first
measured round; the fifth got through a six-second run with 27 timeouts
and a wrong top-1 class. All but one of the timeouts land on the cores
whose slot is not their hardware number, in proportion to the tasks the
scheduler hands them, and in both directions of the mismatch.
Number the cores by their position among the core nodes in the devicetree
instead, which is what the hardware number is.
The wrong value has been assigned since the driver was added, but it only
reached the hardware once the extra bit was introduced, hence the Fixes
tag below.
Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Cc: stable@vger.kernel.org
Assisted-by: LLM sparse checkpatch
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
Supersedes the standalone posting:
https://lore.kernel.org/r/20260905135612.7324-1-royalnet026@gmail.com
Same diff. The message now says the numbers come from a KASAN build,
corrects the timeout range to 27-141 against the raw log (it said 140),
says what the four failed orders showed (the output did not change with
the input; it said an oracle rejected the output), notes the one timeout
that landed on a matching core, and drops the throughput figure, which
was measured under KASAN.
drivers/accel/rocket/rocket_core.h | 5 +++++
drivers/accel/rocket/rocket_drv.c | 31 +++++++++++++++++++++++++++++-
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/accel/rocket/rocket_core.h b/drivers/accel/rocket/rocket_core.h
index f6d7382854ca9..46ed8352a79d2 100644
--- a/drivers/accel/rocket/rocket_core.h
+++ b/drivers/accel/rocket/rocket_core.h
@@ -30,6 +30,11 @@
struct rocket_core {
struct device *dev;
struct rocket_device *rdev;
+ /*
+ * Hardware number of the core: its position among the core nodes in
+ * the devicetree. Not an index into rdev->cores[] - that slot is what
+ * find_core_for_dev() returns.
+ */
unsigned int index;
int irq;
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 2bcfe4ab3c68f..7d927bb6b322d 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -157,10 +157,39 @@ static const struct drm_driver rocket_drm_driver = {
.desc = "rocket DRM",
};
+/*
+ * The extra bit that rocket_job_hw_submit() sets in the S_POINTER registers
+ * is the hardware number of the core, which is its position among the core
+ * nodes in the devicetree: a disabled core keeps its number. The slot a core
+ * takes in rdev->cores[] is the order the cores happened to bind in, and the
+ * two only agree while the cores that bind are a prefix of those nodes, in
+ * devicetree order. Every task submitted to a core whose slot is not its
+ * hardware number then times out.
+ */
+static int rocket_core_hw_index(struct device *dev)
+{
+ struct device_node *np;
+ int index = 0;
+
+ for_each_matching_node(np, dev->driver->of_match_table) {
+ if (np == dev->of_node) {
+ of_node_put(np);
+ return index;
+ }
+ index++;
+ }
+
+ return -ENODEV;
+}
+
static int rocket_probe(struct platform_device *pdev)
{
+ int index = rocket_core_hw_index(&pdev->dev);
int ret;
+ if (index < 0)
+ return index;
+
if (rdev == NULL) {
/* First core probing, initialize DRM device. */
rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
@@ -176,7 +205,7 @@ static int rocket_probe(struct platform_device *pdev)
rdev->cores[core].rdev = rdev;
rdev->cores[core].dev = &pdev->dev;
- rdev->cores[core].index = core;
+ rdev->cores[core].index = index;
rdev->num_cores++;
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 03/11] accel/rocket: search every core slot when looking up a scheduler
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 01/11] accel/rocket: search every core slot when a core is removed Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 02/11] accel/rocket: number the cores by devicetree position, not bind order Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 04/11] accel/rocket: keep core slots stable across unbind and rebind Igor Paunovic
` (7 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
sched_to_core() walks rdev->cores[] up to rdev->num_cores, and
rocket_remove() decrements num_cores for every core it removes. Unbind a
core that is not the last one and the cores behind it fall outside the
search, so sched_to_core() returns NULL for a core that is still bound and
still running jobs. Neither caller checks the result:
rocket_job_run(): rocket_fence_create(core), core->dev
rocket_job_timedout(): dev_err(core->dev, "NPU job timed out")
Unbinding the middle core of the three on an RK3588 while three clients are
submitting to all of them faults twice, once from the surviving core's
job queue and once from its reset work:
KASAN: null-ptr-deref in range [0x0000000000000220-0x0000000000000227]
Workqueue: fdad0000.npu drm_sched_run_job_work [gpu_sched]
pc : rocket_job_run+0x234/0x838 [rocket]
Call trace:
rocket_job_run+0x234/0x838 [rocket]
drm_sched_run_job_work+0x2cc/0xad8 [gpu_sched]
process_one_work+0x640/0x14f0
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
Workqueue: rocket-reset-2 drm_sched_job_timedout [gpu_sched]
pc : rocket_job_timedout+0xf0/0x1e0 [rocket]
Call trace:
rocket_job_timedout+0xf0/0x1e0 [rocket]
drm_sched_job_timedout+0x188/0x6a0 [gpu_sched]
Both are the third core: the workqueue names are its device and its
core->index, and it was left at slot 2 while num_cores had dropped to 2.
Search all the slots that were allocated, the way find_core_for_dev() now
does. A core that is still bound is then found, and the two callers get
the pointer they already assume they have.
This does not make unbinding one core out of several safe. An open client
keeps an entity pointing at the scheduler of the core that went away:
drm_sched reports it as not ready for every job that lands on it, and the
client waits in dma_fence_default_wait for a fence that will never signal.
Stopping the NULL dereference is what belongs in a fix; the rest wants
more thought.
Reported-by: Sidong Yang <sidong.yang@furiosa.ai>
Closes: https://lore.kernel.org/dri-devel/apwUewaRnoTNXHCt@rock-5b-plus/
Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Cc: stable@vger.kernel.org
Assisted-by: LLM sparse checkpatch
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
Unchanged from the standalone posting, which this series supersedes:
https://lore.kernel.org/r/20260905150432.7477-1-royalnet026@gmail.com
drivers/accel/rocket/rocket_job.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index f404355058185..4bc4f9c8ee403 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -283,7 +283,7 @@ static struct rocket_core *sched_to_core(struct rocket_device *rdev,
{
unsigned int core;
- for (core = 0; core < rdev->num_cores; core++) {
+ for (core = 0; core < rdev->max_cores; core++) {
if (&rdev->cores[core].sched == sched)
return &rdev->cores[core];
}
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 04/11] accel/rocket: keep core slots stable across unbind and rebind
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
` (2 preceding siblings ...)
2026-09-22 8:01 ` [PATCH v2 03/11] accel/rocket: search every core slot when looking up a scheduler Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 05/11] accel/rocket: request the core clocks by name Igor Paunovic
` (6 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
rocket_probe() inserts a new core at slot num_cores, and
rocket_remove() only decrements that counter without clearing the
slot. That falls apart as soon as one core is unbound while its
siblings stay bound:
- the next bind reuses the slot of a still-live core and overwrites
it while its IRQ handler (dev_id points into cores[]) and its
DRM scheduler are still active;
- rocket_open() unconditionally uses cores[0].dev, which after an
unbind of core 0 is a stale pointer to an unbound device.
On an RK3588 with three cores, unbinding the first one and binding it
again puts it on top of the third:
rocket fdab0000.npu: drm_sched_init: scheduler already initialized!
One device now sits in two slots and the third core in none. The next
unbind of the first core finds its stale slot, torn down already, and
finishes the same scheduler a second time:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
pc : drm_sched_fini+0x4c/0x1e0 [gpu_sched]
Call trace:
drm_sched_fini+0x4c/0x1e0 [gpu_sched] (P)
rocket_job_fini+0x28/0x60 [rocket]
rocket_core_fini+0x4c/0x78 [rocket]
rocket_remove+0x78/0x110 [rocket]
platform_remove+0x2c/0x68
device_remove+0x58/0xc0
device_release_driver_internal+0x214/0x2e0
device_driver_detach+0x24/0x50
unbind_store+0xd8/0xe8
Make .dev the slot-liveness marker: probe takes the first free slot
and clears it again if core init fails, remove clears .dev after
rocket_core_fini() and warns if the core cannot be found, lookups skip
empty slots, and rocket_open() and rocket_job_open() use only live
slots. num_cores keeps counting bound cores for the last-core teardown
check. A missing core is no reason to refuse a new file: the device is
one core short, not gone, and any bound core will do for the IOMMU
domain, which is attached to the group of whichever core runs a job.
With a single core live the scheduler list that drm_sched_entity_init()
does not keep is freed at once, as rocket_job_close() only frees what
the entity kept.
Fixes: ed98261b4168 ("accel/rocket: Add a new driver for Rockchip's NPU")
Cc: stable@vger.kernel.org
Assisted-by: LLM sparse checkpatch
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
v3, now in this series:
- rebased onto the three fixes before it: "accel/rocket: search
every core slot when a core is removed", which provides max_cores,
"accel/rocket: number the cores by devicetree position, not bind
order", so a slot no longer doubles as the hardware number, and
"accel/rocket: search every core slot when looking up a scheduler"
- the crash above: this is what the devfreq patches ran into when
tested on a kernel without this one
- free the scheduler list when a single core is live
- Cc: stable
v2: https://lore.kernel.org/r/20260731064933.12548-3-royalnet026@gmail.com
- also clear the slot's .dev when rocket_core_init() fails (Jiaxing Hu)
- check .dev in sched_to_core() (Jiaxing Hu)
- document the synchronous-probe assumption at the slot scan
v1: https://lore.kernel.org/dri-devel/20260730080355.177422-3-royalnet026@gmail.com/
Unbinding a core that still has jobs in flight, or that an open file
already holds the scheduler of, has further pre-existing issues that
are out of scope for this bookkeeping fix. One of them, a use-after-free
under KASAN, is described in the cover letter. The driver does not
serialize probe and remove against open; this does not change that.
For stable, this goes with the three patches before it.
Verified on RK3588 (Orange Pi 5 Plus), 7.3.0-rc2 drm-misc-next plus
this series, in-tree rocket, all three cores enabled: 25 rounds of
unbinding and rebinding all three cores; 4 rounds of unbinding a single
core (twice the devicetree-first core, twice the second one), each with
an inference run while the core was absent and one more after all four;
5 rmmod/modprobe rounds; 3 unbind/rebind rounds and 1 rmmod with the
clock raised to the 1 GHz OPP (CRU selector on the PVTPLL before
each). No "scheduler already initialized" message and no
oops; the regulator user count returns to its boot value after every
round. Without this patch the same single-core round oopses in
drm_sched_fini() on the second unbind, as shown above.
The single-core, full, rmmod and raised-clock rounds were repeated
(10 full rounds and 3 rmmod rounds this time) on a KASAN and
PROVE_LOCKING build of the same tree: no report, and lockdep still
enabled afterwards.
drivers/accel/rocket/rocket_device.h | 2 +
drivers/accel/rocket/rocket_drv.c | 55 +++++++++++++++++++++++-----
drivers/accel/rocket/rocket_job.c | 38 ++++++++++++++-----
3 files changed, 77 insertions(+), 18 deletions(-)
diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/rocket_device.h
index c62d567010696..abb88a254e569 100644
--- a/drivers/accel/rocket/rocket_device.h
+++ b/drivers/accel/rocket/rocket_device.h
@@ -18,7 +18,9 @@ struct rocket_device {
struct mutex sched_lock;
struct rocket_core *cores;
+ /* Number of currently bound cores. */
unsigned int num_cores;
+ /* Slot capacity (DT core count); slots with a NULL .dev are free. */
unsigned int max_cores;
};
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 7d927bb6b322d..b9b36c578db20 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -68,11 +68,21 @@ rocket_iommu_domain_put(struct rocket_iommu_domain *domain)
kref_put(&domain->kref, rocket_iommu_domain_destroy);
}
+static struct rocket_core *rocket_first_live_core(struct rocket_device *rdev)
+{
+ for (unsigned int core = 0; core < rdev->max_cores; core++)
+ if (rdev->cores[core].dev)
+ return &rdev->cores[core];
+
+ return NULL;
+}
+
static int
rocket_open(struct drm_device *dev, struct drm_file *file)
{
struct rocket_device *rdev = to_rocket_device(dev);
struct rocket_file_priv *rocket_priv;
+ struct rocket_core *core;
u64 start, end;
int ret;
@@ -85,8 +95,18 @@ rocket_open(struct drm_device *dev, struct drm_file *file)
goto err_put_mod;
}
+ /*
+ * Any bound core will do for the domain: it is attached to the group
+ * of whichever core runs a job, and the NPU IOMMUs are all the same.
+ */
+ core = rocket_first_live_core(rdev);
+ if (!core) {
+ ret = -ENODEV;
+ goto err_free;
+ }
+
rocket_priv->rdev = rdev;
- rocket_priv->domain = rocket_iommu_domain_create(rdev->cores[0].dev);
+ rocket_priv->domain = rocket_iommu_domain_create(core->dev);
if (IS_ERR(rocket_priv->domain)) {
ret = PTR_ERR(rocket_priv->domain);
goto err_free;
@@ -199,10 +219,21 @@ static int rocket_probe(struct platform_device *pdev)
}
}
- unsigned int core = rdev->num_cores;
+ unsigned int core;
dev_set_drvdata(&pdev->dev, rdev);
+ /*
+ * Take the first free slot: cores can unbind and rebind in any
+ * order. The scan-then-claim relies on platform probes running
+ * sequentially; revisit if the driver ever enables async probe.
+ */
+ for (core = 0; core < rdev->max_cores; core++)
+ if (!rdev->cores[core].dev)
+ break;
+ if (WARN_ON(core == rdev->max_cores))
+ return -ENXIO;
+
rdev->cores[core].rdev = rdev;
rdev->cores[core].dev = &pdev->dev;
rdev->cores[core].index = index;
@@ -210,13 +241,18 @@ static int rocket_probe(struct platform_device *pdev)
rdev->num_cores++;
ret = rocket_core_init(&rdev->cores[core]);
- if (ret) {
- rdev->num_cores--;
+ if (ret)
+ goto err_core;
- if (rdev->num_cores == 0) {
- rocket_device_fini(rdev);
- rdev = NULL;
- }
+ return 0;
+
+err_core:
+ rdev->cores[core].dev = NULL;
+ rdev->num_cores--;
+
+ if (rdev->num_cores == 0) {
+ rocket_device_fini(rdev);
+ rdev = NULL;
}
return ret;
@@ -229,10 +265,11 @@ static void rocket_remove(struct platform_device *pdev)
struct device *dev = &pdev->dev;
int core = find_core_for_dev(dev);
- if (core < 0)
+ if (WARN_ON(core < 0))
return;
rocket_core_fini(&rdev->cores[core]);
+ rdev->cores[core].dev = NULL;
rdev->num_cores--;
if (rdev->num_cores == 0) {
diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index 4bc4f9c8ee403..25ee4ab172a82 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -284,7 +284,7 @@ static struct rocket_core *sched_to_core(struct rocket_device *rdev,
unsigned int core;
for (core = 0; core < rdev->max_cores; core++) {
- if (&rdev->cores[core].sched == sched)
+ if (rdev->cores[core].dev && &rdev->cores[core].sched == sched)
return &rdev->cores[core];
}
@@ -511,22 +511,42 @@ void rocket_job_fini(struct rocket_core *core)
int rocket_job_open(struct rocket_file_priv *rocket_priv)
{
struct rocket_device *rdev = rocket_priv->rdev;
- struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
- rdev->num_cores);
- unsigned int core;
+ struct drm_gpu_scheduler **scheds;
+ unsigned int core, n = 0;
int ret;
- for (core = 0; core < rdev->num_cores; core++)
- scheds[core] = &rdev->cores[core].sched;
+ scheds = kmalloc_objs(*scheds, rdev->max_cores);
+ if (!scheds)
+ return -ENOMEM;
+
+ /* Only the cores that are bound right now have a scheduler to offer. */
+ for (core = 0; core < rdev->max_cores; core++)
+ if (rdev->cores[core].dev)
+ scheds[n++] = &rdev->cores[core].sched;
+
+ if (!n) {
+ ret = -ENODEV;
+ goto err_free;
+ }
ret = drm_sched_entity_init(&rocket_priv->sched_entity,
DRM_SCHED_PRIORITY_NORMAL,
- scheds,
- rdev->num_cores, NULL);
+ scheds, n, NULL);
if (WARN_ON(ret))
- return ret;
+ goto err_free;
+
+ /*
+ * drm_sched_entity_init() keeps the list only when it holds more
+ * than one scheduler, and rocket_job_close() frees what it kept.
+ */
+ if (n < 2)
+ kfree(scheds);
return 0;
+
+err_free:
+ kfree(scheds);
+ return ret;
}
void rocket_job_close(struct rocket_file_priv *rocket_priv)
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 05/11] accel/rocket: request the core clocks by name
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
` (3 preceding siblings ...)
2026-09-22 8:01 ` [PATCH v2 04/11] accel/rocket: keep core slots stable across unbind and rebind Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 06/11] dt-bindings: npu: rockchip: allow DVFS and thermal properties Igor Paunovic
` (5 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
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")
Assisted-by: LLM sparse checkpatch
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>
---
Same diff and the same message text (his copy re-wraps the lines) as
01/14 of Jiaxing Hu's RK3576 series, which carries this patch as well;
whichever lands first, the other drops it. The trailers differ: Jiaxing
asked me to drop his Signed-off-by from my own posting of it, as he did
not pass this copy along, and Assisted-by is added because an LLM helped
with the checks on this v2.
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 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);
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 06/11] dt-bindings: npu: rockchip: allow DVFS and thermal properties
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
` (4 preceding siblings ...)
2026-09-22 8:01 ` [PATCH v2 05/11] accel/rocket: request the core clocks by name Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
2026-09-22 16:06 ` Rob Herring
2026-09-22 8:01 ` [PATCH v2 07/11] arm64: dts: rockchip: rk3588: add an OPP table for the NPU Igor Paunovic
` (4 subsequent siblings)
10 siblings, 1 reply; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
The three NPU cores on the RK3588 are fed by a single clock and a single
supply, and the firmware accepts a fixed set of rates for that clock.
Describing those rates as an operating-points-v2 table is what lets a
driver scale the NPU instead of leaving it at whatever rate the bootloader
set, so allow the property on the core node.
Throttling the NPU from a thermal zone needs a core node to be usable as
a cooling device, so allow #cooling-cells too.
The OPP table belongs on every core, with opp-shared: the cores have no
clock of their own, and one shared table for one shared clock is the same
shape a CPU cluster uses. #cooling-cells goes on one core only, the one a
thermal zone's cooling map names, because the cores cannot be throttled
independently. Naming one representative node for a shared frequency
domain is the established shape, as in "Cpufreq cooling device on CPU0"
in Documentation/devicetree/bindings/thermal/thermal-cooling-devices.yaml.
The schema cannot enforce which core carries #cooling-cells, because all
three cores share a compatible string and a node name pattern, so that
stays a devicetree convention. That is the same situation as for CPU
cooling, where cpus.yaml does not restrict #cooling-cells to cpu@0
either.
The example gains #cooling-cells; the operating-points-v2 property is
exercised by the RK3588 devicetree later in this series.
Assisted-by: LLM checkpatch dt_binding_check
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
v2: the text on where the properties go is rewritten for opp-shared on
all three cores, following Nicolas Dufresne's review of v1 3/7. The
only change to the schema file is the wording of the #cooling-cells
description; the constraints and the example are unchanged. Conor, your
Ack is kept on that basis; please say if it no longer holds.
.../bindings/npu/rockchip,rk3588-rknn-core.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml b/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml
index caca2a4903cd1..beba1896156f5 100644
--- a/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml
+++ b/Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml
@@ -42,6 +42,13 @@ properties:
- const: npu
- const: pclk
+ "#cooling-cells":
+ description:
+ Present on one core only, the first, which stands for the shared NPU
+ clock as a cooling device. The other cores have no clock of their own
+ and cannot be throttled independently of it.
+ const: 2
+
interrupts:
maxItems: 1
@@ -50,6 +57,8 @@ properties:
npu-supply: true
+ operating-points-v2: true
+
power-domains:
maxItems: 1
@@ -100,6 +109,7 @@ examples:
clocks = <&cru ACLK_NPU0>, <&cru HCLK_NPU0>,
<&scmi_clk SCMI_CLK_NPU>, <&cru PCLK_NPU_ROOT>;
clock-names = "aclk", "hclk", "npu", "pclk";
+ #cooling-cells = <2>;
interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH 0>;
iommus = <&rknn_mmu_0>;
npu-supply = <&vdd_npu_s0>;
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 06/11] dt-bindings: npu: rockchip: allow DVFS and thermal properties
2026-09-22 8:01 ` [PATCH v2 06/11] dt-bindings: npu: rockchip: allow DVFS and thermal properties Igor Paunovic
@ 2026-09-22 16:06 ` Rob Herring
0 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2026-09-22 16:06 UTC (permalink / raw)
To: Igor Paunovic
Cc: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner, Krzysztof Kozlowski,
Conor Dooley, Jeff Hugo, Robert Foss, Sidong Yang,
Diederik de Haas, Sebastian Reichel, Jiaxing Hu,
Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel
On Tue, Sep 22, 2026 at 10:01:09AM +0200, Igor Paunovic wrote:
> The three NPU cores on the RK3588 are fed by a single clock and a single
> supply, and the firmware accepts a fixed set of rates for that clock.
> Describing those rates as an operating-points-v2 table is what lets a
> driver scale the NPU instead of leaving it at whatever rate the bootloader
> set, so allow the property on the core node.
>
> Throttling the NPU from a thermal zone needs a core node to be usable as
> a cooling device, so allow #cooling-cells too.
>
> The OPP table belongs on every core, with opp-shared: the cores have no
> clock of their own, and one shared table for one shared clock is the same
> shape a CPU cluster uses. #cooling-cells goes on one core only, the one a
> thermal zone's cooling map names, because the cores cannot be throttled
> independently. Naming one representative node for a shared frequency
> domain is the established shape, as in "Cpufreq cooling device on CPU0"
> in Documentation/devicetree/bindings/thermal/thermal-cooling-devices.yaml.
>
> The schema cannot enforce which core carries #cooling-cells, because all
> three cores share a compatible string and a node name pattern, so that
> stays a devicetree convention. That is the same situation as for CPU
> cooling, where cpus.yaml does not restrict #cooling-cells to cpu@0
> either.
>
> The example gains #cooling-cells; the operating-points-v2 property is
> exercised by the RK3588 devicetree later in this series.
>
> Assisted-by: LLM checkpatch dt_binding_check
Drop 'dt_binding_check'. That's expected/assumed.
> Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 07/11] arm64: dts: rockchip: rk3588: add an OPP table for the NPU
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
` (5 preceding siblings ...)
2026-09-22 8:01 ` [PATCH v2 06/11] dt-bindings: npu: rockchip: allow DVFS and thermal properties Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 08/11] accel/rocket: restore the NPU clock boot rate before powering the cores down Igor Paunovic
` (3 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
The NPU compute clock is driven by the firmware, which only accepts one of
the rates in its own PVTPLL table: 300, 400, 500, 600, 700, 800, 900 and
1000 MHz through the PVTPLL, plus 200 MHz off GPLL. Anything else comes
back as SCMI_INVALID_PARAMETERS, and that refusal never reaches the caller:
the clock framework does not look at what the clock's set_rate returns, so
clk_set_rate() reports success and the clock stays where it was. The table
therefore has to name those rates exactly rather than describe a range.
200 MHz is included even though the vendor table stops at 300, because
mainline pins the cores there with assigned-clock-rates, the firmware's
table names 200 MHz exactly, on its GPLL path, and that is the rate the
NPU boots and idles at. Its voltage is the same 700 mV the vendor uses for
300 MHz, so it is conservative.
The voltages are the vendor's, and the upper half of the table matches the
GPU table in this file step for step: 700 MHz at 700 mV, 800 at 750, 900 at
800, 1000 at 850. There is no PVTM or binning here, for the same reason the
GPU table has none: mainline uses conservative worst-case voltages instead
of per-chip nvmem data.
The table is marked opp-shared and referenced from all three cores. They
have one clock and one supply between them and cannot be scaled
independently, and that is what opp-shared describes: one table for one
clock, the way a CPU cluster shares its table.
The full SoC range is described rather than a per-board subset, so that a
board which cannot cool the upper rates drops them in its own .dts with a
/delete-node/ on the OPP it does not want. A board may only delete OPPs
that way, never invent intermediate ones: a rate that is not in the
firmware's table is refused by the firmware, but the kernel never learns
of it, so an invented OPP would be refused while the kernel went on
reporting it as set.
rk3588j.dtsi does not include this file; it carries its own derated tables
for the CPU clusters and the GPU, and it gets no NPU table here. That is
deliberate. The J part is rated lower than the rates in this table and none
of it can be measured on the hardware this was written on, so inventing a
derated NPU table would be guessing. Its NPU node stays disabled, so
nothing binds and the cooling map added later in this series is simply
never resolved.
The same rates and voltages were arrived at independently by Nicolas
Dufresne in a proof of concept that was never posted to the list; his
version differs in that it marks 200 MHz as opp-suspend and drops the
assigned-clock-rates pins.
Link: https://gitlab.collabora.com/nicolas/linux/-/commits/rock5b-npu-poc-4
Assisted-by: LLM checkpatch dtbs_check
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
v2:
- opp-shared, and the table referenced from all three cores (Nicolas).
- The opp-suspend paragraph is gone. In the v1 thread I said v2 would
argue that the driver already puts the device back at its boot rate;
that is again driver behaviour used as a devicetree argument, which is
what Nicolas objected to, so I am not making it. Whether opp-suspend
at 200 MHz describes the hardware is a question for the DT
maintainers, in the cover letter.
- "give a driver nowhere to return to" is gone for the same reason.
- The paragraph on the table being inert until the driver patch is
gone (Nicolas).
- New: the firmware's refusal of a rate is not reported back through the
clock framework. Found by reading clk_change_rate() in drivers/clk/clk.c
after a test that requested a rate outside the table.
arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi | 54 ++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
index b5d630d2c879f..59ecaef5101da 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
@@ -151,6 +151,48 @@ opp-1000000000 {
opp-microvolt = <850000 850000 850000>;
};
};
+
+ npu_opp_table: opp-table-npu {
+ compatible = "operating-points-v2";
+ opp-shared;
+
+ opp-200000000 {
+ opp-hz = /bits/ 64 <200000000>;
+ opp-microvolt = <700000 700000 850000>;
+ };
+ opp-300000000 {
+ opp-hz = /bits/ 64 <300000000>;
+ opp-microvolt = <700000 700000 850000>;
+ };
+ opp-400000000 {
+ opp-hz = /bits/ 64 <400000000>;
+ opp-microvolt = <700000 700000 850000>;
+ };
+ opp-500000000 {
+ opp-hz = /bits/ 64 <500000000>;
+ opp-microvolt = <700000 700000 850000>;
+ };
+ opp-600000000 {
+ opp-hz = /bits/ 64 <600000000>;
+ opp-microvolt = <700000 700000 850000>;
+ };
+ opp-700000000 {
+ opp-hz = /bits/ 64 <700000000>;
+ opp-microvolt = <700000 700000 850000>;
+ };
+ opp-800000000 {
+ opp-hz = /bits/ 64 <800000000>;
+ opp-microvolt = <750000 750000 850000>;
+ };
+ opp-900000000 {
+ opp-hz = /bits/ 64 <900000000>;
+ opp-microvolt = <800000 800000 850000>;
+ };
+ opp-1000000000 {
+ opp-hz = /bits/ 64 <1000000000>;
+ opp-microvolt = <850000 850000 850000>;
+ };
+ };
};
&cpu_b0 {
@@ -188,3 +230,15 @@ &cpu_l3 {
&gpu {
operating-points-v2 = <&gpu_opp_table>;
};
+
+&rknn_core_0 {
+ operating-points-v2 = <&npu_opp_table>;
+};
+
+&rknn_core_1 {
+ operating-points-v2 = <&npu_opp_table>;
+};
+
+&rknn_core_2 {
+ operating-points-v2 = <&npu_opp_table>;
+};
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 08/11] accel/rocket: restore the NPU clock boot rate before powering the cores down
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
` (6 preceding siblings ...)
2026-09-22 8:01 ` [PATCH v2 07/11] arm64: dts: rockchip: rk3588: add an OPP table for the NPU Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
[not found] ` <20260922081326.B46651F000FF@smtp.kernel.org>
2026-09-22 8:01 ` [PATCH v2 09/11] accel/rocket: add devfreq support Igor Paunovic
` (2 subsequent siblings)
10 siblings, 1 reply; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
The compute clock is generated by a PVTPLL that lives inside the NPU power
island. Powering an island up while that clock is above the rate the
bootloader left it at does not work: the domain never acks the power-on,
and the first register access into it afterwards takes an asynchronous
SError. So the rate has to be back down before the last core goes away.
Nothing in the driver raises the clock today, which makes this a no-op on
its own, but it is the guard that has to be in the tree before anything
does, and the next patches do. The .shutdown hook is the same guard for the
handover: once devfreq is driving the clock, a kexec would otherwise pass
the raised rate to the next kernel, which powers the islands up before it
looks at it. What this cannot do is rescue a rate it did not set - the rate
read at probe is taken as the boot rate whatever it is.
The rate is read at probe rather than hardcoded. Mainline pins the RK3588
cores at 200 MHz with assigned-clock-rates, but that is a devicetree
property, not a property of the hardware, and a SoC whose devicetree does
not set it would be left running at a rate this driver had invented.
All three cores share the clock, so only the last core to suspend may lower
it; the others just drop the count. Lowering it is safe with the islands
already down as long as the boot rate is one the firmware serves from GPLL,
which on the RK3588 is the 200 MHz the devicetree pins: for that rate the
firmware writes only CRU clock selectors, never a register inside the NPU.
Assisted-by: LLM sparse checkpatch
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
v2: v1 of this patch kept a struct clk handle in struct rocket_device,
taken from the devres of the first core to probe, and used it from the
runtime suspend of whichever core went down last. Unbinding the cores
freed the handle underneath it: KASAN reported a slab-use-after-free in
clk_set_rate() during a ten-round unbind/rebind test on this board after
v1 was posted. No handle is kept any more; the callback uses the handle of
the core it runs for, which is bound for as long as the call lasts.
"A reboot" is dropped from the kexec sentence and the comment: whether the
clock selectors survive the global reset the firmware does on reboot has
not been checked. The argument that lowering the rate is safe is now
limited to a boot rate the firmware serves from GPLL, which on the RK3588
is the 200 MHz the devicetree pins.
drivers/accel/rocket/rocket_core.c | 10 ++++++
drivers/accel/rocket/rocket_device.h | 15 +++++++++
drivers/accel/rocket/rocket_drv.c | 47 ++++++++++++++++++++++++++++
3 files changed, 72 insertions(+)
diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
index 5dd260bacbff6..c736537cf28f6 100644
--- a/drivers/accel/rocket/rocket_core.c
+++ b/drivers/accel/rocket/rocket_core.c
@@ -12,6 +12,7 @@
#include <linux/reset.h>
#include "rocket_core.h"
+#include "rocket_device.h"
#include "rocket_job.h"
int rocket_core_init(struct rocket_core *core)
@@ -36,6 +37,15 @@ int rocket_core_init(struct rocket_core *core)
if (err)
return dev_err_probe(dev, err, "failed to get clocks for core %d\n", core->index);
+ /*
+ * Record what the compute clock was running at before anything here
+ * touched it, on the first core to probe. Reading it rather than
+ * hardcoding a rate keeps this working on a SoC whose devicetree does
+ * not pin the clock with assigned-clock-rates.
+ */
+ if (!core->rdev->npu_boot_rate)
+ core->rdev->npu_boot_rate = clk_get_rate(core->clks[2].clk);
+
core->pc_iomem = devm_platform_ioremap_resource_byname(pdev, "pc");
if (IS_ERR(core->pc_iomem)) {
dev_err(dev, "couldn't find PC registers %ld\n", PTR_ERR(core->pc_iomem));
diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/rocket_device.h
index abb88a254e569..ba7c977cd6951 100644
--- a/drivers/accel/rocket/rocket_device.h
+++ b/drivers/accel/rocket/rocket_device.h
@@ -22,6 +22,21 @@ struct rocket_device {
unsigned int num_cores;
/* Slot capacity (DT core count); slots with a NULL .dev are free. */
unsigned int max_cores;
+
+ /*
+ * The cores have no clock of their own: one clock feeds all of them,
+ * so any core's handle refers to the same thing. No handle is kept
+ * here: each one belongs to the devres of the core that asked for it
+ * and dies with that core's unbind, while this structure outlives any
+ * single core. Whoever needs the clock uses the handle of the core it
+ * was called for, which is bound for as long as the call lasts.
+ *
+ * npu_boot_rate is the rate the clock was left at before the driver
+ * touched it, and active_cores counts the cores that are runtime
+ * resumed right now.
+ */
+ unsigned long npu_boot_rate;
+ atomic_t active_cores;
};
struct rocket_device *rocket_device_init(struct platform_device *pdev,
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index b9b36c578db20..8f03de1af488c 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -297,6 +297,30 @@ static int find_core_for_dev(struct device *dev)
return -1;
}
+/*
+ * Put the compute clock back where the bootloader had it. The cores share
+ * this clock, so this is only correct once none of them is running any more.
+ *
+ * Lowering the rate is safe with the power islands down as long as the boot
+ * rate is one the firmware serves from GPLL, which on the RK3588 is the
+ * 200 MHz the devicetree pins: for that rate the firmware touches only the
+ * CRU clock selectors, none of the NPU's own registers.
+ */
+static void rocket_npu_restore_boot_rate(struct rocket_core *core)
+{
+ struct rocket_device *rdev = core->rdev;
+ int err;
+
+ if (!rdev->npu_boot_rate)
+ return;
+
+ err = clk_set_rate(core->clks[2].clk, rdev->npu_boot_rate);
+ if (err)
+ dev_warn(core->dev,
+ "failed to restore the NPU boot rate of %lu Hz: %d\n",
+ rdev->npu_boot_rate, err);
+}
+
static int rocket_device_runtime_resume(struct device *dev)
{
struct rocket_device *rdev = dev_get_drvdata(dev);
@@ -312,6 +336,8 @@ static int rocket_device_runtime_resume(struct device *dev)
return err;
}
+ atomic_inc(&rdev->active_cores);
+
return 0;
}
@@ -328,6 +354,9 @@ static int rocket_device_runtime_suspend(struct device *dev)
clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks), rdev->cores[core].clks);
+ if (atomic_dec_and_test(&rdev->active_cores))
+ rocket_npu_restore_boot_rate(&rdev->cores[core]);
+
return 0;
}
@@ -336,9 +365,27 @@ EXPORT_GPL_DEV_PM_OPS(rocket_pm_ops) = {
SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
};
+/*
+ * A kexec hands the next kernel whatever rate is set here, and that kernel
+ * will power the islands up before it looks at the clock.
+ */
+static void rocket_shutdown(struct platform_device *pdev)
+{
+ struct rocket_device *rdev = dev_get_drvdata(&pdev->dev);
+ int core;
+
+ if (!rdev)
+ return;
+
+ core = find_core_for_dev(&pdev->dev);
+ if (core >= 0)
+ rocket_npu_restore_boot_rate(&rdev->cores[core]);
+}
+
static struct platform_driver rocket_driver = {
.probe = rocket_probe,
.remove = rocket_remove,
+ .shutdown = rocket_shutdown,
.driver = {
.name = "rocket",
.pm = pm_ptr(&rocket_pm_ops),
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 09/11] accel/rocket: add devfreq support
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
` (7 preceding siblings ...)
2026-09-22 8:01 ` [PATCH v2 08/11] accel/rocket: restore the NPU clock boot rate before powering the cores down Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
[not found] ` <20260922081855.160451F00893@smtp.kernel.org>
2026-09-22 8:01 ` [PATCH v2 10/11] accel/rocket: register a devfreq cooling device Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 11/11] arm64: dts: rockchip: rk3588: add passive cooling to the NPU thermal zone Igor Paunovic
10 siblings, 1 reply; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
The NPU has run at whatever rate the devicetree pinned it to since the
driver was merged, which on the RK3588 is 200 MHz. The hardware reaches
1 GHz, and the firmware will change the rate on request, so let devfreq
drive it from how busy the cores actually are.
One devfreq device drives all of the cores, because they have one clock and
one supply between them and cannot be scaled apart. It hangs off the first
core in devicetree order that carries an OPP table, which on the RK3588,
where all three cores reference the shared table, is rknn_core_0. The
choice has to be fixed rather than "whichever core bound last": the
devfreq device is named after that core, and a cooling map in the
devicetree resolves against that core's node. core->index is the core's
position among the core nodes, so the lowest index is the first node.
The awkward part is that the clock is generated by a PVTPLL that sits
inside the NPU power islands. An island powered up while the clock is above
the rate the bootloader left never acknowledges the power-on, and the first
register access into it afterwards takes an asynchronous SError. So before
the rate goes up, every core is runtime resumed, and the references are
held for as long as the clock stays raised: no island can transition while
they are held, which is what makes a raised clock safe rather than merely
unlikely to be caught out. The guard and the thing it guards arrive in the
same commit, so no commit in the tree ever raises the rate without it.
Unbinding any core takes the devfreq device down, and it comes back
once every core is bound again: a rate change resumes all of them, so
the device has to be whole for the guard to mean anything. Every walk
of the core array here relies on that.
The cost is that a boosted NPU does not power-gate individual cores. It is
paid only above the boot rate; at the boot rate the references are dropped
and runtime PM behaves as before. What that costs in milliwatts has not
been measured on this board. A measurement, or a design that does not need
the references at all, would both be welcome.
Utilisation is aggregated as the maximum over the cores, not the sum: the
rate has to satisfy the busiest core, and summing would report one
saturated core out of three as a third of the load. Whether that is the
right aggregation for a shared clock is a fair question for review.
Unlike panfrost, panthor, lima and msm, the driver does not call
devfreq_suspend_device() from runtime suspend. That call ends in
cancel_delayed_work_sync() on the governor's worker, and the worker is
what calls ->target(), which resumes every core: a runtime-suspend
callback would wait for a worker that is waiting for that same callback.
Instead ->target() returns early when the rate is unchanged, so a
governor tick on an idle NPU costs one comparison and resumes nothing.
System suspend is different: dpm_suspend() runs devfreq_suspend() over
every registered devfreq before it walks the devices, from a path that
holds none of this driver's locks. What the driver adds is lowering the
rate and dropping the references from every core's ->suspend, not only
the owning core's. pm_runtime_force_suspend() powers a core down whatever
the usage count says, and the owning core need not be the first one
suspended, so a suspend that aborts partway would otherwise leave the
clock raised over islands that are already gated.
The clock and the supply are claimed in one dev_pm_opp_set_config() call
before the table is added. Naming the clock there is not optional: with no
name the OPP core takes the first clock in the node, which is the bus
clock. Configuration and table live on the owning core, which need not be
the device being probed at the time, so none of it can be devres: the call
hands back a token, and rocket_devfreq_fini() releases it, and the table,
by hand. Left to devres, a later bind would fail on an OPP table the
previous teardown never emptied.
The rate is changed through dev_pm_opp_set_rate(), so that the supply moves
with it. Nothing may set the rate behind the OPP core's back: it caches
the OPP it last applied and skips a repeat request for it, so a raw
clk_set_rate() would turn the next request for a raised rate into a
silent no-op. The boot-rate restore added in the previous patch is
converted accordingly.
The rate the previous patch reads at probe is the firmware's own number
and need not be in the OPP table: a board that does not pin the clock
with assigned-clock-rates gets whatever the firmware's divider produces.
So the boot rate is normalised to the table once, at init, to the lowest
OPP that is not below it, and everything here compares against and
returns to that OPP. Comparing cur_freq against a rate that is not in the
table would leave the cores held after the first change, with no
governor tick ever able to match it.
->get_cur_freq() and the status callback report the rate this driver last
requested rather than asking the clock: devfreq queries them from sysfs
with no runtime PM reference of its own, and the answer has to be a rate
from the OPP table or devfreq_get_freq_level() does not find it and
devfreq warns on every transition. "Requested" is the accurate word: a
rate the firmware refuses does not come back as an error, because the
clock framework ignores what the clock's set_rate returns. Keeping every
request to the OPP table, which names only the rates the firmware accepts,
keeps a request from being refused; what sysfs shows is still the request,
not a measurement.
A devicetree with no OPP table is not an error: the driver returns without
a devfreq device and the NPU keeps its boot rate, as before this patch.
The governor thresholds are a starting point taken from the other
accelerators in tree, not a measurement. Inference workloads have not been
profiled against them.
Assisted-by: LLM sparse checkpatch
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
v2:
- The devfreq device goes on the first core in devicetree order with an
OPP table, not on whichever core with the table bound first, as v1
would have done with the table on all three cores (Nicolas).
- The boot rate is normalised to the OPP table at init. Found by reading
the code: without assigned-clock-rates the raw rate is not in the table
and the cores would have stayed held after the first change. Code read
only so far; a devicetree without the property has not been booted.
- Init holds every core while it programs the initial OPP, since
rounding the boot rate up to the table may raise the clock.
- "last programmed" is now "last requested", with the reason.
- Comments and text updated for the shared table; a comment that
counted ten governor ticks a second now counts twenty (50 ms polling).
- The commit message now says that unbinding any core takes the
devfreq device down until all are bound again; v1 did the same
without saying so.
- The utilisation counters of a core that was unbound and bound again
start from zero.
- A comment over hold_all() states the invariant the loops rely on.
drivers/accel/rocket/Kconfig | 2 +
drivers/accel/rocket/Makefile | 1 +
drivers/accel/rocket/rocket_core.h | 11 +
drivers/accel/rocket/rocket_devfreq.c | 500 ++++++++++++++++++++++++++
drivers/accel/rocket/rocket_devfreq.h | 65 ++++
drivers/accel/rocket/rocket_device.h | 3 +
drivers/accel/rocket/rocket_drv.c | 59 ++-
drivers/accel/rocket/rocket_job.c | 7 +
8 files changed, 646 insertions(+), 2 deletions(-)
create mode 100644 drivers/accel/rocket/rocket_devfreq.c
create mode 100644 drivers/accel/rocket/rocket_devfreq.h
diff --git a/drivers/accel/rocket/Kconfig b/drivers/accel/rocket/Kconfig
index 16465abe06607..00ee845c871fa 100644
--- a/drivers/accel/rocket/Kconfig
+++ b/drivers/accel/rocket/Kconfig
@@ -8,6 +8,8 @@ config DRM_ACCEL_ROCKET
depends on MMU
select DRM_SCHED
select DRM_GEM_SHMEM_HELPER
+ select PM_DEVFREQ
+ select DEVFREQ_GOV_SIMPLE_ONDEMAND
help
Choose this option if you have a Rockchip SoC that contains a
compatible Neural Processing Unit (NPU), such as the RK3588. Called by
diff --git a/drivers/accel/rocket/Makefile b/drivers/accel/rocket/Makefile
index 3713dfe223d6e..e0944b3e68121 100644
--- a/drivers/accel/rocket/Makefile
+++ b/drivers/accel/rocket/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_ACCEL_ROCKET) := rocket.o
rocket-y := \
rocket_core.o \
rocket_device.o \
+ rocket_devfreq.o \
rocket_drv.o \
rocket_gem.o \
rocket_job.o
diff --git a/drivers/accel/rocket/rocket_core.h b/drivers/accel/rocket/rocket_core.h
index 46ed8352a79d2..c9995bd9e0553 100644
--- a/drivers/accel/rocket/rocket_core.h
+++ b/drivers/accel/rocket/rocket_core.h
@@ -7,6 +7,7 @@
#include <drm/gpu_scheduler.h>
#include <linux/clk.h>
#include <linux/io.h>
+#include <linux/ktime.h>
#include <linux/mutex_types.h>
#include <linux/reset.h>
@@ -60,6 +61,16 @@ struct rocket_core {
struct drm_gpu_scheduler sched;
u64 fence_context;
u64 emit_seqno;
+
+ /*
+ * Utilisation seen by devfreq, guarded by rdev->devfreq.busy_lock. A
+ * core runs one task at a time, so a flag is exact here and, unlike a
+ * counter, cannot be left skewed by a job the reset path tore down.
+ */
+ bool busy;
+ ktime_t busy_time;
+ ktime_t idle_time;
+ ktime_t time_last_update;
};
int rocket_core_init(struct rocket_core *core);
diff --git a/drivers/accel/rocket/rocket_devfreq.c b/drivers/accel/rocket/rocket_devfreq.c
new file mode 100644
index 0000000000000..871fa370eb432
--- /dev/null
+++ b/drivers/accel/rocket/rocket_devfreq.c
@@ -0,0 +1,500 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright 2025 Igor Paunovic <royalnet026@gmail.com> */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/ktime.h>
+#include <linux/minmax.h>
+#include <linux/of.h>
+#include <linux/pm_opp.h>
+#include <linux/pm_runtime.h>
+
+#include "rocket_core.h"
+#include "rocket_device.h"
+#include "rocket_devfreq.h"
+
+/*
+ * One clock and one supply feed all of the NPU cores, so a single devfreq
+ * device drives them together. It hangs off the first core in devicetree
+ * order that carries an OPP table.
+ *
+ * The awkward part is that the clock is generated by a PVTPLL that sits
+ * inside the NPU power islands. An island that is powered up while the clock
+ * is above the rate the bootloader left never acknowledges the power-on, and
+ * the first register access into it afterwards takes an asynchronous SError.
+ *
+ * Lowering the rate is fine at any time as long as the boot rate is one the
+ * firmware serves from GPLL, which on the RK3588 is the pinned 200 MHz: for
+ * that rate it writes only CRU clock selectors. Raising the rate is not, so
+ * before it goes up every core is runtime resumed and the
+ * references are kept for as long as the clock stays raised. While they are
+ * held no core can suspend, so no island can transition at all, which is the
+ * property that makes the raised clock safe rather than merely unlikely to be
+ * caught out.
+ *
+ * The cost is that a busy NPU does not power-gate individual cores. It is
+ * paid only above the boot rate: at the boot rate the references are dropped
+ * and runtime PM behaves exactly as it did before this file existed. The cost
+ * in milliwatts has not been measured on this board, and a measurement or a
+ * better idea would both be welcome.
+ */
+
+static void rocket_devfreq_update_utilisation(struct rocket_core *core)
+{
+ ktime_t now = ktime_get();
+ ktime_t elapsed = ktime_sub(now, core->time_last_update);
+
+ if (core->busy)
+ core->busy_time = ktime_add(core->busy_time, elapsed);
+ else
+ core->idle_time = ktime_add(core->idle_time, elapsed);
+
+ core->time_last_update = now;
+}
+
+/*
+ * The devfreq device exists only while every slot in rdev->cores[] is
+ * filled: it goes up when num_cores reaches max_cores and comes down in
+ * rocket_remove() before the leaving core empties its slot. The loops
+ * over num_cores here and below rely on that.
+ */
+static int rocket_devfreq_hold_all(struct rocket_device *rdev)
+{
+ unsigned int i;
+ int ret;
+
+ for (i = 0; i < rdev->num_cores; i++) {
+ ret = pm_runtime_resume_and_get(rdev->cores[i].dev);
+ if (ret < 0) {
+ while (i--)
+ pm_runtime_put_autosuspend(rdev->cores[i].dev);
+
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static void rocket_devfreq_release_all(struct rocket_device *rdev)
+{
+ unsigned int i;
+
+ for (i = 0; i < rdev->num_cores; i++)
+ pm_runtime_put_autosuspend(rdev->cores[i].dev);
+}
+
+/* Caller holds rdev->devfreq.lock. */
+static int rocket_devfreq_set_rate(struct rocket_device *rdev, unsigned long freq)
+{
+ struct rocket_devfreq *rdevfreq = &rdev->devfreq;
+ struct device *dev = rdevfreq->owner->dev;
+ int ret;
+
+ ret = dev_pm_opp_set_rate(dev, freq);
+ if (ret) {
+ dev_err(dev, "failed to set the NPU rate to %lu Hz: %d\n", freq, ret);
+ return ret;
+ }
+
+ WRITE_ONCE(rdevfreq->cur_freq, freq);
+
+ return 0;
+}
+
+static int rocket_devfreq_target(struct device *dev, unsigned long *freq, u32 flags)
+{
+ struct rocket_device *rdev = dev_get_drvdata(dev);
+ struct rocket_devfreq *rdevfreq = &rdev->devfreq;
+ struct dev_pm_opp *opp;
+ int ret;
+
+ opp = devfreq_recommended_opp(dev, freq, flags);
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+ dev_pm_opp_put(opp);
+
+ guard(mutex)(&rdevfreq->lock);
+
+ /*
+ * The governor calls this on every tick, including the ticks where it
+ * arrives at the rate the NPU is already running. Without this an idle
+ * NPU would resume all of its cores twenty times a second to set the
+ * rate they already have.
+ */
+ if (*freq == READ_ONCE(rdevfreq->cur_freq))
+ return 0;
+
+ if (!rdevfreq->cores_held) {
+ ret = rocket_devfreq_hold_all(rdev);
+ if (ret) {
+ /*
+ * Runtime PM is disabled on the way into system
+ * suspend, so this is the ordinary way for a governor
+ * tick that raced with it to end.
+ */
+ dev_dbg(dev, "cannot resume the NPU cores to change rate: %d\n", ret);
+ return ret;
+ }
+ rdevfreq->cores_held = true;
+ }
+
+ ret = rocket_devfreq_set_rate(rdev, *freq);
+
+ /* At the boot rate the cores are free to suspend again. */
+ if (READ_ONCE(rdevfreq->cur_freq) <= rdevfreq->boot_freq) {
+ rdevfreq->cores_held = false;
+ rocket_devfreq_release_all(rdev);
+ }
+
+ return ret;
+}
+
+static int rocket_devfreq_get_dev_status(struct device *dev,
+ struct devfreq_dev_status *status)
+{
+ struct rocket_device *rdev = dev_get_drvdata(dev);
+ ktime_t busy = 0, total = 0;
+ unsigned int i;
+
+ scoped_guard(spinlock_irqsave, &rdev->devfreq.busy_lock) {
+ for (i = 0; i < rdev->num_cores; i++) {
+ struct rocket_core *core = &rdev->cores[i];
+
+ rocket_devfreq_update_utilisation(core);
+
+ /*
+ * The cores share the clock, so what the rate has to
+ * satisfy is the busiest of them. Adding the cores up
+ * instead would report one saturated core out of three
+ * as a third of the load, and clock down underneath it.
+ */
+ busy = max(busy, core->busy_time);
+ total = max(total, ktime_add(core->busy_time, core->idle_time));
+
+ core->busy_time = 0;
+ core->idle_time = 0;
+ }
+ }
+
+ status->busy_time = ktime_to_ns(busy);
+ status->total_time = ktime_to_ns(total);
+ status->current_frequency = READ_ONCE(rdev->devfreq.cur_freq);
+
+ dev_dbg(dev, "busy %lu total %lu %lu%% freq %lu MHz\n",
+ status->busy_time, status->total_time,
+ status->busy_time * 100 / max(status->total_time, 1UL),
+ status->current_frequency / 1000 / 1000);
+
+ return 0;
+}
+
+/*
+ * Report the rate this driver last requested, not what the firmware says.
+ * devfreq asks for the current frequency from sysfs as well, without a runtime
+ * PM reference of its own, and the answer has to be one of the rates in the
+ * OPP table or devfreq_get_freq_level() will not find it and every transition
+ * will be logged as unknown.
+ *
+ * "Requested" is the accurate word: a rate the firmware refuses does not come
+ * back as an error, because the clock framework ignores what the clock's
+ * set_rate returns and the clock stays where it was. Keeping every request to
+ * the OPP table, which names only the rates the firmware accepts, keeps a
+ * request from being refused; what is reported is still the request, not a
+ * measurement.
+ */
+static int rocket_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+ struct rocket_device *rdev = dev_get_drvdata(dev);
+
+ *freq = READ_ONCE(rdev->devfreq.cur_freq);
+
+ return 0;
+}
+
+static struct devfreq_dev_profile rocket_devfreq_profile = {
+ .timer = DEVFREQ_TIMER_DELAYED,
+ .polling_ms = 50,
+ .target = rocket_devfreq_target,
+ .get_dev_status = rocket_devfreq_get_dev_status,
+ .get_cur_freq = rocket_devfreq_get_cur_freq,
+};
+
+void rocket_devfreq_record_busy(struct rocket_core *core)
+{
+ struct rocket_devfreq *rdevfreq = &core->rdev->devfreq;
+
+ if (!rdevfreq->devfreq)
+ return;
+
+ scoped_guard(spinlock_irqsave, &rdevfreq->busy_lock) {
+ rocket_devfreq_update_utilisation(core);
+ core->busy = true;
+ }
+}
+
+/*
+ * Idempotent on purpose: a job that times out is torn down by the reset path,
+ * which cannot know whether the completion interrupt got there first.
+ */
+void rocket_devfreq_record_idle(struct rocket_core *core)
+{
+ struct rocket_devfreq *rdevfreq = &core->rdev->devfreq;
+
+ if (!rdevfreq->devfreq)
+ return;
+
+ scoped_guard(spinlock_irqsave, &rdevfreq->busy_lock) {
+ rocket_devfreq_update_utilisation(core);
+ core->busy = false;
+ }
+}
+
+/*
+ * Put the clock back to the boot OPP through the OPP core. Called with no
+ * lock held, from the last core on its way down.
+ */
+int rocket_devfreq_set_boot_rate(struct rocket_device *rdev)
+{
+ struct rocket_devfreq *rdevfreq = &rdev->devfreq;
+ int ret;
+
+ ret = dev_pm_opp_set_rate(rdevfreq->owner->dev, rdevfreq->boot_freq);
+ if (!ret)
+ WRITE_ONCE(rdevfreq->cur_freq, rdevfreq->boot_freq);
+
+ return ret;
+}
+
+/*
+ * System suspend, called from every core's ->suspend before it is forced down.
+ * The first one to get here does the work and the rest are no-ops.
+ *
+ * It has to be every core and not just the one that owns the devfreq device.
+ * That core is suspended last, and a suspend that aborts partway - a pending
+ * wakeup, or this driver's own -EBUSY on a core that is still busy - would
+ * never reach it, leaving the clock raised over already gated islands.
+ *
+ * No governor tick can be in flight here: dpm_suspend() calls devfreq_suspend()
+ * before it walks the devices, which stops the monitor on every registered
+ * devfreq. That is also where this driver does get devfreq_suspend_device() -
+ * from the PM core, on a path that holds no runtime PM lock of ours.
+ */
+void rocket_devfreq_suspend(struct rocket_device *rdev)
+{
+ struct rocket_devfreq *rdevfreq = &rdev->devfreq;
+
+ if (!rdevfreq->devfreq)
+ return;
+
+ guard(mutex)(&rdevfreq->lock);
+
+ if (!rdevfreq->cores_held)
+ return;
+
+ rocket_devfreq_set_rate(rdev, rdevfreq->boot_freq);
+ rdevfreq->cores_held = false;
+ rocket_devfreq_release_all(rdev);
+}
+
+int rocket_devfreq_init(struct rocket_device *rdev)
+{
+ static const char * const clk_names[] = { "npu", NULL };
+ static const char * const supplies[] = { "npu", NULL };
+ struct dev_pm_opp_config config = {
+ .clk_names = clk_names,
+ .regulator_names = supplies,
+ };
+ struct rocket_devfreq *rdevfreq = &rdev->devfreq;
+ struct rocket_core *owner = NULL;
+ struct dev_pm_opp *opp;
+ struct device *dev;
+ unsigned long freq;
+ unsigned int i;
+ int ret;
+
+ /*
+ * The devfreq device hangs off the first core in devicetree order that
+ * carries an OPP table: on the RK3588 every core references the shared
+ * table, so that is rknn_core_0. It has to be a fixed choice and not
+ * whichever core bound last, because the devfreq device is named after
+ * this core and a cooling map in the devicetree resolves against its
+ * node. core->index is the core's position among the core nodes, so
+ * the lowest index is the first node.
+ */
+ for (i = 0; i < rdev->num_cores; i++) {
+ struct rocket_core *core = &rdev->cores[i];
+
+ if (!of_property_present(core->dev->of_node, "operating-points-v2"))
+ continue;
+
+ if (!owner || core->index < owner->index)
+ owner = core;
+ }
+
+ /*
+ * No OPP table is not an error. It asks for the NPU to stay at the
+ * rate it booted at, which is what this driver did before devfreq.
+ */
+ if (!owner)
+ return 0;
+
+ dev = owner->dev;
+
+ /*
+ * None of this can be devres. It is attached to the owning core, while
+ * the device being probed right now is whichever core happened to bind
+ * last, so devres would outlive the teardown in rocket_devfreq_fini()
+ * and a later rebind would find the OPP table still populated.
+ *
+ * Naming the clock matters as much as claiming the supply: without it
+ * the OPP core takes the first clock in the node, which is the bus
+ * clock, and would scale that instead of the compute clock.
+ */
+ ret = dev_pm_opp_set_config(dev, &config);
+ if (ret < 0) {
+ if (ret != -ENODEV)
+ return dev_err_probe(dev, ret,
+ "failed to set the OPP clock and supply\n");
+
+ dev_info(dev, "no NPU supply described, leaving the clock alone\n");
+ return 0;
+ }
+ rdevfreq->opp_token = ret;
+
+ ret = dev_pm_opp_of_add_table(dev);
+ if (ret) {
+ if (ret != -ENODEV)
+ dev_err_probe(dev, ret, "failed to add the OPP table\n");
+ else
+ ret = 0;
+
+ goto err_clear_config;
+ }
+
+ mutex_init(&rdevfreq->lock);
+ spin_lock_init(&rdevfreq->busy_lock);
+
+ for (i = 0; i < rdev->num_cores; i++) {
+ struct rocket_core *core = &rdev->cores[i];
+
+ /* A core that was unbound and bound again starts over. */
+ core->busy = false;
+ core->busy_time = 0;
+ core->idle_time = 0;
+ core->time_last_update = ktime_get();
+ }
+
+ freq = rdev->npu_boot_rate;
+ opp = devfreq_recommended_opp(dev, &freq, 0);
+ if (IS_ERR(opp)) {
+ ret = dev_err_probe(dev, PTR_ERR(opp),
+ "no OPP covers the %lu Hz boot rate\n",
+ rdev->npu_boot_rate);
+ goto err_remove_table;
+ }
+
+ /*
+ * From here on the boot rate is the OPP it maps to. The raw rate is
+ * the firmware's number and need not be in the table at all.
+ */
+ rdevfreq->boot_freq = freq;
+
+ /*
+ * Program the supply for the rate the NPU is already running, so that
+ * the regulator is not switched off underneath it by
+ * regulator_late_cleanup(). That OPP is the boot rate rounded up to
+ * the table, so this may raise the clock, and a raised clock is only
+ * ever programmed with every core held: the same rule as ->target().
+ */
+ ret = rocket_devfreq_hold_all(rdev);
+ if (ret) {
+ dev_pm_opp_put(opp);
+ dev_err_probe(dev, ret,
+ "cannot resume the NPU cores to set the initial OPP\n");
+ goto err_remove_table;
+ }
+ ret = dev_pm_opp_set_opp(dev, opp);
+ rocket_devfreq_release_all(rdev);
+ dev_pm_opp_put(opp);
+ if (ret) {
+ dev_err_probe(dev, ret, "failed to set the initial OPP\n");
+ goto err_remove_table;
+ }
+
+ rdevfreq->cur_freq = freq;
+ rdevfreq->owner = owner;
+ rocket_devfreq_profile.initial_freq = freq;
+
+ /*
+ * A starting point taken from the other accelerators in tree, not a
+ * measurement: inference workloads have not been profiled against
+ * these thresholds.
+ */
+ rdevfreq->gov_data.upthreshold = 50;
+ rdevfreq->gov_data.downdifferential = 10;
+
+ rdevfreq->devfreq = devfreq_add_device(dev, &rocket_devfreq_profile,
+ DEVFREQ_GOV_SIMPLE_ONDEMAND,
+ &rdevfreq->gov_data);
+ if (IS_ERR(rdevfreq->devfreq)) {
+ ret = PTR_ERR(rdevfreq->devfreq);
+ rdevfreq->devfreq = NULL;
+ rdevfreq->owner = NULL;
+
+ dev_err_probe(dev, ret, "failed to add the devfreq device\n");
+ goto err_remove_table;
+ }
+
+ return 0;
+
+err_remove_table:
+ mutex_destroy(&rdevfreq->lock);
+ dev_pm_opp_of_remove_table(dev);
+err_clear_config:
+ dev_pm_opp_clear_config(rdevfreq->opp_token);
+ rdevfreq->opp_token = 0;
+
+ return ret;
+}
+
+void rocket_devfreq_fini(struct rocket_device *rdev)
+{
+ struct rocket_devfreq *rdevfreq = &rdev->devfreq;
+ struct device *dev;
+
+ if (!rdevfreq->devfreq)
+ return;
+
+ dev = rdevfreq->owner->dev;
+
+ devfreq_remove_device(rdevfreq->devfreq);
+ rdevfreq->devfreq = NULL;
+
+ /*
+ * Lower the clock before letting go of the cores, not after: a core
+ * that suspends while the clock is still raised would be unable to
+ * come back.
+ */
+ scoped_guard(mutex, &rdevfreq->lock) {
+ if (rdevfreq->cores_held) {
+ rocket_devfreq_set_rate(rdev, rdevfreq->boot_freq);
+ rdevfreq->cores_held = false;
+ rocket_devfreq_release_all(rdev);
+ }
+ }
+
+ /*
+ * Undo the OPP setup by hand, in the reverse order. Everything above
+ * lives on the owning core rather than on the device that was probing
+ * when it was set up, so nothing here is released by devres; leaving
+ * the table populated would make the next bind fail with the OPP core
+ * complaining that it is not empty. After this the owner is gone, so
+ * anything that still wants the boot rate asks the clock directly.
+ */
+ rdevfreq->owner = NULL;
+ mutex_destroy(&rdevfreq->lock);
+ dev_pm_opp_of_remove_table(dev);
+ dev_pm_opp_clear_config(rdevfreq->opp_token);
+ rdevfreq->opp_token = 0;
+}
diff --git a/drivers/accel/rocket/rocket_devfreq.h b/drivers/accel/rocket/rocket_devfreq.h
new file mode 100644
index 0000000000000..65a9de6d37389
--- /dev/null
+++ b/drivers/accel/rocket/rocket_devfreq.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright 2025 Igor Paunovic <royalnet026@gmail.com> */
+
+#ifndef __ROCKET_DEVFREQ_H__
+#define __ROCKET_DEVFREQ_H__
+
+#include <linux/devfreq.h>
+#include <linux/mutex_types.h>
+#include <linux/spinlock_types.h>
+
+struct rocket_core;
+struct rocket_device;
+
+struct rocket_devfreq {
+ struct devfreq *devfreq;
+ struct devfreq_simple_ondemand_data gov_data;
+
+ /*
+ * The core the devfreq device hangs off: the first in devicetree order
+ * to carry an OPP table. NULL when no core does.
+ */
+ struct rocket_core *owner;
+
+ /*
+ * The OPP clock and supply configuration is attached to the owning
+ * core, which is not the device this driver is probing when it is set
+ * up, so it cannot be devres. Zero means nothing is attached.
+ */
+ int opp_token;
+
+ /*
+ * Serialises rate changes against each other and against the set of
+ * runtime PM references taken below.
+ *
+ * This is never taken from a runtime PM callback. A rate change
+ * resumes every core while holding it, so a core that took it on its
+ * way down would wait for a rate change that is waiting for that same
+ * core to finish suspending.
+ */
+ struct mutex lock;
+ unsigned long cur_freq;
+ bool cores_held;
+
+ /*
+ * The boot rate as an OPP: the lowest rate in the table that is not
+ * below rdev->npu_boot_rate. The raw boot rate is whatever the firmware
+ * reported at probe and need not be in the table, and comparing
+ * cur_freq against a rate that is not in the table would leave the
+ * cores held for good after the first change. Everything here compares
+ * against and returns to this instead.
+ */
+ unsigned long boot_freq;
+
+ /* Guards the utilisation fields of every core. */
+ spinlock_t busy_lock;
+};
+
+int rocket_devfreq_init(struct rocket_device *rdev);
+void rocket_devfreq_fini(struct rocket_device *rdev);
+void rocket_devfreq_suspend(struct rocket_device *rdev);
+int rocket_devfreq_set_boot_rate(struct rocket_device *rdev);
+void rocket_devfreq_record_busy(struct rocket_core *core);
+void rocket_devfreq_record_idle(struct rocket_core *core);
+
+#endif /* __ROCKET_DEVFREQ_H__ */
diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/rocket_device.h
index ba7c977cd6951..a91d5a9b09ed3 100644
--- a/drivers/accel/rocket/rocket_device.h
+++ b/drivers/accel/rocket/rocket_device.h
@@ -11,6 +11,7 @@
#include <linux/platform_device.h>
#include "rocket_core.h"
+#include "rocket_devfreq.h"
struct rocket_device {
struct drm_device ddev;
@@ -37,6 +38,8 @@ struct rocket_device {
*/
unsigned long npu_boot_rate;
atomic_t active_cores;
+
+ struct rocket_devfreq devfreq;
};
struct rocket_device *rocket_device_init(struct platform_device *pdev,
diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 8f03de1af488c..c6eab2239b6a9 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -14,6 +14,7 @@
#include <linux/pm_runtime.h>
#include "rocket_device.h"
+#include "rocket_devfreq.h"
#include "rocket_drv.h"
#include "rocket_gem.h"
#include "rocket_job.h"
@@ -244,6 +245,20 @@ static int rocket_probe(struct platform_device *pdev)
if (ret)
goto err_core;
+ /*
+ * Every core described in the devicetree has to be bound before the
+ * devfreq device goes up. A rate change resumes all of them and keeps
+ * them resumed, and a core that had not probed yet would come up later
+ * underneath a raised clock.
+ */
+ if (rdev->num_cores == rdev->max_cores) {
+ ret = rocket_devfreq_init(rdev);
+ if (ret) {
+ rocket_core_fini(&rdev->cores[core]);
+ goto err_core;
+ }
+ }
+
return 0;
err_core:
@@ -268,6 +283,9 @@ static void rocket_remove(struct platform_device *pdev)
if (WARN_ON(core < 0))
return;
+ /* The devfreq device drives every core, so it goes before any of them. */
+ rocket_devfreq_fini(rdev);
+
rocket_core_fini(&rdev->cores[core]);
rdev->cores[core].dev = NULL;
rdev->num_cores--;
@@ -314,7 +332,18 @@ static void rocket_npu_restore_boot_rate(struct rocket_core *core)
if (!rdev->npu_boot_rate)
return;
- err = clk_set_rate(core->clks[2].clk, rdev->npu_boot_rate);
+ /*
+ * Go through the OPP core once there is a table, never behind its
+ * back: it caches the OPP it last applied and skips a request for that
+ * same OPP, so a raw clk_set_rate() here would make the next request
+ * for the raised rate a silent no-op, with sysfs reporting a rate the
+ * hardware was not running.
+ */
+ if (rdev->devfreq.owner)
+ err = rocket_devfreq_set_boot_rate(rdev);
+ else
+ err = clk_set_rate(core->clks[2].clk, rdev->npu_boot_rate);
+
if (err)
dev_warn(core->dev,
"failed to restore the NPU boot rate of %lu Hz: %d\n",
@@ -360,14 +389,38 @@ static int rocket_device_runtime_suspend(struct device *dev)
return 0;
}
+static int rocket_device_suspend(struct device *dev)
+{
+ struct rocket_device *rdev = dev_get_drvdata(dev);
+
+ /*
+ * pm_runtime_force_suspend() below powers this core down whatever the
+ * runtime PM usage count says, so the references taken while the clock
+ * is raised do not hold it off. Put the rate back first; the call is a
+ * no-op on every core after the first.
+ */
+ rocket_devfreq_suspend(rdev);
+
+ return pm_runtime_force_suspend(dev);
+}
+
EXPORT_GPL_DEV_PM_OPS(rocket_pm_ops) = {
RUNTIME_PM_OPS(rocket_device_runtime_suspend, rocket_device_runtime_resume, NULL)
- SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+ SYSTEM_SLEEP_PM_OPS(rocket_device_suspend, pm_runtime_force_resume)
};
/*
* A kexec hands the next kernel whatever rate is set here, and that kernel
* will power the islands up before it looks at the clock.
+ *
+ * Take the devfreq device down before restoring the rate rather than after.
+ * Nothing freezes workqueues on this path, so a governor tick that landed
+ * after the restore would raise the clock straight back up and hand on exactly
+ * what this is here to prevent.
+ *
+ * The hook runs once per core. After the first call the devfreq device is
+ * gone, and each later restore asks the clock for the rate it already has,
+ * which the clock framework drops before it reaches the firmware.
*/
static void rocket_shutdown(struct platform_device *pdev)
{
@@ -377,6 +430,8 @@ static void rocket_shutdown(struct platform_device *pdev)
if (!rdev)
return;
+ rocket_devfreq_fini(rdev);
+
core = find_core_for_dev(&pdev->dev);
if (core >= 0)
rocket_npu_restore_boot_rate(&rdev->cores[core]);
diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index 25ee4ab172a82..7846b627b00f4 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -15,6 +15,7 @@
#include "rocket_core.h"
#include "rocket_device.h"
+#include "rocket_devfreq.h"
#include "rocket_drv.h"
#include "rocket_job.h"
#include "rocket_registers.h"
@@ -149,6 +150,8 @@ static void rocket_job_hw_submit(struct rocket_core *core, struct rocket_job *jo
rocket_pc_writel(core, TASK_DMA_BASE_ADDR, PC_TASK_DMA_BASE_ADDR_DMA_BASE_ADDR(0x0));
+ rocket_devfreq_record_busy(core);
+
rocket_pc_writel(core, OPERATION_ENABLE, PC_OPERATION_ENABLE_OP_EN(1));
dev_dbg(core->dev, "Submitted regcmd at 0x%llx to core %d", task->regcmd, core->index);
@@ -348,6 +351,8 @@ static void rocket_job_handle_irq(struct rocket_core *core)
rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
+ rocket_devfreq_record_idle(core);
+
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) {
@@ -379,6 +384,8 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
if (core->in_flight_job)
pm_runtime_put_noidle(core->dev);
+ rocket_devfreq_record_idle(core);
+
iommu_detach_group(NULL, core->iommu_group);
core->in_flight_job = NULL;
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 10/11] accel/rocket: register a devfreq cooling device
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
` (8 preceding siblings ...)
2026-09-22 8:01 ` [PATCH v2 09/11] accel/rocket: add devfreq support Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
2026-09-22 8:01 ` [PATCH v2 11/11] arm64: dts: rockchip: rk3588: add passive cooling to the NPU thermal zone Igor Paunovic
10 siblings, 0 replies; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
With devfreq driving the NPU clock, a thermal zone can now throttle the NPU
by capping that clock. Register the cooling device so a devicetree can bind
it to a zone.
The _em variant is used, not because there is an energy model today but so
that there will be one the day a power coefficient for this NPU is
measured. There is none now: the NPU core nodes carry no
dynamic-power-coefficient, Rockchip does not publish one, and a made-up
number would be worse than no number. devfreq_cooling_em_register() logs
the missing model at debug level and registers the cooling device anyway,
so what this gets today is step-wise throttling with no power model for the
IPA governor to use. Measuring the coefficient is follow-up work.
Registration is allowed to fail. A kernel built without DEVFREQ_THERMAL
gets a stub that returns an error, and losing throttling is not a reason to
refuse to drive the NPU at all, so the failure is logged and probe carries
on. The cooling device is unregistered by hand before the devfreq device it
is attached to goes away.
Assisted-by: LLM sparse checkpatch
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
v2: wording only, for the shared table.
drivers/accel/rocket/rocket_devfreq.c | 25 +++++++++++++++++++++++++
drivers/accel/rocket/rocket_devfreq.h | 2 ++
2 files changed, 27 insertions(+)
diff --git a/drivers/accel/rocket/rocket_devfreq.c b/drivers/accel/rocket/rocket_devfreq.c
index 871fa370eb432..c81887734de77 100644
--- a/drivers/accel/rocket/rocket_devfreq.c
+++ b/drivers/accel/rocket/rocket_devfreq.c
@@ -3,6 +3,7 @@
#include <linux/clk.h>
#include <linux/devfreq.h>
+#include <linux/devfreq_cooling.h>
#include <linux/ktime.h>
#include <linux/minmax.h>
#include <linux/of.h>
@@ -446,6 +447,25 @@ int rocket_devfreq_init(struct rocket_device *rdev)
goto err_remove_table;
}
+ /*
+ * Thermal throttling is optional, so a kernel built without
+ * DEVFREQ_THERMAL keeps a working NPU rather than a failed probe.
+ *
+ * The _em variant is used so that the driver is ready for an energy
+ * model the day a power coefficient for this NPU is measured. There is
+ * none today: the NPU core nodes have no dynamic-power-coefficient,
+ * the vendor does not publish one, and inventing a number would be
+ * worse than having none. Without it the EM registration inside is
+ * skipped and throttling is step-wise, with no power model for IPA to
+ * use.
+ */
+ rdevfreq->cooling = devfreq_cooling_em_register(rdevfreq->devfreq, NULL);
+ if (IS_ERR(rdevfreq->cooling)) {
+ dev_info(dev, "no devfreq cooling device (%pe), NPU will not be throttled\n",
+ rdevfreq->cooling);
+ rdevfreq->cooling = NULL;
+ }
+
return 0;
err_remove_table:
@@ -468,6 +488,11 @@ void rocket_devfreq_fini(struct rocket_device *rdev)
dev = rdevfreq->owner->dev;
+ if (rdevfreq->cooling) {
+ devfreq_cooling_unregister(rdevfreq->cooling);
+ rdevfreq->cooling = NULL;
+ }
+
devfreq_remove_device(rdevfreq->devfreq);
rdevfreq->devfreq = NULL;
diff --git a/drivers/accel/rocket/rocket_devfreq.h b/drivers/accel/rocket/rocket_devfreq.h
index 65a9de6d37389..26a3749078b9f 100644
--- a/drivers/accel/rocket/rocket_devfreq.h
+++ b/drivers/accel/rocket/rocket_devfreq.h
@@ -10,9 +10,11 @@
struct rocket_core;
struct rocket_device;
+struct thermal_cooling_device;
struct rocket_devfreq {
struct devfreq *devfreq;
+ struct thermal_cooling_device *cooling;
struct devfreq_simple_ondemand_data gov_data;
/*
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 11/11] arm64: dts: rockchip: rk3588: add passive cooling to the NPU thermal zone
2026-09-22 8:01 [PATCH v2 00/11] accel/rocket: DVFS for the RK3588 NPU Igor Paunovic
` (9 preceding siblings ...)
2026-09-22 8:01 ` [PATCH v2 10/11] accel/rocket: register a devfreq cooling device Igor Paunovic
@ 2026-09-22 8:01 ` Igor Paunovic
10 siblings, 0 replies; 15+ messages in thread
From: Igor Paunovic @ 2026-09-22 8:01 UTC (permalink / raw)
To: Tomeu Vizoso, Oded Gabbay, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff Hugo,
Robert Foss, Sidong Yang, Diederik de Haas, Sebastian Reichel,
Jiaxing Hu, Nicolas Dufresne, Jonas Karlman, Guangshuo Li,
Hüseyin BIYIK, dri-devel, linux-rockchip, linux-arm-kernel,
devicetree, linux-kernel, Igor Paunovic
The NPU zone has had only a critical trip at 115 degrees, which is a
shutdown and not a cooling policy. Now that the NPU can be throttled by
capping its clock, give the zone a passive trip and a cooling map, in the
same shape and at the same temperatures as the GPU zone right above it:
85 degrees with 2 degrees of hysteresis, and a 100 ms passive polling
delay.
The #cooling-cells property goes on rknn_core_0, the first of the three
core nodes, as the one node that stands for the shared clock as a cooling
device. The other two cores have no clock of their own and cannot be
throttled independently of it.
The cooling map is inert until the driver registers a cooling device:
thermal_of_should_bind() only resolves a map entry once a matching cdev
appears, so this patch on its own changes nothing but the trip point. On a
board that leaves the NPU disabled the passive trip has no cooling device
to act on and only changes the polling rate above 85 degrees, as the GPU
zone's trip already does on a board without the GPU.
The thermal path itself has not been exercised on the board this was
written on. Reaching 85 degrees on an NPU workload with the fan curve here
has not been possible, so what is verified is that the zone parses and
binds, not that throttling engages at temperature.
Assisted-by: LLM checkpatch dtbs_check
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
v2: commit message only: wording for the shared table, and a sentence
on boards that leave the NPU disabled. The diff is unchanged.
arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
index 376ad04e07869..c3d22b08f415b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
@@ -1162,6 +1162,7 @@ rknn_core_0: npu@fdab0000 {
clock-names = "aclk", "hclk", "npu", "pclk";
assigned-clocks = <&scmi_clk SCMI_CLK_NPU>;
assigned-clock-rates = <200000000>;
+ #cooling-cells = <2>;
resets = <&cru SRST_A_RKNN0>, <&cru SRST_H_RKNN0>;
reset-names = "srst_a", "srst_h";
power-domains = <&power RK3588_PD_NPUTOP>;
@@ -3212,17 +3213,31 @@ map0 {
};
npu_thermal: npu-thermal {
- polling-delay-passive = <0>;
+ polling-delay-passive = <100>;
polling-delay = <0>;
thermal-sensors = <&tsadc 6>;
trips {
+ npu_alert: npu-alert {
+ temperature = <85000>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+
npu_crit: npu-crit {
temperature = <115000>;
hysteresis = <0>;
type = "critical";
};
};
+
+ cooling-maps {
+ map0 {
+ trip = <&npu_alert>;
+ cooling-device =
+ <&rknn_core_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+ };
+ };
};
};
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread