| Message ID | 20220805234411.303055-2-tom@tom-fitzhenry.me.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for the Pine64 PinePhone Pro phone | expand |
Hi, On Sat, Aug 06, 2022 at 09:44:09AM +1000, Tom Fitzhenry wrote: > From: Samuel Dionne-Riel <samuel@dionne-riel.com> > > 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) 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. [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@dionne-riel.com> > --- > .../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@dionne-riel.com> > + */ > + > +/ { > + 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 >
Hi, On 8/8/22, Ondřej Jirman <megi@xff.cz> wrote: > Hi, > > On Sat, Aug 06, 2022 at 09:44:09AM +1000, Tom Fitzhenry wrote: >> From: Samuel Dionne-Riel <samuel@dionne-riel.com> >> >> 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@dionne-riel.com> >> --- >> .../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@dionne-riel.com> >> + */ >> + >> +/ { >> + 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 >> >
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@dionne-riel.com> + */ + +/ { + 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>; +};