Re: [PATCH v2 09/11] accel/rocket: add devfreq support

[Sidong Yang <sidong.yang@furiosa.ai>]

Hi Igor,

On Tue, Sep 22, 2026 at 10:01:12AM +0200, Igor Paunovic wrote:
> The NPU has run at whatever rate the devicetree pinned it to since the
> driver was merged, which on the RK3588 is 200 MHz. The hardware reaches
> 1 GHz, and the firmware will change the rate on request, so let devfreq
> drive it from how busy the cores actually are.
> 
> One devfreq device drives all of the cores, because they have one clock and
> one supply between them and cannot be scaled apart. It hangs off the first
> core in devicetree order that carries an OPP table, which on the RK3588,
> where all three cores reference the shared table, is rknn_core_0. The
> choice has to be fixed rather than "whichever core bound last": the
> devfreq device is named after that core, and a cooling map in the
> devicetree resolves against that core's node. core->index is the core's
> position among the core nodes, so the lowest index is the first node.
> 
> The awkward part is that the clock is generated by a PVTPLL that sits
> inside the NPU power islands. An island powered up while the clock is above
> the rate the bootloader left never acknowledges the power-on, and the first
> register access into it afterwards takes an asynchronous SError. So before
> the rate goes up, every core is runtime resumed, and the references are
> held for as long as the clock stays raised: no island can transition while
> they are held, which is what makes a raised clock safe rather than merely
> unlikely to be caught out. The guard and the thing it guards arrive in the
> same commit, so no commit in the tree ever raises the rate without it.
> 
> Unbinding any core takes the devfreq device down, and it comes back
> once every core is bound again: a rate change resumes all of them, so
> the device has to be whole for the guard to mean anything. Every walk
> of the core array here relies on that.
> 
> The cost is that a boosted NPU does not power-gate individual cores. It is
> paid only above the boot rate; at the boot rate the references are dropped
> and runtime PM behaves as before. What that costs in milliwatts has not
> been measured on this board. A measurement, or a design that does not need
> the references at all, would both be welcome.
> 
> Utilisation is aggregated as the maximum over the cores, not the sum: the
> rate has to satisfy the busiest core, and summing would report one
> saturated core out of three as a third of the load. Whether that is the
> right aggregation for a shared clock is a fair question for review.
> 
> Unlike panfrost, panthor, lima and msm, the driver does not call
> devfreq_suspend_device() from runtime suspend. That call ends in
> cancel_delayed_work_sync() on the governor's worker, and the worker is
> what calls ->target(), which resumes every core: a runtime-suspend
> callback would wait for a worker that is waiting for that same callback.
> Instead ->target() returns early when the rate is unchanged, so a
> governor tick on an idle NPU costs one comparison and resumes nothing.
> 
> System suspend is different: dpm_suspend() runs devfreq_suspend() over
> every registered devfreq before it walks the devices, from a path that
> holds none of this driver's locks. What the driver adds is lowering the
> rate and dropping the references from every core's ->suspend, not only
> the owning core's. pm_runtime_force_suspend() powers a core down whatever
> the usage count says, and the owning core need not be the first one
> suspended, so a suspend that aborts partway would otherwise leave the
> clock raised over islands that are already gated.
> 
> The clock and the supply are claimed in one dev_pm_opp_set_config() call
> before the table is added. Naming the clock there is not optional: with no
> name the OPP core takes the first clock in the node, which is the bus
> clock. Configuration and table live on the owning core, which need not be
> the device being probed at the time, so none of it can be devres: the call
> hands back a token, and rocket_devfreq_fini() releases it, and the table,
> by hand. Left to devres, a later bind would fail on an OPP table the
> previous teardown never emptied.
> 
> The rate is changed through dev_pm_opp_set_rate(), so that the supply moves
> with it. Nothing may set the rate behind the OPP core's back: it caches
> the OPP it last applied and skips a repeat request for it, so a raw
> clk_set_rate() would turn the next request for a raised rate into a
> silent no-op. The boot-rate restore added in the previous patch is
> converted accordingly.
> 
> The rate the previous patch reads at probe is the firmware's own number
> and need not be in the OPP table: a board that does not pin the clock
> with assigned-clock-rates gets whatever the firmware's divider produces.
> So the boot rate is normalised to the table once, at init, to the lowest
> OPP that is not below it, and everything here compares against and
> returns to that OPP. Comparing cur_freq against a rate that is not in the
> table would leave the cores held after the first change, with no
> governor tick ever able to match it.
> 
> ->get_cur_freq() and the status callback report the rate this driver last
> requested rather than asking the clock: devfreq queries them from sysfs
> with no runtime PM reference of its own, and the answer has to be a rate
> from the OPP table or devfreq_get_freq_level() does not find it and
> devfreq warns on every transition. "Requested" is the accurate word: a
> rate the firmware refuses does not come back as an error, because the
> clock framework ignores what the clock's set_rate returns. Keeping every
> request to the OPP table, which names only the rates the firmware accepts,
> keeps a request from being refused; what sysfs shows is still the request,
> not a measurement.
> 
> A devicetree with no OPP table is not an error: the driver returns without
> a devfreq device and the NPU keeps its boot rate, as before this patch.
> 
> The governor thresholds are a starting point taken from the other
> accelerators in tree, not a measurement. Inference workloads have not been
> profiled against them.
> 
> Assisted-by: LLM sparse checkpatch
> Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
> ---
> v2:
> - The devfreq device goes on the first core in devicetree order with an
>   OPP table, not on whichever core with the table bound first, as v1
>   would have done with the table on all three cores (Nicolas).
> - The boot rate is normalised to the OPP table at init. Found by reading
>   the code: without assigned-clock-rates the raw rate is not in the table
>   and the cores would have stayed held after the first change. Code read
>   only so far; a devicetree without the property has not been booted.
> - Init holds every core while it programs the initial OPP, since
>   rounding the boot rate up to the table may raise the clock.
> - "last programmed" is now "last requested", with the reason.
> - Comments and text updated for the shared table; a comment that
>   counted ten governor ticks a second now counts twenty (50 ms polling).
> - The commit message now says that unbinding any core takes the
>   devfreq device down until all are bound again; v1 did the same
>   without saying so.
> - The utilisation counters of a core that was unbound and bound again
>   start from zero.
> - A comment over hold_all() states the invariant the loops rely on.
> 
>  drivers/accel/rocket/Kconfig          |   2 +
>  drivers/accel/rocket/Makefile         |   1 +
>  drivers/accel/rocket/rocket_core.h    |  11 +
>  drivers/accel/rocket/rocket_devfreq.c | 500 ++++++++++++++++++++++++++
>  drivers/accel/rocket/rocket_devfreq.h |  65 ++++
>  drivers/accel/rocket/rocket_device.h  |   3 +
>  drivers/accel/rocket/rocket_drv.c     |  59 ++-
>  drivers/accel/rocket/rocket_job.c     |   7 +
>  8 files changed, 646 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/accel/rocket/rocket_devfreq.c
>  create mode 100644 drivers/accel/rocket/rocket_devfreq.h
> 
> diff --git a/drivers/accel/rocket/Kconfig b/drivers/accel/rocket/Kconfig
> index 16465abe06607..00ee845c871fa 100644
> --- a/drivers/accel/rocket/Kconfig
> +++ b/drivers/accel/rocket/Kconfig
> @@ -8,6 +8,8 @@ config DRM_ACCEL_ROCKET
>  	depends on MMU
>  	select DRM_SCHED
>  	select DRM_GEM_SHMEM_HELPER
> +	select PM_DEVFREQ
> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>  	help
>  	  Choose this option if you have a Rockchip SoC that contains a
>  	  compatible Neural Processing Unit (NPU), such as the RK3588. Called by
> diff --git a/drivers/accel/rocket/Makefile b/drivers/accel/rocket/Makefile
> index 3713dfe223d6e..e0944b3e68121 100644
> --- a/drivers/accel/rocket/Makefile
> +++ b/drivers/accel/rocket/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_ACCEL_ROCKET) := rocket.o
>  rocket-y := \
>  	rocket_core.o \
>  	rocket_device.o \
> +	rocket_devfreq.o \
>  	rocket_drv.o \
>  	rocket_gem.o \
>  	rocket_job.o
> diff --git a/drivers/accel/rocket/rocket_core.h b/drivers/accel/rocket/rocket_core.h
> index 46ed8352a79d2..c9995bd9e0553 100644
> --- a/drivers/accel/rocket/rocket_core.h
> +++ b/drivers/accel/rocket/rocket_core.h
> @@ -7,6 +7,7 @@
>  #include <drm/gpu_scheduler.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/ktime.h>
>  #include <linux/mutex_types.h>
>  #include <linux/reset.h>
>  
> @@ -60,6 +61,16 @@ struct rocket_core {
>  	struct drm_gpu_scheduler sched;
>  	u64 fence_context;
>  	u64 emit_seqno;
> +
> +	/*
> +	 * Utilisation seen by devfreq, guarded by rdev->devfreq.busy_lock. A
> +	 * core runs one task at a time, so a flag is exact here and, unlike a
> +	 * counter, cannot be left skewed by a job the reset path tore down.
> +	 */
> +	bool busy;
> +	ktime_t busy_time;
> +	ktime_t idle_time;
> +	ktime_t time_last_update;
>  };
>  
>  int rocket_core_init(struct rocket_core *core);
> diff --git a/drivers/accel/rocket/rocket_devfreq.c b/drivers/accel/rocket/rocket_devfreq.c
> new file mode 100644
> index 0000000000000..871fa370eb432
> --- /dev/null
> +++ b/drivers/accel/rocket/rocket_devfreq.c
> @@ -0,0 +1,500 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright 2025 Igor Paunovic <royalnet026@gmail.com> */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/ktime.h>
> +#include <linux/minmax.h>
> +#include <linux/of.h>
> +#include <linux/pm_opp.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "rocket_core.h"
> +#include "rocket_device.h"
> +#include "rocket_devfreq.h"
> +
> +/*
> + * One clock and one supply feed all of the NPU cores, so a single devfreq
> + * device drives them together. It hangs off the first core in devicetree
> + * order that carries an OPP table.
> + *
> + * The awkward part is that the clock is generated by a PVTPLL that sits
> + * inside the NPU power islands. An island that is powered up while the clock
> + * is above the rate the bootloader left never acknowledges the power-on, and
> + * the first register access into it afterwards takes an asynchronous SError.
> + *
> + * Lowering the rate is fine at any time as long as the boot rate is one the
> + * firmware serves from GPLL, which on the RK3588 is the pinned 200 MHz: for
> + * that rate it writes only CRU clock selectors. Raising the rate is not, so
> + * before it goes up every core is runtime resumed and the
> + * references are kept for as long as the clock stays raised. While they are
> + * held no core can suspend, so no island can transition at all, which is the
> + * property that makes the raised clock safe rather than merely unlikely to be
> + * caught out.
> + *
> + * The cost is that a busy NPU does not power-gate individual cores. It is
> + * paid only above the boot rate: at the boot rate the references are dropped
> + * and runtime PM behaves exactly as it did before this file existed. The cost
> + * in milliwatts has not been measured on this board, and a measurement or a
> + * better idea would both be welcome.
> + */
> +
> +static void rocket_devfreq_update_utilisation(struct rocket_core *core)
> +{
> +	ktime_t now = ktime_get();
> +	ktime_t elapsed = ktime_sub(now, core->time_last_update);
> +
> +	if (core->busy)
> +		core->busy_time = ktime_add(core->busy_time, elapsed);
> +	else
> +		core->idle_time = ktime_add(core->idle_time, elapsed);
> +
> +	core->time_last_update = now;
> +}
> +
> +/*
> + * The devfreq device exists only while every slot in rdev->cores[] is
> + * filled: it goes up when num_cores reaches max_cores and comes down in
> + * rocket_remove() before the leaving core empties its slot. The loops
> + * over num_cores here and below rely on that.
> + */
> +static int rocket_devfreq_hold_all(struct rocket_device *rdev)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < rdev->num_cores; i++) {
> +		ret = pm_runtime_resume_and_get(rdev->cores[i].dev);
> +		if (ret < 0) {
> +			while (i--)
> +				pm_runtime_put_autosuspend(rdev->cores[i].dev);
> +
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void rocket_devfreq_release_all(struct rocket_device *rdev)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < rdev->num_cores; i++)
> +		pm_runtime_put_autosuspend(rdev->cores[i].dev);
> +}
> +
> +/* Caller holds rdev->devfreq.lock. */
> +static int rocket_devfreq_set_rate(struct rocket_device *rdev, unsigned long freq)
> +{
> +	struct rocket_devfreq *rdevfreq = &rdev->devfreq;
> +	struct device *dev = rdevfreq->owner->dev;
> +	int ret;
> +
> +	ret = dev_pm_opp_set_rate(dev, freq);
> +	if (ret) {
> +		dev_err(dev, "failed to set the NPU rate to %lu Hz: %d\n", freq, ret);
> +		return ret;
> +	}
> +
> +	WRITE_ONCE(rdevfreq->cur_freq, freq);
> +
> +	return 0;
> +}
> +
> +static int rocket_devfreq_target(struct device *dev, unsigned long *freq, u32 flags)
> +{
> +	struct rocket_device *rdev = dev_get_drvdata(dev);
> +	struct rocket_devfreq *rdevfreq = &rdev->devfreq;
> +	struct dev_pm_opp *opp;
> +	int ret;
> +
> +	opp = devfreq_recommended_opp(dev, freq, flags);
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
> +	dev_pm_opp_put(opp);
> +
> +	guard(mutex)(&rdevfreq->lock);
> +
> +	/*
> +	 * The governor calls this on every tick, including the ticks where it
> +	 * arrives at the rate the NPU is already running. Without this an idle
> +	 * NPU would resume all of its cores twenty times a second to set the
> +	 * rate they already have.
> +	 */
> +	if (*freq == READ_ONCE(rdevfreq->cur_freq))
> +		return 0;
> +
> +	if (!rdevfreq->cores_held) {
> +		ret = rocket_devfreq_hold_all(rdev);
> +		if (ret) {
> +			/*
> +			 * Runtime PM is disabled on the way into system
> +			 * suspend, so this is the ordinary way for a governor
> +			 * tick that raced with it to end.
> +			 */
> +			dev_dbg(dev, "cannot resume the NPU cores to change rate: %d\n", ret);
> +			return ret;
> +		}
> +		rdevfreq->cores_held = true;
> +	}
> +
> +	ret = rocket_devfreq_set_rate(rdev, *freq);
> +
> +	/* At the boot rate the cores are free to suspend again. */
> +	if (READ_ONCE(rdevfreq->cur_freq) <= rdevfreq->boot_freq) {
> +		rdevfreq->cores_held = false;
> +		rocket_devfreq_release_all(rdev);
> +	}
> +
> +	return ret;
> +}
> +
> +static int rocket_devfreq_get_dev_status(struct device *dev,
> +					 struct devfreq_dev_status *status)
> +{
> +	struct rocket_device *rdev = dev_get_drvdata(dev);
> +	ktime_t busy = 0, total = 0;
> +	unsigned int i;
> +
> +	scoped_guard(spinlock_irqsave, &rdev->devfreq.busy_lock) {
> +		for (i = 0; i < rdev->num_cores; i++) {
> +			struct rocket_core *core = &rdev->cores[i];
> +
> +			rocket_devfreq_update_utilisation(core);
> +
> +			/*
> +			 * The cores share the clock, so what the rate has to
> +			 * satisfy is the busiest of them. Adding the cores up
> +			 * instead would report one saturated core out of three
> +			 * as a third of the load, and clock down underneath it.
> +			 */
> +			busy = max(busy, core->busy_time);
> +			total = max(total, ktime_add(core->busy_time, core->idle_time));
> +
> +			core->busy_time = 0;
> +			core->idle_time = 0;
> +		}
> +	}
> +
> +	status->busy_time = ktime_to_ns(busy);
> +	status->total_time = ktime_to_ns(total);
> +	status->current_frequency = READ_ONCE(rdev->devfreq.cur_freq);
> +
> +	dev_dbg(dev, "busy %lu total %lu %lu%% freq %lu MHz\n",
> +		status->busy_time, status->total_time,
> +		status->busy_time * 100 / max(status->total_time, 1UL),
> +		status->current_frequency / 1000 / 1000);
> +
> +	return 0;
> +}
> +
> +/*
> + * Report the rate this driver last requested, not what the firmware says.
> + * devfreq asks for the current frequency from sysfs as well, without a runtime
> + * PM reference of its own, and the answer has to be one of the rates in the
> + * OPP table or devfreq_get_freq_level() will not find it and every transition
> + * will be logged as unknown.
> + *
> + * "Requested" is the accurate word: a rate the firmware refuses does not come
> + * back as an error, because the clock framework ignores what the clock's
> + * set_rate returns and the clock stays where it was. Keeping every request to
> + * the OPP table, which names only the rates the firmware accepts, keeps a
> + * request from being refused; what is reported is still the request, not a
> + * measurement.
> + */
> +static int rocket_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
> +{
> +	struct rocket_device *rdev = dev_get_drvdata(dev);
> +
> +	*freq = READ_ONCE(rdev->devfreq.cur_freq);
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile rocket_devfreq_profile = {
> +	.timer = DEVFREQ_TIMER_DELAYED,
> +	.polling_ms = 50,
> +	.target = rocket_devfreq_target,
> +	.get_dev_status = rocket_devfreq_get_dev_status,
> +	.get_cur_freq = rocket_devfreq_get_cur_freq,
> +};
> +
> +void rocket_devfreq_record_busy(struct rocket_core *core)
> +{
> +	struct rocket_devfreq *rdevfreq = &core->rdev->devfreq;
> +
> +	if (!rdevfreq->devfreq)
> +		return;
> +
> +	scoped_guard(spinlock_irqsave, &rdevfreq->busy_lock) {
> +		rocket_devfreq_update_utilisation(core);
> +		core->busy = true;
> +	}
> +}
> +
> +/*
> + * Idempotent on purpose: a job that times out is torn down by the reset path,
> + * which cannot know whether the completion interrupt got there first.
> + */
> +void rocket_devfreq_record_idle(struct rocket_core *core)
> +{
> +	struct rocket_devfreq *rdevfreq = &core->rdev->devfreq;
> +
> +	if (!rdevfreq->devfreq)
> +		return;
> +
> +	scoped_guard(spinlock_irqsave, &rdevfreq->busy_lock) {
> +		rocket_devfreq_update_utilisation(core);
> +		core->busy = false;
> +	}
> +}
> +
> +/*
> + * Put the clock back to the boot OPP through the OPP core. Called with no
> + * lock held, from the last core on its way down.
> + */
> +int rocket_devfreq_set_boot_rate(struct rocket_device *rdev)
> +{
> +	struct rocket_devfreq *rdevfreq = &rdev->devfreq;
> +	int ret;
> +
> +	ret = dev_pm_opp_set_rate(rdevfreq->owner->dev, rdevfreq->boot_freq);
> +	if (!ret)
> +		WRITE_ONCE(rdevfreq->cur_freq, rdevfreq->boot_freq);
> +
> +	return ret;
> +}
> +
> +/*
> + * System suspend, called from every core's ->suspend before it is forced down.
> + * The first one to get here does the work and the rest are no-ops.
> + *
> + * It has to be every core and not just the one that owns the devfreq device.
> + * That core is suspended last, and a suspend that aborts partway - a pending
> + * wakeup, or this driver's own -EBUSY on a core that is still busy - would
> + * never reach it, leaving the clock raised over already gated islands.
> + *
> + * No governor tick can be in flight here: dpm_suspend() calls devfreq_suspend()
> + * before it walks the devices, which stops the monitor on every registered
> + * devfreq. That is also where this driver does get devfreq_suspend_device() -
> + * from the PM core, on a path that holds no runtime PM lock of ours.
> + */
> +void rocket_devfreq_suspend(struct rocket_device *rdev)
> +{
> +	struct rocket_devfreq *rdevfreq = &rdev->devfreq;
> +
> +	if (!rdevfreq->devfreq)
> +		return;
> +
> +	guard(mutex)(&rdevfreq->lock);
> +
> +	if (!rdevfreq->cores_held)
> +		return;
> +
> +	rocket_devfreq_set_rate(rdev, rdevfreq->boot_freq);
> +	rdevfreq->cores_held = false;
> +	rocket_devfreq_release_all(rdev);
> +}
> +
> +int rocket_devfreq_init(struct rocket_device *rdev)
> +{
> +	static const char * const clk_names[] = { "npu", NULL };
> +	static const char * const supplies[] = { "npu", NULL };
> +	struct dev_pm_opp_config config = {
> +		.clk_names = clk_names,
> +		.regulator_names = supplies,
> +	};
> +	struct rocket_devfreq *rdevfreq = &rdev->devfreq;
> +	struct rocket_core *owner = NULL;
> +	struct dev_pm_opp *opp;
> +	struct device *dev;
> +	unsigned long freq;
> +	unsigned int i;
> +	int ret;
> +
> +	/*
> +	 * The devfreq device hangs off the first core in devicetree order that
> +	 * carries an OPP table: on the RK3588 every core references the shared
> +	 * table, so that is rknn_core_0. It has to be a fixed choice and not
> +	 * whichever core bound last, because the devfreq device is named after
> +	 * this core and a cooling map in the devicetree resolves against its
> +	 * node. core->index is the core's position among the core nodes, so
> +	 * the lowest index is the first node.
> +	 */
> +	for (i = 0; i < rdev->num_cores; i++) {
> +		struct rocket_core *core = &rdev->cores[i];
> +
> +		if (!of_property_present(core->dev->of_node, "operating-points-v2"))
> +			continue;
> +
> +		if (!owner || core->index < owner->index)
> +			owner = core;
> +	}
> +
> +	/*
> +	 * No OPP table is not an error. It asks for the NPU to stay at the
> +	 * rate it booted at, which is what this driver did before devfreq.
> +	 */
> +	if (!owner)
> +		return 0;
> +
> +	dev = owner->dev;
> +
> +	/*
> +	 * None of this can be devres. It is attached to the owning core, while
> +	 * the device being probed right now is whichever core happened to bind
> +	 * last, so devres would outlive the teardown in rocket_devfreq_fini()
> +	 * and a later rebind would find the OPP table still populated.
> +	 *
> +	 * Naming the clock matters as much as claiming the supply: without it
> +	 * the OPP core takes the first clock in the node, which is the bus
> +	 * clock, and would scale that instead of the compute clock.
> +	 */
> +	ret = dev_pm_opp_set_config(dev, &config);
> +	if (ret < 0) {
> +		if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "failed to set the OPP clock and supply\n");
> +
> +		dev_info(dev, "no NPU supply described, leaving the clock alone\n");
> +		return 0;
> +	}
> +	rdevfreq->opp_token = ret;
> +
> +	ret = dev_pm_opp_of_add_table(dev);
> +	if (ret) {
> +		if (ret != -ENODEV)
> +			dev_err_probe(dev, ret, "failed to add the OPP table\n");
> +		else
> +			ret = 0;
> +
> +		goto err_clear_config;
> +	}
> +
> +	mutex_init(&rdevfreq->lock);
> +	spin_lock_init(&rdevfreq->busy_lock);
> +
> +	for (i = 0; i < rdev->num_cores; i++) {
> +		struct rocket_core *core = &rdev->cores[i];
> +
> +		/* A core that was unbound and bound again starts over. */
> +		core->busy = false;
> +		core->busy_time = 0;
> +		core->idle_time = 0;
> +		core->time_last_update = ktime_get();
> +	}
> +
> +	freq = rdev->npu_boot_rate;
> +	opp = devfreq_recommended_opp(dev, &freq, 0);
> +	if (IS_ERR(opp)) {
> +		ret = dev_err_probe(dev, PTR_ERR(opp),
> +				    "no OPP covers the %lu Hz boot rate\n",
> +				    rdev->npu_boot_rate);
> +		goto err_remove_table;
> +	}
> +
> +	/*
> +	 * From here on the boot rate is the OPP it maps to. The raw rate is
> +	 * the firmware's number and need not be in the table at all.
> +	 */
> +	rdevfreq->boot_freq = freq;
> +
> +	/*
> +	 * Program the supply for the rate the NPU is already running, so that
> +	 * the regulator is not switched off underneath it by
> +	 * regulator_late_cleanup(). That OPP is the boot rate rounded up to
> +	 * the table, so this may raise the clock, and a raised clock is only
> +	 * ever programmed with every core held: the same rule as ->target().
> +	 */
> +	ret = rocket_devfreq_hold_all(rdev);
> +	if (ret) {
> +		dev_pm_opp_put(opp);
> +		dev_err_probe(dev, ret,
> +			      "cannot resume the NPU cores to set the initial OPP\n");
> +		goto err_remove_table;
> +	}
> +	ret = dev_pm_opp_set_opp(dev, opp);
> +	rocket_devfreq_release_all(rdev);
> +	dev_pm_opp_put(opp);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "failed to set the initial OPP\n");
> +		goto err_remove_table;
> +	}
> +
> +	rdevfreq->cur_freq = freq;
> +	rdevfreq->owner = owner;
> +	rocket_devfreq_profile.initial_freq = freq;
> +
> +	/*
> +	 * A starting point taken from the other accelerators in tree, not a
> +	 * measurement: inference workloads have not been profiled against
> +	 * these thresholds.
> +	 */
> +	rdevfreq->gov_data.upthreshold = 50;
> +	rdevfreq->gov_data.downdifferential = 10;
> +
> +	rdevfreq->devfreq = devfreq_add_device(dev, &rocket_devfreq_profile,
> +					       DEVFREQ_GOV_SIMPLE_ONDEMAND,
> +					       &rdevfreq->gov_data);
> +	if (IS_ERR(rdevfreq->devfreq)) {
> +		ret = PTR_ERR(rdevfreq->devfreq);
> +		rdevfreq->devfreq = NULL;
> +		rdevfreq->owner = NULL;
> +
> +		dev_err_probe(dev, ret, "failed to add the devfreq device\n");
> +		goto err_remove_table;
> +	}
> +
> +	return 0;
> +
> +err_remove_table:
> +	mutex_destroy(&rdevfreq->lock);
> +	dev_pm_opp_of_remove_table(dev);
> +err_clear_config:
> +	dev_pm_opp_clear_config(rdevfreq->opp_token);
> +	rdevfreq->opp_token = 0;
> +
> +	return ret;
> +}
> +
> +void rocket_devfreq_fini(struct rocket_device *rdev)
> +{
> +	struct rocket_devfreq *rdevfreq = &rdev->devfreq;
> +	struct device *dev;
> +
> +	if (!rdevfreq->devfreq)
> +		return;
> +
> +	dev = rdevfreq->owner->dev;
> +
> +	devfreq_remove_device(rdevfreq->devfreq);
> +	rdevfreq->devfreq = NULL;
> +
> +	/*
> +	 * Lower the clock before letting go of the cores, not after: a core
> +	 * that suspends while the clock is still raised would be unable to
> +	 * come back.
> +	 */
> +	scoped_guard(mutex, &rdevfreq->lock) {
> +		if (rdevfreq->cores_held) {
> +			rocket_devfreq_set_rate(rdev, rdevfreq->boot_freq);
> +			rdevfreq->cores_held = false;
> +			rocket_devfreq_release_all(rdev);
> +		}
> +	}
> +
> +	/*
> +	 * Undo the OPP setup by hand, in the reverse order. Everything above
> +	 * lives on the owning core rather than on the device that was probing
> +	 * when it was set up, so nothing here is released by devres; leaving
> +	 * the table populated would make the next bind fail with the OPP core
> +	 * complaining that it is not empty. After this the owner is gone, so
> +	 * anything that still wants the boot rate asks the clock directly.
> +	 */
> +	rdevfreq->owner = NULL;
> +	mutex_destroy(&rdevfreq->lock);
> +	dev_pm_opp_of_remove_table(dev);
> +	dev_pm_opp_clear_config(rdevfreq->opp_token);
> +	rdevfreq->opp_token = 0;
> +}
> diff --git a/drivers/accel/rocket/rocket_devfreq.h b/drivers/accel/rocket/rocket_devfreq.h
> new file mode 100644
> index 0000000000000..65a9de6d37389
> --- /dev/null
> +++ b/drivers/accel/rocket/rocket_devfreq.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright 2025 Igor Paunovic <royalnet026@gmail.com> */
> +
> +#ifndef __ROCKET_DEVFREQ_H__
> +#define __ROCKET_DEVFREQ_H__
> +
> +#include <linux/devfreq.h>
> +#include <linux/mutex_types.h>
> +#include <linux/spinlock_types.h>
> +
> +struct rocket_core;
> +struct rocket_device;
> +
> +struct rocket_devfreq {
> +	struct devfreq *devfreq;
> +	struct devfreq_simple_ondemand_data gov_data;
> +
> +	/*
> +	 * The core the devfreq device hangs off: the first in devicetree order
> +	 * to carry an OPP table. NULL when no core does.
> +	 */
> +	struct rocket_core *owner;
> +
> +	/*
> +	 * The OPP clock and supply configuration is attached to the owning
> +	 * core, which is not the device this driver is probing when it is set
> +	 * up, so it cannot be devres. Zero means nothing is attached.
> +	 */
> +	int opp_token;
> +
> +	/*
> +	 * Serialises rate changes against each other and against the set of
> +	 * runtime PM references taken below.
> +	 *
> +	 * This is never taken from a runtime PM callback. A rate change
> +	 * resumes every core while holding it, so a core that took it on its
> +	 * way down would wait for a rate change that is waiting for that same
> +	 * core to finish suspending.
> +	 */
> +	struct mutex lock;
> +	unsigned long cur_freq;
> +	bool cores_held;
> +
> +	/*
> +	 * The boot rate as an OPP: the lowest rate in the table that is not
> +	 * below rdev->npu_boot_rate. The raw boot rate is whatever the firmware
> +	 * reported at probe and need not be in the table, and comparing
> +	 * cur_freq against a rate that is not in the table would leave the
> +	 * cores held for good after the first change. Everything here compares
> +	 * against and returns to this instead.
> +	 */
> +	unsigned long boot_freq;
> +
> +	/* Guards the utilisation fields of every core. */
> +	spinlock_t busy_lock;
> +};
> +
> +int rocket_devfreq_init(struct rocket_device *rdev);
> +void rocket_devfreq_fini(struct rocket_device *rdev);
> +void rocket_devfreq_suspend(struct rocket_device *rdev);
> +int rocket_devfreq_set_boot_rate(struct rocket_device *rdev);
> +void rocket_devfreq_record_busy(struct rocket_core *core);
> +void rocket_devfreq_record_idle(struct rocket_core *core);
> +
> +#endif /* __ROCKET_DEVFREQ_H__ */
> diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/rocket_device.h
> index ba7c977cd6951..a91d5a9b09ed3 100644
> --- a/drivers/accel/rocket/rocket_device.h
> +++ b/drivers/accel/rocket/rocket_device.h
> @@ -11,6 +11,7 @@
>  #include <linux/platform_device.h>
>  
>  #include "rocket_core.h"
> +#include "rocket_devfreq.h"
>  
>  struct rocket_device {
>  	struct drm_device ddev;
> @@ -37,6 +38,8 @@ struct rocket_device {
>  	 */
>  	unsigned long npu_boot_rate;
>  	atomic_t active_cores;
> +
> +	struct rocket_devfreq devfreq;
>  };
>  
>  struct rocket_device *rocket_device_init(struct platform_device *pdev,
> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 8f03de1af488c..c6eab2239b6a9 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
> @@ -14,6 +14,7 @@
>  #include <linux/pm_runtime.h>
>  
>  #include "rocket_device.h"
> +#include "rocket_devfreq.h"
>  #include "rocket_drv.h"
>  #include "rocket_gem.h"
>  #include "rocket_job.h"
> @@ -244,6 +245,20 @@ static int rocket_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_core;
>  
> +	/*
> +	 * Every core described in the devicetree has to be bound before the
> +	 * devfreq device goes up. A rate change resumes all of them and keeps
> +	 * them resumed, and a core that had not probed yet would come up later
> +	 * underneath a raised clock.
> +	 */
> +	if (rdev->num_cores == rdev->max_cores) {
> +		ret = rocket_devfreq_init(rdev);

IMHO, handling rocket_devfreq_init() error as critical error to disable core is
too much. How about just printing error for user?

Thanks,
Sidong

> +		if (ret) {
> +			rocket_core_fini(&rdev->cores[core]);
> +			goto err_core;
> +		}
> +	}
> +
>  	return 0;
>  
>  err_core:
> @@ -268,6 +283,9 @@ static void rocket_remove(struct platform_device *pdev)
>  	if (WARN_ON(core < 0))
>  		return;
>  
> +	/* The devfreq device drives every core, so it goes before any of them. */
> +	rocket_devfreq_fini(rdev);
> +
>  	rocket_core_fini(&rdev->cores[core]);
>  	rdev->cores[core].dev = NULL;
>  	rdev->num_cores--;
> @@ -314,7 +332,18 @@ static void rocket_npu_restore_boot_rate(struct rocket_core *core)
>  	if (!rdev->npu_boot_rate)
>  		return;
>  
> -	err = clk_set_rate(core->clks[2].clk, rdev->npu_boot_rate);
> +	/*
> +	 * Go through the OPP core once there is a table, never behind its
> +	 * back: it caches the OPP it last applied and skips a request for that
> +	 * same OPP, so a raw clk_set_rate() here would make the next request
> +	 * for the raised rate a silent no-op, with sysfs reporting a rate the
> +	 * hardware was not running.
> +	 */
> +	if (rdev->devfreq.owner)
> +		err = rocket_devfreq_set_boot_rate(rdev);
> +	else
> +		err = clk_set_rate(core->clks[2].clk, rdev->npu_boot_rate);
> +
>  	if (err)
>  		dev_warn(core->dev,
>  			 "failed to restore the NPU boot rate of %lu Hz: %d\n",
> @@ -360,14 +389,38 @@ static int rocket_device_runtime_suspend(struct device *dev)
>  	return 0;
>  }
>  
> +static int rocket_device_suspend(struct device *dev)
> +{
> +	struct rocket_device *rdev = dev_get_drvdata(dev);
> +
> +	/*
> +	 * pm_runtime_force_suspend() below powers this core down whatever the
> +	 * runtime PM usage count says, so the references taken while the clock
> +	 * is raised do not hold it off. Put the rate back first; the call is a
> +	 * no-op on every core after the first.
> +	 */
> +	rocket_devfreq_suspend(rdev);
> +
> +	return pm_runtime_force_suspend(dev);
> +}
> +
>  EXPORT_GPL_DEV_PM_OPS(rocket_pm_ops) = {
>  	RUNTIME_PM_OPS(rocket_device_runtime_suspend, rocket_device_runtime_resume, NULL)
> -	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +	SYSTEM_SLEEP_PM_OPS(rocket_device_suspend, pm_runtime_force_resume)
>  };
>  
>  /*
>   * A kexec hands the next kernel whatever rate is set here, and that kernel
>   * will power the islands up before it looks at the clock.
> + *
> + * Take the devfreq device down before restoring the rate rather than after.
> + * Nothing freezes workqueues on this path, so a governor tick that landed
> + * after the restore would raise the clock straight back up and hand on exactly
> + * what this is here to prevent.
> + *
> + * The hook runs once per core. After the first call the devfreq device is
> + * gone, and each later restore asks the clock for the rate it already has,
> + * which the clock framework drops before it reaches the firmware.
>   */
>  static void rocket_shutdown(struct platform_device *pdev)
>  {
> @@ -377,6 +430,8 @@ static void rocket_shutdown(struct platform_device *pdev)
>  	if (!rdev)
>  		return;
>  
> +	rocket_devfreq_fini(rdev);
> +
>  	core = find_core_for_dev(&pdev->dev);
>  	if (core >= 0)
>  		rocket_npu_restore_boot_rate(&rdev->cores[core]);
> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 25ee4ab172a82..7846b627b00f4 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -15,6 +15,7 @@
>  
>  #include "rocket_core.h"
>  #include "rocket_device.h"
> +#include "rocket_devfreq.h"
>  #include "rocket_drv.h"
>  #include "rocket_job.h"
>  #include "rocket_registers.h"
> @@ -149,6 +150,8 @@ static void rocket_job_hw_submit(struct rocket_core *core, struct rocket_job *jo
>  
>  	rocket_pc_writel(core, TASK_DMA_BASE_ADDR, PC_TASK_DMA_BASE_ADDR_DMA_BASE_ADDR(0x0));
>  
> +	rocket_devfreq_record_busy(core);
> +
>  	rocket_pc_writel(core, OPERATION_ENABLE, PC_OPERATION_ENABLE_OP_EN(1));
>  
>  	dev_dbg(core->dev, "Submitted regcmd at 0x%llx to core %d", task->regcmd, core->index);
> @@ -348,6 +351,8 @@ static void rocket_job_handle_irq(struct rocket_core *core)
>  	rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
>  	rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
>  
> +	rocket_devfreq_record_idle(core);
> +
>  	scoped_guard(mutex, &core->job_lock)
>  		if (core->in_flight_job) {
>  			if (core->in_flight_job->next_task_idx < core->in_flight_job->task_count) {
> @@ -379,6 +384,8 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>  		if (core->in_flight_job)
>  			pm_runtime_put_noidle(core->dev);
>  
> +		rocket_devfreq_record_idle(core);
> +
>  		iommu_detach_group(NULL, core->iommu_group);
>  
>  		core->in_flight_job = NULL;
> -- 
> 2.43.0
>