Re: [RFC PATCH] arm64: dts: rockchip: Make preparations for per-RK3588-variant OPPs

[Dragan Simic]

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@xxxxxxxxxxx> 
> > 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.