Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi all,

On 2/16/25 1:32 PM, Alexey Charkov wrote:
> Hi Heiko,
> 
> On Sat, Feb 15, 2025 at 11:30 PM Heiko Stübner <heiko@sntech.de> wrote:
>>
>> Am Samstag, 15. Februar 2025, 19:59:44 MEZ schrieb Alexey Charkov:
>>> On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>> On 6/17/24 8:28 PM, Alexey Charkov wrote:
>>>>> RK3588j is the 'industrial' variant of RK3588, and it uses a different
>>>>> set of OPPs both in terms of allowed frequencies and in terms of
>>>>> applicable voltages at each frequency setpoint.
>>>>>
>>>>> Add the OPPs that apply to RK3588j (and apparently RK3588m too) to
>>>>> enable dynamic CPU frequency scaling.
>>>>>
>>>>> OPP values are derived from Rockchip downstream sources [1] by taking
>>>>> only those OPPs which have the highest frequency for a given voltage
>>>>> level and dropping the rest (if they are included, the kernel complains
>>>>> at boot time about them being inefficient)
>>>>>
>>>>> [1] https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>>>>
>>>>
>>>> Funny stuff actually. Rockchip have their own downstream cpufreq driver
>>>> which autodetects at runtime the SoC variant and filter out OPPs based
>>>> on that info. And this patch is based on those values and filters.
>>>>
>>>> However, they also decided that maybe this isn't the best way to do it
>>>> and they decided to have an rk3588j.dtsi where they remove some of those
>>>> OPPs instead of just updating the mask/filter in their base dtsi
>>>> (rk3588s.dtsi downstream). See
>>>> https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>>>
>>> Funny stuff indeed! Judging by the comments in the file you
>>> referenced, those higher OPP values constitute an 'overdrive' mode,
>>> and apparently the chip shouldn't stay in those for prolonged periods
>>> of time. However, I couldn't locate any dts file inside Rockchip
>>> sources that would include this rk3588j.dtsi - so not sure if we
>>> should follow what it says too zealously.
>>>

That was a clear oversight on my side.

The commit adding support for the J/M OPPs in rk3588s.dtsi downstream 
just precedes the one adding the removal of the J OPPs in rk3588j.dtsi, 
so not sure what was the intended effect there. I'll open a ticket with 
them to see if I can get some answer until/unless Kever chimes in.

