Message-ID: <a56b59a21dc3c21192fe45197eee4865@manjaro.org>
Date: Wed, 12 Mar 2025 11:34:21 +0100
From: Dragan Simic <dsimic@...jaro.org>
To: Quentin Schulz <quentin.schulz@...rry.de>
Cc: Alexey Charkov <alchark@...il.com>, Heiko Stübner
 <heiko@...ech.de>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 Daniel Lezcano <daniel.lezcano@...aro.org>, Viresh Kumar
 <viresh.kumar@...aro.org>, Chen-Yu Tsai <wens@...nel.org>, Diederik de Haas
 <didi.debian@...ow.org>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org, Kever Yang <kever.yang@...k-chips.com>
Subject: Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores
 on RK3588j

Hello Quentin,

On 2025-03-12 11:15, Quentin Schulz wrote:
> On 2/16/25 1:32 PM, Alexey Charkov wrote:
>> On Sat, Feb 15, 2025 at 11:30 PM Heiko Stübner <heiko@...ech.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@...rry.de> wrote:
>>>>> On 6/17/24 8:28 PM, Alexey Charkov wrote:
> [...]
>>>>>> +     };
>>>>>> +
>>>>>> +     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
> 
> Rockchip support stated:
> 
> """
> If you run overdrive mode, you do not need to include rk3588j.dtsi,
> and you can change it to #incldue rk3588.dtsi to ensure that the
> maximum frequency can reach 2GHz
> 
> below picture from datasheet.
> """
> 
> The picture is the table 3-2 Recommended operating conditions, page 30
> from the RK3588J datasheet, e.g.
> https://www.lcsc.com/datasheet/lcsc_datasheet_2403201054_Rockchip-RK3588J_C22364189.pdf
> 
> What is overdrive?
> 
> """
> Overdrive mode brings higher frequency, and the voltage will increase
> accordingly. Under
> the overdrive mode for a long time, the chipset may shorten the
> lifetime, especially in high temperature condition
> """
> 
> according to the same datasheet, end of the same table, page 31.
> 
> so this seems like a turbo-mode frequency to me.
> 
> Additionally, the note for the "normal mode" (the one with the lower
> freqs) states:
> 
> """
> Normal mode means the chipset works under safety voltage and frequency. 
> For the
> industrial environment, highly recommend to keep in normal mode, the 
> lifetime is
> reasonably guaranteed.
> """
> 
> I believe "industrial environment" means RK3588J used in the
> temperatures that aren't OK for the standard RK3588 variant?

Thanks a lot for obtaining these clarifications!

Yes, I'd say that in this case "industrial environment" boils down
to the extended temperature range for the RK3588J variant.

>> 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).
>> 
>>> So while the situation might be strange for the rk3588j, I really 
>>> only want
>>> correct frequencies here please - not any pseudo "turbo freqs".
>>> 
>>> I don't care that much what people do on their own device{s/trees}, 
>>> but
>>> the devicetrees we supply centrally (and to u-boot, etc) should only
>>> contain frequencies vetted by the manufacturer.
>> 
>> Fair enough. Let's just try and get another data point on whether
>> these frequencies are vetted or not.
> 
> So the higher freqs seems to be vetted (and used by default on
> Rockchip's BSP kernel), it just feels like you aren't really supposed
> to run those higher frequencies all the time? And especially not in
> "industrial environment"?
> 
> I would assume we want to stay on the safer side and remove those
> higher frequencies? Heiko?

I agree, we should remove the higher-frequency OPPs.  I'd like
to go through everything once again in detail and prepare a patch
that removes the unsafe OPPs, and you could review it once it's
on the ML, to make sure it's fine.

Powered by blists - more mailing lists