Message-ID: <82db817a908b761d8c3d73ea04714314@manjaro.org>
Date: Thu, 30 May 2024 21:31:34 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Alexey Charkov <alchark@...il.com>
Cc: linux-rockchip@...ts.infradead.org, heiko@...ech.de,
 linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
 robh+dt@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 linux-kernel@...r.kernel.org, quentin.schulz@...rry.de, wens@...nel.org,
 daniel.lezcano@...aro.org, didi.debian@...ow.org,
 krzysztof.kozlowski+dt@...aro.org, viresh.kumar@...aro.org
Subject: Re: [RFC PATCH] arm64: dts: rockchip: Make preparations for
 per-RK3588-variant OPPs

Hello Alexey,

I'm sorry for my delayed response, had some "IRL stuff" to take care of.

On 2024-05-29 16:05, Alexey Charkov wrote:
> On Wed, May 29, 2024 at 4:22 PM Dragan Simic <dsimic@...jaro.org> 
> wrote:
>> On 2024-05-29 14:04, Alexey Charkov wrote:
>> > On Wed, May 29, 2024 at 2:45 PM Dragan Simic <dsimic@...jaro.org>
>> > wrote:
>> >> On 2024-05-29 11:57, Alexey Charkov wrote:
>> >> > On Wed, May 29, 2024 at 6:14 AM Dragan Simic <dsimic@...jaro.org>
>> >> > wrote:
>> >> >>
>> >> >> 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, [1][2][3] 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.
>> >> >
>> >> > Indeed, including just one .dtsi for the correct SoC variant and not
>> >> > having to bother about what other bits and pieces are required to use
>> >> > the full SoC functionality sounds great! Thanks for putting this
>> >> > together so promptly!
>> >>
>> >> You're welcome. :)
>> >>
>> >> > Some considerations below.
>> >> >
>> >> >> No intended functional changes are introduced.  (Still to be
>> >> >> additionally
>> >> >> verified if the patch moves forward from the RFC state.)
>> >> >>
>> >> >> [1]
>> >> >> https://lore.kernel.org/linux-rockchip/646a33e0-5c1b-471c-8183-2c0df40ea51a@cherry.de/
>> >> >> [2]
>> >> >> https://lore.kernel.org/linux-rockchip/CABjd4Yxi=+3gkNnH3BysUzzYsji-=-yROtzEc8jM_g0roKB0-w@mail.gmail.com/
>> >> >> [3]
>> >> >> https://lore.kernel.org/linux-rockchip/035a274be262528012173d463e25b55f@manjaro.org/
>> >> >>
>> >> >> Signed-off-by: Dragan Simic <dsimic@...jaro.org>
>> >> >> ---
>> >> >>  ...inctrl.dtsi => rk3588-common-pinctrl.dtsi} |    0
>> >> >
>> >> > Renaming the pinctrl includes seems superfluous - maybe keep them as
>> >> > they were to minimize churn?
>> >>
>> >> Believe it or not, the same thoughts crossed my mind.  However,
>> >> after thinking a bit about it, I concluded that renaming the pinctrl
>> >> .dtsi files as well makes the overall naming of the reworked RK3588
>> >> .dtsi files more consistent.
>> >>
>> >> It's also rather neat to have the "common" and "fullfat" file pairs
>> >> together in the arch/arm64/boot/dts/rockchip directory listing, when
>> >> it's sorted by the file name, which is the default in most (if not
>> >> all) environments.
>> >>
>> >> >>  .../{rk3588s.dtsi => rk3588-common.dtsi}      |    2 +-
>> >> >>  ...nctrl.dtsi => rk3588-fullfat-pinctrl.dtsi} |    0
>> >> >>  .../{rk3588.dtsi => rk3588-fullfat.dtsi}      |    4 +-
>> >> >
>> >> > To me, "fullfat" doesn't look super descriptive, albeit fun :)
>> >>
>> >> Yeah, I resorted to "fullfat" as some kind of a funny option. :)
>> >> That's for the "beefy" SoC variants, so it kind of fits well. :)
>> >>
>> >> > How about we rename the existing rk3588.dtsi and rk3588s.dtsi to
>> >> > something like rk3588-devices.dtsi and rk3588s-devices.dtsi
>> >> > (preserving the inheritance between them), and then I add
>> >> > rk3588s-opp.dtsi and rk3588j-opp.dtsi in a follow-up patch?
>> >>
>> >> The trouble with including "devices" into the filenames is, well,
>> >> the fact that those files isn't about any devices, but about the
>> >> SoC variants.  Thus, "devices" simply wouldn't fit there.
>> >
>> > My thinking was along the lines of "here is the file that contains all
>> > DT nodes for built-in _devices_ (as in on-chip IP blocks) on RK3588 -
>> > thus it's called rk3588-devices.dtsi", and "here's the file that
>> > contains operating parameters that distinguish RK3588 and RK3588s from
>> > RK3588j - it's called rk3588s-opp.dtsi"
>> 
>> Hmm...  I see, but a device is usually though of as some standalone
>> computer, at least in this context.  Surely, an SoC is also some kind
>> of a device, for example, but I'm pretty sure we'll very rarely call
>> an SoC a device.  It's always good to adhere to the usual meaning of
>> the words in a particular lingo.
> 
> Agreed, and calling the SoC itself a device is not what I implied by
> the suggested naming.