>>>> So...
>>>>
>>>>> Signed-off-by: Alexey Charkov <alchark@gmail.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/rockchip/rk3588j.dtsi | 108 ++++++++++++++++++++++++++++++
>>>>>    1 file changed, 108 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>>>>> index 0bbeee399a63..b7e69553857b 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>>>>> @@ -5,3 +5,111 @@
>>>>>     */
>>>>>
>>>>>    #include "rk3588-extra.dtsi"
>>>>> +
>>>>> +/ {
>>>>> +     cluster0_opp_table: opp-table-cluster0 {
>>>>> +             compatible = "operating-points-v2";
>>>>> +             opp-shared;
>>>>> +
>>>>> +             opp-1416000000 {
>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
>>>>> +                     opp-microvolt = <750000 750000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +                     opp-suspend;
>>>>> +             };
>>>>> +             opp-1608000000 {
>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
>>>>> +                     opp-microvolt = <887500 887500 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1704000000 {
>>>>> +                     opp-hz = /bits/ 64 <1704000000>;
>>>>> +                     opp-microvolt = <937500 937500 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>
>>>> None of those are valid according to Rockchip, we should have
>>>
>>> Well, valid but more taxing on the hardware apparently.
>>>
>>>>                  opp-1296000000 {
>>>>                          opp-hz = /bits/ 64 <1296000000>;
>>>>                          opp-microvolt = <750000 750000 950000>;
>>>>                          clock-latency-ns = <40000>;
>>>>                          opp-suspend;
>>>>                  };
>>>>
>>>> instead?
>>>
>>> I dropped this one because it uses a lower frequency but same voltage
>>> as the 1.416 GHz one, thus being 'inefficient' as the thermal
>>> subsystem says in the logs. It can be added back if we decide to
>>> remove opp-1416000000.

That was exactly my point, the 1.296GHz OPP would then be the highest 
frequency at that voltage.

>>>
>>>> and...
>>>>
>>>>> +     };
>>>>> +
>>>>> +     cluster1_opp_table: opp-table-cluster1 {
>>>>> +             compatible = "operating-points-v2";
>>>>> +             opp-shared;
>>>>> +
>>>>> +             opp-1416000000 {
>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
>>>>> +                     opp-microvolt = <750000 750000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1608000000 {
>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
>>>>> +                     opp-microvolt = <787500 787500 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1800000000 {
>>>>> +                     opp-hz = /bits/ 64 <1800000000>;
>>>>> +                     opp-microvolt = <875000 875000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-2016000000 {
>>>>> +                     opp-hz = /bits/ 64 <2016000000>;
>>>>> +                     opp-microvolt = <950000 950000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>
>>>> opp-1800000000 and opp-2016000000 should be removed.
>>>>
>>>> and...
>>>>
>>>>> +     };
>>>>> +
>>>>> +     cluster2_opp_table: opp-table-cluster2 {
>>>>> +             compatible = "operating-points-v2";
>>>>> +             opp-shared;
>>>>> +
>>>>> +             opp-1416000000 {
>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
>>>>> +                     opp-microvolt = <750000 750000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1608000000 {
>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
>>>>> +                     opp-microvolt = <787500 787500 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1800000000 {
>>>>> +                     opp-hz = /bits/ 64 <1800000000>;
>>>>> +                     opp-microvolt = <875000 875000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-2016000000 {
>>>>> +                     opp-hz = /bits/ 64 <2016000000>;
>>>>> +                     opp-microvolt = <950000 950000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>
>>>> opp-1800000000 and opp-2016000000 should be removed as well.
>>>>
>>>> Did I misunderstand what Rockchip did here? Adding Kever in Cc at least
>>>> for awareness on Rockchip side :)
>>>>
>>>> I guess another option could be to mark those above as
>>>>
>>>> turbo-mode;
>>>>
>>>> though no clue what this would entail. From a glance at cpufreq, it
>>>> seems that for Rockchip since we use the default cpufreq-dt, it would
>>>> mark those as unusable, so essentially a no-op, so better remove them
>>>> entirely?
>>>
>>> I believe the opp core just marks 'turbo-mode' opps as
>>> CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to the
>>> cpufreq core. But then to actually use those boost frequencies I would
>>> guess the governor needs to somehow know the power/thermal constraints
>>> of the chip?.. Don't know where that is defined.
>>
>> personally I don't believe in "boost" frequencies - except when they are
>> declared officially.
>>
>> Rockchip for the longest time maintains that running the chip outside
>> the declared power/rate envelope can reduce its overall lifetime.
>>
>> So for Rockchip in mainline a "it runs stable for me" has never been a
>> suitable reasoning ;-) .
> 
> My key concern here was perhaps that we are looking at a file inside
> the Rockchip source tree which looks relevant by the name of it, but
> doesn't actually get included anywhere for any of the boards. This may
> or may not constitute an endorsement by Rockchip of any OPPs listed
> there :-D
> 

Will ask for confirmation through their ticket system.

> I haven't seen a TRM for the J variant, nor do I have a board with
> RK3588J to run tests, so it would be great if Kever could chime in
> with how these OPPs are different from the others (or not).
> 

I do have access to some J variant product but that won't help if you 
need to run the boards for months/years at the highest freq in a 
temperature-controlled chamber at the highest supported temperature for 
the SoC. I don't think there's a TRM for the J variant, that wouldn't 
make sense as it's the exact same SoC, just a different binning. 
Similarly to what was done for RK3588S, RK3588S2, RK3582, RK3583, etc.. 
I don't think there's a TRM for those, but there's a datasheet, c.f. 
https://www.lcsc.com/datasheet/lcsc_datasheet_2403201054_Rockchip-RK3588J_C22364189.pdf 
but that says 1.6GHz for the A76 cores and 1.3GHz for the A55 while the 
the j-m OPP in rk3558s.dtsi downstream list the highest as 1.7GHz (but 
rk3588j.dtsi removes it).

Will report if Rockchip has answered on their ticket system.

Cheers,
Quentin