| Message ID | 20220504130540.5902-3-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
On 04-05-22, 21:05, Rex-BC Chen wrote: > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > >From this opp notifier, cpufreq should listen to opp notification and do What happened with the extra ">" here ? > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > { > struct device *cpu_dev; > @@ -396,6 +458,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > info->intermediate_voltage = dev_pm_opp_get_voltage(opp); > dev_pm_opp_put(opp); > > + info->opp_cpu = cpu; > + info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier; > + ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb); > + if (ret) { > + dev_err(cpu_dev, "cpu%d: failed to register opp notifier\n", cpu); > + goto out_disable_inter_clock; > + } > + > + mutex_init(&info->reg_lock); You should always initialize a resource before its users. The notifier callback, which can get called right after dev_pm_opp_register_notifier() returns, will use this mutex. > + info->opp_freq = clk_get_rate(info->cpu_clk); > + > /* > * If SRAM regulator is present, software "voltage tracking" is needed > * for this CPU power domain. > @@ -451,6 +524,9 @@ static void mtk_cpu_dvfs_info_release(struct mtk_cpu_dvfs_info *info) > } > > dev_pm_opp_of_cpumask_remove_table(&info->cpus); > + > + if (!IS_ERR_OR_NULL(info->cpu_dev)) cpu_dev can never be error here. > + dev_pm_opp_unregister_notifier(info->cpu_dev, &info->opp_nb); > } > > static int mtk_cpufreq_init(struct cpufreq_policy *policy) I also asked you last time to stack things in a order so they are easier for me to apply. Bugfixes, followed by simple cleanups, which don't make behavioral changes, followed by real patches. Now you have sent this patch at an early stage, which blocks me from applying anything after this. I can see the earlier comments weren't all considered, and it doesn't look nice.
On Thu, 2022-05-05 at 14:13 +0530, Viresh Kumar wrote: > On 04-05-22, 21:05, Rex-BC Chen wrote: > > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > > > > From this opp notifier, cpufreq should listen to opp notification > > > and do > > What happened with the extra ">" here ? > Hello Viresh, Sorry for this. I don't know why it appear this ">". In my patch, there is no ">" here. I will check this.. > > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, > > int cpu) > > { > > struct device *cpu_dev; > > @@ -396,6 +458,17 @@ static int mtk_cpu_dvfs_info_init(struct > > mtk_cpu_dvfs_info *info, int cpu) > > info->intermediate_voltage = dev_pm_opp_get_voltage(opp); > > dev_pm_opp_put(opp); > > > > + info->opp_cpu = cpu; > > + info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier; > > + ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb); > > + if (ret) { > > + dev_err(cpu_dev, "cpu%d: failed to register opp > > notifier\n", cpu); > > + goto out_disable_inter_clock; > > + } > > + > > + mutex_init(&info->reg_lock); > > You should always initialize a resource before its users. The > notifier > callback, which can get called right after > dev_pm_opp_register_notifier() returns, will use this mutex. > ok, I will move mutex_init(&info->reg_lock) before dev_pm_opp_register_notifier(). > > + info->opp_freq = clk_get_rate(info->cpu_clk); > > + > > /* > > * If SRAM regulator is present, software "voltage tracking" is > > needed > > * for this CPU power domain. > > @@ -451,6 +524,9 @@ static void mtk_cpu_dvfs_info_release(struct > > mtk_cpu_dvfs_info *info) > > } > > > > dev_pm_opp_of_cpumask_remove_table(&info->cpus); > > + > > + if (!IS_ERR_OR_NULL(info->cpu_dev)) > > cpu_dev can never be error here. ok, I will drop this. > > > + dev_pm_opp_unregister_notifier(info->cpu_dev, &info- > > >opp_nb); > > } > > > > static int mtk_cpufreq_init(struct cpufreq_policy *policy) > > I also asked you last time to stack things in a order so they are > easier for me to apply. Bugfixes, followed by simple cleanups, which > don't make behavioral changes, followed by real patches. > > Now you have sent this patch at an early stage, which blocks me from > applying anything after this. > > I can see the earlier comments weren't all considered, and it doesn't > look nice. > Sorry for inconvenience. I will move "cpufreq: mediatek: Move voltage limits to platform data" before this patch. The order will be: //cleanup and bug fix new patch for add "platform_device_unregister(cpufreq_pdev);" cpufreq: mediatek: Move voltage limits to platform data //driver refinement cpufreq: mediatek: Refine mtk_cpufreq_voltage_tracking() //new feature cpufreq: mediatek: Add opp notification support cpufreq: mediatek: Link CCI device to CPU //support new soc cpufreq: mediatek: Add support for MT8186 //dts arm64: dts: mediatek: Add opp table and clock property for MT8183 cpufreq arm64: dts: mediatek: Add MediaTek CCI node for MT8183 arm64: dts: mediatek: Add mediatek, cci property for MT8183 cpufreq BRs, Rex
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index 901042e9a240..363ebecb2c80 100644 --- a/drivers/cpufreq/mediatek-cpufreq.c +++ b/drivers/cpufreq/mediatek-cpufreq.c @@ -41,6 +41,11 @@ struct mtk_cpu_dvfs_info { int intermediate_voltage; bool need_voltage_tracking; int pre_vproc; + /* Avoid race condition for regulators between notify and policy */ + struct mutex reg_lock; + struct notifier_block opp_nb; + unsigned int opp_cpu; + unsigned long opp_freq; }; static LIST_HEAD(dvfs_info_list); @@ -221,6 +226,8 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, pre_freq_hz = clk_get_rate(cpu_clk); + mutex_lock(&info->reg_lock); + if (unlikely(info->pre_vproc <= 0)) pre_vproc = regulator_get_voltage(info->proc_reg); else @@ -253,7 +260,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, dev_err(cpu_dev, "cpu%d: failed to scale up voltage!\n", policy->cpu); mtk_cpufreq_set_voltage(info, pre_vproc); - return ret; + goto out; } } @@ -263,8 +270,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, dev_err(cpu_dev, "cpu%d: failed to re-parent cpu clock!\n", policy->cpu); mtk_cpufreq_set_voltage(info, pre_vproc); - WARN_ON(1); - return ret; + goto out; } /* Set the original PLL to target rate. */ @@ -274,7 +280,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, "cpu%d: failed to scale cpu clock rate!\n", policy->cpu); clk_set_parent(cpu_clk, armpll); mtk_cpufreq_set_voltage(info, pre_vproc); - return ret; + goto out; } /* Set parent of CPU clock back to the original PLL. */ @@ -283,8 +289,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, dev_err(cpu_dev, "cpu%d: failed to re-parent cpu clock!\n", policy->cpu); mtk_cpufreq_set_voltage(info, inter_vproc); - WARN_ON(1); - return ret; + goto out; } /* @@ -299,15 +304,72 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, clk_set_parent(cpu_clk, info->inter_clk); clk_set_rate(armpll, pre_freq_hz); clk_set_parent(cpu_clk, armpll); - return ret; + goto out; } } - return 0; + info->opp_freq = freq_hz; + +out: + mutex_unlock(&info->reg_lock); + + return ret; } #define DYNAMIC_POWER "dynamic-power-coefficient" +static int mtk_cpufreq_opp_notifier(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct dev_pm_opp *opp = data; + struct dev_pm_opp *new_opp; + struct mtk_cpu_dvfs_info *info; + unsigned long freq, volt; + struct cpufreq_policy *policy; + int ret = 0; + + info = container_of(nb, struct mtk_cpu_dvfs_info, opp_nb); + + if (event == OPP_EVENT_ADJUST_VOLTAGE) { + freq = dev_pm_opp_get_freq(opp); + + mutex_lock(&info->reg_lock); + if (info->opp_freq == freq) { + volt = dev_pm_opp_get_voltage(opp); + ret = mtk_cpufreq_set_voltage(info, volt); + if (ret) + dev_err(info->cpu_dev, + "failed to scale voltage: %d\n", ret); + } + mutex_unlock(&info->reg_lock); + } else if (event == OPP_EVENT_DISABLE) { + freq = dev_pm_opp_get_freq(opp); + + /* case of current opp item is disabled */ + if (info->opp_freq == freq) { + freq = 1; + new_opp = dev_pm_opp_find_freq_ceil(info->cpu_dev, + &freq); + if (IS_ERR(new_opp)) { + dev_err(info->cpu_dev, + "all opp items are disabled\n"); + ret = PTR_ERR(new_opp); + return notifier_from_errno(ret); + } + + dev_pm_opp_put(new_opp); + policy = cpufreq_cpu_get(info->opp_cpu); + if (policy) { + cpufreq_driver_target(policy, freq / 1000, + CPUFREQ_RELATION_L); + cpufreq_cpu_put(policy); + } + } + } + + return notifier_from_errno(ret); +} + static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) { struct device *cpu_dev; @@ -396,6 +458,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) info->intermediate_voltage = dev_pm_opp_get_voltage(opp); dev_pm_opp_put(opp); + info->opp_cpu = cpu; + info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier; + ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb); + if (ret) { + dev_err(cpu_dev, "cpu%d: failed to register opp notifier\n", cpu); + goto out_disable_inter_clock; + } + + mutex_init(&info->reg_lock); + info->opp_freq = clk_get_rate(info->cpu_clk); + /* * If SRAM regulator is present, software "voltage tracking" is needed * for this CPU power domain. @@ -451,6 +524,9 @@ static void mtk_cpu_dvfs_info_release(struct mtk_cpu_dvfs_info *info) } dev_pm_opp_of_cpumask_remove_table(&info->cpus); + + if (!IS_ERR_OR_NULL(info->cpu_dev)) + dev_pm_opp_unregister_notifier(info->cpu_dev, &info->opp_nb); } static int mtk_cpufreq_init(struct cpufreq_policy *policy)