[PATCH 1/4] cpufreq: mt8173: add CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag

[PATCH 1/4] cpufreq: mt8173: add CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag

[Pi-Cheng Chen]

Add CPUFREQ_HAVE_GOVERNOR_PER_POLICY to have individual set of tunables
for each cluster of MT8173.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
---
 drivers/cpufreq/mt8173-cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 83001dc..c438109 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -469,7 +469,8 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
 }
 
 static struct cpufreq_driver mt8173_cpufreq_driver = {
-	.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
+	.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+		 CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
 	.verify = cpufreq_generic_frequency_table_verify,
 	.target_index = mtk_cpufreq_set_target,
 	.get = cpufreq_generic_get,
-- 
1.9.1

[PATCH 1/4] cpufreq: mt8173: add CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag

[Pi-Cheng Chen]

Add CPUFREQ_HAVE_GOVERNOR_PER_POLICY to have individual set of tunables
for each cluster of MT8173.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
---
 drivers/cpufreq/mt8173-cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 83001dc..c438109 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -469,7 +469,8 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
 }
 
 static struct cpufreq_driver mt8173_cpufreq_driver = {
-	.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
+	.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+		 CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
 	.verify = cpufreq_generic_frequency_table_verify,
 	.target_index = mtk_cpufreq_set_target,
 	.get = cpufreq_generic_get,
-- 
1.9.1

[PATCH 2/4] cpufreq: mt8173: remove redundant regulator_get_voltage() call

[Pi-Cheng Chen]

Remove redundant regulator_get_voltage() call to get Vsram value
since it will be obtained later at the beginning of voltage tracking
loop.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
---
 drivers/cpufreq/mt8173-cpufreq.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index c438109..750cda7 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -59,7 +59,6 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
 
 	old_vproc = regulator_get_voltage(proc_reg);
-	old_vsram = regulator_get_voltage(sram_reg);
 	/* Vsram should not exceed the maximum allowed voltage of SoC. */
 	new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
 
-- 
1.9.1

[PATCH 2/4] cpufreq: mt8173: remove redundant regulator_get_voltage() call

[Pi-Cheng Chen]

Remove redundant regulator_get_voltage() call to get Vsram value
since it will be obtained later at the beginning of voltage tracking
loop.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
---
 drivers/cpufreq/mt8173-cpufreq.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index c438109..750cda7 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -59,7 +59,6 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
 
 	old_vproc = regulator_get_voltage(proc_reg);
-	old_vsram = regulator_get_voltage(sram_reg);
 	/* Vsram should not exceed the maximum allowed voltage of SoC. */
 	new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
 
-- 
1.9.1

[PATCH 3/4] cpufreq: mt8173: check return value of regulator_get_voltage() call

[Pi-Cheng Chen]

Sometimes regulator_get_voltage() call returns negative values for
reasons(e.g. underlying I2C bus timeout). Add check for the return
values and fail out early.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
---
 drivers/cpufreq/mt8173-cpufreq.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 750cda7..9d0fe37 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -59,6 +59,10 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
 
 	old_vproc = regulator_get_voltage(proc_reg);
+	if (old_vproc < 0) {
+		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
+		return old_vproc;
+	}
 	/* Vsram should not exceed the maximum allowed voltage of SoC. */
 	new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
 
@@ -71,7 +75,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 		 */
 		do {
 			old_vsram = regulator_get_voltage(sram_reg);
+			if (old_vsram < 0) {
+				pr_err("%s: invalid Vsram value: %d\n",
+				       __func__, old_vsram);
+				return old_vsram;
+			}
 			old_vproc = regulator_get_voltage(proc_reg);
+			if (old_vproc < 0) {
+				pr_err("%s: invalid Vproc value: %d\n",
+				       __func__, old_vproc);
+				return old_vproc;
+			}
 
 			vsram = min(new_vsram, old_vproc + MAX_VOLT_SHIFT);
 
@@ -116,7 +130,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 		 */
 		do {
 			old_vproc = regulator_get_voltage(proc_reg);
+			if (old_vproc < 0) {
+				pr_err("%s: invalid Vproc value: %d\n",
+				       __func__, old_vproc);
+				return old_vproc;
+			}
 			old_vsram = regulator_get_voltage(sram_reg);
+			if (old_vsram < 0) {
+				pr_err("%s: invalid Vsram value: %d\n",
+				       __func__, old_vsram);
+				return old_vsram;
+			}
 
 			vproc = max(new_vproc, old_vsram - MAX_VOLT_SHIFT);
 			ret = regulator_set_voltage(proc_reg, vproc,
@@ -184,6 +208,10 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 
 	old_freq_hz = clk_get_rate(cpu_clk);
 	old_vproc = regulator_get_voltage(info->proc_reg);
+	if (old_vproc < 0) {
+		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
+		return old_vproc;
+	}
 
 	freq_hz = freq_table[index].frequency * 1000;
 
-- 
1.9.1

[PATCH 3/4] cpufreq: mt8173: check return value of regulator_get_voltage() call

[Pi-Cheng Chen]

Sometimes regulator_get_voltage() call returns negative values for
reasons(e.g. underlying I2C bus timeout). Add check for the return
values and fail out early.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
---
 drivers/cpufreq/mt8173-cpufreq.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 750cda7..9d0fe37 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -59,6 +59,10 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
 
 	old_vproc = regulator_get_voltage(proc_reg);
+	if (old_vproc < 0) {
+		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
+		return old_vproc;
+	}
 	/* Vsram should not exceed the maximum allowed voltage of SoC. */
 	new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
 
@@ -71,7 +75,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 		 */
 		do {
 			old_vsram = regulator_get_voltage(sram_reg);
+			if (old_vsram < 0) {
+				pr_err("%s: invalid Vsram value: %d\n",
+				       __func__, old_vsram);
+				return old_vsram;
+			}
 			old_vproc = regulator_get_voltage(proc_reg);
+			if (old_vproc < 0) {
+				pr_err("%s: invalid Vproc value: %d\n",
+				       __func__, old_vproc);
+				return old_vproc;
+			}
 
 			vsram = min(new_vsram, old_vproc + MAX_VOLT_SHIFT);
 
@@ -116,7 +130,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 		 */
 		do {
 			old_vproc = regulator_get_voltage(proc_reg);
+			if (old_vproc < 0) {
+				pr_err("%s: invalid Vproc value: %d\n",
+				       __func__, old_vproc);
+				return old_vproc;
+			}
 			old_vsram = regulator_get_voltage(sram_reg);
+			if (old_vsram < 0) {
+				pr_err("%s: invalid Vsram value: %d\n",
+				       __func__, old_vsram);
+				return old_vsram;
+			}
 
 			vproc = max(new_vproc, old_vsram - MAX_VOLT_SHIFT);
 			ret = regulator_set_voltage(proc_reg, vproc,
@@ -184,6 +208,10 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 
 	old_freq_hz = clk_get_rate(cpu_clk);
 	old_vproc = regulator_get_voltage(info->proc_reg);
+	if (old_vproc < 0) {
+		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
+		return old_vproc;
+	}
 
 	freq_hz = freq_table[index].frequency * 1000;
 
-- 
1.9.1

[PATCH 4/4] cpufreq: mt8173: move resources allocation into ->probe()

[Pi-Cheng Chen]

Since the return value of ->init() of cpufreq driver is not propagated
to the device driver model now, move resources allocation into
->probe() to handle -EPROBE_DEFER properly.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
---
 drivers/cpufreq/mt8173-cpufreq.c | 94 ++++++++++++++++++++++++++++------------
 1 file changed, 67 insertions(+), 27 deletions(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 9d0fe37..1142625 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -41,16 +41,35 @@
  * the original PLL becomes stable at target frequency.
  */
 struct mtk_cpu_dvfs_info {
+	struct cpumask cpus;
 	struct device *cpu_dev;
 	struct regulator *proc_reg;
 	struct regulator *sram_reg;
 	struct clk *cpu_clk;
 	struct clk *inter_clk;
 	struct thermal_cooling_device *cdev;
+	struct list_head list_head;
 	int intermediate_voltage;
 	bool need_voltage_tracking;
 };
 
+static LIST_HEAD(dvfs_info_list);
+
+struct mtk_cpu_dvfs_info *get_dvfs_info(int cpu)
+{
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list;
+
+	list_for_each(list, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		if (cpumask_test_cpu(cpu, &info->cpus))
+			return info;
+	}
+
+	return NULL;
+}
+
 static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 					int new_vproc)
 {
@@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	 */
 	info->need_voltage_tracking = !IS_ERR(sram_reg);
 
+	/* CPUs in the same cluster share a clock and power domain. */
+	cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);
+
 	return 0;
 
 out_free_opp_table:
@@ -440,47 +462,32 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 	struct cpufreq_frequency_table *freq_table;
 	int ret;
 
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info)
-		return -ENOMEM;
-
-	ret = mtk_cpu_dvfs_info_init(info, policy->cpu);
-	if (ret) {
-		pr_err("%s failed to initialize dvfs info for cpu%d\n",
-		       __func__, policy->cpu);
-		goto out_free_dvfs_info;
+	info = get_dvfs_info(policy->cpu);
+	if (!info) {
+		pr_err("dvfs info for cpu%d is not initialized.\n",
+		       policy->cpu);
+		return -EINVAL;
 	}
 
 	ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
 	if (ret) {
 		pr_err("failed to init cpufreq table for cpu%d: %d\n",
 		       policy->cpu, ret);
-		goto out_release_dvfs_info;
+		return ret;
 	}
 
 	ret = cpufreq_table_validate_and_show(policy, freq_table);
 	if (ret) {
 		pr_err("%s: invalid frequency table: %d\n", __func__, ret);
-		goto out_free_cpufreq_table;
+		dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
+		return ret;
 	}
 
-	/* CPUs in the same cluster share a clock and power domain. */
-	cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling);
+	cpumask_copy(policy->cpus, &info->cpus);
 	policy->driver_data = info;
 	policy->clk = info->cpu_clk;
 
 	return 0;
-
-out_free_cpufreq_table:
-	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
-
-out_release_dvfs_info:
-	mtk_cpu_dvfs_info_release(info);
-
-out_free_dvfs_info:
-	kfree(info);
-
-	return ret;
 }
 
 static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
@@ -489,8 +496,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
 
 	cpufreq_cooling_unregister(info->cdev);
 	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
-	mtk_cpu_dvfs_info_release(info);
-	kfree(info);
 
 	return 0;
 }
@@ -510,12 +515,47 @@ static struct cpufreq_driver mt8173_cpufreq_driver = {
 
 static int mt8173_cpufreq_probe(struct platform_device *pdev)
 {
-	int ret;
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list, *tmp;
+	int cpu, ret;
+
+	for_each_possible_cpu(cpu) {
+		info = get_dvfs_info(cpu);
+		if (info)
+			continue;
+
+		info = kzalloc(sizeof(*info), GFP_KERNEL);
+		if (!info) {
+			ret = -ENOMEM;
+			goto release_dvfs_info_list;
+		}
+
+		ret = mtk_cpu_dvfs_info_init(info, cpu);
+		if (ret) {
+			pr_err("%s failed to initialize dvfs info for cpu%d\n",
+			       __func__, cpu);
+			kfree(info);
+			goto release_dvfs_info_list;
+		}
+
+		list_add(&info->list_head, &dvfs_info_list);
+	}
 
 	ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
 	if (ret)
 		pr_err("failed to register mtk cpufreq driver\n");
 
+	return 0;
+
+release_dvfs_info_list:
+	list_for_each_safe(list, tmp, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		mtk_cpu_dvfs_info_release(info);
+		list_del(list);
+		kfree(info);
+	}
+
 	return ret;
 }
 
-- 
1.9.1

[PATCH 4/4] cpufreq: mt8173: move resources allocation into ->probe()

[Pi-Cheng Chen]

Since the return value of ->init() of cpufreq driver is not propagated
to the device driver model now, move resources allocation into
->probe() to handle -EPROBE_DEFER properly.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
---
 drivers/cpufreq/mt8173-cpufreq.c | 94 ++++++++++++++++++++++++++++------------
 1 file changed, 67 insertions(+), 27 deletions(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 9d0fe37..1142625 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -41,16 +41,35 @@
  * the original PLL becomes stable at target frequency.
  */
 struct mtk_cpu_dvfs_info {
+	struct cpumask cpus;
 	struct device *cpu_dev;
 	struct regulator *proc_reg;
 	struct regulator *sram_reg;
 	struct clk *cpu_clk;
 	struct clk *inter_clk;
 	struct thermal_cooling_device *cdev;
+	struct list_head list_head;
 	int intermediate_voltage;
 	bool need_voltage_tracking;
 };
 
+static LIST_HEAD(dvfs_info_list);
+
+struct mtk_cpu_dvfs_info *get_dvfs_info(int cpu)
+{
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list;
+
+	list_for_each(list, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		if (cpumask_test_cpu(cpu, &info->cpus))
+			return info;
+	}
+
+	return NULL;
+}
+
 static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 					int new_vproc)
 {
@@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	 */
 	info->need_voltage_tracking = !IS_ERR(sram_reg);
 
+	/* CPUs in the same cluster share a clock and power domain. */
+	cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);
+
 	return 0;
 
 out_free_opp_table:
@@ -440,47 +462,32 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 	struct cpufreq_frequency_table *freq_table;
 	int ret;
 
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info)
-		return -ENOMEM;
-
-	ret = mtk_cpu_dvfs_info_init(info, policy->cpu);
-	if (ret) {
-		pr_err("%s failed to initialize dvfs info for cpu%d\n",
-		       __func__, policy->cpu);
-		goto out_free_dvfs_info;
+	info = get_dvfs_info(policy->cpu);
+	if (!info) {
+		pr_err("dvfs info for cpu%d is not initialized.\n",
+		       policy->cpu);
+		return -EINVAL;
 	}
 
 	ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
 	if (ret) {
 		pr_err("failed to init cpufreq table for cpu%d: %d\n",
 		       policy->cpu, ret);
-		goto out_release_dvfs_info;
+		return ret;
 	}
 
 	ret = cpufreq_table_validate_and_show(policy, freq_table);
 	if (ret) {
 		pr_err("%s: invalid frequency table: %d\n", __func__, ret);
-		goto out_free_cpufreq_table;
+		dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
+		return ret;
 	}
 
-	/* CPUs in the same cluster share a clock and power domain. */
-	cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling);
+	cpumask_copy(policy->cpus, &info->cpus);
 	policy->driver_data = info;
 	policy->clk = info->cpu_clk;
 
 	return 0;
-
-out_free_cpufreq_table:
-	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
-
-out_release_dvfs_info:
-	mtk_cpu_dvfs_info_release(info);
-
-out_free_dvfs_info:
-	kfree(info);
-
-	return ret;
 }
 
 static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
@@ -489,8 +496,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
 
 	cpufreq_cooling_unregister(info->cdev);
 	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
-	mtk_cpu_dvfs_info_release(info);
-	kfree(info);
 
 	return 0;
 }
@@ -510,12 +515,47 @@ static struct cpufreq_driver mt8173_cpufreq_driver = {
 
 static int mt8173_cpufreq_probe(struct platform_device *pdev)
 {
-	int ret;
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list, *tmp;
+	int cpu, ret;
+
+	for_each_possible_cpu(cpu) {
+		info = get_dvfs_info(cpu);
+		if (info)
+			continue;
+
+		info = kzalloc(sizeof(*info), GFP_KERNEL);
+		if (!info) {
+			ret = -ENOMEM;
+			goto release_dvfs_info_list;
+		}
+
+		ret = mtk_cpu_dvfs_info_init(info, cpu);
+		if (ret) {
+			pr_err("%s failed to initialize dvfs info for cpu%d\n",
+			       __func__, cpu);
+			kfree(info);
+			goto release_dvfs_info_list;
+		}
+
+		list_add(&info->list_head, &dvfs_info_list);
+	}
 
 	ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
 	if (ret)
 		pr_err("failed to register mtk cpufreq driver\n");
 
+	return 0;
+
+release_dvfs_info_list:
+	list_for_each_safe(list, tmp, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		mtk_cpu_dvfs_info_release(info);
+		list_del(list);
+		kfree(info);
+	}
+
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH 1/4] cpufreq: mt8173: add CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag

[Viresh Kumar]

On 29-11-15, 16:31, Pi-Cheng Chen wrote:
> Add CPUFREQ_HAVE_GOVERNOR_PER_POLICY to have individual set of tunables
> for each cluster of MT8173.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> ---
>  drivers/cpufreq/mt8173-cpufreq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index 83001dc..c438109 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -469,7 +469,8 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
>  }
>  
>  static struct cpufreq_driver mt8173_cpufreq_driver = {
> -	.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
> +	.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> +		 CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>  	.verify = cpufreq_generic_frequency_table_verify,
>  	.target_index = mtk_cpufreq_set_target,
>  	.get = cpufreq_generic_get,

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 1/4] cpufreq: mt8173: add CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag

[Viresh Kumar]

On 29-11-15, 16:31, Pi-Cheng Chen wrote:
> Add CPUFREQ_HAVE_GOVERNOR_PER_POLICY to have individual set of tunables
> for each cluster of MT8173.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> ---
>  drivers/cpufreq/mt8173-cpufreq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index 83001dc..c438109 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -469,7 +469,8 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
>  }
>  
>  static struct cpufreq_driver mt8173_cpufreq_driver = {
> -	.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
> +	.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> +		 CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>  	.verify = cpufreq_generic_frequency_table_verify,
>  	.target_index = mtk_cpufreq_set_target,
>  	.get = cpufreq_generic_get,

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 2/4] cpufreq: mt8173: remove redundant regulator_get_voltage() call

[Viresh Kumar]

On 29-11-15, 16:31, Pi-Cheng Chen wrote:
> Remove redundant regulator_get_voltage() call to get Vsram value
> since it will be obtained later at the beginning of voltage tracking
> loop.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> ---
>  drivers/cpufreq/mt8173-cpufreq.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index c438109..750cda7 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -59,7 +59,6 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>  	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
>  
>  	old_vproc = regulator_get_voltage(proc_reg);
> -	old_vsram = regulator_get_voltage(sram_reg);
>  	/* Vsram should not exceed the maximum allowed voltage of SoC. */
>  	new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
>  

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 2/4] cpufreq: mt8173: remove redundant regulator_get_voltage() call

