[PATCH] cpufreq/amd-pstate: Refactor max frequency calculation

[PATCH] cpufreq/amd-pstate: Refactor max frequency calculation

[Naresh Solanki]

Refactor to calculate max-freq more accurately.

Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
---
 drivers/cpufreq/amd-pstate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index d7630bab2516..78a2cbd14952 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -892,7 +892,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 	u64 numerator;
 	u32 nominal_perf, nominal_freq;
 	u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
-	u32 boost_ratio, lowest_nonlinear_ratio;
+	u32 lowest_nonlinear_ratio;
 	struct cppc_perf_caps cppc_perf;
 
 	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
@@ -914,8 +914,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 	ret = amd_get_boost_ratio_numerator(cpudata->cpu, &numerator);
 	if (ret)
 		return ret;
-	boost_ratio = div_u64(numerator << SCHED_CAPACITY_SHIFT, nominal_perf);
-	max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
+	max_freq = div_u64(numerator * nominal_freq * 1000, nominal_perf);
 
 	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
 	lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
-- 
2.42.0

Re: [PATCH] cpufreq/amd-pstate: Refactor max frequency calculation

[Mario Limonciello]

On 12/18/2024 13:00, Naresh Solanki wrote:
> Refactor to calculate max-freq more accurately.

Can you add some more detail about what you're finding?
What was it before, what is it now, why is it more accurate?

> 
> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index d7630bab2516..78a2cbd14952 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -892,7 +892,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>   	u64 numerator;
>   	u32 nominal_perf, nominal_freq;
>   	u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
> -	u32 boost_ratio, lowest_nonlinear_ratio;
> +	u32 lowest_nonlinear_ratio;
>   	struct cppc_perf_caps cppc_perf;
>   
>   	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> @@ -914,8 +914,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>   	ret = amd_get_boost_ratio_numerator(cpudata->cpu, &numerator);
>   	if (ret)
>   		return ret;
> -	boost_ratio = div_u64(numerator << SCHED_CAPACITY_SHIFT, nominal_perf);
> -	max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
> +	max_freq = div_u64(numerator * nominal_freq * 1000, nominal_perf);

This doesn't apply currently, because of some changes in the 
superm1.git/linux-next branch; specifically:

https://git.kernel.org/superm1/c/68cb0e77b6439

I haven't sent this out to linux-pm yet so it could be in linux-next, 
but will be doing that soon.  So can you please rebase on that branch if 
this change still makes sense?

>   
>   	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
>   	lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,

Re: [PATCH] cpufreq/amd-pstate: Refactor max frequency calculation

[Naresh Solanki]

Hi ,

On Thu, 19 Dec 2024 at 00:50, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 12/18/2024 13:00, Naresh Solanki wrote:
> > Refactor to calculate max-freq more accurately.
>
> Can you add some more detail about what you're finding?
> What was it before, what is it now, why is it more accurate?
Sure.
The previous approach had some roundoff error due to division compute
when calculating boost ratio.
Which intern affected max-freq calculation resulting in reporting lower value
In my Glinda SoC board,
See below calculations with below values:
max_perf = 208
nominal_perf = 100
normal_freq = 2600

How linux kernel calculates:
freq = ( (max_perf * 1024 / nominal_perf) * normal_freq) / 1024
freq = 5405 // Integer arithmatic.

With the changes
freq = (max_perf * normal_freq) / nominal_perf
freq = 5408



>
> >
> > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> > ---
> >   drivers/cpufreq/amd-pstate.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > index d7630bab2516..78a2cbd14952 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -892,7 +892,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> >       u64 numerator;
> >       u32 nominal_perf, nominal_freq;
> >       u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
> > -     u32 boost_ratio, lowest_nonlinear_ratio;
> > +     u32 lowest_nonlinear_ratio;
> >       struct cppc_perf_caps cppc_perf;
> >
> >       ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> > @@ -914,8 +914,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> >       ret = amd_get_boost_ratio_numerator(cpudata->cpu, &numerator);
> >       if (ret)
> >               return ret;
> > -     boost_ratio = div_u64(numerator << SCHED_CAPACITY_SHIFT, nominal_perf);
> > -     max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
> > +     max_freq = div_u64(numerator * nominal_freq * 1000, nominal_perf);
>
> This doesn't apply currently, because of some changes in the
> superm1.git/linux-next branch; specifically:
>
> https://git.kernel.org/superm1/c/68cb0e77b6439
>
> I haven't sent this out to linux-pm yet so it could be in linux-next,
> but will be doing that soon.  So can you please rebase on that branch if
> this change still makes sense?
Sure. Will do

Regards,
Naresh Solanki
>
> >
> >       lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> >       lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
>

Re: [PATCH] cpufreq/amd-pstate: Refactor max frequency calculation

[Mario Limonciello]

On 12/19/2024 03:21, Naresh Solanki wrote:
> Hi ,
> 
> On Thu, 19 Dec 2024 at 00:50, Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 12/18/2024 13:00, Naresh Solanki wrote:
>>> Refactor to calculate max-freq more accurately.
>>
>> Can you add some more detail about what you're finding?
>> What was it before, what is it now, why is it more accurate?
> Sure.
> The previous approach had some roundoff error due to division compute
> when calculating boost ratio.
> Which intern affected max-freq calculation resulting in reporting lower value
> In my Glinda SoC board,
> See below calculations with below values:
> max_perf = 208
> nominal_perf = 100
> normal_freq = 2600
> 
> How linux kernel calculates:
> freq = ( (max_perf * 1024 / nominal_perf) * normal_freq) / 1024
> freq = 5405 // Integer arithmatic.
> 
> With the changes
> freq = (max_perf * normal_freq) / nominal_perf
> freq = 5408
> 
> 

Ah, thanks!  Please include some of this info in the commit message for 
the next version.

> 
>>
>>>
>>> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
>>> ---
>>>    drivers/cpufreq/amd-pstate.c | 5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>>> index d7630bab2516..78a2cbd14952 100644
>>> --- a/drivers/cpufreq/amd-pstate.c
>>> +++ b/drivers/cpufreq/amd-pstate.c
>>> @@ -892,7 +892,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>>>        u64 numerator;
>>>        u32 nominal_perf, nominal_freq;
>>>        u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
>>> -     u32 boost_ratio, lowest_nonlinear_ratio;
>>> +     u32 lowest_nonlinear_ratio;
>>>        struct cppc_perf_caps cppc_perf;
>>>
>>>        ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
>>> @@ -914,8 +914,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>>>        ret = amd_get_boost_ratio_numerator(cpudata->cpu, &numerator);
>>>        if (ret)
>>>                return ret;
>>> -     boost_ratio = div_u64(numerator << SCHED_CAPACITY_SHIFT, nominal_perf);
>>> -     max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
>>> +     max_freq = div_u64(numerator * nominal_freq * 1000, nominal_perf);
>>
>> This doesn't apply currently, because of some changes in the
>> superm1.git/linux-next branch; specifically:
>>
>> https://git.kernel.org/superm1/c/68cb0e77b6439
>>
>> I haven't sent this out to linux-pm yet so it could be in linux-next,
>> but will be doing that soon.  So can you please rebase on that branch if
>> this change still makes sense?
> Sure. Will do
> 

Thanks!

> Regards,
> Naresh Solanki
>>
>>>
>>>        lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
>>>        lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
>>

Re: [PATCH] cpufreq/amd-pstate: Refactor max frequency calculation

[Naresh Solanki]

Hi Mario

On Thu, 19 Dec 2024 at 20:31, Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 12/19/2024 03:21, Naresh Solanki wrote:
> > Hi ,
> >
> > On Thu, 19 Dec 2024 at 00:50, Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 12/18/2024 13:00, Naresh Solanki wrote:
> >>> Refactor to calculate max-freq more accurately.
> >>
> >> Can you add some more detail about what you're finding?
> >> What was it before, what is it now, why is it more accurate?
> > Sure.
> > The previous approach had some roundoff error due to division compute
> > when calculating boost ratio.
> > Which intern affected max-freq calculation resulting in reporting lower value
> > In my Glinda SoC board,
> > See below calculations with below values:
> > max_perf = 208
> > nominal_perf = 100
> > normal_freq = 2600
> >
> > How linux kernel calculates:
> > freq = ( (max_perf * 1024 / nominal_perf) * normal_freq) / 1024
> > freq = 5405 // Integer arithmatic.
> >
> > With the changes
> > freq = (max_perf * normal_freq) / nominal_perf
> > freq = 5408
> >
> >
>
> Ah, thanks!  Please include some of this info in the commit message for
> the next version.
Sure
>
> >
> >>
> >>>
> >>> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> >>> ---
> >>>    drivers/cpufreq/amd-pstate.c | 5 ++---
> >>>    1 file changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> >>> index d7630bab2516..78a2cbd14952 100644
> >>> --- a/drivers/cpufreq/amd-pstate.c
> >>> +++ b/drivers/cpufreq/amd-pstate.c
> >>> @@ -892,7 +892,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> >>>        u64 numerator;
> >>>        u32 nominal_perf, nominal_freq;
> >>>        u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
> >>> -     u32 boost_ratio, lowest_nonlinear_ratio;
> >>> +     u32 lowest_nonlinear_ratio;
> >>>        struct cppc_perf_caps cppc_perf;
> >>>
> >>>        ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> >>> @@ -914,8 +914,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> >>>        ret = amd_get_boost_ratio_numerator(cpudata->cpu, &numerator);
> >>>        if (ret)
> >>>                return ret;
> >>> -     boost_ratio = div_u64(numerator << SCHED_CAPACITY_SHIFT, nominal_perf);
> >>> -     max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
> >>> +     max_freq = div_u64(numerator * nominal_freq * 1000, nominal_perf);
> >>
> >> This doesn't apply currently, because of some changes in the
> >> superm1.git/linux-next branch; specifically:
> >>
> >> https://git.kernel.org/superm1/c/68cb0e77b6439
> >>
> >> I haven't sent this out to linux-pm yet so it could be in linux-next,
> >> but will be doing that soon.  So can you please rebase on that branch if
> >> this change still makes sense?
> > Sure. Will do
> >
>
> Thanks!
Regards,
Naresh
>
> > Regards,
> > Naresh Solanki
> >>
> >>>
> >>>        lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> >>>        lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
> >>
>