From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pz2-f41.google.com (mail-pz2-f41.google.com [74.125.228.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EF2E751DAEC for ; Wed, 23 Sep 2026 13:15:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.228.41 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790169362; cv=none; b=n90hkgO5/atjlr8bH3V3dhtZRWt1GTS/cQuKXX4cdssKwtNItsArED10jQVUlw2Dgrk++g6nCP8Ukid7EBbz5mjL2C7RP8ZhSk5E9V5/v4tL1diF6+kPiRAH699J4RJ8dRCdQsiSibsEAEW6wEcHtTPpHAMLR5WBG9XRh5NjOeE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790169362; c=relaxed/simple; bh=oz5F2E/2iG2PprQSuxGhbFufwG4H591m+IwO0/XFGbU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=sA5mRowAoBJp8vtysemCQZX2btWJsc6vy5lqw22K3Hc3NkawOGvHijBE76FjLUREp2/6zpPz8mzzcDjr5ao8bIp/y8ouSkZgUXzwopKzXuh19RqYioKtuRDIXqfiZUmH9tU6hfEQlFO9PNTofu88jVT2EP/cpj0InaKyNZLvwcE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=furiosa.ai; spf=none smtp.mailfrom=furiosa.ai; dkim=pass (1024-bit key) header.d=furiosa.ai header.i=@furiosa.ai header.b=CObBSH3S; arc=none smtp.client-ip=74.125.228.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=furiosa.ai Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=furiosa.ai Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=furiosa.ai header.i=@furiosa.ai header.b="CObBSH3S" Received: by mail-pz2-f41.google.com with SMTP id d2e1a72fcca58-85469e2254dso110392b3a.1 for ; Wed, 23 Sep 2026 06:15:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=furiosa.ai; s=google; t=1790169358; x=1790774158; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=ID2TgS0sKt7ncgE+1Qe9AfzwImm+RaD43d15X2G7G3w=; b=CObBSH3SOREi5rabZwzcEDhHonhMerXT1i7rbFcvCs81u/gjBKywBSC4y+3aSA0YOp Ot3ZDpo0Yv0Rtn4gNA/BQ9J+cOrnvDFaV6rsO2QYzTvPOcR31bAPGZ1TVWRerbH7JMAy riPi9y599USaCFMTbTfm8qBFBaw8psNdRu6Cg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1790169358; x=1790774158; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=ID2TgS0sKt7ncgE+1Qe9AfzwImm+RaD43d15X2G7G3w=; b=ZlgmgMYrGmCyK66+MhYkIMUkFNfIssS38VDS8vxpfs4BvRDd/gJ5oxTiaFbWPjLmtf 0bam/yqAtRLnV+vkeY9k6BT89x61eqHUZaZCCxuAH7pzmzclLVFlpStUuJtL+faqxeo/ 0NZG1R+BPixKP8PeMD8sDkDFexgXV8d3BMaqPAeXr6VcLEdu/nH0rzNEDHXTbpsHIEo2 uvK0CN/LkiKF+CZoIVlI7uRq1oL/P2j4v2wZ2aEncPzYF3JxCYaZKnWt03AQEy1rwscz onPKkwwupxhJJz7kABRAoHA+aHO1mxMKY50m/J+rPK4u2ISZxY+X7aHO08RXqST1wzh+ zxLA== X-Forwarded-Encrypted: i=1; AKwUvBzz9EXxlsafQV693AmyT9LeuIpfgxDCrAJQvqHjA2lzq6+UpgdsmVirBEoXj41Z9Fo+vjY272+2pH8uHkg=@vger.kernel.org X-Gm-Message-State: AFuF++lVY6aHMqwS3nRSNSNMVRAprYqSGpmEV9X0CQ29bwZZiTe6S0qv Mji3mo6c6FFu3uuTGdDH/uJjFq0IRVD6OS89/kCNzNdttTDmwzDHZbQZqreDQ5rcFno= X-Gm-Gg: AYBFou2wco2POjNR2yAhJMFGc504J8A/EL1nXyCPD1hUHTa2c9YizRICq2n/NVsCM6I EZjxjXEE1Du0P7ogBYuUibuXw9qnr5W4dhvI5SHaSzoMfJrpMutiqeMWf2D3h9WTRZoPx+8pjWa L5cGyOqxxEmQlykcfNJ9O2cjvqfOmFnsGmqOK6PTZN2ntRvUuB2Ii6uclfZygTOGsJVBiznDlVt KDyfGaUeU4hgMqODsU7Uu9/KZR+EWpyxhmERv/Tog+JFkZ8N3ghTC3YmlGQhTrNVSADbxpefWd9 cAnQ4EtF9hvfsf3y8ha5OYItKYKQuRvxzX2GwIcWSDzhuaDYd5JEG8wvbWyDwAo3/FLJsS1Izta iIdRK4WdXbjBoN/CioWKSl5X+tbPfWjAYRTcvi36ughBqhyTitVoeHcyf7LFwEcJjD4RFT87eKt cq5XnXaaNckR4YGj0xEeKG194evO5sUhrmJJVRR+fQ3936N3axbuSv/ZDlqCes3bE9ATXbgfJo X-Received: by 2002:a05:6a00:ace:b0:878:3704:e0ee with SMTP id d2e1a72fcca58-87d2545fb09mr1521266b3a.20.1790169357733; Wed, 23 Sep 2026 06:15:57 -0700 (PDT) Received: from rock-5b-plus ([61.83.209.48]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-87d1d5c1646sm1233527b3a.30.2026.09.23.06.15.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Sep 2026 06:15:57 -0700 (PDT) Date: Wed, 23 Sep 2026 22:14:51 +0900 From: Sidong Yang To: Igor Paunovic Cc: Tomeu Vizoso , Oded Gabbay , Heiko Stuebner , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Jeff Hugo , Robert Foss , Diederik de Haas , Sebastian Reichel , Jiaxing Hu , Nicolas Dufresne , Jonas Karlman , Guangshuo Li , =?utf-8?Q?H=C3=BCseyin?= BIYIK , dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 09/11] accel/rocket: add devfreq support Message-ID: References: <20260922080114.44662-1-royalnet026@gmail.com> <20260922080114.44662-10-royalnet026@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260922080114.44662-10-royalnet026@gmail.com> 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 > --- > 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 > #include > #include > +#include > #include > #include > > @@ -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 */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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 */ > + > +#ifndef __ROCKET_DEVFREQ_H__ > +#define __ROCKET_DEVFREQ_H__ > + > +#include > +#include > +#include > + > +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 > > #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 > > #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 >