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

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
Date: Mon, 11 Apr 2022 11:13:12 -0700	[thread overview]
Message-ID: <7hwnfv4hfr.fsf@baylibre.com> (raw)
In-Reply-To: <bc6dd020a1cc3f00f5be2bf2929046b9116bbeef.camel@mediatek.com>

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

> On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote:
>> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
>> 
>> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>> > 
>> > In some MediaTek SoCs, like MT8183, CPU and CCI share the same
>> > power
>> > supplies. Cpufreq needs to check if CCI devfreq exists and wait
>> > until
>> > CCI devfreq ready before scaling frequency.
>> > 
>> > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will
>> > start
>> >   DVFS when CCI is ready.
>> > - Add platform data for MT8183.
>> > 
>> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>> 
>> The checks here are not enough, and will lead to unexpected behavior.
>> IIUC, before doing DVFS, you're checking:
>> 
>> 1) if the "cci" DT node is present and
>> 2) if the driver for that device is bound
>> 
>> If both those conditions are not met, you don't actually fail, you
>> just
>> silently do nothing in ->set_target().  As Angelo pointed out also,
>> this
>> is not a good idea, and will be rather confusing to users.
>> 
>> The same thing would happen if the cci DT node was present, but the
>> CCI
>> devfreq driver was disabled.  Silent failure would also be quite
>> unexpected behavior.  Similarily, if the cci DT node is not present
>> at
>> all (like it is in current upstream DT), this CPUfreq driver will
>> silently do nothing.  Not good.
>> 
>> So, this patch needs to handle several scenarios:
>> 
>> 1) CCI DT node not present
>> 
>> In this case, the driver should still operate normally.  With no CCI
>> node, or driver there's no conflict.
>> 
>> 2) CCI DT present/enabled but not yet bound
>> 
>> In this case, you could return -EAGAIN as suggested by Angelo, or
>> maybe
>> better, it should do a deferred probe.
>> 
>> 3) CCI DT present, but driver disabled
>> 
>> This case is similar to (1), this driver should continue to work.
>> 
>> Kevin
>
> Hello Kevin and Angelo,
>
> In my review, if we do not get the link or the link status is not
> correct between cci and cpufreq in target_index, I think it will never
> established again for this link.
> Because it's not checked in probe stage.
>
> So I think we just need to deal with the issue without cci device, and
> don't expect the link between cci and cpufreq will be connected again.
>
> If I am wrong, please correct me.

I don't fully understand your questions, but I think what your getting
at suggest that you might need to use deferred probe to handle the case
where the ordering of CCI and cpufreq probing is not predictable.

Kevin

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek