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.
