linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] DDR/L3 Scaling support on SC7280 SoCs
@ 2021-04-30 14:28 Sibi Sankar
  2021-04-30 14:28 ` [PATCH 1/2] cpufreq: blacklist SC7280 in cpufreq-dt-platdev Sibi Sankar
  2021-04-30 14:28 ` [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables Sibi Sankar
  0 siblings, 2 replies; 14+ messages in thread
From: Sibi Sankar @ 2021-04-30 14:28 UTC (permalink / raw)
  To: bjorn.andersson
  Cc: viresh.kumar, swboyd, agross, robh+dt, rjw, linux-arm-msm,
	devicetree, linux-kernel, linux-pm, dianders, mka, Sibi Sankar

The patch series adds support for DDR/L3 Scaling on SC7280 SoCs.

Depends on the following patch series:
L3 Provider Support: https://lore.kernel.org/lkml/1618556290-28303-1-git-send-email-okukatla@codeaurora.org/
CPUfreq Support: https://lore.kernel.org/lkml/1618020280-5470-2-git-send-email-tdas@codeaurora.org/
RPMH Provider Support: https://lore.kernel.org/lkml/1619517059-12109-1-git-send-email-okukatla@codeaurora.org/

It also depends on L3 and cpufreq dt nodes from the ^^ series to not have
overlapping memory regions.

Sibi Sankar (2):
  cpufreq: blacklist SC7280 in cpufreq-dt-platdev
  arm64: dts: qcom: sc7280: Add cpu OPP tables

 arch/arm64/boot/dts/qcom/sc7280.dtsi | 135 +++++++++++++++++++++++++++++++++++
 drivers/cpufreq/cpufreq-dt-platdev.c |   1 +
 2 files changed, 136 insertions(+)

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] cpufreq: blacklist SC7280 in cpufreq-dt-platdev
  2021-04-30 14:28 [PATCH 0/2] DDR/L3 Scaling support on SC7280 SoCs Sibi Sankar
@ 2021-04-30 14:28 ` Sibi Sankar
  2021-05-03 16:20   ` Matthias Kaehlcke
  2021-04-30 14:28 ` [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables Sibi Sankar
  1 sibling, 1 reply; 14+ messages in thread
From: Sibi Sankar @ 2021-04-30 14:28 UTC (permalink / raw)
  To: bjorn.andersson
  Cc: viresh.kumar, swboyd, agross, robh+dt, rjw, linux-arm-msm,
	devicetree, linux-kernel, linux-pm, dianders, mka, Sibi Sankar

Add SC7280 to cpufreq-dt-platdev blacklist since the actual scaling is
handled by the 'qcom-cpufreq-hw' driver.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 5e07065ec22f..345418b8250e 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -137,6 +137,7 @@ static const struct of_device_id blacklist[] __initconst = {
 	{ .compatible = "qcom,msm8996", },
 	{ .compatible = "qcom,qcs404", },
 	{ .compatible = "qcom,sc7180", },
+	{ .compatible = "qcom,sc7280", },
 	{ .compatible = "qcom,sdm845", },
 
 	{ .compatible = "st,stih407", },
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-04-30 14:28 [PATCH 0/2] DDR/L3 Scaling support on SC7280 SoCs Sibi Sankar
  2021-04-30 14:28 ` [PATCH 1/2] cpufreq: blacklist SC7280 in cpufreq-dt-platdev Sibi Sankar
