[04/13] cpufreq: qcom: Refactor the driver to make it easier to extend
diff mbox series

Message ID 20190705095726.21433-5-niklas.cassel@linaro.org
State Superseded
Headers show
Series
  • Add support for QCOM Core Power Reduction
Related show

Commit Message

Niklas Cassel July 5, 2019, 9:57 a.m. UTC
Refactor the driver to make it easier to extend in a later commit.

Create a driver struct to collect all common resources, in order to make
it easier to free up all common resources.
Create a driver match_data struct to make it easier to extend the driver
with support for new features that might only be supported on certain SoCs.

Co-developed-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
---
Changes since RFC:
-Changed type of versions to u32 from u32*.
-Make the driver use a match_data struct, so that different SoC can have
different features.
-Fixed error handling.

 drivers/cpufreq/qcom-cpufreq-nvmem.c | 123 +++++++++++++++++----------
 1 file changed, 79 insertions(+), 44 deletions(-)

Comments

Ilia Lin July 8, 2019, 6:30 a.m. UTC | #1
Reviewed-by: Ilia Lin <ilia.lin@kernel.org>

On Fri, Jul 5, 2019 at 12:58 PM Niklas Cassel <niklas.cassel@linaro.org> wrote:
>
> Refactor the driver to make it easier to extend in a later commit.
>
> Create a driver struct to collect all common resources, in order to make
> it easier to free up all common resources.
> Create a driver match_data struct to make it easier to extend the driver
> with support for new features that might only be supported on certain SoCs.
>
> Co-developed-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> ---
> Changes since RFC:
> -Changed type of versions to u32 from u32*.
> -Make the driver use a match_data struct, so that different SoC can have
> different features.
> -Fixed error handling.
>
>  drivers/cpufreq/qcom-cpufreq-nvmem.c | 123 +++++++++++++++++----------
>  1 file changed, 79 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> index fad6509eecb5..c0377b0eb2f4 100644
> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> @@ -43,6 +43,20 @@ enum _msm8996_version {
>         NUM_OF_MSM8996_VERSIONS,
>  };
>
> +struct qcom_cpufreq_drv;
> +
> +struct qcom_cpufreq_match_data {
> +       int (*get_version)(struct device *cpu_dev,
> +                          struct nvmem_cell *speedbin_nvmem,
> +                          struct qcom_cpufreq_drv *drv);
> +};
> +
> +struct qcom_cpufreq_drv {
> +       struct opp_table **opp_tables;
> +       u32 versions;
> +       const struct qcom_cpufreq_match_data *data;
> +};
> +
>  static struct platform_device *cpufreq_dt_pdev, *cpufreq_pdev;
>
>  static enum _msm8996_version qcom_cpufreq_get_msm_id(void)
> @@ -76,7 +90,7 @@ static enum _msm8996_version qcom_cpufreq_get_msm_id(void)
>
>  static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
>                                           struct nvmem_cell *speedbin_nvmem,
> -                                         u32 *versions)
> +                                         struct qcom_cpufreq_drv *drv)
>  {
>         size_t len;
>         u8 *speedbin;
> @@ -94,10 +108,10 @@ static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
>
>         switch (msm8996_version) {
>         case MSM8996_V3:
> -               *versions = 1 << (unsigned int)(*speedbin);
> +               drv->versions = 1 << (unsigned int)(*speedbin);
>                 break;
>         case MSM8996_SG:
> -               *versions = 1 << ((unsigned int)(*speedbin) + 4);
> +               drv->versions = 1 << ((unsigned int)(*speedbin) + 4);
>                 break;
>         default:
>                 BUG();
> @@ -108,17 +122,17 @@ static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
>         return 0;
>  }
>
> +static const struct qcom_cpufreq_match_data match_data_kryo = {
> +       .get_version = qcom_cpufreq_kryo_name_version,
> +};
> +
>  static int qcom_cpufreq_probe(struct platform_device *pdev)
>  {
> -       struct opp_table **opp_tables;
> -       int (*get_version)(struct device *cpu_dev,
> -                          struct nvmem_cell *speedbin_nvmem,
> -                          u32 *versions);
> +       struct qcom_cpufreq_drv *drv;
>         struct nvmem_cell *speedbin_nvmem;
>         struct device_node *np;
>         struct device *cpu_dev;
>         unsigned cpu;
> -       u32 versions;
>         const struct of_device_id *match;
>         int ret;
>
> @@ -126,11 +140,6 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>         if (!cpu_dev)
>                 return -ENODEV;
>
> -       match = pdev->dev.platform_data;
> -       get_version = match->data;
> -       if (!get_version)
> -               return -ENODEV;
> -
>         np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
>         if (!np)
>                 return -ENOENT;
> @@ -141,23 +150,43 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>                 return -ENOENT;
>         }
>
> -       speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> -       of_node_put(np);
> -       if (IS_ERR(speedbin_nvmem)) {
> -               if (PTR_ERR(speedbin_nvmem) != -EPROBE_DEFER)
> -                       dev_err(cpu_dev, "Could not get nvmem cell: %ld\n",
> -                               PTR_ERR(speedbin_nvmem));
> -               return PTR_ERR(speedbin_nvmem);
> +       drv = kzalloc(sizeof(*drv), GFP_KERNEL);
> +       if (!drv)
> +               return -ENOMEM;
> +
> +       match = pdev->dev.platform_data;
> +       drv->data = match->data;
> +       if (!drv->data) {
> +               ret = -ENODEV;
> +               goto free_drv;
>         }
>
> -       ret = get_version(cpu_dev, speedbin_nvmem, &versions);
> -       nvmem_cell_put(speedbin_nvmem);
> -       if (ret)
> -               return ret;
> +       if (drv->data->get_version) {
> +               speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> +               of_node_put(np);
> +               if (IS_ERR(speedbin_nvmem)) {
> +                       if (PTR_ERR(speedbin_nvmem) != -EPROBE_DEFER)
> +                               dev_err(cpu_dev,
> +                                       "Could not get nvmem cell: %ld\n",
> +                                       PTR_ERR(speedbin_nvmem));
> +                       ret = PTR_ERR(speedbin_nvmem);
> +                       goto free_drv;
> +               }
>
> -       opp_tables = kcalloc(num_possible_cpus(), sizeof(*opp_tables), GFP_KERNEL);
> -       if (!opp_tables)
> -               return -ENOMEM;
> +               ret = drv->data->get_version(cpu_dev, speedbin_nvmem, drv);
> +               if (ret) {
> +                       nvmem_cell_put(speedbin_nvmem);
> +                       goto free_drv;
> +               }
> +               nvmem_cell_put(speedbin_nvmem);
> +       }
> +
> +       drv->opp_tables = kcalloc(num_possible_cpus(), sizeof(*drv->opp_tables),
> +                                 GFP_KERNEL);
> +       if (!drv->opp_tables) {
> +               ret = -ENOMEM;
> +               goto free_drv;
> +       }
>
>         for_each_possible_cpu(cpu) {
>                 cpu_dev = get_cpu_device(cpu);
> @@ -166,19 +195,23 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>                         goto free_opp;
>                 }
>
> -               opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> -                                                             &versions, 1);
> -               if (IS_ERR(opp_tables[cpu])) {
> -                       ret = PTR_ERR(opp_tables[cpu]);
> -                       dev_err(cpu_dev, "Failed to set supported hardware\n");
> -                       goto free_opp;
> +               if (drv->data->get_version) {
> +                       drv->opp_tables[cpu] =
> +                               dev_pm_opp_set_supported_hw(cpu_dev,
> +                                                           &drv->versions, 1);
> +                       if (IS_ERR(drv->opp_tables[cpu])) {
> +                               ret = PTR_ERR(drv->opp_tables[cpu]);
> +                               dev_err(cpu_dev,
> +                                       "Failed to set supported hardware\n");
> +                               goto free_opp;
> +                       }
>                 }
>         }
>
>         cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
>                                                           NULL, 0);
>         if (!IS_ERR(cpufreq_dt_pdev)) {
> -               platform_set_drvdata(pdev, opp_tables);
> +               platform_set_drvdata(pdev, drv);
>                 return 0;
>         }
>
> @@ -187,26 +220,30 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>
>  free_opp:
>         for_each_possible_cpu(cpu) {
> -               if (IS_ERR_OR_NULL(opp_tables[cpu]))
> +               if (IS_ERR_OR_NULL(drv->opp_tables[cpu]))
>                         break;
> -               dev_pm_opp_put_supported_hw(opp_tables[cpu]);
> +               dev_pm_opp_put_supported_hw(drv->opp_tables[cpu]);
>         }
> -       kfree(opp_tables);
> +       kfree(drv->opp_tables);
> +free_drv:
> +       kfree(drv);
>
>         return ret;
>  }
>
>  static int qcom_cpufreq_remove(struct platform_device *pdev)
>  {
> -       struct opp_table **opp_tables = platform_get_drvdata(pdev);
> +       struct qcom_cpufreq_drv *drv = platform_get_drvdata(pdev);
>         unsigned int cpu;
>
>         platform_device_unregister(cpufreq_dt_pdev);
>
>         for_each_possible_cpu(cpu)
> -               dev_pm_opp_put_supported_hw(opp_tables[cpu]);
> +               if (drv->opp_tables[cpu])
> +                       dev_pm_opp_put_supported_hw(drv->opp_tables[cpu]);
>
> -       kfree(opp_tables);
> +       kfree(drv->opp_tables);
> +       kfree(drv);
>
>         return 0;
>  }
> @@ -220,10 +257,8 @@ static struct platform_driver qcom_cpufreq_driver = {
>  };
>
>  static const struct of_device_id qcom_cpufreq_match_list[] __initconst = {
> -       { .compatible = "qcom,apq8096",
> -         .data = qcom_cpufreq_kryo_name_version },
> -       { .compatible = "qcom,msm8996",
> -         .data = qcom_cpufreq_kryo_name_version },
> +       { .compatible = "qcom,apq8096", .data = &match_data_kryo },
> +       { .compatible = "qcom,msm8996", .data = &match_data_kryo },
>         {},
>  };
>
> --
> 2.21.0
>
Viresh Kumar July 10, 2019, 6:30 a.m. UTC | #2
On 05-07-19, 11:57, Niklas Cassel wrote:
> +	drv->opp_tables = kcalloc(num_possible_cpus(), sizeof(*drv->opp_tables),
> +				  GFP_KERNEL);
> +	if (!drv->opp_tables) {
> +		ret = -ENOMEM;
> +		goto free_drv;
> +	}
>  
>  	for_each_possible_cpu(cpu) {
>  		cpu_dev = get_cpu_device(cpu);
> @@ -166,19 +195,23 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>  			goto free_opp;
>  		}
>  
> -		opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> -							      &versions, 1);
> -		if (IS_ERR(opp_tables[cpu])) {
> -			ret = PTR_ERR(opp_tables[cpu]);
> -			dev_err(cpu_dev, "Failed to set supported hardware\n");
> -			goto free_opp;
> +		if (drv->data->get_version) {

Why depend on get_version here ? The OPP table is already allocated
unconditionally.

> +			drv->opp_tables[cpu] =
> +				dev_pm_opp_set_supported_hw(cpu_dev,
> +							    &drv->versions, 1);
> +			if (IS_ERR(drv->opp_tables[cpu])) {
> +				ret = PTR_ERR(drv->opp_tables[cpu]);
> +				dev_err(cpu_dev,
> +					"Failed to set supported hardware\n");
> +				goto free_opp;
> +			}
>  		}
>  	}
Niklas Cassel July 16, 2019, 11:11 a.m. UTC | #3
On Wed, Jul 10, 2019 at 12:00:26PM +0530, Viresh Kumar wrote:
> On 05-07-19, 11:57, Niklas Cassel wrote:
> > +	drv->opp_tables = kcalloc(num_possible_cpus(), sizeof(*drv->opp_tables),
> > +				  GFP_KERNEL);
> > +	if (!drv->opp_tables) {
> > +		ret = -ENOMEM;
> > +		goto free_drv;
> > +	}
> >  
> >  	for_each_possible_cpu(cpu) {
> >  		cpu_dev = get_cpu_device(cpu);
> > @@ -166,19 +195,23 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> >  			goto free_opp;
> >  		}
> >  
> > -		opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> > -							      &versions, 1);
> > -		if (IS_ERR(opp_tables[cpu])) {
> > -			ret = PTR_ERR(opp_tables[cpu]);
> > -			dev_err(cpu_dev, "Failed to set supported hardware\n");
> > -			goto free_opp;
> > +		if (drv->data->get_version) {
> 
> Why depend on get_version here ? The OPP table is already allocated
> unconditionally.

Since the reading of the speedbin efuse is now optional,
it is now inside "if (drv->data->get_version)".

So if I don't also protect "dev_pm_opp_set_supported_hw()"
with "if (drv->data->get_version)" I get:

[    3.135092] cpu cpu0: _opp_is_supported: failed to read opp-supported-hw property at index 0: -22
[    3.139364] cpu cpu0: _opp_is_supported: failed to read opp-supported-hw property at index 0: -22
[    3.148330] cpu cpu0: _opp_is_supported: failed to read opp-supported-hw property at index 0: -22

Probably since drv->versions is initialized to 0,
and if there is no opp-supported-hw in device tree,
OPP framework prints failures.

So it feels safest to only call dev_pm_opp_set_supported_hw()
if we know that we are supposed to parse the speedbin efuse.


Kind regards,
Niklas

> 
> > +			drv->opp_tables[cpu] =
> > +				dev_pm_opp_set_supported_hw(cpu_dev,
> > +							    &drv->versions, 1);
> > +			if (IS_ERR(drv->opp_tables[cpu])) {
> > +				ret = PTR_ERR(drv->opp_tables[cpu]);
> > +				dev_err(cpu_dev,
> > +					"Failed to set supported hardware\n");
> > +				goto free_opp;
> > +			}
> >  		}
> >  	}
> 
> -- 
> viresh

