Message-ID: <a729c591-694f-4bed-bf09-b4430381766f@cherry.de>
Date: Wed, 8 May 2024 12:50:12 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Alexey Charkov <alchark@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Daniel Lezcano <daniel.lezcano@...aro.org>, Dragan Simic
 <dsimic@...jaro.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 v4 6/6] arm64: dts: rockchip: Add OPP data for CPU cores
 on RK3588

Hi Alexey,

On 5/8/24 11:43 AM, Alexey Charkov wrote:
> Hi Quentin,
> 
> On Wed, May 8, 2024 at 1:12 PM Quentin Schulz <quentin.schulz@...rry.de> wrote:
>>
>> Hi Alexey,
>>
>> On 5/6/24 11:36 AM, Alexey Charkov wrote:
>>> By default the CPUs on RK3588 start up in a conservative performance
>>> mode. Add frequency and voltage mappings to the device tree to enable
>>> dynamic scaling via cpufreq.
>>>
>>> OPP values are adapted from Radxa's downstream kernel for Rock 5B [1],
>>> stripping them down to the minimum frequency and voltage combinations
>>> as expected by the generic upstream cpufreq-dt driver, and also dropping
>>> those OPPs that don't differ in voltage but only in frequency (keeping
>>> the top frequency OPP in each case).
>>>
>>> Note that this patch ignores voltage scaling for the CPU memory
>>> interface which the downstream kernel does through a custom cpufreq
>>> driver, and which is why the downstream version has two sets of voltage
>>> values for each OPP (the second one being meant for the memory
>>> interface supply regulator). This is done instead via regulator
>>> coupling between CPU and memory interface supplies on affected boards.
>>>
>>
>> I'm not sure this is everything we need though.
>>
>> For the LITTLE cores cluster, all OPPs up to 1.416GHz are using the same
>> opp-supported-hw, however the ones above, aren't.
> 
> Thanks a lot for pointing this out - could you please elaborate which
> downstream kernel you referred to?
> 

The one provided by Rockchip directly :) No intermediates.

I can give you the one we use on our products at the moment: 
https://git.embedded.cherry.de/tiger-linux.git/ (or jaguar-linux, 
doesn't matter).

The one that is (publicly) "maintained" by Rockchip is:
https://github.com/rockchip-linux/kernel/tree/develop-5.10

 From Cherry's git repo:
"""
$ rg -B1 --color never -N opp-supported-hw 
arch/arm64/boot/dts/rockchip/rk3588s.dtsi
		opp-408000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-600000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-816000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1008000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1200000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1416000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1608000000 {
			opp-supported-hw = <0xfb 0xffff>;
--
		opp-1704000000 {
			opp-supported-hw = <0x02 0xffff>;
--
		opp-1800000000 {
			opp-supported-hw = <0xf9 0xffff>;
--
		opp-408000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-600000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-816000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1008000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1200000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1416000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1608000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1800000000 {
			opp-supported-hw = <0xfb 0xffff>;
--
		opp-2016000000 {
			opp-supported-hw = <0xfb 0xffff>;
--
		opp-2208000000 {
			opp-supported-hw = <0xf9 0xffff>;
--
		opp-2256000000 {
			opp-supported-hw = <0xf9 0x13>;
--
		opp-2304000000 {
			opp-supported-hw = <0xf9 0x24>;
--
		opp-2352000000 {
			opp-supported-hw = <0xf9 0x48>;
--
		opp-2400000000 {
			opp-supported-hw = <0xf9 0x80>;
--
		opp-408000000 {
			opp-supported-hw = <0xff 0x0ffff>;
--
		opp-600000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-816000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1008000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1200000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1416000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1608000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-1800000000 {
			opp-supported-hw = <0xfb 0xffff>;
--
		opp-2016000000 {
			opp-supported-hw = <0xfb 0xffff>;
--
		opp-2208000000 {
			opp-supported-hw = <0xf9 0xffff>;
--
		opp-2256000000 {
			opp-supported-hw = <0xf9 0x13>;
--
		opp-2304000000 {
			opp-supported-hw = <0xf9 0x24>;
--
		opp-2352000000 {
			opp-supported-hw = <0xf9 0x48>;
--
		opp-2400000000 {
			opp-supported-hw = <0xf9 0x80>;
--
		opp-300000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-400000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-500000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-600000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-700000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-800000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-900000000 {
			opp-supported-hw = <0xfb 0xffff>;
--
		opp-1000000000 {
			opp-supported-hw = <0xfb 0xffff>;
--
		opp-300000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-400000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-500000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-600000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-700000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-800000000 {
			opp-supported-hw = <0xff 0xffff>;
--
		opp-900000000 {
			opp-supported-hw = <0xfb 0xffff>;
--
		opp-1000000000 {
			opp-supported-hw = <0xfb 0xffff>;
"""

