Received-SPF: pass (google.com: domain of linux-kernel+bounces-194025-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
Date: Wed, 29 May 2024 13:33:00 +0200
From: Dragan Simic <dsimic@manjaro.org>
To: Diederik de Haas <didi.debian@cknow.org>
Cc: Alexey Charkov <alchark@gmail.com>, linux-rockchip@lists.infradead.org,
 heiko@sntech.de, linux-arm-kernel@lists.infradead.org,
 devicetree@vger.kernel.org, robh+dt@kernel.org, krzk+dt@kernel.org,
 conor+dt@kernel.org, linux-kernel@vger.kernel.org, quentin.schulz@cherry.de,
 wens@kernel.org, daniel.lezcano@linaro.org,
 krzysztof.kozlowski+dt@linaro.org, viresh.kumar@linaro.org
Subject: Re: [RFC PATCH] arm64: dts: rockchip: Make preparations for
 per-RK3588-variant OPPs
In-Reply-To: <9996796.SDjBYy7pSV@bagend>
References: <673dcf47596e7bc8ba065034e339bb1bbf9cdcb0.1716948159.git.dsimic@manjaro.org>
 <CABjd4YxD41DEkBCZfkznLboEY9ZVOfTCLcj4S_kkcsVswbANyQ@mail.gmail.com>
 <9996796.SDjBYy7pSV@bagend>
Message-ID: <511137f077495007f467d5927f42f85d@manjaro.org>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit

Hello Diederik,

On 2024-05-29 13:09, Diederik de Haas wrote:
> Hi Dragan,
> 
> I think the idea is very good.
> I do have some remarks about its implementation though.

Thanks for your feedback!

> title: s/Make preparations/Prepare/

Or even better: "Prepare RK3588 SoC dtsi files for per-variant OPPs"

> On Wednesday, 29 May 2024 11:57:45 CEST Alexey Charkov wrote:
>> On Wed, May 29, 2024 at 6:14 AM Dragan Simic <dsimic@manjaro.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.
> 
> "Rename the RK3588 dtsi files in preparation of the ability to specify
> different
> CPU and GPU OPPs combinations for all the supported RK3588 SoC 
> variants."?
> 
> There's no partial renaming of things. That you then also change the 
> include
> files to match, is implied.
> The "modify ... a bit" implies you'll do something else (too), which 
> should be
> in its own separate patch (if that were actually the case).

Oh, the entire description of the patch was cobbled together in a rather
"relaxed" way, because it was past 2 AM over here, :) and because it's 
just
an RFC patch.  For the final version of the patch, if we agree upon 
moving
it forward from the RFC status, I'll prepare a more "formal" description
that will be much more detailed and more accurate.

> If you mention one variant but not (any) others, makes people like me 
> wonder:
> why is RK3588J treated differently then f.e. RK3588M?
> In this case I don't see a reason to single out one variant.

Good remark.  Will be described in the final patch description.

>> > 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.
>> 
>> 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!
> 
> Indeed :)

Thanks. :)

>> > No intended functional changes are introduced.
> 
> ...
> 
>> >  ...inctrl.dtsi => rk3588-common-pinctrl.dtsi} |    0
>> 
>> Renaming the pinctrl includes seems superfluous - maybe keep them as
>> they were to minimize churn?
> 
> The reason for that wasn't clear to me either. If there is a good 
> reason for
> it, then a (git commit) message specify *why* is appreciated.

Another good remark.  Will be addressed in the final patch description.

>> >  .../{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 :)
> 
> Agreed with the non-descriptive part. Please choose a different name.

I'll think about it.  I'm not crazy about "fullfat" either.

> And document in the git commit message why it was renamed and what is 
> expected
> to be in the new dtsi file, but would be incorrect for the old dtsi 
> file.
> 
> That you went from rk3588s.dtsi to rk3588-common.dtsi (I miss the 's') 
> should
> be described (assuming that was intentional and not a typo).

Omitting the "s" wasn't a typo.  It's just that rk3588s.dtsi served as
the base for other .dtsi files before, but it's now called 
rk3588-common.dtsi,
which makes its purpose a bit more self-descriptive, and separates it
from the actual SoC variants (S, J, M), which should also help a bit
with its self-descriptiveness.

Note that "common" is already used in the .dtsi filenames for some other
SoC families, which I actually took inspiration from.

> IOW: let this commit (message) describe what should and should not go 
> in the
> respective dtsi files, which can then be used as reference for future 
> rk3588
> board additions.

Of course.  Again, more material for the final patch description.

>> How about we rename the existing rk3588.dtsi and rk3588s.dtsi to
>> something like rk3588-devices.dtsi and rk3588s-devices.dtsi
> 
> Whether it's '-devices' or '-common', I think it's impossible for a 
> (short)
> name to be fully self-descriptive.
> Thus document what you mean by it in the commit message.

Agreed.  Once again, more material for the final patch description.

> Then we can use those 'rules' to consistently apply them.
> 
>> >  arch/arm64/boot/dts/rockchip/rk3588.dtsi      |  414 +--
>> >  arch/arm64/boot/dts/rockchip/rk3588j.dtsi     |    6 +-
>> >  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 2671 +----------------
>> 
>> Rename detection didn't do a particularly great job here - wonder if
>> we can do anything about it to minimize the patch size and ensure that
>> the change history is preserved for git blame...
> 
> +1
> The diff does look awfully big for a rename operation, which was 
> supposed to
> (also only) "modify ... a bit".

I also don't like the size of the patch.  I just tried playing with
specifying different values for the --find-renames and --find-copies
options, but with no good results.  I'll have a look into the Git
source later, to see what's actually going on with those options.