Re: [PATCH v2 1/3] arm64: dts: rockchip: Add RK3399-T opp

[Samuel Dionne-Riel]

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