[PATCH 1/4] cpufreq/amd-pstate: Use nominal perf for limits when boost is disabled

[PATCH 1/4] cpufreq/amd-pstate: Use nominal perf for limits when boost is disabled

[Mario Limonciello]

When boost has been disabled the limit for perf should be nominal perf not
the highest perf.  Using the latter to do calculations will lead to
incorrect values that are still above nominal.

Fixes: ad4caad58d91 ("cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()")
Reported-by: Peter Jung <ptr1337@cachyos.org>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219348
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 30415c30d8b4..dfa9a146769b 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -536,11 +536,16 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy)
 
 static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
 {
-	u32 max_limit_perf, min_limit_perf, lowest_perf;
+	u32 max_limit_perf, min_limit_perf, lowest_perf, max_perf;
 	struct amd_cpudata *cpudata = policy->driver_data;
 
-	max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata->max_freq);
-	min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata->max_freq);
+	if (cpudata->boost_supported && !policy->boost_enabled)
+		max_perf = READ_ONCE(cpudata->nominal_perf);
+	else
+		max_perf = READ_ONCE(cpudata->highest_perf);
+
+	max_limit_perf = div_u64(policy->max * max_perf, policy->cpuinfo.max_freq);
+	min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq);
 
 	lowest_perf = READ_ONCE(cpudata->lowest_perf);
 	if (min_limit_perf < lowest_perf)
@@ -1506,10 +1511,13 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 	u64 value;
 	s16 epp;
 
-	max_perf = READ_ONCE(cpudata->highest_perf);
+	if (cpudata->boost_supported && !policy->boost_enabled)
+		max_perf = READ_ONCE(cpudata->nominal_perf);
+	else
+		max_perf = READ_ONCE(cpudata->highest_perf);
 	min_perf = READ_ONCE(cpudata->lowest_perf);
-	max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata->max_freq);
-	min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata->max_freq);
+	max_limit_perf = div_u64(policy->max * max_perf, policy->cpuinfo.max_freq);
+	min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq);
 
 	if (min_limit_perf < min_perf)
 		min_limit_perf = min_perf;
-- 
2.43.0

[PATCH 2/4] cpufreq/amd-pstate: Don't update CPPC request in amd_pstate_cpu_boost_update()

[Mario Limonciello]

When boost is changed the CPPC value is changed in amd_pstate_cpu_boost_update()
but then changed again when refresh_frequency_limits() and all it's callbacks
occur.  The first is a pointless write, so instead just update the limits for
the policy and let the policy refresh anchor everything properly.

Fixes: c8c68c38b56f ("cpufreq: amd-pstate: initialize core precision boost state")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index dfa9a146769b..13dec8b1e7a8 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -665,34 +665,12 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	struct cppc_perf_ctrls perf_ctrls;
-	u32 highest_perf, nominal_perf, nominal_freq, max_freq;
+	u32 nominal_freq, max_freq;
 	int ret = 0;
 
-	highest_perf = READ_ONCE(cpudata->highest_perf);
-	nominal_perf = READ_ONCE(cpudata->nominal_perf);
 	nominal_freq = READ_ONCE(cpudata->nominal_freq);
 	max_freq = READ_ONCE(cpudata->max_freq);
 
-	if (boot_cpu_has(X86_FEATURE_CPPC)) {
-		u64 value = READ_ONCE(cpudata->cppc_req_cached);
-
-		value &= ~GENMASK_ULL(7, 0);
-		value |= on ? highest_perf : nominal_perf;
-		WRITE_ONCE(cpudata->cppc_req_cached, value);
-
-		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
-	} else {
-		perf_ctrls.max_perf = on ? highest_perf : nominal_perf;
-		ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);
-		if (ret) {
-			cpufreq_cpu_release(policy);
-			pr_debug("Failed to set max perf on CPU:%d. ret:%d\n",
-				cpudata->cpu, ret);
-			return ret;
-		}
-	}
-
 	if (on)
 		policy->cpuinfo.max_freq = max_freq;
 	else if (policy->cpuinfo.max_freq > nominal_freq * 1000)
-- 
2.43.0

RE: [PATCH 2/4] cpufreq/amd-pstate: Don't update CPPC request in amd_pstate_cpu_boost_update()

