| Message ID | 2e1e100284b1edb470d6e7fde021a0f1779336c8.1728752527.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Update, encapsulate and expand the RK356x SoC dtsi files | expand |
Hi Dragan, 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@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? Cheers, Diederik
Hello Diederik, 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@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 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.
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@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 ... > 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
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@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. :)
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; }; opp-1104000000 { opp-hz = /bits/ 64 <1104000000>; opp-microvolt = <900000 900000 1150000>; + clock-latency-ns = <40000>; }; opp-1416000000 { opp-hz = /bits/ 64 <1416000000>; - opp-microvolt = <900000 900000 1150000>; + opp-microvolt = <1025000 1025000 1150000>; + clock-latency-ns = <40000>; }; opp-1608000000 { opp-hz = /bits/ 64 <1608000000>; - opp-microvolt = <975000 975000 1150000>; + opp-microvolt = <1100000 1100000 1150000>; + clock-latency-ns = <40000>; }; opp-1800000000 { opp-hz = /bits/ 64 <1800000000>; - opp-microvolt = <1050000 1050000 1150000>; + opp-microvolt = <1150000 1150000 1150000>; + clock-latency-ns = <40000>; }; };
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@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(-)