Re: [PATCH v4 06/12] cpufreq: qcom: Update the bandwidth levels on frequency change
On 27-05-20, 09:23, Viresh Kumar wrote:
> On 26-05-20, 23:18, Sibi Sankar wrote:
> > https://patchwork.kernel.org/cover/11548479/
> > GPU driver uses Georgi's series
> > for scaling and will need a way to
> > remove the icc votes in the suspend
> > path, (this looks like a pattern
> > that might be used by other clients
> > as well) I could probably update
> > opp_set_bw to support removing bw
> > when NULL opp is specified. Similarly
> > opp_set_rate will need to support
> > set bw to 0 when set_rate is passed
> > 0 as target freq for the same use case.
>
> Sure, please send a patch for that.
On a second thought, here is the patch. Please test it.
-8<-
Subject: [PATCH] opp: Remove bandwidth votes when target_freq is zero
We already drop several votes when target_freq is set to zero, drop
bandwidth votes as well.
Reported-by: Sibi Sankar
Signed-off-by: Viresh Kumar
---
drivers/opp/core.c | 47 +++---
1 file changed, 36 insertions(+), 11 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 56d3022c1ca2..0c259d5ed232 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -725,6 +725,34 @@ static int _generic_set_opp_regulator(struct opp_table
*opp_table,
return ret;
}
+static int _set_opp_bw(const struct opp_table *opp_table,
+ struct dev_pm_opp *opp, bool remove)
+{
+ u32 avg, peak;
+ int i, ret;
+
+ if (!opp_table->paths)
+ return 0;
+
+ for (i = 0; i < opp_table->path_count; i++) {
+ if (remove) {
+ avg = 0;
+ peak = 0;
+ } else {
+ avg = opp->bandwidth[i].avg;
+ peak = opp->bandwidth[i].peak;
+ }
+ ret = icc_set_bw(opp_table->paths[i], avg, peak);
+ if (ret) {
+ dev_err(dev, "Failed to %s bandwidth[%d]: %d\n",
+ remove ? "remove" : "set", i, ret);
+ retrun ret;
+ }
+ }
+
+ return 0;
+}
+
static int _set_opp_custom(const struct opp_table *opp_table,
struct device *dev, unsigned long old_freq,
unsigned long freq,
@@ -837,12 +865,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long
target_freq)
if (!_get_opp_count(opp_table))
return 0;
- if (!opp_table->required_opp_tables && !opp_table->regulators) {
+ if (!opp_table->required_opp_tables && !opp_table->regulators &&
+ !opp_table->paths) {
dev_err(dev, "target frequency can't be 0\n");
ret = -EINVAL;
goto put_opp_table;
}
+ ret = _set_opp_bw(opp_table, opp, true);
+ if (ret)
+ return ret;
+
if (opp_table->regulator_enabled) {
regulator_disable(opp_table->regulators[0]);
opp_table->regulator_enabled = false;
@@ -932,16 +965,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long
target_freq)
dev_err(dev, "Failed to set required opps: %d\n", ret);
}
- if (!ret && opp_table->paths) {
- for (i = 0; i < opp_table->path_count; i++) {
- ret = icc_set_bw(opp_table->paths[i],
-opp->bandwidth[i].avg,
-opp->bandwidth[i].peak);
- if (ret)
- dev_err(dev, "Failed to set bandwidth[%d]:
%d\n",
- i, ret);
- }
- }
+ if (!ret)
+ ret = _set_opp_bw(opp_table, opp, false);
put_opp:
dev_pm_opp_put(opp);
Re: [PATCH v4 06/12] cpufreq: qcom: Update the bandwidth levels on frequency change
On 26-05-20, 23:18, Sibi Sankar wrote: > https://patchwork.kernel.org/cover/11548479/ > GPU driver uses Georgi's series > for scaling and will need a way to > remove the icc votes in the suspend > path, (this looks like a pattern > that might be used by other clients > as well) I could probably update > opp_set_bw to support removing bw > when NULL opp is specified. Similarly > opp_set_rate will need to support > set bw to 0 when set_rate is passed > 0 as target freq for the same use case. Sure, please send a patch for that. -- viresh
Re: [PATCH v4 06/12] cpufreq: qcom: Update the bandwidth levels on frequency change
On 2020-05-05 12:49, Sibi Sankar wrote: On 2020-05-05 10:20, Viresh Kumar wrote: On 05-05-20, 01:52, Sibi Sankar wrote: 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 What about using opp_set_rate instead ? I can't use opp_set_rate since the cpu dev does not have a clock associated with it and the scaling is done through writing on perf state register. Viresh, https://patchwork.kernel.org/cover/11548479/ GPU driver uses Georgi's series for scaling and will need a way to remove the icc votes in the suspend path, (this looks like a pattern that might be used by other clients as well) I could probably update opp_set_bw to support removing bw when NULL opp is specified. Similarly opp_set_rate will need to support set bw to 0 when set_rate is passed 0 as target freq for the same use case. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v4 06/12] cpufreq: qcom: Update the bandwidth levels on frequency change
On 2020-05-05 10:20, Viresh Kumar wrote: On 05-05-20, 01:52, Sibi Sankar wrote: 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 What about using opp_set_rate instead ? I can't use opp_set_rate since the cpu dev does not have a clock associated with it and the scaling is done through writing on perf state register. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v4 06/12] cpufreq: qcom: Update the bandwidth levels on frequency change
On 05-05-20, 01:52, Sibi Sankar wrote: > 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 What about using opp_set_rate instead ? -- viresh
[PATCH v4 06/12] cpufreq: qcom: Update the bandwidth levels on frequency change
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
---
v4:
* Split fast switch disable into another patch [Lukasz]
drivers/cpufreq/qcom-cpufreq-hw.c | 85 ++-
1 file changed, 83 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c
b/drivers/cpufreq/qcom-cpufreq-hw.c
index fc92a8842e252..4fb489b69bc61 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -31,6 +32,63 @@
static unsigned long cpu_hw_rate, xo_rate;
static struct platform_device *global_pdev;
+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;
+
+ if (dev_pm_opp_update_voltage(cpu_dev, freq_hz, volt))
+ return dev_pm_opp_add(cpu_dev, freq_hz, volt);
+
+ /* Enable the opp after voltage update*/
+ return dev_pm_opp_enable(cpu_dev, freq_hz);
+}
+
+/* Check for optional interconnect paths on CPU0 */
+static int qcom_cpufreq_verify_icc_paths(struct device *dev)
+{
+ struct device *cpu_dev;
+ struct icc_path *path;
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev)
+ return -EPROBE_DEFER;
+
+ path = of_icc_get(cpu_dev, NULL);
+ ret = PTR_ERR_OR_ZERO(path);
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(cpu_dev, "Failed to get paths ddr/l3 scaling
off\n");
+ return ret;
+ }
+
+ icc_put(path);
+ return ret;
+}
+
static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
unsigned int index)
{
@@ -39,6 +97,8 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy
*policy,
writel_relaxed(index, perf_state_reg);
+ qcom_cpufreq_set_bw(policy, freq);
+
arch_set_freq_scale(policy->related_cpus, freq,
policy->cpuinfo.max_freq);
return 0;
@@ -88,12 +148,27 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
{
u32 data, src, lval, i, core_count, prev_freq = 0, freq;
u32 volt;
+ u64 rate;
struct cpufreq_frequency_table *table;
+ struct device_node *opp_table_np, *np;
+ 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 */
+ opp_table_np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
+ for_each_available_child_of_node(opp_table_np, np) {
+ ret = of_property_read_u64(np, "opp-hz", );
+ if (!ret)
+ dev_pm_opp_disable(cpu_dev, rate);
+ }
+ of_node_put(opp_table_np);
+ }
+
for (i = 0; i < LUT_MAX_ENTRIES; i++) {
data = readl_relaxed(base + REG_FREQ_LUT +
i * LUT_ROW_SIZE);
@@ -112,7 +187,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 +208,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,
+
