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

[Rex-BC Chen]

On Mon, 2022-04-11 at 11:13 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@xxxxxxxxxxxx> writes:
> 
> > On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote:
> > > Rex-BC Chen <rex-bc.chen@xxxxxxxxxxxx> writes:
> > > 
> > > > From: Jia-Wei Chang <jia-wei.chang@xxxxxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxxxxx>
> > > 
> > > 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

Hello Kevin and Angelo,

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.
      You can refer to the chrome project bootloader "Coreboot" patch:
      
https://review.coreboot.org/c/coreboot/+/59569/2/src/mainboard/google/corsola/romstage.c

   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.

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
   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?

BRs,
Rex