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

[Stephen Boyd <sboyd@kernel.org>]

Quoting Viresh Kumar (2018-12-04 22:16:00)
> 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.

If we move the ioremap of registers to the cpufreq_driver::init path
then we need to unmap it on cpufreq_driver::exit. In fact, all the
devm_*() code in the init path needs to change to non-devm. Otherwise
it's a nice simplification!

> +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);

This one.

> +       if (!table)
>                 return -ENOMEM;
>  
>         for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> @@ -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);

And this one.

>         if (IS_ERR(base))
>                 return PTR_ERR(base);

Here's a patch to squash in to fix those tiny problems. I left it as
devm_ioremap_resource() because that has all the nice features of
resource requesting inside and it didn't seem too bad to devm_iounmap()
on the exit path.

---------8<--------------

 drivers/cpufreq/qcom-cpufreq-hw.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index bcf9bb37298a..1e865e216839 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
+#include <linux/slab.h>
 
 #define LUT_MAX_ENTRIES			40U
 #define LUT_SRC				GENMASK(31, 30)
@@ -77,8 +78,7 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
 	unsigned int max_cores = cpumask_weight(policy->cpus);
 	struct cpufreq_frequency_table	*table;
 
-	table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
-				sizeof(*table), GFP_KERNEL);
+	table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
 	if (!table)
 		return -ENOMEM;
 
@@ -144,7 +144,7 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
 
 		ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
 						 "#freq-domain-cells", 0,
-						  &args);
+						 &args);
 		of_node_put(cpu_np);
 		if (ret < 0)
 			continue;
@@ -170,7 +170,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 	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;
 
@@ -184,13 +183,15 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 	/* HW should be in enabled state to proceed */
 	if (!(readl_relaxed(base + REG_ENABLE) & 0x1)) {
 		dev_err(dev, "Domain-%d cpufreq hardware not enabled\n", index);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto error;
 	}
 
 	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;
+		ret = -ENOENT;
+		goto error;
 	}
 
 	policy->driver_data = base + REG_PERF_STATE;
@@ -198,11 +199,25 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 	ret = qcom_cpufreq_hw_read_lut(dev, policy, base);
 	if (ret) {
 		dev_err(dev, "Domain-%d failed to read LUT\n", index);
-		return ret;
+		goto error;
 	}
 
 	policy->fast_switch_possible = true;
 
+	return 0;
+
+error:
+	devm_iounmap(dev, base);
+	return ret;
+}
+
+static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
+{
+	void __iomem *base = policy->driver_data - REG_PERF_STATE;
+
+	kfree(policy->freq_table);
+	devm_iounmap(&global_pdev->dev, base);
+
 	return 0;
 }
 
@@ -219,6 +234,7 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
 	.target_index	= qcom_cpufreq_hw_target_index,
 	.get		= qcom_cpufreq_hw_get,
 	.init		= qcom_cpufreq_hw_cpu_init,
+	.exit		= qcom_cpufreq_hw_cpu_exit,
 	.fast_switch    = qcom_cpufreq_hw_fast_switch,
 	.name		= "qcom-cpufreq-hw",
 	.attr		= qcom_cpufreq_hw_attr,
@@ -248,12 +264,11 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
 	if (ret) {
 		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
-		return ret;
+	} else {
+		dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
 	}
 
-	dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
-
-	return 0;
+	return ret;
 }
 
 static int qcom_cpufreq_hw_driver_remove(struct platform_device *pdev)