Received-SPF: pass (google.com: domain of linux-kernel+bounces-172984-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240506-rk-dts-additions-v4-0-271023ddfd40@gmail.com>
 <20240506-rk-dts-additions-v4-6-271023ddfd40@gmail.com> <e220061e-0bb5-4fa4-9a91-aefd81e6636c@cherry.de>
In-Reply-To: <e220061e-0bb5-4fa4-9a91-aefd81e6636c@cherry.de>
From: Alexey Charkov <alchark@gmail.com>
Date: Wed, 8 May 2024 13:43:17 +0400
Message-ID: <CABjd4YweBc_0d=ej-=0o6FPdHxh-i3FSbSp5bDs9cZcmosEMeA@mail.gmail.com>
Subject: Re: [PATCH v4 6/6] arm64: dts: rockchip: Add OPP data for CPU cores
 on RK3588
To: Quentin Schulz <quentin.schulz@cherry.de>
Cc: Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Heiko Stuebner <heiko@sntech.de>, Daniel Lezcano <daniel.lezcano@linaro.org>, 
	Dragan Simic <dsimic@manjaro.org>, Viresh Kumar <viresh.kumar@linaro.org>, 
	Chen-Yu Tsai <wens@kernel.org>, Diederik de Haas <didi.debian@cknow.org>, devicetree@vger.kernel.org, 
	linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, 
	linux-kernel@vger.kernel.org, Kever Yang <kever.yang@rock-chips.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Quentin,

On Wed, May 8, 2024 at 1:12=E2=80=AFPM Quentin Schulz <quentin.schulz@cherr=
y.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 droppin=
g
> > 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?

> 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 =3D <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 =3D <0xff 0x0>; (but note that I
dropped them all from my patch here)

> 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 :)

> I think this may have something to do with "binning" and I would see the
> ones above the "common" OPPs as "overclocking". Not all CPUs would
> support them and some may not run stable at some lower frequency than
> their stable max. Adding Kever from Rockchip in Cc to have some input on
> the need to support those.

Would be great to understand those in more detail, indeed!

Thanks a lot,
Alexey

[1] https://github.com/radxa/kernel/blob/c428536281d69aeb2b3480f65b2b227210=
b61535/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L588
[2] https://lore.kernel.org/linux-rockchip/CABjd4YzTL=3D5S7cS8ACNAYVa730WA3=
iGd5L_wP1Vn9=3Df83RCORA@mail.gmail.com/