Skip to contentSkip to conversation

DDR/L3 Scaling support on SDM845 and SC7180 SoCs

PATCH Superseded Sibi Sankar 3 participants 15 messages Jun 5, 2020 – Jun 18, 2020

Subsystems (4)
Arm Qualcomm mailing list CPU frequency scaling framework Power management core Operating performance points (OPP)
lore
Share & tools
lore

Sibi Sankar

author
#0 lore raw

v6 cover DDR/L3 Scaling support on SDM845 and SC7180 SoCs

This patch series aims to extend cpu based scaling support to L3/DDR on SDM845 and SC7180 SoCs.

Patches [1-2] - Blacklist SDM845 and SC7180 in cpufreq-dt-platdev Patches [3-5] - Update bw levels based on cpu frequency change

Based on Viresh's opp-next:
https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=opp/linux-next

V6:
* Add global flag to distinguish between voltage update and opp add. Use the same flag before trying to scale ddr/l3 bw [Viresh]
* Use dev_pm_opp_find_freq_ceil to grab all opps [Viresh]
* Move dev_pm_opp_of_find_icc_paths into probe [Viresh]

V5:
* Pick up R-bs from Amit
* Drop icc tag support/dt changes till the a consensus is achieved
* Use dev_pm_opp_adjust_voltage instead [Viresh]
* Drop dev_pm_opp_get_path_count [Saravana]
* Rework dev_pm_opp_set_bw

V4:
* Migrate to using Georgi's new bindings
* Misc fixups based on Matthias comments
* API fixups based on Bjorn's comments on v2
* Picked up a few R-bs from Matthias

v3:
* Migrated to using Saravana's opp-kBps bindings [1]
* Fixed some misc comments from Rajendra
* Added support for SC7180

v2:
* Incorporated Viresh's comments from:
https://lore.kernel.org/lkml/20190410102429.r6j6brm5kspmqxc3@vireshk-i7/
https://lore.kernel.org/lkml/20190410112516.gnh77jcwawvld6et@vireshk-i7/
* Dropped cpufreq-map passive governor

Sibi Sankar (5):
cpufreq: blacklist SDM845 in cpufreq-dt-platdev
cpufreq: blacklist SC7180 in cpufreq-dt-platdev
OPP: Add and export helper to set bandwidth
cpufreq: qcom: Update the bandwidth levels on frequency change
cpufreq: qcom: Disable fast switch when scaling DDR/L3

 drivers/cpufreq/cpufreq-dt-platdev.c |  2 +
 drivers/cpufreq/qcom-cpufreq-hw.c    | 86 ++++++++++++++++++++++++++--
 drivers/opp/core.c                   | 31 ++++++++++
 include/linux/pm_opp.h               |  6 ++
 4 files changed, 121 insertions(+), 4 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#1 lore raw

PATCH v6 1/5 cpufreq: blacklist SDM845 in cpufreq-dt-platdev

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

Reviewed-by Amit Kucheria <[email protected]>
Reviewed-by Matthias Kaehlcke <[email protected]>
Signed-off-by Sibi Sankar <[email protected]>

---

v6:
* No change

v5:
* Picked up R-b from Amit

v4:
* Updated commit message [Matthias]
* Picked up R-b from Matthias

 drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
 1 file changed, 1 insertion(+)
