Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Kevin Hilman <khilman@baylibre.com>
To:     Rex-BC Chen <rex-bc.chen@mediatek.com>, rafael@kernel.org,
        viresh.kumar@linaro.org, robh+dt@kernel.org, krzk+dt@kernel.org
Cc:     matthias.bgg@gmail.com, jia-wei.chang@mediatek.com,
        roger.lu@mediatek.com, hsinyi@google.com, linux-pm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org,
        Project_Global_Chrome_Upstream_Group@mediatek.com
Subject: Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU
In-Reply-To: <f00e3df2e270e5edc160f8ff1bd8c52a49bf71d5.camel@mediatek.com>
References: <20220408045908.21671-1-rex-bc.chen@mediatek.com>
 <20220408045908.21671-14-rex-bc.chen@mediatek.com>
 <7hfsmn5m9f.fsf@baylibre.com>
 <bc6dd020a1cc3f00f5be2bf2929046b9116bbeef.camel@mediatek.com>
 <7hwnfv4hfr.fsf@baylibre.com>
 <f00e3df2e270e5edc160f8ff1bd8c52a49bf71d5.camel@mediatek.com>
Date:   Tue, 12 Apr 2022 11:50:34 -0700
Message-ID: <7h5yne3zlx.fsf@baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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

[...]

> I can summary what I got now:
>
> 1. Why we need cci for cpufreq in MT8183 and MT8186:
>    a. CCI is a mediatek hw module.
>    b. For mediatek SoCs before MT8183, like MT8173, the CCI hw
>       is not introduced.
>    c. The frequency for cci and cpufreq are determined could
>       be configed at bootloader stage, so the frequency when
>       entering kernel is unknown.
>    d. Cpu little core and cci are using the same regulator.
>    e. If we do not control CCI and just adjust the voltage in
>       cpufreq driver.
>       When we adjust the voltage smaller because we need to reduce
>       the frequency, the CCI could run in high frequency which is
>       set in bootloader.
>       This will cause some problem, the cci could crash.
>
>       Use MT8186 for a example, the bootloader set cci freq as
>       1.385GHz and cpufreq as 2GHz.
>       If entering kernel and we only adjust the cpufreq voltage, if
>       the cpufreq is under 1.618GHz, the cci will be out of spec.
>
>    f. If cpufreq driver wait cci ready, regulator framework will take
>       the highest voltage requests from cci and cpufreq as output
>       so that it prevents from high freqeuncy low voltage crash.
>
>    d. Therefore, I think it's not a good idea to bypass cci device if
>       the ccifreq_supported is true in MT8183 and MT8186.

I do not propose to bypass CCI device.  What both Angelo and I are
saying is just that you need a better way to handle the cases when CCI
is not (yet) enabled.  The current way in the propsed patch is not good
enough.

I fully understand the potential problems with high frequency & low
voltage when using a shared regulator.  But, I think the problem we're
trying to solve here is specific to the initial boot of the platform,
while we are waiting for the CCI driver to be loaded.

The root of the problem is that the CCI bus has constraints on the
voltage regulator that are not defined anywhere until the CCI driver is
loaded.  So to fix that, you need to either:

1) not allow any voltage changes
2) register the CCI device constraints

In the current patch, you attempt to do (1).  There's nothing wrong with
the idea, we just pointed out problems in your implementation,
especially the fact that it does nothing, but it "succeeds" so the
CPUfreq framework will think the OPPs are different from what they
actually are.

Just an idea, but another option could be (2).  While waiting for the
CCI device to be ready, the CPUfreq driver could check the current CCI
freq/voltage and set min/max constraints on the regulator that prevent
CCI from breaking.  These constraints would stay in place until the CCI
driver is ready.  Once the real CCI driver is ready/registerd these
contraints would be removed.

Another version of this same idea would be to check the CCI freq/voltage
and then limit the OPPs available to CPUfreq to only the ones that would
not break CCI.  Then, when CCI is ready/registered, you remove the
limits.

> 2. Check the device link status is DL_DEV_DRIVER_BOUND is used for
>    promising the cci is probed done.
>
> 3. About the cpufreq_driver->target_index
>    a. When I trace the common drivers, I found if the return value is
>       not zero, it will be BUG_ON.
>       ref:
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1471

Right, you should not try to do deferred probe in the ->set_target()
callback.  Deferred probe is meant for init/probe time.

>    b. I also try to move is_ccifreq_ready() to other place, like
>       cpufreq_driver->init and cpufreq probe function.
>       There will be new issue. Do you have any suggetion that we can
>       retern value of DEFER_PROBE?

The only appropriate place is in the probe function.

Kevin