Date: Sun, 09 Jun 2024 14:09:31 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Heiko Stübner <heiko@...ech.de>
Cc: linux-rockchip@...ts.infradead.org,
 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, alchark@...il.com, didi.debian@...ow.org,
 jonas@...boo.se
Subject: Re: [PATCH] arm64: dts: rockchip: Prepare RK3588 SoC dtsi files for
 per-variant OPPs

Hello Heiko,

On 2024-06-09 14:02, Heiko Stübner wrote:
> Am Sonntag, 9. Juni 2024, 10:58:19 CEST schrieb Dragan Simic:
>> Rename the Rockchip RK3588 SoC dtsi files and, consequently, adjust 
>> their
>> contents appropriately, to prepare them for the ability to specify 
>> different
>> CPU and GPU OPPs for each of the supported RK3588 SoC variants.
>> 
>> As already discussed, [1][2][3][4] some of the RK3588 SoC variants 
>> require
>> different OPPs, and it makes more sense to have the OPPs already 
>> defined when
>> a board dts(i) file includes one of the SoC variant dtsi files 
>> (rk3588.dtsi,
>> rk3588j.dtsi or rk3588s.dtsi), rather than requiring the board dts(i) 
>> 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 into the 
>> board
>> dts(i) file, and it doesn't make much sense to, effectively, allow the 
>> board
>> dts(i) file to include and use an incompatible set of OPPs for the 
>> already
>> selected RK3588 SoC variant.
>> 
>> The new naming scheme for the RK3588 SoC dtsi files uses "-base" and 
>> "-extra"
>> suffixes to denote the DT data shared between all RK5588 SoC variants, 
>> and
>> the DT data shared between the unrestricted SoC variants, 
>> respectively.
>> For example, the DT data for the RK3588 includes both rk3588-base.dtsi 
>> and
>> rk3588-extra.dtsi, because it's an unrestricted SoC variant, while the 
>> DT
>> data for the RK3588S variant includes rk3588-base.dtsi only, because 
>> it's
>> a restricted SoC variant, feature- and interface-wise.  This achieves 
>> a more
>> logical naming of the RK3588 SoC dtsi files, which reflects the way DT 
>> data
>> for the SoC variants is built by "stacking" the SoC variant features 
>> made
>> available through the "-base" and "-extra" SoC dtsi files.  
>> Additionally,
>> the SoC variant dtsi files (rk3588.dtsi, rk3588j.dtsi and 
>> rk3588s.dtsi) are
>> no longer parents to any other SoC variant dtsi files, which should 
>> help with
>> making the new "stacking" approach cleaner and easier to follow.
>> 
>> The RK3588 pinctrl dtsi files are also renamed in the same way, for 
>> the sake
>> of consistency.  This also keeps the "-base" and "-extra" groups of 
>> the dtsi
>> files together when looked at in a directory listing, which is 
>> helpful.
>> 
>> The per-SoC-variant OPPs should go directly into the SoC dtsi files, 
>> if no
>> more than one SoC variant uses those OPPs, or be put into a separate 
>> "-opp"
>> dtsi file that's shared between and included from two or more SoC 
>> variant
>> dtsi files.  An example for the former is the non-shared OPP data that 
>> should
>> go directly into the RK3588J SoC variant dtsi file (i.e. 
>> rk3588j.dtsi), and
>> an example for the latter is the shared OPP data that should be put 
>> into
>> rk3588-opp.dtsi and be included from the RK3588 and RK3588S SoC 
>> variant dtsi
>> files (i.e. rk3588.dtsi and rk3588s.dtsi, respectively).  
>> Consequently, if
>> the OPPs for the RK3588 and RK3588S SoC variants are ever made 
>> different,
>> the shared rk3588-opp.dtsi file should be deleted and the new OPPs 
>> should
>> be put directly into rk3588.dtsi and rk3588s.dtsi. [4]
>> 
>> No functional changes are introduced, which was validated by 
>> decompiling and
>> comparing all affected dtb files before and after these changes.
>> 
>> As a side note, due to the nature of introduced changes, this commit 
>> is best
>> viewed using the --break-rewrites option for git-log(1).
>> 
>> [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/
>> [4] 
>> https://lore.kernel.org/linux-rockchip/673dcf47596e7bc8ba065034e339bb1bbf9cdcb0.1716948159.git.dsimic@manjaro.org/T/#u
>> 
>> Signed-off-by: Dragan Simic <dsimic@...jaro.org>
> 
> Well that diff definitly is beautiful. Thanks for finding an option to 
> make
> it easily readable :-) .

Thanks, I'm glad that you like it. :)