drivers/cpufreq/cpufreq-dt-platdev.c +1 -0
index e8e20fef400b0..be85eb494a6b3 100644
@@ -132,6 +132,7 @@ static const struct of_device_id blacklist[] __initconst = {
 	{ .compatible = "qcom,apq8096", },
 	{ .compatible = "qcom,msm8996", },
 	{ .compatible = "qcom,qcs404", },
+	{ .compatible = "qcom,sdm845", },
 
 	{ .compatible = "st,stih407", },
 	{ .compatible = "st,stih410", },
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#2 lore raw

PATCH v6 2/5 cpufreq: blacklist SC7180 in cpufreq-dt-platdev

In reply to Sibi Sankar

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

Reviewed-by Amit Kucheria <[email protected]>
Reviewed-by Matthias Kaehlcke <[email protected]>
Signed-off-by Sibi Sankar <[email protected]>

---

v6:
* No change

v5:
* Picked up R-b from Amit

v4:
* Updated commit message [Matthias]
* Picked up R-b from Matthias

 drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
 1 file changed, 1 insertion(+)
drivers/cpufreq/cpufreq-dt-platdev.c +1 -0
index be85eb494a6b3..7d01df7bfa6cd 100644
@@ -132,6 +132,7 @@ static const struct of_device_id blacklist[] __initconst = {
 	{ .compatible = "qcom,apq8096", },
 	{ .compatible = "qcom,msm8996", },
 	{ .compatible = "qcom,qcs404", },
+	{ .compatible = "qcom,sc7180", },
 	{ .compatible = "qcom,sdm845", },
 
 	{ .compatible = "st,stih407", },
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#3 lore raw

PATCH v6 3/5 OPP: Add and export helper to set bandwidth

In reply to Sibi Sankar

Add and export 'dev_pm_opp_set_bw' to set the bandwidth levels associated with an OPP.

Signed-off-by Sibi Sankar <[email protected]>

---

v6:
* Pass NULL to _set_opp_bw

v5:
https://lkml.org/lkml/2020/5/27/7
* Rework the patch based on ^^

v4:
https://patchwork.kernel.org/patch/11019737/
* Pass device opp to set bw levels [Bjorn]

 drivers/opp/core.c     | 31 +++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 37 insertions(+)
drivers/opp/core.c +31 -0
index dfbd3d10410ca..6937bf45f497f 100644
@@ -831,6 +831,37 @@ static int _set_required_opps(struct device *dev,
 	return ret;
 }
 
+/**
+ * dev_pm_opp_set_bw() - sets bandwidth levels corresponding to an opp
+ * @dev:	device for which we do this operation
+ * @opp:	opp based on which the bandwidth levels are to be configured
+ *
+ * This configures the bandwidth to the levels specified by the OPP. However
+ * if the OPP specified is NULL the bandwidth levels are cleared out.
+ *
+ * Return: 0 on success or a negative error value.
+ */
+int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
+{
+	struct opp_table *opp_table;
+	int ret;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		dev_err(dev, "%s: device opp table doesn't exist\n", __func__);
+		return PTR_ERR(opp_table);
+	}
+
+	if (opp)
+		ret = _set_opp_bw(opp_table, opp, dev, false);
+	else
+		ret = _set_opp_bw(opp_table, NULL, dev, true);
+
+	dev_pm_opp_put_opp_table(opp_table);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
+
 /**
  * dev_pm_opp_set_rate() - Configure new OPP based on frequency
  * @dev:	 device for which we do this operation
include/linux/pm_opp.h +6 -0
index d5c4a329321dd..ae68417c0ae00 100644
@@ -151,6 +151,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names
 void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
+int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
 void dev_pm_opp_remove_table(struct device *dev);

@@ -342,6 +343,11 @@ static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_f
 	return -ENOTSUPP;
 }
 
+static inline int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask)
 {
 	return -ENOTSUPP;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#5 lore raw

PATCH v6 4/5 cpufreq: qcom: Update the bandwidth levels on frequency change

In reply to Sibi Sankar

Add support to parse optional OPP table attached to the cpu node when the OPP bandwidth values are populated. This allows for scaling of DDR/L3 bandwidth levels with frequency change.

Signed-off-by Sibi Sankar <[email protected]>

---

v6:
* Add global flag to distinguish between voltage update and opp add. Use the same flag before trying to scale ddr/l3 bw [Viresh]
* Use dev_pm_opp_find_freq_ceil to grab all opps [Viresh]
* Move dev_pm_opp_of_find_icc_paths into probe [Viresh]

v5:
* Use dev_pm_opp_adjust_voltage instead [Viresh]
* Misc cleanup

v4:
* Split fast switch disable into another patch [Lukasz]

 drivers/cpufreq/qcom-cpufreq-hw.c | 82 ++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 2 deletions(-)
drivers/cpufreq/qcom-cpufreq-hw.c +80 -2
index fc92a8842e252..8fa6ab6e0e4b6 100644
@@ -6,6 +6,7 @@
 #include <linux/bitfield.h>
 #include <linux/cpufreq.h>
 #include <linux/init.h>
+#include <linux/interconnect.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_address.h>

@@ -30,6 +31,48 @@
 
 static unsigned long cpu_hw_rate, xo_rate;
 static struct platform_device *global_pdev;
+static bool icc_scaling_enabled;
+
+static int qcom_cpufreq_set_bw(struct cpufreq_policy *policy,
+			       unsigned long freq_khz)
+{
+	unsigned long freq_hz = freq_khz * 1000;
+	struct dev_pm_opp *opp;
+	struct device *dev;
+	int ret;
+
+	dev = get_cpu_device(policy->cpu);
+	if (!dev)
+		return -ENODEV;
+
+	opp = dev_pm_opp_find_freq_exact(dev, freq_hz, true);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+
+	ret = dev_pm_opp_set_bw(dev, opp);
+	dev_pm_opp_put(opp);
+	return ret;
+}
+
+static int qcom_cpufreq_update_opp(struct device *cpu_dev,
+				   unsigned long freq_khz,
+				   unsigned long volt)
+{
+	unsigned long freq_hz = freq_khz * 1000;
+	int ret;
+
+	/* Skip voltage update if the opp table is not available */
+	if (!icc_scaling_enabled)
+		return dev_pm_opp_add(cpu_dev, freq_hz, volt);
+
+	ret = dev_pm_opp_adjust_voltage(cpu_dev, freq_hz, volt, volt, volt);
+	if (ret) {
+		dev_err(cpu_dev, "Voltage update failed freq=%ld\n", freq_khz);
+		return ret;
+	}
+
+	return dev_pm_opp_enable(cpu_dev, freq_hz);
+}
 
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 					unsigned int index)