Thanks.  As a clarification, I just used calling an SoC a device as
another example of how "device" can be used in an uncommon way.

>> When it comes to rk3588s-opp.dtsi, it would be more of a file that
>> completes the rk3588s.dtsi by adding the OPPs, but we can in fact
>> add the OPPs directly into rk3588s.dtsi, because in the new layout
>> rk3588s.dtsi isn't a parent to any other file.
>> 
>> >> Moreover, in the envisioned scheme there should be no separate
>> >> OPP .dtsi files;  the OPP data should go directly into the new
>> >> rk3588.dtsi, rk3588s.dtsi and rk3588j.dtsi files, where it actually
>> >> belongs.  That's why those .dtsi files exist and are mostly empty,
>> >> at least the way I see it.  I'll get back to this below.
>> >>
>> >> > Then we keep "thin" versions of rk3588.dtsi, rk3588s.dtsi and
>> >> > rk3588j.dtsi for board files to include. The mix-and-match of
>> >> > different on-chip devices and different OPPs then gets transparently
>> >> > represented within those "thin" .dtsi, something like this:
>> >> >
>> >> > rk3588.dtsi:
>> >> > #include "rk3588-devices.dtsi"
>> >> > #include "rk3588s-opp.dtsi"
>> >> >
>> >> > rk3588s.dtsi:
>> >> > #include "rk3588s-devices.dtsi"
>> >> > #include "rk3588s-opp.dtsi"
>> >> >
>> >> > rk3588j.dtsi:
>> >> > #include "rk3588-devices.dtsi"
>> >> > #include "rk3588j-opp.dtsi"
>> >>
>> >> Such a layout, in general, has also crossed my mind, but my conclusion
>> >> was that having the per-SoC-variant OPP data specified directly in the
>> >> reworked rk3588.dtsi, rk3588s.dtsi and rk3588j.dtsi files is a better
>> >> option in the long term, even if we end up that way with rk3588.dtsi
>> >> and
>> >> rk3588s.dtsi repeating most (if not all) of the same OPP data.
>> >
>> > From all the info we have available so far, RK3588 and RK3588s share
>> > identical OPPs, so having one file with OPPs that both variants refer
>> > to should be more maintainable (and fewer lines of DT code).
>> 
>> Hmm.  Perhaps we can have rk3588-opp.dtsi as a separate file with the
>> OPPs shared between the RK3588 and the RK3588S, which would be 
>> included
>> from rk3588.dtsi and rk3588s.dtsi.
>> 
>> Though, the OPPs for the RK3588J should go directly into rk3588j.dtsi;
>> if we ever split the OPPs for the RK3588 and the RK3588S, the new OPPs
>> should also go directly into rk3588.dtsi and rk3588s.dtsi.
> 
> Sounds good to me!

Thanks!  I'm glad you agree.

