| Message ID | 20220408045908.21671-7-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
Il 08/04/22 06:58, Rex-BC Chen ha scritto: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > We found the buck voltage may not be exactly the same with what we set > because CPU may share the same buck with other module. > Therefore, we need to record the previous desired value instead of reading > it from regulators. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index dc4a87e68940..472f4de29e5f 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { > struct list_head list_head; > int intermediate_voltage; > bool need_voltage_tracking; > + int old_vproc; > }; > > static LIST_HEAD(dvfs_info_list); > @@ -190,11 +191,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) > { > + int ret; > + > if (info->need_voltage_tracking) > - return mtk_cpufreq_voltage_tracking(info, vproc); > + ret = mtk_cpufreq_voltage_tracking(info, vproc); > else > - return regulator_set_voltage(info->proc_reg, vproc, > - vproc + VOLT_TOL); > + ret = regulator_set_voltage(info->proc_reg, vproc, > + MAX_VOLT_LIMIT); > + if (!ret) > + info->old_vproc = vproc; > + > + return ret; > } > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > inter_vproc = info->intermediate_voltage; > > - old_freq_hz = clk_get_rate(cpu_clk); > - old_vproc = regulator_get_voltage(info->proc_reg); > - if (old_vproc < 0) { > - pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc); > - return old_vproc; > - } > - > freq_hz = freq_table[index].frequency * 1000; > - > opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); > if (IS_ERR(opp)) { > pr_err("cpu%d: failed to find OPP for %ld\n", > @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > vproc = dev_pm_opp_get_voltage(opp); > dev_pm_opp_put(opp); > > + old_freq_hz = clk_get_rate(cpu_clk); > + old_vproc = info->old_vproc; > + if (old_vproc == 0) > + old_vproc = regulator_get_voltage(info->proc_reg); > + if (old_vproc < 0) { > + dev_err(cpu_dev, "%s: invalid Vproc value: %d\n", > + __func__, old_vproc); > + return old_vproc; > + } From my understandment, if this fails once, it fails forever! info->old_vproc is set only if info->need_voltage_tracking is true, and only in mtk_cpufreq_set_voltage(): this function is called only after the checks that you've introduced there, and that's on previously stored values. While this was fine in the previous version, because it was always calling regulator_get_voltage(), here it's not. I think that a good option here is to: old_vproc = info->old_vproc; if (old_vproc <= 0) old_vproc = regulator_get_voltage(info->proc_reg); if (old_vproc < 0) { dev_err and return } ...or, if this is not applicable, we should still find another way to not let this driver to simply fail forever in case anything goes wrong. Regards, Angelo
On 08-04-22, 12:58, Rex-BC Chen wrote: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > We found the buck voltage may not be exactly the same with what we set > because CPU may share the same buck with other module. > Therefore, we need to record the previous desired value instead of reading > it from regulators. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index dc4a87e68940..472f4de29e5f 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { > struct list_head list_head; > int intermediate_voltage; > bool need_voltage_tracking; > + int old_vproc; I like prev_vproc better somehow, but it is up to you to name it :) > }; > > static LIST_HEAD(dvfs_info_list); > @@ -190,11 +191,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) > { > + int ret; > + > if (info->need_voltage_tracking) > - return mtk_cpufreq_voltage_tracking(info, vproc); > + ret = mtk_cpufreq_voltage_tracking(info, vproc); > else > - return regulator_set_voltage(info->proc_reg, vproc, > - vproc + VOLT_TOL); > + ret = regulator_set_voltage(info->proc_reg, vproc, > + MAX_VOLT_LIMIT); > + if (!ret) > + info->old_vproc = vproc; > + > + return ret; > } > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > inter_vproc = info->intermediate_voltage; > > - old_freq_hz = clk_get_rate(cpu_clk); > - old_vproc = regulator_get_voltage(info->proc_reg); > - if (old_vproc < 0) { > - pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc); > - return old_vproc; > - } > - Why did you move it down from here? I think it was fine to error out early if voltage isn't available. > freq_hz = freq_table[index].frequency * 1000; > - > opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); > if (IS_ERR(opp)) { > pr_err("cpu%d: failed to find OPP for %ld\n", > @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > vproc = dev_pm_opp_get_voltage(opp); > dev_pm_opp_put(opp); > > + old_freq_hz = clk_get_rate(cpu_clk); > + old_vproc = info->old_vproc; > + if (old_vproc == 0) > + old_vproc = regulator_get_voltage(info->proc_reg); > + if (old_vproc < 0) { > + dev_err(cpu_dev, "%s: invalid Vproc value: %d\n", > + __func__, old_vproc); > + return old_vproc; > + } > + > /* > * If the new voltage or the intermediate voltage is higher than the > * current voltage, scale up voltage first. > -- > 2.18.0
On Mon, 2022-04-11 at 08:56 +0530, Viresh Kumar wrote: > On 08-04-22, 12:58, Rex-BC Chen wrote: > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > We found the buck voltage may not be exactly the same with what we > > set > > because CPU may share the same buck with other module. > > Therefore, we need to record the previous desired value instead of > > reading > > it from regulators. > > > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++------- > > ---- > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index dc4a87e68940..472f4de29e5f 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { > > struct list_head list_head; > > int intermediate_voltage; > > bool need_voltage_tracking; > > + int old_vproc; > > I like prev_vproc better somehow, but it is up to you to name it :) Hello Viresh, Thanks for your review. I will modify this as prev_vproc in next version. > > > }; > > > > static LIST_HEAD(dvfs_info_list); > > @@ -190,11 +191,17 @@ static int > > mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > > > static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, > > int vproc) > > { > > + int ret; > > + > > if (info->need_voltage_tracking) > > - return mtk_cpufreq_voltage_tracking(info, vproc); > > + ret = mtk_cpufreq_voltage_tracking(info, vproc); > > else > > - return regulator_set_voltage(info->proc_reg, vproc, > > - vproc + VOLT_TOL); > > + ret = regulator_set_voltage(info->proc_reg, vproc, > > + MAX_VOLT_LIMIT); > > + if (!ret) > > + info->old_vproc = vproc; > > + > > + return ret; > > } > > > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > > > inter_vproc = info->intermediate_voltage; > > > > - old_freq_hz = clk_get_rate(cpu_clk); > > - old_vproc = regulator_get_voltage(info->proc_reg); > > - if (old_vproc < 0) { > > - pr_err("%s: invalid Vproc value: %d\n", __func__, > > old_vproc); > > - return old_vproc; > > - } > > - > > Why did you move it down from here? I think it was fine to error out > early if voltage isn't available. I will move them to original position in next version. Thanks! BRs, Rex > > > freq_hz = freq_table[index].frequency * 1000; > > - > > opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); > > if (IS_ERR(opp)) { > > pr_err("cpu%d: failed to find OPP for %ld\n", > > @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > vproc = dev_pm_opp_get_voltage(opp); > > dev_pm_opp_put(opp); > > > > + old_freq_hz = clk_get_rate(cpu_clk); > > + old_vproc = info->old_vproc; > > + if (old_vproc == 0) > > + old_vproc = regulator_get_voltage(info->proc_reg); > > + if (old_vproc < 0) { > > + dev_err(cpu_dev, "%s: invalid Vproc value: %d\n", > > + __func__, old_vproc); > > + return old_vproc; > > + } > > + > > /* > > * If the new voltage or the intermediate voltage is higher > > than the > > * current voltage, scale up voltage first. > > -- > > 2.18.0 > >
On Fri, 2022-04-08 at 15:36 +0200, AngeloGioacchino Del Regno wrote: > Il 08/04/22 06:58, Rex-BC Chen ha scritto: > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > We found the buck voltage may not be exactly the same with what we > > set > > because CPU may share the same buck with other module. > > Therefore, we need to record the previous desired value instead of > > reading > > it from regulators. > > > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++---- > > ------- > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index dc4a87e68940..472f4de29e5f 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { > > struct list_head list_head; > > int intermediate_voltage; > > bool need_voltage_tracking; > > + int old_vproc; > > }; > > > > static LIST_HEAD(dvfs_info_list); > > @@ -190,11 +191,17 @@ static int > > mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > > > static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info > > *info, int vproc) > > { > > + int ret; > > + > > if (info->need_voltage_tracking) > > - return mtk_cpufreq_voltage_tracking(info, vproc); > > + ret = mtk_cpufreq_voltage_tracking(info, vproc); > > else > > - return regulator_set_voltage(info->proc_reg, vproc, > > - vproc + VOLT_TOL); > > + ret = regulator_set_voltage(info->proc_reg, vproc, > > + MAX_VOLT_LIMIT); > > + if (!ret) > > + info->old_vproc = vproc; > > + > > + return ret; > > } > > > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > > > inter_vproc = info->intermediate_voltage; > > > > - old_freq_hz = clk_get_rate(cpu_clk); > > - old_vproc = regulator_get_voltage(info->proc_reg); > > - if (old_vproc < 0) { > > - pr_err("%s: invalid Vproc value: %d\n", __func__, > > old_vproc); > > - return old_vproc; > > - } > > - > > freq_hz = freq_table[index].frequency * 1000; > > - > > opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); > > if (IS_ERR(opp)) { > > pr_err("cpu%d: failed to find OPP for %ld\n", > > @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > vproc = dev_pm_opp_get_voltage(opp); > > dev_pm_opp_put(opp); > > > > + old_freq_hz = clk_get_rate(cpu_clk); > > + old_vproc = info->old_vproc; > > + if (old_vproc == 0) > > + old_vproc = regulator_get_voltage(info->proc_reg); > > + if (old_vproc < 0) { > > + dev_err(cpu_dev, "%s: invalid Vproc value: %d\n", > > + __func__, old_vproc); > > + return old_vproc; > > + } > > From my understandment, if this fails once, it fails forever! > > info->old_vproc is set only if info->need_voltage_tracking is true, > and only > in mtk_cpufreq_set_voltage(): this function is called only after the > checks > that you've introduced there, and that's on previously stored values. > While this was fine in the previous version, because it was always > calling > regulator_get_voltage(), here it's not. > > I think that a good option here is to: > > old_vproc = info->old_vproc; > if (old_vproc <= 0) > old_vproc = regulator_get_voltage(info->proc_reg); > if (old_vproc < 0) { > dev_err and return > } > > ...or, if this is not applicable, we should still find another way to > not > let this driver to simply fail forever in case anything goes wrong. > > Regards, > Angelo Hello Angelo, Yes, your concern is right. I will add this in next version. if (old_vproc <= 0) old_vproc = regulator_get_voltage(info->proc_reg); if (old_vproc < 0) { dev_err and return } BRs, Rex
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index dc4a87e68940..472f4de29e5f 100644 --- a/drivers/cpufreq/mediatek-cpufreq.c +++ b/drivers/cpufreq/mediatek-cpufreq.c @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { struct list_head list_head; int intermediate_voltage; bool need_voltage_tracking; + int old_vproc; }; static LIST_HEAD(dvfs_info_list); @@ -190,11 +191,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) { + int ret; + if (info->need_voltage_tracking) - return mtk_cpufreq_voltage_tracking(info, vproc); + ret = mtk_cpufreq_voltage_tracking(info, vproc); else - return regulator_set_voltage(info->proc_reg, vproc, - vproc + VOLT_TOL); + ret = regulator_set_voltage(info->proc_reg, vproc, + MAX_VOLT_LIMIT); + if (!ret) + info->old_vproc = vproc; + + return ret; } static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, inter_vproc = info->intermediate_voltage; - old_freq_hz = clk_get_rate(cpu_clk); - old_vproc = regulator_get_voltage(info->proc_reg); - if (old_vproc < 0) { - pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc); - return old_vproc; - } - freq_hz = freq_table[index].frequency * 1000; - opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); if (IS_ERR(opp)) { pr_err("cpu%d: failed to find OPP for %ld\n", @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, vproc = dev_pm_opp_get_voltage(opp); dev_pm_opp_put(opp); + old_freq_hz = clk_get_rate(cpu_clk); + old_vproc = info->old_vproc; + if (old_vproc == 0) + old_vproc = regulator_get_voltage(info->proc_reg); + if (old_vproc < 0) { + dev_err(cpu_dev, "%s: invalid Vproc value: %d\n", + __func__, old_vproc); + return old_vproc; + } + /* * If the new voltage or the intermediate voltage is higher than the * current voltage, scale up voltage first.