| Message ID | 20220408045908.21671-8-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
Il 08/04/22 06:59, Rex-BC Chen ha scritto: > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > The Smart Voltage Scaling (SVS) is a hardware which calculates suitable > SVS bank voltages to OPP voltage table. > > When the SVS is enabled, cpufreq should listen to opp notification and do > proper actions when receiving events of disable and voltage adjustment. > > 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> > --- > drivers/cpufreq/mediatek-cpufreq.c | 90 +++++++++++++++++++++++++++--- > 1 file changed, 82 insertions(+), 8 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index 472f4de29e5f..1369da62780a 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c ..snip.. > + > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > { > struct device *cpu_dev; > @@ -392,6 +455,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); You are registering this notifier, but never unregistering it. Please unregister it in error conditions and also upon runtime removal of the mediatek-cpufreq module. -> dev_pm_opp_unregister_notifier() Regards, Angelo > + if (ret) { > + dev_warn(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.
Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > The Smart Voltage Scaling (SVS) is a hardware which calculates suitable > SVS bank voltages to OPP voltage table. > > When the SVS is enabled, cpufreq should listen to opp notification and do > proper actions when receiving events of disable and voltage adjustment. So listenting for OPP notifications should be done only when SVS is enabled... [...] > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > { > struct device *cpu_dev; > @@ -392,6 +455,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); ...but here youlisten to OPP notifications unconditionally. Seems there should be a check whether SVS is enabled before deciding to register. Kevin
On Fri, 2022-04-08 at 13:29 -0700, Kevin Hilman wrote: > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > > > The Smart Voltage Scaling (SVS) is a hardware which calculates > > suitable > > SVS bank voltages to OPP voltage table. > > > > When the SVS is enabled, cpufreq should listen to opp notification > > and do > > proper actions when receiving events of disable and voltage > > adjustment. > > So listenting for OPP notifications should be done only when SVS is > enabled... > Hello Kevin, Thanks for your review. Yes, the OPP notification is only called from MediaTek SVS. > [...] > > > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, > > int cpu) > > { > > struct device *cpu_dev; > > @@ -392,6 +455,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); > > ...but here youlisten to OPP notifications unconditionally. Seems > there > should be a check whether SVS is enabled before deciding to register. > > Kevin Do you think it's ok that we wrap it with the SVS Kconfig define? like #ifdef CONFIG_MTK_SVS mtk_cpufreq_opp_notifier() ... dev_pm_opp_register_notifier() #endif SVS series: message-id:20220221063939.14969-4-roger.lu@mediatek.com BRs, Rex
On Fri, 2022-04-08 at 15:36 +0200, AngeloGioacchino Del Regno wrote: > Il 08/04/22 06:59, Rex-BC Chen ha scritto: > > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > > > The Smart Voltage Scaling (SVS) is a hardware which calculates > > suitable > > SVS bank voltages to OPP voltage table. > > > > When the SVS is enabled, cpufreq should listen to opp notification > > and do > > proper actions when receiving events of disable and voltage > > adjustment. > > > > 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> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 90 > > +++++++++++++++++++++++++++--- > > 1 file changed, 82 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index 472f4de29e5f..1369da62780a 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > ..snip.. > > > + > > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, > > int cpu) > > { > > struct device *cpu_dev; > > @@ -392,6 +455,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); > > You are registering this notifier, but never unregistering it. > Please unregister it in error conditions and also upon runtime > removal of the > mediatek-cpufreq module. > > -> dev_pm_opp_unregister_notifier() > > Regards, > Angelo Hello Angelo, Thanks for your review. I will add this inside mtk_cpu_dvfs_info_release() in next version. BRs, Rex > > > + if (ret) { > > + dev_warn(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. > >
Hi Rex, Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > On Fri, 2022-04-08 at 13:29 -0700, Kevin Hilman wrote: >> Rex-BC Chen <rex-bc.chen@mediatek.com> writes: >> >> > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> >> > >> > The Smart Voltage Scaling (SVS) is a hardware which calculates >> > suitable >> > SVS bank voltages to OPP voltage table. >> > >> > When the SVS is enabled, cpufreq should listen to opp notification >> > and do >> > proper actions when receiving events of disable and voltage >> > adjustment. >> >> So listenting for OPP notifications should be done only when SVS is >> enabled... >> > > Thanks for your review. > Yes, the OPP notification is only called from MediaTek SVS. > >> [...] >> >> > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, >> > int cpu) >> > { >> > struct device *cpu_dev; >> > @@ -392,6 +455,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); >> >> ...but here youlisten to OPP notifications unconditionally. Seems >> there >> should be a check whether SVS is enabled before deciding to register. >> >> Kevin >> > Do you think it's ok that we wrap it with the SVS Kconfig define? > like > #ifdef CONFIG_MTK_SVS > mtk_cpufreq_opp_notifier() > ... > dev_pm_opp_register_notifier() > #endif Generally, we don't like to see #ifdefs in C files[1]. But more importantly, compile-time check is not enough, because SVS feature could be compiled into kernel, but not actually enabled for an SoC (e.g. DT node not enabled, etc.) so checking this at compile time is not enough. Ideally, the SVSdriver should provide a function that allows others to check if it's enabled. That function needs to know not only if it's compile in, but if it's enabled/running. If SVS is not compiled in, then that function just returns false. Kevin [1] https://www.kernel.org/doc/html/latest/process/4.Coding.html?highlight=ifdef#ifdef-and-preprocessor-use-in-general
On Mon, 2022-04-11 at 11:09 -0700, Kevin Hilman wrote: > Hi Rex, > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > > On Fri, 2022-04-08 at 13:29 -0700, Kevin Hilman wrote: > > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > > > > > > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > > > > > > > The Smart Voltage Scaling (SVS) is a hardware which calculates > > > > suitable > > > > SVS bank voltages to OPP voltage table. > > > > > > > > When the SVS is enabled, cpufreq should listen to opp > > > > notification > > > > and do > > > > proper actions when receiving events of disable and voltage > > > > adjustment. > > > > > > So listenting for OPP notifications should be done only when SVS > > > is > > > enabled... > > > > > > > Thanks for your review. > > Yes, the OPP notification is only called from MediaTek SVS. > > > > > [...] > > > > > > > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info > > > > *info, > > > > int cpu) > > > > { > > > > struct device *cpu_dev; > > > > @@ -392,6 +455,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); > > > > > > ...but here youlisten to OPP notifications > > > unconditionally. Seems > > > there > > > should be a check whether SVS is enabled before deciding to > > > register. > > > > > > Kevin > > > > > > > Do you think it's ok that we wrap it with the SVS Kconfig define? > > like > > #ifdef CONFIG_MTK_SVS > > mtk_cpufreq_opp_notifier() > > ... > > dev_pm_opp_register_notifier() > > #endif > > Generally, we don't like to see #ifdefs in C files[1]. > > But more importantly, compile-time check is not enough, because SVS > feature could be compiled into kernel, but not actually enabled for > an > SoC (e.g. DT node not enabled, etc.) so checking this at compile time > is > not enough. > > Ideally, the SVSdriver should provide a function that allows others > to > check if it's enabled. That function needs to know not only if it's > compile in, but if it's enabled/running. If SVS is not compiled in, > then that function just returns false. > > Kevin > > [1] > https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/process/4.Coding.html?highlight=ifdef*ifdef-and-preprocessor-use-in-general__;Iw!!CTRNKA9wMg0ARbw!z6SrEcQOLu2u-R1OLedrRUXHYXCzuQoK3F_h9Bhzv8jNFmjV5mdNVy41eND67CuV9uIS$ > Hello Kevin, After our internal discussion, we think the register of notifier should not be bound for certain module. If we provide the moethod to adjust voltage/disable opp, we think if anyone call dev_pm_opp_adjust_voltage and dev_pm_opp_disable, it could be used. May I ask what is your concern? BRs, Rex
Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > On Mon, 2022-04-11 at 11:09 -0700, Kevin Hilman wrote: >> Hi Rex, >> >> Rex-BC Chen <rex-bc.chen@mediatek.com> writes: >> >> > On Fri, 2022-04-08 at 13:29 -0700, Kevin Hilman wrote: >> > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: >> > > >> > > > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> >> > > > >> > > > The Smart Voltage Scaling (SVS) is a hardware which calculates >> > > > suitable >> > > > SVS bank voltages to OPP voltage table. >> > > > >> > > > When the SVS is enabled, cpufreq should listen to opp >> > > > notification >> > > > and do >> > > > proper actions when receiving events of disable and voltage >> > > > adjustment. >> > > >> > > So listenting for OPP notifications should be done only when SVS >> > > is >> > > enabled... >> > > >> > >> > Thanks for your review. >> > Yes, the OPP notification is only called from MediaTek SVS. >> > >> > > [...] >> > > >> > > > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info >> > > > *info, >> > > > int cpu) >> > > > { >> > > > struct device *cpu_dev; >> > > > @@ -392,6 +455,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); >> > > >> > > ...but here youlisten to OPP notifications >> > > unconditionally. Seems >> > > there >> > > should be a check whether SVS is enabled before deciding to >> > > register. >> > > >> > > Kevin >> > > >> > >> > Do you think it's ok that we wrap it with the SVS Kconfig define? >> > like >> > #ifdef CONFIG_MTK_SVS >> > mtk_cpufreq_opp_notifier() >> > ... >> > dev_pm_opp_register_notifier() >> > #endif >> >> Generally, we don't like to see #ifdefs in C files[1]. >> >> But more importantly, compile-time check is not enough, because SVS >> feature could be compiled into kernel, but not actually enabled for >> an >> SoC (e.g. DT node not enabled, etc.) so checking this at compile time >> is >> not enough. >> >> Ideally, the SVSdriver should provide a function that allows others >> to >> check if it's enabled. That function needs to know not only if it's >> compile in, but if it's enabled/running. If SVS is not compiled in, >> then that function just returns false. >> >> Kevin >> >> [1] >> https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/process/4.Coding.html?highlight=ifdef*ifdef-and-preprocessor-use-in-general__;Iw!!CTRNKA9wMg0ARbw!z6SrEcQOLu2u-R1OLedrRUXHYXCzuQoK3F_h9Bhzv8jNFmjV5mdNVy41eND67CuV9uIS$ >> > > Hello Kevin, > > After our internal discussion, we think the register of notifier should > not be bound for certain module. > If we provide the moethod to adjust voltage/disable opp, we think if > anyone call dev_pm_opp_adjust_voltage and dev_pm_opp_disable, it could > be used. I don't think I understand what you mean. Do you mean that this OPP notifier could be registered all the time, even if SVS is not enabled? That's fine with me. If SVS is not compiled or enabled, then the notifiers will never be called, so that's fine. > May I ask what is your concern? My concern was primarily that the changelog description did not match the code. The changelog says "when SVS is enabled, CPUfreq should listen to OPP notifications." But if I understand you correctly above, I think what you mean is that CPUfreq should always listen to OPP notifications because there are other users (e.g. SVS) that could change the OPP outside of this driver. If that's what you mean, then I think the only thing to change is the wording of the changelog. Thanks, Kevin
On Tue, 2022-04-12 at 11:04 -0700, Kevin Hilman wrote: > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > > On Mon, 2022-04-11 at 11:09 -0700, Kevin Hilman wrote: > > > Hi Rex, > > > > > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > > > > > > On Fri, 2022-04-08 at 13:29 -0700, Kevin Hilman wrote: > > > > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > > > > > > > > > > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > > > > > > > > > > > The Smart Voltage Scaling (SVS) is a hardware which > > > > > > calculates > > > > > > suitable > > > > > > SVS bank voltages to OPP voltage table. > > > > > > > > > > > > When the SVS is enabled, cpufreq should listen to opp > > > > > > notification > > > > > > and do > > > > > > proper actions when receiving events of disable and voltage > > > > > > adjustment. > > > > > > > > > > So listenting for OPP notifications should be done only when > > > > > SVS > > > > > is > > > > > enabled... > > > > > > > > > > > > > Thanks for your review. > > > > Yes, the OPP notification is only called from MediaTek SVS. > > > > > > > > > [...] > > > > > > > > > > > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info > > > > > > *info, > > > > > > int cpu) > > > > > > { > > > > > > struct device *cpu_dev; > > > > > > @@ -392,6 +455,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); > > > > > > > > > > ...but here youlisten to OPP notifications > > > > > unconditionally. Seems > > > > > there > > > > > should be a check whether SVS is enabled before deciding to > > > > > register. > > > > > > > > > > Kevin > > > > > > > > > > > > > Do you think it's ok that we wrap it with the SVS Kconfig > > > > define? > > > > like > > > > #ifdef CONFIG_MTK_SVS > > > > mtk_cpufreq_opp_notifier() > > > > ... > > > > dev_pm_opp_register_notifier() > > > > #endif > > > > > > Generally, we don't like to see #ifdefs in C files[1]. > > > > > > But more importantly, compile-time check is not enough, because > > > SVS > > > feature could be compiled into kernel, but not actually enabled > > > for > > > an > > > SoC (e.g. DT node not enabled, etc.) so checking this at compile > > > time > > > is > > > not enough. > > > > > > Ideally, the SVSdriver should provide a function that allows > > > others > > > to > > > check if it's enabled. That function needs to know not only if > > > it's > > > compile in, but if it's enabled/running. If SVS is not compiled > > > in, > > > then that function just returns false. > > > > > > Kevin > > > > > > [1] > > > https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/process/4.Coding.html?highlight=ifdef*ifdef-and-preprocessor-use-in-general__;Iw!!CTRNKA9wMg0ARbw!z6SrEcQOLu2u-R1OLedrRUXHYXCzuQoK3F_h9Bhzv8jNFmjV5mdNVy41eND67CuV9uIS$ > > > > > > > Hello Kevin, > > > > After our internal discussion, we think the register of notifier > > should > > not be bound for certain module. > > If we provide the moethod to adjust voltage/disable opp, we think > > if > > anyone call dev_pm_opp_adjust_voltage and dev_pm_opp_disable, it > > could > > be used. > > I don't think I understand what you mean. > > Do you mean that this OPP notifier could be registered all the time, > even if SVS is not enabled? > > That's fine with me. If SVS is not compiled or enabled, then the > notifiers will never be called, so that's fine. > > > May I ask what is your concern? > > My concern was primarily that the changelog description did not match > the code. The changelog says "when SVS is enabled, CPUfreq should > listen to OPP notifications." > > But if I understand you correctly above, I think what you mean is > that > CPUfreq should always listen to OPP notifications because there are > other users (e.g. SVS) that could change the OPP outside of this > driver. > > If that's what you mean, then I think the only thing to change is the > wording of the changelog. > > Thanks, > > Kevin > > > Hello Kevin, Got it. Thanks for your kindly explaination for this. I will update the commit message in next version. BRs, Rex
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index 472f4de29e5f..1369da62780a 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 old_vproc; + /* Avoid race condition for regulators between notify and policy */ + struct mutex reg_lock; + struct notifier_block opp_nb; + int opp_cpu; + unsigned long opp_freq; }; static LIST_HEAD(dvfs_info_list); @@ -238,6 +243,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, return old_vproc; } + mutex_lock(&info->reg_lock); /* * If the new voltage or the intermediate voltage is higher than the * current voltage, scale up voltage first. @@ -249,7 +255,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, pr_err("cpu%d: failed to scale up voltage!\n", policy->cpu); mtk_cpufreq_set_voltage(info, old_vproc); - return ret; + goto out; } } @@ -259,8 +265,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, pr_err("cpu%d: failed to re-parent cpu clock!\n", policy->cpu); mtk_cpufreq_set_voltage(info, old_vproc); - WARN_ON(1); - return ret; + goto out; } /* Set the original PLL to target rate. */ @@ -270,7 +275,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, policy->cpu); clk_set_parent(cpu_clk, armpll); mtk_cpufreq_set_voltage(info, old_vproc); - return ret; + goto out; } /* Set parent of CPU clock back to the original PLL. */ @@ -279,8 +284,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, pr_err("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; } /* @@ -295,15 +299,74 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, clk_set_parent(cpu_clk, info->inter_clk); clk_set_rate(armpll, old_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, + "%s: failed to scale voltage: %d\n", + __func__, 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, + "%s: all opp items are disabled\n", + __func__); + 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; @@ -392,6 +455,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_warn(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.