Message-ID: <c547d6165ae80d4008966021eb0d1ef2@manjaro.org>
Date: Wed, 29 May 2024 04:16:49 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Alexey Charkov <alchark@...il.com>
Cc: Quentin Schulz <quentin.schulz@...rry.de>, Heiko Stuebner
 <heiko@...ech.de>, Chen-Yu Tsai <wens@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Daniel Lezcano <daniel.lezcano@...aro.org>, Diederik
 de Haas <didi.debian@...ow.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Rob Herring <robh+dt@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v4 0/6] RK3588 and Rock 5B dts additions: thermal, OPP and
 fan

On 2024-05-29 02:35, Dragan Simic wrote:
> On 2024-05-28 21:26, Alexey Charkov wrote:
>> On Tue, May 28, 2024 at 8:08 PM Quentin Schulz 
>> <quentin.schulz@...rry.de> wrote:
>>> On 5/28/24 5:42 PM, Alexey Charkov wrote:
>>> > On Tue, 28 May 2024 at 19:16, Heiko Stuebner <heiko@...ech.de> wrote:
>>> >> Am Dienstag, 28. Mai 2024, 17:01:48 CEST schrieb Dragan Simic:
>>> >>> On 2024-05-28 16:34, Heiko Stuebner wrote:
>>> >>>> Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic:
>>> >>>>> On 2024-05-28 11:49, Alexey Charkov wrote:
>>> >>>>>> Hi Heiko,
>>> >>>>>>
>>> >>>>>> Do you think this can be merged for 6.11? Looks like there hasn't
>>> >> been
>>> >>>>>> any new feedback in a while, and it would be good to have frequency
>>> >>>>>> scaling in place for RK3588.
>>> >>>>>>
>>> >>>>>> Please let me know if you have any reservations or if we need any
>>> >>>>>> broader discussion.
>>> >>>>
>>> >>>> not really reservations, more like there was still discussion going on
>>> >>>> around the OPPs. Meanwhile we had more discussions regarding the whole
>>> >>>> speed binning Rockchip seems to do for rk3588 variants.
>>> >>>>
>>> >>>> And waiting for the testing Dragan wanted to do ;-) .
>>> >>>
>>> >>> I'm sorry for the delays.
>>> >>
>>> >> Was definitly _not_ meant as blame ;-) .
>>> >>
>>> >> The series has just too many discussions threads to unravel on half
>>> >> an afternoon.
>>> >
>>> >
>>> > FWIW, I think the latest exchange we had with Quentin regarding the OPPs
>>> > concluded in “false alarm”, given that this version of the series only
>>> > introduces a subset of them which should apply to all RK3588(s)
>>> 
>>> Correct.
>>> 
>>> However... I'm wondering if we shouldn't somehow follow the same 
>>> pattern
>>> we have used for the rk3399 OPPs? We have a file for the "true" 
>>> RK3399
>>> OPPs, then the OP1 variant and the RK3399T.
>>> 
>>> We already know there are a few variants of RK3588 with different 
>>> OPPs:
>>> RK3588(S/S2?), RK3588J and RK3588M. I wouldn't be surprised if the
>>> RK3582 (though this one has already one big cluster (or two big 
>>> cores)
>>> fewer than RK3588) has different OPPs as well?
>>> 
>>> So. We have already discussed that the OPPs in that patch are valid 
>>> for
>>> RK3588(S) but they aren't for the other variants.
>> 
>> Hmm. Looking at Rockchip sources [1] more closely as opposed to the
>> Radxa tree I've been using as the basis, it does indeed show that
>> RK3588J and RK3588M use a different OPP table altogether (frequencies
>> are similar, but voltages differ).
>> 
>> We currently have an RK3588J based board in the mainline tree
>> (rk3588-edgeble-neu6b-io.dts), so it can't be ignored. However, given
>> that Rockchip sources only differentiate those OPPs by SoC revision,
>> and that is (currently?) known for each board at dtb compile time, it
>> seems easier to just include two different OPP tables for RK3588(S)
>> vs. RK3588J - thus avoiding all the headache with opp-supported-hw.
>> Similar to RK3399, yes.
>> 
>> Will split those out and send a separate version.
> 
> Ah, you already answered my question about the existence of supported
> boards with the RK3588J and RK3588M variants. :) [2]  I totally agree
> about splitting the OPPs into the separate .dtsi files, i.e. following
> the approach already established by the RK3399.
> 
> Perhaps the new files should be named rk3588-opp.dtsi and 
> rk3588j-opp.dtsi,
> to (almost fully) follow the already established naming scheme.  Maybe
> the OPP data could instead be added to the already existing 
> rk3588.dtsi,
> rk3588s.dtsi and rk3588j.dtsi files, which would actually make more 
> sense
> because those .dtsi files already address the specific SoC variants, 
> but
> the approach with the separate new rk3588*-opp.dtsi files seems cleaner
> from the implementation point of view, and is a bit safer.
> 
> If you see a clean way for stuffing the OPP data into the already 
> existing
> rk3588.dtsi, rk3588s.dtsi and rk3588j.dtsi files, instead of 
> introducing
> new rk3588*-opp.dtsi files, I'd be happy to support it.
> 
> [2] 
> https://lore.kernel.org/linux-rockchip/7b09e18e850ff0832bd7236810b83e64@manjaro.org/

