Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@xxxxxxxxxxxx> writes:

> On Wed, 2022-04-13 at 14:41 -0700, Kevin Hilman wrote:
>> Rex-BC Chen <rex-bc.chen@xxxxxxxxxxxx> writes:
>> 
>> [...]
>> 
>> > From the Chanwoo's devfreq passive govonor series, it's impossible
>> > to
>> > let cci devreq probed done before cpufreq because the passive
>> > govonor
>> > will search for cpufreq node and use it.
>> > 
>> > Ref: function: cpufreq_passive_register_notifier()
>> > 
>> > 
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=b670978ddc43eb0c60735c3af6e4a370603ab673__;!!CTRNKA9wMg0ARbw!z58Lc1p9REo88oHn-NkxroN_fBd0TsHYmhscNZwnWwT71ecRkTeqZ6vFl5l7HpkTdM6t$
>> >  
>> 
>> Well this is a problem, because CCI depends on CPUfreq, but CPUfreq
>> depends on CCI, so one of them has to load and then wait for the
>> other.
>> 
>> > After I discuss with Angelo and Jia-wei, we think we are keeping
>> > the
>> > function in target_index and if the cci is not ready we will use
>> > the
>> > voltage which is set by bootloader to prevent high freqeuncy low
>> > voltage crash. And then we can keep seting the target frequency.
>> > 
>> 
>>  > We assume the setting of bootloader is correct and we can do this.
>> 
>> I'm still not crazy about this because you're lying to the CPUfreq
>> framework.  It's requesting one OPP, but you're not setting that,
>> you're
>> just keeping the bootloader frequency.
>> 
>> In my earlier reply, I gave two other options for handling this.
>> 
>> 1) set a (temporary) constraint on the voltage regulator so that it
>> cannot change.
>> 
>> or more clean, IMO:
>> 
>> 2) set a CPUfreq policy that restricts available OPPs to ones that
>> will
>> not break CCI.
>> 
>> Either of these solutions allow you to load the CPUfreq driver early,
>> and then wait for the CCI driver to be ready before removing the
>> restrictions.
>
> Hello Kevin,
>
> I think I do not describe this clearly.
> The proposal is:
>
> In cpufreq probe:
> we record the voltage value which is set by bootloader.
>
> In mtk_cpufreq_set_target():
> We do NOT directly return 0.
> Instead, we will find the voltage of target cpufreq and use the value
> max(booting voltage, target cpufreq voltage)
>
> mtk_cpufreq_set_target() {
> 	/* NOT return 0 if !is_ccifreq_ready */
> 	....
> 	vproc = get voltage of target cpufreq from opp.
>
> 	if (ccifreq_supported && !is_ccifreq_ready)
> 		vproc = max(vproc, vproc_on_boot)
>
> 	//setting voltage and target frequency
> 	....
> }

You explained this well, but it's still not an appropriate solution IMO,
because you're still not setting the target that is requested by the
CPUfreq core.

The job of ->set_target() is to set the frequency *requested by CPUfreq
core*.  If you cannot do that, you should return failure.  What you posted
in the original patch and what you're proposing here is to ignore the
frequency passed to ->set_target() and do something else.  In the
orignal patch, you propose do to nothing.  Now, you're ignoring the 
target passed in and setting something else.  In both cases, the CPUfreq
core things you have successfuly set the frequency requested, but you
have not.  This means there's a mismatch between what the CPUfreq core &
governer things the frequency is and what is actually set.  *This* is
the part that I think is wrong.

Instead, the proper way of restricting available frequencies is to use
governors or policies.  This ensures that the core & governors are
aligned with what the platform driver actually does.

As I proposed earlier, I think a clean solution to this problem is to
create a temporary policy at probe time that restricts the available
OPPs based on what the current CCI freq/voltage are.  Once CCI driver is
loaded and working, this policy can be removed.

Kevin