@@ -39,6 +82,9 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 
 	writel_relaxed(index, perf_state_reg);
 
+	if (icc_scaling_enabled)
+		qcom_cpufreq_set_bw(policy, freq);
+
 	arch_set_freq_scale(policy->related_cpus, freq,
 			    policy->cpuinfo.max_freq);
 	return 0;

@@ -89,11 +135,31 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 	u32 data, src, lval, i, core_count, prev_freq = 0, freq;
 	u32 volt;
 	struct cpufreq_frequency_table	*table;
+	struct dev_pm_opp *opp;
+	unsigned long rate;
+	int ret;
 
 	table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
 	if (!table)
 		return -ENOMEM;
 
+	ret = dev_pm_opp_of_add_table(cpu_dev);
+	if (!ret) {
+		/* Disable all opps and cross-validate against LUT */
+		icc_scaling_enabled = true;
+		for (rate = 0; ; rate++) {
+			opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
+			if (IS_ERR(opp))
+				break;
+
+			dev_pm_opp_put(opp);
+			dev_pm_opp_disable(cpu_dev, rate);
+		}
+	} else if (ret != -ENODEV) {
+		dev_err(cpu_dev, "Invalid opp table in device tree\n");
+		return ret;
+	}
+
 	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
 		data = readl_relaxed(base + REG_FREQ_LUT +
 				      i * LUT_ROW_SIZE);

@@ -112,7 +178,7 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 
 		if (freq != prev_freq && core_count != LUT_TURBO_IND) {
 			table[i].frequency = freq;
-			dev_pm_opp_add(cpu_dev, freq * 1000, volt);
+			qcom_cpufreq_update_opp(cpu_dev, freq, volt);
 			dev_dbg(cpu_dev, "index=%d freq=%d, core_count %d\n", i,
 				freq, core_count);
 		} else if (core_count == LUT_TURBO_IND) {

@@ -133,7 +199,8 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
 				prev->frequency = prev_freq;
 				prev->flags = CPUFREQ_BOOST_FREQ;
-				dev_pm_opp_add(cpu_dev,	prev_freq * 1000, volt);
+				qcom_cpufreq_update_opp(cpu_dev, prev_freq,
+							volt);
 			}
 
 			break;

@@ -254,6 +321,7 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
 	void __iomem *base = policy->driver_data - REG_PERF_STATE;
 
 	dev_pm_opp_remove_all_dynamic(cpu_dev);
+	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
 	kfree(policy->freq_table);
 	devm_iounmap(&global_pdev->dev, base);
 