[Viresh Kumar]

On 29-11-15, 16:31, Pi-Cheng Chen wrote:
> Remove redundant regulator_get_voltage() call to get Vsram value
> since it will be obtained later at the beginning of voltage tracking
> loop.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> ---
>  drivers/cpufreq/mt8173-cpufreq.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index c438109..750cda7 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -59,7 +59,6 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>  	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
>  
>  	old_vproc = regulator_get_voltage(proc_reg);
> -	old_vsram = regulator_get_voltage(sram_reg);
>  	/* Vsram should not exceed the maximum allowed voltage of SoC. */
>  	new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
>  

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 3/4] cpufreq: mt8173: check return value of regulator_get_voltage() call

[Viresh Kumar]

On 29-11-15, 16:31, Pi-Cheng Chen wrote:
> Sometimes regulator_get_voltage() call returns negative values for
> reasons(e.g. underlying I2C bus timeout). Add check for the return
> values and fail out early.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> ---
>  drivers/cpufreq/mt8173-cpufreq.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index 750cda7..9d0fe37 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -59,6 +59,10 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>  	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
>  
>  	old_vproc = regulator_get_voltage(proc_reg);
> +	if (old_vproc < 0) {
> +		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
> +		return old_vproc;
> +	}
>  	/* Vsram should not exceed the maximum allowed voltage of SoC. */
>  	new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
>  
> @@ -71,7 +75,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>  		 */
>  		do {
>  			old_vsram = regulator_get_voltage(sram_reg);
> +			if (old_vsram < 0) {
> +				pr_err("%s: invalid Vsram value: %d\n",
> +				       __func__, old_vsram);
> +				return old_vsram;
> +			}
>  			old_vproc = regulator_get_voltage(proc_reg);
> +			if (old_vproc < 0) {
> +				pr_err("%s: invalid Vproc value: %d\n",
> +				       __func__, old_vproc);
> +				return old_vproc;
> +			}
>  
>  			vsram = min(new_vsram, old_vproc + MAX_VOLT_SHIFT);
>  
> @@ -116,7 +130,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>  		 */
>  		do {
>  			old_vproc = regulator_get_voltage(proc_reg);
> +			if (old_vproc < 0) {
> +				pr_err("%s: invalid Vproc value: %d\n",
> +				       __func__, old_vproc);
> +				return old_vproc;
> +			}
>  			old_vsram = regulator_get_voltage(sram_reg);
> +			if (old_vsram < 0) {
> +				pr_err("%s: invalid Vsram value: %d\n",
> +				       __func__, old_vsram);
> +				return old_vsram;
> +			}
>  
>  			vproc = max(new_vproc, old_vsram - MAX_VOLT_SHIFT);
>  			ret = regulator_set_voltage(proc_reg, vproc,
> @@ -184,6 +208,10 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>  
>  	old_freq_hz = clk_get_rate(cpu_clk);
>  	old_vproc = regulator_get_voltage(info->proc_reg);
> +	if (old_vproc < 0) {
> +		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
> +		return old_vproc;
> +	}
>  
>  	freq_hz = freq_table[index].frequency * 1000;

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 3/4] cpufreq: mt8173: check return value of regulator_get_voltage() call

[Viresh Kumar]

On 29-11-15, 16:31, Pi-Cheng Chen wrote:
> Sometimes regulator_get_voltage() call returns negative values for
> reasons(e.g. underlying I2C bus timeout). Add check for the return
> values and fail out early.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> ---
>  drivers/cpufreq/mt8173-cpufreq.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index 750cda7..9d0fe37 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -59,6 +59,10 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>  	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
>  
>  	old_vproc = regulator_get_voltage(proc_reg);
> +	if (old_vproc < 0) {
> +		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
> +		return old_vproc;
> +	}
>  	/* Vsram should not exceed the maximum allowed voltage of SoC. */
>  	new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
>  
> @@ -71,7 +75,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>  		 */
>  		do {
>  			old_vsram = regulator_get_voltage(sram_reg);
> +			if (old_vsram < 0) {
> +				pr_err("%s: invalid Vsram value: %d\n",
> +				       __func__, old_vsram);
> +				return old_vsram;
> +			}
>  			old_vproc = regulator_get_voltage(proc_reg);
> +			if (old_vproc < 0) {
> +				pr_err("%s: invalid Vproc value: %d\n",
> +				       __func__, old_vproc);
> +				return old_vproc;
> +			}
>  
>  			vsram = min(new_vsram, old_vproc + MAX_VOLT_SHIFT);
>  
> @@ -116,7 +130,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>  		 */
>  		do {
>  			old_vproc = regulator_get_voltage(proc_reg);
> +			if (old_vproc < 0) {
> +				pr_err("%s: invalid Vproc value: %d\n",
> +				       __func__, old_vproc);
> +				return old_vproc;
> +			}
>  			old_vsram = regulator_get_voltage(sram_reg);
> +			if (old_vsram < 0) {
> +				pr_err("%s: invalid Vsram value: %d\n",
> +				       __func__, old_vsram);
> +				return old_vsram;
> +			}
>  
>  			vproc = max(new_vproc, old_vsram - MAX_VOLT_SHIFT);
>  			ret = regulator_set_voltage(proc_reg, vproc,
> @@ -184,6 +208,10 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>  
>  	old_freq_hz = clk_get_rate(cpu_clk);
>  	old_vproc = regulator_get_voltage(info->proc_reg);
> +	if (old_vproc < 0) {
> +		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
> +		return old_vproc;
> +	}
>  
>  	freq_hz = freq_table[index].frequency * 1000;

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Pi-Cheng Chen]

Since the return value of ->init() of cpufreq driver is not propagated
to the device driver model now, move resources allocation into
->probe() to handle -EPROBE_DEFER properly.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
---
v1->v2:
- fix error handling path in ->probe()
---
 drivers/cpufreq/mt8173-cpufreq.c | 98 ++++++++++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 28 deletions(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 9d0fe37..257bcb9 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -41,16 +41,35 @@
  * the original PLL becomes stable at target frequency.
  */
 struct mtk_cpu_dvfs_info {
+	struct cpumask cpus;
 	struct device *cpu_dev;
 	struct regulator *proc_reg;
 	struct regulator *sram_reg;
 	struct clk *cpu_clk;
 	struct clk *inter_clk;
 	struct thermal_cooling_device *cdev;
+	struct list_head list_head;
 	int intermediate_voltage;
 	bool need_voltage_tracking;
 };
 
+static LIST_HEAD(dvfs_info_list);
+
+struct mtk_cpu_dvfs_info *get_dvfs_info(int cpu)
+{
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list;
+
+	list_for_each(list, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		if (cpumask_test_cpu(cpu, &info->cpus))
+			return info;
+	}
+
+	return NULL;
+}
+
 static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 					int new_vproc)
 {
@@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	 */
 	info->need_voltage_tracking = !IS_ERR(sram_reg);
 
+	/* CPUs in the same cluster share a clock and power domain. */
+	cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);
+
 	return 0;
 
 out_free_opp_table:
@@ -440,47 +462,32 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 	struct cpufreq_frequency_table *freq_table;
 	int ret;
 
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info)
-		return -ENOMEM;
-
-	ret = mtk_cpu_dvfs_info_init(info, policy->cpu);
-	if (ret) {
-		pr_err("%s failed to initialize dvfs info for cpu%d\n",
-		       __func__, policy->cpu);
-		goto out_free_dvfs_info;
+	info = get_dvfs_info(policy->cpu);
+	if (!info) {
+		pr_err("dvfs info for cpu%d is not initialized.\n",
+		       policy->cpu);
+		return -EINVAL;
 	}
 
 	ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
 	if (ret) {
 		pr_err("failed to init cpufreq table for cpu%d: %d\n",
 		       policy->cpu, ret);
-		goto out_release_dvfs_info;
+		return ret;
 	}
 
 	ret = cpufreq_table_validate_and_show(policy, freq_table);
 	if (ret) {
 		pr_err("%s: invalid frequency table: %d\n", __func__, ret);
-		goto out_free_cpufreq_table;
+		dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
+		return ret;
 	}
 
-	/* CPUs in the same cluster share a clock and power domain. */
-	cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling);
+	cpumask_copy(policy->cpus, &info->cpus);
 	policy->driver_data = info;
 	policy->clk = info->cpu_clk;
 
 	return 0;
-
-out_free_cpufreq_table:
-	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
-
-out_release_dvfs_info:
-	mtk_cpu_dvfs_info_release(info);
-
-out_free_dvfs_info:
-	kfree(info);
-
-	return ret;
 }
 
 static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
@@ -489,8 +496,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
 
 	cpufreq_cooling_unregister(info->cdev);
 	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
-	mtk_cpu_dvfs_info_release(info);
-	kfree(info);
 
 	return 0;
 }
@@ -510,11 +515,48 @@ static struct cpufreq_driver mt8173_cpufreq_driver = {
 
 static int mt8173_cpufreq_probe(struct platform_device *pdev)
 {
-	int ret;
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list, *tmp;
+	int cpu, ret;
+
+	for_each_possible_cpu(cpu) {
+		info = get_dvfs_info(cpu);
+		if (info)
+			continue;
+
+		info = kzalloc(sizeof(*info), GFP_KERNEL);
+		if (!info) {
+			ret = -ENOMEM;
+			goto release_dvfs_info_list;
+		}
+
+		ret = mtk_cpu_dvfs_info_init(info, cpu);
+		if (ret) {
+			pr_err("%s failed to initialize dvfs info for cpu%d\n",
+			       __func__, cpu);
+			kfree(info);
+			goto release_dvfs_info_list;
+		}
+
+		list_add(&info->list_head, &dvfs_info_list);
+	}
 
 	ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
-	if (ret)
+	if (ret) {
 		pr_err("failed to register mtk cpufreq driver\n");
+		goto release_dvfs_info_list;
+	}
+
+	return 0;
+
+release_dvfs_info_list:
+	list_for_each_safe(list, tmp, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		mtk_cpu_dvfs_info_release(info);
+		list_del(list);
+		kfree(info);
+	}
 
 	return ret;
 }
-- 
1.9.1

[PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Pi-Cheng Chen]

Since the return value of ->init() of cpufreq driver is not propagated
to the device driver model now, move resources allocation into
->probe() to handle -EPROBE_DEFER properly.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
---
v1->v2:
- fix error handling path in ->probe()
---
 drivers/cpufreq/mt8173-cpufreq.c | 98 ++++++++++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 28 deletions(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 9d0fe37..257bcb9 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -41,16 +41,35 @@
  * the original PLL becomes stable at target frequency.
  */
 struct mtk_cpu_dvfs_info {
+	struct cpumask cpus;
 	struct device *cpu_dev;
 	struct regulator *proc_reg;
 	struct regulator *sram_reg;
 	struct clk *cpu_clk;
 	struct clk *inter_clk;
 	struct thermal_cooling_device *cdev;
+	struct list_head list_head;
 	int intermediate_voltage;
 	bool need_voltage_tracking;
 };
 
+static LIST_HEAD(dvfs_info_list);
+
+struct mtk_cpu_dvfs_info *get_dvfs_info(int cpu)
+{
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list;
+
+	list_for_each(list, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		if (cpumask_test_cpu(cpu, &info->cpus))
+			return info;
+	}
+
+	return NULL;
+}
+
 static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 					int new_vproc)
 {
@@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	 */
 	info->need_voltage_tracking = !IS_ERR(sram_reg);
 
+	/* CPUs in the same cluster share a clock and power domain. */
+	cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);
+
 	return 0;
 
 out_free_opp_table:
@@ -440,47 +462,32 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 	struct cpufreq_frequency_table *freq_table;
 	int ret;
 
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info)
-		return -ENOMEM;
-
-	ret = mtk_cpu_dvfs_info_init(info, policy->cpu);
-	if (ret) {
-		pr_err("%s failed to initialize dvfs info for cpu%d\n",
-		       __func__, policy->cpu);
-		goto out_free_dvfs_info;
+	info = get_dvfs_info(policy->cpu);
+	if (!info) {
+		pr_err("dvfs info for cpu%d is not initialized.\n",
+		       policy->cpu);
+		return -EINVAL;
 	}
 
 	ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
 	if (ret) {
 		pr_err("failed to init cpufreq table for cpu%d: %d\n",
 		       policy->cpu, ret);
-		goto out_release_dvfs_info;
+		return ret;
 	}
 
 	ret = cpufreq_table_validate_and_show(policy, freq_table);
 	if (ret) {
 		pr_err("%s: invalid frequency table: %d\n", __func__, ret);
-		goto out_free_cpufreq_table;
+		dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
+		return ret;
 	}
 
-	/* CPUs in the same cluster share a clock and power domain. */
-	cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling);
+	cpumask_copy(policy->cpus, &info->cpus);
 	policy->driver_data = info;
 	policy->clk = info->cpu_clk;
 
 	return 0;
-
-out_free_cpufreq_table:
-	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
-
-out_release_dvfs_info:
-	mtk_cpu_dvfs_info_release(info);
-
-out_free_dvfs_info:
-	kfree(info);
-
-	return ret;
 }
 
 static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
