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

_______________________________________________
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

[Sidong Yang]

On Wed, Jul 29, 2026 at 03:07:43PM +0200, Igor Paunovic wrote:
> Found on an Orange Pi 5 Plus (RK3588) by reading the live clock tree:

I see the same thing on a Radxa ROCK 5B+ (RK3588), and the patch fixes
it here as well.

Before, clk_summary has the four rocket handles piled up on the AXI
clock, with no driver consumer at all on the other three:

  aclk_npu0       4x fdab0000.npu    1x npu@fdab0000
  hclk_npu0        -                 1x npu@fdab0000
  pclk_npu_root    -                 3x npu@fdXX0000
  scmi_clk_npu     -                 3x npu@fdXX0000

After the patch every clock has exactly one fdXX0000.npu consumer, and
scmi_clk_npu picks up driver handles for all three cores.

No regressions here:

 - 90 runtime PM suspend/resume cycles over the three cores: no
   power-domain ack failures, no SErrors, nothing logged at all.
   scmi_clk_npu stays at its boot rate and returns to enable_cnt 0
   when the cores go idle.

 - Five MobileNetV2 subgraphs through the Teflon TFLite delegate still
   match the CPU reference (max diff 1).  I could not measure any change
   in inference time either way, though run-to-run spread on this box is
   around 10%, so that only rules out a large regression.

Tested on v7.2-rc3.

Tested-by: Sidong Yang <sidong.yang@furiosa.ai>

Thanks,
Sidong

_______________________________________________
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

[Diederik de Haas]

Hi Igor,

On Wed Jul 29, 2026 at 3:07 PM CEST, Igor Paunovic wrote:
> 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().

I have confirmed both the 'old' situation and the 'new' one on both my
NanoPC-T6 LTS and NanoPC-T6 Plus (both RK3588), so feel free to add

Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, NanoPC-T6 Plus


Cheers,
  Diederik

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


_______________________________________________
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

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 4776 bytes --]

Hi,

On Wed, Jul 29, 2026 at 03:07:43PM +0200, Igor Paunovic wrote:
> 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>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
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

[Igor Paunovic]

Hi Tomeu,

A gentle ping on this one. It has been on the list since 29 July and has
picked up review and test coverage in the meantime:

  Reviewed-by: Jiaxing Hu <gahing@gahingwoo.com>
  Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
  Tested-by: Sidong Yang <sidong.yang@furiosa.ai>
  Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, NanoPC-T6 Plus

so it now carries two reviews and two independent test reports, covering
the two NanoPC-T6 variants besides my own board.

The reason I am pinging rather than simply waiting: Jiaxing's RK3576
enablement series is based on this patch as of v6,

  https://lore.kernel.org/linux-rockchip/20260806063413.350184-1-gahing@gahingwoo.com/

and he has a v7 in preparation. Up to v5 that series carried a duplicate
of the same hunk, which would have conflicted whichever of the two landed
first. Basing on this patch is the right fix, but it does mean his series
is now waiting behind it.

Nothing has changed in the patch itself since v2 and there is no open
comment on it, so it should apply as is. Happy to resend it with the tags
collected if that is easier for you.

Thanks,
Igor

_______________________________________________
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

[Igor Paunovic]

A correction to my own message above, before it stands uncorrected in
the archive.

I wrote that the two test reports cover "the two NanoPC-T6 variants
besides my own board". That is wrong, and it misattributes Sidong's
work. Sidong tested on a Radxa ROCK 5B+, not on a NanoPC-T6:

  https://lore.kernel.org/all/20260802131328.75038-1-sidong.yang@furiosa.ai/

Diederik's report is the one covering the two NanoPC-T6 variants. So the
coverage is three boards besides mine, on three different designs:

  Tested-by: Sidong Yang <sidong.yang@furiosa.ai>        # Radxa ROCK 5B+
  Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, NanoPC-T6 Plus

Apologies to Sidong for getting his hardware wrong - his report was the
more thorough of the two, with clk_summary before and after, 90 runtime
PM cycles across the three cores, and bit-exact inference results, and
it deserved to be quoted accurately.

Nothing else in that message changes.

Igor

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