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

[PATCH] 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.

Fixes: ed98261b4168 ("accel/rocket: Add a new driver for Rockchip's NPU")
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
Sent following Tomeu's request to send fixes upfront. Jiaxing Hu's
RK3576 enablement series carries the same id assignments inside
[RFC PATCH v2 6/8] accel/rocket: add RK3576 NPU (RKNN) support
(Message-Id: <20260718031146.3368811-7-gahing@gahingwoo.com>), together
with new per-SoC match data. This standalone fix is intentionally
minimal - ARRAY_SIZE(core->clks) and the clks[4] array are left
untouched - so it rebases trivially under both the RK3576 v3 and the
RK3568 series, and can be backported.

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

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

[Jiaxing Hu]

Hi Igor,

Thanks for asking first rather than just sending it -- and for the way
you did it. Sending it standalone is the right call; it should not have
been sitting in my RFC.

The analysis matches what I see. devm_clk_bulk_get() calls
clk_get(dev, clks[i].id) per entry, and with a NULL id of_parse_clkspec()
never looks at "clock-names", so all four resolve to index 0. The ids and
their order match both the binding

  Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml
    clock-names: aclk, hclk, npu, pclk

and the in-tree DT (rk3588-base.dtsi, all three cores). It also mirrors
the core->resets[].id block right above it, so it reads consistently.

Reviewed-by: Jiaxing Hu <gahing@gahingwoo.com>

I can't give you a Tested-by: I don't have an RK3588 board at hand at the
moment. What I can say is that the same four assignments are board-tested
on RK3576 -- they are byte-for-byte the ones in my v2 6/8, running on a
ROCK 4D, where the NPU probes, powers up and down through runtime PM and
executes jobs. So between us the change has been exercised on two SoCs.

One thing I would put in the commit message, because it is a real
behaviour change and not obvious from the diff: before this, the four
requests resolved by index and therefore always succeeded, whatever the
DT said. After it they resolve by name, so a DT that does not carry all
four names now fails probe with -ENOENT instead of quietly working. That
is fine in-tree -- rk3588-base.dtsi has all four and the binding makes
clock-names required with exactly those items -- but it is worth a
sentence, particularly for the RK3568 series you mention: a DT there that
does not list all four names would go from silently working to not
probing at all, and that is a much easier failure to diagnose if the log
message says so.

On the rebase, your reading is right, with one correction: v3 is not out
yet. I have not posted it. When I do, that hunk shrinks to just the two
new CBUF ids (aclk_cbuf, hclk_cbuf) at clks[4] and clks[5] with
ARRAY_SIZE going to 6. Nothing of yours has to move either way.

On the iommu patches: confirmed, and it was Will who applied them
himself. Both are in linux-next as of next-20260727:

  841363ebb508 iommu/rockchip: Take all DT clocks
  b10d5920cafa iommu/rockchip: Clear stale page faults before enabling stall

Thanks again,
Jiaxing

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

[Igor Paunovic]

Hi Jiaxing,

Thanks a lot for the review - and for the RK3576 data point. Good to
know the same four ids are running on a ROCK 4D; that makes two SoCs
between us indeed.

Good catch on the behaviour change: resolving by name turns a
permissive lookup into a strict one, and that deserves to be findable
in git log when an out-of-tree DT without all four clock-names stops
probing. I have added a paragraph about it to the commit message and
will send a v2 right away with your Reviewed-by collected.

Noted on the v3 timing, and thanks for confirming the iommu patches in
linux-next.

Thanks,
Igor