| Message ID | 1616499241-4906-3-git-send-email-andrew-sh.cheng@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add cpufreq and cci devfreq for mt8183, and SVS support | expand |
On 23-03-21, 19:33, Andrew-sh.Cheng wrote: > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > Need to enable regulator, > so that the max/min requested value will be recorded > even it is not applied right away. > > Intermediate clock is not always enabled by ccf in different projects, > so cpufreq should enable it by itself. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 33 +++++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index f2e491b25b07..432368707ea6 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c > @@ -350,6 +350,11 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > ret = PTR_ERR(proc_reg); > goto out_free_resources; > } > + ret = regulator_enable(proc_reg); > + if (ret) { > + pr_warn("enable vproc for cpu%d fail\n", cpu); > + goto out_free_resources; > + } Regulators are enabled by OPP core as well now, you sure this is required ?
On Tue, 2021-03-30 at 10:06 +0530, Viresh Kumar wrote: > On 23-03-21, 19:33, Andrew-sh.Cheng wrote: > > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > > > Need to enable regulator, > > so that the max/min requested value will be recorded > > even it is not applied right away. > > > > Intermediate clock is not always enabled by ccf in different projects, > > so cpufreq should enable it by itself. > > > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 33 +++++++++++++++++++++++++++++---- > > 1 file changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > > index f2e491b25b07..432368707ea6 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > @@ -350,6 +350,11 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > > ret = PTR_ERR(proc_reg); > > goto out_free_resources; > > } > > + ret = regulator_enable(proc_reg); > > + if (ret) { > > + pr_warn("enable vproc for cpu%d fail\n", cpu); > > + goto out_free_resources; > > + } > > Regulators are enabled by OPP core as well now, you sure this is > required ? > Hi Viresh, Yes. As you mentioned, it will be enable by OPP core. Per discuss with hotplug owner and regulator owner, they suggest that "users should not suppose other module, will enable regulators for them". They suggest to add enable_regulator here.
On 31-03-21, 13:21, andrew-sh.cheng wrote: > Hi Viresh, > Yes. > As you mentioned, it will be enable by OPP core. > > Per discuss with hotplug owner and regulator owner, > they suggest that "users should not suppose other module, will enable > regulators for them". > They suggest to add enable_regulator here. Which is fine if the modules in question aren't closely related to each other, but OPP core and cpufreq are too closely bound to each other. So much that the cpufreq driver can depend on the OPP core for doing it. Though I won't Nack a patch just for that, but it was just a suggestion.
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index f2e491b25b07..432368707ea6 100644 --- a/drivers/cpufreq/mediatek-cpufreq.c +++ b/drivers/cpufreq/mediatek-cpufreq.c @@ -350,6 +350,11 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) ret = PTR_ERR(proc_reg); goto out_free_resources; } + ret = regulator_enable(proc_reg); + if (ret) { + pr_warn("enable vproc for cpu%d fail\n", cpu); + goto out_free_resources; + } /* Both presence and absence of sram regulator are valid cases. */ sram_reg = regulator_get_exclusive(cpu_dev, "sram"); @@ -368,13 +373,21 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) goto out_free_resources; } + ret = clk_prepare_enable(cpu_clk); + if (ret) + goto out_free_opp_table; + + ret = clk_prepare_enable(inter_clk); + if (ret) + goto out_disable_mux_clock; + /* Search a safe voltage for intermediate frequency. */ rate = clk_get_rate(inter_clk); opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); if (IS_ERR(opp)) { pr_err("failed to get intermediate opp for cpu%d\n", cpu); ret = PTR_ERR(opp); - goto out_free_opp_table; + goto out_disable_inter_clock; } info->intermediate_voltage = dev_pm_opp_get_voltage(opp); dev_pm_opp_put(opp); @@ -393,6 +406,12 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) return 0; +out_disable_inter_clock: + clk_disable_unprepare(inter_clk); + +out_disable_mux_clock: + clk_disable_unprepare(cpu_clk); + out_free_opp_table: dev_pm_opp_of_cpumask_remove_table(&info->cpus); @@ -411,14 +430,20 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) static void mtk_cpu_dvfs_info_release(struct mtk_cpu_dvfs_info *info) { - if (!IS_ERR(info->proc_reg)) + if (!IS_ERR(info->proc_reg)) { + regulator_disable(info->proc_reg); regulator_put(info->proc_reg); + } if (!IS_ERR(info->sram_reg)) regulator_put(info->sram_reg); - if (!IS_ERR(info->cpu_clk)) + if (!IS_ERR(info->cpu_clk)) { + clk_disable_unprepare(info->cpu_clk); clk_put(info->cpu_clk); - if (!IS_ERR(info->inter_clk)) + } + if (!IS_ERR(info->inter_clk)) { + clk_disable_unprepare(info->inter_clk); clk_put(info->inter_clk); + } dev_pm_opp_of_cpumask_remove_table(&info->cpus); }