Re: [PATCH v2 08/11] accel/rocket: restore the NPU clock boot rate before powering the cores down

[Heiko Stübner]

Am Dienstag, 22. September 2026, 10:01:11 Mitteleuropäische 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.
> 
> 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 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 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 writes only CRU clock selectors, never a register inside the NPU.
> 
> Assisted-by: LLM sparse checkpatch
> Signed-off-by: Igor Paunovic <royalnet026@xxxxxxxxx>
> ---
> 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.
> 
>  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(+)
> 
> diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_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 <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,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", 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.
> +	 */

shorter comment please

> +	if (!core->rdev->npu_boot_rate)
> +		core->rdev->npu_boot_rate = clk_get_rate(core->clks[2].clk);

see comment about using a constant for the array index in patch 5.

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