>> > All in all, it appears that RK3588 and RK3588j have an identical set
>> > of built-in devices, but different OPPs due to RK3588j being spec'd
>> > for industrial use which apparently has different requirements. At the
>> > same time, RK3588s uses an identical set of OPPs as RK3588 but a
>> > reduced set of built-in devices. The combination of includes I
>> > proposed above makes this explicit.
>> 
>> Again, calling IP blocks devices is, AFAIK, very uncommon.
> 
> Perhaps. Shall we settle on something like "-devicenodes.dtsi" or
> "-hwblocks.dtsi"? :) This will delineate what goes to which .dtsi:
> things that describe the hardware composition go to "-hwblocks.dtsi",
> things that describe performance parameters for that hardware go to
> "-opp.dtsi" (or directly to the SoC .dtsi if there is only one SoC
> variant that uses a particular OPP table).

But why not use "-common" instead?  It's simple and aready used for
another SoC family, which I actually took the inspiration from.  That
way we'll have both simplicity (it is what it is, common part of the
RK3588 dtsi, shared among all RK3588 variants) and some arm64-dts-wide
consistency.  (See also below for further thoughts.)

> The problem I have with -common is that there are several layers of
> "common" even among just the three of these chip revisions, and a
> clear inheritance hierarchy between them (i.e. RK3588j and RK3588 also
> have a sizeable chunk of their IP blocks that is "common" between
> these two variants, in addition to those shared among all three
> variants)

Hmm, I see, that's a rather valid concern.  How about using "-base"
for what I called "-common", and "-extra" for what I called "-fullfat",
for the lack of a better term?  Using "-extra" takes inspiration from
the way Linux distribution package repositories are commonly named, so
it should be rather familiar to nearly everyone.

Also, "-base" and "-extra" are rather short, so their shortness would
also make them stand out in the directory listing as something that 
isn't
just another board .dts or .dtsi file, which can only help.

>> > As for RK3588m, it seems to include all the same devices as RK3588 and
>> > RK3588j, but a slightly modified set of OPPs compared to RK3588j. We
>> > don't have any boards using that variant in the mainline tree right
>> > now, but it would be easy, when the need arises, to add something
>> > along the lines of:
>> >
>> > rk3588m.dtsi:
>> > #include "rk3588-devices.dtsi"
>> > #include "rk3588m-opp.dtsi"
>> >
>> > rk3588m-opp.dtsi:
>> > #include "rk3588j-opp.dtsi"
>> > &cluster0_opp_table {
>> >         /delete-node/ opp-1296000000;
>> > };
>> 
>> I don't think that deleting nodes that way is a good option.  It's
>> rather error-prone and hard to track, so it would be better to have
>> the OPPs for RK3588M specified directly (and fully) in the future
>> rk3588m.dtsi file, just like in rk3588j.dtsi.
> 
> You're right, that does indeed look fishy upon further thought.

Thanks, I'm glad you agree.

>> >> That way we'll have no roadblocks if, at some point, we end up with
>> >> having
>> >> different OPPs defined for the RK3588 and the RK3588S variants.  Or
>> >> maybe
>> >> even for the RK3582, which we don't know much about yet.
>> >
>> > Guess we'll deal with that one once we stumble upon an actual RK3582
>> > board out in the wild and heading to the mainline kernel tree :)
>> 
>> Of course, that was just an example for the future use.
> 
> In fact, I've just discovered that Radxa has recently released Rock 5C
> Lite which is based on RK3582, and starts at just $29 for the 1GB
> version, making it interesting for tinkering. Especially given that
> its GPU, one of the big-core clusters and one of the VPU cores seem to
> be disabled in software (u-boot) rather than in hardware, which means
> there is some chance that a particular SoC specimen would actually
> have them in a working condition and possible to re-enable at no cost.
> Ordered myself one to investigate :)

Yes, I also saw the RK3582-based ROCK 5C Lite a couple of days ago. :)
It seems that the disabled IP blocks are detected as defective during
the manufacturing, which means that they might work correctly, or might
actually misbehave.  It seems similar to the way old three-core AMD
Phenom II CPUs could sometimes be made quad-core.

Powered by blists - more mailing lists