Hi, On 8/8/22, Ondřej Jirman <megi@xxxxxx> wrote: > Hi, > > On Sat, Aug 06, 2022 at 09:44:09AM +1000, Tom Fitzhenry wrote: >> From: Samuel Dionne-Riel <samuel@xxxxxxxxxxxxxxx> >> >> These tables were derived from the regular RK3399 table, by dropping >> entries exceeding recommended operating conditions from the datasheet, >> and clamping the last exceeding value where it made sense. > > Do we really need to duplicate the whole OPP table of rk3399-opp.dtsi > just to disable a few top opp## entries? > > This will make it more annoying to add PVTM/eFuse leakage based voltage > selection support later on, because then there would have to be identical > multi-level operating points across multiple files. And that sounds like > a LOT of dupplication for little benefit. > > Also Pinephone Pro has RK3399S not -T. RK3399 seems to be RK3399 selected > for > low leakage (values I've seen from eFuses indicate the leakage is half that > of > RK3399 available in Pinebook Pro) The vendor (PINE64) asked me to make these changes while stating specifically that the Pinephone Pro uses the RK3399-T. Though earlier units and current batches use the RK3399[s], the design was reportedly made with the RK3399-T in mind. The device was also designed to use the OPP from the RK3399-T on RK3399[s] for the designed thermal operation of the device, reportedly. > I'd suggest just adding references to select operating point nodes that > are "too much" and disabling them with status = "disabled". This > can be done from the pinephone device tree file directly. > > Otherwise we'll eventually end up with several files containing > something like this [1] and only differing in absence of some opp## nodes > and not their actual useful content. As to why this is a new file? I have assumed it would be preferable since this is how it was done for the "OP1" variant of the RK3399. I will defer to the Rockchip and ARM maintainers to determine which way is better. I will note that in practice I agree here. Cheers, > [1] > https://github.com/rockchip-linux/kernel/blob/develop-4.19/arch/arm64/boot/dts/rockchip/rk3399-opp.dtsi#L6 > > kind regards, > o. > >> Signed-off-by: Samuel Dionne-Riel <samuel@xxxxxxxxxxxxxxx> >> --- >> .../arm64/boot/dts/rockchip/rk3399-t-opp.dtsi | 118 ++++++++++++++++++ >> 1 file changed, 118 insertions(+) >> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi >> b/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi >> new file mode 100644 >> index 0000000000000..ec153015d9d13 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi >> @@ -0,0 +1,118 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Copyright (c) 2016-2017 Fuzhou Rockchip Electronics Co., Ltd >> + * Copyright (c) 2022 Samuel Dionne-Riel <samuel@xxxxxxxxxxxxxxx> >> + */ >> + >> +/ { >> + cluster0_opp: opp-table-0 { >> + compatible = "operating-points-v2"; >> + opp-shared; >> + >> + opp00 { >> + opp-hz = /bits/ 64 <408000000>; >> + opp-microvolt = <825000 825000 925000>; >> + clock-latency-ns = <40000>; >> + }; >> + opp01 { >> + opp-hz = /bits/ 64 <600000000>; >> + opp-microvolt = <825000 825000 925000>; >> + }; >> + opp02 { >> + opp-hz = /bits/ 64 <816000000>; >> + opp-microvolt = <850000 850000 925000>; >> + }; >> + opp03 { >> + opp-hz = /bits/ 64 <1008000000>; >> + opp-microvolt = <925000 925000 925000>; >> + }; >> + }; >> + >> + cluster1_opp: opp-table-1 { >> + compatible = "operating-points-v2"; >> + opp-shared; >> + >> + opp00 { >> + opp-hz = /bits/ 64 <408000000>; >> + opp-microvolt = <825000 825000 1150000>; >> + clock-latency-ns = <40000>; >> + }; >> + opp01 { >> + opp-hz = /bits/ 64 <600000000>; >> + opp-microvolt = <825000 825000 1150000>; >> + }; >> + opp02 { >> + opp-hz = /bits/ 64 <816000000>; >> + opp-microvolt = <825000 825000 1150000>; >> + }; >> + opp03 { >> + opp-hz = /bits/ 64 <1008000000>; >> + opp-microvolt = <875000 875000 1150000>; >> + }; >> + opp04 { >> + opp-hz = /bits/ 64 <1200000000>; >> + opp-microvolt = <950000 950000 1150000>; >> + }; >> + opp05 { >> + opp-hz = /bits/ 64 <1416000000>; >> + opp-microvolt = <1025000 1025000 1150000>; >> + }; >> + opp06 { >> + opp-hz = /bits/ 64 <1500000000>; >> + opp-microvolt = <1100000 1100000 1150000>; >> + }; >> + }; >> + >> + gpu_opp_table: opp-table-2 { >> + compatible = "operating-points-v2"; >> + >> + opp00 { >> + opp-hz = /bits/ 64 <200000000>; >> + opp-microvolt = <825000 825000 975000>; >> + }; >> + opp01 { >> + opp-hz = /bits/ 64 <297000000>; >> + opp-microvolt = <825000 825000 975000>; >> + }; >> + opp02 { >> + opp-hz = /bits/ 64 <400000000>; >> + opp-microvolt = <825000 825000 975000>; >> + }; >> + opp03 { >> + opp-hz = /bits/ 64 <500000000>; >> + opp-microvolt = <875000 875000 975000>; >> + }; >> + opp04 { >> + opp-hz = /bits/ 64 <600000000>; >> + opp-microvolt = <925000 925000 975000>; >> + }; >> + }; >> +}; >> + >> +&cpu_l0 { >> + operating-points-v2 = <&cluster0_opp>; >> +}; >> + >> +&cpu_l1 { >> + operating-points-v2 = <&cluster0_opp>; >> +}; >> + >> +&cpu_l2 { >> + operating-points-v2 = <&cluster0_opp>; >> +}; >> + >> +&cpu_l3 { >> + operating-points-v2 = <&cluster0_opp>; >> +}; >> + >> +&cpu_b0 { >> + operating-points-v2 = <&cluster1_opp>; >> +}; >> + >> +&cpu_b1 { >> + operating-points-v2 = <&cluster1_opp>; >> +}; >> + >> +&gpu { >> + operating-points-v2 = <&gpu_opp_table>; >> +}; >> -- >> 2.37.1 >> > -- — Samuel Dionne-Riel
