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

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

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; }

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

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; }