@@ -489,8 +496,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
 
 	cpufreq_cooling_unregister(info->cdev);
 	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
-	mtk_cpu_dvfs_info_release(info);
-	kfree(info);
 
 	return 0;
 }
@@ -510,11 +515,48 @@ static struct cpufreq_driver mt8173_cpufreq_driver = {
 
 static int mt8173_cpufreq_probe(struct platform_device *pdev)
 {
-	int ret;
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list, *tmp;
+	int cpu, ret;
+
+	for_each_possible_cpu(cpu) {
+		info = get_dvfs_info(cpu);
+		if (info)
+			continue;
+
+		info = kzalloc(sizeof(*info), GFP_KERNEL);
+		if (!info) {
+			ret = -ENOMEM;
+			goto release_dvfs_info_list;
+		}
+
+		ret = mtk_cpu_dvfs_info_init(info, cpu);
+		if (ret) {
+			pr_err("%s failed to initialize dvfs info for cpu%d\n",
+			       __func__, cpu);
+			kfree(info);
+			goto release_dvfs_info_list;
+		}
+
+		list_add(&info->list_head, &dvfs_info_list);
+	}
 
 	ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
-	if (ret)
+	if (ret) {
 		pr_err("failed to register mtk cpufreq driver\n");
+		goto release_dvfs_info_list;
+	}
+
+	return 0;
+
+release_dvfs_info_list:
+	list_for_each_safe(list, tmp, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		mtk_cpu_dvfs_info_release(info);
+		list_del(list);
+		kfree(info);
+	}
 
 	return ret;
 }
-- 
1.9.1

Re: [PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Viresh Kumar]

On 05-12-15, 14:01, Pi-Cheng Chen wrote:
> Since the return value of ->init() of cpufreq driver is not propagated
> to the device driver model now, move resources allocation into
> ->probe() to handle -EPROBE_DEFER properly.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> ---
> v1->v2:
> - fix error handling path in ->probe()

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Viresh Kumar]

