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

Rex-BC Chen
2022-04-14T02:32:43Z

On Wed, 2022-04-13 at 14:41 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@mediatek.com> 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