[PATCH 0/7] accel/rocket: DVFS for the RK3588 NPU

[PATCH 0/7] accel/rocket: DVFS for the RK3588 NPU

[Igor Paunovic]

The rocket driver runs the NPU at whatever rate the devicetree pinned it to.
On the RK3588 that is 200 MHz, out of the 1 GHz the hardware reaches, and the
firmware will change it on request. This series adds devfreq so the driver
scales the clock with the load, plus the OPP table and the thermal plumbing
that go with it.

Hardware constraint
-------------------

The three cores share one clock and one supply, so they cannot be scaled
apart, and there is exactly one devfreq device for all of them.

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. Lowering the rate is safe at any
time: the firmware serves the boot rate from GPLL and writes only CRU clock
selectors to get there.

The firmware also accepts only the rates in its own PVTPLL table - 300 to
1000 MHz in 100 MHz steps, plus 200 MHz off GPLL. Any other rate is
rejected outright, which is why patch 3/7 names exactly those nine.

Design
------

The devfreq device hangs off the core that carries the OPP table in the
devicetree, found through the property and not through the core index: the
index is handed out in probe order, which is not the order the cores are
written in.

Before the rate goes up, every core is runtime resumed, and those references
are held for as long as the clock stays raised. While they are held no core
can suspend, so no island can transition at all - that is what makes a
raised clock safe rather than merely unlikely to be caught out. At the boot
rate the references are dropped and runtime PM behaves as it did before.
The cost is that a boosted NPU does not power-gate individual cores; what
that costs in milliwatts has not been measured, and I would rather say so
than guess.

Utilisation is aggregated as the maximum over the cores rather than the sum,
because the clock has to satisfy the busiest of them.

devfreq_suspend_device() and devfreq_resume_device() are deliberately not
called, which is a deviation from panfrost, panthor, lima and msm. See the
commit message of 5/7: they end in cancel_delayed_work_sync() on the
governor worker, and that worker is what calls back into ->target(), which
resumes every core.

Testing
-------

Measured on an Orange Pi 5 Plus with this series applied: MobileNetV1
through Teflon, three 20-second blocks per arm, pinned to one CPU, with a
bit-exact oracle checked on every inference.

  200 MHz, userspace      91.1 inf/s    rail 700 mV
  1000 MHz, userspace    234.0 inf/s    rail 850 mV
  simple_ondemand        231.2 inf/s    rail 700-850 mV
  200 MHz again           91.0 inf/s    (drift 0.999)

That is 2.57x for the fixed maximum and 2.54x for the governor, which
costs 1.2 % against pinning. The oracle passed bit-exact in all four arms,
the interrupt count per inference was identical in all four, and the
kernel log has nothing from this driver for the whole run.

The rail voltages are read back from vdd_npu_s0 inside each arm. Nothing
in the test sets them: they are the OPP core following the table in 3/7.
In a separate run the power islands were cycled five times at 700 mV
without an SError, and at 1000 MHz all three cores stay runtime resumed,
0 of 3 suspended, which is the hold described under Design doing its job.

Under simple_ondemand the governor spent most of the run at 900 MHz rather
than at 1000 - 290 samples against 145, three transitions - and still
landed within 1.2 % of the pinned maximum.

Everything above is one inference thread, which cannot see a fault that
only appears when several cores compute at once. Jiaxing Hu found exactly
that on the RK3576, where two cores in flight together corrupt single
words of the second core's output, and the cure is voltage: the rate his
board came up at wanted 800 mV and had 750. So the same oracle was run
again with three concurrent clients, each checking its own output
bit-exact, at the two top rates in the table:

  900 MHz, rail 800 mV    3 x 199 inf/s, 598 total, all bit-exact
  1000 MHz, rail 850 mV   3 x 204 inf/s, 611 total, all bit-exact

Single-client control on the same rates was 232 and 240 inf/s, so the
aggregate is 2.57x and 2.55x of one client - the cores really were
computing together, which is what makes the bit-exact result mean
anything. Rail voltages read back during the run, set by the OPP core.

