[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

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

[Nicolas Dufresne]

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

Hi there,

please note, I didn't submit as I didn't finish learning the implication of
everything in the series. I've used AI because I needed something quick and
dirty for a demo. But let me ask few questions here though:

Le vendredi 04 septembre 2026 à 15:08 +0200, Igor Paunovic a écrit :
> 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 goal of DT is to describe the hardware. You made the choice to not describe
the rate of core 1 and 2, and also are missing something to describe the clock
relation (well indirectly you can probably notice they point to the same clock).
My impression, and I was to study this properly is that having the same table
on every core and adding the opp-shared set on it was actually probably the
proper way to describe this "single clock for all" relationship.

My driver implementation though hard coded this fact for simplicity, but if we
add a variant in the future that does not have this limitation, we can just read
the opp-shared property to differentiate them instead of coding it for every
compatibles. Matching clock to be the same would also be an option, but more
work. I'm curious what's the right approach, and what is the real meaning of
opp-shared if I got that wrong.

> 
> 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.

We must not justify our DTS choices based on driver behaviours (or miss-
behaviour). We must justify it based on how accurate the hardware description
is. In my attempt, I was unable to go back to 200MHz and I could only resume at
200MHz (could have been a bug ...). So opp-suspend described the rate the core
will be once resumed. But it goes a little confused, as resume/suspend isn't per
core. I'm also curious the exact meaning of opp-suspend, and if my
interpretation was right or wrong.

It should probably be fine to not use opp-suspend, if transition back to 200Mhz
works. It not fine if its to avoid a driver deadlock (argually due to a bug).

> 
> 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.

This is irrelevant, I think you can drop this paragraph.

> 
> 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.

Ack, this is safe thing to do.

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Igor Paunovic]

On Mon, 2026-09-08 at 15:44 -0400, Nicolas Dufresne wrote:

Thank you for taking the time on this - and no worries about the PoC not
being posted; the credit in the commit message was deliberate, the rates
and voltages really were arrived at twice independently.

> My impression, and I was to study this properly is that having the same
> table on every core and adding the opp-shared set on it was actually
> probably the proper way to describe this "single clock for all"
> relationship.
> [...]
> I'm curious what's the right approach, and what is the real meaning of
> opp-shared if I got that wrong.

You got it right, and I got it wrong. The binding says (opp-v2-base.yaml):

  opp-shared: Indicates that device nodes using this OPP Table Node's
  phandle switch their DVFS state together, i.e. they share
  clock/voltage/current lines. Missing property means devices have
  independent clock/voltage/current lines, but they share OPP tables.

That is exactly the hardware here: one compute clock and one supply for
all three cores. And it is not only documentation for non-CPU devices
either: in drivers/opp/of.c, _managed_opp() only lets several devices
share a single opp_table instance when the table node carries opp-shared;
without it each device that points at the same node gets its own table.

So the accurate description is the one you had: the same phandle on
rknn_core_0/1/2 plus opp-shared on the table. Putting the table on core 0
alone describes core 0 and says nothing about the other two, which is a
worse description of the same hardware. I will change this in v2 unless a
DT maintainer disagrees.

One consequence is mine to fix on the driver side, not yours to work
around in DT: the driver currently picks the core that carries the table
by taking the first core whose node has operating-points-v2. With three
carriers that choice - and with it the name under /sys/class/devfreq -
would follow whichever core bound first. That is a driver bug the moment
the DT stops being lopsided, so it gets fixed in the same v2.

> We must not justify our DTS choices based on driver behaviours (or miss-
> behaviour). We must justify it based on how accurate the hardware
> description is.
> [...]
> It should probably be fine to not use opp-suspend, if transition back to
> 200Mhz works. It not fine if its to avoid a driver deadlock (argually due
> to a bug).

Accepted, and it is a fair hit. The paragraph as written justifies a DT
choice with a driver limitation, and that is backwards regardless of
whether the limitation is real. It comes out in v2.

On the meaning: your interpretation matches mine, and it is stronger than
"descriptive". The binding says opp-suspend "marks the OPP to be used
during device suspend", and the devfreq core acts on it directly -
devfreq_add_device() reads it into devfreq->suspend_freq, and
devfreq_suspend_device() then sets that rate.

