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

login
register
mail settings
Submitter Viresh Kumar
Date Dec. 5, 2018, 6:16 a.m.
Message ID <20181205061600.7zglbpkgbktn27am@vireshk-i7>
Download mbox | patch
Permalink /patch/672573/
State New
Headers show

Comments

Viresh Kumar - Dec. 5, 2018, 6:16 a.m.
On 05-12-18, 09:07, Taniya Das wrote:
> Hello Stephen, Viresh
> 
> Thanks for the code and suggestions.
> 
> Having a NR_DOMAINS '2' makes the driver not scalable for re-use.

Sure, I didn't like it either and that wasn't really what I was suggesting in
the first place. I didn't wanted to write the code myself and pass it on, but I
have it now anyway :)

It may have a bug or two in there, but compiles just fine and is rebased over
your patch Taniya.
Taniya Das - Dec. 11, 2018, 1:35 p.m.
Hi Viresh,

Sorry for the delayed response. Thanks for the patch.

On 12/5/2018 11:46 AM, Viresh Kumar wrote:
> On 05-12-18, 09:07, Taniya Das wrote:
>> Hello Stephen, Viresh
>>
>> Thanks for the code and suggestions.
>>
>> Having a NR_DOMAINS '2' makes the driver not scalable for re-use.
> 
> Sure, I didn't like it either and that wasn't really what I was suggesting in
> the first place. I didn't wanted to write the code myself and pass it on, but I
> have it now anyway :)
> 
> It may have a bug or two in there, but compiles just fine and is rebased over
> your patch Taniya.
> 

The design here assumes that there would not be any per-cpu/per-cluster 
based SW requirement for the HW during frequency transitions, which 
again makes me think that we would require to re-introduce these 
structures again in case we have such requirements in near future.

Also I think leaving the structures also helps us debug any boot up 
issues looking at the ram contents of the per-(cpu/cluster) structures 
with the contents from the firmware.

Hope these above helps us to go ahead with the current SW design.
Viresh Kumar - Dec. 12, 2018, 4:47 a.m.
On 11-12-18, 19:05, Taniya Das wrote:
> The design here assumes that there would not be any per-cpu/per-cluster
> based SW requirement for the HW during frequency transitions, which again
> makes me think that we would require to re-introduce these structures again
> in case we have such requirements in near future.

Firstly, even in such cases we can go ahead with the design we proposed. And I
am not at all concerned about some hardware which we don't have right now. We
will see what to do when such hardware comes, maybe reintroduce the structures,
but that doesn't matter right now.

> Also I think leaving the structures also helps us debug any boot up issues
> looking at the ram contents of the per-(cpu/cluster) structures with the
> contents from the firmware.

I don't see how debugging would be hard without those structures in place.

> Hope these above helps us to go ahead with the current SW design.

Sorry, but I don't see any satisfactory reason on why you shouldn't make the
suggested changes. We are trying to make your (and any other developer who will
work on that driver) life simple by simplifying the code. Nothing beyond that :)
Taniya Das - Dec. 13, 2018, 7:48 a.m.
Hello Viresh,

On 12/12/2018 10:17 AM, Viresh Kumar wrote:
> On 11-12-18, 19:05, Taniya Das wrote:
>> The design here assumes that there would not be any per-cpu/per-cluster
>> based SW requirement for the HW during frequency transitions, which again
>> makes me think that we would require to re-introduce these structures again
>> in case we have such requirements in near future.
> 
> Firstly, even in such cases we can go ahead with the design we proposed. And I
> am not at all concerned about some hardware which we don't have right now. We
> will see what to do when such hardware comes, maybe reintroduce the structures,
> but that doesn't matter right now.
> 
>> Also I think leaving the structures also helps us debug any boot up issues
>> looking at the ram contents of the per-(cpu/cluster) structures with the
>> contents from the firmware.
> 
> I don't see how debugging would be hard without those structures in place.
> 
>> Hope these above helps us to go ahead with the current SW design.
> 
> Sorry, but I don't see any satisfactory reason on why you shouldn't make the
> suggested changes. We are trying to make your (and any other developer who will
> work on that driver) life simple by simplifying the code. Nothing beyond that :)
> 

Sure, Thanks, will submit the next patch series for your ACK :).

Patch

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 8dc6b73c2f22..bcf9bb37298a 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -23,13 +23,8 @@ 
 #define REG_LUT_TABLE			0x110
 #define REG_PERF_STATE			0x920
 
-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 unsigned long cpu_hw_rate, xo_rate;
+static struct platform_device *global_pdev;
 
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 					unsigned int index)
@@ -74,53 +69,17 @@  static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
 	return policy->freq_table[index].frequency;
 }
 
