Date:   Thu, 14 Apr 2022 10:32:29 +0800
From:   Rex-BC Chen <rex-bc.chen@...iatek.com>
To:     Kevin Hilman <khilman@...libre.com>, <rafael@...nel.org>,
        <viresh.kumar@...aro.org>, <robh+dt@...nel.org>,
        <krzk+dt@...nel.org>
CC:     <matthias.bgg@...il.com>, <jia-wei.chang@...iatek.com>,
        <roger.lu@...iatek.com>, <hsinyi@...gle.com>,
        <linux-pm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        <Project_Global_Chrome_Upstream_Group@...iatek.com>
Subject: Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

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

Powered by blists - more mailing lists