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/[email protected]/ > > > 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
signature.asc
Description: This is a digitally signed message part