kernel 7.3.0-rc1 plus this series, rocket srcversion
C1FE916B551FCF00F0B2D81, BL31 v2.12.0-10-g70d814213.

For anyone comparing against numbers I posted earlier: an out-of-tree
devfreq module on 7.2.0-rc7 measured 2.47x for the governor. The
difference is not the NPU. Time spent NPU-side per inference is the same
to within 1 % (3.02 ms then, 3.00 ms now); what moved is the CPU share of
the wall clock, 32.5 % down to 29.5 %. The ratio improved because the host
side got cheaper, not because the accelerator got faster.

Build-tested with W=1, with sparse, and with CONFIG_DEVFREQ_THERMAL=n, at
every patch in the series and not only at the tip; dt_binding_check and
CHECK_DTBS pass on the binding and on both RK3588 boards here. checkpatch
--strict is clean on six of the seven; on 5/7 it asks whether MAINTAINERS
needs updating for the new files, which it does not - the driver is already
covered by its existing entry.

Not done, and worth saying: the thermal path has not been exercised. 85
degrees could not be reached on an NPU workload with the fan curve on this
board, so what is verified about 7/7 is that the zone parses and binds, not
that throttling engages at temperature.

Routing
-------

The driver patches and the binding go through drm-misc; the two dts patches
(3/7 and 7/7) belong to Heiko's rockchip tree.

Ordering matters: the binding has additionalProperties: false, so 3/7 and
7/7 must not land before 2/7 or CHECK_DTBS breaks. The reverse asymmetry is
harmless - without an OPP table in the devicetree the driver simply returns
without a devfreq device, and a cooling map with no cooling device is left
unresolved by thermal_of_should_bind().

If it is easier, I am happy to resend the two dts patches separately once
the driver side has landed.

Dependency
----------

This series needs "accel/rocket: search every core slot when a core is
removed", sent separately as a fix:
https://lore.kernel.org/linux-rockchip/20260904125936.26234-1-royalnet026@gmail.com/
5/7 keys the devfreq setup on rdev->max_cores, and that field arrives with
the fix rather than with this series. Without it 5/7 does not build.

1/7 is also carried as 01/14 in Jiaxing Hu's RK3576 series; whichever lands
first, the other drops it. It is included here so this series is
self-contained and reviewable on its own.

The series applies to drm-misc-next. On top of that RK3576 series, 2/7
through 4/7 apply with git am -3 as they are, and 5/7 needs only its two
job-path hooks moved into rocket_job_next_locked(), which that series
factors out. I am happy to do that rebase in whichever order suits.

Questions
---------

Q1. Where should the OPP table live when three devices share a clock? It is
    on rknn_core_0 here, because the devfreq device hangs off that core and
    of_devfreq_cooling_register_power() takes dev->of_node. The alternative
    is all three nodes with opp-shared, which has a precedent on this very
    SoC in cluster0_opp_table. I do not have a strong opinion.

Q2. Is maximum-over-cores the right aggregation, or should it be a summed
    busy count?

Q3. The driver returns without devfreq when there is no OPP table, rather
    than failing probe. That matches panfrost and lima. Is that the policy
    you want here?

Q4. The governor thresholds are a starting point copied from the other
    accelerators, not a measurement. Inference workloads have not been
    profiled against them.

Q5. There is no energy model: the NPU has no measured
    dynamic-power-coefficient and I would rather ship none than invent one.
    Throttling is therefore step-wise.

Q6. All three core nodes name the same npu-supply, but only the core with
    the OPP table hands it to the OPP core. Is that worth changing?