On 05-12-15, 14:01, Pi-Cheng Chen wrote:
> Since the return value of ->init() of cpufreq driver is not propagated
> to the device driver model now, move resources allocation into
> ->probe() to handle -EPROBE_DEFER properly.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> ---
> v1->v2:
> - fix error handling path in ->probe()

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Daniel Kurtz]

Hi Pi-Cheng,

On Sat, Dec 5, 2015 at 2:01 PM, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote:
> Since the return value of ->init() of cpufreq driver is not propagated
> to the device driver model now, move resources allocation into
> ->probe() to handle -EPROBE_DEFER properly.
>
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> ---
> v1->v2:
> - fix error handling path in ->probe()
> ---
>  drivers/cpufreq/mt8173-cpufreq.c | 98 ++++++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index 9d0fe37..257bcb9 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -41,16 +41,35 @@
>   * the original PLL becomes stable at target frequency.
>   */
>  struct mtk_cpu_dvfs_info {
> +       struct cpumask cpus;
>         struct device *cpu_dev;
>         struct regulator *proc_reg;
>         struct regulator *sram_reg;
>         struct clk *cpu_clk;
>         struct clk *inter_clk;
>         struct thermal_cooling_device *cdev;
> +       struct list_head list_head;
>         int intermediate_voltage;
>         bool need_voltage_tracking;
>  };
>
> +static LIST_HEAD(dvfs_info_list);

This struct is specific to this driver, so:

mtk_cpu_dvfs_info_list

> +
> +struct mtk_cpu_dvfs_info *get_dvfs_info(int cpu)

static

Also, it would be clearer to name this function something like
"mtk_cpu_dfs_info_lookup" since "mtk_cpu_dfs_info" is the type this
function looks up in the list.
It would also match the other functions such as "mtk_cpu_dvfs_info_init()".

> +{
> +       struct mtk_cpu_dvfs_info *info;
> +       struct list_head *list;
> +
> +       list_for_each(list, &dvfs_info_list) {
> +               info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
> +
> +               if (cpumask_test_cpu(cpu, &info->cpus))
> +                       return info;
> +       }
> +
> +       return NULL;
> +}
> +
>  static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>                                         int new_vproc)
>  {
> @@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>          */
>         info->need_voltage_tracking = !IS_ERR(sram_reg);
>
> +       /* CPUs in the same cluster share a clock and power domain. */
> +       cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);
> +
>         return 0;
>
>  out_free_opp_table:
> @@ -440,47 +462,32 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
>         struct cpufreq_frequency_table *freq_table;
>         int ret;
>
> -       info = kzalloc(sizeof(*info), GFP_KERNEL);
> -       if (!info)
> -               return -ENOMEM;
> -
> -       ret = mtk_cpu_dvfs_info_init(info, policy->cpu);
> -       if (ret) {
> -               pr_err("%s failed to initialize dvfs info for cpu%d\n",
> -                      __func__, policy->cpu);
> -               goto out_free_dvfs_info;
> +       info = get_dvfs_info(policy->cpu);
> +       if (!info) {
> +               pr_err("dvfs info for cpu%d is not initialized.\n",
> +                      policy->cpu);
> +               return -EINVAL;
>         }
>
>         ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);

Hmm. Does this change at init() time?  Or can we move this to probe(), too?

