Re: [PATCH V4 07/14] cpufreq: mediatek: Add .get function

[Rex-BC Chen <rex-bc.chen@mediatek.com>]

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.
>