Which leaves the question you actually asked, so I measured it rather than
argued it. On an Orange Pi 5 Plus with this series applied, in-tree rocket,
the OPP table of 3/7 read out of DT, simple_ondemand with min_freq/max_freq
left alone: 25 s of inference, then 60 s idle.

  - The governor takes it back down on its own. trans_stat records six
    transitions for the run - 200->1000, 1000->800, 800->1000, 1000->900,
    900->500, 500->200 - and then 60129 ms at 200 MHz with zero
    milliseconds at any other level for the rest of the window. The step
    down happened within one 250 ms sample of the load ending.

  - The transition completed, it was not merely requested. vdd_npu_s0 goes
    700 -> 850 mV under load and sits flat at 700 mV for the whole idle
    window. In _set_opp(), when scaling down, config_regulators() runs only
    after config_clks() has returned success, so the supply could not have
    come back down to the 200 MHz voltage if the SCMI clock set had failed.
    All four voltages of the table were exercised: 700, 750, 800 and 850 mV.

  - The clock summary in debugfs agrees, reading 200000000 for scmi_clk_npu
    after the load. I only sampled it after the load, so I am offering it as
    consistent rather than as an independent check.

So the transition back works here, and by your own criterion it is fine not
to use opp-suspend. This is one board and one part, so I would not call it
more than that.

Two things I want to keep apart rather than join with a "therefore", because
joining them is what made the original paragraph wrong:

  - What guarantees the rate across suspend is the driver, not the governor
    and not the DT. rocket_devfreq_suspend() in 5/7 sets the recorded boot
    rate itself on the system suspend path, and 4/7 restores it before the
    last core goes down and on .shutdown. That is the answer to "is it safe
    without opp-suspend".

  - Whether the governor walks back down to 200 MHz when the NPU goes idle
    is a separate fact, and it is the one you asked me to check. It does.

opp-suspend acts on the first of those. Since the driver already puts the
device back at its boot rate on that path, the property has nothing left to
do here - and that, rather than any deadlock, is the argument v2 will make.

Worth saying plainly: every number in the cover was taken with the limits
pinned by hand, so this is the first time the governor was left to decide
anything on this board. Your question is what exposed that.

> This is irrelevant, I think you can drop this paragraph.

Agreed, dropped in v2.

> Ack, this is safe thing to do.

Thanks.

Regards,
Igor

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

[Nicolas Dufresne]

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

Hi,

Le mercredi 09 septembre 2026 à 11:18 +0200, Igor Paunovic a écrit :
> On Mon, 2026-09-08 at 15:44 -0400, Nicolas Dufresne wrote:
> 
> Thank you for taking the time on this - and no worries about the PoC not
> being posted; the credit in the commit message was deliberate, the rates
> and voltages really were arrived at twice independently.
> 
> > My impression, and I was to study this properly is that having the same
> > table on every core and adding the opp-shared set on it was actually
> > probably the proper way to describe this "single clock for all"
> > relationship.
> > [...]
> > I'm curious what's the right approach, and what is the real meaning of
> > opp-shared if I got that wrong.
> 
> You got it right, and I got it wrong. The binding says (opp-v2-base.yaml):
> 
>   opp-shared: Indicates that device nodes using this OPP Table Node's
>   phandle switch their DVFS state together, i.e. they share
>   clock/voltage/current lines. Missing property means devices have
>   independent clock/voltage/current lines, but they share OPP tables.
> 
> That is exactly the hardware here: one compute clock and one supply for
> all three cores. And it is not only documentation for non-CPU devices
> either: in drivers/opp/of.c, _managed_opp() only lets several devices
> share a single opp_table instance when the table node carries opp-shared;
> without it each device that points at the same node gets its own table.
> 
> So the accurate description is the one you had: the same phandle on
> rknn_core_0/1/2 plus opp-shared on the table. Putting the table on core 0
> alone describes core 0 and says nothing about the other two, which is a
> worse description of the same hardware. I will change this in v2 unless a
> DT maintainer disagrees.

Great, let's hope a maintainer talks sooner then later, but it really looks
like the right approach.

> 
> One consequence is mine to fix on the driver side, not yours to work
> around in DT: the driver currently picks the core that carries the table
> by taking the first core whose node has operating-points-v2. With three
> carriers that choice - and with it the name under /sys/class/devfreq -
> would follow whichever core bound first. That is a driver bug the moment
> the DT stops being lopsided, so it gets fixed in the same v2.

