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

[Diederik de Haas]

Hi Dragan,

I think the idea is very good.
I do have some remarks about its implementation though.

title: s/Make preparations/Prepare/

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

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.

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

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

> >  .../{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.

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

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.

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

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".

Cheers,
  Diederik

Attachment: signature.asc
Description: This is a digitally signed message part.