>         if (ret) {
>                 pr_err("failed to init cpufreq table for cpu%d: %d\n",
>                        policy->cpu, ret);
> -               goto out_release_dvfs_info;
> +               return ret;
>         }
>
>         ret = cpufreq_table_validate_and_show(policy, freq_table);
>         if (ret) {
>                 pr_err("%s: invalid frequency table: %d\n", __func__, ret);
> -               goto out_free_cpufreq_table;
> +               dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
> +               return ret;

If dev_pm_opp_init_cpufreq_table() really is needed at init() time,
then please don't change this; still use "goto out_free_cpufreq_table"
here.

>         }
>
> -       /* CPUs in the same cluster share a clock and power domain. */
> -       cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling);
> +       cpumask_copy(policy->cpus, &info->cpus);
>         policy->driver_data = info;
>         policy->clk = info->cpu_clk;
>
>         return 0;
> -
> -out_free_cpufreq_table:
> -       dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
> -
> -out_release_dvfs_info:
> -       mtk_cpu_dvfs_info_release(info);
> -
> -out_free_dvfs_info:
> -       kfree(info);
> -
> -       return ret;
>  }
>
>  static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
> @@ -489,8 +496,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
>
>         cpufreq_cooling_unregister(info->cdev);
>         dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
> -       mtk_cpu_dvfs_info_release(info);
> -       kfree(info);
>
>         return 0;
>  }
> @@ -510,11 +515,48 @@ static struct cpufreq_driver mt8173_cpufreq_driver = {
>
>  static int mt8173_cpufreq_probe(struct platform_device *pdev)
>  {
> -       int ret;
> +       struct mtk_cpu_dvfs_info *info;
> +       struct list_head *list, *tmp;
> +       int cpu, ret;
> +
> +       for_each_possible_cpu(cpu) {

Is there an iterator for cpu clusters?  Would be useful here.

Or, even better:
 * Is there a way to way to map cpu to cpu cluster ID?
 * Is there a way to count the number of clusters?
If so you can just use an array of mtk_cpu_dvfs_info, and use a cpu's
cluster ID as its index into the array.

> +               info = get_dvfs_info(cpu);
> +               if (info)
> +                       continue;

How could this ever return non-NULL?
This is probe; it won't be called twice - not unless a prior probe()
failed (and/or after .remove()), right?

> +
> +               info = kzalloc(sizeof(*info), GFP_KERNEL);

devm_kzalloc() (and you can then remove the kfree()'s below).

> +               if (!info) {
> +                       ret = -ENOMEM;
> +                       goto release_dvfs_info_list;
> +               }
> +
> +               ret = mtk_cpu_dvfs_info_init(info, cpu);
> +               if (ret) {
> +                       pr_err("%s failed to initialize dvfs info for cpu%d\n",
> +                              __func__, cpu);

dev_err(&pdev->dev, ) here and elsewhere.

-Dan

> +                       kfree(info);
> +                       goto release_dvfs_info_list;
> +               }
> +
> +               list_add(&info->list_head, &dvfs_info_list);
> +       }
>
>         ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
> -       if (ret)
> +       if (ret) {
>                 pr_err("failed to register mtk cpufreq driver\n");
> +               goto release_dvfs_info_list;
> +       }
> +
> +       return 0;
> +
> +release_dvfs_info_list:
> +       list_for_each_safe(list, tmp, &dvfs_info_list) {
> +               info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
> +
> +               mtk_cpu_dvfs_info_release(info);
> +               list_del(list);
> +               kfree(info);
> +       }
>
>         return ret;
>  }
> --
> 1.9.1
>

[PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Daniel Kurtz]

Hi Pi-Cheng,

On Sat, Dec 5, 2015 at 2:01 PM, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote:
> Since the return value of ->init() of cpufreq driver is not propagated
> to the device driver model now, move resources allocation into
> ->probe() to handle -EPROBE_DEFER properly.
>
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> ---
> v1->v2:
> - fix error handling path in ->probe()
> ---
>  drivers/cpufreq/mt8173-cpufreq.c | 98 ++++++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index 9d0fe37..257bcb9 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -41,16 +41,35 @@
>   * the original PLL becomes stable at target frequency.
>   */
>  struct mtk_cpu_dvfs_info {
> +       struct cpumask cpus;
>         struct device *cpu_dev;
>         struct regulator *proc_reg;
>         struct regulator *sram_reg;
>         struct clk *cpu_clk;
>         struct clk *inter_clk;
>         struct thermal_cooling_device *cdev;
> +       struct list_head list_head;
>         int intermediate_voltage;
>         bool need_voltage_tracking;
>  };
>
> +static LIST_HEAD(dvfs_info_list);

This struct is specific to this driver, so:

mtk_cpu_dvfs_info_list

> +
> +struct mtk_cpu_dvfs_info *get_dvfs_info(int cpu)

static

Also, it would be clearer to name this function something like
"mtk_cpu_dfs_info_lookup" since "mtk_cpu_dfs_info" is the type this
function looks up in the list.
It would also match the other functions such as "mtk_cpu_dvfs_info_init()".

> +{
> +       struct mtk_cpu_dvfs_info *info;
> +       struct list_head *list;
> +
> +       list_for_each(list, &dvfs_info_list) {
> +               info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
> +
> +               if (cpumask_test_cpu(cpu, &info->cpus))
> +                       return info;
> +       }
> +
> +       return NULL;
> +}
> +
>  static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>                                         int new_vproc)
>  {
> @@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>          */
>         info->need_voltage_tracking = !IS_ERR(sram_reg);
>
> +       /* CPUs in the same cluster share a clock and power domain. */
> +       cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);
> +
>         return 0;
>
>  out_free_opp_table:
> @@ -440,47 +462,32 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
>         struct cpufreq_frequency_table *freq_table;
>         int ret;
>
> -       info = kzalloc(sizeof(*info), GFP_KERNEL);
> -       if (!info)
> -               return -ENOMEM;
> -
> -       ret = mtk_cpu_dvfs_info_init(info, policy->cpu);
> -       if (ret) {
> -               pr_err("%s failed to initialize dvfs info for cpu%d\n",
> -                      __func__, policy->cpu);
> -               goto out_free_dvfs_info;
> +       info = get_dvfs_info(policy->cpu);
> +       if (!info) {
> +               pr_err("dvfs info for cpu%d is not initialized.\n",
> +                      policy->cpu);
> +               return -EINVAL;
>         }
>
>         ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);

Hmm. Does this change at init() time?  Or can we move this to probe(), too?

>         if (ret) {
>                 pr_err("failed to init cpufreq table for cpu%d: %d\n",
>                        policy->cpu, ret);
> -               goto out_release_dvfs_info;
> +               return ret;
>         }
>
>         ret = cpufreq_table_validate_and_show(policy, freq_table);
>         if (ret) {
>                 pr_err("%s: invalid frequency table: %d\n", __func__, ret);
> -               goto out_free_cpufreq_table;
> +               dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
> +               return ret;

If dev_pm_opp_init_cpufreq_table() really is needed at init() time,
then please don't change this; still use "goto out_free_cpufreq_table"
here.

>         }
>
> -       /* CPUs in the same cluster share a clock and power domain. */
> -       cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling);
> +       cpumask_copy(policy->cpus, &info->cpus);
>         policy->driver_data = info;
>         policy->clk = info->cpu_clk;
>
>         return 0;
> -
> -out_free_cpufreq_table:
> -       dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
> -
> -out_release_dvfs_info:
> -       mtk_cpu_dvfs_info_release(info);
> -
> -out_free_dvfs_info:
> -       kfree(info);
> -
> -       return ret;
>  }
>
>  static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
> @@ -489,8 +496,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
>
>         cpufreq_cooling_unregister(info->cdev);
>         dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
> -       mtk_cpu_dvfs_info_release(info);
> -       kfree(info);
>
>         return 0;
>  }
> @@ -510,11 +515,48 @@ static struct cpufreq_driver mt8173_cpufreq_driver = {
>
>  static int mt8173_cpufreq_probe(struct platform_device *pdev)
>  {
> -       int ret;
> +       struct mtk_cpu_dvfs_info *info;
> +       struct list_head *list, *tmp;
> +       int cpu, ret;
> +
> +       for_each_possible_cpu(cpu) {

Is there an iterator for cpu clusters?  Would be useful here.

Or, even better:
 * Is there a way to way to map cpu to cpu cluster ID?
 * Is there a way to count the number of clusters?
If so you can just use an array of mtk_cpu_dvfs_info, and use a cpu's
cluster ID as its index into the array.

> +               info = get_dvfs_info(cpu);
> +               if (info)
> +                       continue;

How could this ever return non-NULL?
This is probe; it won't be called twice - not unless a prior probe()
failed (and/or after .remove()), right?

> +
> +               info = kzalloc(sizeof(*info), GFP_KERNEL);

devm_kzalloc() (and you can then remove the kfree()'s below).

> +               if (!info) {
> +                       ret = -ENOMEM;
> +                       goto release_dvfs_info_list;
> +               }
> +
> +               ret = mtk_cpu_dvfs_info_init(info, cpu);
> +               if (ret) {
> +                       pr_err("%s failed to initialize dvfs info for cpu%d\n",
> +                              __func__, cpu);

dev_err(&pdev->dev, ) here and elsewhere.

-Dan

> +                       kfree(info);
> +                       goto release_dvfs_info_list;
> +               }
> +
> +               list_add(&info->list_head, &dvfs_info_list);
> +       }
>
>         ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
> -       if (ret)
> +       if (ret) {
>                 pr_err("failed to register mtk cpufreq driver\n");
> +               goto release_dvfs_info_list;
> +       }
> +
> +       return 0;
> +
> +release_dvfs_info_list:
> +       list_for_each_safe(list, tmp, &dvfs_info_list) {
> +               info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
> +
> +               mtk_cpu_dvfs_info_release(info);
> +               list_del(list);
> +               kfree(info);
> +       }
>
>         return ret;
>  }
> --
> 1.9.1
>

Re: [PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Pi-Cheng Chen]

Hi Daniel,

On Mon, Dec 7, 2015 at 10:44 AM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Hi Pi-Cheng,
>
> On Sat, Dec 5, 2015 at 2:01 PM, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote:
>> Since the return value of ->init() of cpufreq driver is not propagated
>> to the device driver model now, move resources allocation into
>> ->probe() to handle -EPROBE_DEFER properly.
>>
>> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
>> ---
>> v1->v2:
>> - fix error handling path in ->probe()
>> ---
>>  drivers/cpufreq/mt8173-cpufreq.c | 98 ++++++++++++++++++++++++++++------------
>>  1 file changed, 70 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
>> index 9d0fe37..257bcb9 100644
>> --- a/drivers/cpufreq/mt8173-cpufreq.c
>> +++ b/drivers/cpufreq/mt8173-cpufreq.c
>> @@ -41,16 +41,35 @@
>>   * the original PLL becomes stable at target frequency.
>>   */
>>  struct mtk_cpu_dvfs_info {
>> +       struct cpumask cpus;
>>         struct device *cpu_dev;
>>         struct regulator *proc_reg;
>>         struct regulator *sram_reg;
>>         struct clk *cpu_clk;
>>         struct clk *inter_clk;
>>         struct thermal_cooling_device *cdev;
>> +       struct list_head list_head;
>>         int intermediate_voltage;
>>         bool need_voltage_tracking;
>>  };
>>
>> +static LIST_HEAD(dvfs_info_list);
>
> This struct is specific to this driver, so:
>
> mtk_cpu_dvfs_info_list

That's fair. Both work for me.
But since it's a static variable, I prefer the shorter name since it won't
be misunderstood.

>
>> +
>> +struct mtk_cpu_dvfs_info *get_dvfs_info(int cpu)
>
> static

Will do it.

>
> Also, it would be clearer to name this function something like
> "mtk_cpu_dfs_info_lookup" since "mtk_cpu_dfs_info" is the type this
> function looks up in the list.
> It would also match the other functions such as "mtk_cpu_dvfs_info_init()".

mtk_cpu_dvfs_info_lookup() looks good to me.
WIll do it.

>
>> +{
>> +       struct mtk_cpu_dvfs_info *info;
>> +       struct list_head *list;
>> +
>> +       list_for_each(list, &dvfs_info_list) {
>> +               info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
>> +
>> +               if (cpumask_test_cpu(cpu, &info->cpus))
>> +                       return info;
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>>  static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>>                                         int new_vproc)
>>  {
>> @@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>>          */
>>         info->need_voltage_tracking = !IS_ERR(sram_reg);
>>
>> +       /* CPUs in the same cluster share a clock and power domain. */
>> +       cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);
>> +
>>         return 0;
>>
>>  out_free_opp_table:
>> @@ -440,47 +462,32 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
>>         struct cpufreq_frequency_table *freq_table;
>>         int ret;
>>
>> -       info = kzalloc(sizeof(*info), GFP_KERNEL);
>> -       if (!info)
>> -               return -ENOMEM;
>> -
>> -       ret = mtk_cpu_dvfs_info_init(info, policy->cpu);
>> -       if (ret) {
>> -               pr_err("%s failed to initialize dvfs info for cpu%d\n",
>> -                      __func__, policy->cpu);
>> -               goto out_free_dvfs_info;
>> +       info = get_dvfs_info(policy->cpu);
>> +       if (!info) {
>> +               pr_err("dvfs info for cpu%d is not initialized.\n",
>> +                      policy->cpu);
>> +               return -EINVAL;
>>         }
>>
>>         ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
>
> Hmm. Does this change at init() time?  Or can we move this to probe(), too?

It doesn't change at init() time for MT8173 since we don't add OPP
dynamically and I don't see any problem to move it. I will move it to probe().

>
>>         if (ret) {
>>                 pr_err("failed to init cpufreq table for cpu%d: %d\n",
>>                        policy->cpu, ret);
>> -               goto out_release_dvfs_info;
>> +               return ret;
>>         }
>>
>>         ret = cpufreq_table_validate_and_show(policy, freq_table);
>>         if (ret) {
>>                 pr_err("%s: invalid frequency table: %d\n", __func__, ret);
>> -               goto out_free_cpufreq_table;
>> +               dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
>> +               return ret;
>
> If dev_pm_opp_init_cpufreq_table() really is needed at init() time,
> then please don't change this; still use "goto out_free_cpufreq_table"
> here.
>
>>         }
>>
>> -       /* CPUs in the same cluster share a clock and power domain. */
>> -       cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling);
>> +       cpumask_copy(policy->cpus, &info->cpus);
>>         policy->driver_data = info;
>>         policy->clk = info->cpu_clk;
>>
>>         return 0;
>> -
>> -out_free_cpufreq_table:
>> -       dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
>> -
>> -out_release_dvfs_info:
>> -       mtk_cpu_dvfs_info_release(info);
>> -
>> -out_free_dvfs_info:
>> -       kfree(info);
>> -
>> -       return ret;
>>  }
>>
>>  static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
>> @@ -489,8 +496,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
>>
>>         cpufreq_cooling_unregister(info->cdev);
>>         dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
>> -       mtk_cpu_dvfs_info_release(info);
>> -       kfree(info);
>>
>>         return 0;
>>  }
>> @@ -510,11 +515,48 @@ static struct cpufreq_driver mt8173_cpufreq_driver = {
>>
>>  static int mt8173_cpufreq_probe(struct platform_device *pdev)
>>  {
>> -       int ret;
>> +       struct mtk_cpu_dvfs_info *info;
>> +       struct list_head *list, *tmp;
>> +       int cpu, ret;
>> +
>> +       for_each_possible_cpu(cpu) {
>
> Is there an iterator for cpu clusters?  Would be useful here.
>
> Or, even better:
>  * Is there a way to way to map cpu to cpu cluster ID?
>  * Is there a way to count the number of clusters?
> If so you can just use an array of mtk_cpu_dvfs_info, and use a cpu's
> cluster ID as its index into the array.

As I originally planned, this driver should be easily extended support
for other
Mediatek platforms. For this reason, we should not assume there's a fixed
mapping for CPU ID and cluster ID and the CPU DVFS domain and cluster neither.
Furthermore, the CPU DVFS domain sharing information will be provided by
operating-points-v2 bindings on which I am working to migrate mt8173-cpufreq
driver. We should not put the same information in two different places.

>
>> +               info = get_dvfs_info(cpu);
>> +               if (info)
>> +                       continue;
>
> How could this ever return non-NULL?
> This is probe; it won't be called twice - not unless a prior probe()
> failed (and/or after .remove()), right?

Yes. it does return non-NULL for CPU1 and CPU3.
When it goes into the loop with cpu=0, mtk_cpu_dvfs_info_init() will copy the
cluster core_sibling mask to info->cpus:

cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);

which means cpu0 and cpu1 share this struct mtk_cpu_dvfs_info variable.
So when it goes into the loop with cpu=1, get_dvfs_info() will return the shared
data structure. Similar for CPU2 and CPU3.

BTW, the shared information from core_sibling cpumask will be replaced by
operating-points-v2 bindings later.

>
>> +
>> +               info = kzalloc(sizeof(*info), GFP_KERNEL);
>
> devm_kzalloc() (and you can then remove the kfree()'s below).

Yes. Will do it.

>
>> +               if (!info) {
>> +                       ret = -ENOMEM;
>> +                       goto release_dvfs_info_list;
>> +               }
>> +
>> +               ret = mtk_cpu_dvfs_info_init(info, cpu);
>> +               if (ret) {
>> +                       pr_err("%s failed to initialize dvfs info for cpu%d\n",
>> +                              __func__, cpu);
>
> dev_err(&pdev->dev, ) here and elsewhere.

Will do it.
Thanks for reviewing.

Pi-Cheng

>
> -Dan
>
>> +                       kfree(info);
>> +                       goto release_dvfs_info_list;
>> +               }
>> +
>> +               list_add(&info->list_head, &dvfs_info_list);
>> +       }
>>
>>         ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
>> -       if (ret)
>> +       if (ret) {
>>                 pr_err("failed to register mtk cpufreq driver\n");
>> +               goto release_dvfs_info_list;
>> +       }
>> +
>> +       return 0;
>> +
>> +release_dvfs_info_list:
>> +       list_for_each_safe(list, tmp, &dvfs_info_list) {
>> +               info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
>> +
>> +               mtk_cpu_dvfs_info_release(info);
>> +               list_del(list);
>> +               kfree(info);
>> +       }
>>
>>         return ret;
>>  }
>> --
>> 1.9.1
>>

[PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Pi-Cheng Chen]

Hi Daniel,

On Mon, Dec 7, 2015 at 10:44 AM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Hi Pi-Cheng,
>
> On Sat, Dec 5, 2015 at 2:01 PM, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote:
>> Since the return value of ->init() of cpufreq driver is not propagated
>> to the device driver model now, move resources allocation into
>> ->probe() to handle -EPROBE_DEFER properly.
>>
>> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
>> ---
>> v1->v2:
>> - fix error handling path in ->probe()
>> ---
>>  drivers/cpufreq/mt8173-cpufreq.c | 98 ++++++++++++++++++++++++++++------------
>>  1 file changed, 70 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
>> index 9d0fe37..257bcb9 100644
>> --- a/drivers/cpufreq/mt8173-cpufreq.c
>> +++ b/drivers/cpufreq/mt8173-cpufreq.c
>> @@ -41,16 +41,35 @@
>>   * the original PLL becomes stable at target frequency.
>>   */
>>  struct mtk_cpu_dvfs_info {
>> +       struct cpumask cpus;
>>         struct device *cpu_dev;
>>         struct regulator *proc_reg;
>>         struct regulator *sram_reg;
>>         struct clk *cpu_clk;
>>         struct clk *inter_clk;
>>         struct thermal_cooling_device *cdev;
>> +       struct list_head list_head;
>>         int intermediate_voltage;
>>         bool need_voltage_tracking;
>>  };
>>
>> +static LIST_HEAD(dvfs_info_list);
>
> This struct is specific to this driver, so:
>
> mtk_cpu_dvfs_info_list

That's fair. Both work for me.
But since it's a static variable, I prefer the shorter name since it won't
be misunderstood.

>
>> +
>> +struct mtk_cpu_dvfs_info *get_dvfs_info(int cpu)
>
> static

Will do it.

>
> Also, it would be clearer to name this function something like
> "mtk_cpu_dfs_info_lookup" since "mtk_cpu_dfs_info" is the type this
> function looks up in the list.
> It would also match the other functions such as "mtk_cpu_dvfs_info_init()".

mtk_cpu_dvfs_info_lookup() looks good to me.
WIll do it.

>
>> +{
>> +       struct mtk_cpu_dvfs_info *info;
>> +       struct list_head *list;
>> +
>> +       list_for_each(list, &dvfs_info_list) {
>> +               info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
>> +
>> +               if (cpumask_test_cpu(cpu, &info->cpus))
>> +                       return info;
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>>  static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>>                                         int new_vproc)
>>  {
>> @@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>>          */
>>         info->need_voltage_tracking = !IS_ERR(sram_reg);
>>
>> +       /* CPUs in the same cluster share a clock and power domain. */
>> +       cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);
>> +
>>         return 0;
>>
>>  out_free_opp_table:
>> @@ -440,47 +462,32 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
>>         struct cpufreq_frequency_table *freq_table;
>>         int ret;
>>
>> -       info = kzalloc(sizeof(*info), GFP_KERNEL);
>> -       if (!info)
>> -               return -ENOMEM;
>> -
>> -       ret = mtk_cpu_dvfs_info_init(info, policy->cpu);
>> -       if (ret) {
>> -               pr_err("%s failed to initialize dvfs info for cpu%d\n",
>> -                      __func__, policy->cpu);
>> -               goto out_free_dvfs_info;
>> +       info = get_dvfs_info(policy->cpu);
>> +       if (!info) {
>> +               pr_err("dvfs info for cpu%d is not initialized.\n",
>> +                      policy->cpu);
>> +               return -EINVAL;
>>         }
>>
>>         ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
>
> Hmm. Does this change at init() time?  Or can we move this to probe(), too?

It doesn't change at init() time for MT8173 since we don't add OPP
dynamically and I don't see any problem to move it. I will move it to probe().

>
>>         if (ret) {
>>                 pr_err("failed to init cpufreq table for cpu%d: %d\n",
>>                        policy->cpu, ret);
>> -               goto out_release_dvfs_info;
>> +               return ret;
>>         }
>>
>>         ret = cpufreq_table_validate_and_show(policy, freq_table);
>>         if (ret) {
>>                 pr_err("%s: invalid frequency table: %d\n", __func__, ret);
>> -               goto out_free_cpufreq_table;
>> +               dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
>> +               return ret;
>
> If dev_pm_opp_init_cpufreq_table() really is needed at init() time,
> then please don't change this; still use "goto out_free_cpufreq_table"
> here.
>
>>         }
>>
>> -       /* CPUs in the same cluster share a clock and power domain. */
>> -       cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling);
>> +       cpumask_copy(policy->cpus, &info->cpus);
>>         policy->driver_data = info;
>>         policy->clk = info->cpu_clk;
>>
>>         return 0;
>> -
>> -out_free_cpufreq_table:
>> -       dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
>> -
>> -out_release_dvfs_info:
>> -       mtk_cpu_dvfs_info_release(info);
>> -
>> -out_free_dvfs_info:
>> -       kfree(info);
>> -
>> -       return ret;
>>  }
>>
>>  static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
>> @@ -489,8 +496,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
>>
>>         cpufreq_cooling_unregister(info->cdev);
>>         dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
>> -       mtk_cpu_dvfs_info_release(info);
>> -       kfree(info);
>>
>>         return 0;
>>  }
>> @@ -510,11 +515,48 @@ static struct cpufreq_driver mt8173_cpufreq_driver = {
>>
>>  static int mt8173_cpufreq_probe(struct platform_device *pdev)
>>  {
>> -       int ret;
>> +       struct mtk_cpu_dvfs_info *info;
>> +       struct list_head *list, *tmp;
>> +       int cpu, ret;
>> +
>> +       for_each_possible_cpu(cpu) {
>
> Is there an iterator for cpu clusters?  Would be useful here.
>
> Or, even better:
>  * Is there a way to way to map cpu to cpu cluster ID?
>  * Is there a way to count the number of clusters?
> If so you can just use an array of mtk_cpu_dvfs_info, and use a cpu's
> cluster ID as its index into the array.

As I originally planned, this driver should be easily extended support
for other
Mediatek platforms. For this reason, we should not assume there's a fixed
mapping for CPU ID and cluster ID and the CPU DVFS domain and cluster neither.
Furthermore, the CPU DVFS domain sharing information will be provided by
operating-points-v2 bindings on which I am working to migrate mt8173-cpufreq
driver. We should not put the same information in two different places.

>
>> +               info = get_dvfs_info(cpu);
>> +               if (info)
>> +                       continue;
>
> How could this ever return non-NULL?
> This is probe; it won't be called twice - not unless a prior probe()
> failed (and/or after .remove()), right?

Yes. it does return non-NULL for CPU1 and CPU3.
When it goes into the loop with cpu=0, mtk_cpu_dvfs_info_init() will copy the
cluster core_sibling mask to info->cpus:

cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);

which means cpu0 and cpu1 share this struct mtk_cpu_dvfs_info variable.
So when it goes into the loop with cpu=1, get_dvfs_info() will return the shared
data structure. Similar for CPU2 and CPU3.

BTW, the shared information from core_sibling cpumask will be replaced by
operating-points-v2 bindings later.

>
>> +
>> +               info = kzalloc(sizeof(*info), GFP_KERNEL);
>
> devm_kzalloc() (and you can then remove the kfree()'s below).

Yes. Will do it.

>
>> +               if (!info) {
>> +                       ret = -ENOMEM;
>> +                       goto release_dvfs_info_list;
>> +               }
>> +
>> +               ret = mtk_cpu_dvfs_info_init(info, cpu);
>> +               if (ret) {
>> +                       pr_err("%s failed to initialize dvfs info for cpu%d\n",
>> +                              __func__, cpu);
>
> dev_err(&pdev->dev, ) here and elsewhere.

Will do it.
Thanks for reviewing.

Pi-Cheng

>
> -Dan
>
>> +                       kfree(info);
>> +                       goto release_dvfs_info_list;
>> +               }
>> +
>> +               list_add(&info->list_head, &dvfs_info_list);
>> +       }
>>
>>         ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
>> -       if (ret)
>> +       if (ret) {
>>                 pr_err("failed to register mtk cpufreq driver\n");
>> +               goto release_dvfs_info_list;
>> +       }
>> +
>> +       return 0;
>> +
>> +release_dvfs_info_list:
>> +       list_for_each_safe(list, tmp, &dvfs_info_list) {
>> +               info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
>> +
>> +               mtk_cpu_dvfs_info_release(info);
>> +               list_del(list);
>> +               kfree(info);
>> +       }
>>
>>         return ret;
>>  }
>> --
>> 1.9.1
>>

Re: [PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Viresh Kumar]

On 07-12-15, 20:42, Pi-Cheng Chen wrote:
> >>         ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
> >
> > Hmm. Does this change at init() time?  Or can we move this to probe(), too?
> 
> It doesn't change at init() time for MT8173 since we don't add OPP
> dynamically and I don't see any problem to move it. I will move it to probe().

No. None of this belongs to probe(), we do it there just to handle the
EPROBE_DEFER thing. Please don't move anything that isn't required to
be.

-- 
viresh

[PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Viresh Kumar]

On 07-12-15, 20:42, Pi-Cheng Chen wrote:
> >>         ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
> >
> > Hmm. Does this change at init() time?  Or can we move this to probe(), too?
> 
> It doesn't change at init() time for MT8173 since we don't add OPP
> dynamically and I don't see any problem to move it. I will move it to probe().

No. None of this belongs to probe(), we do it there just to handle the
EPROBE_DEFER thing. Please don't move anything that isn't required to
be.

-- 
viresh

Re: [PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Rafael J. Wysocki]

On Saturday, December 05, 2015 02:01:46 PM Pi-Cheng Chen wrote:
> Since the return value of ->init() of cpufreq driver is not propagated
> to the device driver model now, move resources allocation into
> ->probe() to handle -EPROBE_DEFER properly.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>

I'm expecting a v3 here.  Please let me know if I should apply this instead.

Thanks,
Rafael

[PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()

[Rafael J. Wysocki]

On Saturday, December 05, 2015 02:01:46 PM Pi-Cheng Chen wrote:
> Since the return value of ->init() of cpufreq driver is not propagated
> to the device driver model now, move resources allocation into
> ->probe() to handle -EPROBE_DEFER properly.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>

I'm expecting a v3 here.  Please let me know if I should apply this instead.

Thanks,
Rafael

[PATCH v3] cpufreq: mt8173: move resources allocation into ->probe()

[Pi-Cheng Chen]

Since the return value of ->init() of cpufreq driver is not propagated
to the device driver model now, move resources allocation into
->probe() to handle -EPROBE_DEFER properly.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
v2->v3:
- rename get_dvfs_info() as mtk_cpu_dvfs_info_lookup()
- fix some minor issues of v2
v1->v2:
- fix error handling path in ->probe()
---
 drivers/cpufreq/mt8173-cpufreq.c | 92 +++++++++++++++++++++++++++++-----------
 1 file changed, 68 insertions(+), 24 deletions(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 9d0fe37..fd601b9 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -41,16 +41,35 @@
  * the original PLL becomes stable at target frequency.
  */
 struct mtk_cpu_dvfs_info {
+	struct cpumask cpus;
 	struct device *cpu_dev;
 	struct regulator *proc_reg;
 	struct regulator *sram_reg;
 	struct clk *cpu_clk;
 	struct clk *inter_clk;
 	struct thermal_cooling_device *cdev;
+	struct list_head list_head;
 	int intermediate_voltage;
 	bool need_voltage_tracking;
 };
 
+static LIST_HEAD(dvfs_info_list);
+
+static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_lookup(int cpu)
+{
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list;
+
+	list_for_each(list, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		if (cpumask_test_cpu(cpu, &info->cpus))
+			return info;
+	}
+
+	return NULL;
+}
+
 static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 					int new_vproc)
 {
@@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	 */
 	info->need_voltage_tracking = !IS_ERR(sram_reg);
 
+	/* CPUs in the same cluster share a clock and power domain. */
+	cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);
+
 	return 0;
 
 out_free_opp_table:
@@ -440,22 +462,18 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 	struct cpufreq_frequency_table *freq_table;
 	int ret;
 
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info)
-		return -ENOMEM;
-
-	ret = mtk_cpu_dvfs_info_init(info, policy->cpu);
-	if (ret) {
-		pr_err("%s failed to initialize dvfs info for cpu%d\n",
-		       __func__, policy->cpu);
-		goto out_free_dvfs_info;
+	info = mtk_cpu_dvfs_info_lookup(policy->cpu);
+	if (!info) {
+		pr_err("dvfs info for cpu%d is not initialized.\n",
+		       policy->cpu);
+		return -EINVAL;
 	}
 
 	ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
 	if (ret) {
 		pr_err("failed to init cpufreq table for cpu%d: %d\n",
 		       policy->cpu, ret);
-		goto out_release_dvfs_info;
+		return ret;
 	}
 
 	ret = cpufreq_table_validate_and_show(policy, freq_table);
@@ -464,8 +482,7 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 		goto out_free_cpufreq_table;
 	}
 
-	/* CPUs in the same cluster share a clock and power domain. */
-	cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling);
+	cpumask_copy(policy->cpus, &info->cpus);
 	policy->driver_data = info;
 	policy->clk = info->cpu_clk;
 
@@ -473,13 +490,6 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 
 out_free_cpufreq_table:
 	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
-
-out_release_dvfs_info:
-	mtk_cpu_dvfs_info_release(info);
-
-out_free_dvfs_info:
-	kfree(info);
-
 	return ret;
 }
 