I was not aware of the component framework, but some of the gpu driver, and
recently some of the media drivers are using this framework to facilitate
the initilization of n-cores in one combined device. Just food for the mind
here.

https://lore.kernel.org/all/20260810-rkvdec-multicore-v2-4-986f89d22cdc@collabora.com/

> 
> > We must not justify our DTS choices based on driver behaviours (or miss-
> > behaviour). We must justify it based on how accurate the hardware
> > description is.
> > [...]
> > It should probably be fine to not use opp-suspend, if transition back to
> > 200Mhz works. It not fine if its to avoid a driver deadlock (argually due
> > to a bug).
> 
> Accepted, and it is a fair hit. The paragraph as written justifies a DT
> choice with a driver limitation, and that is backwards regardless of
> whether the limitation is real. It comes out in v2.
> 
> On the meaning: your interpretation matches mine, and it is stronger than
> "descriptive". The binding says opp-suspend "marks the OPP to be used
> during device suspend", and the devfreq core acts on it directly -
> devfreq_add_device() reads it into devfreq->suspend_freq, and
> devfreq_suspend_device() then sets that rate.
> 
> Which leaves the question you actually asked, so I measured it rather than
> argued it. On an Orange Pi 5 Plus with this series applied, in-tree rocket,
> the OPP table of 3/7 read out of DT, simple_ondemand with min_freq/max_freq
> left alone: 25 s of inference, then 60 s idle.
> 
>   - The governor takes it back down on its own. trans_stat records six
>     transitions for the run - 200->1000, 1000->800, 800->1000, 1000->900,
>     900->500, 500->200 - and then 60129 ms at 200 MHz with zero
>     milliseconds at any other level for the rest of the window. The step
>     down happened within one 250 ms sample of the load ending.
> 
>   - The transition completed, it was not merely requested. vdd_npu_s0 goes
>     700 -> 850 mV under load and sits flat at 700 mV for the whole idle
>     window. In _set_opp(), when scaling down, config_regulators() runs only
>     after config_clks() has returned success, so the supply could not have
>     come back down to the 200 MHz voltage if the SCMI clock set had failed.
>     All four voltages of the table were exercised: 700, 750, 800 and 850 mV.
> 
>   - The clock summary in debugfs agrees, reading 200000000 for scmi_clk_npu
>     after the load. I only sampled it after the load, so I am offering it as
>     consistent rather than as an independent check.
> 
> So the transition back works here, and by your own criterion it is fine not
> to use opp-suspend. This is one board and one part, so I would not call it
> more than that.

If you can script the test, I can run it on Rock5B later on.

> 
> Two things I want to keep apart rather than join with a "therefore", because
> joining them is what made the original paragraph wrong:
> 
>   - What guarantees the rate across suspend is the driver, not the governor
>     and not the DT. rocket_devfreq_suspend() in 5/7 sets the recorded boot
>     rate itself on the system suspend path, and 4/7 restores it before the
>     last core goes down and on .shutdown. That is the answer to "is it safe
>     without opp-suspend".
> 
>   - Whether the governor walks back down to 200 MHz when the NPU goes idle
>     is a separate fact, and it is the one you asked me to check. It does.
> 
> opp-suspend acts on the first of those. Since the driver already puts the
> device back at its boot rate on that path, the property has nothing left to
> do here - and that, rather than any deadlock, is the argument v2 will make.
> 
> Worth saying plainly: every number in the cover was taken with the limits
> pinned by hand, so this is the first time the governor was left to decide
> anything on this board. Your question is what exposed that.

I don't want to pretend the doc is ambiguous for this one, but its not 100%
fit, but is not 100% unfit either.

- opp-suspend: Marks the OPP to be used during device suspend. If multiple OPPs
  in the table have this, the OPP with highest opp-hz will be used.

I don't think we had to transition to 200MHz before suspending, but it
will be at 200MHz once resumed. I think some maintainer feedback here would
be helpful.

> 
> > This is irrelevant, I think you can drop this paragraph.
> 
> Agreed, dropped in v2.
> 
> > Ack, this is safe thing to do.
> 
> Thanks.
> 
> Regards,
> Igor

thanks for working on this.

cheers,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Igor Paunovic]

Hi Nicolas,

> If you can script the test, I can run it on Rock5B later on.