Patch
diff mbox series

diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
index fad6509eecb5..c0377b0eb2f4 100644
--- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
+++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
@@ -43,6 +43,20 @@  enum _msm8996_version {
 	NUM_OF_MSM8996_VERSIONS,
 };
 
+struct qcom_cpufreq_drv;
+
+struct qcom_cpufreq_match_data {
+	int (*get_version)(struct device *cpu_dev,
+			   struct nvmem_cell *speedbin_nvmem,
+			   struct qcom_cpufreq_drv *drv);
+};
+
+struct qcom_cpufreq_drv {
+	struct opp_table **opp_tables;
+	u32 versions;
+	const struct qcom_cpufreq_match_data *data;
+};
+
 static struct platform_device *cpufreq_dt_pdev, *cpufreq_pdev;
 
 static enum _msm8996_version qcom_cpufreq_get_msm_id(void)
@@ -76,7 +90,7 @@  static enum _msm8996_version qcom_cpufreq_get_msm_id(void)
 
 static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
 					  struct nvmem_cell *speedbin_nvmem,
-					  u32 *versions)
+					  struct qcom_cpufreq_drv *drv)
 {
 	size_t len;
 	u8 *speedbin;
@@ -94,10 +108,10 @@  static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
 
 	switch (msm8996_version) {
 	case MSM8996_V3:
-		*versions = 1 << (unsigned int)(*speedbin);
+		drv->versions = 1 << (unsigned int)(*speedbin);
 		break;
 	case MSM8996_SG:
-		*versions = 1 << ((unsigned int)(*speedbin) + 4);
+		drv->versions = 1 << ((unsigned int)(*speedbin) + 4);
 		break;
 	default:
 		BUG();
@@ -108,17 +122,17 @@  static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
 	return 0;
 }
 
+static const struct qcom_cpufreq_match_data match_data_kryo = {
+	.get_version = qcom_cpufreq_kryo_name_version,
+};
+
 static int qcom_cpufreq_probe(struct platform_device *pdev)
 {
-	struct opp_table **opp_tables;
-	int (*get_version)(struct device *cpu_dev,
-			   struct nvmem_cell *speedbin_nvmem,
-			   u32 *versions);
+	struct qcom_cpufreq_drv *drv;
 	struct nvmem_cell *speedbin_nvmem;
 	struct device_node *np;
 	struct device *cpu_dev;
 	unsigned cpu;
-	u32 versions;
 	const struct of_device_id *match;
 	int ret;
 
@@ -126,11 +140,6 @@  static int qcom_cpufreq_probe(struct platform_device *pdev)
 	if (!cpu_dev)
 		return -ENODEV;
 
-	match = pdev->dev.platform_data;
-	get_version = match->data;
-	if (!get_version)
-		return -ENODEV;
-
 	np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
 	if (!np)
 		return -ENOENT;
@@ -141,23 +150,43 @@  static int qcom_cpufreq_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
-	of_node_put(np);
-	if (IS_ERR(speedbin_nvmem)) {
-		if (PTR_ERR(speedbin_nvmem) != -EPROBE_DEFER)
-			dev_err(cpu_dev, "Could not get nvmem cell: %ld\n",
-				PTR_ERR(speedbin_nvmem));
-		return PTR_ERR(speedbin_nvmem);
+	drv = kzalloc(sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	match = pdev->dev.platform_data;
+	drv->data = match->data;
+	if (!drv->data) {
+		ret = -ENODEV;
+		goto free_drv;
 	}
 
-	ret = get_version(cpu_dev, speedbin_nvmem, &versions);
-	nvmem_cell_put(speedbin_nvmem);
-	if (ret)
-		return ret;
+	if (drv->data->get_version) {
+		speedbin_nvmem = of_nvmem_cell_get(np, NULL);
+		of_node_put(np);
+		if (IS_ERR(speedbin_nvmem)) {
+			if (PTR_ERR(speedbin_nvmem) != -EPROBE_DEFER)
+				dev_err(cpu_dev,
+					"Could not get nvmem cell: %ld\n",
+					PTR_ERR(speedbin_nvmem));
+			ret = PTR_ERR(speedbin_nvmem);
+			goto free_drv;
+		}
 
-	opp_tables = kcalloc(num_possible_cpus(), sizeof(*opp_tables), GFP_KERNEL);
-	if (!opp_tables)
-		return -ENOMEM;
+		ret = drv->data->get_version(cpu_dev, speedbin_nvmem, drv);
+		if (ret) {
+			nvmem_cell_put(speedbin_nvmem);
+			goto free_drv;
+		}
+		nvmem_cell_put(speedbin_nvmem);
+	}
+
+	drv->opp_tables = kcalloc(num_possible_cpus(), sizeof(*drv->opp_tables),
+				  GFP_KERNEL);
+	if (!drv->opp_tables) {
+		ret = -ENOMEM;
+		goto free_drv;
+	}
 
 	for_each_possible_cpu(cpu) {
 		cpu_dev = get_cpu_device(cpu);
@@ -166,19 +195,23 @@  static int qcom_cpufreq_probe(struct platform_device *pdev)
 			goto free_opp;
 		}
 
-		opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
-							      &versions, 1);
-		if (IS_ERR(opp_tables[cpu])) {
-			ret = PTR_ERR(opp_tables[cpu]);
-			dev_err(cpu_dev, "Failed to set supported hardware\n");
-			goto free_opp;
+		if (drv->data->get_version) {
+			drv->opp_tables[cpu] =
+				dev_pm_opp_set_supported_hw(cpu_dev,
+							    &drv->versions, 1);
+			if (IS_ERR(drv->opp_tables[cpu])) {
+				ret = PTR_ERR(drv->opp_tables[cpu]);
+				dev_err(cpu_dev,
+					"Failed to set supported hardware\n");
+				goto free_opp;
+			}
 		}
 	}
 
 	cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
 							  NULL, 0);
 	if (!IS_ERR(cpufreq_dt_pdev)) {
-		platform_set_drvdata(pdev, opp_tables);
+		platform_set_drvdata(pdev, drv);
 		return 0;
 	}
 
@@ -187,26 +220,30 @@  static int qcom_cpufreq_probe(struct platform_device *pdev)
 
 free_opp:
 	for_each_possible_cpu(cpu) {
-		if (IS_ERR_OR_NULL(opp_tables[cpu]))
+		if (IS_ERR_OR_NULL(drv->opp_tables[cpu]))
 			break;
-		dev_pm_opp_put_supported_hw(opp_tables[cpu]);
+		dev_pm_opp_put_supported_hw(drv->opp_tables[cpu]);
 	}
-	kfree(opp_tables);
+	kfree(drv->opp_tables);
+free_drv:
+	kfree(drv);
 
 	return ret;
 }
 
 static int qcom_cpufreq_remove(struct platform_device *pdev)
 {
-	struct opp_table **opp_tables = platform_get_drvdata(pdev);
+	struct qcom_cpufreq_drv *drv = platform_get_drvdata(pdev);
 	unsigned int cpu;
 
 	platform_device_unregister(cpufreq_dt_pdev);
 
 	for_each_possible_cpu(cpu)
-		dev_pm_opp_put_supported_hw(opp_tables[cpu]);
+		if (drv->opp_tables[cpu])
+			dev_pm_opp_put_supported_hw(drv->opp_tables[cpu]);
 
-	kfree(opp_tables);
+	kfree(drv->opp_tables);
+	kfree(drv);
 
 	return 0;
 }
@@ -220,10 +257,8 @@  static struct platform_driver qcom_cpufreq_driver = {
 };
 
 static const struct of_device_id qcom_cpufreq_match_list[] __initconst = {
-	{ .compatible = "qcom,apq8096",
-	  .data = qcom_cpufreq_kryo_name_version },
-	{ .compatible = "qcom,msm8996",
-	  .data = qcom_cpufreq_kryo_name_version },
+	{ .compatible = "qcom,apq8096", .data = &match_data_kryo },
+	{ .compatible = "qcom,msm8996", .data = &match_data_kryo },
 	{},
 };