[PATCH 0/3] Add support to scale DDR and L3 on SA8775P

[PATCH 0/3] Add support to scale DDR and L3 on SA8775P

[Jagadeesh Kona]

Add support to scale DDR and L3 based on CPU frequencies
on Qualcomm SA8775P platform. Also add support for LMH
interrupts to indicate if there is any thermal throttle.

The changes in this series are dependent on below series changes:
https://lore.kernel.org/linux-arm-msm/20240904171209.29120-1-quic_rlaggysh@quicinc.com/#t

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
Jagadeesh Kona (2):
      arm64: dts: qcom: sa8775p: Add support to scale DDR/L3
      arm64: dts: qcom: sa8775p: Add LMH interrupts support

Shivnandan Kumar (1):
      arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 215 ++++++++++++++++++++++++++++++++++
 1 file changed, 215 insertions(+)
---
base-commit: d1ef2d48e83b32417eb55480c097737364535405
change-id: 20241017-sa8775p-cpufreq-l3-ddr-scaling-a1a9abce98c6

Best regards,
-- 
Jagadeesh Kona <quic_jkona@quicinc.com>

[PATCH 1/3] arm64: dts: qcom: sa8775p: Add support to scale DDR/L3

[Jagadeesh Kona]

Add support to scale DDR and L3 based on CPU frequencies
on SA8775P platform.

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 06bf2ba556b89b643da901857a9aa7cdc7ba90cc..d8b90bd4b1f05604185f015929a1f296799ad6a4 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -4,6 +4,7 @@
  */
 
 #include <dt-bindings/interconnect/qcom,icc.h>
+#include <dt-bindings/interconnect/qcom,osm-l3.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/clock/qcom,rpmh.h>
 #include <dt-bindings/clock/qcom,sa8775p-gcc.h>