Here it is. I ran this exact script on my board before sending it, so it
is the same code, not a cleaned-up retelling of what I did.

It answers one question and refuses to answer anything else. Two modes:

  ./rk3588-npu-devfreq-check.sh --check     # discovery and gates only
  NPU_LOAD_CMD='...' ./rk3588-npu-devfreq-check.sh

The load command is yours - anything that keeps the NPU busy for about
25 s and exits. I did not want to ship my benchmark, both because it is
not interesting and because the point is that the result should not
depend on which load you use.

The gates matter more than the measurement, so let me say what they do.
It refuses to run if the governor is not simple_ondemand, if min_freq or
max_freq are pinned (then the governor decides nothing and the number is
meaningless), or if the loaded rocket module is not the one belonging to
the running kernel. That last one caught me: my everyday kernel carries
an out-of-tree rocket with its own OPP list built by dev_pm_opp_add()
with no voltages, and no opp-table-npu in DT at all. A measurement there
would have described a different program. It warns rather than refuses
when there is no DT table, or when the table carries opp-suspend, since
both change what is being measured rather than invalidating it.

Everything is discovered, nothing is hardcoded: the devfreq device by
name, the lowest and highest OPP from available_frequencies, the supply
by regulator name. That last one is not paranoia - on my board the NPU
regulator was regulator.2, .6, .7 and .8 across four boots today.

It needs no sudo. Passwordless sudo, if present, only adds a clock
summary sample from debugfs; without it the script says so and goes on.

The verdict is deliberately narrow. It looks only at the window after
the load stops, and it wants the device to reach the lowest OPP and stay
there, not merely touch it - simple_ondemand dips to the bottom between
batches while the load is still running, and an earlier version of this
script counted that as success. It also prints the trans_stat delta for
the idle window, which is the part I would trust most if we disagree.

If it says INVALID, that is the intended outcome for a run that cannot
support a conclusion, not a bug.

What it says here, on an Orange Pi 5 Plus with the series applied,
in-tree rocket, the 3/7 table read out of DT, 25 s of load then 60 s
idle:

  under load : peak 1000 MHz, 700-850 mV
  after load : 200 MHz immediately, held for the whole 59.8 s, 700 mV
  trans_stat : +60055 ms at 200 MHz in the idle window, 0 ms elsewhere
  RESULT: YES

That is worth one remark. I had already measured this earlier today with
a different script of my own, which reported 60129 ms at 200 MHz for the
same window. Two differently written tools, the same answer to within
their sampling noise - which is a better reason to believe it than one
tool run twice. Neither of them, though, is a second board, which is why
your offer is worth more than either.

The script follows below.

Igor
---

#!/bin/bash
# Does the NPU devfreq governor return to the lowest OPP after the load stops?
#
# Written for the discussion on "[PATCH 3/7] arm64: dts: rockchip: rk3588: add an
# OPP table for the NPU", where the question was whether opp-suspend is needed.
# It answers exactly that one question and nothing else.
#
#   ./rk3588-npu-devfreq-check.sh --check          # discovery + gates only, no load
#   NPU_LOAD_CMD='...' ./rk3588-npu-devfreq-check.sh
#
# The load command is yours: anything that keeps the NPU busy for LOAD_S seconds
# and then exits. It is run in the foreground and its exit status is checked.
#
# Requires: bash, python3, awk. No sudo for the measurement itself; sudo is used
# only to read the clock summary in debugfs, and the script works without it.
set -u

CHECK_ONLY=0
[ "${1:-}" = "--check" ] && CHECK_ONLY=1

LOAD_S=${LOAD_S:-25}          # how long the load runs
IDLE_S=${IDLE_S:-60}          # how long we watch after it stops
SETTLE_S=${SETTLE_S:-20}      # how long it must stay at the low OPP to count
OUT=${OUT:-$PWD/npu-devfreq-$(date +%Y%m%d-%H%M%S)}

die() { echo "FAIL: $*" >&2; exit 1; }
ok()  { echo "  ok   $*"; }
note(){ echo "  --   $*"; }

echo "=== rk3588 npu devfreq check, $(date '+%F %T %Z') ==="
echo "kernel: $(uname -r)"

