Messages in this thread | | | Date | Tue, 23 May 2023 16:56:47 +0200 | Subject | Re: [PATCH v2 4/4] cpufreq: mediatek: Raise proc and sram max voltage for MT7622/7623 | From | AngeloGioacchino Del Regno <> |
| |
Il 22/05/23 20:03, Daniel Golle ha scritto: > Hi Jia-Wei, > Hi AngeloGioacchino, > > On Fri, Mar 24, 2023 at 06:11:30PM +0800, jia-wei.chang wrote: >> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> >> During the addition of SRAM voltage tracking for CCI scaling, this >> driver got some voltage limits set for the vtrack algorithm: these >> were moved to platform data first, then enforced in a later commit >> 6a17b3876bc8 ("cpufreq: mediatek: Refine mtk_cpufreq_voltage_tracking()") >> using these as max values for the regulator_set_voltage() calls. >> >> In this case, the vsram/vproc constraints for MT7622 and MT7623 >> were supposed to be the same as MT2701 (and a number of other SoCs), >> but that turned out to be a mistake because the aforementioned two >> SoCs' maximum voltage for both VPROC and VPROC_SRAM is 1.36V. >> >> Fix that by adding new platform data for MT7622/7623 declaring the >> right {proc,sram}_max_volt parameter. >> >> Fixes: ead858bd128d ("cpufreq: mediatek: Move voltage limits to platform data") >> Fixes: 6a17b3876bc8 ("cpufreq: mediatek: Refine mtk_cpufreq_voltage_tracking()") >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> >> --- >> drivers/cpufreq/mediatek-cpufreq.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c >> index 764e4fbdd536..9a39a7ccfae9 100644 >> --- a/drivers/cpufreq/mediatek-cpufreq.c >> +++ b/drivers/cpufreq/mediatek-cpufreq.c >> @@ -693,6 +693,15 @@ static const struct mtk_cpufreq_platform_data mt2701_platform_data = { >> .ccifreq_supported = false, >> }; >> >> +static const struct mtk_cpufreq_platform_data mt7622_platform_data = { >> + .min_volt_shift = 100000, >> + .max_volt_shift = 200000, >> + .proc_max_volt = 1360000, >> + .sram_min_volt = 0, >> + .sram_max_volt = 1360000, > > This change breaks cpufreq (with ondemand scheduler) on my BPi R64 > board (having MT7622AV SoC with MT6380N PMIC). > ... > [ 2.540091] cpufreq: __target_index: Failed to change cpu frequency: -22 > [ 2.556985] cpu cpu0: cpu0: failed to scale up voltage! > ... > (repeating a lot, every time the highest operating point is selected > by the cpufreq governor) > > The reason is that the MT6380N doesn't support 1360000uV on the supply > outputs used for SRAM and processor. > > As for some reason cpufreq-mediatek tries to rise the SRAM supply > voltage to the maximum for a short moment (probably a side-effect of > the voltage tracking algorithm), this fails because the PMIC only > supports up to 1350000uV. As the highest operating point is anyway > using only 1310000uV the simple fix is setting 1350000uV as the maximum > instead for both proc_max_volt and sram_max_volt. > > A similar situation applies also for BPi R2 (MT7623NI with MT6323L > PMIC), here the maximum supported voltage of the PMIC which also only > supports up to 1350000uV, and the SoC having its highest operating > voltage defined at 1300000uV. > > If all agree with the simple fix I will post a patch for that. > > However, to me it feels fishy to begin with that the tracking algorithm > tries to rise the voltage above the highest operating point defined in > device tree, see here: > > 6a17b3876bc830 drivers/cpufreq/mediatek-cpufreq.c (Jia-Wei Chang 2022-05-05 19:52:20 +0800 100) new_vsram = clamp(new_vproc + soc_data->min_volt_shift, > 6a17b3876bc830 drivers/cpufreq/mediatek-cpufreq.c (Jia-Wei Chang 2022-05-05 19:52:20 +0800 101) soc_data->sram_min_volt, soc_data->sram_max_volt); > > However, I did not investigate in depth the purpose of this > initial rise and can impossibly test my modifications to the > tracking algorithm on all supported SoCs. >
Thanks for actually reporting that, I don't think that there's any valid reason why the algorithm should set a voltage higher than the maximum votage specified in the fastest OPP.
Anyway - the logic for the platform data of this driver is to declare the maximum voltage that SoC model X supports, regardless of the actual board-specific OPPs, so that part is right; to solve this issue, I guess that the only way is for this driver to parse the OPPs during .probe() and then always use in the algorithm
vproc_max = max(proc_max_volt, opp_vproc_max); vsram_max = max(sram_max_volt, vsram_vreg_max);
Jia-Wei, can you please handle this?
Thanks, Angelo
| |