@@ -47,6 +48,10 @@ CPU0: cpu@0 {
 			next-level-cache = <&L2_0>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+					<&epss_l3_cl0 MASTER_EPSS_L3_APPS
+					&epss_l3_cl0 SLAVE_EPSS_L3_SHARED>;
 			L2_0: l2-cache {
 				compatible = "cache";
 				cache-level = <2>;
@@ -69,6 +74,10 @@ CPU1: cpu@100 {
 			next-level-cache = <&L2_1>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+					<&epss_l3_cl0 MASTER_EPSS_L3_APPS
+					&epss_l3_cl0 SLAVE_EPSS_L3_SHARED>;
 			L2_1: l2-cache {
 				compatible = "cache";
 				cache-level = <2>;
@@ -86,6 +95,10 @@ CPU2: cpu@200 {
 			next-level-cache = <&L2_2>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+					<&epss_l3_cl0 MASTER_EPSS_L3_APPS
+					&epss_l3_cl0 SLAVE_EPSS_L3_SHARED>;
 			L2_2: l2-cache {
 				compatible = "cache";
 				cache-level = <2>;
@@ -103,6 +116,10 @@ CPU3: cpu@300 {
 			next-level-cache = <&L2_3>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+					<&epss_l3_cl0 MASTER_EPSS_L3_APPS
+					&epss_l3_cl0 SLAVE_EPSS_L3_SHARED>;
 			L2_3: l2-cache {
 				compatible = "cache";
 				cache-level = <2>;
@@ -120,6 +137,10 @@ CPU4: cpu@10000 {
 			next-level-cache = <&L2_4>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+					<&epss_l3_cl1 MASTER_EPSS_L3_APPS
+					&epss_l3_cl1 SLAVE_EPSS_L3_SHARED>;
 			L2_4: l2-cache {
 				compatible = "cache";
 				cache-level = <2>;
@@ -143,6 +164,10 @@ CPU5: cpu@10100 {
 			next-level-cache = <&L2_5>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+					<&epss_l3_cl1 MASTER_EPSS_L3_APPS
+					&epss_l3_cl1 SLAVE_EPSS_L3_SHARED>;
 			L2_5: l2-cache {
 				compatible = "cache";
 				cache-level = <2>;
@@ -160,6 +185,10 @@ CPU6: cpu@10200 {
 			next-level-cache = <&L2_6>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+					<&epss_l3_cl1 MASTER_EPSS_L3_APPS
+					&epss_l3_cl1 SLAVE_EPSS_L3_SHARED>;
 			L2_6: l2-cache {
 				compatible = "cache";
 				cache-level = <2>;
@@ -177,6 +206,10 @@ CPU7: cpu@10300 {
 			next-level-cache = <&L2_7>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+					<&epss_l3_cl1 MASTER_EPSS_L3_APPS
+					&epss_l3_cl1 SLAVE_EPSS_L3_SHARED>;
 			L2_7: l2-cache {
 				compatible = "cache";
 				cache-level = <2>;

-- 
2.34.1

[PATCH 2/3] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

[Jagadeesh Kona]

From: Shivnandan Kumar <quic_kshivnan@quicinc.com>

Add OPP tables required to scale DDR and L3 per freq-domain
on SA8775P platform.

Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 178 ++++++++++++++++++++++++++++++++++
 1 file changed, 178 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index d8b90bd4b1f05604185f015929a1f296799ad6a4..47eca50b30ffa38a652706014d35ef9e833003ec 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -48,6 +48,7 @@ CPU0: cpu@0 {
 			next-level-cache = <&L2_0>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
 					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
 					<&epss_l3_cl0 MASTER_EPSS_L3_APPS
@@ -74,6 +75,7 @@ CPU1: cpu@100 {
 			next-level-cache = <&L2_1>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
 					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
 					<&epss_l3_cl0 MASTER_EPSS_L3_APPS
@@ -95,6 +97,7 @@ CPU2: cpu@200 {
 			next-level-cache = <&L2_2>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
 					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
 					<&epss_l3_cl0 MASTER_EPSS_L3_APPS
@@ -116,6 +119,7 @@ CPU3: cpu@300 {
 			next-level-cache = <&L2_3>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
 					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
 					<&epss_l3_cl0 MASTER_EPSS_L3_APPS
@@ -137,6 +141,7 @@ CPU4: cpu@10000 {
 			next-level-cache = <&L2_4>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			operating-points-v2 = <&cpu4_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
 					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
 					<&epss_l3_cl1 MASTER_EPSS_L3_APPS
@@ -164,6 +169,7 @@ CPU5: cpu@10100 {
 			next-level-cache = <&L2_5>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			operating-points-v2 = <&cpu4_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
 					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
 					<&epss_l3_cl1 MASTER_EPSS_L3_APPS
@@ -185,6 +191,7 @@ CPU6: cpu@10200 {
 			next-level-cache = <&L2_6>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			operating-points-v2 = <&cpu4_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
 					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
 					<&epss_l3_cl1 MASTER_EPSS_L3_APPS
@@ -206,6 +213,7 @@ CPU7: cpu@10300 {
 			next-level-cache = <&L2_7>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
+			operating-points-v2 = <&cpu4_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
 					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
 					<&epss_l3_cl1 MASTER_EPSS_L3_APPS
@@ -299,6 +307,176 @@ CLUSTER_SLEEP_APSS_RSC_PC: cluster-sleep-1 {
 		};
 	};
 
+	cpu0_opp_table: opp-table-cpu0 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		cpu0_opp_1267mhz: opp-1267200000 {
+			opp-hz = /bits/ 64 <1267200000>;
+			opp-peak-kBps = <6220800 29491200>;
+		};
+
+		cpu0_opp_1363mhz: opp-1363200000 {
+			opp-hz = /bits/ 64 <1363200000>;
+			opp-peak-kBps = <6220800 29491200>;
+		};
+
+		cpu0_opp_1459mhz: opp-1459200000 {
+			opp-hz = /bits/ 64 <1459200000>;
+			opp-peak-kBps = <6220800 29491200>;
+		};
+
+		cpu0_opp_1536mhz: opp-1536000000 {
+			opp-hz = /bits/ 64 <1536000000>;
+			opp-peak-kBps = <6220800 29491200>;
+		};
+
+		cpu0_opp_1632mhz: opp-1632000000 {
+			opp-hz = /bits/ 64 <1632000000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu0_opp_1708mhz: opp-1708800000 {
+			opp-hz = /bits/ 64 <1708800000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu0_opp_1785mhz: opp-1785600000 {
+			opp-hz = /bits/ 64 <1785600000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu0_opp_1862mhz: opp-1862400000 {
+			opp-hz = /bits/ 64 <1862400000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu0_opp_1939mhz: opp-1939200000 {
+			opp-hz = /bits/ 64 <1939200000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu0_opp_2016mhz: opp-2016000000 {
+			opp-hz = /bits/ 64 <2016000000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu0_opp_2112mhz: opp-2112000000 {
+			opp-hz = /bits/ 64 <2112000000>;
+			opp-peak-kBps = <8371200 49766400>;
+		};
+
+		cpu0_opp_2188mhz: opp-2188800000 {
+			opp-hz = /bits/ 64 <2188800000>;
+			opp-peak-kBps = <8371200 49766400>;
+		};
+
+		cpu0_opp_2265mhz: opp-2265600000 {
+			opp-hz = /bits/ 64 <2265600000>;
+			opp-peak-kBps = <8371200 49766400>;
+		};
+
+		cpu0_opp_2361mhz: opp-2361600000 {
+			opp-hz = /bits/ 64 <2361600000>;
+			opp-peak-kBps = <12787200 51609600>;
+		};
+
+		cpu0_opp_2457mhz: opp-2457600000 {
+			opp-hz = /bits/ 64 <2457600000>;
+			opp-peak-kBps = <12787200 51609600>;
+		};
+
+		cpu0_opp_2553mhz: opp-2553600000 {
+			opp-hz = /bits/ 64 <2553600000>;
+			opp-peak-kBps = <12787200 54681600>;
+		};
+	};
+
+	cpu4_opp_table: opp-table-cpu4 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		cpu4_opp_1267mhz: opp-1267200000 {
+			opp-hz = /bits/ 64 <1267200000>;
+			opp-peak-kBps = <6220800 29491200>;
+		};
+
+		cpu4_opp_1363mhz: opp-1363200000 {
+			opp-hz = /bits/ 64 <1363200000>;
+			opp-peak-kBps = <6220800 29491200>;
+		};
+
+		cpu4_opp_1459mhz: opp-1459200000 {
+			opp-hz = /bits/ 64 <1459200000>;
+			opp-peak-kBps = <6220800 29491200>;
+		};
+
+		cpu4_opp_1536mhz: opp-1536000000 {
+			opp-hz = /bits/ 64 <1536000000>;
+			opp-peak-kBps = <6220800 29491200>;
+		};
+
+		cpu4_opp_1632mhz: opp-1632000000 {
+			opp-hz = /bits/ 64 <1632000000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu4_opp_1708mhz: opp-1708800000 {
+			opp-hz = /bits/ 64 <1708800000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu4_opp_1785mhz: opp-1785600000 {
+			opp-hz = /bits/ 64 <1785600000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu4_opp_1862mhz: opp-1862400000 {
+			opp-hz = /bits/ 64 <1862400000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu4_opp_1939mhz: opp-1939200000 {
+			opp-hz = /bits/ 64 <1939200000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu4_opp_2016mhz: opp-2016000000 {
+			opp-hz = /bits/ 64 <2016000000>;
+			opp-peak-kBps = <6835200 39321600>;
+		};
+
+		cpu4_opp_2112mhz: opp-2112000000 {
+			opp-hz = /bits/ 64 <2112000000>;
+			opp-peak-kBps = <8371200 49766400>;
+		};
+
+		cpu4_opp_2188mhz: opp-2188800000 {
+			opp-hz = /bits/ 64 <2188800000>;
+			opp-peak-kBps = <8371200 49766400>;
+		};
+
+		cpu4_opp_2265mhz: opp-2265600000 {
+			opp-hz = /bits/ 64 <2265600000>;
+			opp-peak-kBps = <8371200 49766400>;
+		};
+
+		cpu4_opp_2361mhz: opp-2361600000 {
+			opp-hz = /bits/ 64 <2361600000>;
+			opp-peak-kBps = <12787200 51609600>;
+		};
+
+		cpu4_opp_2457mhz: opp-2457600000 {
+			opp-hz = /bits/ 64 <2457600000>;
+			opp-peak-kBps = <12787200 51609600>;
+		};
+
+		cpu4_opp_2553mhz: opp-2553600000 {
+			opp-hz = /bits/ 64 <2553600000>;
+			opp-peak-kBps = <12787200 54681600>;
+		};
+	};
+
 	dummy-sink {
 		compatible = "arm,coresight-dummy-sink";
 

-- 
2.34.1

[PATCH 3/3] arm64: dts: qcom: sa8775p: Add LMH interrupts support

[Jagadeesh Kona]

Add LMH interrupts support to indicate if there is any
thermal throttle.

Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 47eca50b30ffa38a652706014d35ef9e833003ec..bd86bc2cb6c304aa0b4000f3226639bef57a9b9a 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -4005,6 +4005,10 @@ cpufreq_hw: cpufreq@18591000 {
 			      <0x0 0x18593000 0x0 0x1000>;
 			reg-names = "freq-domain0", "freq-domain1";
 
+			interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "dcvsh-irq-0", "dcvsh-irq-1";
+
 			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_GPLL0>;
 			clock-names = "xo", "alternate";
 

-- 
2.34.1

Re: [PATCH 2/3] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

[Brian Masney]

On Thu, Oct 17, 2024 at 02:58:31PM +0530, Jagadeesh Kona wrote:
> +	cpu0_opp_table: opp-table-cpu0 {
> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		cpu0_opp_1267mhz: opp-1267200000 {
> +			opp-hz = /bits/ 64 <1267200000>;
> +			opp-peak-kBps = <6220800 29491200>;
> +		};
> +
> +		cpu0_opp_1363mhz: opp-1363200000 {
> +			opp-hz = /bits/ 64 <1363200000>;
> +			opp-peak-kBps = <6220800 29491200>;
> +		};

[snip]

> +	cpu4_opp_table: opp-table-cpu4 {
> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		cpu4_opp_1267mhz: opp-1267200000 {
> +			opp-hz = /bits/ 64 <1267200000>;
> +			opp-peak-kBps = <6220800 29491200>;
> +		};
> +
> +		cpu4_opp_1363mhz: opp-1363200000 {
> +			opp-hz = /bits/ 64 <1363200000>;
> +			opp-peak-kBps = <6220800 29491200>;
> +		};

There's no functional differences in the cpu0 and cpu4 opp tables. Can
a single table be used?

This aligns with my recollection that this particular SoC only has the
gold cores.

Brian

Re: [PATCH 1/3] arm64: dts: qcom: sa8775p: Add support to scale DDR/L3

[Konrad Dybcio]

On 17.10.2024 11:28 AM, Jagadeesh Kona wrote:
> Add support to scale DDR and L3 based on CPU frequencies
> on SA8775P platform.
> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> index 06bf2ba556b89b643da901857a9aa7cdc7ba90cc..d8b90bd4b1f05604185f015929a1f296799ad6a4 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <dt-bindings/interconnect/qcom,icc.h>
> +#include <dt-bindings/interconnect/qcom,osm-l3.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/qcom,rpmh.h>
>  #include <dt-bindings/clock/qcom,sa8775p-gcc.h>
> @@ -47,6 +48,10 @@ CPU0: cpu@0 {
>  			next-level-cache = <&L2_0>;
>  			capacity-dmips-mhz = <1024>;
>  			dynamic-power-coefficient = <100>;
> +			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
> +					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,

Please align the '&'s and squash with patch 2. This one doesn't cause
much difference on its own, which makes the commit message misleading

Konrad

Re: [PATCH 3/3] arm64: dts: qcom: sa8775p: Add LMH interrupts support

[Konrad Dybcio]

On 17.10.2024 11:28 AM, Jagadeesh Kona wrote:
> Add LMH interrupts support to indicate if there is any
> thermal throttle.
> 
> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> ---

'support' doesn't fit here, you're describing the interrupts, passing
them to the cpufreq node. 

Adding support for interrupts would be fitting if you e.g. added code
in the driver that would start consuming them.

The code itself looks in line with the docs

Konrad

Re: [PATCH 2/3] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

[Jagadeesh Kona]

On 10/17/2024 9:12 PM, Brian Masney wrote:
> On Thu, Oct 17, 2024 at 02:58:31PM +0530, Jagadeesh Kona wrote:
>> +	cpu0_opp_table: opp-table-cpu0 {
>> +		compatible = "operating-points-v2";
>> +		opp-shared;
>> +
>> +		cpu0_opp_1267mhz: opp-1267200000 {
>> +			opp-hz = /bits/ 64 <1267200000>;
>> +			opp-peak-kBps = <6220800 29491200>;
>> +		};
>> +
>> +		cpu0_opp_1363mhz: opp-1363200000 {
>> +			opp-hz = /bits/ 64 <1363200000>;
>> +			opp-peak-kBps = <6220800 29491200>;
>> +		};
> 
> [snip]
> 
>> +	cpu4_opp_table: opp-table-cpu4 {
>> +		compatible = "operating-points-v2";
>> +		opp-shared;
>> +
>> +		cpu4_opp_1267mhz: opp-1267200000 {
>> +			opp-hz = /bits/ 64 <1267200000>;
>> +			opp-peak-kBps = <6220800 29491200>;
>> +		};
>> +
>> +		cpu4_opp_1363mhz: opp-1363200000 {
>> +			opp-hz = /bits/ 64 <1363200000>;
>> +			opp-peak-kBps = <6220800 29491200>;
>> +		};
> 
> There's no functional differences in the cpu0 and cpu4 opp tables. Can
> a single table be used?
> 
> This aligns with my recollection that this particular SoC only has the
> gold cores.
> 
> Brian
> 

Thanks Brian for your review. Sorry for the delayed response.

We require separate OPP tables for CPU0 and CPU4 to allow independent
scaling of DDR and L3 frequencies for each CPU domain, with the final
DDR and L3 frequencies being an aggregate of both.

If we use a single OPP table for both CPU domains, then _allocate_opp_table() [1]
won't be invoked for CPU4. As a result both CPU devices will end up in sharing
the same ICC path handle, which could lead to one CPU device overwriting the bandwidth
votes of other.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1588

Thanks,
Jagadeesh
 

Re: [PATCH 1/3] arm64: dts: qcom: sa8775p: Add support to scale DDR/L3

[Jagadeesh Kona]

On 10/18/2024 4:22 AM, Konrad Dybcio wrote:
> On 17.10.2024 11:28 AM, Jagadeesh Kona wrote:
>> Add support to scale DDR and L3 based on CPU frequencies
>> on SA8775P platform.
>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> ---
>>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 33 +++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> index 06bf2ba556b89b643da901857a9aa7cdc7ba90cc..d8b90bd4b1f05604185f015929a1f296799ad6a4 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> @@ -4,6 +4,7 @@
>>   */
>>  
>>  #include <dt-bindings/interconnect/qcom,icc.h>
>> +#include <dt-bindings/interconnect/qcom,osm-l3.h>
>>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>>  #include <dt-bindings/clock/qcom,rpmh.h>
>>  #include <dt-bindings/clock/qcom,sa8775p-gcc.h>
>> @@ -47,6 +48,10 @@ CPU0: cpu@0 {
>>  			next-level-cache = <&L2_0>;
>>  			capacity-dmips-mhz = <1024>;
>>  			dynamic-power-coefficient = <100>;
>> +			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
>> +					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
> 
> Please align the '&'s and squash with patch 2. This one doesn't cause
> much difference on its own, which makes the commit message misleading
> 
> Konrad

Thanks Konrad for your review. Sure will take care of this in next series.

Thanks,
Jagadeesh

Re: [PATCH 3/3] arm64: dts: qcom: sa8775p: Add LMH interrupts support

[Jagadeesh Kona]

On 10/26/2024 5:57 PM, Konrad Dybcio wrote:
> On 17.10.2024 11:28 AM, Jagadeesh Kona wrote:
>> Add LMH interrupts support to indicate if there is any
>> thermal throttle.
>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> ---
> 
> 'support' doesn't fit here, you're describing the interrupts, passing
> them to the cpufreq node. 
> 
> Adding support for interrupts would be fitting if you e.g. added code
> in the driver that would start consuming them.
> 
> The code itself looks in line with the docs
> 
> Konrad

Sure, will update the commit text in next series.

Thanks,
Jagadeesh

Re: [PATCH 2/3] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

[Dmitry Baryshkov]

On Mon, Nov 11, 2024 at 06:39:48PM +0530, Jagadeesh Kona wrote:
> 
> 
> On 10/17/2024 9:12 PM, Brian Masney wrote:
> > On Thu, Oct 17, 2024 at 02:58:31PM +0530, Jagadeesh Kona wrote:
> >> +	cpu0_opp_table: opp-table-cpu0 {
> >> +		compatible = "operating-points-v2";
> >> +		opp-shared;
> >> +
> >> +		cpu0_opp_1267mhz: opp-1267200000 {
> >> +			opp-hz = /bits/ 64 <1267200000>;
> >> +			opp-peak-kBps = <6220800 29491200>;
> >> +		};
> >> +
> >> +		cpu0_opp_1363mhz: opp-1363200000 {
> >> +			opp-hz = /bits/ 64 <1363200000>;
> >> +			opp-peak-kBps = <6220800 29491200>;
> >> +		};
> > 
> > [snip]
> > 
> >> +	cpu4_opp_table: opp-table-cpu4 {
> >> +		compatible = "operating-points-v2";
> >> +		opp-shared;
> >> +
> >> +		cpu4_opp_1267mhz: opp-1267200000 {
> >> +			opp-hz = /bits/ 64 <1267200000>;
> >> +			opp-peak-kBps = <6220800 29491200>;
> >> +		};
> >> +
> >> +		cpu4_opp_1363mhz: opp-1363200000 {
> >> +			opp-hz = /bits/ 64 <1363200000>;
> >> +			opp-peak-kBps = <6220800 29491200>;
> >> +		};
> > 
> > There's no functional differences in the cpu0 and cpu4 opp tables. Can
> > a single table be used?
> > 
> > This aligns with my recollection that this particular SoC only has the
> > gold cores.
> > 
> > Brian
> > 
> 
> Thanks Brian for your review. Sorry for the delayed response.
> 
> We require separate OPP tables for CPU0 and CPU4 to allow independent
> scaling of DDR and L3 frequencies for each CPU domain, with the final
> DDR and L3 frequencies being an aggregate of both.
> 
> If we use a single OPP table for both CPU domains, then _allocate_opp_table() [1]
> won't be invoked for CPU4. As a result both CPU devices will end up in sharing
> the same ICC path handle, which could lead to one CPU device overwriting the bandwidth
> votes of other.

All of this should be a part of the commit message.

> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1588
> 
> Thanks,
> Jagadeesh
>  

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

[Konrad Dybcio]

On 14.11.2024 11:48 PM, Dmitry Baryshkov wrote:
> On Mon, Nov 11, 2024 at 06:39:48PM +0530, Jagadeesh Kona wrote:
>>
>>
>> On 10/17/2024 9:12 PM, Brian Masney wrote:
>>> On Thu, Oct 17, 2024 at 02:58:31PM +0530, Jagadeesh Kona wrote:
>>>> +	cpu0_opp_table: opp-table-cpu0 {
>>>> +		compatible = "operating-points-v2";
>>>> +		opp-shared;
>>>> +
>>>> +		cpu0_opp_1267mhz: opp-1267200000 {
>>>> +			opp-hz = /bits/ 64 <1267200000>;
>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>> +		};
>>>> +
>>>> +		cpu0_opp_1363mhz: opp-1363200000 {
>>>> +			opp-hz = /bits/ 64 <1363200000>;
>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>> +		};
>>>
>>> [snip]
>>>
>>>> +	cpu4_opp_table: opp-table-cpu4 {
>>>> +		compatible = "operating-points-v2";
>>>> +		opp-shared;
>>>> +
>>>> +		cpu4_opp_1267mhz: opp-1267200000 {
>>>> +			opp-hz = /bits/ 64 <1267200000>;
>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>> +		};
>>>> +
>>>> +		cpu4_opp_1363mhz: opp-1363200000 {
>>>> +			opp-hz = /bits/ 64 <1363200000>;
>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>> +		};
>>>
>>> There's no functional differences in the cpu0 and cpu4 opp tables. Can
>>> a single table be used?
>>>
>>> This aligns with my recollection that this particular SoC only has the
>>> gold cores.
>>>
>>> Brian
>>>
>>
>> Thanks Brian for your review. Sorry for the delayed response.
>>
>> We require separate OPP tables for CPU0 and CPU4 to allow independent
>> scaling of DDR and L3 frequencies for each CPU domain, with the final
>> DDR and L3 frequencies being an aggregate of both.
>>
>> If we use a single OPP table for both CPU domains, then _allocate_opp_table() [1]
>> won't be invoked for CPU4. As a result both CPU devices will end up in sharing
>> the same ICC path handle, which could lead to one CPU device overwriting the bandwidth
>> votes of other.

Oh that's a fun find.. clocks get the same treatment.. very bad,
but may explain some schroedingerbugs.

Taking a peek at some code paths, wouldn't dropping opp-shared
solve our issues? dev_pm_opp_set_sharing_cpus() overrides it

Konrad

Re: [PATCH 2/3] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

[Jagadeesh Kona]

On 11/15/2024 4:18 AM, Dmitry Baryshkov wrote:
> On Mon, Nov 11, 2024 at 06:39:48PM +0530, Jagadeesh Kona wrote:
>>
>>
>> On 10/17/2024 9:12 PM, Brian Masney wrote:
>>> On Thu, Oct 17, 2024 at 02:58:31PM +0530, Jagadeesh Kona wrote:
>>>> +	cpu0_opp_table: opp-table-cpu0 {
>>>> +		compatible = "operating-points-v2";
>>>> +		opp-shared;
>>>> +
>>>> +		cpu0_opp_1267mhz: opp-1267200000 {
>>>> +			opp-hz = /bits/ 64 <1267200000>;
>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>> +		};
>>>> +
>>>> +		cpu0_opp_1363mhz: opp-1363200000 {
>>>> +			opp-hz = /bits/ 64 <1363200000>;
>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>> +		};
>>>
>>> [snip]
>>>
>>>> +	cpu4_opp_table: opp-table-cpu4 {
>>>> +		compatible = "operating-points-v2";
>>>> +		opp-shared;
>>>> +
>>>> +		cpu4_opp_1267mhz: opp-1267200000 {
>>>> +			opp-hz = /bits/ 64 <1267200000>;
>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>> +		};
>>>> +
>>>> +		cpu4_opp_1363mhz: opp-1363200000 {
>>>> +			opp-hz = /bits/ 64 <1363200000>;
>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>> +		};
>>>
>>> There's no functional differences in the cpu0 and cpu4 opp tables. Can
>>> a single table be used?
>>>
>>> This aligns with my recollection that this particular SoC only has the
>>> gold cores.
>>>
>>> Brian
>>>
>>
>> Thanks Brian for your review. Sorry for the delayed response.
>>
>> We require separate OPP tables for CPU0 and CPU4 to allow independent
>> scaling of DDR and L3 frequencies for each CPU domain, with the final
>> DDR and L3 frequencies being an aggregate of both.
>>
>> If we use a single OPP table for both CPU domains, then _allocate_opp_table() [1]
>> won't be invoked for CPU4. As a result both CPU devices will end up in sharing
>> the same ICC path handle, which could lead to one CPU device overwriting the bandwidth
>> votes of other.
> 
> All of this should be a part of the commit message.
> 

Thanks Dmitry for your review. Will include this in commit text while posting the next series.

Thanks,
Jagadeesh

>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1588
>>
>> Thanks,
>> Jagadeesh
>>  
> 

Re: [PATCH 2/3] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

[Jagadeesh Kona]

On 11/30/2024 8:02 PM, Konrad Dybcio wrote:
> On 14.11.2024 11:48 PM, Dmitry Baryshkov wrote:
>> On Mon, Nov 11, 2024 at 06:39:48PM +0530, Jagadeesh Kona wrote:
>>>
>>>
>>> On 10/17/2024 9:12 PM, Brian Masney wrote:
>>>> On Thu, Oct 17, 2024 at 02:58:31PM +0530, Jagadeesh Kona wrote:
>>>>> +	cpu0_opp_table: opp-table-cpu0 {
>>>>> +		compatible = "operating-points-v2";
>>>>> +		opp-shared;
>>>>> +
>>>>> +		cpu0_opp_1267mhz: opp-1267200000 {
>>>>> +			opp-hz = /bits/ 64 <1267200000>;
>>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>>> +		};
>>>>> +
>>>>> +		cpu0_opp_1363mhz: opp-1363200000 {
>>>>> +			opp-hz = /bits/ 64 <1363200000>;
>>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>>> +		};
>>>>
>>>> [snip]
>>>>
>>>>> +	cpu4_opp_table: opp-table-cpu4 {
>>>>> +		compatible = "operating-points-v2";
>>>>> +		opp-shared;
>>>>> +
>>>>> +		cpu4_opp_1267mhz: opp-1267200000 {
>>>>> +			opp-hz = /bits/ 64 <1267200000>;
>>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>>> +		};
>>>>> +
>>>>> +		cpu4_opp_1363mhz: opp-1363200000 {
>>>>> +			opp-hz = /bits/ 64 <1363200000>;
>>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>>> +		};
>>>>
>>>> There's no functional differences in the cpu0 and cpu4 opp tables. Can
>>>> a single table be used?
>>>>
>>>> This aligns with my recollection that this particular SoC only has the
>>>> gold cores.
>>>>
>>>> Brian
>>>>
>>>
>>> Thanks Brian for your review. Sorry for the delayed response.
>>>
>>> We require separate OPP tables for CPU0 and CPU4 to allow independent
>>> scaling of DDR and L3 frequencies for each CPU domain, with the final
>>> DDR and L3 frequencies being an aggregate of both.
>>>
>>> If we use a single OPP table for both CPU domains, then _allocate_opp_table() [1]
>>> won't be invoked for CPU4. As a result both CPU devices will end up in sharing
>>> the same ICC path handle, which could lead to one CPU device overwriting the bandwidth
>>> votes of other.
> 
> Oh that's a fun find.. clocks get the same treatment.. very bad,
> but may explain some schroedingerbugs.
> 
> Taking a peek at some code paths, wouldn't dropping opp-shared
> solve our issues? dev_pm_opp_set_sharing_cpus() overrides it
> 
> Konrad

Thanks Konrad for your review.

Yes, correct. I tried dropping opp-shared but it is again getting set due to
dev_pm_opp_set_sharing_cpus().

Thanks,
Jagadeesh

Re: [PATCH 2/3] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

[Dmitry Baryshkov]

On Tue, Dec 03, 2024 at 08:33:46PM +0530, Jagadeesh Kona wrote:
> 
> 
> On 11/30/2024 8:02 PM, Konrad Dybcio wrote:
> > On 14.11.2024 11:48 PM, Dmitry Baryshkov wrote:
> >> On Mon, Nov 11, 2024 at 06:39:48PM +0530, Jagadeesh Kona wrote:
> >>>
> >>>
> >>> On 10/17/2024 9:12 PM, Brian Masney wrote:
> >>>> On Thu, Oct 17, 2024 at 02:58:31PM +0530, Jagadeesh Kona wrote:
> >>>>> +	cpu0_opp_table: opp-table-cpu0 {
> >>>>> +		compatible = "operating-points-v2";
> >>>>> +		opp-shared;
> >>>>> +
> >>>>> +		cpu0_opp_1267mhz: opp-1267200000 {
> >>>>> +			opp-hz = /bits/ 64 <1267200000>;
> >>>>> +			opp-peak-kBps = <6220800 29491200>;
> >>>>> +		};
> >>>>> +
> >>>>> +		cpu0_opp_1363mhz: opp-1363200000 {
> >>>>> +			opp-hz = /bits/ 64 <1363200000>;
> >>>>> +			opp-peak-kBps = <6220800 29491200>;
> >>>>> +		};
> >>>>
> >>>> [snip]
> >>>>
> >>>>> +	cpu4_opp_table: opp-table-cpu4 {
> >>>>> +		compatible = "operating-points-v2";
> >>>>> +		opp-shared;
> >>>>> +
> >>>>> +		cpu4_opp_1267mhz: opp-1267200000 {
> >>>>> +			opp-hz = /bits/ 64 <1267200000>;
> >>>>> +			opp-peak-kBps = <6220800 29491200>;
> >>>>> +		};
> >>>>> +
> >>>>> +		cpu4_opp_1363mhz: opp-1363200000 {
> >>>>> +			opp-hz = /bits/ 64 <1363200000>;
> >>>>> +			opp-peak-kBps = <6220800 29491200>;
> >>>>> +		};
> >>>>
> >>>> There's no functional differences in the cpu0 and cpu4 opp tables. Can
> >>>> a single table be used?
> >>>>
> >>>> This aligns with my recollection that this particular SoC only has the
> >>>> gold cores.
> >>>>
> >>>> Brian
> >>>>
> >>>
> >>> Thanks Brian for your review. Sorry for the delayed response.
> >>>
> >>> We require separate OPP tables for CPU0 and CPU4 to allow independent
> >>> scaling of DDR and L3 frequencies for each CPU domain, with the final
> >>> DDR and L3 frequencies being an aggregate of both.
> >>>
> >>> If we use a single OPP table for both CPU domains, then _allocate_opp_table() [1]
> >>> won't be invoked for CPU4. As a result both CPU devices will end up in sharing
> >>> the same ICC path handle, which could lead to one CPU device overwriting the bandwidth
> >>> votes of other.
> > 
> > Oh that's a fun find.. clocks get the same treatment.. very bad,
> > but may explain some schroedingerbugs.
> > 
> > Taking a peek at some code paths, wouldn't dropping opp-shared
> > solve our issues? dev_pm_opp_set_sharing_cpus() overrides it
> > 
> > Konrad
> 
> Thanks Konrad for your review.
> 
> Yes, correct. I tried dropping opp-shared but it is again getting set due to
> dev_pm_opp_set_sharing_cpus().

It should be set, but then it should get the limited CPU mask rather
than the full CPU set. Isn't that enough for your case?

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

[Jagadeesh Kona]

On 12/4/2024 8:43 AM, Dmitry Baryshkov wrote:
> On Tue, Dec 03, 2024 at 08:33:46PM +0530, Jagadeesh Kona wrote:
>>
>>
>> On 11/30/2024 8:02 PM, Konrad Dybcio wrote:
>>> On 14.11.2024 11:48 PM, Dmitry Baryshkov wrote:
>>>> On Mon, Nov 11, 2024 at 06:39:48PM +0530, Jagadeesh Kona wrote:
>>>>>
>>>>>
>>>>> On 10/17/2024 9:12 PM, Brian Masney wrote:
>>>>>> On Thu, Oct 17, 2024 at 02:58:31PM +0530, Jagadeesh Kona wrote:
>>>>>>> +	cpu0_opp_table: opp-table-cpu0 {
>>>>>>> +		compatible = "operating-points-v2";
>>>>>>> +		opp-shared;
>>>>>>> +
>>>>>>> +		cpu0_opp_1267mhz: opp-1267200000 {
>>>>>>> +			opp-hz = /bits/ 64 <1267200000>;
>>>>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>>>>> +		};
>>>>>>> +
>>>>>>> +		cpu0_opp_1363mhz: opp-1363200000 {
>>>>>>> +			opp-hz = /bits/ 64 <1363200000>;
>>>>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>>>>> +		};
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>> +	cpu4_opp_table: opp-table-cpu4 {
>>>>>>> +		compatible = "operating-points-v2";
>>>>>>> +		opp-shared;
>>>>>>> +
>>>>>>> +		cpu4_opp_1267mhz: opp-1267200000 {
>>>>>>> +			opp-hz = /bits/ 64 <1267200000>;
>>>>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>>>>> +		};
>>>>>>> +
>>>>>>> +		cpu4_opp_1363mhz: opp-1363200000 {
>>>>>>> +			opp-hz = /bits/ 64 <1363200000>;
>>>>>>> +			opp-peak-kBps = <6220800 29491200>;
>>>>>>> +		};
>>>>>>
>>>>>> There's no functional differences in the cpu0 and cpu4 opp tables. Can
>>>>>> a single table be used?
>>>>>>
>>>>>> This aligns with my recollection that this particular SoC only has the
>>>>>> gold cores.
>>>>>>
>>>>>> Brian
>>>>>>
>>>>>
>>>>> Thanks Brian for your review. Sorry for the delayed response.
>>>>>
>>>>> We require separate OPP tables for CPU0 and CPU4 to allow independent
>>>>> scaling of DDR and L3 frequencies for each CPU domain, with the final
>>>>> DDR and L3 frequencies being an aggregate of both.
>>>>>
>>>>> If we use a single OPP table for both CPU domains, then _allocate_opp_table() [1]
>>>>> won't be invoked for CPU4. As a result both CPU devices will end up in sharing
>>>>> the same ICC path handle, which could lead to one CPU device overwriting the bandwidth
>>>>> votes of other.
>>>
>>> Oh that's a fun find.. clocks get the same treatment.. very bad,
>>> but may explain some schroedingerbugs.
>>>
>>> Taking a peek at some code paths, wouldn't dropping opp-shared
>>> solve our issues? dev_pm_opp_set_sharing_cpus() overrides it
>>>
>>> Konrad
>>
>> Thanks Konrad for your review.
>>
>> Yes, correct. I tried dropping opp-shared but it is again getting set due to
>> dev_pm_opp_set_sharing_cpus().
> 
> It should be set, but then it should get the limited CPU mask rather
> than the full CPU set. Isn't that enough for your case?
> 

Even if we call dev_pm_opp_set_sharing_cpus() with the limited CPU mask, it adds
OPP_TABLE_ACCESS_SHARED flag to the OPP table. Due to this flag being set, if this
same opp table is used for another CPU domain(CPU4-7) also in DT, then _managed_opp[1]
which gets called inside from dev_pm_opp_of_add_table() for CPU4 will return the same
CPU0 OPP table. 

Due to above, _allocate_opp_table() [2] won't be invoked for CPU4 but instead CPU4 will be
added as device under the CPU0 OPP table [3]. Due to this, dev_pm_opp_of_find_icc_paths() [4]
won't be invoked for CPU4 device and hence CPU4 won't be able to independently scale it's
interconnects. Both CPU0 and CPU4 devices will scale the same ICC path which can lead to one
device overwriting the BW vote placed by other device. So we need two separate OPP tables for
both domains.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1600
[2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1613
[3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1606
[4] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1484

Thanks,
Jagadeesh

Re: [PATCH 2/3] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

[Dmitry Baryshkov]

On Wed, Dec 04, 2024 at 02:15:09PM +0530, Jagadeesh Kona wrote:
> 
> 
> On 12/4/2024 8:43 AM, Dmitry Baryshkov wrote:
> > On Tue, Dec 03, 2024 at 08:33:46PM +0530, Jagadeesh Kona wrote:
> >>
> >>
> >> On 11/30/2024 8:02 PM, Konrad Dybcio wrote:
> >>> On 14.11.2024 11:48 PM, Dmitry Baryshkov wrote:
> >>>> On Mon, Nov 11, 2024 at 06:39:48PM +0530, Jagadeesh Kona wrote:
> >>>>>
> >>>>>
> >>>>> On 10/17/2024 9:12 PM, Brian Masney wrote:
> >>>>>> On Thu, Oct 17, 2024 at 02:58:31PM +0530, Jagadeesh Kona wrote:
> >>>>>>> +	cpu0_opp_table: opp-table-cpu0 {
> >>>>>>> +		compatible = "operating-points-v2";
> >>>>>>> +		opp-shared;
> >>>>>>> +
> >>>>>>> +		cpu0_opp_1267mhz: opp-1267200000 {
> >>>>>>> +			opp-hz = /bits/ 64 <1267200000>;
> >>>>>>> +			opp-peak-kBps = <6220800 29491200>;
> >>>>>>> +		};
> >>>>>>> +
> >>>>>>> +		cpu0_opp_1363mhz: opp-1363200000 {
> >>>>>>> +			opp-hz = /bits/ 64 <1363200000>;
> >>>>>>> +			opp-peak-kBps = <6220800 29491200>;
> >>>>>>> +		};
> >>>>>>
> >>>>>> [snip]
> >>>>>>
> >>>>>>> +	cpu4_opp_table: opp-table-cpu4 {
> >>>>>>> +		compatible = "operating-points-v2";
> >>>>>>> +		opp-shared;
> >>>>>>> +
> >>>>>>> +		cpu4_opp_1267mhz: opp-1267200000 {
> >>>>>>> +			opp-hz = /bits/ 64 <1267200000>;
> >>>>>>> +			opp-peak-kBps = <6220800 29491200>;
> >>>>>>> +		};
> >>>>>>> +
> >>>>>>> +		cpu4_opp_1363mhz: opp-1363200000 {
> >>>>>>> +			opp-hz = /bits/ 64 <1363200000>;
> >>>>>>> +			opp-peak-kBps = <6220800 29491200>;
> >>>>>>> +		};
> >>>>>>
> >>>>>> There's no functional differences in the cpu0 and cpu4 opp tables. Can
> >>>>>> a single table be used?
> >>>>>>
> >>>>>> This aligns with my recollection that this particular SoC only has the
> >>>>>> gold cores.
> >>>>>>
> >>>>>> Brian
> >>>>>>
> >>>>>
> >>>>> Thanks Brian for your review. Sorry for the delayed response.
> >>>>>
> >>>>> We require separate OPP tables for CPU0 and CPU4 to allow independent
> >>>>> scaling of DDR and L3 frequencies for each CPU domain, with the final
> >>>>> DDR and L3 frequencies being an aggregate of both.
> >>>>>
> >>>>> If we use a single OPP table for both CPU domains, then _allocate_opp_table() [1]
> >>>>> won't be invoked for CPU4. As a result both CPU devices will end up in sharing
> >>>>> the same ICC path handle, which could lead to one CPU device overwriting the bandwidth
> >>>>> votes of other.
> >>>
> >>> Oh that's a fun find.. clocks get the same treatment.. very bad,
> >>> but may explain some schroedingerbugs.
> >>>
> >>> Taking a peek at some code paths, wouldn't dropping opp-shared
> >>> solve our issues? dev_pm_opp_set_sharing_cpus() overrides it
> >>>
> >>> Konrad
> >>
> >> Thanks Konrad for your review.
> >>
> >> Yes, correct. I tried dropping opp-shared but it is again getting set due to
> >> dev_pm_opp_set_sharing_cpus().
> > 
> > It should be set, but then it should get the limited CPU mask rather
> > than the full CPU set. Isn't that enough for your case?
> > 
> 
> Even if we call dev_pm_opp_set_sharing_cpus() with the limited CPU mask, it adds
> OPP_TABLE_ACCESS_SHARED flag to the OPP table. Due to this flag being set, if this
> same opp table is used for another CPU domain(CPU4-7) also in DT, then _managed_opp[1]
> which gets called inside from dev_pm_opp_of_add_table() for CPU4 will return the same
> CPU0 OPP table. 
> 
> Due to above, _allocate_opp_table() [2] won't be invoked for CPU4 but instead CPU4 will be
> added as device under the CPU0 OPP table [3]. Due to this, dev_pm_opp_of_find_icc_paths() [4]
> won't be invoked for CPU4 device and hence CPU4 won't be able to independently scale it's
> interconnects. Both CPU0 and CPU4 devices will scale the same ICC path which can lead to one
> device overwriting the BW vote placed by other device. So we need two separate OPP tables for
> both domains.

Ack, that makes sense. Thanks for the explanation!

> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1600
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1613
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1606
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/opp/core.c#n1484
> 
> Thanks,
> Jagadeesh

-- 
With best wishes
Dmitry