[Yuan, Perry]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Mario,

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Sunday, October 13, 2024 1:45 AM
> To: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com>
> Cc: Yuan, Perry <Perry.Yuan@amd.com>; linux-kernel@vger.kernel.org; linux-
> pm@vger.kernel.org; Ugwekar, Dhananjay <Dhananjay.Ugwekar@amd.com>;
> Limonciello, Mario <Mario.Limonciello@amd.com>
> Subject: [PATCH 2/4] cpufreq/amd-pstate: Don't update CPPC request in
> amd_pstate_cpu_boost_update()
>
> When boost is changed the CPPC value is changed in
> amd_pstate_cpu_boost_update() but then changed again when
> refresh_frequency_limits() and all it's callbacks occur.  The first is a pointless write,
> so instead just update the limits for the policy and let the policy refresh anchor
> everything properly.
>
> Fixes: c8c68c38b56f ("cpufreq: amd-pstate: initialize core precision boost state")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index
> dfa9a146769b..13dec8b1e7a8 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -665,34 +665,12 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
> static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)  {
>       struct amd_cpudata *cpudata = policy->driver_data;
> -     struct cppc_perf_ctrls perf_ctrls;
> -     u32 highest_perf, nominal_perf, nominal_freq, max_freq;
> +     u32 nominal_freq, max_freq;
>       int ret = 0;
>
> -     highest_perf = READ_ONCE(cpudata->highest_perf);
> -     nominal_perf = READ_ONCE(cpudata->nominal_perf);
>       nominal_freq = READ_ONCE(cpudata->nominal_freq);
>       max_freq = READ_ONCE(cpudata->max_freq);
>
> -     if (boot_cpu_has(X86_FEATURE_CPPC)) {
> -             u64 value = READ_ONCE(cpudata->cppc_req_cached);
> -
> -             value &= ~GENMASK_ULL(7, 0);
> -             value |= on ? highest_perf : nominal_perf;
> -             WRITE_ONCE(cpudata->cppc_req_cached, value);

The original idea was to update CPU firmware MSR register to limit Frequency from lowlevel,
If the not updating MSR,  could you please check if the boost off can limit the frequency
from power firmware?
If the limitation is working or not needed,  please pick the flag,

Reviewed-by: Perry Yuan <perry.yuan@amd.com>.


Perry.

> -
> -             wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> -     } else {
> -             perf_ctrls.max_perf = on ? highest_perf : nominal_perf;
> -             ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);
> -             if (ret) {
> -                     cpufreq_cpu_release(policy);
> -                     pr_debug("Failed to set max perf on CPU:%d. ret:%d\n",
> -                             cpudata->cpu, ret);
> -                     return ret;
> -             }
> -     }
> -
>       if (on)
>               policy->cpuinfo.max_freq = max_freq;
>       else if (policy->cpuinfo.max_freq > nominal_freq * 1000)
> --
> 2.43.0

Re: [PATCH 2/4] cpufreq/amd-pstate: Don't update CPPC request in amd_pstate_cpu_boost_update()

[Gautham R. Shenoy]

Hello Mario,

[..snip..]

> >
> > When boost is changed the CPPC value is changed in
> > amd_pstate_cpu_boost_update() but then changed again when
> > refresh_frequency_limits() and all it's callbacks occur.  The first is a pointless write,
> > so instead just update the limits for the policy and let the policy refresh anchor
> > everything properly.
> >
> > Fixes: c8c68c38b56f ("cpufreq: amd-pstate: initialize core precision boost state")
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >  drivers/cpufreq/amd-pstate.c | 24 +-----------------------
> >  1 file changed, 1 insertion(+), 23 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index
> > dfa9a146769b..13dec8b1e7a8 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -665,34 +665,12 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
> > static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)  {
> >       struct amd_cpudata *cpudata = policy->driver_data;
> > -     struct cppc_perf_ctrls perf_ctrls;
> > -     u32 highest_perf, nominal_perf, nominal_freq, max_freq;
> > +     u32 nominal_freq, max_freq;
> >       int ret = 0;
> >
> > -     highest_perf = READ_ONCE(cpudata->highest_perf);
> > -     nominal_perf = READ_ONCE(cpudata->nominal_perf);
> >       nominal_freq = READ_ONCE(cpudata->nominal_freq);
> >       max_freq = READ_ONCE(cpudata->max_freq);
> >
> > -     if (boot_cpu_has(X86_FEATURE_CPPC)) {
> > -             u64 value = READ_ONCE(cpudata->cppc_req_cached);
> > -
> > -             value &= ~GENMASK_ULL(7, 0);
> > -             value |= on ? highest_perf : nominal_perf;
> > -             WRITE_ONCE(cpudata->cppc_req_cached, value);
> > -
> > -             wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);

So, for amd-pstate-epp driver

refresh_frequency_limits()
  --> cpufreq_set_policy()
       --> amd_pstate_epp_set_policy()
           --> amd_pstate_epp_update_limit()

will ensure that the MSR is updated with the is updated with the
value after Patch 1.


> > -     } else {
> > -             perf_ctrls.max_perf = on ? highest_perf : nominal_perf;
> > -             ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);


