Messages in this thread | | | Date | Wed, 24 May 2023 09:28:39 +0200 | Subject | Re: [PATCH v2 4/4] cpufreq: mediatek: Raise proc and sram max voltage for MT7622/7623 | From | AngeloGioacchino Del Regno <> |
| |
Il 23/05/23 19:37, Daniel Golle ha scritto: > On Tue, May 23, 2023 at 04:56:47PM +0200, AngeloGioacchino Del Regno wrote: >> 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); > > You probably meant to write > vproc_max = min(proc_max_volt, opp_vproc_max); > vsram_max = min(sram_max_volt, vsram_vreg_max); > > right? >
Apparently, some of my braincells was apparently taking a break. :-)
Yes, I was meaning min(), not max() :-)
Cheers!
>> >> Jia-Wei, can you please handle this? >> >> Thanks, >> Angelo >>
| |