In order: LITTLE, big0, big1, DMC (memory), GPU and then NPU OPP table.

Looking at the 6.1 development branch from Rockchip 
(https://github.com/JeffyCN/mirrors/blob/kernel-6.1). The LITTLE cluster 
OPPs seem to all be using the same opp-supported-hw entry now (but 
different from the one in 5.10). But, the big cluster OPPs in 6.1 are 
matching the one in 5.10 (that is, not the ones from Radxa).

>> 1.608GHz, 1.704GHz and 1.8GHz are all using different opp-supported-hw.
> 
> In Radxa's downstream kernel source that I looked at [1] the LITTLE
> core cluster has all OPPs listed with opp-supported-hw = <0xff
> 0xffff>;
> 
>> Similarly, for the big cores clusters, all OPPs up to 1.608GHz are using
>> the same opp-supported-hw, but not the ones above.
>>
>> 1.8GHz and 2.016GHz, 2.208GHz, 2.256GHz, 2.304GHz, 2.352GHz and 2.4GHz
>> all have a different opp-supported-hw.
> 
> Hmm, only 2.256GHz, 2.304GHz and 2.352GHz in the sources I'm looking
> at have a different opp-supported-hw = <0xff 0x0>; (but note that I
> dropped them all from my patch here)
> 

Seems to be a change made by Radxa folks: 
https://github.com/radxa/kernel/commit/cf277d5eb46ef55517afffa10d48dd71bdd00c61 
(yay to no commit log \o/)

>> The values in that array are coming from cpu leakage (different for
>> LITTLE, big0 and big1 clusters) and "specification serial number"
>> (whatever that means), those are coming from the SoC OTP. In the
>> downstream kernel from Rockchip, the former value is called "SoC
>> Version" and the latter "Speed Grade".
> 
>  From what I understood by studying Radxa's downstream kernel sources
> and TF-A sources [2], the "leakage" in NVMEM cells drives the
> selection of power-optimized voltage levels (opp-microvolt-L1 through
> opp-microvolt-L7) for each OPP depending on a OTP-programmed silicon
> quality metric, whereas in my patch I only kept the most conservative
> voltage values for each OPP (i.e. highest-voltage default ones) and
> not the power-optimized ones.
> 
> So the proposed patch should (supposedly?) work on any silicon, only
> the heat death of the universe becomes marginally closer :)
> 

An OPP from the DT is selected if _opp_is_supported returns true. This 
is based on supported_hw member of the opp_table, which we set through 
dev_pm_opp_set_supported_hw. This is called by 
drivers/cpufreq/rockchip-cpufreq.c with two values: SoC Version and 
Speed Grade. The SoC version is a bitmap set by rk3588_get_soc_info by 
reading specification_serial_number region in the OTP and reading the 
first byte. If it is anything but 0xd (RK3588M) or 0xa (RK3588J), it is 
BIT(0).

To know if the opp is supported, you extract the first value of the 
array and mask it with the value gotten from rk3588_get_soc_info (the 
bitfield). This means that for RK3588 (and not the M or J variant), the 
first value of the OPP opp-supported-hw is a match if it is an odd 
number, so only opp-1704000000 in LITTLE cluster is excluded (on that 
sole match).

The second value in opp-supported-hw seems to be derived somehow from 
the cpu_leakage OTP. This is likely the same rabbit hole you dug two 
months ago, so I'll trust your findings there to avoid getting my hands 
dirty :)

In summary, false alarm (but still surprising changes made by Radxa 
here, not that they matter if they only run their kernel on "pure" 
RK3588). Sorry for the noise, and thanks for the explanations :)

I'm surprised that we removed the lowest frequencies at the same 
voltage, are they not even allowing us to save a teeny tiny bit of power 
consumption? (I'm asking because I'm pretty sure we'll eventually get 
customers complaining the CPU freq doesn't go in super low frequency "so 
this must be a way to consume less power in idle!").

Cheers,
Quentin

Powered by blists - more mailing lists