From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=185.11.138.130 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1793790897; b=UCzUkolIrc55GLMfBYwoPLtQPbeB/IwsmO5v5oPuJkXg5l54o+HfwVKM7T6VhrCDGhMs SGpcR2lkXqd73fZWpV7YWE+SUCxQFMyqpfOQsr9+yVPgNewv+p7JbVy3E8QVzZCyZ1bg0 I9n2aiLFFUhghh5ZgE0bP3Tj+8Q1iPNnk6/a/H4IutSHp5ppBQA1nvuowXG1BWOe687pc 11nlc66en5EXIcsd3OHqxIR68lO/kfNI3Wp8r1d96TNWht7o2lNxqNsj5caL/59vtmsI7 w2KdqSB8vHqcqjzwaKzwsJNs8gC+KLSlQJ5q9fkXBGX7j6Uqvu58r7GInYaLRXnZWKQ== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1793790897; h=DMARC-Filter:DKIM-Signature:From:To:Cc:Subject:Date:Message-ID: In-Reply-To:References:MIME-Version:Content-Transfer-Encoding: Content-Type; bh=ElMP7E+td4PbWaeCgFxYru40sXUejYFoNFjAdaBjGOg=; b=TdVermDa4oNeNDgwcxwdpYcZGa8pPfkeMIgcaNLZFdB8G4dnYhz5TSohX56xIXUo7SYZ Wgo6rP5T/7ruzK4aMsUKOAcmofsUE5t+dL2Bo4Hpg5QN9lQXxczGEa53KutB2XdQC46Kq L1zanEU+h39LHwB+tNDkIVX6RDPHJhaJN15r9B2YbYzQsOxwp3m27Yva3HNkj854dUFF/ ZI/BKnTKKbMlUFVaDC/wOmI9LW6T5MjQ1+eCEKUxiiHm7FFt0umsW7nqq2fvt5AUBzx9G TUevueLWBaiLYxBxgUh/dAQbz8RFpxtOPf2yFwkpenEGXHZAzTWM4S/oaqRhy3J1tJg== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass header.from=sntech.de; spf=pass smtp.mailfrom=sntech.de; dkim=pass header.d=sntech.de header.i=@sntech.de header.a=rsa-sha256 header.s=gloria202408 header.b=J2xn6IhH; arc=none smtp.remote-ip=185.11.138.130 Received: from gloria.sntech.de (gloria.sntech.de [185.11.138.130]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8C208606A337; Wed, 4 Nov 2026 11:14:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.2 smtp.subspace.kernel.org 8C208606A337 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=sntech.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=sntech.de Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key, unprotected) header.d=sntech.de header.i=@sntech.de header.a=rsa-sha256 header.s=gloria202408 header.b=J2xn6IhH DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sntech.de; s=gloria202408; h=Content-Type:Content-Transfer-Encoding:MIME-Version: References:In-Reply-To:Message-ID:Date:Subject:Cc:To:From:Reply-To; bh=ElMP7E+td4PbWaeCgFxYru40sXUejYFoNFjAdaBjGOg=; b=J2xn6IhHyGYpOrVe5sEcUkfaKY Nc+tIqwchMNBf6e0WxuM/1TyGy200YvdFq0TlADp3CGjiLoDRN4P1kPKAACXT0gFqgBz+H+EZU5LL l8MGFNTnKN6Dw6NaGRwxVSZP0l498mS3SSQWuzEzC9HZYR0FHtGUJrXnDj+KNO7A4AAFj77a5ICam ana+vlz5r7rgK19JEJLpgUsamSCyCJim3XtYGeF0vsra/5pAwgl2HHS1phJAmA4J5PLNdBlmeBbJ/ 8siN4Ma/gCPNxpy8AmG3cZmmo697E7EIdH6MSRlsHdnWI7A5f+fMT970ATK0zQZK7Oc//yLd6+cOo /8dLGruQ==; From: Heiko =?UTF-8?B?U3TDvGJuZXI=?= To: Tomeu Vizoso , Oded Gabbay , Igor Paunovic Cc: Rob Herring , Krzysztof Kozlowski , Conor Dooley , Jeff Hugo , Robert Foss , Sidong Yang , Diederik de Haas , Sebastian Reichel , Jiaxing Hu , Nicolas Dufresne , Jonas Karlman , Guangshuo Li , =?UTF-8?B?SMO8c2V5aW4=?= 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, Igor Paunovic Subject: Re: [PATCH v2 08/11] accel/rocket: restore the NPU clock boot rate before powering the cores down Date: Wed, 04 Nov 2026 12:14:37 +0100 Message-ID: <5089783.JsnAkG3lO3@diego> In-Reply-To: <20260922080114.44662-9-royalnet026@gmail.com> References: <20260922080114.44662-1-royalnet026@gmail.com> <20260922080114.44662-9-royalnet026@gmail.com> X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" Am Dienstag, 22. September 2026, 10:01:11 Mitteleurop=C3=A4ische Normalzeit= schrieb 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. >=20 > 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 t= he > handover: once devfreq is driving the clock, a kexec would otherwise pass > the raised rate to the next kernel, which powers the islands up before it > looks at it. What this cannot do is rescue a rate it did not set - the ra= te > read at probe is taken as the boot rate whatever it is. >=20 > 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. >=20 > All three cores share the clock, so only the last core to suspend may low= er > it; the others just drop the count. Lowering it is safe with the islands > already down as long as the boot rate is one the firmware serves from GPL= L, > which on the RK3588 is the 200 MHz the devicetree pins: for that rate the > firmware writes only CRU clock selectors, never a register inside the NPU. >=20 > Assisted-by: LLM sparse checkpatch > Signed-off-by: Igor Paunovic > --- > v2: v1 of this patch kept a struct clk handle in struct rocket_device, > taken from the devres of the first core to probe, and used it from the > runtime suspend of whichever core went down last. Unbinding the cores > freed the handle underneath it: KASAN reported a slab-use-after-free in > clk_set_rate() during a ten-round unbind/rebind test on this board after > v1 was posted. No handle is kept any more; the callback uses the handle of > the core it runs for, which is bound for as long as the call lasts. > "A reboot" is dropped from the kexec sentence and the comment: whether the > clock selectors survive the global reset the firmware does on reboot has > not been checked. The argument that lowering the rate is safe is now > limited to a boot rate the firmware serves from GPLL, which on the RK3588 > is the 200 MHz the devicetree pins. >=20 > drivers/accel/rocket/rocket_core.c | 10 ++++++ > drivers/accel/rocket/rocket_device.h | 15 +++++++++ > drivers/accel/rocket/rocket_drv.c | 47 ++++++++++++++++++++++++++++ > 3 files changed, 72 insertions(+) >=20 > diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/ro= cket_core.c > index 5dd260bacbff6..c736537cf28f6 100644 > --- a/drivers/accel/rocket/rocket_core.c > +++ b/drivers/accel/rocket/rocket_core.c > @@ -12,6 +12,7 @@ > #include > =20 > #include "rocket_core.h" > +#include "rocket_device.h" > #include "rocket_job.h" > =20 > int rocket_core_init(struct rocket_core *core) > @@ -36,6 +37,15 @@ int rocket_core_init(struct rocket_core *core) > if (err) > return dev_err_probe(dev, err, "failed to get clocks for core %d\n", c= ore->index); > =20 > + /* > + * 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. > + */ shorter comment please > + if (!core->rdev->npu_boot_rate) > + core->rdev->npu_boot_rate =3D clk_get_rate(core->clks[2].clk); see comment about using a constant for the array index in patch 5. > + > core->pc_iomem =3D 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_iome= m)); > diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/= rocket_device.h > index abb88a254e569..ba7c977cd6951 100644 > --- a/drivers/accel/rocket/rocket_device.h > +++ b/drivers/accel/rocket/rocket_device.h > @@ -22,6 +22,21 @@ struct rocket_device { > unsigned int num_cores; > /* Slot capacity (DT core count); slots with a NULL .dev are free. */ > unsigned int max_cores; > + > + /* > + * The cores have no clock of their own: one clock feeds all of them, > + * so any core's handle refers to the same thing. No handle is kept > + * here: each one belongs to the devres of the core that asked for it > + * and dies with that core's unbind, while this structure outlives any > + * single core. Whoever needs the clock uses the handle of the core it > + * was called for, which is bound for as long as the call lasts. > + * > + * npu_boot_rate is the rate the clock was left at before the driver > + * touched it, and active_cores counts the cores that are runtime > + * resumed right now. > + */ shorter comment please > + unsigned long npu_boot_rate; > + atomic_t active_cores; > }; > =20 > struct rocket_device *rocket_device_init(struct platform_device *pdev, > diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/roc= ket_drv.c > index b9b36c578db20..8f03de1af488c 100644 > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c > @@ -297,6 +297,30 @@ static int find_core_for_dev(struct device *dev) > return -1; > } > =20 > +/* > + * Put the compute clock back where the bootloader had it. The cores sha= re > + * this clock, so this is only correct once none of them is running any = more. > + * > + * Lowering the rate is safe with the power islands down as long as the = boot > + * rate is one the firmware serves from GPLL, which on the RK3588 is the > + * 200 MHz the devicetree pins: for that rate the firmware touches only = the > + * CRU clock selectors, none of the NPU's own registers. shorter comments please. Thanks Heiko