| Message ID | 20220422075239.16437-8-rex-bc.chen@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
On 22-04-22, 15:52, Rex-BC Chen wrote: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > We want to get opp frequency via opp table. Therefore, we add the function > "mtk_cpufreq_get()" to do this. > > 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 | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index e070a2619bcb..0b2ca0c8eddc 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c > @@ -71,6 +71,15 @@ static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_lookup(int cpu) > return NULL; > } > > +static unsigned int mtk_cpufreq_get(unsigned int cpu) > +{ > + struct mtk_cpu_dvfs_info *info; > + > + info = mtk_cpu_dvfs_info_lookup(cpu); > + > + return !info ? 0 : (info->opp_freq / 1000); > +} > + > static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > int new_vproc) > { > @@ -588,7 +597,7 @@ static struct cpufreq_driver mtk_cpufreq_driver = { > CPUFREQ_IS_COOLING_DEV, > .verify = cpufreq_generic_frequency_table_verify, > .target_index = mtk_cpufreq_set_target, > - .get = cpufreq_generic_get, > + .get = mtk_cpufreq_get, The .get callback should read the real frequency from hardware instead of fetching a cached freq value. > .init = mtk_cpufreq_init, > .exit = mtk_cpufreq_exit, > .register_em = cpufreq_register_em_with_opp, > -- > 2.18.0
On Mon, 2022-04-25 at 11:05 +0530, Viresh Kumar wrote: > On 22-04-22, 15:52, Rex-BC Chen wrote: > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > We want to get opp frequency via opp table. Therefore, we add the > > function > > "mtk_cpufreq_get()" to do this. > > > > 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 | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index e070a2619bcb..0b2ca0c8eddc 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > @@ -71,6 +71,15 @@ static struct mtk_cpu_dvfs_info > > *mtk_cpu_dvfs_info_lookup(int cpu) > > return NULL; > > } > > > > +static unsigned int mtk_cpufreq_get(unsigned int cpu) > > +{ > > + struct mtk_cpu_dvfs_info *info; > > + > > + info = mtk_cpu_dvfs_info_lookup(cpu); > > + > > + return !info ? 0 : (info->opp_freq / 1000); > > +} > > + > > static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info > > *info, > > int new_vproc) > > { > > @@ -588,7 +597,7 @@ static struct cpufreq_driver mtk_cpufreq_driver > > = { > > CPUFREQ_IS_COOLING_DEV, > > .verify = cpufreq_generic_frequency_table_verify, > > .target_index = mtk_cpufreq_set_target, > > - .get = cpufreq_generic_get, > > + .get = mtk_cpufreq_get, > > The .get callback should read the real frequency from hardware > instead of > fetching a cached freq value. > Hello Viresh, We found that the pulses of cpu voltage could be observed when frequency is fixed (scaling_max_freq == scaling_min_freq) if using cpufreq_generic_get as '.get' callback in MT8186. cpufreq framework will constantly (~ 1 sec) call 'update' if the policy frequency is NOT equal to hardware frequency in cpufreq_verify_current_freq. The problem is that there might be a tiny difference between the policy frequency and the hardware frequency even they are very close. e.g. policy frequency is 500,000,000 Hz however, hardware frequency is 499,999,726 Hz for MT8186 opp15. To prevent the voltage pulses, we currently use the software cached values as you pointed out. I wonder is it possible to add a tolerence for checking difference between policy frequency and hardware frequency in cpufreq framework so that we can use cpufreq_generic_get as callback without pulse issue. Or any suggestion would be appreciated. Thanks. BRs, Rex > > .init = mtk_cpufreq_init, > > .exit = mtk_cpufreq_exit, > > .register_em = cpufreq_register_em_with_opp, > > -- > > 2.18.0 > >
On 25-04-22, 17:34, Rex-BC Chen wrote: > We found that the pulses of cpu voltage could be observed when > frequency is fixed (scaling_max_freq == scaling_min_freq) if using > cpufreq_generic_get as '.get' callback in MT8186. > cpufreq framework will constantly (~ 1 sec) call 'update' if the policy Which function gets called here in that case ? I would expect cpufreq_driver_target() to not make a call to MTK driver in that case, after it finds that new and old frequency are same (it will check the corresponding freq from cpufreq table). > frequency is NOT equal to hardware frequency in > cpufreq_verify_current_freq. > The problem is that there might be a tiny difference between the policy > frequency and the hardware frequency even they are very close. > e.g. policy frequency is 500,000,000 Hz however, hardware frequency is > 499,999,726 Hz for MT8186 opp15. > > To prevent the voltage pulses, we currently use the software cached > values as you pointed out. > I wonder is it possible to add a tolerence for checking difference > between policy frequency and hardware frequency in cpufreq framework so > that we can use cpufreq_generic_get as callback without pulse issue. > Or any suggestion would be appreciated.
On Mon, 2022-04-25 at 15:30 +0530, Viresh Kumar wrote: > On 25-04-22, 17:34, Rex-BC Chen wrote: > > We found that the pulses of cpu voltage could be observed when > > frequency is fixed (scaling_max_freq == scaling_min_freq) if using > > cpufreq_generic_get as '.get' callback in MT8186. > > cpufreq framework will constantly (~ 1 sec) call 'update' if the > > policy > > Which function gets called here in that case ? I would expect > cpufreq_driver_target() to not make a call to MTK driver in that > case, after it > finds that new and old frequency are same (it will check the > corresponding freq > from cpufreq table). > > > frequency is NOT equal to hardware frequency in > > cpufreq_verify_current_freq. > > The problem is that there might be a tiny difference between the > > policy > > frequency and the hardware frequency even they are very close. > > e.g. policy frequency is 500,000,000 Hz however, hardware frequency > > is > > 499,999,726 Hz for MT8186 opp15. > > > > To prevent the voltage pulses, we currently use the software cached > > values as you pointed out. > > I wonder is it possible to add a tolerence for checking difference > > between policy frequency and hardware frequency in cpufreq > > framework so > > that we can use cpufreq_generic_get as callback without pulse > > issue. > > Or any suggestion would be appreciated. > > Hello Viresh, We have a non-upstream driver which tries to get frequency by 'cpufreq_get'. When we use that non-upstream driver, 'cpufreq_verify_current_freq' will be further invoked by 'cpufreq_get' and it would cause voltage pulse issue as I described previously. Therefore, we apply the solution in this series. Recently, we found that using 'cpufreq_generic_get' directly in our non-upstream driver can do the same thing without pulse issue. It can meet your request as well. So here, for cpufreq, I think it is proper to drop this patch and I will do it in the next version. Thanks for your review. BRs, Rex
On 26-04-22, 19:13, Rex-BC Chen wrote: > We have a non-upstream driver which tries to get frequency by > 'cpufreq_get'. This is the right thing to do there. > When we use that non-upstream driver, 'cpufreq_verify_current_freq' > will be further invoked by 'cpufreq_get' and it would cause voltage > pulse issue as I described previously. I see this will eventually resolve to __cpufreq_driver_target(), which should return without any frequency updates. What do you mean by "voltage pulse" here? What actually happens which you want to avoid. > Therefore, we apply the solution in this series. I won't call it a solution but a Bug as .get() is supposed to read real freq of the hardware. > Recently, we found that using 'cpufreq_generic_get' directly in our > non-upstream driver can do the same thing without pulse issue. That would be an abuse of the cpufreq_generic_get() API. It is ONLY allowed to be used while setting .get callback in the driver.
On Wed, 2022-04-27 at 08:41 +0530, Viresh Kumar wrote: > On 26-04-22, 19:13, Rex-BC Chen wrote: > > We have a non-upstream driver which tries to get frequency by > > 'cpufreq_get'. > > This is the right thing to do there. > > > When we use that non-upstream driver, 'cpufreq_verify_current_freq' > > will be further invoked by 'cpufreq_get' and it would cause voltage > > pulse issue as I described previously. > > I see this will eventually resolve to __cpufreq_driver_target(), > which > should return without any frequency updates. > Hello Viresh, Yes, the call stack will eventually go to __cpufreq_driver_target. However, we can observe the mismatch between target_freq and policy-cur with a tiny difference. e.g. [ 553.065356] cpufreq: target for CPU 0: 500000 kHz, relation 0, requested 500000 kHz [ 553.066366] cpufreq: target_freq/policy->cur: 500000/499999 kHz We check the assignment of policy->cur could be either from cpufreq_driver->get_intermediate or from cpufreq_driver->get. But it is strange to have the frequency value like 499999 kHz. Is the result of tiny frequency difference expected from your point of view? > What do you mean by "voltage pulse" here? What actually happens which > you want to avoid. > When cpufreq is fixed to lowest opp, "voltage pulse" is a quick voltage rising and falling phenomenon which can be observed if 'cpufreq_get' is invoked.
On 28-04-22, 19:16, Rex-BC Chen wrote: > Yes, the call stack will eventually go to __cpufreq_driver_target. > However, we can observe the mismatch between target_freq and policy-cur > with a tiny difference. > e.g. > [ 553.065356] cpufreq: target for CPU 0: 500000 kHz, relation 0, > requested 500000 kHz > [ 553.066366] cpufreq: target_freq/policy->cur: 500000/499999 kHz So you are trying to set the frequency to 500 MHz now, but policy->cur says it is 499 MHz. > We check the assignment of policy->cur could be either from > cpufreq_driver->get_intermediate or from cpufreq_driver->get. policy->cur is set only at two places, in your case: - CPUFREQ_POSTCHANGE - cpufreq_online()
On Thu, 2022-04-28 at 17:18 +0530, Viresh Kumar wrote: > On 28-04-22, 19:16, Rex-BC Chen wrote: > > Yes, the call stack will eventually go to __cpufreq_driver_target. > > However, we can observe the mismatch between target_freq and > > policy-cur > > with a tiny difference. > > e.g. > > [ 553.065356] cpufreq: target for CPU 0: 500000 kHz, relation 0, > > requested 500000 kHz > > [ 553.066366] cpufreq: target_freq/policy->cur: 500000/499999 kHz > > So you are trying to set the frequency to 500 MHz now, but policy- > >cur says it > is 499 MHz. > Hello Viresh, Yes. > > We check the assignment of policy->cur could be either from > > cpufreq_driver->get_intermediate or from cpufreq_driver->get. > > policy->cur is set only at two places, in your case: > - CPUFREQ_POSTCHANGE > - cpufreq_online() > > From what I understand, it is possible that cpufreq_online() is > setting your > frequency to 499999 (once at boot), but as soon as a frequency change > has > happened after that, policy->cur should be set to 500 MHz and you > should see > this problem only once. > Our observation tells us cpufreq_online is setting only once at boot for one cpu cluster. But we can see the problem repeatly occurs once cpufreq_get is invoked. e.g. [ 71.154560] cpufreq: Warning: CPU frequency out of sync: cpufreq and timing core thinks of 500000, is 499999 kHz [ 71.155880] cpufreq: notification 0 of frequency transition to 499999 kHz [ 71.156777] cpufreq: notification 1 of frequency transition to 499999 kHz [ 71.187241] cpufreq: target for CPU 0: 500000 kHz, relation 0, requested 500000 kHz [ 71.188246] cpufreq: target_freq/policy->cur: 500000/499999 kHz > From CPUFREQ_POSTCHANGE notifier, we always set policy->cur from the > table > itself, which should be 500000 MHz. > Our observation tells me it can be either 499999 kHz or 500000 kHz. This can be printed at the last line of CPUFREQ_POSTCHANGE within 'cpufreq_notify_transition' > I wonder how you see policy->cur to be 499999 here. Does this happen > only once ? > Or repeatedly ? > It repeatly happens. > > But it is strange to have the frequency value like 499999 kHz. > > Is the result of tiny frequency difference expected from your point > > of > > view? > > Clock driver can give this value, that is fine. > Thanks for your answer. > > > What do you mean by "voltage pulse" here? What actually happens > > > which > > > you want to avoid. > > > > > > > When cpufreq is fixed to lowest opp, "voltage pulse" is a quick > > voltage > > rising and falling phenomenon which can be observed if > > 'cpufreq_get' is > > invoked. > > Do check if the call is reaching your driver's ->target_index(), it > should be > which it should not, ideally. > Yes, 'cpufreq_get' will eventually go to '->target_index()' because of inequality between target_freq and policy->cur. And we realized that the "voltage pulse" is generated by quick switching voltage from 500 MHz to intermediate voltage and back to 500 MHz in current mediatek-cpufreq.c. To fix it, we think two possible ways to approach. One is from cpufreq framework side. Is it expected to update the cpufreq platform driver repeatly for this case? If it is expected, then from platform driver side, mediatek-cpufreq should handle a condition to avoid unnecessary intermediate voltage switching. BRs, Rex > > Thank you for sharing the correct information. > > Is it possible to get frequency (API) a simple way, like get > > current > > opp frequency? > > Lets dig/debug a bit further and fix this if a real problem exists. >
On Wed, 2022-05-04 at 13:52 +0530, Viresh Kumar wrote: > On 03-05-22, 19:33, Rex-BC Chen wrote: > > Our observation tells us cpufreq_online is setting only once at > > boot > > for one cpu cluster. > > But we can see the problem repeatly occurs once cpufreq_get is > > invoked. > > > > e.g. > > [ 71.154560] cpufreq: Warning: CPU frequency out of sync: cpufreq > > and > > timing core thinks of 500000, is 499999 kHz > > [ 71.155880] cpufreq: notification 0 of frequency transition to > > 499999 > > kHz > > [ 71.156777] cpufreq: notification 1 of frequency transition to > > 499999 > > kHz > > [ 71.187241] cpufreq: target for CPU 0: 500000 kHz, relation 0, > > requested 500000 kHz > > [ 71.188246] cpufreq: target_freq/policy->cur: 500000/499999 kHz > > Lemme know if this helps: > > https://lore.kernel.org/lkml/39e39a7d30c8ee6af81fb64670a330abeb87402e.1651652493.git.viresh.kumar@linaro.org/ > Hello Viresh, Thanks a lot! It helps to fix this issue. And I will drop this patch in the next version. BRs, Rex
On Wed, 2022-05-04 at 17:28 +0530, Viresh Kumar wrote: > On 04-05-22, 19:57, Rex-BC Chen wrote: > > Thanks a lot! It helps to fix this issue. > > And I will drop this patch in the next version. > > Please provide your Tested-by in that thread then, it will help. > Hello Viresh, The verification is done by Jia-wei. I will help him to provide Tested-by. Thanks. BRs, Rex
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index e070a2619bcb..0b2ca0c8eddc 100644 --- a/drivers/cpufreq/mediatek-cpufreq.c +++ b/drivers/cpufreq/mediatek-cpufreq.c @@ -71,6 +71,15 @@ static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_lookup(int cpu) return NULL; } +static unsigned int mtk_cpufreq_get(unsigned int cpu) +{ + struct mtk_cpu_dvfs_info *info; + + info = mtk_cpu_dvfs_info_lookup(cpu); + + return !info ? 0 : (info->opp_freq / 1000); +} + static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, int new_vproc) { @@ -588,7 +597,7 @@ static struct cpufreq_driver mtk_cpufreq_driver = { CPUFREQ_IS_COOLING_DEV, .verify = cpufreq_generic_frequency_table_verify, .target_index = mtk_cpufreq_set_target, - .get = cpufreq_generic_get, + .get = mtk_cpufreq_get, .init = mtk_cpufreq_init, .exit = mtk_cpufreq_exit, .register_em = cpufreq_register_em_with_opp,