Message-ID: <9d7e62049e3442582bd64dbc9e4d2a64f1ad0c1a.camel@mediatek.com>
Date:   Mon, 29 May 2023 04:12:12 +0000
From:   Jia-wei Chang (張佳偉) 
        <Jia-wei.Chang@...iatek.com>
To:     "daniel@...rotopia.org" <daniel@...rotopia.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "vincent@...temli.org" <vincent@...temli.org>,
        "hsinyi@...gle.com" <hsinyi@...gle.com>,
        "viresh.kumar@...aro.org" <viresh.kumar@...aro.org>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        "khilman@...libre.com" <khilman@...libre.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "rafael@...nel.org" <rafael@...nel.org>,
        Rex-BC Chen (陳柏辰) 
        <Rex-BC.Chen@...iatek.com>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>,
        Chen Zhong (钟辰) <Chen.Zhong@...iatek.com>,
        "error27@...il.com" <error27@...il.com>
Subject: Re: [PATCH v2 4/4] cpufreq: mediatek: Raise proc and sram max voltage
 for MT7622/7623

On Fri, 2023-05-26 at 11:27 +0100, Daniel Golle wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Fri, May 26, 2023 at 08:27:25AM +0000, Jia-wei Chang (張佳偉) wrote:
> > On Wed, 2023-05-24 at 13:42 +0100, Daniel Golle wrote:
> > > 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:
> > > > > 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@...labora.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@...labora.com>
> > > > > > > > > Signed-off-by: Jia-Wei Chang <
> jia-wei.chang@...iatek.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.
> 
> Unfortunately that also doesn't work. The tracking algorithm then
> apparently still tries to set unsupported voltages, I assume that
> your suggestion will result in SRAM voltage being requested as
> 1310000uV (proc_max_volt) + 200000uV (max_step_size) = 1330000uV
> which also isn't supported by the regulator.
> 
> [    1.972654] cpu cpu0: cpu0: failed to scale up voltage!
> [    1.977951] cpufreq: __target_index: Failed to change cpu
> frequency: -22
> [    1.984776] cpu cpu0: cpu0: failed to scale up voltage!
> [    1.990039] cpufreq: __target_index: Failed to change cpu
> frequency: -22
> ...
> 
> With my initial suggestion to set both, proc_max_volt and
> sram_max_volt
> to 1350000 it does work.
> 
> However, I think we are now botching around with work-arounds not
> addressing the underlying problems which are that
>  a) the tracking algorithm initially tries to raise the SRAM voltage
> to
>     be **exactly** the minimum of proc_max_volt + max_step_size or
>     sram_max_volt.
>  b) requesting an exact voltage, ie. regulator_set_voltage(reg, X,
> X),
>     is always problematic in case of regulators only supporting a
>     limited set of supported voltages.
> 
> While adjusting the voltages in the SoC's platform data as a
> work-around may be good enough as a hot-fix for now, imho the best
> would be to re-write the tracking algorithm addressing both of the
> above flaws.
> 

Yes, there will be a short-term, i.e. hot-fix, solution for now, and a
long-term solution to handle the potential risk while cpufreq trying to
set any voltage which is not valid or available on regulator.

The platform data of MT7622 is required to fix anyway.
Actually I would not expect the difference of results by setting
platform data as [2] or [3].

[1]:
.proc_max_volt = 1360000,
.sram_max_volt = 1360000,

[2]:
.proc_max_volt = 1350000,
.sram_max_volt = 1350000,

[3]:
.proc_max_volt = 1310000,
.sram_max_volt = 1350000,

Since SRAM request will be 1310000 + 200000 = 1510000, and then be
clamped between min volt of 0 and max volt of 1350000. Eventually,
'new_vsram' will be assigned to 1350000 in the case [3], before do-
while tracking.

Furthermore, I've tried to use an easy simulator to verify the function
of mtk_cpufreq_voltage_tracking by iterating all possibilities of
voltage transitions.

The simulator results of [1] is failed as expected, but [2] and [3] are
pass!
The test code and test results are available in attachments, feel free
to check them.

If I'm getting wrong, please let me know.. Thanks!

> > 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.
> > 
> > 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.
> 
> I've introduced dedicated platform_data for MT7623 setting
> proc_max_volt to 1300000, and yes, that does work.
> However, on MT7623 there has not been any problem before as well.
> 
> 
> > 
> > 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
> > > > > > > 
> > > > > 
> > > > > 
> > > > > 

View attachment "mtk_cpufreq_simulator.c" of type "text/x-csrc" (5432 bytes)

View attachment "t1_proc1360_sram1360.log" of type "text/x-log" (31642 bytes)

View attachment "t2_proc1350_sram1350.log" of type "text/x-log" (31936 bytes)

View attachment "t3_proc1310_sram1350.log" of type "text/x-log" (31936 bytes)

Powered by blists - more mailing lists