| Message ID | 20220422075239.16437-6-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
On 22-04-22, 15:52, 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 Why the extra ">" here ? > proper actions when receiving events of disable and voltage adjustment. > > One of the user for this opp notifier is MediaTek SVS. > The MediaTek Smart Voltage Scaling (SVS) is a hardware which calculates > suitable SVS bank voltages to OPP voltage table. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 92 +++++++++++++++++++++++++++--- > 1 file changed, 84 insertions(+), 8 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > +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) { I don't see any call to dev_pm_opp_adjust_voltage() for your platform, how is this ever going to get called ? > + 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); IIUC, then you are trying to change the frequency if a currently used OPP is getting removed ? In that case, this problem is generic enough not to be fixed in a end driver. This should be fixed in core somehow.
On Mon, 2022-04-25 at 10:50 +0530, Viresh Kumar wrote: > On 22-04-22, 15:52, 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 > > Why the extra ">" here ? > > > proper actions when receiving events of disable and voltage > > adjustment. > > > > One of the user for this opp notifier is MediaTek SVS. > > The MediaTek Smart Voltage Scaling (SVS) is a hardware which > > calculates > > suitable SVS bank voltages to OPP voltage table. > > > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 92 > > +++++++++++++++++++++++++++--- > > 1 file changed, 84 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > +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) { > > I don't see any call to dev_pm_opp_adjust_voltage() for your > platform, how is > this ever going to get called ? > Hello Viresh, Thanks for your review! These two event OPP_EVENT_ADJUST_VOLTAGE and OPP_EVENT_DISABLE are basically called by mediatek svs drivers. It's not merged in mainline kernel and you can reger to [1]. [1]: https://lore.kernel.org/all/20220420102044.10832-4-roger.lu@mediatek.com/ For OPP_EVENT_ADJUST_VOLTAGE: mediatek svs will optimize the voltage, so mediatek svs will call dev_pm_opp_adjust_voltage() to adjust frequency and 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); > > IIUC, then you are trying to change the frequency if a currently used > OPP is > getting removed ? In that case, this problem is generic enough not to > be fixed > in a end driver. This should be fixed in core somehow. > For OPP_EVENT_DISABLE: when mediatek svs needs to be disable, the voltage should recover to original voltage and frequency. BRs, Rex
On Mon, 2022-04-25 at 10:50 +0530, Viresh Kumar wrote: > On 22-04-22, 15:52, 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 > > Why the extra ">" here ? Hello Viresh, Sorry that I miss this message. I will remove it in next version. Thanks. BRs, Rex > > > proper actions when receiving events of disable and voltage > > adjustment. > > > > One of the user for this opp notifier is MediaTek SVS. > > The MediaTek Smart Voltage Scaling (SVS) is a hardware which > > calculates > > suitable SVS bank voltages to OPP voltage table. > > > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 92 > > +++++++++++++++++++++++++++--- > > 1 file changed, 84 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > +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) { > > I don't see any call to dev_pm_opp_adjust_voltage() for your > platform, how is > this ever going to get called ? > > > + 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); > > IIUC, then you are trying to change the frequency if a currently used > OPP is > getting removed ? In that case, this problem is generic enough not to > be fixed > in a end driver. This should be fixed in core somehow. >
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index 1688cf68849c..37c02785a01f 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)