On second thought, it makes more sense to rename and shuffle the RK3588
dtsi files a bit, to make it possible to have the OPPs already specified
when a board dts file includes the dtsi file for a RK3588 variant.  
Thus,
I went ahead and prepared an RFC patch that does exactly that. [3]

Please, have a look at that RFC patch.  To quote its description:

---------------------------------------------------------------------------
Rename and modify the RK3588 dtsi files a bit, to make preparations for
the ability to specify different CPU and GPU OPPs for each of the 
supported
RK3588 SoC variants, including the RK3588J.

As already discussed, some of the different RK3588 SoC variants
require different OPPs, and it makes more sense to have the OPPs already
defined when a board dts includes one of the SoC dtsi files 
(rk3588.dtsi,
rk3588j.dtsi or rk3588s.dtsi), rather than requiring the board dts file 
to
also include a separate rk3588*-opp.dtsi file.  The choice of the SoC 
variant
is already made by the inclusion of the SoC dtsi file, and it doesn't 
make
much sense to, effectively, allow the board dts file to include and use 
an
incompatible set of OPPs for the already selected SoC variant.

No intended functional changes are introduced.  (Still to be 
additionally
verified if the patch moves forward from the RFC state.)
---------------------------------------------------------------------------

[3] 
https://lore.kernel.org/linux-rockchip/673dcf47596e7bc8ba065034e339bb1bbf9cdcb0.1716948159.git.dsimic@manjaro.org/

>>> In the downstream kernel, any OPP whose opp-supported-hw has a first
>>> value masked by BIT(1) return non-0 is supported by RK3588M. In the
>>> downstream kernel, any OPP whose opp-supported-hw has a first value
>>> masked by BIT(2) return non-0 is supported by RK3588J.
>>> 
>>> This means that, for LITTLE clusters:
>>> - opp-1608000000 not supported on RK3588J
>>> - opp-1704000000 only supported on RK3588M (but already absent in 
>>> this
>>> patch series)
>>> - opp-1800000000 only supported on RK3588(S), not RK3588J nor RK3588M
>>> 
>>> For big clusters:
>>> - opp-1800000000 not supported on RK3588J
>>> - opp-2016000000 not supported on RK3588J
>>> - opp-2208000000 only supported on RK3588(S), not RK3588J nor RK3588M
>>> - opp-2256000000 only supported on RK3588(S), not RK3588J nor RK3588M
>>> - opp-2304000000 only supported on RK3588(S), not RK3588J nor RK3588M
>>> - opp-2352000000 only supported on RK3588(S), not RK3588J nor RK3588M
>>> - opp-2400000000 only supported on RK3588(S), not RK3588J nor RK3588M
>>> 
>>> This is somehow also enforced in downstream kernel by removing the 
>>> OPP
>>> nodes directly (hence, not even requiring the check of 
>>> opp-supported-hw
>>> value), c.f.:
>>> https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>>> https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588m.dtsi
>>> 
>>> You'll not that the RK3588J also has less OPPs for the GPU and NPU 
>>> (but
>>> those should also be masked by the opp-supported-hw value).
>> 
>> Same with DMC, but we don't currently have either DMC or NPU in the
>> mainline tree, so it sounds like something to be dealt with later :)
>> 
>> [1] 
>> https://github.com/rockchip-linux/kernel/blob/develop-5.10/arch/arm64/boot/dts/rockchip/rk3588s.dtsi

Powered by blists - more mailing lists