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: <3fc36d6b803ff3e1bef601839aaca3f4f03e62ad.camel@mediatek.com>
References: <7hbkx3fiac.fsf@baylibre.com>
 <3fc36d6b803ff3e1bef601839aaca3f4f03e62ad.camel@mediatek.com>
Date:   Tue, 19 Apr 2022 11:16:35 -0700
Message-ID: <7hwnfldjlo.fsf@baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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

> On Thu, 2022-04-14 at 14:48 -0700, Kevin Hilman wrote:

[...]

>> 
>> 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
>> 
>> 
>
> Hello Kevin,
>
> In new proposal, we DO set the cpufreq passed by cpufreq core.
> We just not set the corresponding voltage of target frequency which
> is lookedup from opp table.

OK, this makes more sense.  I thought you were ignoring frequency change
also.

[...]

> In summary, we think it's a proper solution to cover the situation
> when cci is not probed.

Yes, this is OK with me.

> I think there is something to improve:
> We can choose to lookup cci opp table using cci freq to determine
> the voltage instead of voltage_on_boot.
> But IMO, it's not neccessary to register cci opp table inside cpufreq
> driver just for the short period.

I agree.

> Because I finish to prepare other patches and I think we also can
> take a look at other patches which are including some cleanup, I will
> send next version today.
> If there is any concern and question, we can discuss in next version.

OK, I'll have a closer look at the new version.

Kevin