diff mbox series

[V3,10/15] cpufreq: mediatek: Add counter to prevent infinite loop when tracking voltage

Message ID 20220415055916.28350-11-rex-bc.chen@mediatek.com (mailing list archive)
State Superseded, archived
Headers show
Series cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand

Commit Message

Rex-BC Chen (陳柏辰) April 15, 2022, 5:59 a.m. UTC
To prevent infinite loop when tracking voltage, we calculate the maximum
value for each platform data.
We assume min voltage is 0 and tracking target voltage using
min_volt_shift for each iteration.
The retry_max is 3 times of expeted iteration count.

Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Hsin-Yi Wang April 15, 2022, 6:14 a.m. UTC | #1
On Fri, Apr 15, 2022 at 1:59 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote:
>
> To prevent infinite loop when tracking voltage, we calculate the maximum
> value for each platform data.
> We assume min voltage is 0 and tracking target voltage using
> min_volt_shift for each iteration.
> The retry_max is 3 times of expeted iteration count.
>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> ---
>  drivers/cpufreq/mediatek-cpufreq.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index cc44a7a9427a..d4c00237e862 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -86,6 +86,16 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>         struct regulator *proc_reg = info->proc_reg;
>         struct regulator *sram_reg = info->sram_reg;
>         int pre_vproc, pre_vsram, new_vsram, vsram, vproc, ret;
> +       int retry_max;
> +
> +       /*
> +        * We assume min voltage is 0 and tracking target voltage using
> +        * min_volt_shift for each iteration.
> +        * The retry_max is 3 times of expeted iteration count.
> +        */
> +       retry_max = 3 * DIV_ROUND_UP(max(info->soc_data->sram_max_volt,
> +                                        info->soc_data->proc_max_volt),
> +                                    info->soc_data->min_volt_shift);

mtk_cpufreq_voltage_tracking() will be called very frequently.
retry_max is the same every time mtk_cpufreq_voltage_tracking() is
called. Is it better to calculate before and store in
mtk_cpu_dvfs_info?

>
>         pre_vproc = regulator_get_voltage(proc_reg);
>         if (pre_vproc < 0) {
> @@ -151,6 +161,12 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>
>                 pre_vproc = vproc;
>                 pre_vsram = vsram;
> +
> +               if (--retry_max < 0) {
> +                       dev_err(info->cpu_dev,
> +                               "over loop count, failed to set voltage\n");
> +                       return -EINVAL;
> +               }
>         } while (vproc != new_vproc || vsram != new_vsram);
>
>         return 0;
> --
> 2.18.0
>
Rex-BC Chen (陳柏辰) April 15, 2022, 6:29 a.m. UTC | #2
On Fri, 2022-04-15 at 14:14 +0800, Hsin-Yi Wang wrote:
> On Fri, Apr 15, 2022 at 1:59 PM Rex-BC Chen <rex-bc.chen@mediatek.com
> > wrote:
> > 
> > To prevent infinite loop when tracking voltage, we calculate the
> > maximum
> > value for each platform data.
> > We assume min voltage is 0 and tracking target voltage using
> > min_volt_shift for each iteration.
> > The retry_max is 3 times of expeted iteration count.
> > 
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> > ---
> >  drivers/cpufreq/mediatek-cpufreq.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c
> > b/drivers/cpufreq/mediatek-cpufreq.c
> > index cc44a7a9427a..d4c00237e862 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> > @@ -86,6 +86,16 @@ static int mtk_cpufreq_voltage_tracking(struct
> > mtk_cpu_dvfs_info *info,
> >         struct regulator *proc_reg = info->proc_reg;
> >         struct regulator *sram_reg = info->sram_reg;
> >         int pre_vproc, pre_vsram, new_vsram, vsram, vproc, ret;
> > +       int retry_max;
> > +
> > +       /*
> > +        * We assume min voltage is 0 and tracking target voltage
> > using
> > +        * min_volt_shift for each iteration.
> > +        * The retry_max is 3 times of expeted iteration count.
> > +        */
> > +       retry_max = 3 * DIV_ROUND_UP(max(info->soc_data-
> > >sram_max_volt,
> > +                                        info->soc_data-
> > >proc_max_volt),
> > +                                    info->soc_data-
> > >min_volt_shift);
> 
> mtk_cpufreq_voltage_tracking() will be called very frequently.
> retry_max is the same every time mtk_cpufreq_voltage_tracking() is
> called. Is it better to calculate before and store in
> mtk_cpu_dvfs_info?
> 

Hello Hsin-Yi,

Thanks for your reviwew.
I will do this in next version.

BRs,
Rex

> > 
> >         pre_vproc = regulator_get_voltage(proc_reg);
> >         if (pre_vproc < 0) {
> > @@ -151,6 +161,12 @@ static int mtk_cpufreq_voltage_tracking(struct
> > mtk_cpu_dvfs_info *info,
> > 
> >                 pre_vproc = vproc;
> >                 pre_vsram = vsram;
> > +
> > +               if (--retry_max < 0) {
> > +                       dev_err(info->cpu_dev,
> > +                               "over loop count, failed to set
> > voltage\n");
> > +                       return -EINVAL;
> > +               }
> >         } while (vproc != new_vproc || vsram != new_vsram);
> > 
> >         return 0;
> > --
> > 2.18.0
> >
AngeloGioacchino Del Regno April 15, 2022, 12:24 p.m. UTC | #3
Il 15/04/22 08:14, Hsin-Yi Wang ha scritto:
> On Fri, Apr 15, 2022 at 1:59 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote:
>>
>> To prevent infinite loop when tracking voltage, we calculate the maximum
>> value for each platform data.
>> We assume min voltage is 0 and tracking target voltage using
>> min_volt_shift for each iteration.
>> The retry_max is 3 times of expeted iteration count.
>>
>> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>

I'm sorry Rex, but this commit has to be squashed with 09/15, as the logic is
that each commit has to be acceptable, and 09/15 is not, without this fix.

Besides, as Hsin-Yi suggested, calculating this every time may hit performance,
but at the same time I don't want to lose this explicit calculation...

>> ---
>>   drivers/cpufreq/mediatek-cpufreq.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
>> index cc44a7a9427a..d4c00237e862 100644
>> --- a/drivers/cpufreq/mediatek-cpufreq.c
>> +++ b/drivers/cpufreq/mediatek-cpufreq.c
>> @@ -86,6 +86,16 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>>          struct regulator *proc_reg = info->proc_reg;
>>          struct regulator *sram_reg = info->sram_reg;
>>          int pre_vproc, pre_vsram, new_vsram, vsram, vproc, ret;
>> +       int retry_max;
>> +
>> +       /*
>> +        * We assume min voltage is 0 and tracking target voltage using
>> +        * min_volt_shift for each iteration.
>> +        * The retry_max is 3 times of expeted iteration count.
>> +        */
>> +       retry_max = 3 * DIV_ROUND_UP(max(info->soc_data->sram_max_volt,
>> +                                        info->soc_data->proc_max_volt),
>> +                                    info->soc_data->min_volt_shift);
> 
> mtk_cpufreq_voltage_tracking() will be called very frequently.
> retry_max is the same every time mtk_cpufreq_voltage_tracking() is
> called. Is it better to calculate before and store in
> mtk_cpu_dvfs_info?
> 

...so I agree with this solution: perhaps you can add a "vtrack_max" variable to
mtk_cpu_dvfs_info as suggested, and fill in that one in function
mtk_cpu_dvfs_info_init(), where we effectively initialize all-the-things.

Cheers,
Angelo
Rex-BC Chen (陳柏辰) April 18, 2022, 1:40 a.m. UTC | #4
On Fri, 2022-04-15 at 14:24 +0200, AngeloGioacchino Del Regno wrote:
> Il 15/04/22 08:14, Hsin-Yi Wang ha scritto:
> > On Fri, Apr 15, 2022 at 1:59 PM Rex-BC Chen <
> > rex-bc.chen@mediatek.com> wrote:
> > > 
> > > To prevent infinite loop when tracking voltage, we calculate the
> > > maximum
> > > value for each platform data.
> > > We assume min voltage is 0 and tracking target voltage using
> > > min_volt_shift for each iteration.
> > > The retry_max is 3 times of expeted iteration count.
> > > 
> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> 
> I'm sorry Rex, but this commit has to be squashed with 09/15, as the
> logic is
> that each commit has to be acceptable, and 09/15 is not, without this
> fix.
> 
> Besides, as Hsin-Yi suggested, calculating this every time may hit
> performance,
> but at the same time I don't want to lose this explicit
> calculation...
> 

Hello Angelo,

I will squash thius patch into 9/15 and will use info->vtrack_max to
record the value in probe function.

Thanks!

BRs,
Rex

> > > ---
> > >   drivers/cpufreq/mediatek-cpufreq.c | 16 ++++++++++++++++
> > >   1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c
> > > b/drivers/cpufreq/mediatek-cpufreq.c
> > > index cc44a7a9427a..d4c00237e862 100644
> > > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> > > @@ -86,6 +86,16 @@ static int mtk_cpufreq_voltage_tracking(struct
> > > mtk_cpu_dvfs_info *info,
> > >          struct regulator *proc_reg = info->proc_reg;
> > >          struct regulator *sram_reg = info->sram_reg;
> > >          int pre_vproc, pre_vsram, new_vsram, vsram, vproc, ret;
> > > +       int retry_max;
> > > +
> > > +       /*
> > > +        * We assume min voltage is 0 and tracking target voltage
> > > using
> > > +        * min_volt_shift for each iteration.
> > > +        * The retry_max is 3 times of expeted iteration count.
> > > +        */
> > > +       retry_max = 3 * DIV_ROUND_UP(max(info->soc_data-
> > > >sram_max_volt,
> > > +                                        info->soc_data-
> > > >proc_max_volt),
> > > +                                    info->soc_data-
> > > >min_volt_shift);
> > 
> > mtk_cpufreq_voltage_tracking() will be called very frequently.
> > retry_max is the same every time mtk_cpufreq_voltage_tracking() is
> > called. Is it better to calculate before and store in
> > mtk_cpu_dvfs_info?
> > 
> 
> ...so I agree with this solution: perhaps you can add a "vtrack_max"
> variable to
> mtk_cpu_dvfs_info as suggested, and fill in that one in function
> mtk_cpu_dvfs_info_init(), where we effectively initialize all-the-
> things.
> 
> Cheers,
> Angelo
diff mbox series

Patch

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index cc44a7a9427a..d4c00237e862 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -86,6 +86,16 @@  static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 	struct regulator *proc_reg = info->proc_reg;
 	struct regulator *sram_reg = info->sram_reg;
 	int pre_vproc, pre_vsram, new_vsram, vsram, vproc, ret;
+	int retry_max;
+
+	/*
+	 * We assume min voltage is 0 and tracking target voltage using
+	 * min_volt_shift for each iteration.
+	 * The retry_max is 3 times of expeted iteration count.
+	 */
+	retry_max = 3 * DIV_ROUND_UP(max(info->soc_data->sram_max_volt,
+					 info->soc_data->proc_max_volt),
+				     info->soc_data->min_volt_shift);
 
 	pre_vproc = regulator_get_voltage(proc_reg);
 	if (pre_vproc < 0) {
@@ -151,6 +161,12 @@  static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 
 		pre_vproc = vproc;
 		pre_vsram = vsram;
+
+		if (--retry_max < 0) {
+			dev_err(info->cpu_dev,
+				"over loop count, failed to set voltage\n");
+			return -EINVAL;
+		}
 	} while (vproc != new_vproc || vsram != new_vsram);
 
 	return 0;