@@ -489,8 +499,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
 
 	cpufreq_cooling_unregister(info->cdev);
 	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
-	mtk_cpu_dvfs_info_release(info);
-	kfree(info);
 
 	return 0;
 }
@@ -510,11 +518,47 @@ static struct cpufreq_driver mt8173_cpufreq_driver = {
 
 static int mt8173_cpufreq_probe(struct platform_device *pdev)
 {
-	int ret;
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list, *tmp;
+	int cpu, ret;
+
+	for_each_possible_cpu(cpu) {
+		info = mtk_cpu_dvfs_info_lookup(cpu);
+		if (info)
+			continue;
+
+		info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+		if (!info) {
+			ret = -ENOMEM;
+			goto release_dvfs_info_list;
+		}
+
+		ret = mtk_cpu_dvfs_info_init(info, cpu);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"failed to initialize dvfs info for cpu%d\n",
+				cpu);
+			goto release_dvfs_info_list;
+		}
+
+		list_add(&info->list_head, &dvfs_info_list);
+	}
 
 	ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
-	if (ret)
-		pr_err("failed to register mtk cpufreq driver\n");
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register mtk cpufreq driver\n");
+		goto release_dvfs_info_list;
+	}
+
+	return 0;
+
+release_dvfs_info_list:
+	list_for_each_safe(list, tmp, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		mtk_cpu_dvfs_info_release(info);
+		list_del(list);
+	}
 
 	return ret;
 }