@ 2021-04-30 14:28 ` Sibi Sankar
  2021-05-03 16:36   ` Matthias Kaehlcke
  2021-05-04 14:42   ` Sudeep Holla
  1 sibling, 2 replies; 14+ messages in thread
From: Sibi Sankar @ 2021-04-30 14:28 UTC (permalink / raw)
  To: bjorn.andersson
  Cc: viresh.kumar, swboyd, agross, robh+dt, rjw, linux-arm-msm,
	devicetree, linux-kernel, linux-pm, dianders, mka, Sibi Sankar

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

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 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
@@ -7,6 +7,7 @@
 
 #include <dt-bindings/clock/qcom,gcc-sc7280.h>
 #include <dt-bindings/clock/qcom,rpmh.h>
+#include <dt-bindings/interconnect/qcom,osm-l3.h>
 #include <dt-bindings/interconnect/qcom,sc7280.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/mailbox/qcom-ipcc.h>
@@ -71,6 +72,9 @@
 					   &LITTLE_CPU_SLEEP_1
 					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_0>;
+			operating-points-v2 = <&cpu0_opp_table>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
+					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			L2_0: l2-cache {
 				compatible = "cache";
@@ -90,6 +94,9 @@
 					   &LITTLE_CPU_SLEEP_1
 					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_100>;
+			operating-points-v2 = <&cpu0_opp_table>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
+					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			L2_100: l2-cache {
 				compatible = "cache";
@@ -106,6 +113,9 @@
 					   &LITTLE_CPU_SLEEP_1
 					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_200>;
+			operating-points-v2 = <&cpu0_opp_table>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
+					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			L2_200: l2-cache {
 				compatible = "cache";
@@ -122,6 +132,9 @@
 					   &LITTLE_CPU_SLEEP_1
 					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_300>;
+			operating-points-v2 = <&cpu0_opp_table>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
+					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			L2_300: l2-cache {
 				compatible = "cache";
@@ -138,6 +151,9 @@
 					   &BIG_CPU_SLEEP_1
 					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_400>;
+			operating-points-v2 = <&cpu4_opp_table>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
+					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			L2_400: l2-cache {
 				compatible = "cache";
@@ -154,6 +170,9 @@
 					   &BIG_CPU_SLEEP_1
 					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_500>;
+			operating-points-v2 = <&cpu4_opp_table>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
+					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			L2_500: l2-cache {
 				compatible = "cache";
@@ -170,6 +189,9 @@
 					   &BIG_CPU_SLEEP_1
 					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_600>;
+			operating-points-v2 = <&cpu4_opp_table>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
+					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			L2_600: l2-cache {
 				compatible = "cache";
@@ -186,6 +208,9 @@
 					   &BIG_CPU_SLEEP_1
 					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_700>;
+			operating-points-v2 = <&cpu4_opp_table>;
+			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
+					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
 			qcom,freq-domain = <&cpufreq_hw 2>;
 			L2_700: l2-cache {
 				compatible = "cache";
@@ -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>;
+		};
+	};
+
+	cpu4_opp_table: cpu4_opp_table {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		cpu4_opp1: opp-691200000 {
+			opp-hz = /bits/ 64 <691200000>;
+			opp-peak-kBps = <1804000 9600000>;
+		};
+
+		cpu4_opp2: opp-940800000 {
+			opp-hz = /bits/ 64 <940800000>;
+			opp-peak-kBps = <2188000 17817600>;
+		};
+
+		cpu4_opp3: opp-1228800000 {
+			opp-hz = /bits/ 64 <1228800000>;
+			opp-peak-kBps = <4068000 24576000>;
+		};
+
+		cpu4_opp4: opp-1344000000 {
+			opp-hz = /bits/ 64 <1344000000>;
+			opp-peak-kBps = <4068000 24576000>;
+		};
+
+		cpu4_opp5: opp-1516800000 {
+			opp-hz = /bits/ 64 <1516800000>;
+			opp-peak-kBps = <4068000 24576000>;
+		};
+
+		cpu4_opp6: opp-1651200000 {
+			opp-hz = /bits/ 64 <1651200000>;
+			opp-peak-kBps = <6220000 38092800>;
+		};
+
+		cpu4_opp7: opp-1900800000 {
+			opp-hz = /bits/ 64 <1900800000>;
+			opp-peak-kBps = <6220000 44851200>;
+		};
+
+		cpu4_opp8: opp-2054400000 {
+			opp-hz = /bits/ 64 <2054400000>;
+			opp-peak-kBps = <6220000 44851200>;
+		};
+
+		cpu4_opp9: opp-2131200000 {
+			opp-hz = /bits/ 64 <2131200000>;
+			opp-peak-kBps = <6220000 44851200>;
+		};
+
+		cpu4_opp10: opp-2400000000 {
+			opp-hz = /bits/ 64 <2400000000>;
+			opp-peak-kBps = <6832000 48537600>;
+		};
+	};
+
 	memory@80000000 {
 		device_type = "memory";
 		/* We expect the bootloader to fill in the size */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] cpufreq: blacklist SC7280 in cpufreq-dt-platdev
  2021-04-30 14:28 ` [PATCH 1/2] cpufreq: blacklist SC7280 in cpufreq-dt-platdev Sibi Sankar
