| Message ID | 20220422075239.16437-1-rex-bc.chen@mediatek.com |
|---|---|
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
Il 22/04/22 09:52, Rex-BC Chen ha scritto: > - Add cpufreq opp table. > - Add MediaTek cci opp table. > - Add property of opp table and clock fro cpufreq. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 22/04/22 09:52, Rex-BC Chen ha scritto: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Because the difference of sram and proc should in a range of min_volt_shift > and max_volt_shift. We need to adjust the sram and proc step by step. > > We replace VOLT_TOL (voltage tolerance) with the platform data and update the > logic to determine the voltage boundary and invoking regulator_set_voltage. > > - Use 'sram_min_volt' and 'sram_max_volt' to determine the voltage boundary > of sram regulator. > - Use (sram_min_volt - min_volt_shift) and 'proc_max_volt' to determine the > voltage boundary of vproc regulator. > > Moreover, to prevent infinite loop when tracking voltage, we calculate the > maximum value for each platform data. > We assume min voltage is 0 and tracking target voltage using > min_volt_shift for each iteration. > The retry_max is 3 times of expeted iteration count. > > 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>
Il 22/04/22 09:52, Rex-BC Chen ha scritto: > 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> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 22/04/22 09:52, 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> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On 22/04/2022 09:52, Rex-BC Chen wrote: > > Reference series: > [1]: V1 of this series is present by Jia-Wei Chang. > message-id:20220307122151.11666-1-jia-wei.chang@mediatek.com > > [2]: The MediaTek CCI devfreq driver is introduced in another series. > message-id:20220408052150.22536-1-johnson.wang@mediatek.com > > [3]: The MediaTek SVS driver is introduced in another series. > message-id:20220221063939.14969-1-roger.lu@mediatek.com These are not proper links. Please use lore references. Best regards, Krzysztof
On 22-04-22, 15:52, Rex-BC Chen wrote: > - Replace pr_* with dev_* to show logs. > - Remove usage of __func__. > > 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 | 54 ++++++++++++++++-------------- > 1 file changed, 28 insertions(+), 26 deletions(-) Applied. Thanks.
On 22-04-22, 15:52, Rex-BC Chen wrote: > To make driver more readable, replace old_* with pre_*. > > 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 | 84 +++++++++++++++--------------- > 1 file changed, 42 insertions(+), 42 deletions(-) Applied. Thanks.
On 22-04-22, 10:21, AngeloGioacchino Del Regno wrote: > Il 22/04/22 09:52, 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> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Applied. Thanks.
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 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 22-04-22, 15:52, Rex-BC Chen wrote: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > For some MediaTek SoCs, like MT8186, it's possible that the sram regulator > is shared between CPU and CCI. > We hope regulator framework can return error for error handling rather > than a dummy handler from regulator_get api. > Therefore, we choose to use regulator_get_optional. > > 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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index 0b2ca0c8eddc..97ce96421241 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c > @@ -438,7 +438,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > } > > /* Both presence and absence of sram regulator are valid cases. */ > - info->sram_reg = regulator_get_exclusive(cpu_dev, "sram"); > + info->sram_reg = regulator_get_optional(cpu_dev, "sram"); > if (IS_ERR(info->sram_reg)) > info->sram_reg = NULL; > else { Applied. Thanks.
On Fri, 2022-04-22 at 19:23 +0200, Krzysztof Kozlowski wrote: > On 22/04/2022 09:52, Rex-BC Chen wrote: > > > > Reference series: > > [1]: V1 of this series is present by Jia-Wei Chang. > > message-id:20220307122151.11666-1-jia-wei.chang@mediatek.com > > > > [2]: The MediaTek CCI devfreq driver is introduced in another > > series. > > message-id:20220408052150.22536-1-johnson.wang@mediatek.com > > > > [3]: The MediaTek SVS driver is introduced in another series. > > message-id:20220221063939.14969-1-roger.lu@mediatek.com > > These are not proper links. Please use lore references. > > > Best regards, > Krzysztof Hello Krzysztof, I will use lore references in next version. Thanks. BRs, Rex
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. From top of view, if 'cpufreq_get' is NOT invoked in that condition, the voltage pulse will no longer occur. That's why we add this patch for this series. > > 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. > Thank you for sharing the correct information. Is it possible to get frequency (API) a simple way, like get current opp frequency? BRs, Rex
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() 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. From CPUFREQ_POSTCHANGE notifier, we always set policy->cur from the table itself, which should be 500000 MHz. I wonder how you see policy->cur to be 499999 here. Does this happen only once ? Or repeatedly ? > 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. > > 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. > 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 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 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/
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 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.
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
Cpufreq is a DVFS driver used for power saving to scale the clock frequency and supply the voltage for CPUs. This series do some cleanup for MediaTek cpufreq drivers and add support for MediaTek SVS[2] and MediaTek CCI devfreq[3] which are supported in MT8183 and MT8186. Changes for V4: 1. Revise drivers from reviewers' suggestion. 2. Fix name of opp table issue. Changes for V3: 1. Rebased to linux-next-20220414. 2. Drop accepted patches. 3. Drop "cpufreq: mediatek: Use maximum voltage in init stage" because we make sure the voltage we set is safe for both mediatek cci and cpufreq. 4. Rename cci property to mediatek,cci. 5. Adjust order of cleanup patches. 6. Add new patches for cleanup, handle infinite loop and MT8183 dts. 7. Revise drivers from reviewers' suggestion. 8. Revise commit message of some patches to avoid confusion and misunderstand. 9. Revise "cpufreq: mediatek: Link CCI device to CPU". We do not return successful to pretend we set the target frequency done when cci is not ready. Instead, we find and set a safe voltage so that we can set the target cpufrequency. Changes for V2: 1. Drop the modification of transforming cpufreq-mediatek into yaml and only add the MediaTek CCI property for MediaTek cpufreq. 2. Split the original patches into several patches. Reference series: [1]: V1 of this series is present by Jia-Wei Chang. message-id:20220307122151.11666-1-jia-wei.chang@mediatek.com [2]: The MediaTek CCI devfreq driver is introduced in another series. message-id:20220408052150.22536-1-johnson.wang@mediatek.com [3]: The MediaTek SVS driver is introduced in another series. message-id:20220221063939.14969-1-roger.lu@mediatek.com Andrew-sh.Cheng (1): cpufreq: mediatek: Add opp notification support Jia-Wei Chang (6): cpufreq: mediatek: Record previous target vproc value cpufreq: mediatek: Move voltage limits to platform data cpufreq: mediatek: Add .get function cpufreq: mediatek: Make sram regulator optional cpufreq: mediatek: Refine mtk_cpufreq_voltage_tracking() cpufreq: mediatek: Add support for MT8186 Rex-BC Chen (7): dt-bindings: cpufreq: mediatek: Add MediaTek CCI property cpufreq: mediatek: Use device print to show logs cpufreq: mediatek: Replace old_* with pre_* cpufreq: mediatek: Link CCI device to CPU 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 .../bindings/cpufreq/cpufreq-mediatek.txt | 5 + arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 36 ++ .../arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 4 + arch/arm64/boot/dts/mediatek/mt8183.dtsi | 285 ++++++++++ drivers/cpufreq/mediatek-cpufreq.c | 504 ++++++++++++------ 5 files changed, 670 insertions(+), 164 deletions(-)