> On first glance looks great, but I'll let this simmer a bit to give 
> others
> the time to voice opinions.
> 
>> ---
>> 
>> Notes:
>>     Changes since RFC:
>>       - Improved the accuracy, formality and the level of detail in 
>> the patch
>>         description, while also addressing all remarks from the RFC
>>       - Moved on to using "-base" and "-extra" suffixes instead of 
>> "-common"
>>         and "-fullfat" suffixes, respectively, as parts of the RK3588 
>> SoC
>>         variant dtsi filenames, for a bit better self-descriptiveness 
>> and
>>         to follow a more formal naming approach
>>       - Drastically reduced the size of the diff, using 
>> --break-rewrites
>>         as an option for git-diff(1) and git-format-patch(1), [5] 
>> while also
>>         adding a hopefully useful related note to the patch 
>> description
>> 
>>     Link to RFC: 
>> https://lore.kernel.org/linux-rockchip/673dcf47596e7bc8ba065034e339bb1bbf9cdcb0.1716948159.git.dsimic@manjaro.org/T/#u
>> 
>>     [5] 
>> https://git-scm.com/docs/git-diff#Documentation/git-diff.txt--Bltngtltmgt
>> 
>>  .../{rk3588s-pinctrl.dtsi => rk3588-base-pinctrl.dtsi}        | 0
>>  .../boot/dts/rockchip/{rk3588s.dtsi => rk3588-base.dtsi}      | 2 +-
>>  .../{rk3588-pinctrl.dtsi => rk3588-extra-pinctrl.dtsi}        | 0
>>  .../boot/dts/rockchip/{rk3588.dtsi => rk3588-extra.dtsi}      | 4 
>> ++--
>>  arch/arm64/boot/dts/rockchip/{rk3588j.dtsi => rk3588.dtsi}    | 2 +-
>>  arch/arm64/boot/dts/rockchip/rk3588j.dtsi                     | 2 +-
>>  arch/arm64/boot/dts/rockchip/{rk3588j.dtsi => rk3588s.dtsi}   | 2 +-
>>  7 files changed, 6 insertions(+), 6 deletions(-)
>>  rename arch/arm64/boot/dts/rockchip/{rk3588s-pinctrl.dtsi => 
>> rk3588-base-pinctrl.dtsi} (100%)
>>  rename arch/arm64/boot/dts/rockchip/{rk3588s.dtsi => 
>> rk3588-base.dtsi} (99%)
>>  rename arch/arm64/boot/dts/rockchip/{rk3588-pinctrl.dtsi => 
>> rk3588-extra-pinctrl.dtsi} (100%)
>>  rename arch/arm64/boot/dts/rockchip/{rk3588.dtsi => 
>> rk3588-extra.dtsi} (99%)
>>  copy arch/arm64/boot/dts/rockchip/{rk3588j.dtsi => rk3588.dtsi} (79%)
>>  copy arch/arm64/boot/dts/rockchip/{rk3588j.dtsi => rk3588s.dtsi} 
>> (79%)
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-pinctrl.dtsi 
>> b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi
>> similarity index 100%
>> rename from arch/arm64/boot/dts/rockchip/rk3588s-pinctrl.dtsi
>> rename to arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi 
>> b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
>> similarity index 99%
>> rename from arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> rename to arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
>> index 6ac5ac8b48ab..629049f3dc16 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
>> @@ -2667,4 +2667,4 @@ gpio4: gpio@...50000 {
>>  	};
>>  };
>> 
>> -#include "rk3588s-pinctrl.dtsi"
>> +#include "rk3588-base-pinctrl.dtsi"
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-pinctrl.dtsi 
>> b/arch/arm64/boot/dts/rockchip/rk3588-extra-pinctrl.dtsi
>> similarity index 100%
>> rename from arch/arm64/boot/dts/rockchip/rk3588-pinctrl.dtsi
>> rename to arch/arm64/boot/dts/rockchip/rk3588-extra-pinctrl.dtsi
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588.dtsi 
>> b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
>> similarity index 99%
>> rename from arch/arm64/boot/dts/rockchip/rk3588.dtsi
>> rename to arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
>> index 5984016b5f96..37101768999b 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
>> @@ -3,8 +3,8 @@
>>   * Copyright (c) 2021 Rockchip Electronics Co., Ltd.
>>   */
>> 
>> -#include "rk3588s.dtsi"
>> -#include "rk3588-pinctrl.dtsi"
>> +#include "rk3588-base.dtsi"
>> +#include "rk3588-extra-pinctrl.dtsi"
>> 
>>  / {
>>  	usb_host1_xhci: usb@...00000 {
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi 
>> b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
>> similarity index 79%
>> copy from arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> copy to arch/arm64/boot/dts/rockchip/rk3588.dtsi
>> index 38b9dbf38a21..0bbeee399a63 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
>> @@ -4,4 +4,4 @@
>>   *
>>   */
>> 
>> -#include "rk3588.dtsi"
>> +#include "rk3588-extra.dtsi"
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi 
>> b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> index 38b9dbf38a21..0bbeee399a63 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> @@ -4,4 +4,4 @@
>>   *
>>   */
>> 
>> -#include "rk3588.dtsi"
>> +#include "rk3588-extra.dtsi"
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi 
>> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> similarity index 79%
>> copy from arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> copy to arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> index 38b9dbf38a21..a379269147c4 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> @@ -4,4 +4,4 @@
>>   *
>>   */
>> 
>> -#include "rk3588.dtsi"
>> +#include "rk3588-base.dtsi"
>> 
> 
> 
> 
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Powered by blists - more mailing lists