@ 2021-05-03 16:20   ` Matthias Kaehlcke
  0 siblings, 0 replies; 14+ messages in thread
From: Matthias Kaehlcke @ 2021-05-03 16:20 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: bjorn.andersson, viresh.kumar, swboyd, agross, robh+dt, rjw,
	linux-arm-msm, devicetree, linux-kernel, linux-pm, dianders

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

> Subject: cpufreq: blacklist SC7280 in cpufreq-dt-platdev
>
> Add SC7280 to cpufreq-dt-platdev blacklist since the actual scaling is
> handled by the 'qcom-cpufreq-hw' driver.

The coding style recommends the use of "allowlist" or "passlist" of
"blacklist":

https://elixir.bootlin.com/linux/v5.12/source/Documentation/process/coding-style.rst#L338

It's existing code so you aren't necessarily expected to change it. But
you could still use the recommended terminology in the commit message.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-04-30 14:28 ` [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables Sibi Sankar
@ 2021-05-03 16:36   ` Matthias Kaehlcke
  2021-05-04  7:05     ` Sibi Sankar
  2021-05-04 14:42   ` Sudeep Holla
  1 sibling, 1 reply; 14+ messages in thread
From: Matthias Kaehlcke @ 2021-05-03 16:36 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: bjorn.andersson, viresh.kumar, swboyd, agross, robh+dt, rjw,
	linux-arm-msm, devicetree, linux-kernel, linux-pm, dianders

On Fri, Apr 30, 2021 at 07:58:21PM +0530, Sibi Sankar wrote:
> Add OPP tables required to scale DDR/L3 per freq-domain on SC7280 SoCs.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  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
> @@ -7,6 +7,7 @@
>  
>  #include <dt-bindings/clock/qcom,gcc-sc7280.h>
>  #include <dt-bindings/clock/qcom,rpmh.h>
> +#include <dt-bindings/interconnect/qcom,osm-l3.h>
>  #include <dt-bindings/interconnect/qcom,sc7280.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/mailbox/qcom-ipcc.h>
> @@ -71,6 +72,9 @@
>  					   &LITTLE_CPU_SLEEP_1
>  					   &CLUSTER_SLEEP_0>;
>  			next-level-cache = <&L2_0>;
> +			operating-points-v2 = <&cpu0_opp_table>;
> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;

This patch seems to depend on the 'Add SC7280 interconnect provider
driver' series (https://patchwork.kernel.org/project/linux-arm-msm/list/?series=473747)
and 'Add L3 provider support for SC7280' (https://patchwork.kernel.org/project/linux-arm-msm/list/?series=468285),
none of them has landed yet. The dependencies should be mentioned in the
commit notes (under '---').

>  			qcom,freq-domain = <&cpufreq_hw 0>;
>  			L2_0: l2-cache {
>  				compatible = "cache";
> @@ -90,6 +94,9 @@
>  					   &LITTLE_CPU_SLEEP_1
>  					   &CLUSTER_SLEEP_0>;
>  			next-level-cache = <&L2_100>;
> +			operating-points-v2 = <&cpu0_opp_table>;
> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>  			qcom,freq-domain = <&cpufreq_hw 0>;
>  			L2_100: l2-cache {
>  				compatible = "cache";
> @@ -106,6 +113,9 @@
>  					   &LITTLE_CPU_SLEEP_1
>  					   &CLUSTER_SLEEP_0>;
>  			next-level-cache = <&L2_200>;
> +			operating-points-v2 = <&cpu0_opp_table>;
> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>  			qcom,freq-domain = <&cpufreq_hw 0>;
>  			L2_200: l2-cache {
>  				compatible = "cache";
> @@ -122,6 +132,9 @@
>  					   &LITTLE_CPU_SLEEP_1
>  					   &CLUSTER_SLEEP_0>;
>  			next-level-cache = <&L2_300>;
> +			operating-points-v2 = <&cpu0_opp_table>;
> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>  			qcom,freq-domain = <&cpufreq_hw 0>;
>  			L2_300: l2-cache {
>  				compatible = "cache";
> @@ -138,6 +151,9 @@
>  					   &BIG_CPU_SLEEP_1
>  					   &CLUSTER_SLEEP_0>;
>  			next-level-cache = <&L2_400>;
> +			operating-points-v2 = <&cpu4_opp_table>;
> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>  			qcom,freq-domain = <&cpufreq_hw 1>;
>  			L2_400: l2-cache {
>  				compatible = "cache";
> @@ -154,6 +170,9 @@
>  					   &BIG_CPU_SLEEP_1
>  					   &CLUSTER_SLEEP_0>;
>  			next-level-cache = <&L2_500>;
> +			operating-points-v2 = <&cpu4_opp_table>;
> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>  			qcom,freq-domain = <&cpufreq_hw 1>;
>  			L2_500: l2-cache {
>  				compatible = "cache";
> @@ -170,6 +189,9 @@
>  					   &BIG_CPU_SLEEP_1
>  					   &CLUSTER_SLEEP_0>;
>  			next-level-cache = <&L2_600>;
> +			operating-points-v2 = <&cpu4_opp_table>;
> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>  			qcom,freq-domain = <&cpufreq_hw 1>;
>  			L2_600: l2-cache {
>  				compatible = "cache";
> @@ -186,6 +208,9 @@
>  					   &BIG_CPU_SLEEP_1
>  					   &CLUSTER_SLEEP_0>;
>  			next-level-cache = <&L2_700>;
> +			operating-points-v2 = <&cpu4_opp_table>;
> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>  			qcom,freq-domain = <&cpufreq_hw 2>;
>  			L2_700: l2-cache {
>  				compatible = "cache";
> @@ -248,6 +273,116 @@
>  		};
>  	};
>  
> +	cpu0_opp_table: cpu0_opp_table {

the node name should use dashes as separators instead of underscores, i.e.
it should be '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>;
> +		};
> +	};
> +
> +	cpu4_opp_table: cpu4_opp_table {

node name should be 'cpu4-opp-table'

> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		cpu4_opp1: opp-691200000 {
> +			opp-hz = /bits/ 64 <691200000>;
> +			opp-peak-kBps = <1804000 9600000>;
> +		};
> +
> +		cpu4_opp2: opp-940800000 {
> +			opp-hz = /bits/ 64 <940800000>;
> +			opp-peak-kBps = <2188000 17817600>;
> +		};
> +
> +		cpu4_opp3: opp-1228800000 {
> +			opp-hz = /bits/ 64 <1228800000>;
> +			opp-peak-kBps = <4068000 24576000>;
> +		};
> +
> +		cpu4_opp4: opp-1344000000 {
> +			opp-hz = /bits/ 64 <1344000000>;
> +			opp-peak-kBps = <4068000 24576000>;
> +		};
> +
> +		cpu4_opp5: opp-1516800000 {
> +			opp-hz = /bits/ 64 <1516800000>;
> +			opp-peak-kBps = <4068000 24576000>;
> +		};
> +
> +		cpu4_opp6: opp-1651200000 {
> +			opp-hz = /bits/ 64 <1651200000>;
> +			opp-peak-kBps = <6220000 38092800>;
> +		};
> +
> +		cpu4_opp7: opp-1900800000 {
> +			opp-hz = /bits/ 64 <1900800000>;
> +			opp-peak-kBps = <6220000 44851200>;
> +		};
> +
> +		cpu4_opp8: opp-2054400000 {
> +			opp-hz = /bits/ 64 <2054400000>;
> +			opp-peak-kBps = <6220000 44851200>;
> +		};
> +
> +		cpu4_opp9: opp-2131200000 {
> +			opp-hz = /bits/ 64 <2131200000>;
> +			opp-peak-kBps = <6220000 44851200>;
> +		};
> +
> +		cpu4_opp10: opp-2400000000 {
> +			opp-hz = /bits/ 64 <2400000000>;
> +			opp-peak-kBps = <6832000 48537600>;
> +		};
> +	};
> +
>  	memory@80000000 {
>  		device_type = "memory";
>  		/* We expect the bootloader to fill in the size */

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-05-03 16:36   ` Matthias Kaehlcke
@ 2021-05-04  7:05     ` Sibi Sankar
  0 siblings, 0 replies; 14+ messages in thread
From: Sibi Sankar @ 2021-05-04  7:05 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: bjorn.andersson, viresh.kumar, swboyd, agross, robh+dt, rjw,
	linux-arm-msm, devicetree, linux-kernel, linux-pm, dianders

On 2021-05-03 22:06, Matthias Kaehlcke wrote:
> On Fri, Apr 30, 2021 at 07:58:21PM +0530, Sibi Sankar wrote:
>> Add OPP tables required to scale DDR/L3 per freq-domain on SC7280 
>> SoCs.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  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
>> @@ -7,6 +7,7 @@
>> 
>>  #include <dt-bindings/clock/qcom,gcc-sc7280.h>
>>  #include <dt-bindings/clock/qcom,rpmh.h>
>> +#include <dt-bindings/interconnect/qcom,osm-l3.h>
>>  #include <dt-bindings/interconnect/qcom,sc7280.h>
>>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>>  #include <dt-bindings/mailbox/qcom-ipcc.h>
>> @@ -71,6 +72,9 @@
>>  					   &LITTLE_CPU_SLEEP_1
>>  					   &CLUSTER_SLEEP_0>;
>>  			next-level-cache = <&L2_0>;
>> +			operating-points-v2 = <&cpu0_opp_table>;
>> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 
>> 3>,
>> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
> 
> This patch seems to depend on the 'Add SC7280 interconnect provider
> driver' series
> (https://patchwork.kernel.org/project/linux-arm-msm/list/?series=473747)
> and 'Add L3 provider support for SC7280'
> (https://patchwork.kernel.org/project/linux-arm-msm/list/?series=468285),
> none of them has landed yet. The dependencies should be mentioned in 
> the
> commit notes (under '---').

Had all the dependencies listed in the cover
letter. I'll make sure I put them ^^ as well
in v3.

> 
>>  			qcom,freq-domain = <&cpufreq_hw 0>;
>>  			L2_0: l2-cache {
>>  				compatible = "cache";
>> @@ -90,6 +94,9 @@
>>  					   &LITTLE_CPU_SLEEP_1
>>  					   &CLUSTER_SLEEP_0>;
>>  			next-level-cache = <&L2_100>;
>> +			operating-points-v2 = <&cpu0_opp_table>;
>> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 
>> 3>,
>> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>>  			qcom,freq-domain = <&cpufreq_hw 0>;
>>  			L2_100: l2-cache {
>>  				compatible = "cache";
>> @@ -106,6 +113,9 @@
>>  					   &LITTLE_CPU_SLEEP_1
>>  					   &CLUSTER_SLEEP_0>;
>>  			next-level-cache = <&L2_200>;
>> +			operating-points-v2 = <&cpu0_opp_table>;
>> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 
>> 3>,
>> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>>  			qcom,freq-domain = <&cpufreq_hw 0>;
>>  			L2_200: l2-cache {
>>  				compatible = "cache";
>> @@ -122,6 +132,9 @@
>>  					   &LITTLE_CPU_SLEEP_1
>>  					   &CLUSTER_SLEEP_0>;
>>  			next-level-cache = <&L2_300>;
>> +			operating-points-v2 = <&cpu0_opp_table>;
>> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 
>> 3>,
>> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>>  			qcom,freq-domain = <&cpufreq_hw 0>;
>>  			L2_300: l2-cache {
>>  				compatible = "cache";
>> @@ -138,6 +151,9 @@
>>  					   &BIG_CPU_SLEEP_1
>>  					   &CLUSTER_SLEEP_0>;
>>  			next-level-cache = <&L2_400>;
>> +			operating-points-v2 = <&cpu4_opp_table>;
>> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 
>> 3>,
>> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>>  			qcom,freq-domain = <&cpufreq_hw 1>;
>>  			L2_400: l2-cache {
>>  				compatible = "cache";
>> @@ -154,6 +170,9 @@
>>  					   &BIG_CPU_SLEEP_1
>>  					   &CLUSTER_SLEEP_0>;
>>  			next-level-cache = <&L2_500>;
>> +			operating-points-v2 = <&cpu4_opp_table>;
>> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 
>> 3>,
>> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>>  			qcom,freq-domain = <&cpufreq_hw 1>;
>>  			L2_500: l2-cache {
>>  				compatible = "cache";
>> @@ -170,6 +189,9 @@
>>  					   &BIG_CPU_SLEEP_1
>>  					   &CLUSTER_SLEEP_0>;
>>  			next-level-cache = <&L2_600>;
>> +			operating-points-v2 = <&cpu4_opp_table>;
>> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 
>> 3>,
>> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>>  			qcom,freq-domain = <&cpufreq_hw 1>;
>>  			L2_600: l2-cache {
>>  				compatible = "cache";
>> @@ -186,6 +208,9 @@
>>  					   &BIG_CPU_SLEEP_1
>>  					   &CLUSTER_SLEEP_0>;
>>  			next-level-cache = <&L2_700>;
>> +			operating-points-v2 = <&cpu4_opp_table>;
>> +			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 
>> 3>,
>> +					<&epss_l3 MASTER_EPSS_L3_APPS &epss_l3 SLAVE_EPSS_L3_SHARED>;
>>  			qcom,freq-domain = <&cpufreq_hw 2>;
>>  			L2_700: l2-cache {
>>  				compatible = "cache";
>> @@ -248,6 +273,116 @@
>>  		};
>>  	};
>> 
>> +	cpu0_opp_table: cpu0_opp_table {
> 
> the node name should use dashes as separators instead of underscores, 
> i.e.
> it should be 'cpu0-opp-table'.

will fix it in v3.

> 
>> +		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>;
>> +		};
>> +	};
>> +
>> +	cpu4_opp_table: cpu4_opp_table {
> 
> node name should be 'cpu4-opp-table'

will fix it in v3.

> 
>> +		compatible = "operating-points-v2";
>> +		opp-shared;
>> +
>> +		cpu4_opp1: opp-691200000 {
>> +			opp-hz = /bits/ 64 <691200000>;
>> +			opp-peak-kBps = <1804000 9600000>;
>> +		};
>> +
>> +		cpu4_opp2: opp-940800000 {
>> +			opp-hz = /bits/ 64 <940800000>;
>> +			opp-peak-kBps = <2188000 17817600>;
>> +		};
>> +
>> +		cpu4_opp3: opp-1228800000 {
>> +			opp-hz = /bits/ 64 <1228800000>;
>> +			opp-peak-kBps = <4068000 24576000>;
>> +		};
>> +
>> +		cpu4_opp4: opp-1344000000 {
>> +			opp-hz = /bits/ 64 <1344000000>;
>> +			opp-peak-kBps = <4068000 24576000>;
>> +		};
>> +
>> +		cpu4_opp5: opp-1516800000 {
>> +			opp-hz = /bits/ 64 <1516800000>;
>> +			opp-peak-kBps = <4068000 24576000>;
>> +		};
>> +
>> +		cpu4_opp6: opp-1651200000 {
>> +			opp-hz = /bits/ 64 <1651200000>;
>> +			opp-peak-kBps = <6220000 38092800>;
>> +		};
>> +
>> +		cpu4_opp7: opp-1900800000 {
>> +			opp-hz = /bits/ 64 <1900800000>;
>> +			opp-peak-kBps = <6220000 44851200>;
>> +		};
>> +
>> +		cpu4_opp8: opp-2054400000 {
>> +			opp-hz = /bits/ 64 <2054400000>;
>> +			opp-peak-kBps = <6220000 44851200>;
>> +		};
>> +
>> +		cpu4_opp9: opp-2131200000 {
>> +			opp-hz = /bits/ 64 <2131200000>;
>> +			opp-peak-kBps = <6220000 44851200>;
>> +		};
>> +
>> +		cpu4_opp10: opp-2400000000 {
>> +			opp-hz = /bits/ 64 <2400000000>;
>> +			opp-peak-kBps = <6832000 48537600>;
>> +		};
>> +	};
>> +
>>  	memory@80000000 {
>>  		device_type = "memory";
>>  		/* We expect the bootloader to fill in the size */

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-04-30 14:28 ` [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables Sibi Sankar
  2021-05-03 16:36   ` Matthias Kaehlcke
@ 2021-05-04 14:42   ` Sudeep Holla
  2021-05-04 18:25     ` Sibi Sankar
  1 sibling, 1 reply; 14+ messages in thread
From: Sudeep Holla @ 2021-05-04 14:42 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: bjorn.andersson, viresh.kumar, Sudeep Holla, swboyd, agross,
	robh+dt, rjw, linux-arm-msm, devicetree, linux-kernel, linux-pm,
	dianders, mka

On Fri, Apr 30, 2021 at 07:58:21PM +0530, Sibi Sankar wrote:
> Add OPP tables required to scale DDR/L3 per freq-domain on SC7280 SoCs.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  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


[...]

> @@ -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.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-05-04 14:42   ` Sudeep Holla
@ 2021-05-04 18:25     ` Sibi Sankar
  2021-05-04 19:16       ` Matthias Kaehlcke
  2021-05-05  8:49       ` Sudeep Holla
  0 siblings, 2 replies; 14+ messages in thread
From: Sibi Sankar @ 2021-05-04 18:25 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: bjorn.andersson, viresh.kumar, swboyd, agross, robh+dt, rjw,
	linux-arm-msm, devicetree, linux-kernel, linux-pm, dianders, mka

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:
>> Add OPP tables required to scale DDR/L3 per freq-domain on SC7280 
>> SoCs.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  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
> 
> 
> [...]
> 
>> @@ -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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-05-04 18:25     ` Sibi Sankar
@ 2021-05-04 19:16       ` Matthias Kaehlcke
  2021-05-05  8:49       ` Sudeep Holla
  1 sibling, 0 replies; 14+ messages in thread
From: Matthias Kaehlcke @ 2021-05-04 19:16 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: Sudeep Holla, bjorn.andersson, viresh.kumar, swboyd, agross,
	robh+dt, rjw, linux-arm-msm, devicetree, linux-kernel, linux-pm,
	dianders

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:
> > On Fri, Apr 30, 2021 at 07:58:21PM +0530, Sibi Sankar wrote:
> > > Add OPP tables required to scale DDR/L3 per freq-domain on SC7280
> > > SoCs.
> > > 
> > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> > > ---
> > >  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
> > 
> > 
> > [...]
> > 
> > > @@ -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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-05-04 18:25     ` Sibi Sankar
  2021-05-04 19:16       ` Matthias Kaehlcke
@ 2021-05-05  8:49       ` Sudeep Holla
  2021-05-05 10:09         ` Sibi Sankar
  2021-05-05 11:37         ` Viresh Kumar
  1 sibling, 2 replies; 14+ messages in thread
From: Sudeep Holla @ 2021-05-05  8:49 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: bjorn.andersson, viresh.kumar, Sudeep Holla, swboyd, agross,
	robh+dt, rjw, linux-arm-msm, devicetree, linux-kernel, linux-pm,
	dianders, mka

Hi Sibi,

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:

[...]

> >
> > 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

(You can always add commit sha to make it easy to search)

But I am not sure how this is related to the above commit anyways.

>
> 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.

No my main concern is this platform uses "qcom-cpufreq-hw" driver and the
fact that the OPPs are retrieved from the hardware lookup table invalidates
whatever we have in DT. In short it will be junk and becomes obsolete.
So what I suggested before is still valid. You simply can't have static
OPP tables in the DT for this platform. Do get some boot code to fetch the
same from the h/w LUT and patch to the DT or figure out any other way to
manage dynamically.

So NACK still stands for static addition of OPPs to the DT as in this patch.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-05-05  8:49       ` Sudeep Holla
@ 2021-05-05 10:09         ` Sibi Sankar
  2021-05-05 11:41           ` Sudeep Holla
  2021-05-05 11:37         ` Viresh Kumar
  1 sibling, 1 reply; 14+ messages in thread
From: Sibi Sankar @ 2021-05-05 10:09 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: bjorn.andersson, viresh.kumar, swboyd, agross, robh+dt, rjw,
	linux-arm-msm, devicetree, linux-kernel, linux-pm, dianders, mka

On 2021-05-05 14:19, Sudeep Holla wrote:
> Hi Sibi,
> 
> 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:
> 
> [...]
> 
>> >
>> > 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
> 
> (You can always add commit sha to make it easy to search)
> 
> But I am not sure how this is related to the above commit anyways.
> 
>> 
>> 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.
> 
> No my main concern is this platform uses "qcom-cpufreq-hw" driver and 
> the
> fact that the OPPs are retrieved from the hardware lookup table 
> invalidates
> whatever we have in DT. In short it will be junk and becomes obsolete.

The table provides mapping to bandwidths
which aren't available in the firmware
though. In short we do have to store the
mapping somewhere i.e. a mapping that
lists all possible frequencies to its
bandwidth requirements needs to be present
and using a opp table with the interconnect
bw bindings was the consensus reached.

Given that a duplicate mapping that lists
all possible frequencies to bw is inevitable
and Qualcomm has a way of listing all the
supported frequencies for the SoC, I feel
that dt breakage in the future should be
a non-concern. Not sure why you call it
junk since it solves the perf/power
requirements on SDM845/SC7180 SoCs. When
it becomes obsolete it would mean that
they are better devfreq governors available
upstream and that's a good reason for the
opp tables to go away.

> So what I suggested before is still valid. You simply can't have static
> OPP tables in the DT for this platform. Do get some boot code to fetch 
> the
> same from the h/w LUT and patch to the DT or figure out any other way 
> to
> manage dynamically.

moving the logic to boot loader doesn't
magically fix your concerns though (since
it would also need a superset of available
frequencies). It will suffer from the same
problems with an additional dependency on
firmware propagation in case of breakages
which is something you can avoid for the
simple cpu based scaling solution.

> 
> So NACK still stands for static addition of OPPs to the DT as in this 
> patch.

I'll let Viresh take the call since this
solution is already used on older SoCs.

> 
> --
> Regards,
> Sudeep

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-05-05  8:49       ` Sudeep Holla
  2021-05-05 10:09         ` Sibi Sankar
@ 2021-05-05 11:37         ` Viresh Kumar
  2021-05-05 11:44           ` Sudeep Holla
  1 sibling, 1 reply; 14+ messages in thread
From: Viresh Kumar @ 2021-05-05 11:37 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Sibi Sankar, bjorn.andersson, swboyd, agross, robh+dt, rjw,
	linux-arm-msm, devicetree, linux-kernel, linux-pm, dianders, mka

On 05-05-21, 09:49, Sudeep Holla wrote:
> No my main concern is this platform uses "qcom-cpufreq-hw" driver and the
> fact that the OPPs are retrieved from the hardware lookup table invalidates
> whatever we have in DT.

Not exactly.

It disables them all, and then call dev_pm_opp_adjust_voltage() and
enable them again. This is how it started initially. Though the driver
also works if the DT doesn't have the table, in that case it calls
dev_pm_opp_add() for all the OPPs.

> In short it will be junk and becomes obsolete.
> So what I suggested before is still valid. You simply can't have static
> OPP tables in the DT for this platform. Do get some boot code to fetch the
> same from the h/w LUT and patch to the DT or figure out any other way to
> manage dynamically.
> 
> So NACK still stands for static addition of OPPs to the DT as in this patch.

-- 
viresh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-05-05 10:09         ` Sibi Sankar
@ 2021-05-05 11:41           ` Sudeep Holla
  0 siblings, 0 replies; 14+ messages in thread
From: Sudeep Holla @ 2021-05-05 11:41 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: bjorn.andersson, viresh.kumar, Sudeep Holla, swboyd, agross,
	robh+dt, rjw, linux-arm-msm, devicetree, linux-kernel, linux-pm,
	dianders, mka

On Wed, May 05, 2021 at 03:39:17PM +0530, Sibi Sankar wrote:
> On 2021-05-05 14:19, Sudeep Holla wrote:

[...]

> > But I am not sure how this is related to the above commit anyways.
> >
> > >
> > > 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.
> >
> > No my main concern is this platform uses "qcom-cpufreq-hw" driver and
> > the
> > fact that the OPPs are retrieved from the hardware lookup table
> > invalidates
> > whatever we have in DT. In short it will be junk and becomes obsolete.
>
> The table provides mapping to bandwidths which aren't available in the
> firmware though. In short we do have to store the mapping somewhere i.e. a
> mapping that lists all possible frequencies to its bandwidth requirements
> needs to be present and using a opp table with the interconnect bw bindings
> was the consensus reached.

I understand and that's exactly what I had pointed out earlier when I
mentioned that I had raised this concern previously.

> Given that a duplicate mapping that lists all possible frequencies to bw is
> inevitable

I disagree, it was made inevitable by not listening to all feedback, sorry.

> and Qualcomm has a way of listing all the supported frequencies for the SoC,
> I feel that dt breakage in the future should be a non-concern.

I don't completely understand this TBH. Also my main worry is as we move more
towards abstract scale and/or index based, any addition or deletion of OPPs
results in change in the index or scale. It may be dealt on absolute scale
today everywhere and not a problem *today*, but it will break IMO.

> Not sure why you call it junk since it solves the perf/power requirements on
> SDM845/SC7180 SoCs. When it becomes obsolete it would mean that they are
> better devfreq governors available upstream and that's a good reason for the
> opp tables to go away.
>

Nope, I meant the firmware updates the OPP table underneath for whatever
valid reasons it may have.

> > So what I suggested before is still valid. You simply can't have static
> > OPP tables in the DT for this platform. Do get some boot code to fetch
> > the same from the h/w LUT and patch to the DT or figure out any other way
> > to manage dynamically.
>
> moving the logic to boot loader doesn't magically fix your concerns though
> (since it would also need a superset of available frequencies).


OK that's interesting. I wanted to fetch the exact list from the hardware
every time.

> It will suffer from the same problems with an additional dependency on
> firmware propagation in case of breakages which is something you can avoid
> for the simple cpu based scaling solution.

IMO the cross dependency is one part of problem and fetching the exact list
of OPPs available for the CPU is another. I meant to fix the latter in boot
code. The former is something I assume DT bindings deals with.

> >
> > So NACK still stands for static addition of OPPs to the DT as in this
> > patch.
>
> I'll let Viresh take the call since this solution is already used on older SoCs.

Sure, definitely I am just expressing my concern with NACK and I don't have
the final say 😁.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables
  2021-05-05 11:37         ` Viresh Kumar
@ 2021-05-05 11:44           ` Sudeep Holla
  0 siblings, 0 replies; 14+ messages in thread
From: Sudeep Holla @ 2021-05-05 11:44 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Sibi Sankar, bjorn.andersson, Sudeep Holla, swboyd, agross,
	robh+dt, rjw, linux-arm-msm, devicetree, linux-kernel, linux-pm,
	dianders, mka

On Wed, May 05, 2021 at 05:07:24PM +0530, Viresh Kumar wrote:
> On 05-05-21, 09:49, Sudeep Holla wrote:
> > No my main concern is this platform uses "qcom-cpufreq-hw" driver and the
> > fact that the OPPs are retrieved from the hardware lookup table invalidates
> > whatever we have in DT.
>
> Not exactly.
>
> It disables them all, and then call dev_pm_opp_adjust_voltage() and
> enable them again. This is how it started initially. Though the driver
> also works if the DT doesn't have the table, in that case it calls
> dev_pm_opp_add() for all the OPPs.
>

Ah OK, if it is handled in the driver, I will shut up then 😄. I did a
quick look at it but couldn't understand the connection, so I started
and continued the discussion. Thanks for the confirmation. I am fine if
it is handled.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-05-05 11:44 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-30 14:28 [PATCH 0/2] DDR/L3 Scaling support on SC7280 SoCs Sibi Sankar
2021-04-30 14:28 ` [PATCH 1/2] cpufreq: blacklist SC7280 in cpufreq-dt-platdev Sibi Sankar
2021-05-03 16:20   ` Matthias Kaehlcke
2021-04-30 14:28 ` [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables Sibi Sankar
2021-05-03 16:36   ` Matthias Kaehlcke
2021-05-04  7:05     ` Sibi Sankar
2021-05-04 14:42   ` Sudeep Holla
2021-05-04 18:25     ` Sibi Sankar
2021-05-04 19:16       ` Matthias Kaehlcke
2021-05-05  8:49       ` Sudeep Holla
2021-05-05 10:09         ` Sibi Sankar
2021-05-05 11:41           ` Sudeep Holla
2021-05-05 11:37         ` Viresh Kumar
2021-05-05 11:44           ` Sudeep Holla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).