Q7. assigned-clock-rates stays on all three nodes, and I am no longer
    sure it should. of_clk_set_defaults() runs on every probe over the
    shared clock, so re-probing one core while devfreq holds a raised
    rate would quietly lower it - which I have not managed to
    reproduce. But Jiaxing Hu reports that on the RK3576 an
    assigned-clock-rates on the SCMI clock in the NPU node hangs the
    board before the console comes up, with no output at all, and that
    the vendor driver never writes that rate from DT either:
    rockchip_opp_config_clks() returns early for an SCMI clock on a
    device that is not runtime active. That is a good deal worse than
    the case I was worried about. Dropping the property is a small
    patch on top, and with the OPP table carrying the rate I do not
    think anything here needs it. I have left it in only because a
    devicetree with no OPP table still wants a rate that is correct at
    whatever voltage the board boots with.

Credits
-------

Nicolas Dufresne arrived at the same rates and voltages independently in a
proof of concept he never posted, and said to take whatever was useful from
it. His version differs: opp-suspend on the lowest entry, one shared table
across all three cores, and no assigned-clock-rates pins.
Link: https://gitlab.collabora.com/nicolas/linux/-/commits/rock5b-npu-poc-4

Tomeu Vizoso agreed to the full-range table with a per-board cap.

Jiaxing Hu carried 1/7 in the RK3576 series. If you have RK3576 hardware, I
would be glad of a test of 4/7 there: it reads the boot rate rather than
assuming one, which should be the right behaviour on a devicetree that does
not pin the clock.

The Assisted-by: LLM tags on the patches are Claude Opus 5. We wrote the
driver code and these commit messages together, over a long back-and-forth
in which it talked me out of more than one design before it reached a
compiler, and in which I threw out plenty of what it proposed. The board,
every measurement and every boot in Testing above, the decision to send
this, and the responsibility for all of it are mine.

Igor Paunovic (7):
  accel/rocket: request the core clocks by name
  dt-bindings: npu: rockchip: allow DVFS and thermal properties
  arm64: dts: rockchip: rk3588: add an OPP table for the NPU
  accel/rocket: restore the NPU clock boot rate before powering the
    cores down
  accel/rocket: add devfreq support
  accel/rocket: register a devfreq cooling device
  arm64: dts: rockchip: rk3588: add passive cooling to the NPU thermal
    zone

 .../npu/rockchip,rk3588-rknn-core.yaml        |  10 +
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi |  17 +-
 arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi  |  45 ++
 drivers/accel/rocket/Kconfig                  |   2 +
 drivers/accel/rocket/Makefile                 |   1 +
 drivers/accel/rocket/rocket_core.c            |  16 +
 drivers/accel/rocket/rocket_core.h            |  11 +
 drivers/accel/rocket/rocket_devfreq.c         | 482 ++++++++++++++++++
 drivers/accel/rocket/rocket_devfreq.h         |  57 +++
 drivers/accel/rocket/rocket_device.h          |  13 +
 drivers/accel/rocket/rocket_drv.c             | 110 +++-
 drivers/accel/rocket/rocket_job.c             |   7 +
 12 files changed, 763 insertions(+), 8 deletions(-)
 create mode 100644 drivers/accel/rocket/rocket_devfreq.c
 create mode 100644 drivers/accel/rocket/rocket_devfreq.h


base-commit: a9f09b5ea0c3db1e2d4c0f8d3ebdd612d8aa0366
prerequisite-patch-id: 519bcdfdde80d902309c8346f749ebc4bb6b29c0
-- 
2.43.0

[PATCH 1/7] accel/rocket: request the core clocks by name

[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")
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Tested-by: Sidong Yang <sidong.yang@furiosa.ai>
Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, NanoPC-T6 Plus
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Reviewed-by: Jiaxing Hu <gahing@gahingwoo.com>
Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
---
 drivers/accel/rocket/rocket_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
index 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

[PATCH 2/7] dt-bindings: npu: rockchip: allow DVFS and thermal properties

[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 the same node to be usable as
a cooling device, so allow #cooling-cells too.

Both properties belong on the core that carries the shared clock, not on
all three: the cores have no clock of their own and cannot be scaled or
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 that placement, because all three cores share a
compatible string and a node name pattern, so which core carries them
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.

Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM checkpatch dt_binding_check
---
 .../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..595aacfbf8608 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 the core that drives the shared NPU clock, which is the one
+      the operating-points-v2 table below is attached to. The other cores
+      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

Re: [PATCH 2/7] dt-bindings: npu: rockchip: allow DVFS and thermal properties

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 75 bytes --]

Acked-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 3/7] arm64: dts: rockchip: rk3588: add an OPP table for the NPU

