Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

From: Taniya Das
Date: Tue Dec 04 2018 - 22:37:13 EST

Hello Stephen, Viresh

Thanks for the code and suggestions.

Having a NR_DOMAINS '2' makes the driver not scalable for re-use.
This assumption is only true for the current version of the HW and do not intend to update/clean-up this logic again. So want to stick keeping current logic of having the *qcom_freq_domain_map[NR_CPUS].

On 12/5/2018 4:35 AM, Stephen Boyd wrote:
Quoting Stephen Boyd (2018-12-04 14:28:11)
Quoting Viresh Kumar (2018-12-03 21:12:31)
Hi Taniya,

Sorry that I haven't been reviewing it much from last few iterations as I was
letting others get this into a better shape. Thanks for your efforts..

On 02-12-18, 09:25, Taniya Das wrote:
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c

+struct cpufreq_qcom {
+ struct cpufreq_frequency_table *table;
+ void __iomem *perf_state_reg;
+ cpumask_t related_cpus;
+};
+
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];

Now that the code is much more simplified, I am not sure if you need this
per-cpu structure at all. The only place where you are using it is in
qcom_cpufreq_hw_cpu_init() and probe(). Why not merge qcom_cpu_resources_init()
completely into qcom_cpufreq_hw_cpu_init() and get rid of this structure
entirely ?


Good point. Here's an untested patch to handle that. It removes the
related functionality and makes an array of pointers to the domains that
are created each time qcom_cpu_resources_init() is called.

---8<----

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 8dc6b73c2f22..04e7cfd70316 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -23,14 +23,14 @@
#define REG_LUT_TABLE 0x110
#define REG_PERF_STATE 0x920
+#define MAX_FREQ_DOMAINS 2
+
struct cpufreq_qcom {
struct cpufreq_frequency_table *table;
void __iomem *perf_state_reg;
cpumask_t related_cpus;
};
-static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
-
static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
unsigned int index)
{
@@ -76,9 +76,14 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
{
+ struct cpufreq_qcom **freq_domain_map;
struct cpufreq_qcom *c;
- c = qcom_freq_domain_map[policy->cpu];
+ freq_domain_map = cpufreq_get_driver_data();
+ if (!freq_domain_map)
+ return -ENODEV;
+
+ c = freq_domain_map[policy->cpu];

And this fails now because it indexes based on cpu number. We have to
parse the frequency domain out of the cpunode again to fix that.

Here's the updated (still untested) patch and I also just allocated the
freq domain array up front instead of in each domain init routine to
simplify some more.

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 8dc6b73c2f22..bc0d734f7e3c 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -23,14 +23,14 @@
#define REG_LUT_TABLE 0x110
#define REG_PERF_STATE 0x920
+#define MAX_FREQ_DOMAINS 2
+
struct cpufreq_qcom {
struct cpufreq_frequency_table *table;
void __iomem *perf_state_reg;
cpumask_t related_cpus;
};
-static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
-
static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
unsigned int index)
{
@@ -76,9 +76,26 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
{
- struct cpufreq_qcom *c;
+ struct cpufreq_qcom *freq_domain_map, *c;
+ struct device_node *cpu_np;
+ struct of_phandle_args args;
+ int ret;
+
+ freq_domain_map = cpufreq_get_driver_data();
+ if (!freq_domain_map)
+ return -ENODEV;
+
+ cpu_np = of_cpu_device_node_get(policy->cpu);
+ if (!cpu_np)
+ return -ENODEV;
+
+ ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
+ "#freq-domain-cells", 0, &args);
+ of_node_put(cpu_np);
+ if (ret)
+ return ret;
- c = qcom_freq_domain_map[policy->cpu];
+ c = &freq_domain_map[args.args[0]];
if (!c) {
pr_err("No scaling support for CPU%d\n", policy->cpu);
return -ENODEV;
@@ -171,43 +188,15 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
return 0;
}
-static void qcom_get_related_cpus(int index, struct cpumask *m)
-{
- struct device_node *cpu_np;
- struct of_phandle_args args;
- int cpu, ret;
-
- for_each_possible_cpu(cpu) {
- cpu_np = of_cpu_device_node_get(cpu);
- if (!cpu_np)
- continue;
-
- ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
- "#freq-domain-cells", 0,
- &args);
- of_node_put(cpu_np);
- if (ret < 0)
- continue;
-
- if (index == args.args[0])
- cpumask_set_cpu(cpu, m);
- }
-}
-
static int qcom_cpu_resources_init(struct platform_device *pdev,
- unsigned int cpu, int index,
- unsigned long xo_rate,
- unsigned long cpu_hw_rate)
+ int index, unsigned long xo_rate,
+ unsigned long cpu_hw_rate,
+ struct cpufreq_qcom *c)
{
- struct cpufreq_qcom *c;
struct resource *res;
struct device *dev = &pdev->dev;
void __iomem *base;
- int ret, cpu_r;
-
- c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
- if (!c)
- return -ENOMEM;
+ int ret;
res = platform_get_resource(pdev, IORESOURCE_MEM, index);
base = devm_ioremap_resource(dev, res);
@@ -220,12 +209,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
return -ENODEV;
}
- qcom_get_related_cpus(index, &c->related_cpus);
- if (!cpumask_weight(&c->related_cpus)) {
- dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
- return -ENOENT;
- }
-
c->perf_state_reg = base + REG_PERF_STATE;
ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
@@ -234,9 +217,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
return ret;
}
- for_each_cpu(cpu_r, &c->related_cpus)
- qcom_freq_domain_map[cpu_r] = c;
-
return 0;
}
@@ -245,9 +225,16 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
struct device_node *cpu_np;
struct of_phandle_args args;
struct clk *clk;
- unsigned int cpu;
+ unsigned int cpu, domain;
unsigned long xo_rate, cpu_hw_rate;
int ret;
+ struct cpufreq_qcom *freq_domain_map, *freq_domain;
+
+ freq_domain_map = devm_kcalloc(&pdev->dev, MAX_FREQ_DOMAINS,
+ sizeof(*freq_domain_map), GFP_KERNEL);
+ cpufreq_qcom_hw_driver.driver_data = freq_domain_map;
+ if (!freq_domain_map)
+ return -ENOMEM;
clk = clk_get(&pdev->dev, "xo");
if (IS_ERR(clk))
@@ -273,16 +260,26 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
"#freq-domain-cells", 0,
- &args);
+ &args);
of_node_put(cpu_np);
if (ret)
return ret;
- if (qcom_freq_domain_map[cpu])
+ domain = args.args[0];
+ if (WARN_ON(domain >= MAX_FREQ_DOMAINS))
+ continue;
+
+ freq_domain = &freq_domain_map[domain];
+ cpumask_set_cpu(cpu, &freq_domain->related_cpus);
+ /*
+ * If we've already populated the frequency table for this domain
+ * just mark it related and get out of here
+ */
+ if (cpumask_weight(&freq_domain->related_cpus) > 1)
continue;
- ret = qcom_cpu_resources_init(pdev, cpu, args.args[0],
- xo_rate, cpu_hw_rate);
+ ret = qcom_cpu_resources_init(pdev, domain, xo_rate,
+ cpu_hw_rate, freq_domain);
if (ret)
return ret;
}


--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--