Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1171019 > unrolled thread
| Started by | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| First post | 2015-06-24 03:00 +0200 |
| Last post | 2015-06-24 11:20 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver Viresh Kumar <viresh.kumar@linaro.org> - 2015-06-24 03:00 +0200
Re: [PATCH 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver Pi-Cheng Chen <pi-cheng.chen@linaro.org> - 2015-06-24 10:50 +0200
Re: [PATCH 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver Viresh Kumar <viresh.kumar@linaro.org> - 2015-06-24 11:00 +0200
Re: [PATCH 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver Pi-Cheng Chen <pi-cheng.chen@linaro.org> - 2015-06-24 11:20 +0200
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2015-06-24 03:00 +0200 |
| Subject | Re: [PATCH 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver |
| Message-ID | <pEHcJ-2vM-7@gated-at.bofh.it> |
On 23-06-15, 23:25, Pi-Cheng Chen wrote:
> On Mon, Jun 22, 2015 at 7:45 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >> +static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_get(int cpu)
> >
> > A very bad name to a routine with very specific functionality.
>
> Would get_mtk_cpu_dvfs_info() or cpu_to_mtk_cpu_dvfs_info() be better?
>
> >
> >> +{
> >> + struct mtk_cpu_dvfs_info *info;
> >> + struct list_head *list;
> >> +
> >> + list_for_each(list, &cpu_dvfs_info_list) {
> >> + info = to_mtk_cpu_dvfs_info(list);
> >> +
> >> + if (cpumask_test_cpu(cpu, info->cpus))
> >> + return info;
> >> + }
Firstly there is no need of extra long name for static routines. Just
keep them simple. i.e. no need of mtk_cpu_.
And then I was a bit confused about the functionality earlier and your
initial name wasn't that bad. Sorry :(
So maybe just, get_dvfs_info().
> >> + freq_hz = freq_table[index].frequency * 1000;
> >
> > A blank line here.
>
> Will remove it.
>
I was asking you to add it :)
> >> + rcu_read_lock();
> >> +static int mtk_cpu_dvfs_info_init(int cpu)
> >> +{
> >> + struct device *cpu_dev;
> >> + struct regulator *proc_reg = ERR_PTR(-ENODEV);
> >> + struct regulator *sram_reg = ERR_PTR(-ENODEV);
> >> + struct clk *cpu_clk = ERR_PTR(-ENODEV);
> >> + struct clk *inter_clk = ERR_PTR(-ENODEV);
> >> + struct mtk_cpu_dvfs_info *info;
> >> + struct cpufreq_frequency_table *freq_table;
> >> + struct dev_pm_opp *opp;
> >> + unsigned long rate;
> >> + int ret;
> >> +
> >> + cpu_dev = get_cpu_device(cpu);
> >> + if (!cpu_dev) {
> >> + pr_err("failed to get cpu%d device\n", cpu);
> >> + return -ENODEV;
> >> + }
> >> +
> >> + ret = of_init_opp_table(cpu_dev);
> >> + if (ret) {
> >> + pr_warn("no OPP table for cpu%d\n", cpu);
> >> + return ret;
> >> + }
> >> +
> >> + cpu_clk = clk_get(cpu_dev, "cpu");
> >> + if (IS_ERR(cpu_clk)) {
> >> + if (PTR_ERR(cpu_clk) == -EPROBE_DEFER)
> >> + pr_warn("cpu clk for cpu%d not ready, retry.\n", cpu);
> >> + else
> >> + pr_err("failed to get cpu clk for cpu%d\n", cpu);
> >> +
> >> + ret = PTR_ERR(cpu_clk);
> >> + goto out_free_opp_table;
> >> + }
> >> +
> >> + inter_clk = clk_get(cpu_dev, "intermediate");
> >> + if (IS_ERR(inter_clk)) {
> >> + if (PTR_ERR(inter_clk) == -EPROBE_DEFER)
> >> + pr_warn("intermediate clk for cpu%d not ready, retry.\n",
> >> + cpu);
> >> + else
> >> + pr_err("failed to get intermediate clk for cpu%d\n",
> >> + cpu);
> >> +
> >> + ret = PTR_ERR(cpu_clk);
> >> + goto out_free_resources;
> >> + }
> >> +
> >> + proc_reg = regulator_get_exclusive(cpu_dev, "proc");
> >> + if (IS_ERR(proc_reg)) {
> >> + if (PTR_ERR(proc_reg) == -EPROBE_DEFER)
> >> + pr_warn("proc regulator for cpu%d not ready, retry.\n",
> >> + cpu);
> >> + else
> >> + pr_err("failed to get proc regulator for cpu%d\n",
> >> + cpu);
> >> +
> >> + ret = PTR_ERR(proc_reg);
> >> + goto out_free_resources;
> >> + }
> >> +
> >> + /* Both presence and absence of sram regulator are valid cases. */
> >> + sram_reg = regulator_get_exclusive(cpu_dev, "sram");
> >> +
> >> + info = kzalloc(sizeof(*info), GFP_KERNEL);
> >> + if (!info) {
> >> + ret = -ENOMEM;
> >> + goto out_free_resources;
> >> + }
> >> +
> >> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
> >> + if (ret) {
> >> + pr_err("failed to init cpufreq table for cpu%d: %d\n",
> >> + cpu, ret);
> >> + goto out_free_mtk_cpu_dvfs_info;
> >> + }
> >> +
> >> + if (!alloc_cpumask_var(&info->cpus, GFP_KERNEL))
> >
> > Why getting into such trouble? What about just saving policy's pointer
> > in info and use it for freq_table, cpus mask, etc ?
>
> This function is called from driver probe function. At this moment, cpufreq
> driver is not yet registered and the policy struct for each cluster is not yet
> allocated.
This should be called from ->init() callback of the cpufreq driver.
> Also, the mtk_cpu_dvfs_info structs for all clusters are stored in a list
> after probe done. cpus mask is used to find out the proper info struct
> for corresponding cpu.
Yeah, but this will be exactly same to policy->related_cpus, just use
that. Why allocate another mask for same purpose?
> For freq_table, I simply don't want to free and allocate the table
> every time when cpu is unplugged and plugged so I just allocate it
> in the probe function call path, and cache it in the info struct.
->exit() will be called only when all the CPUs from a policy are
hotplugged out. Not on every hotplug. So you probably have 4 big and 4
little cores. So policy for big cluster will be removed only when all
the CPUs are down. And so no point storing things from probe.
Or is there anything special with your usecase?
> But for module_platform_driver(mt8173_cpufreq_platdrv), I saw many
> built-in drivers in kernel call module_platform_driver() also so I use it.
That was wrong for them as well :)
> Would it be better to use platform_driver_register() instead?
Hmm..
--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Pi-Cheng Chen <pi-cheng.chen@linaro.org> |
|---|---|
| Date | 2015-06-24 10:50 +0200 |
| Message-ID | <pEOxC-4Lc-37@gated-at.bofh.it> |
| In reply to | #1171019 |
Hi Viresh,
On Wed, Jun 24, 2015 at 8:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 23-06-15, 23:25, Pi-Cheng Chen wrote:
>> On Mon, Jun 22, 2015 at 7:45 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> >> +static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_get(int cpu)
>> >
>> > A very bad name to a routine with very specific functionality.
>>
>> Would get_mtk_cpu_dvfs_info() or cpu_to_mtk_cpu_dvfs_info() be better?
>>
>> >
>> >> +{
>> >> + struct mtk_cpu_dvfs_info *info;
>> >> + struct list_head *list;
>> >> +
>> >> + list_for_each(list, &cpu_dvfs_info_list) {
>> >> + info = to_mtk_cpu_dvfs_info(list);
>> >> +
>> >> + if (cpumask_test_cpu(cpu, info->cpus))
>> >> + return info;
>> >> + }
>
> Firstly there is no need of extra long name for static routines. Just
> keep them simple. i.e. no need of mtk_cpu_.
>
> And then I was a bit confused about the functionality earlier and your
> initial name wasn't that bad. Sorry :(
That's ok. :)
>
> So maybe just, get_dvfs_info().
Yes. Thanks. I'll pick this one.
>
>> >> + freq_hz = freq_table[index].frequency * 1000;
>> >
>> > A blank line here.
>>
>> Will remove it.
>>
>
> I was asking you to add it :)
I thought you were asking to remove the previous one originally.
Sorry for my misunderstanding.
Will add it.
>
>> >> + rcu_read_lock();
>
>
>> >> +static int mtk_cpu_dvfs_info_init(int cpu)
>> >> +{
>> >> + struct device *cpu_dev;
>> >> + struct regulator *proc_reg = ERR_PTR(-ENODEV);
>> >> + struct regulator *sram_reg = ERR_PTR(-ENODEV);
>> >> + struct clk *cpu_clk = ERR_PTR(-ENODEV);
>> >> + struct clk *inter_clk = ERR_PTR(-ENODEV);
>> >> + struct mtk_cpu_dvfs_info *info;
>> >> + struct cpufreq_frequency_table *freq_table;
>> >> + struct dev_pm_opp *opp;
>> >> + unsigned long rate;
>> >> + int ret;
>> >> +
>> >> + cpu_dev = get_cpu_device(cpu);
>> >> + if (!cpu_dev) {
>> >> + pr_err("failed to get cpu%d device\n", cpu);
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + ret = of_init_opp_table(cpu_dev);
>> >> + if (ret) {
>> >> + pr_warn("no OPP table for cpu%d\n", cpu);
>> >> + return ret;
>> >> + }
>> >> +
>> >> + cpu_clk = clk_get(cpu_dev, "cpu");
>> >> + if (IS_ERR(cpu_clk)) {
>> >> + if (PTR_ERR(cpu_clk) == -EPROBE_DEFER)
>> >> + pr_warn("cpu clk for cpu%d not ready, retry.\n", cpu);
>> >> + else
>> >> + pr_err("failed to get cpu clk for cpu%d\n", cpu);
>> >> +
>> >> + ret = PTR_ERR(cpu_clk);
>> >> + goto out_free_opp_table;
>> >> + }
>> >> +
>> >> + inter_clk = clk_get(cpu_dev, "intermediate");
>> >> + if (IS_ERR(inter_clk)) {
>> >> + if (PTR_ERR(inter_clk) == -EPROBE_DEFER)
>> >> + pr_warn("intermediate clk for cpu%d not ready, retry.\n",
>> >> + cpu);
>> >> + else
>> >> + pr_err("failed to get intermediate clk for cpu%d\n",
>> >> + cpu);
>> >> +
>> >> + ret = PTR_ERR(cpu_clk);
>> >> + goto out_free_resources;
>> >> + }
>> >> +
>> >> + proc_reg = regulator_get_exclusive(cpu_dev, "proc");
>> >> + if (IS_ERR(proc_reg)) {
>> >> + if (PTR_ERR(proc_reg) == -EPROBE_DEFER)
>> >> + pr_warn("proc regulator for cpu%d not ready, retry.\n",
>> >> + cpu);
>> >> + else
>> >> + pr_err("failed to get proc regulator for cpu%d\n",
>> >> + cpu);
>> >> +
>> >> + ret = PTR_ERR(proc_reg);
>> >> + goto out_free_resources;
>> >> + }
>> >> +
>> >> + /* Both presence and absence of sram regulator are valid cases. */
>> >> + sram_reg = regulator_get_exclusive(cpu_dev, "sram");
>> >> +
>> >> + info = kzalloc(sizeof(*info), GFP_KERNEL);
>> >> + if (!info) {
>> >> + ret = -ENOMEM;
>> >> + goto out_free_resources;
>> >> + }
>> >> +
>> >> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
>> >> + if (ret) {
>> >> + pr_err("failed to init cpufreq table for cpu%d: %d\n",
>> >> + cpu, ret);
>> >> + goto out_free_mtk_cpu_dvfs_info;
>> >> + }
>> >> +
>> >> + if (!alloc_cpumask_var(&info->cpus, GFP_KERNEL))
>> >
>> > Why getting into such trouble? What about just saving policy's pointer
>> > in info and use it for freq_table, cpus mask, etc ?
>>
>> This function is called from driver probe function. At this moment, cpufreq
>> driver is not yet registered and the policy struct for each cluster is not yet
>> allocated.
>
> This should be called from ->init() callback of the cpufreq driver.
Some reasons I did this in probe(). Please see below.
>
>> Also, the mtk_cpu_dvfs_info structs for all clusters are stored in a list
>> after probe done. cpus mask is used to find out the proper info struct
>> for corresponding cpu.
>
> Yeah, but this will be exactly same to policy->related_cpus, just use
> that. Why allocate another mask for same purpose?
Yes. I agree. Please see the reasons why I did it below.
>
>> For freq_table, I simply don't want to free and allocate the table
>> every time when cpu is unplugged and plugged so I just allocate it
>> in the probe function call path, and cache it in the info struct.
>
> ->exit() will be called only when all the CPUs from a policy are
> hotplugged out. Not on every hotplug. So you probably have 4 big and 4
> little cores. So policy for big cluster will be removed only when all
> the CPUs are down. And so no point storing things from probe.
>
> Or is there anything special with your usecase?
One reason to put those initialization and resource allocation in probe is
that it's easier to handle the return value -PROBE_DEFER from clock
and regulator framework when trying to get clocks and regulators
consumed by cpufreq driver.
The other reason is when the whole system is resuming from suspend,
the other subsystem e.g. i2c on which cpufreq driver depends might not
ready yet during cpufreq driver initialization. In this case, the cpufreq
driver will be blocked when trying to get resources e.g. regulator on i2c
bus, and the whole system will stuck for seconds.
For the two reasons above, I try to do all resource initialization and
allocation in probe(), and leave less things to do in init() as possible.
Do you have any suggestion for this?
>
>> But for module_platform_driver(mt8173_cpufreq_platdrv), I saw many
>> built-in drivers in kernel call module_platform_driver() also so I use it.
>
> That was wrong for them as well :)
>
>> Would it be better to use platform_driver_register() instead?
>
> Hmm..
Will fix it.
Thanks.
Pi-Cheng
>
> --
> viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2015-06-24 11:00 +0200 |
| Message-ID | <pEOHh-4WG-21@gated-at.bofh.it> |
| In reply to | #1171199 |
On 24-06-15, 16:44, Pi-Cheng Chen wrote:
> One reason to put those initialization and resource allocation in probe is
> that it's easier to handle the return value -PROBE_DEFER from clock
> and regulator framework when trying to get clocks and regulators
> consumed by cpufreq driver.
This is the sequence of events if you move these things to ->init().
- your driver's probe()
-> cpufreq_register_driver()
-> init()
-> clk/reg get, failed, return -EPROBE_DEFER
And the failure here will be propagated to probe(). So, it should
work IMHO.
> The other reason is when the whole system is resuming from suspend,
> the other subsystem e.g. i2c on which cpufreq driver depends might not
> ready yet during cpufreq driver initialization. In this case, the cpufreq
> driver will be blocked when trying to get resources e.g. regulator on i2c
> bus, and the whole system will stuck for seconds.
That's something else you must investigate on. This dependency should
be resolved in some other way, I thought DT might have taken care of
such dependencies.
--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Pi-Cheng Chen <pi-cheng.chen@linaro.org> |
|---|---|
| Date | 2015-06-24 11:20 +0200 |
| Message-ID | <pEP0B-5yL-1@gated-at.bofh.it> |
| In reply to | #1171203 |
On Wed, Jun 24, 2015 at 4:56 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 24-06-15, 16:44, Pi-Cheng Chen wrote: >> One reason to put those initialization and resource allocation in probe is >> that it's easier to handle the return value -PROBE_DEFER from clock >> and regulator framework when trying to get clocks and regulators >> consumed by cpufreq driver. > > This is the sequence of events if you move these things to ->init(). > > - your driver's probe() > -> cpufreq_register_driver() > -> init() > -> clk/reg get, failed, return -EPROBE_DEFER > > And the failure here will be propagated to probe(). So, it should > work IMHO. I will try it. > >> The other reason is when the whole system is resuming from suspend, >> the other subsystem e.g. i2c on which cpufreq driver depends might not >> ready yet during cpufreq driver initialization. In this case, the cpufreq >> driver will be blocked when trying to get resources e.g. regulator on i2c >> bus, and the whole system will stuck for seconds. > > That's something else you must investigate on. This dependency should > be resolved in some other way, I thought DT might have taken care of > such dependencies. I will check if it could be solved in some other way. Thanks. Pi-Cheng > > -- > viresh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web