refresh_frequency_limits()
 --> cpufreq_start_governor()
   --> governor->limits()
      --> cpufreq_policy_apply_limits()
        --> __cpufreq_driver_target()
	   --> amd_pstate_target()
	     --> amd_pstate_update_freq()
	       --> amd_pstate_update()

So these updates in amd_pstate_cpu_boost_update() seem redundant.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

[PATCH 3/4] cpufreq/amd-pstate: Use amd_pstate_update_min_max_limit() for EPP limits

[Mario Limonciello]

When the EPP updates are set the maximum capable frequency for the
CPU is used to set the upper limit instead of that of the policy.

Adjust amd_pstate_epp_update_limit() to reuse policy calculation code
from amd_pstate_update_min_max_limit().

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 13dec8b1e7a8..8d2541f2c74b 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1485,26 +1485,13 @@ static void amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
 static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	u32 max_perf, min_perf, min_limit_perf, max_limit_perf;
+	u32 max_perf, min_perf;
 	u64 value;
 	s16 epp;
 
-	if (cpudata->boost_supported && !policy->boost_enabled)
-		max_perf = READ_ONCE(cpudata->nominal_perf);
-	else
-		max_perf = READ_ONCE(cpudata->highest_perf);
+	max_perf = READ_ONCE(cpudata->highest_perf);
 	min_perf = READ_ONCE(cpudata->lowest_perf);
-	max_limit_perf = div_u64(policy->max * max_perf, policy->cpuinfo.max_freq);
-	min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq);
-
-	if (min_limit_perf < min_perf)
-		min_limit_perf = min_perf;
-
-	if (max_limit_perf < min_limit_perf)
-		max_limit_perf = min_limit_perf;
-
-	WRITE_ONCE(cpudata->max_limit_perf, max_limit_perf);
-	WRITE_ONCE(cpudata->min_limit_perf, min_limit_perf);
+	amd_pstate_update_min_max_limit(policy);
 
 	max_perf = clamp_t(unsigned long, max_perf, cpudata->min_limit_perf,
 			cpudata->max_limit_perf);
-- 
2.43.0

RE: [PATCH 3/4] cpufreq/amd-pstate: Use amd_pstate_update_min_max_limit() for EPP limits

[Yuan, Perry]

[Public]

Hi Mario,

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Sunday, October 13, 2024 1:45 AM
> To: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com>
> Cc: Yuan, Perry <Perry.Yuan@amd.com>; linux-kernel@vger.kernel.org; linux-
> pm@vger.kernel.org; Ugwekar, Dhananjay <Dhananjay.Ugwekar@amd.com>;
> Limonciello, Mario <Mario.Limonciello@amd.com>
> Subject: [PATCH 3/4] cpufreq/amd-pstate: Use
> amd_pstate_update_min_max_limit() for EPP limits
>
> When the EPP updates are set the maximum capable frequency for the CPU is
> used to set the upper limit instead of that of the policy.
>
> Adjust amd_pstate_epp_update_limit() to reuse policy calculation code from
> amd_pstate_update_min_max_limit().
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index
> 13dec8b1e7a8..8d2541f2c74b 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1485,26 +1485,13 @@ static void amd_pstate_epp_cpu_exit(struct
> cpufreq_policy *policy)  static int amd_pstate_epp_update_limit(struct
> cpufreq_policy *policy)  {
>       struct amd_cpudata *cpudata = policy->driver_data;
> -     u32 max_perf, min_perf, min_limit_perf, max_limit_perf;
> +     u32 max_perf, min_perf;
>       u64 value;
>       s16 epp;
>
> -     if (cpudata->boost_supported && !policy->boost_enabled)
> -             max_perf = READ_ONCE(cpudata->nominal_perf);
> -     else
> -             max_perf = READ_ONCE(cpudata->highest_perf);
> +     max_perf = READ_ONCE(cpudata->highest_perf);
>       min_perf = READ_ONCE(cpudata->lowest_perf);
> -     max_limit_perf = div_u64(policy->max * max_perf, policy-
> >cpuinfo.max_freq);
> -     min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq);
> -
> -     if (min_limit_perf < min_perf)
> -             min_limit_perf = min_perf;
> -
> -     if (max_limit_perf < min_limit_perf)
> -             max_limit_perf = min_limit_perf;
> -
> -     WRITE_ONCE(cpudata->max_limit_perf, max_limit_perf);
> -     WRITE_ONCE(cpudata->min_limit_perf, min_limit_perf);
> +     amd_pstate_update_min_max_limit(policy);
>
>       max_perf = clamp_t(unsigned long, max_perf, cpudata->min_limit_perf,
>                       cpudata->max_limit_perf);
> --
> 2.43.0

