[PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()
Pi-Cheng Chen
pi-cheng.chen at linaro.org
Mon Dec 7 04:42:19 PST 2015
Hi Daniel,
On Mon, Dec 7, 2015 at 10:44 AM, Daniel Kurtz <djkurtz at chromium.org> wrote:
> Hi Pi-Cheng,
>
> On Sat, Dec 5, 2015 at 2:01 PM, Pi-Cheng Chen <pi-cheng.chen at 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 at 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
>>
More information about the Linux-mediatek
mailing list