@@ -282,6 +350,7 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
 
 static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 {
+	struct device *cpu_dev;
 	struct clk *clk;
 	int ret;
 

@@ -301,6 +370,15 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 
 	global_pdev = pdev;
 
+	/* Check for optional interconnect paths on CPU0 */
+	cpu_dev = get_cpu_device(0);
+	if (!cpu_dev)
+		return -EPROBE_DEFER;
+
+	ret = dev_pm_opp_of_find_icc_paths(cpu_dev, NULL);
+	if (ret)
+		return ret;
+
 	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
 	if (ret)
 		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#6 lore raw

Re: [PATCH v6 4/5] cpufreq: qcom: Update the bandwidth levels on frequency change

Hi Sibi,

Quoted 36 lines · Sibi Sankar

Add support to parse optional OPP table attached to the cpu node when the OPP bandwidth values are populated. This allows for scaling of DDR/L3 bandwidth levels with frequency change.

Signed-off-by Sibi Sankar <[email protected]>

---

v6:
* Add global flag to distinguish between voltage update and opp add. Use the same flag before trying to scale ddr/l3 bw [Viresh]
* Use dev_pm_opp_find_freq_ceil to grab all opps [Viresh]
* Move dev_pm_opp_of_find_icc_paths into probe [Viresh]

v5:
* Use dev_pm_opp_adjust_voltage instead [Viresh]
* Misc cleanup

v4:
* Split fast switch disable into another patch [Lukasz]

 drivers/cpufreq/qcom-cpufreq-hw.c | 82 ++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 2 deletions(-)
drivers/cpufreq/qcom-cpufreq-hw.c +1 -0
index fc92a8842e252..8fa6ab6e0e4b6 100644
@@ -6,6 +6,7 @@
 #include <linux/bitfield.h>
 #include <linux/cpufreq.h>
 #include <linux/init.h>
+#include <linux/interconnect.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_address.h>

drivers/cpufreq/qcom-cpufreq-hw.c line 34

Quoted 4 lines
drivers/cpufreq/qcom-cpufreq-hw.c +1 -0
@@ -30,6 +31,48 @@
 
 static unsigned long cpu_hw_rate, xo_rate;
 static struct platform_device *global_pdev;
+static bool icc_scaling_enabled;

It seem you rely on 'icc_scaling_enabled' to be initialized to 'false'. This works during the first initialization, but not if the 'device' is unbound/rebound. In theory things shouldn't be different in a succesive initialization, however for robustness the variable should be explicitly set to 'false' somewhere in the code path (_probe(), _read_lut(), ...).

Quoted 58 lines
drivers/cpufreq/qcom-cpufreq-hw.c +44 -0
+static int qcom_cpufreq_set_bw(struct cpufreq_policy *policy,
+			       unsigned long freq_khz)
+{
+	unsigned long freq_hz = freq_khz * 1000;
+	struct dev_pm_opp *opp;
+	struct device *dev;
+	int ret;
+
+	dev = get_cpu_device(policy->cpu);
+	if (!dev)
+		return -ENODEV;
+
+	opp = dev_pm_opp_find_freq_exact(dev, freq_hz, true);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+
+	ret = dev_pm_opp_set_bw(dev, opp);
+	dev_pm_opp_put(opp);
+	return ret;
+}
+
+static int qcom_cpufreq_update_opp(struct device *cpu_dev,
+				   unsigned long freq_khz,
+				   unsigned long volt)
+{
+	unsigned long freq_hz = freq_khz * 1000;
+	int ret;
+
+	/* Skip voltage update if the opp table is not available */
+	if (!icc_scaling_enabled)
+		return dev_pm_opp_add(cpu_dev, freq_hz, volt);
+
+	ret = dev_pm_opp_adjust_voltage(cpu_dev, freq_hz, volt, volt, volt);
+	if (ret) {
+		dev_err(cpu_dev, "Voltage update failed freq=%ld\n", freq_khz);
+		return ret;
+	}
+
+	return dev_pm_opp_enable(cpu_dev, freq_hz);
+}
 
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 					unsigned int index)

@@ -39,6 +82,9 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 
 	writel_relaxed(index, perf_state_reg);
 
+	if (icc_scaling_enabled)
+		qcom_cpufreq_set_bw(policy, freq);
+
 	arch_set_freq_scale(policy->related_cpus, freq,
 			    policy->cpuinfo.max_freq);
 	return 0;

@@ -89,11 +135,31 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 	u32 data, src, lval, i, core_count, prev_freq = 0, freq;
 	u32 volt;
 	struct cpufreq_frequency_table	*table;
+	struct dev_pm_opp *opp;

drivers/cpufreq/qcom-cpufreq-hw.c static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev, · line 148

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c +5 -0
+	unsigned long rate;
+	int ret;
 
 	table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
 	if (!table)
 		return -ENOMEM;
 