LGTM, thanks for the optimization.

Reviewed-by: Perry Yuan <perry.yuan@amd.com>


Best Regards.

Perry.

Re: [PATCH 3/4] cpufreq/amd-pstate: Use amd_pstate_update_min_max_limit() for EPP limits

[Gautham R. Shenoy]

Hello Mario,

On Sat, Oct 12, 2024 at 12:45:18PM -0500, Mario Limonciello wrote:
> When the EPP updates are set the maximum capable frequency for the
> CPU is used to set the upper limit instead of that of the policy.
> 
> Adjust amd_pstate_epp_update_limit() to reuse policy calculation code
> from amd_pstate_update_min_max_limit().
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  drivers/cpufreq/amd-pstate.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 13dec8b1e7a8..8d2541f2c74b 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1485,26 +1485,13 @@ static void amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
>  static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	u32 max_perf, min_perf, min_limit_perf, max_limit_perf;
> +	u32 max_perf, min_perf;
>  	u64 value;
>  	s16 epp;
>  
> -	if (cpudata->boost_supported && !policy->boost_enabled)
> -		max_perf = READ_ONCE(cpudata->nominal_perf);
> -	else
> -		max_perf = READ_ONCE(cpudata->highest_perf);
> +	max_perf = READ_ONCE(cpudata->highest_perf);
>  	min_perf = READ_ONCE(cpudata->lowest_perf);
> -	max_limit_perf = div_u64(policy->max * max_perf, policy->cpuinfo.max_freq);
> -	min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq);
> -
> -	if (min_limit_perf < min_perf)
> -		min_limit_perf = min_perf;
> -
> -	if (max_limit_perf < min_limit_perf)
> -		max_limit_perf = min_limit_perf;
> -
> -	WRITE_ONCE(cpudata->max_limit_perf, max_limit_perf);
> -	WRITE_ONCE(cpudata->min_limit_perf, min_limit_perf);
> +	amd_pstate_update_min_max_limit(policy);
>  
>  	max_perf = clamp_t(unsigned long, max_perf, cpudata->min_limit_perf,
>  			cpudata->max_limit_perf);
> -- 
> 2.43.0
> 

[PATCH 4/4] cpufreq/amd-pstate: Drop needless EPP initialization

[Mario Limonciello]

The EPP value doesn't need to be cached to the CPPC request in
amd_pstate_epp_update_limit() because it's passed as an argument
at the end to amd_pstate_set_epp() and stored at that time.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 8d2541f2c74b..90868c8b214e 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1528,12 +1528,6 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
 		epp = 0;
 
-	/* Set initial EPP value */
-	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
-		value &= ~GENMASK_ULL(31, 24);
-		value |= (u64)epp << 24;
-	}
-
 	WRITE_ONCE(cpudata->cppc_req_cached, value);
 	return amd_pstate_set_epp(cpudata, epp);
 }
-- 
2.43.0

Re: [PATCH 4/4] cpufreq/amd-pstate: Drop needless EPP initialization

[Dhananjay Ugwekar]

Hello Mario,

On 10/12/2024 11:15 PM, Mario Limonciello wrote:
> The EPP value doesn't need to be cached to the CPPC request in
> amd_pstate_epp_update_limit() because it's passed as an argument
> at the end to amd_pstate_set_epp() and stored at that time.
> 

