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

[Nicolas Dufresne <nicolas@ndufresne.ca>]

[-- Attachment #1: Type: text/plain, Size: 6939 bytes --]

Hi,

Le mercredi 09 septembre 2026 à 11:18 +0200, Igor Paunovic a écrit :
> On Mon, 2026-09-08 at 15:44 -0400, Nicolas Dufresne wrote:
> 
> Thank you for taking the time on this - and no worries about the PoC not
> being posted; the credit in the commit message was deliberate, the rates
> and voltages really were arrived at twice independently.
> 
> > 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.
> > [...]
> > I'm curious what's the right approach, and what is the real meaning of
> > opp-shared if I got that wrong.
> 
> You got it right, and I got it wrong. The binding says (opp-v2-base.yaml):
> 
>   opp-shared: Indicates that device nodes using this OPP Table Node's
>   phandle switch their DVFS state together, i.e. they share
>   clock/voltage/current lines. Missing property means devices have
>   independent clock/voltage/current lines, but they share OPP tables.
> 
> That is exactly the hardware here: one compute clock and one supply for
> all three cores. And it is not only documentation for non-CPU devices
> either: in drivers/opp/of.c, _managed_opp() only lets several devices
> share a single opp_table instance when the table node carries opp-shared;
> without it each device that points at the same node gets its own table.
> 
> So the accurate description is the one you had: the same phandle on
> rknn_core_0/1/2 plus opp-shared on the table. Putting the table on core 0
> alone describes core 0 and says nothing about the other two, which is a
> worse description of the same hardware. I will change this in v2 unless a
> DT maintainer disagrees.

Great, let's hope a maintainer talks sooner then later, but it really looks
like the right approach.

> 
> One consequence is mine to fix on the driver side, not yours to work
> around in DT: the driver currently picks the core that carries the table
> by taking the first core whose node has operating-points-v2. With three
> carriers that choice - and with it the name under /sys/class/devfreq -
> would follow whichever core bound first. That is a driver bug the moment
> the DT stops being lopsided, so it gets fixed in the same v2.

I was not aware of the component framework, but some of the gpu driver, and
recently some of the media drivers are using this framework to facilitate
the initilization of n-cores in one combined device. Just food for the mind
here.

https://lore.kernel.org/all/20260810-rkvdec-multicore-v2-4-986f89d22cdc@collabora.com/

> 
> > 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.
> > [...]
> > 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).
> 
> Accepted, and it is a fair hit. The paragraph as written justifies a DT
> choice with a driver limitation, and that is backwards regardless of
> whether the limitation is real. It comes out in v2.
> 
> On the meaning: your interpretation matches mine, and it is stronger than
> "descriptive". The binding says opp-suspend "marks the OPP to be used
> during device suspend", and the devfreq core acts on it directly -
> devfreq_add_device() reads it into devfreq->suspend_freq, and
> devfreq_suspend_device() then sets that rate.
> 
> Which leaves the question you actually asked, so I measured it rather than
> argued it. On an Orange Pi 5 Plus with this series applied, in-tree rocket,
> the OPP table of 3/7 read out of DT, simple_ondemand with min_freq/max_freq
> left alone: 25 s of inference, then 60 s idle.
> 
>   - The governor takes it back down on its own. trans_stat records six
>     transitions for the run - 200->1000, 1000->800, 800->1000, 1000->900,
>     900->500, 500->200 - and then 60129 ms at 200 MHz with zero
>     milliseconds at any other level for the rest of the window. The step
>     down happened within one 250 ms sample of the load ending.
> 
>   - The transition completed, it was not merely requested. vdd_npu_s0 goes
>     700 -> 850 mV under load and sits flat at 700 mV for the whole idle
>     window. In _set_opp(), when scaling down, config_regulators() runs only
>     after config_clks() has returned success, so the supply could not have
>     come back down to the 200 MHz voltage if the SCMI clock set had failed.
>     All four voltages of the table were exercised: 700, 750, 800 and 850 mV.
> 
>   - The clock summary in debugfs agrees, reading 200000000 for scmi_clk_npu
>     after the load. I only sampled it after the load, so I am offering it as
>     consistent rather than as an independent check.
> 
> So the transition back works here, and by your own criterion it is fine not
> to use opp-suspend. This is one board and one part, so I would not call it
> more than that.

If you can script the test, I can run it on Rock5B later on.

> 
> Two things I want to keep apart rather than join with a "therefore", because
> joining them is what made the original paragraph wrong:
> 
>   - What guarantees the rate across suspend is the driver, not the governor
>     and not the DT. rocket_devfreq_suspend() in 5/7 sets the recorded boot
>     rate itself on the system suspend path, and 4/7 restores it before the
>     last core goes down and on .shutdown. That is the answer to "is it safe
>     without opp-suspend".
> 
>   - Whether the governor walks back down to 200 MHz when the NPU goes idle
>     is a separate fact, and it is the one you asked me to check. It does.
> 
> opp-suspend acts on the first of those. Since the driver already puts the
> device back at its boot rate on that path, the property has nothing left to
> do here - and that, rather than any deadlock, is the argument v2 will make.
> 
> Worth saying plainly: every number in the cover was taken with the limits
> pinned by hand, so this is the first time the governor was left to decide
> anything on this board. Your question is what exposed that.

I don't want to pretend the doc is ambiguous for this one, but its not 100%
fit, but is not 100% unfit either.

- opp-suspend: Marks the OPP to be used during device suspend. If multiple OPPs
  in the table have this, the OPP with highest opp-hz will be used.

I don't think we had to transition to 200MHz before suspending, but it
will be at 200MHz once resumed. I think some maintainer feedback here would
be helpful.

> 
> > This is irrelevant, I think you can drop this paragraph.
> 
> Agreed, dropped in v2.
> 
> > Ack, this is safe thing to do.
> 
> Thanks.
> 
> Regards,
> Igor

thanks for working on this.

cheers,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]