[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, so the table 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 and that is the
rate the NPU boots and idles at. Leaving it out would put the boot state
outside the table and give a driver nowhere to return to. 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 attached to rknn_core_0 alone. All three cores share one clock
and one supply and cannot be scaled independently, and the driver hangs its
devfreq device off the core that carries the 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 rejected outright.

There is deliberately no opp-suspend property. The driver has to resume
every core before it may touch the shared clock, so letting the devfreq
core drive a suspend OPP from inside a runtime-suspend callback would
deadlock against the driver's own governor worker. The driver records the
boot rate and restores it itself instead.

The consumer is the devfreq support added later in this series; until then
the table is inert and the NPU keeps the fixed rate that
assigned-clock-rates gives it today.

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, shares one table
across all three cores and drops the assigned-clock-rates pins.
Link: https://gitlab.collabora.com/nicolas/linux/-/commits/rock5b-npu-poc-4

Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM checkpatch dtbs_check
---
 arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi | 45 ++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
index b5d630d2c879f..3711727020ed1 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
@@ -151,6 +151,47 @@ opp-1000000000 {
 			opp-microvolt = <850000 850000 850000>;
 		};
 	};
+
+	npu_opp_table: opp-table-npu {
+		compatible = "operating-points-v2";
+
+		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 +229,7 @@ &cpu_l3 {
 &gpu {
 	operating-points-v2 = <&gpu_opp_table>;
 };
+
+&rknn_core_0 {
+	operating-points-v2 = <&npu_opp_table>;
+};
-- 
2.43.0

[PATCH 4/7] accel/rocket: restore the NPU clock boot rate before powering the cores down

[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 reboot or 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, because the firmware serves the boot rate from GPLL and
writes only CRU clock selectors to get there, never a register inside the
NPU.

Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM sparse checkpatch
---
 drivers/accel/rocket/rocket_core.c   | 12 +++++++++
 drivers/accel/rocket/rocket_device.h | 10 +++++++
 drivers/accel/rocket/rocket_drv.c    | 40 ++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+)

diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
index 5dd260bacbff6..61200e5d5ac0d 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,17 @@ 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_clk) {
+		core->rdev->npu_clk = core->clks[2].clk;
+		core->rdev->npu_boot_rate = clk_get_rate(core->rdev->npu_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 c62d567010696..466ebc4c26a8a 100644
--- a/drivers/accel/rocket/rocket_device.h
+++ b/drivers/accel/rocket/rocket_device.h
@@ -20,6 +20,16 @@ struct rocket_device {
 	struct rocket_core *cores;
 	unsigned int num_cores;
 	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. npu_boot_rate is the
+	 * rate it was left at before the driver touched it, and active_cores
+	 * counts the cores that are runtime resumed right now.
+	 */
+	struct clk *npu_clk;
+	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 2bcfe4ab3c68f..b7199de57ccc7 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -231,6 +231,28 @@ 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: the firmware serves
+ * the boot rate from GPLL and touches only the CRU clock selectors on the way
+ * there, none of the NPU's own registers.
+ */
+static void rocket_npu_restore_boot_rate(struct rocket_device *rdev)
+{
+	int err;
+
+	if (!rdev->npu_clk)
+		return;
+
+	err = clk_set_rate(rdev->npu_clk, rdev->npu_boot_rate);
+	if (err)
+		dev_warn(rdev->cores[0].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);
@@ -246,6 +268,8 @@ static int rocket_device_runtime_resume(struct device *dev)
 		return err;
 	}
 
+	atomic_inc(&rdev->active_cores);
+
 	return 0;
 }
 
@@ -262,6 +286,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);
+
 	return 0;
 }
 
@@ -270,9 +297,22 @@ EXPORT_GPL_DEV_PM_OPS(rocket_pm_ops) = {
 	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
 };
 
+/*
+ * A kexec or a reboot 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);
+
+	if (rdev)
+		rocket_npu_restore_boot_rate(rdev);
+}
+
 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

[PATCH 5/7] accel/rocket: add devfreq support

[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 core
that carries the OPP table in the devicetree, found by looking for the
property rather than by taking core 0: the core index is handed out in
probe order, which is not the order the cores are written in.

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. While they are held no core can
suspend, so no island can transition at all. That is what makes a raised
clock safe rather than merely unlikely to be caught out, and it is why the
guard and the thing it guards arrive in the same commit: no commit in the
tree ever raises the rate without it.

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, and I would rather say so than guess. 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 and clock down
underneath it. Whether that is the right aggregation for a shared clock is
a fair question for review.

The driver does not call devfreq_suspend_device() and
devfreq_resume_device() itself, which is a deviation from panfrost,
panthor, lima and msm. It ends in cancel_delayed_work_sync() on the
governor's own worker, and that worker is what calls back into ->target(),
which resumes every core; from a runtime-suspend callback that waits for a
worker that is waiting for the same callback to finish. An active-core
count narrows the window but does not close it. In its place, ->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 a different matter, and there the call does happen:
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 one that owns the devfreq device.
pm_runtime_force_suspend() powers a core down whatever the usage count
says, and the owning core is suspended last, 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, and would scale that instead of the compute clock. Configuration
and table are set up on the core that carries them in the devicetree,
which is not 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. Leaving it to devres would make a later bind
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, which also means 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 with sysfs reporting a rate the hardware was not running.
The boot-rate restore added in the previous patch is converted accordingly.

->get_cur_freq() and the status callback report the rate this driver last
programmed 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() will not find it and every
transition is logged as unknown.

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.

Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM sparse checkpatch
---
 drivers/accel/rocket/Kconfig          |   2 +
 drivers/accel/rocket/Makefile         |   1 +
 drivers/accel/rocket/rocket_core.h    |  11 +
 drivers/accel/rocket/rocket_devfreq.c | 458 ++++++++++++++++++++++++++
 drivers/accel/rocket/rocket_devfreq.h |  55 ++++
 drivers/accel/rocket/rocket_device.h  |   3 +
 drivers/accel/rocket/rocket_drv.c     |  80 ++++-
 drivers/accel/rocket/rocket_job.c     |   7 +
 8 files changed, 605 insertions(+), 12 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 f6d7382854ca9..7ada497571bf9 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>
 
@@ -55,6 +56,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..9d923a0b6ea30
--- /dev/null
+++ b/drivers/accel/rocket/rocket_devfreq.c
@@ -0,0 +1,458 @@
+// 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 core that carries the OPP
+ * table in the devicetree.
+ *
+ * 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, because the firmware serves the boot
+ * rate from GPLL and only writes CRU clock selectors on the way there. Raising
+ * it is not, so before the rate 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;
+}
+
+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 ten 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) <= rdev->npu_boot_rate) {
+		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, 1),
+		status->current_frequency / 1000 / 1000);
+
+	return 0;
+}
+
+/*
+ * Report what this driver last programmed, 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.
+ */
+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 rate 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, rdev->npu_boot_rate);
+	if (!ret)
+		WRITE_ONCE(rdevfreq->cur_freq, rdev->npu_boot_rate);
+
+	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, rdev->npu_boot_rate);
+	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;
+
+	/*
+	 * Find the core the OPP table is attached to. This has to come from
+	 * the devicetree and not from core->index, because the index is handed
+	 * out in probe order, which is not the order the cores are written in.
+	 */
+	for (i = 0; i < rdev->num_cores; i++) {
+		if (of_property_present(rdev->cores[i].dev->of_node,
+					"operating-points-v2")) {
+			owner = &rdev->cores[i];
+			break;
+		}
+	}
+
+	/*
+	 * 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 core that carries
+	 * the OPP table, 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++)
+		rdev->cores[i].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;
+	}
+
+	/*
+	 * 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().
+	 */
+	ret = dev_pm_opp_set_opp(dev, opp);
+	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, rdev->npu_boot_rate);
+			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..bdf8e89ed3761
--- /dev/null
+++ b/drivers/accel/rocket/rocket_devfreq.h
@@ -0,0 +1,55 @@
+/* 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 OPP table is attached to, and so the one the devfreq
+	 * device hangs off. NULL when the devicetree describes no OPP table.
+	 */
+	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;
+
+	/* 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 466ebc4c26a8a..979ef4685443f 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;
@@ -30,6 +31,8 @@ struct rocket_device {
 	struct clk *npu_clk;
 	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 b7199de57ccc7..779e1cb48c241 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"
@@ -181,15 +182,33 @@ static int rocket_probe(struct platform_device *pdev)
 	rdev->num_cores++;
 
 	ret = rocket_core_init(&rdev->cores[core]);
-	if (ret) {
-		rdev->num_cores--;
-
-		if (rdev->num_cores == 0) {
-			rocket_device_fini(rdev);
-			rdev = NULL;
+	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:
+	rdev->num_cores--;
+
+	if (rdev->num_cores == 0) {
+		rocket_device_fini(rdev);
+		rdev = NULL;
+	}
+
 	return ret;
 }
 
@@ -203,6 +222,9 @@ static void rocket_remove(struct platform_device *pdev)
 	if (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->num_cores--;
 
@@ -246,7 +268,18 @@ static void rocket_npu_restore_boot_rate(struct rocket_device *rdev)
 	if (!rdev->npu_clk)
 		return;
 
-	err = clk_set_rate(rdev->npu_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(rdev->npu_clk, rdev->npu_boot_rate);
+
 	if (err)
 		dev_warn(rdev->cores[0].dev,
 			 "failed to restore the NPU boot rate of %lu Hz: %d\n",
@@ -292,21 +325,44 @@ 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 or a reboot hands the next kernel whatever rate is set here, and
- * that kernel will power the islands up before it looks at the clock.
+ * A kexec or a reboot 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.
  */
 static void rocket_shutdown(struct platform_device *pdev)
 {
 	struct rocket_device *rdev = dev_get_drvdata(&pdev->dev);
 
-	if (rdev)
-		rocket_npu_restore_boot_rate(rdev);
+	if (!rdev)
+		return;
+
+	rocket_devfreq_fini(rdev);
+	rocket_npu_restore_boot_rate(rdev);
 }
 
 static struct platform_driver rocket_driver = {
diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index 3141f210fcd1b..2ba03d85b598e 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"
@@ -151,6 +152,8 @@ static void rocket_job_hw_submit(struct rocket_core *core, struct rocket_job *jo
 
 	rocket_pc_writel(core, OPERATION_ENABLE, PC_OPERATION_ENABLE_OP_EN(1));
 
+	rocket_devfreq_record_busy(core);
+
 	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

[PATCH 6/7] accel/rocket: register a devfreq cooling device

[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 node carries 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.

Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM sparse checkpatch
---
 drivers/accel/rocket/rocket_devfreq.c | 24 ++++++++++++++++++++++++
 drivers/accel/rocket/rocket_devfreq.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/accel/rocket/rocket_devfreq.c b/drivers/accel/rocket/rocket_devfreq.c
index 9d923a0b6ea30..86bc34819a187 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>
@@ -404,6 +405,24 @@ 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 node has 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:
@@ -426,6 +445,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 bdf8e89ed3761..d5876d62a0b7c 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

[PATCH 7/7] arm64: dts: rockchip: rk3588: add passive cooling to the NPU thermal zone

[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 core that carries the
shared clock and the OPP table. 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.

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.

Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM checkpatch dtbs_check
---
 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