Tested this on an AMD Zen4 EPYC server system, ran some sanity tests, 
both modes (active and passive) seem to be working fine with the boost 
disabled and enabled.

You may add,
Tested-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Regards,
Dhananjay

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 8d2541f2c74b..90868c8b214e 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1528,12 +1528,6 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>  	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>  		epp = 0;
>  
> -	/* Set initial EPP value */
> -	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -		value &= ~GENMASK_ULL(31, 24);
> -		value |= (u64)epp << 24;
> -	}
> -
>  	WRITE_ONCE(cpudata->cppc_req_cached, value);
>  	return amd_pstate_set_epp(cpudata, epp);
>  }

RE: [PATCH 4/4] cpufreq/amd-pstate: Drop needless EPP initialization

[Yuan, Perry]

[Public]

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Sunday, October 13, 2024 1:45 AM
> To: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com>
> Cc: Yuan, Perry <Perry.Yuan@amd.com>; linux-kernel@vger.kernel.org; linux-
> pm@vger.kernel.org; Ugwekar, Dhananjay <Dhananjay.Ugwekar@amd.com>;
> Limonciello, Mario <Mario.Limonciello@amd.com>
> Subject: [PATCH 4/4] cpufreq/amd-pstate: Drop needless EPP initialization
>
> The EPP value doesn't need to be cached to the CPPC request in
> amd_pstate_epp_update_limit() because it's passed as an argument at the end to
> amd_pstate_set_epp() and stored at that time.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index
> 8d2541f2c74b..90868c8b214e 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1528,12 +1528,6 @@ static int amd_pstate_epp_update_limit(struct
> cpufreq_policy *policy)
>       if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>               epp = 0;
>
> -     /* Set initial EPP value */
> -     if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -             value &= ~GENMASK_ULL(31, 24);
> -             value |= (u64)epp << 24;
> -     }
> -
>       WRITE_ONCE(cpudata->cppc_req_cached, value);
>       return amd_pstate_set_epp(cpudata, epp);  }
> --
> 2.43.0

LGTM, thanks.

Reviewed-by: Perry Yuan <perry.yuan@amd.com>

Re: [PATCH 4/4] cpufreq/amd-pstate: Drop needless EPP initialization

[Gautham R. Shenoy]

On Sat, Oct 12, 2024 at 12:45:19PM -0500, Mario Limonciello wrote:
> The EPP value doesn't need to be cached to the CPPC request in
> amd_pstate_epp_update_limit() because it's passed as an argument
> at the end to amd_pstate_set_epp() and stored at that time.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Thanks for cleaning it up.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  drivers/cpufreq/amd-pstate.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 8d2541f2c74b..90868c8b214e 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1528,12 +1528,6 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>  	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>  		epp = 0;
>  
> -	/* Set initial EPP value */
> -	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -		value &= ~GENMASK_ULL(31, 24);
> -		value |= (u64)epp << 24;
> -	}
> -
>  	WRITE_ONCE(cpudata->cppc_req_cached, value);
>  	return amd_pstate_set_epp(cpudata, epp);
>  }
> -- 
> 2.43.0
> 

Re: [PATCH 1/4] cpufreq/amd-pstate: Use nominal perf for limits when boost is disabled

[Gautham R. Shenoy]

Hello Mario,

On Sat, Oct 12, 2024 at 12:45:16PM -0500, Mario Limonciello wrote:
> When boost has been disabled the limit for perf should be nominal perf not
> the highest perf.  Using the latter to do calculations will lead to
> incorrect values that are still above nominal.
> 
> Fixes: ad4caad58d91 ("cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()")
> Reported-by: Peter Jung <ptr1337@cachyos.org>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219348
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

The patch looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  drivers/cpufreq/amd-pstate.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 30415c30d8b4..dfa9a146769b 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -536,11 +536,16 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy)
>  
>  static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
>  {
> -	u32 max_limit_perf, min_limit_perf, lowest_perf;
> +	u32 max_limit_perf, min_limit_perf, lowest_perf, max_perf;
>  	struct amd_cpudata *cpudata = policy->driver_data;
>  
> -	max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata->max_freq);
> -	min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata->max_freq);
> +	if (cpudata->boost_supported && !policy->boost_enabled)
> +		max_perf = READ_ONCE(cpudata->nominal_perf);
> +	else
> +		max_perf = READ_ONCE(cpudata->highest_perf);
> +
> +	max_limit_perf = div_u64(policy->max * max_perf, policy->cpuinfo.max_freq);
> +	min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq);


