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

[Rex-BC Chen]

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

> 
> > For the SoCs that including ci hardware (8183 and 8186), we think
> > it's
> > not ok if we don't probe cci correctly.
> > If we failed to get cci node, I think we sould return -ENODEV and
> > the
> > probe of cpufreq failed.
> > 
> > What do you think the solution?
> 
> I think it would be better if CPUfreq probes sucessfully, but
> restricts
> the OPPs available until CCI is ready.  If CCI fails to probe/load,
> you
> still have a working CPUfreq driver, it just has a restricted set of
> OPPs.
> 
> Kevin

If we can use the solution.
I think it will be ok for this situation.

Thanks!

BRs,
Rex