accel/rocket: clks[].id fix - standalone patch, or would you prefer to split it out?

accel/rocket: clks[].id fix - standalone patch, or would you prefer to split it out?

[Igor Paunovic]

Hi Jiaxing,

I have been working with the rocket NPU driver on RK3588 (Orange Pi 5
Plus) and ran into the missing core->clks[].id initialisation in
rocket_core_init().

I noticed it while reading the live clock tree rather than the source:
/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. The driver believes it owns four clocks
but really holds four handles to the AXI clock.

Before writing anything up I went through the lists, and I see you
already fix exactly this as part of

  [RFC PATCH v2 6/8] accel/rocket: add RK3576 NPU (RKNN) support
  Message-Id: <20260718031146.3368811-7-gahing@gahingwoo.com>

so I would rather ask you first than send something that looks like a
duplicate.

The reason I care: I would like to look into OPP/devfreq support for
rocket later on - Jonas Karlman mentioned it as future work in the
RK3568 thread - and today the driver holds no reference at all to the
clock that actually feeds the NPU ("npu", SCMI_CLK_NPU on RK3588). That
is in the way of any frequency scaling work. The NPU still runs anyway -
clk_summary shows scmi_clk_npu with the hardware enable bit set but no
Linux consumer, i.e. firmware keeps it running - so the bug is latent
rather than fatal.

Timing-wise this also seems like a natural moment to ask: Will has
already picked up the two iommu patches from your v2, and after
Alexey's review you mentioned a v3 respin of the accel patch. Since the
RK3576 inference issue is still being debugged, the clks[].id fix would
otherwise sit inside an RFC for a while, even though it is independent
of that work and the bug it fixes is in mainline today.

Would you prefer to:

 (a) split the clks[].id fix out of your series and send it standalone
     with a Fixes: tag? That way it can land independently of the RK3576
     enablement work and be backported. I would be glad to give it a
     Tested-by on RK3588 / Orange Pi 5 Plus.

 (b) or, if you would rather keep your series as it is, I can send the
     standalone fix with you on Cc and a note under the --- pointing at
     your patch.

Either works for me. I mostly want to avoid us duplicating effort, and
you posted it first.

Thanks,
Igor Paunovic

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

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

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

[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

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but probe failure or probe deferral causes a devres memory leak on the global facade device (drm_dev).
- [High] This problem wasn't introduced by this patch, but the interrupt handler leaks an IOMMU group reference on every job completion.
--

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

This commit updates the rocket driver to explicitly request the core
clocks by name (aclk, hclk, npu, pclk). This ensures the correct clocks
are prepared and enabled during runtime power management, preventing a
latent issue where the driver held no reference to the actual NPU clock.

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

[Severity: High]
This isn't a bug introduced by this patch, but does the increased likelihood
of probe deferral here expose a devres memory leak?

In rocket_device_init(), rdev and rdev->cores are allocated via devm_ on the
global drm_dev:

drivers/accel/rocket/rocket_device.c:rocket_device_init() {
    ...
    rdev = devm_drm_dev_alloc(dev, rocket_drm_driver, struct rocket_device, ddev);
    ...
    rdev->cores = devm_kcalloc(dev, num_cores, sizeof(*rdev->cores), GFP_KERNEL);
    ...
}

If devm_clk_bulk_get() returns -EPROBE_DEFER, rocket_probe() cleans up by
calling rocket_device_fini() and nullifies rdev:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
    ...
    err = rocket_core_init(core);
    if (err) {
        rocket_device_fini(rdev);
        rdev = NULL;
        ...
    }
    ...
}

Because drm_dev is a global platform device that is never unbound, the devres
allocations remain attached to it. On the next probe retry, new allocations
are made, leaking the previous ones.

[Severity: High]
This problem wasn't introduced by this patch, but does the interrupt handler
leak an IOMMU group reference on every job completion?

Looking at rocket_job_handle_irq(), when a job finishes, it detaches the
IOMMU group:

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
    ...
    iommu_detach_group(NULL, iommu_group_get(core->dev));
    ...
}

