[PATCH 3/7] arm64: dts: rockchip: rk3588: add an OPP table for the NPU

Tue Sep 8 12:44:56 PDT 2026

Hi there,

please note, I didn't submit as I didn't finish learning the implication of
everything in the series. I've used AI because I needed something quick and
dirty for a demo. But let me ask few questions here though:

Le vendredi 04 septembre 2026 à 15:08 +0200, Igor Paunovic a écrit :
> The NPU compute clock is driven by the firmware, which only accepts one of
> the rates in its own PVTPLL table: 300, 400, 500, 600, 700, 800, 900 and
> 1000 MHz through the PVTPLL, plus 200 MHz off GPLL. Anything else comes
> back as SCMI_INVALID_PARAMETERS, so the table has to name those rates
> exactly rather than describe a range.
> 
> 200 MHz is included even though the vendor table stops at 300, because
> mainline pins the cores there with assigned-clock-rates and that is the
> rate the NPU boots and idles at. Leaving it out would put the boot state
> outside the table and give a driver nowhere to return to. Its voltage is
> the same 700 mV the vendor uses for 300 MHz, so it is conservative.
> 
> The voltages are the vendor's, and the upper half of the table matches the
> GPU table in this file step for step: 700 MHz at 700 mV, 800 at 750, 900 at
> 800, 1000 at 850. There is no PVTM or binning here, for the same reason the
> GPU table has none: mainline uses conservative worst-case voltages instead
> of per-chip nvmem data.
> 
> The table is attached to rknn_core_0 alone. All three cores share one clock
> and one supply and cannot be scaled independently, and the driver hangs its
> devfreq device off the core that carries the table.

The goal of DT is to describe the hardware. You made the choice to not describe
the rate of core 1 and 2, and also are missing something to describe the clock
relation (well indirectly you can probably notice they point to the same clock).
My impression, and I was to study this properly is that having the same table
on every core and adding the opp-shared set on it was actually probably the
proper way to describe this "single clock for all" relationship.

My driver implementation though hard coded this fact for simplicity, but if we
add a variant in the future that does not have this limitation, we can just read
the opp-shared property to differentiate them instead of coding it for every
compatibles. Matching clock to be the same would also be an option, but more
work. I'm curious what's the right approach, and what is the real meaning of
opp-shared if I got that wrong.

> 
> The full SoC range is described rather than a per-board subset, so that a
> board which cannot cool the upper rates drops them in its own .dts with a
> /delete-node/ on the OPP it does not want. A board may only delete OPPs
> that way, never invent intermediate ones: a rate that is not in the
> firmware's table is rejected outright.
> 
> There is deliberately no opp-suspend property. The driver has to resume
> every core before it may touch the shared clock, so letting the devfreq
> core drive a suspend OPP from inside a runtime-suspend callback would
> deadlock against the driver's own governor worker. The driver records the
> boot rate and restores it itself instead.

We must not justify our DTS choices based on driver behaviours (or miss-
behaviour). We must justify it based on how accurate the hardware description
is. In my attempt, I was unable to go back to 200MHz and I could only resume at
200MHz (could have been a bug ...). So opp-suspend described the rate the core
will be once resumed. But it goes a little confused, as resume/suspend isn't per
core. I'm also curious the exact meaning of opp-suspend, and if my
interpretation was right or wrong.

It should probably be fine to not use opp-suspend, if transition back to 200Mhz
works. It not fine if its to avoid a driver deadlock (argually due to a bug).

> 
> The consumer is the devfreq support added later in this series; until then
> the table is inert and the NPU keeps the fixed rate that
> assigned-clock-rates gives it today.

This is irrelevant, I think you can drop this paragraph.

> 
> rk3588j.dtsi does not include this file; it carries its own derated tables
> for the CPU clusters and the GPU, and it gets no NPU table here. That is
> deliberate. The J part is rated lower than the rates in this table and none
> of it can be measured on the hardware this was written on, so inventing a
> derated NPU table would be guessing. Its NPU node stays disabled, so
> nothing binds and the cooling map added later in this series is simply
> never resolved.

Ack, this is safe thing to do.

> 
> The same rates and voltages were arrived at independently by Nicolas
> Dufresne in a proof of concept that was never posted to the list; his
> version differs in that it marks 200 MHz as opp-suspend, shares one table
> across all three cores and drops the assigned-clock-rates pins.
> Link: https://gitlab.collabora.com/nicolas/linux/-/commits/rock5b-npu-poc-4
> 
> Signed-off-by: Igor Paunovic <royalnet026 at gmail.com>
> Assisted-by: LLM checkpatch dtbs_check
> ---
>  arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi | 45 ++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
> index b5d630d2c879f..3711727020ed1 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
> @@ -151,6 +151,47 @@ opp-1000000000 {
>  			opp-microvolt = <850000 850000 850000>;
>  		};
>  	};
> +
> +	npu_opp_table: opp-table-npu {
> +		compatible = "operating-points-v2";
> +
> +		opp-200000000 {
> +			opp-hz = /bits/ 64 <200000000>;
> +			opp-microvolt = <700000 700000 850000>;
> +		};
> +		opp-300000000 {
> +			opp-hz = /bits/ 64 <300000000>;
> +			opp-microvolt = <700000 700000 850000>;
> +		};
> +		opp-400000000 {
> +			opp-hz = /bits/ 64 <400000000>;
> +			opp-microvolt = <700000 700000 850000>;
> +		};
> +		opp-500000000 {
> +			opp-hz = /bits/ 64 <500000000>;
> +			opp-microvolt = <700000 700000 850000>;
> +		};
> +		opp-600000000 {
> +			opp-hz = /bits/ 64 <600000000>;
> +			opp-microvolt = <700000 700000 850000>;
> +		};
> +		opp-700000000 {
> +			opp-hz = /bits/ 64 <700000000>;
> +			opp-microvolt = <700000 700000 850000>;
> +		};
> +		opp-800000000 {
> +			opp-hz = /bits/ 64 <800000000>;
> +			opp-microvolt = <750000 750000 850000>;
> +		};
> +		opp-900000000 {
> +			opp-hz = /bits/ 64 <900000000>;
> +			opp-microvolt = <800000 800000 850000>;
> +		};
> +		opp-1000000000 {
> +			opp-hz = /bits/ 64 <1000000000>;
> +			opp-microvolt = <850000 850000 850000>;
> +		};
> +	};
>  };
>  
>  &cpu_b0 {
> @@ -188,3 +229,7 @@ &cpu_l3 {
>  &gpu {
>  	operating-points-v2 = <&gpu_opp_table>;
>  };
> +
> +&rknn_core_0 {
> +	operating-points-v2 = <&npu_opp_table>;
> +};
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20260908/abf15f93/attachment.sig>