[PATCH v2] accel/rocket: request the core clocks by name

[PATCH v2] 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>
Reviewed-by: Jiaxing Hu <gahing@gahingwoo.com>
---
v2:
 - document that resolving by name is a behaviour change for DTs that
   do not carry all four clock-names (Jiaxing Hu)
 - collect Jiaxing's Reviewed-by
v1: https://lore.kernel.org/linux-rockchip/20260729092939.118779-1-royalnet026@gmail.com/

The same four id assignments are board-tested on RK3576 (ROCK 4D) as
part of Jiaxing's RK3576 enablement series (v2 6/8), so the change has
been exercised on two SoCs between us.

Verified on RK3588 (Orange Pi 5 Plus): after the change clk_summary
shows one consumer handle per clock instead of four handles on aclk, the
NPU still powers up and down cleanly through runtime PM, and a
MobileNetV1 inference run via the Teflon TFLite delegate produces
bit-identical output tensors to the unpatched driver.

 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 b3b2fa9..5dd260b 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);

[PATCH v2] 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>
Reviewed-by: Jiaxing Hu <gahing@gahingwoo.com>
---
v2:
 - document that resolving by name is a behaviour change for DTs that
   do not carry all four clock-names (Jiaxing Hu)
 - collect Jiaxing's Reviewed-by
v1: https://lore.kernel.org/linux-rockchip/20260729092939.118779-1-royalnet026@gmail.com/

The same four id assignments are board-tested on RK3576 (ROCK 4D) as
part of Jiaxing's RK3576 enablement series (v2 6/8), so the change has
been exercised on two SoCs between us.

Verified on RK3588 (Orange Pi 5 Plus): after the change clk_summary
shows one consumer handle per clock instead of four handles on aclk, the
NPU still powers up and down cleanly through runtime PM, and a
MobileNetV1 inference run via the Teflon TFLite delegate produces
bit-identical output tensors to the unpatched driver.

 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 b3b2fa9..5dd260b 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);

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2] accel/rocket: request the core clocks by name

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] If `rocket_device_init` fails during the first core's probe, the global `rdev` pointer is left as an `ERR_PTR` rather than being reset to `NULL`, causing a kernel panic on subsequent probes.
- [High] The driver leaks the shared `rdev` and `rdev->cores` structures upon probe deferral or failure.
- [High] The driver incorrectly uses `rdev->num_cores` both as a count of active cores and as the boundary index, causing state corruption and resource leaks upon out-of-order unbind.
--

--- Patch [1]: [PATCH v2] accel/rocket: request the core clocks by name ---
commit 4687ac2abf99b6ec4120f9d98c0f4148fc2a6b97
Author: Igor Paunovic <royalnet026@gmail.com>
accel/rocket: request the core clocks by name

This commit fixes clock initialization by setting the proper names for the
"aclk", "hclk", "npu", and "pclk" clocks before requesting them. Previously,
all four clocks defaulted to index 0, which meant only the AXI clock was
controlled by the driver.

> 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);

[Severity: High]
This isn't a bug introduced by this patch, but this devm_clk_bulk_get() call
can defer probe. If rocket_core_init() fails here, I noticed a potential 
memory leak in the cleanup path.

In drivers/accel/rocket/rocket_device.c:rocket_device_init(), the rdev 
structure is allocated via devres attached to the static drm_dev device:

drivers/accel/rocket/rocket_device.c:rocket_device_init() {
...
	rdev = devm_drm_dev_alloc(dev, rocket_drm_driver, struct rocket_device, ddev);
...
}

However, if probe fails, drivers/accel/rocket/rocket_drv.c:rocket_probe() 
abandons it without freeing the devres:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
...
		if (rdev->num_cores == 0) {
			rocket_device_fini(rdev);
			rdev = NULL;
		}
...
}

Since drm_dev is never unbound while the module is loaded, does this leak 
the rdev and rdev->cores structures on every probe deferral?

[Severity: Critical]
This is a pre-existing issue, but looking at the error handling in 
drivers/accel/rocket/rocket_drv.c:rocket_probe():

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
...
	if (rdev == NULL) {
		/* First core probing, initialize DRM device. */
		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
		if (IS_ERR(rdev)) {
			dev_err(&pdev->dev, "failed to initialize rocket device\n");
			return PTR_ERR(rdev);
		}
	}

	unsigned int core = rdev->num_cores;
...
}

If rocket_device_init() returns an error, rdev is left holding an ERR_PTR 
instead of being reset to NULL. Will a subsequent probe attempt try to 
dereference the poisoned rdev pointer when evaluating rdev->num_cores?

[Severity: High]
This is a pre-existing issue, but does the driver mishandle out-of-order 
unbinds? 

In drivers/accel/rocket/rocket_drv.c:find_core_for_dev(), num_cores is used
as a strict upper bound:

drivers/accel/rocket/rocket_drv.c:find_core_for_dev() {
...
	for (unsigned int core = 0; core < rdev->num_cores; core++) {
		if (dev == rdev->cores[core].dev)
			return core;
	}
...
}

If an earlier core is unbound, num_cores is decremented. If a higher-indexed 
core is then unbound, find_core_for_dev() won't search far enough to find it. 

Furthermore, if another core is then bound, rocket_probe() inserts it using 
num_cores as the index:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
...
	rdev->cores[core].rdev = rdev;
	rdev->cores[core].dev = &pdev->dev;
...
}

Could this overwrite an active higher-indexed core that was never properly 
cleaned up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729130743.128876-1-royalnet026@gmail.com?part=1