-- 
1.9.1

[PATCH v3] cpufreq: mt8173: move resources allocation into ->probe()

[Pi-Cheng Chen]

Since the return value of ->init() of cpufreq driver is not propagated
to the device driver model now, move resources allocation into
->probe() to handle -EPROBE_DEFER properly.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
v2->v3:
- rename get_dvfs_info() as mtk_cpu_dvfs_info_lookup()
- fix some minor issues of v2
v1->v2:
- fix error handling path in ->probe()
---
 drivers/cpufreq/mt8173-cpufreq.c | 92 +++++++++++++++++++++++++++++-----------
 1 file changed, 68 insertions(+), 24 deletions(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 9d0fe37..fd601b9 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -41,16 +41,35 @@
  * the original PLL becomes stable at target frequency.
  */
 struct mtk_cpu_dvfs_info {
+	struct cpumask cpus;
 	struct device *cpu_dev;
 	struct regulator *proc_reg;
 	struct regulator *sram_reg;
 	struct clk *cpu_clk;
 	struct clk *inter_clk;
 	struct thermal_cooling_device *cdev;
+	struct list_head list_head;
 	int intermediate_voltage;
 	bool need_voltage_tracking;
 };
 
+static LIST_HEAD(dvfs_info_list);
+
+static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_lookup(int cpu)
+{
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list;
+
+	list_for_each(list, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		if (cpumask_test_cpu(cpu, &info->cpus))
+			return info;
+	}
+
+	return NULL;
+}
+
 static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 					int new_vproc)
 {
@@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	 */
 	info->need_voltage_tracking = !IS_ERR(sram_reg);
 
+	/* CPUs in the same cluster share a clock and power domain. */
+	cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling);
+
 	return 0;
 
 out_free_opp_table:
@@ -440,22 +462,18 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 	struct cpufreq_frequency_table *freq_table;
 	int ret;
 
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info)
-		return -ENOMEM;
-
-	ret = mtk_cpu_dvfs_info_init(info, policy->cpu);
-	if (ret) {
-		pr_err("%s failed to initialize dvfs info for cpu%d\n",
-		       __func__, policy->cpu);
-		goto out_free_dvfs_info;
+	info = mtk_cpu_dvfs_info_lookup(policy->cpu);
+	if (!info) {
+		pr_err("dvfs info for cpu%d is not initialized.\n",
+		       policy->cpu);
+		return -EINVAL;
 	}
 
 	ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
 	if (ret) {
 		pr_err("failed to init cpufreq table for cpu%d: %d\n",
 		       policy->cpu, ret);
-		goto out_release_dvfs_info;
+		return ret;
 	}
 
 	ret = cpufreq_table_validate_and_show(policy, freq_table);
@@ -464,8 +482,7 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 		goto out_free_cpufreq_table;
 	}
 
-	/* CPUs in the same cluster share a clock and power domain. */
-	cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling);
+	cpumask_copy(policy->cpus, &info->cpus);
 	policy->driver_data = info;
 	policy->clk = info->cpu_clk;
 
@@ -473,13 +490,6 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 
 out_free_cpufreq_table:
 	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
-
-out_release_dvfs_info:
-	mtk_cpu_dvfs_info_release(info);
-
-out_free_dvfs_info:
-	kfree(info);
-
 	return ret;
 }
 
@@ -489,8 +499,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
 
 	cpufreq_cooling_unregister(info->cdev);
 	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
-	mtk_cpu_dvfs_info_release(info);
-	kfree(info);
 
 	return 0;
 }
@@ -510,11 +518,47 @@ static struct cpufreq_driver mt8173_cpufreq_driver = {
 
 static int mt8173_cpufreq_probe(struct platform_device *pdev)
 {
-	int ret;
+	struct mtk_cpu_dvfs_info *info;
+	struct list_head *list, *tmp;
+	int cpu, ret;
+
+	for_each_possible_cpu(cpu) {
+		info = mtk_cpu_dvfs_info_lookup(cpu);
+		if (info)
+			continue;
+
+		info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+		if (!info) {
+			ret = -ENOMEM;
+			goto release_dvfs_info_list;
+		}
+
+		ret = mtk_cpu_dvfs_info_init(info, cpu);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"failed to initialize dvfs info for cpu%d\n",
+				cpu);
+			goto release_dvfs_info_list;
+		}
+
+		list_add(&info->list_head, &dvfs_info_list);
+	}
 
 	ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
-	if (ret)
-		pr_err("failed to register mtk cpufreq driver\n");
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register mtk cpufreq driver\n");
+		goto release_dvfs_info_list;
+	}
+
+	return 0;
+
+release_dvfs_info_list:
+	list_for_each_safe(list, tmp, &dvfs_info_list) {
+		info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+		mtk_cpu_dvfs_info_release(info);
+		list_del(list);
+	}
 
 	return ret;
 }
-- 
1.9.1

Re: [PATCH v3] cpufreq: mt8173: move resources allocation into ->probe()

[Rafael J. Wysocki]

On Thursday, December 10, 2015 11:48:13 AM Pi-Cheng Chen wrote:
> Since the return value of ->init() of cpufreq driver is not propagated
> to the device driver model now, move resources allocation into
> ->probe() to handle -EPROBE_DEFER properly.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Applied along with the [1-3] in this series.

Thanks,
Rafael

[PATCH v3] cpufreq: mt8173: move resources allocation into ->probe()

[Rafael J. Wysocki]

On Thursday, December 10, 2015 11:48:13 AM Pi-Cheng Chen wrote:
> Since the return value of ->init() of cpufreq driver is not propagated
> to the device driver model now, move resources allocation into
> ->probe() to handle -EPROBE_DEFER properly.
> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Applied along with the [1-3] in this series.

Thanks,
Rafael