The iommu_group_get() function increments the IOMMU group's reference count,
but a matching iommu_group_put() is never called. Because this is a hot path
triggered by hardware interrupts, the reference count will quickly overflow,
permanently leaking the structure. 

Could the driver use its cached core->iommu_group pointer here instead, 
similar to how it is handled in rocket_reset()?

>  	if (err)
>  		return dev_err_probe(dev, err, "failed to get clocks for core %d\n", core->index);

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

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

[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

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

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

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

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

Re: accel/rocket: clks[].id fix - standalone patch, or would you prefer to split it out?

[Igor Paunovic]

Hi Tomeu

(re-adding linux-rockchip to Cc, I think it got dropped by accident)

Will do - I just sent the clks[].id fix as a standalone patch:

[PATCH] accel/rocket: request the core clocks by name
https://lore.kernel.org/linux-rockchip/20260729092939.118779-1-royalnet026@gmail.com/

It is intentionally minimal (no per-SoC match data, ARRAY_SIZE and
clks[4] untouched) so it should rebase trivially under both the RK3576
v3 and the RK3568 series.

Jiaxing: your version in 6/8 of course stays correct - with this landed
first, that hunk in v3 just shrinks to the two new CBUF ids.
Thanks,

Igor

On Wed, Jul 29, 2026 at 10:22 AM Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>
> Hi Igor and Jiaxing,
>
> On Wed, Jul 29, 2026 at 9:20 AM Igor Paunovic <royalnet026@gmail.com> wrote:
> >
> > Hi Jiaxing,
> >
> > I have been working with the rocket NPU driver on RK3588 (Orange Pi 5
> > Plus) and ran into the missing core->clks[].id initialisation in
> > rocket_core_init().
> >
> > I noticed it while reading the live clock tree rather than the source:
> > /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. The driver believes it owns four clocks
> > but really holds four handles to the AXI clock.
> >
> > Before writing anything up I went through the lists, and I see you
> > already fix exactly this as part of
> >
> >   [RFC PATCH v2 6/8] accel/rocket: add RK3576 NPU (RKNN) support
> >   Message-Id: <20260718031146.3368811-7-gahing@gahingwoo.com>
> >
> > so I would rather ask you first than send something that looks like a
> > duplicate.
> >
> > The reason I care: I would like to look into OPP/devfreq support for
> > rocket later on - Jonas Karlman mentioned it as future work in the
> > RK3568 thread - and today the driver holds no reference at all to the
> > clock that actually feeds the NPU ("npu", SCMI_CLK_NPU on RK3588). That
> > is in the way of any frequency scaling work. The NPU still runs anyway -
> > clk_summary shows scmi_clk_npu with the hardware enable bit set but no
> > Linux consumer, i.e. firmware keeps it running - so the bug is latent
> > rather than fatal.
> >
> > Timing-wise this also seems like a natural moment to ask: Will has
> > already picked up the two iommu patches from your v2, and after
> > Alexey's review you mentioned a v3 respin of the accel patch. Since the
> > RK3576 inference issue is still being debugged, the clks[].id fix would
> > otherwise sit inside an RFC for a while, even though it is independent
> > of that work and the bug it fixes is in mainline today.
> >
> > Would you prefer to:
> >
> >  (a) split the clks[].id fix out of your series and send it standalone
> >      with a Fixes: tag? That way it can land independently of the RK3576
> >      enablement work and be backported. I would be glad to give it a
> >      Tested-by on RK3588 / Orange Pi 5 Plus.
> >
> >  (b) or, if you would rather keep your series as it is, I can send the
> >      standalone fix with you on Cc and a note under the --- pointing at
> >      your patch.
> >
> > Either works for me. I mostly want to avoid us duplicating effort, and
> > you posted it first.
>
> I would be glad if you could send any fixes upfront, specially if
> getting more SoCs supported will still take some time.
>
> Thanks,
>
> Tomeu
>
> > Thanks,
> > Igor Paunovic

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