+	ret = dev_pm_opp_of_add_table(cpu_dev);
+	if (!ret) {
+		/* Disable all opps and cross-validate against LUT */

nit: IIUC the cross-validation doesn't happen in this branch, so the comment is a bit misleading. Maybe change it to "Disable all opps to cross-validate against the LUT {below,later}".

Quoted 18 lines
drivers/cpufreq/qcom-cpufreq-hw.c +14 -0
+		icc_scaling_enabled = true;
+		for (rate = 0; ; rate++) {
+			opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
+			if (IS_ERR(opp))
+				break;
+
+			dev_pm_opp_put(opp);
+			dev_pm_opp_disable(cpu_dev, rate);
+		}
+	} else if (ret != -ENODEV) {
+		dev_err(cpu_dev, "Invalid opp table in device tree\n");
+		return ret;
+	}
+
 	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
 		data = readl_relaxed(base + REG_FREQ_LUT +
 				      i * LUT_ROW_SIZE);

drivers/cpufreq/qcom-cpufreq-hw.c static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev, · line 181

Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c +1 -1
@@ -112,7 +178,7 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 
 		if (freq != prev_freq && core_count != LUT_TURBO_IND) {
 			table[i].frequency = freq;
-			dev_pm_opp_add(cpu_dev, freq * 1000, volt);
+			qcom_cpufreq_update_opp(cpu_dev, freq, volt);

This is the cross-validation mentioned above, right? Shouldn't it include a check of the return value?

drivers/cpufreq/qcom-cpufreq-hw.c static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev, · line 203

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c +2 -1
 			dev_dbg(cpu_dev, "index=%d freq=%d, core_count %d\n", i,
 				freq, core_count);
 		} else if (core_count == LUT_TURBO_IND) {

@@ -133,7 +199,8 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
 				prev->frequency = prev_freq;
 				prev->flags = CPUFREQ_BOOST_FREQ;
-				dev_pm_opp_add(cpu_dev,	prev_freq * 1000, volt);
+				qcom_cpufreq_update_opp(cpu_dev, prev_freq,
+							volt);

ditto

nit: with the updated max line length it isn't necessary anymore to break
this into multiple lines (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?h=v5.8-rc1#n54),
though the coding style still has the old limit.

author
#7 lore raw

Re: [PATCH v6 4/5] cpufreq: qcom: Update the bandwidth levels on frequency change

Hey Matthias,
Thanks for taking time to review
the series.

Quoted 41 lines · Matthias Kaehlcke

Hi Sibi,

Quoted 38 lines · Sibi Sankar

Add support to parse optional OPP table attached to the cpu node when the OPP bandwidth values are populated. This allows for scaling of DDR/L3 bandwidth levels with frequency change.

Signed-off-by Sibi Sankar <[email protected]>

---

v6:
* Add global flag to distinguish between voltage update and opp add. Use the same flag before trying to scale ddr/l3 bw [Viresh]
* Use dev_pm_opp_find_freq_ceil to grab all opps [Viresh]
* Move dev_pm_opp_of_find_icc_paths into probe [Viresh]

v5:
* Use dev_pm_opp_adjust_voltage instead [Viresh]
* Misc cleanup

v4:
* Split fast switch disable into another patch [Lukasz]

 drivers/cpufreq/qcom-cpufreq-hw.c | 82 
++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 2 deletions(-)
drivers/cpufreq/qcom-cpufreq-hw.c +1 -0
index fc92a8842e252..8fa6ab6e0e4b6 100644
@@ -6,6 +6,7 @@
 #include <linux/bitfield.h>
 #include <linux/cpufreq.h>
 #include <linux/init.h>
+#include <linux/interconnect.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_address.h>

@@ -30,6 +31,48 @@
Quoted 7 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 34

Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c +1 -0
 static unsigned long cpu_hw_rate, xo_rate;
 static struct platform_device *global_pdev;
+static bool icc_scaling_enabled;

It seem you rely on 'icc_scaling_enabled' to be initialized to 'false'. This works during the first initialization, but not if the 'device' is unbound/rebound. In theory things shouldn't be different in a succesive

yes it shouldn't but sure I'll set
it to false along the way.

Quoted 72 lines

initialization, however for robustness the variable should be explicitly set to 'false' somewhere in the code path (_probe(), _read_lut(), ...).

Quoted 68 lines
drivers/cpufreq/qcom-cpufreq-hw.c +43 -0
+static int qcom_cpufreq_set_bw(struct cpufreq_policy *policy,
+			       unsigned long freq_khz)
+{
+	unsigned long freq_hz = freq_khz * 1000;
+	struct dev_pm_opp *opp;
+	struct device *dev;
+	int ret;
+
+	dev = get_cpu_device(policy->cpu);
+	if (!dev)
+		return -ENODEV;
+
+	opp = dev_pm_opp_find_freq_exact(dev, freq_hz, true);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+
+	ret = dev_pm_opp_set_bw(dev, opp);
+	dev_pm_opp_put(opp);
+	return ret;
+}
+
+static int qcom_cpufreq_update_opp(struct device *cpu_dev,
+				   unsigned long freq_khz,
+				   unsigned long volt)
+{
+	unsigned long freq_hz = freq_khz * 1000;
+	int ret;
+
+	/* Skip voltage update if the opp table is not available */
+	if (!icc_scaling_enabled)
+		return dev_pm_opp_add(cpu_dev, freq_hz, volt);
+
+	ret = dev_pm_opp_adjust_voltage(cpu_dev, freq_hz, volt, volt, volt);
+	if (ret) {
+		dev_err(cpu_dev, "Voltage update failed freq=%ld\n", freq_khz);
+		return ret;
+	}
+
+	return dev_pm_opp_enable(cpu_dev, freq_hz);
+}
 
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy 
 *policy,
 					unsigned int index)

@@ -39,6 +82,9 @@ static int qcom_cpufreq_hw_target_index(struct 
 cpufreq_policy *policy,
 
 	writel_relaxed(index, perf_state_reg);
 
+	if (icc_scaling_enabled)
+		qcom_cpufreq_set_bw(policy, freq);
+
 	arch_set_freq_scale(policy->related_cpus, freq,
 			    policy->cpuinfo.max_freq);
drivers/cpufreq/qcom-cpufreq-hw.c +3 -0
 	return 0;

@@ -89,11 +135,31 @@ static int qcom_cpufreq_hw_read_lut(struct device 
 *cpu_dev,
 	u32 data, src, lval, i, core_count, prev_freq = 0, freq;
 	u32 volt;
 	struct cpufreq_frequency_table	*table;
+	struct dev_pm_opp *opp;
+	unsigned long rate;
+	int ret;
 
 	table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
 	if (!table)
 		return -ENOMEM;
Quoted 7 lines

drivers/cpufreq/qcom-cpufreq-hw.c static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev, · line 148

Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c +3 -0
+	ret = dev_pm_opp_of_add_table(cpu_dev);
+	if (!ret) {
+		/* Disable all opps and cross-validate against LUT */

nit: IIUC the cross-validation doesn't happen in this branch, so the comment is a bit misleading. Maybe change it to "Disable all opps to cross-validate against the LUT {below,later}".

sure will re-word it.

Quoted 21 lines
Quoted 21 lines
drivers/cpufreq/qcom-cpufreq-hw.c +14 -0
+		icc_scaling_enabled = true;
+		for (rate = 0; ; rate++) {
+			opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
+			if (IS_ERR(opp))
+				break;
+
+			dev_pm_opp_put(opp);
+			dev_pm_opp_disable(cpu_dev, rate);
+		}
+	} else if (ret != -ENODEV) {
+		dev_err(cpu_dev, "Invalid opp table in device tree\n");
+		return ret;
+	}
+
 	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
 		data = readl_relaxed(base + REG_FREQ_LUT +
drivers/cpufreq/qcom-cpufreq-hw.c +0 -0
 				      i * LUT_ROW_SIZE);

@@ -112,7 +178,7 @@ static int qcom_cpufreq_hw_read_lut(struct device 
 *cpu_dev,
Quoted 7 lines

drivers/cpufreq/qcom-cpufreq-hw.c static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev, · line 181

Quoted 4 lines
drivers/cpufreq/qcom-cpufreq-hw.c +1 -1
 		if (freq != prev_freq && core_count != LUT_TURBO_IND) {
 			table[i].frequency = freq;
-			dev_pm_opp_add(cpu_dev, freq * 1000, volt);
+			qcom_cpufreq_update_opp(cpu_dev, freq, volt);

This is the cross-validation mentioned above, right? Shouldn't it include a check of the return value?

Yes, this is the cross-validation step,
we adjust the voltage if opp-tables are
present/added successfully and enable
them, else we would just do a add opp.
We don't want to exit early on a single
opp failure. We will error out a bit
later if the opp-count ends up to be
zero.

Quoted 15 lines

drivers/cpufreq/qcom-cpufreq-hw.c static int qcom_cpufreq_hw_read_lut(struct device · line 204

Quoted 12 lines
drivers/cpufreq/qcom-cpufreq-hw.c +0 -0
 			dev_dbg(cpu_dev, "index=%d freq=%d, core_count %d\n", i,
 				freq, core_count);
drivers/cpufreq/qcom-cpufreq-hw.c +2 -1
 		} else if (core_count == LUT_TURBO_IND) {

@@ -133,7 +199,8 @@ static int qcom_cpufreq_hw_read_lut(struct device 
 *cpu_dev,
 			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
 				prev->frequency = prev_freq;
 				prev->flags = CPUFREQ_BOOST_FREQ;
-				dev_pm_opp_add(cpu_dev,	prev_freq * 1000, volt);
+				qcom_cpufreq_update_opp(cpu_dev, prev_freq,
+							volt);

ditto

Quoted 4 lines

nit: with the updated max line length it isn't necessary anymore to break this into multiple lines
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?h=v5.8-rc1#n54),
though the coding style still has the old limit.

yeah I'll expand it.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
#8 lore raw

Re: [PATCH v6 4/5] cpufreq: qcom: Update the bandwidth levels on frequency change

Hi Sibi,

after doing the review I noticed that Viresh replied on the cover letter that he picked the series up for v5.9, so I'm not sure if it makes sense to send a v7.

Quoted 12 lines · Sibi Sankar
Quoted 11 lines

drivers/cpufreq/qcom-cpufreq-hw.c static int qcom_cpufreq_hw_read_lut(struct · line 182

Quoted 7 lines
drivers/cpufreq/qcom-cpufreq-hw.c +1 -1
@@ -112,7 +178,7 @@ static int qcom_cpufreq_hw_read_lut(struct
 device *cpu_dev,
 
 		if (freq != prev_freq && core_count != LUT_TURBO_IND) {
 			table[i].frequency = freq;
-			dev_pm_opp_add(cpu_dev, freq * 1000, volt);
+			qcom_cpufreq_update_opp(cpu_dev, freq, volt);

This is the cross-validation mentioned above, right? Shouldn't it include
a check of the return value?

Quoted 8 lines

Yes, this is the cross-validation step, we adjust the voltage if opp-tables are present/added successfully and enable them, else we would just do a add opp. We don't want to exit early on a single opp failure. We will error out a bit later if the opp-count ends up to be zero.

At least an error/warning message would seem convenient when adjusting/adding an OPP fails, otherwise you would only notice by looking at the sysfs attributes (if you'd even spot a single/few OPPs to be missing).

maintainer
#9 lore raw

Re: [PATCH v6 4/5] cpufreq: qcom: Update the bandwidth levels on frequency change

Quoted 5 lines · Matthias Kaehlcke

Hi Sibi,

after doing the review I noticed that Viresh replied on the cover letter that he picked the series up for v5.9, so I'm not sure if it makes sense to send a v7.

Its okay, you can send a new version and I will apply that instead.

Signature · 1 line
viresh
author
#10 lore raw

Re: [PATCH v6 4/5] cpufreq: qcom: Update the bandwidth levels on frequency change

In reply to Matthias Kaehlcke
Quoted 29 lines · Matthias Kaehlcke

Hi Sibi,

after doing the review I noticed that Viresh replied on the cover letter that he picked the series up for v5.9, so I'm not sure if it makes sense to send a v7.

Quoted 12 lines · Sibi Sankar
Quoted 11 lines

drivers/cpufreq/qcom-cpufreq-hw.c static int qcom_cpufreq_hw_read_lut(struct · line 182

Quoted 7 lines
drivers/cpufreq/qcom-cpufreq-hw.c +1 -1
@@ -112,7 +178,7 @@ static int qcom_cpufreq_hw_read_lut(struct
 device *cpu_dev,
 
 		if (freq != prev_freq && core_count != LUT_TURBO_IND) {
 			table[i].frequency = freq;
-			dev_pm_opp_add(cpu_dev, freq * 1000, volt);
+			qcom_cpufreq_update_opp(cpu_dev, freq, volt);

This is the cross-validation mentioned above, right? Shouldn't it include
a check of the return value?

Quoted 8 lines

Yes, this is the cross-validation step, we adjust the voltage if opp-tables are present/added successfully and enable them, else we would just do a add opp. We don't want to exit early on a single opp failure. We will error out a bit later if the opp-count ends up to be zero.

Quoted 3 lines

At least an error/warning message would seem convenient when adjusting/adding an OPP fails, otherwise you would only notice by looking at the sysfs attributes (if you'd even spot a single/few OPPs to be missing).

I did consider the case where adjust
voltage fails and we do report the
freq for which it fails for as well.
If adding a OPP fails we will still
it being listed in the sysfs cpufreq
scaling_available_frequencies since
it lists the freq_table in khz there
instead.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
#11 lore raw

Re: [PATCH v6 4/5] cpufreq: qcom: Update the bandwidth levels on frequency change

Quoted 35 lines · Sibi Sankar
Quoted 29 lines · Matthias Kaehlcke

Hi Sibi,

after doing the review I noticed that Viresh replied on the cover letter that he picked the series up for v5.9, so I'm not sure if it makes sense to send a v7.

Quoted 12 lines · Sibi Sankar
Quoted 11 lines

drivers/cpufreq/qcom-cpufreq-hw.c static int qcom_cpufreq_hw_read_lut(struct · line 182

Quoted 7 lines
drivers/cpufreq/qcom-cpufreq-hw.c +1 -1
@@ -112,7 +178,7 @@ static int qcom_cpufreq_hw_read_lut(struct
 device *cpu_dev,
 
 		if (freq != prev_freq && core_count != LUT_TURBO_IND) {
 			table[i].frequency = freq;
-			dev_pm_opp_add(cpu_dev, freq * 1000, volt);
+			qcom_cpufreq_update_opp(cpu_dev, freq, volt);

This is the cross-validation mentioned above, right? Shouldn't it include
a check of the return value?

Quoted 8 lines

Yes, this is the cross-validation step, we adjust the voltage if opp-tables are present/added successfully and enable them, else we would just do a add opp. We don't want to exit early on a single opp failure. We will error out a bit later if the opp-count ends up to be zero.

Quoted 4 lines

At least an error/warning message would seem convenient when adjusting/adding
an OPP fails, otherwise you would only notice by looking at the sysfs attributes (if you'd even spot a single/few OPPs to be missing).

Quoted 8 lines

I did consider the case where adjust voltage fails and we do report the freq for which it fails for as well. If adding a OPP fails we will still it being listed in the sysfs cpufreq scaling_available_frequencies since it lists the freq_table in khz there instead.

Ah, right, I missed that v6 added the error log to qcom_cpufreq_update_opp(), please ignore my comment :)

author
#12 lore raw

PATCH v6 5/5 cpufreq: qcom: Disable fast switch when scaling DDR/L3

In reply to Sibi Sankar

Disable fast switch when the opp-tables required for scaling DDR/L3 are populated.

Signed-off-by Sibi Sankar <[email protected]>

---

v6:
* No change

v5:
* Drop dev_pm_opp_get_path_count [Saravana]

 drivers/cpufreq/qcom-cpufreq-hw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/cpufreq/qcom-cpufreq-hw.c +2 -2
index 8fa6ab6e0e4b6..56f01049fd3a3 100644
@@ -158,6 +158,8 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 	} else if (ret != -ENODEV) {
 		dev_err(cpu_dev, "Invalid opp table in device tree\n");
 		return ret;
+	} else {
+		policy->fast_switch_possible = true;
 	}
 
 	for (i = 0; i < LUT_MAX_ENTRIES; i++) {

@@ -307,8 +309,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 
 	dev_pm_opp_of_register_em(policy->cpus);
 
-	policy->fast_switch_possible = true;
-
 	return 0;
 error:
 	devm_iounmap(dev, base);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
maintainer
#14 lore raw
In reply to Sibi Sankar
Quoted 6 lines · Sibi Sankar

This patch series aims to extend cpu based scaling support to L3/DDR on SDM845 and SC7180 SoCs.

Patches [1-2] - Blacklist SDM845 and SC7180 in cpufreq-dt-platdev Patches [3-5] - Update bw levels based on cpu frequency change

Quoted 8 lines

Based on Viresh's opp-next:
https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=opp/linux-next

V6:
* Add global flag to distinguish between voltage update and opp add. Use the same flag before trying to scale ddr/l3 bw [Viresh]
* Use dev_pm_opp_find_freq_ceil to grab all opps [Viresh]
* Move dev_pm_opp_of_find_icc_paths into probe [Viresh]

Picked for 5.9, will push to my branch after rc1 is out.

Signature · 1 line
viresh