On Tue, 2022-04-12 at 11:50 -0700, Kevin Hilman wrote: > Rex-BC Chen <rex-bc.chen@xxxxxxxxxxxx> 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://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c*L1471__;Iw!!CTRNKA9wMg0ARbw!wgawOs1JSuJihgxA1nxhbd2Ekoys_bPCAlIH9YJhe2N9ckG6O1mDB-7zqSf6x2MhCfXo$ > > > > 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 Hello Kevin,
