Re: [PATCH v2 4/4] cpufreq: mediatek: Raise proc and sram max voltage for MT7622/7623

["Jia-wei Chang (張佳偉)" <Jia-wei.Chang@mediatek.com>]

On Fri, 2023-05-26 at 16:27 +0800, Jia-Wei Chang wrote:
> On Wed, 2023-05-24 at 13:42 +0100, Daniel Golle wrote:
> > External email : Please do not click links or open attachments
> > until
> > you have verified the sender or the content.
> > 
> > 
> > On Wed, May 24, 2023 at 08:43:31AM +0000, Jia-wei Chang (張佳偉)
> > wrote:
> > > On Wed, 2023-05-24 at 09:28 +0200, AngeloGioacchino Del Regno
> > > wrote:
> > > > External email : Please do not click links or open attachments
> > > > until
> > > > you have verified the sender or the content.
> > > > 
> > > > 
> > > > 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);
> > > 
> > > Hi Daniel, Angelo Sir,
> > > 
> > > Thanks for the issue report and suggestions.
> > > 
> > > Is it possible to modify the value of proc_max_volt and
> > > sram_max_volt
> > > to 1310000 in mt7622_platform_data as the highest voltage
> > > declared
> > > in
> > > mt7622.dtsi and then give it a try?
> > > 
> > > Sorry, I need someone help to check this on mt7622 since I don't
> > > have
> > > mt7622 platform..
> > 
> > Unfortunately also setting proc_max_volt and sram_max_volt to
> > 1310000
> > doesn't work:
> > [    1.983325] cpu cpu0: cpu0: failed to scale up voltage!
> > [    1.988621] cpufreq: __target_index: Failed to change cpu
> > frequency: -22
> > ::repeating infinitely::
> > 
> > This is because in mt6380-regulator.c you can see
> > static const unsigned int ldo_volt_table1[] = {
> >         1400000, 1350000, 1300000, 1250000, 1200000, 1150000,
> > 1100000, 1050000,
> > };
> > 
> > So 1310000 is not among the supported voltages but mediatek-
> > cpufreq.c
> > will repeatedly call
> > regulator_set_voltage(sram_reg, 1310000, 1310000);
> > which will fail for obvious reasons.
> > 
> > Using 1350000 for proc_max_volt and sram_max_volt like I have
> > suggested
> > as a simple work-around does work because 1350000 is among the
> > supported
> > voltages of the MT6380 regulator.
> > 
> > On MT7623 the whole problem is anyway non-existent because there is
> > no
> > separate sram-supply, hence the tracking algorithm isn't used at
> > all.
> > 
> 
> Exactly.
> 
> For MT7622 platform data, I think it is proper to configure as:
> .proc_max_volt = 1310000,
> .sram_max_volt = 1350000,  // since mt6380_vm_reg ldo only supporting
> {..., 1300000, 1350000, 1400000} as you mentioned.
> 
> For MT7623 platform data, it is required to add a new one.
> .proc_max_volt = 1300000,
> .sram_max_volt = 0,  // since no sram-supply like you said.
> 

Note that:

Actually, proc and sram of MT7623 are supplied by one power rail so
that to add sram-supply in dts or assign sram_max_volt = 1300000 in
driver are NOT necessary.

> If MT7622 and MT7623 supplied voltage issues can be fixed by above
> platform data, feel free to send the fix patch or inform me to do
> that.
> 
> Thanks for your help! :)
> 
> > > 
> > > Thanks.
> > > 
> > > > > 
> > > > > 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
> > > > > > 
> > > > 
> > > > 
> > > >