On Tue, May 04, 2021 at 11:55:10PM +0530, Sibi Sankar wrote:

Hey Sudeep,

Thanks for the review!

On 2021-05-04 20:12, Sudeep Holla wrote:

quoted

On Fri, Apr 30, 2021 at 07:58:21PM +0530, Sibi Sankar wrote:

quoted

Add OPP tables required to scale DDR/L3 per freq-domain on SC7280
SoCs.

Signed-off-by: Sibi Sankar <redacted>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 135
+++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi

b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 0bb835aeae33..90220cecb368 100644

--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi

[...]

quoted

@@ -248,6 +273,116 @@
 		};
 	};

+	cpu0_opp_table: cpu0_opp_table {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		cpu0_opp1: opp-300000000 {
+			opp-hz = /bits/ 64 <300000000>;
+			opp-peak-kBps = <800000 9600000>;
+		};
+
+		cpu0_opp2: opp-691200000 {
+			opp-hz = /bits/ 64 <691200000>;
+			opp-peak-kBps = <800000 17817600>;
+		};
+
+		cpu0_opp3: opp-806400000 {
+			opp-hz = /bits/ 64 <806400000>;
+			opp-peak-kBps = <800000 20889600>;
+		};
+
+		cpu0_opp4: opp-940800000 {
+			opp-hz = /bits/ 64 <940800000>;
+			opp-peak-kBps = <1804000 24576000>;
+		};
+
+		cpu0_opp5: opp-1152000000 {
+			opp-hz = /bits/ 64 <1152000000>;
+			opp-peak-kBps = <2188000 27033600>;
+		};
+
+		cpu0_opp6: opp-1324800000 {
+			opp-hz = /bits/ 64 <1324800000>;
+			opp-peak-kBps = <2188000 33792000>;
+		};
+
+		cpu0_opp7: opp-1516800000 {
+			opp-hz = /bits/ 64 <1516800000>;
+			opp-peak-kBps = <3072000 38092800>;
+		};
+
+		cpu0_opp8: opp-1651200000 {
+			opp-hz = /bits/ 64 <1651200000>;
+			opp-peak-kBps = <3072000 41779200>;
+		};
+
+		cpu0_opp9: opp-1804800000 {
+			opp-hz = /bits/ 64 <1804800000>;
+			opp-peak-kBps = <4068000 48537600>;
+		};
+
+		cpu0_opp10: opp-1958400000 {
+			opp-hz = /bits/ 64 <1958400000>;
+			opp-peak-kBps = <4068000 48537600>;
+		};
+	};
+

NACK, this breaks if there is a mismatch from what is read from the
hardware
and what is presented in this table above. Either add it from the some
bootloader or other boot code to this table reading from the
hardware/firmware
or find a way to link them without this.

Sorry I had warned long back about this when such links were discussed
as
part of interconnect binding.

Not sure why this warrants a NACK,
as this was consensus for mapping
cpu freq to DDR/L3 bandwidth votes.
(We use the same solution on SDM845
and SC7180). The opp tables are
optional and when specified puts in
votes for DDR/L3. In the future the
table can be safely dropped when more
useful devfreq governors are upstreamed.

cpufreq: qcom: Don't add frequencies without an OPP

I guess your main concern for breakage
is ^^ commit? The original design is
to list a super set of frequencies
supported by all variants of the SoC
along with the required DDR/L3 bandwidth
values. When we run into non-documented
frequency we just wouldn't put in bw
votes for it which should be fine since
the entire opp_table is optional. If
this is the reason for the NACK I can
try get it reverted with Matthias's ack.

Couldn't omitting the vote result in inconsistent performance at OPPs
w/o DT entry?

Let's assume the Soc has (at least) the following OPPs:

  cpu0_opp1: opp-300000000 {
           opp-hz = /bits/ 64 <300000000>;
           opp-peak-kBps = <800000 9600000>;
  };

  cpu0_opp3: opp-806400000 {
          opp-hz = /bits/ 64 <806400000>;
          opp-peak-kBps = <800000 20889600>;
  };

  /* missing in the device tree */
  cpu0_opp4: opp-940800000 {
          opp-hz = /bits/ 64 <940800000>;
          opp-peak-kBps = <1804000 24576000>;
  };

  cpu0_opp5: opp-1152000000 {
          opp-hz = /bits/ 64 <1152000000>;
          opp-peak-kBps = <2188000 27033600>;
  };


When the CPU frequency changes from 1.152 GHz to 940.4 MHz the
bandwidth vote is omitted because the new OPP isn't listed in the
DT, the DDR/L3 bandwidth remains at the value for 1.152 GHz, which
is fine in terms of performance, but has a penalty in terms of power.

In case the CPU frequency changes from 806.4 MHz to 940.4 MHz the
bandwidth vote is also omitted, but now the DDR/L3 bandwith stays
at the value for 806.4 MHz, which could have a negative impact on
performance. The impact would be even larger if the switch happened
from a lower OPP, like OPP 1.