>  
>  	lowest_perf = READ_ONCE(cpudata->lowest_perf);
>  	if (min_limit_perf < lowest_perf)
> @@ -1506,10 +1511,13 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>  	u64 value;
>  	s16 epp;
>  
> -	max_perf = READ_ONCE(cpudata->highest_perf);
> +	if (cpudata->boost_supported && !policy->boost_enabled)
> +		max_perf = READ_ONCE(cpudata->nominal_perf);
> +	else
> +		max_perf = READ_ONCE(cpudata->highest_perf);
>  	min_perf = READ_ONCE(cpudata->lowest_perf);
> -	max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata->max_freq);
> -	min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata->max_freq);
> +	max_limit_perf = div_u64(policy->max * max_perf, policy->cpuinfo.max_freq);
> +	min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq);
>  
>  	if (min_limit_perf < min_perf)
>  		min_limit_perf = min_perf;
> -- 
> 2.43.0
> 

RE: [PATCH 1/4] cpufreq/amd-pstate: Use nominal perf for limits when boost is disabled

[Yuan, Perry]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Sunday, October 13, 2024 1:45 AM
> To: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com>
> Cc: Yuan, Perry <Perry.Yuan@amd.com>; linux-kernel@vger.kernel.org; linux-
> pm@vger.kernel.org; Ugwekar, Dhananjay <Dhananjay.Ugwekar@amd.com>;
> Limonciello, Mario <Mario.Limonciello@amd.com>; Peter Jung
> <ptr1337@cachyos.org>
> Subject: [PATCH 1/4] cpufreq/amd-pstate: Use nominal perf for limits when boost is
> disabled
>
> When boost has been disabled the limit for perf should be nominal perf not the
> highest perf.  Using the latter to do calculations will lead to incorrect values that are
> still above nominal.
>
> Fixes: ad4caad58d91 ("cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set()
> into amd_get_boost_ratio_numerator()")
> Reported-by: Peter Jung <ptr1337@cachyos.org>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219348
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index
> 30415c30d8b4..dfa9a146769b 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -536,11 +536,16 @@ static int amd_pstate_verify(struct cpufreq_policy_data
> *policy)
>
>  static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)  {
> -     u32 max_limit_perf, min_limit_perf, lowest_perf;
> +     u32 max_limit_perf, min_limit_perf, lowest_perf, max_perf;
>       struct amd_cpudata *cpudata = policy->driver_data;
>
> -     max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata-
> >max_freq);
> -     min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata-
> >max_freq);
> +     if (cpudata->boost_supported && !policy->boost_enabled)
> +             max_perf = READ_ONCE(cpudata->nominal_perf);
> +     else
> +             max_perf = READ_ONCE(cpudata->highest_perf);
> +
> +     max_limit_perf = div_u64(policy->max * max_perf, policy-
> >cpuinfo.max_freq);
> +     min_limit_perf = div_u64(policy->min * max_perf,
> +policy->cpuinfo.max_freq);
>
>       lowest_perf = READ_ONCE(cpudata->lowest_perf);
>       if (min_limit_perf < lowest_perf)
> @@ -1506,10 +1511,13 @@ static int amd_pstate_epp_update_limit(struct
> cpufreq_policy *policy)
>       u64 value;
>       s16 epp;
>
> -     max_perf = READ_ONCE(cpudata->highest_perf);
> +     if (cpudata->boost_supported && !policy->boost_enabled)
> +             max_perf = READ_ONCE(cpudata->nominal_perf);
> +     else
> +             max_perf = READ_ONCE(cpudata->highest_perf);
>       min_perf = READ_ONCE(cpudata->lowest_perf);
> -     max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata-
> >max_freq);
> -     min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata-
> >max_freq);
> +     max_limit_perf = div_u64(policy->max * max_perf, policy-
> >cpuinfo.max_freq);
> +     min_limit_perf = div_u64(policy->min * max_perf,
> +policy->cpuinfo.max_freq);
>
>       if (min_limit_perf < min_perf)
>               min_limit_perf = min_perf;
> --
> 2.43.0

LGTM, thanks for the fix.

Reviewed-by: Perry Yuan <perry.yuan@amd.com>