[PATCH 1/3] arm64: dts: rockchip: Update CPU OPP voltages in RK356x SoC dtsi

[Dragan Simic]

Hello Diederik,

On 2024-10-12 22:02, Diederik de Haas wrote:
> On Sat Oct 12, 2024 at 9:45 PM CEST, Dragan Simic wrote:
>> On 2024-10-12 21:27, Diederik de Haas wrote:
>> > On Sat Oct 12, 2024 at 7:04 PM CEST, Dragan Simic wrote:
>> >> Update the lower/upper voltage limits and the exact voltages for the
>> >> Rockchip
>> >> RK356x CPU OPPs, using the most conservative values (i.e. the highest
>> >> per-OPP
>> >> voltages) found in the vendor kernel source. [1]
>> >>
>> >> Using the most conservative per-OPP voltages ensures reliable CPU
>> >> operation
>> >> regardless of the actual CPU binning, with the downside of possibly
>> >> using
>> >> a bit more power for the CPU cores than absolutely needed.
>> >>
>> >> Additionally, fill in the missing "clock-latency-ns" CPU OPP
>> >> properties, using
>> >> the values found in the vendor kernel source. [1]
>> >>
>> >> [1]
>> >> https://raw.githubusercontent.com/rockchip-linux/kernel/f8b9431ee38ed561650be7092ab93f564598daa9/arch/arm64/boot/dts/rockchip/rk3568.dtsi
>> >>
>> >> Related-to: eb665b1c06bc ("arm64: dts: rockchip: Update GPU OPP
>> >> voltages in RK356x SoC dtsi")
>> >> Signed-off-by: Dragan Simic <dsimic at manjaro.org>
>> >> ---
>> >>  arch/arm64/boot/dts/rockchip/rk3568.dtsi |  1 +
>> >>  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 18 ++++++++++++------
>> >>  2 files changed, 13 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
>> >> b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
>> >> index 0946310e8c12..5c54898f6ed1 100644
>> >> --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
>> >> +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
>> >> @@ -273,6 +273,7 @@ &cpu0_opp_table {
>> >>  	opp-1992000000 {
>> >>  		opp-hz = /bits/ 64 <1992000000>;
>> >>  		opp-microvolt = <1150000 1150000 1150000>;
>> >> +		clock-latency-ns = <40000>;
>> >>  	};
>> >>  };
>> >>
>> >> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
>> >> b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
>> >> index 0ee0ada6f0ab..534593f2ed0b 100644
>> >> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
>> >> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
>> >> @@ -134,39 +134,45 @@ cpu0_opp_table: opp-table-0 {
>> >>
>> >>  		opp-408000000 {
>> >>  			opp-hz = /bits/ 64 <408000000>;
>> >> -			opp-microvolt = <900000 900000 1150000>;
>> >> +			opp-microvolt = <850000 850000 1150000>;
>> >>  			clock-latency-ns = <40000>;
>> >>  		};
>> >>
>> >>  		opp-600000000 {
>> >>  			opp-hz = /bits/ 64 <600000000>;
>> >> -			opp-microvolt = <900000 900000 1150000>;
>> >> +			opp-microvolt = <850000 850000 1150000>;
>> >> +			clock-latency-ns = <40000>;
>> >>  		};
>> >>
>> >>  		opp-816000000 {
>> >>  			opp-hz = /bits/ 64 <816000000>;
>> >> -			opp-microvolt = <900000 900000 1150000>;
>> >> +			opp-microvolt = <850000 850000 1150000>;
>> >> +			clock-latency-ns = <40000>;
>> >>  			opp-suspend;
>> >>  		};
>> >
>> > While it felt a bit much to send a patch just to remove the blank lines
>> > between the opp nodes, this sounds like an excellent opportunity to
>> > make it consistent with the opp list in other DT files?
>> 
>> Actually, my plan is to work on the SoC binning, which will involve
>> touching nearly every OPP in the Rockchip DTs, and will add much more
>> data to each OPP node.  Thus, having empty lines as the separators
>> between the OPP nodes is something we should actually want, because
> 
> As indicated in the "arm64: dts: rockchip: Add dtsi file for RK3399S 
> SoC
> variant" patch series, I do prefer the separator lines ...

Oh, I also prefer them.  I just opted for introducing fewer changes
in the RK3399S patch series, to keep the size of the series smaller,
and to end up with easier diffing of the SoC variant dtsi files.

>> not having them will actually reduce the readability after the size
>> of the individual OPP nodes is increased.
>> 
>> That's the reason why I opted for having the separator lines in this
>> patch series, i.e. because having them everywhere should be the final
>> outcome, and because in this case they were already present where the
>> OPPs were moved or copied from.
> 
> ... but you actually removed those lines in the other patch set.

Well, I didn't remove them in place, but en route to another, much
larger file, :) which originally didn't have them.  As described
above, that was my choice to keep the things a bit more consistent
(in the "RK3399 way"), and to reduce the amount of "code churn"
a bit.  I'll try to explain it further below.

> While I'm looking forward to the extra data to the OPP nodes, I don't
> think the amount of properties should determine whether it should have 
> a
> separator line or not.

True, the size of the node shouldn't be the determining factor.
The reason why I emphasized the node size was because other people
may be a bit "allergic" to whitespace changes and such "cosmetic"
code layout changes, so justifying such changes can only help.

See, just as you're "triggered" by whitespace inconsistency, some
other people may be "triggered" by cosmetic code changes.   It's
quite tough to strike some kind of balance there. :)