# ---------------------------------------------------------------- discovery
D=""
for d in /sys/class/devfreq/*; do
  [ -e "$d/available_frequencies" ] || continue
  case "$(basename "$d")" in *npu*) D="$d"; break;; esac
done
[ -n "$D" ] || die "no NPU devfreq device under /sys/class/devfreq"
ok "devfreq device: $D"

MINF=$(tr ' ' '\n' < "$D/available_frequencies" | grep -v '^$' | sort -n | head -1)
MAXF=$(tr ' ' '\n' < "$D/available_frequencies" | grep -v '^$' | sort -n | tail -1)
ok "OPPs: $(tr ' ' '\n' < "$D/available_frequencies" | grep -cv '^$') steps, $((MINF/1000000))-$((MAXF/1000000)) MHz"

REG=""
for f in /sys/class/regulator/*/name; do
  case "$(cat "$f" 2>/dev/null)" in *npu*) REG=$(dirname "$f"); break;; esac
done
if [ -n "$REG" ]; then ok "supply: $(cat "$REG/name") = $(cat "$REG/microvolts") uV  ($REG)"
else note "no NPU regulator found by name - voltage will not be sampled"; fi

SUDO=0; sudo -n true 2>/dev/null && SUDO=1
[ $SUDO = 1 ] && ok "sudo available - clock summary will be sampled" \
              || note "no passwordless sudo - clock summary will be skipped"

# ---------------------------------------------------------------- gates
GOV=$(cat "$D/governor"); MIN=$(cat "$D/min_freq"); MAX=$(cat "$D/max_freq")
[ "$GOV" = simple_ondemand ] || die "governor is '$GOV'; this test only means something with simple_ondemand"
ok "governor: $GOV"
[ "$MIN" = "$MINF" ] && [ "$MAX" = "$MAXF" ] \
  || die "min_freq/max_freq are pinned ($MIN/$MAX); the governor decides nothing. Reset them first."
ok "limits not pinned: $MIN / $MAX"

DTOPP=/proc/device-tree/opp-table-npu
[ -d "$DTOPP" ] && ok "OPP table comes from DT ($DTOPP)" \
                || note "no $DTOPP - the driver is building its own OPP list, results describe that instead"
if [ -d "$DTOPP" ] && ls "$DTOPP"/opp-*/opp-suspend >/dev/null 2>&1; then
  note "the DT table carries opp-suspend - that changes what is being measured"
fi

SRC_LIVE=$(cat /sys/module/rocket/srcversion 2>/dev/null || echo -)
SRC_TREE=$(modinfo -F srcversion "/lib/modules/$(uname -r)/kernel/drivers/accel/rocket/rocket.ko" 2>/dev/null || echo -)
if [ "$SRC_LIVE" != - ] && [ "$SRC_TREE" != - ] && [ "$SRC_LIVE" != "$SRC_TREE" ]; then
  die "the loaded rocket module ($SRC_LIVE) is not this kernel's ($SRC_TREE) - out-of-tree module in the way"
fi
ok "rocket module matches the running kernel"

if [ $CHECK_ONLY = 1 ]; then
  echo
  echo "=== --check only: everything above passed, no load was run ==="
  echo "To measure, give it a load command, for example:"
  echo "  NPU_LOAD_CMD='your-inference-tool --seconds $LOAD_S' $0"
  exit 0
fi

[ -n "${NPU_LOAD_CMD:-}" ] || die "set NPU_LOAD_CMD to something that keeps the NPU busy for ~${LOAD_S}s and exits"

# ---------------------------------------------------------------- measure
mkdir -p "$OUT" || die "cannot create $OUT"
cp "$D/trans_stat" "$OUT/trans_stat.begin" 2>/dev/null

SAMPLES=$OUT/samples.txt; : > "$SAMPLES"
( while :; do
    printf '%s %s %s\n' "$(date +%s.%N)" "$(cat "$D/cur_freq" 2>/dev/null || echo 0)" \
      "$([ -n "$REG" ] && cat "$REG/microvolts" 2>/dev/null || echo 0)" >> "$SAMPLES"
    sleep 0.2
  done ) &
SPID=$!
cleanup(){ kill $SPID 2>/dev/null; }
trap 'cleanup; echo; echo "INTERRUPTED - result is not valid"; exit 130' INT TERM
trap cleanup EXIT

echo
echo "--- load: $NPU_LOAD_CMD"
T0=$(date +%s.%N)
if ! eval "$NPU_LOAD_CMD" > "$OUT/load.log" 2>&1; then
  cleanup; die "the load command exited non-zero - see $OUT/load.log"
