Hey Sudeep,

Thanks for the review!

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

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.


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.