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

[Diederik de Haas]

Hi Dragan,

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

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

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.

My 0.02

Cheers,
  Diederik

Attachment: signature.asc
Description: PGP signature