fi
T1=$(date +%s.%N)
cp "$D/trans_stat" "$OUT/trans_stat.load_end" 2>/dev/null
[ $SUDO = 1 ] && sudo -n cat /sys/kernel/debug/clk/clk_summary 2>/dev/null | grep -i npu > "$OUT/clk.load_end"

echo "--- load ran for $(python3 -c "print(f'{$T1-$T0:.1f}')") s; now watching for ${IDLE_S}s, keep the machine idle"
sleep "$IDLE_S"
T2=$(date +%s.%N)
cp "$D/trans_stat" "$OUT/trans_stat.end" 2>/dev/null
[ $SUDO = 1 ] && sudo -n cat /sys/kernel/debug/clk/clk_summary 2>/dev/null | grep -i npu > "$OUT/clk.end"
cleanup; trap - EXIT INT TERM

python3 - "$SAMPLES" "$T0" "$T1" "$T2" "$SETTLE_S" "$IDLE_S" "$LOAD_S" "$MINF" <<'PY' | tee "$OUT/verdict.txt"
import sys
f,t0,t1,t2,settle,idle_s,load_s,minf = sys.argv[1], *map(float,sys.argv[2:8]), int(sys.argv[8])
S=[]
for line in open(f):
    p=line.split()
    if len(p)==3:
        try: S.append((float(p[0]),int(p[1]),int(p[2])))
        except ValueError: pass
load=[x for x in S if t0<=x[0]<=t1]; post=[x for x in S if x[0]>t1]
mhz=lambda v: v//1000000
bad=[]
if (t1-t0) < 0.9*load_s: bad.append(f"load ran {t1-t0:.1f}s, expected ~{load_s:.0f}s")
if not post: bad.append("no samples after the load")
elif post[-1][0]-t1 < 0.95*idle_s: bad.append(f"idle window only {post[-1][0]-t1:.1f}s of {idle_s:.0f}s")
peak = max((x[1] for x in load), default=0)
print()
print(f"under load : peak {mhz(peak)} MHz, {len(load)} samples"
      + (f", {min(x[2] for x in load)//1000}-{max(x[2] for x in load)//1000} mV" if load and load[0][2] else ""))
first=tail=None
if post:
    t=post[0][0]
    for ts,v,u in post:
        if v==minf: first=ts-t; break
    tail=0.0
    for ts,v,u in reversed(post):
        if v==minf: tail=post[-1][0]-ts
        else: break
    print(f"after load : first {mhz(minf)} MHz after {'never' if first is None else f'{first:.1f}s'}, "
          f"continuously at {mhz(minf)} MHz for the last {tail:.1f}s (need >= {settle:.0f}s)"
          + (f", {min(x[2] for x in post)//1000}-{max(x[2] for x in post)//1000} mV" if post[0][2] else ""))
print()
if bad:
    print("RESULT: INVALID -", "; ".join(bad))
elif peak <= minf:
    print(f"RESULT: INVALID - the NPU never went above {mhz(minf)} MHz; the load did not reach it")
elif first is not None and tail>=settle:
    print(f"RESULT: YES - the governor returned to {mhz(minf)} MHz on its own and stayed there")
elif first is not None:
    print(f"RESULT: PARTIAL - it reached {mhz(minf)} MHz but did not stay; try a longer IDLE_S")
else:
    print(f"RESULT: NO - it did not return to {mhz(minf)} MHz within {idle_s:.0f}s")
PY

echo
echo "trans_stat delta (ms per OPP, idle window only):"
python3 - "$OUT/trans_stat.load_end" "$OUT/trans_stat.end" <<'PY'
import sys
def parse(p):
    out={}
    try:
        for l in open(p):
            l=l.strip().lstrip('*').strip()
            if ':' in l and l.split(':')[0].strip().isdigit():
                k=int(l.split(':')[0]); c=l.split(':')[1].split()
                if c: out[k]=int(c[-1])
    except OSError: pass
    return out
a,b=parse(sys.argv[1]),parse(sys.argv[2])
for k in sorted(set(a)|set(b)):
    d=b.get(k,0)-a.get(k,0)
    if d: print(f"  {k//1000000:>5} MHz: +{d} ms")
PY
echo
echo "everything written to: $OUT"

[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