-static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
-{
-	struct cpufreq_qcom *c;
-
-	c = qcom_freq_domain_map[policy->cpu];
-	if (!c) {
-		pr_err("No scaling support for CPU%d\n", policy->cpu);
-		return -ENODEV;
-	}
-
-	cpumask_copy(policy->cpus, &c->related_cpus);
-
-	policy->fast_switch_possible = true;
-	policy->freq_table = c->table;
-	policy->driver_data = c->perf_state_reg;
-
-	return 0;
-}
-
-static struct freq_attr *qcom_cpufreq_hw_attr[] = {
-	&cpufreq_freq_attr_scaling_available_freqs,
-	&cpufreq_freq_attr_scaling_boost_freqs,
-	NULL
-};
-
-static struct cpufreq_driver cpufreq_qcom_hw_driver = {
-	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
-			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
-	.verify		= cpufreq_generic_frequency_table_verify,
-	.target_index	= qcom_cpufreq_hw_target_index,
-	.get		= qcom_cpufreq_hw_get,
-	.init		= qcom_cpufreq_hw_cpu_init,
-	.fast_switch    = qcom_cpufreq_hw_fast_switch,
-	.name		= "qcom-cpufreq-hw",
-	.attr		= qcom_cpufreq_hw_attr,
-};
-
-static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
-				    void __iomem *base, unsigned long xo_rate,
-				    unsigned long cpu_hw_rate)
+static int qcom_cpufreq_hw_read_lut(struct device *dev,
+				    struct cpufreq_policy *policy,
+				    void __iomem *base)
 {
 	u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, freq;
-	unsigned int max_cores = cpumask_weight(&c->related_cpus);
+	unsigned int max_cores = cpumask_weight(policy->cpus);
+	struct cpufreq_frequency_table	*table;
 
-	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
-				sizeof(*c->table), GFP_KERNEL);
-	if (!c->table)
+	table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
+				sizeof(*table), GFP_KERNEL);
+	if (!table)
 		return -ENOMEM;
 
 	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
@@ -136,9 +95,9 @@  static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
 
 		/* Ignore boosts in the middle of the table */
 		if (core_count != max_cores) {
-			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
+			table[i].frequency = CPUFREQ_ENTRY_INVALID;
 		} else {
-			c->table[i].frequency = freq;
+			table[i].frequency = freq;
 			dev_dbg(dev, "index=%d freq=%d, core_count %d\n", i,
 				freq, core_count);
 		}
@@ -148,7 +107,7 @@  static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
 		 * end of table
 		 */
 		if (i > 0 && prev_freq == freq && prev_cc == core_count) {
-			struct cpufreq_frequency_table *prev = &c->table[i - 1];
+			struct cpufreq_frequency_table *prev = &table[i - 1];
 
 			/*
 			 * Only treat the last frequency that might be a boost
@@ -166,7 +125,8 @@  static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
 		prev_freq = freq;
 	}
 
-	c->table[i].frequency = CPUFREQ_TABLE_END;
+	table[i].frequency = CPUFREQ_TABLE_END;
+	policy->freq_table = table;
 
 	return 0;
 }
@@ -194,22 +154,29 @@  static void qcom_get_related_cpus(int index, struct cpumask *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)
+static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
-	struct cpufreq_qcom *c;
+	struct device *dev = &global_pdev->dev;
+	struct of_phandle_args args;
+	struct device_node *cpu_np;
 	struct resource *res;
-	struct device *dev = &pdev->dev;
 	void __iomem *base;
-	int ret, cpu_r;
+	int ret, index;
 
-	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
-	if (!c)
-		return -ENOMEM;
+	cpu_np = of_cpu_device_node_get(policy->cpu);
+	if (!cpu_np)
+		return -EINVAL;
+
+	ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
+			"#freq-domain-cells", 0, &args);
+	of_node_put(cpu_np);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
+	if (ret)
+		return ret;
+
+	index = args.args[0];
+
+	res = platform_get_resource(global_pdev, IORESOURCE_MEM, index);
 	base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
@@ -220,33 +187,46 @@  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)) {
+	qcom_get_related_cpus(index, policy->cpus);
+	if (!cpumask_weight(policy->cpus)) {
 		dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
 		return -ENOENT;
 	}
 
-	c->perf_state_reg = base + REG_PERF_STATE;
+	policy->driver_data = base + REG_PERF_STATE;
 
-	ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
+	ret = qcom_cpufreq_hw_read_lut(dev, policy, base);
 	if (ret) {
 		dev_err(dev, "Domain-%d failed to read LUT\n", index);
 		return ret;
 	}
 
-	for_each_cpu(cpu_r, &c->related_cpus)
-		qcom_freq_domain_map[cpu_r] = c;
+	policy->fast_switch_possible = true;
 
 	return 0;
 }
 
+static struct freq_attr *qcom_cpufreq_hw_attr[] = {
+	&cpufreq_freq_attr_scaling_available_freqs,
+	&cpufreq_freq_attr_scaling_boost_freqs,
+	NULL
+};
+
+static struct cpufreq_driver cpufreq_qcom_hw_driver = {
+	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+	.verify		= cpufreq_generic_frequency_table_verify,
+	.target_index	= qcom_cpufreq_hw_target_index,
+	.get		= qcom_cpufreq_hw_get,
+	.init		= qcom_cpufreq_hw_cpu_init,
+	.fast_switch    = qcom_cpufreq_hw_fast_switch,
+	.name		= "qcom-cpufreq-hw",
+	.attr		= qcom_cpufreq_hw_attr,
+};
+
 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 long xo_rate, cpu_hw_rate;
 	int ret;
 
 	clk = clk_get(&pdev->dev, "xo");
@@ -263,29 +243,7 @@  static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 	cpu_hw_rate = clk_get_rate(clk) / CLK_HW_DIV;
 	clk_put(clk);
 
-	for_each_possible_cpu(cpu) {
-		cpu_np = of_cpu_device_node_get(cpu);
-		if (!cpu_np) {
-			dev_dbg(&pdev->dev, "Failed to get cpu %d device\n",
-				cpu);
-			continue;
-		}
-
-		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;
-
-		if (qcom_freq_domain_map[cpu])
-			continue;
-
-		ret = qcom_cpu_resources_init(pdev, cpu, args.args[0],
-					      xo_rate, cpu_hw_rate);
-		if (ret)
-			return ret;
-	}
+	global_pdev = pdev;
 
 	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
 	if (ret) {