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

Rex-BC Chen
2022-04-11T11:51:33Z

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,

Thanks for your review.
I will review these three situations and do some modification.

BRs,
Rex