[PATCH 0/2] AMD Pstate driver fixes

[PATCH 0/2] AMD Pstate driver fixes

[Dhananjay Ugwekar]

1. Handle the nominal freq units inconsistency in amd-pstate-ut, which was
leading to the below error on inserting the amd-pstate-ut module.

[ 4982.498864] amd_pstate_ut: 1    amd_pstate_ut_acpi_cpc_valid  success!
[ 4982.498873] amd_pstate_ut: 2    amd_pstate_ut_check_enabled   success!
[ 4982.509151] amd_pstate_ut: 3    amd_pstate_ut_check_perf      success!
[ 4982.509155] amd_pstate_ut: amd_pstate_ut_check_freq cpu0 max=3709000  >= nominal=2401 > lowest_nonlinear=1903000 > min=400000 > 0, the formula is incorrect!
[ 4982.509157] amd_pstate_ut: 4    amd_pstate_ut_check_freq      fail!

2. Setting the scaling_min/max_freq on shared memory CPPC systems was
broken in amd-pstate-epp driver(amd_pstate=active mode). The
scaling_min_freq and scaling_max_freq value was not being propagated to
the shared memory area, so the frequency capping was not being honored.

Tested on a AMD Zen3 Milan machine(shared memory CPPC): 

stress-ng is running on the system to keep the CPU utilization at 100%
to test the scaling_max_freq capping.

Before the patch:
We can see below, setting the scaling_max_freq is not taking effect.
 
linux/tools/power/x86/turbostat# ./turbostat --Summary
turbostat version 2023.11.07 - Len Brown <lenb@kernel.org>
[Snip]
cpu0: cpufreq driver: amd-pstate-epp
cpu0: cpufreq governor: performance
[Snip]
Avg_MHz Busy%   Bzy_MHz TSC_MHz IPC     IRQ     POLL    C1      C2      POLL%   C1%     C2%     CorWatt PkgWatt
2620    100.00  2620    2026    0.80    164935  0       0       0       0.00    0.00    0.00    176.07  249.18
2580    100.00  2580    1995    0.80    162208  0       0       0       0.00    0.00    0.00    173.27  245.37
2584    100.00  2584    1998    0.79    162379  0       0       0       0.00    0.00    0.00    173.42  245.68
2577    100.00  2577    1996    0.79    162146  0       0       0       0.00    0.00    0.00    173.15  245.41
2578    100.00  2578    1996    0.80    162025  0       0       0       0.00    0.00    0.00    173.07  245.46
2575    100.00  2575    1996    0.80    162115  0       0       0       0.00    0.00    0.00    172.96  245.41
2576    100.00  2576    1996    0.79    161998  0       0       0       0.00    0.00    0.00    172.87  245.32
linux/tools/power/x86/turbostat# echo 2000000 | tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq
2000000
linux/tools/power/x86/turbostat# cat /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq | uniq
2000000
linux/tools/power/x86/turbostat# ./turbostat --Summary
turbostat version 2023.11.07 - Len Brown <lenb@kernel.org>
[Snip]
cpu0: cpufreq driver: amd-pstate-epp
cpu0: cpufreq governor: performance
[Snip]
Avg_MHz Busy%   Bzy_MHz TSC_MHz IPC     IRQ     POLL    C1      C2      POLL%   C1%     C2%     CorWatt PkgWatt
2620    100.00  2620    2038    0.79    166103  0       0       2       0.00    0.00    0.00    175.44  250.96
2566    100.00  2566    1996    0.79    162038  0       0       0       0.00    0.00    0.00    171.79  245.23
2566    100.00  2566    1996    0.79    162289  0       0       0       0.00    0.00    0.00    171.76  245.59
2571    100.00  2571    1996    0.80    162034  0       0       0       0.00    0.00    0.00    171.69  245.44
2566    100.00  2566    1996    0.79    162179  0       0       0       0.00    0.00    0.00    171.62  245.41
2567    100.00  2567    1996    0.79    162028  0       0       0       0.00    0.00    0.00    171.57  245.46
2567    100.00  2567    1996    0.80    162037  0       0       0       0.00    0.00    0.00    171.53  245.41

After applying the patch:
On setting scaling_max_freq at 2GHz, the CPU frequency gets capped at 2GHz.

linux/tools/power/x86/turbostat# ./turbostat --Summary
turbostat version 2023.11.07 - Len Brown <lenb@kernel.org>
[Snip]
cpu0: cpufreq driver: amd-pstate-epp
cpu0: cpufreq governor: performance
[Snip]
Avg_MHz Busy%   Bzy_MHz TSC_MHz IPC     IRQ     POLL    C1      C2      POLL%   C1%     C2%     CorWatt PkgWatt
2551    100.00  2551    1956    0.80    165998  0       0       0       0.00    0.00    0.00    171.34  231.22
2713    100.00  2713    2078    0.79    175801  0       0       0       0.00    0.00    0.00    181.92  266.13
2594    100.00  2594    1991    0.79    162183  0       0       1       0.00    0.00    0.00    173.99  244.50
2606    100.00  2606    2003    0.79    162632  0       0       0       0.00    0.00    0.00    174.81  246.51
2599    100.00  2599    1996    0.79    162168  0       0       0       0.00    0.00    0.00    174.05  245.46
linux/tools/power/x86/turbostat# echo 2000000 | tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq
2000000
linux/tools/power/x86/turbostat# cat /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq | uniq
2000000
linux/tools/power/x86/turbostat# ./turbostat --Summary
turbostat version 2023.11.07 - Len Brown <lenb@kernel.org>
[Snip]
cpu0: cpufreq driver: amd-pstate-epp
cpu0: cpufreq governor: performance
[Snip]
Avg_MHz Busy%   Bzy_MHz TSC_MHz IPC     IRQ     POLL    C1      C2      POLL%   C1%     C2%     CorWatt PkgWatt
2010    100.00  2010    2030    0.80    165565  0       0       0       0.00    0.00    0.00    101.22  173.52
1975    100.00  1975    1995    0.80    162042  0       0       0       0.00    0.00    0.00    99.03   169.88
1977    100.00  1977    1998    0.80    162559  0       0       0       0.00    0.00    0.00    99.16   170.20
1976    100.00  1976    1996    0.80    162243  0       0       0       0.00    0.00    0.00    99.09   170.08
1976    100.00  1976    1996    0.80    162490  0       0       0       0.00    0.00    0.00    99.17   170.16
1976    100.00  1976    1996    0.80    162056  0       0       0       0.00    0.00    0.00    99.11   170.17


Dhananjay Ugwekar (2):
  cpufreq/amd-pstate-ut: Handle the inconsistency between nominal_freq and
    other *_freq units
  cpufreq/amd-pstate: Fix the scaling_min/max_freq setting on shared
    memory CPPC systems

 drivers/cpufreq/amd-pstate-ut.c | 12 +++++++-----
 drivers/cpufreq/amd-pstate.c    | 10 ++++++++++
 2 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

[Dhananjay Ugwekar]

cpudata->nominal_freq being in MHz whereas other frequencies being in
KHz breaks the amd-pstate-ut frequency sanity check. This fixes it.

Fixes: 14eb1c96e3a3 ("cpufreq: amd-pstate: Add test module for amd-pstate driver")
Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate-ut.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
index fc275d41d51e..66b73c308ce6 100644
--- a/drivers/cpufreq/amd-pstate-ut.c
+++ b/drivers/cpufreq/amd-pstate-ut.c
@@ -202,6 +202,7 @@ static void amd_pstate_ut_check_freq(u32 index)
 	int cpu = 0;
 	struct cpufreq_policy *policy = NULL;
 	struct amd_cpudata *cpudata = NULL;
+	u32 nominal_freq_khz;
 
 	for_each_possible_cpu(cpu) {
 		policy = cpufreq_cpu_get(cpu);
@@ -209,13 +210,14 @@ static void amd_pstate_ut_check_freq(u32 index)
 			break;
 		cpudata = policy->driver_data;
 
-		if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
-			(cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
+		nominal_freq_khz = cpudata->nominal_freq*1000;
+		if (!((cpudata->max_freq >= nominal_freq_khz) &&
+			(nominal_freq_khz > cpudata->lowest_nonlinear_freq) &&
 			(cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
 			(cpudata->min_freq > 0))) {
 			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
 			pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
-				__func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
+				__func__, cpu, cpudata->max_freq, nominal_freq_khz,
 				cpudata->lowest_nonlinear_freq, cpudata->min_freq);
 			goto skip_test;
 		}
@@ -229,13 +231,13 @@ static void amd_pstate_ut_check_freq(u32 index)
 
 		if (cpudata->boost_supported) {
 			if ((policy->max == cpudata->max_freq) ||
-					(policy->max == cpudata->nominal_freq))
+					(policy->max == nominal_freq_khz))
 				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
 			else {
 				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
 				pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
 					__func__, cpu, policy->max, cpudata->max_freq,
-					cpudata->nominal_freq);
+					nominal_freq_khz);
 				goto skip_test;
 			}
 		} else {
-- 
2.34.1

[PATCH 2/2] cpufreq/amd-pstate: Fix the scaling_min/max_freq setting on shared memory CPPC systems

[Dhananjay Ugwekar]

On shared memory CPPC systems, with amd_pstate=active mode, the change
in scaling_min/max_freq doesn't get written to the shared memory
region. Due to this, the writes to the scaling_min/max_freq sysfs file
don't take effect. Fix this by propagating the scaling_min/max_freq
changes to the shared memory region.

Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors")
Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 9ad62dbe8bfb..7c1c96abe5bd 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -264,6 +264,15 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
 			cpudata->epp_cached = epp;
 	} else {
 		perf_ctrls.energy_perf = epp;
+		perf_ctrls.max_perf = cpudata->max_limit_perf;
+		perf_ctrls.min_perf = cpudata->min_limit_perf;
+		perf_ctrls.desired_perf = 0U;
+
+		ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);
+		if (ret) {
+			pr_debug("failed to set min max limits (%d)\n", ret);
+			return ret;
+		}
 		ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
 		if (ret) {
 			pr_debug("failed to set energy perf value (%d)\n", ret);
@@ -1547,6 +1556,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 	}
 
 	WRITE_ONCE(cpudata->cppc_req_cached, value);
+
 	amd_pstate_set_epp(cpudata, epp);
 }
 
-- 
2.34.1

Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

[Dhananjay Ugwekar]

Minor modification, the commit subject is supposed to be 
"cpufreq/amd-pstate-ut: Handle the inconsistency between nominal_freq and other *_freq units"

The second half disappeared due to the word wrapping I guess.

Regards,
Dhananjay

On 6/25/2024 7:11 PM, Dhananjay Ugwekar wrote:
> cpudata->nominal_freq being in MHz whereas other frequencies being in
> KHz breaks the amd-pstate-ut frequency sanity check. This fixes it.
> 
> Fixes: 14eb1c96e3a3 ("cpufreq: amd-pstate: Add test module for amd-pstate driver")
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
>  drivers/cpufreq/amd-pstate-ut.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
> index fc275d41d51e..66b73c308ce6 100644
> --- a/drivers/cpufreq/amd-pstate-ut.c
> +++ b/drivers/cpufreq/amd-pstate-ut.c
> @@ -202,6 +202,7 @@ static void amd_pstate_ut_check_freq(u32 index)
>  	int cpu = 0;
>  	struct cpufreq_policy *policy = NULL;
>  	struct amd_cpudata *cpudata = NULL;
> +	u32 nominal_freq_khz;
>  
>  	for_each_possible_cpu(cpu) {
>  		policy = cpufreq_cpu_get(cpu);
> @@ -209,13 +210,14 @@ static void amd_pstate_ut_check_freq(u32 index)
>  			break;
>  		cpudata = policy->driver_data;
>  
> -		if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
> -			(cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
> +		nominal_freq_khz = cpudata->nominal_freq*1000;
> +		if (!((cpudata->max_freq >= nominal_freq_khz) &&
> +			(nominal_freq_khz > cpudata->lowest_nonlinear_freq) &&
>  			(cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
>  			(cpudata->min_freq > 0))) {
>  			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>  			pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
> -				__func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
> +				__func__, cpu, cpudata->max_freq, nominal_freq_khz,
>  				cpudata->lowest_nonlinear_freq, cpudata->min_freq);
>  			goto skip_test;
>  		}
> @@ -229,13 +231,13 @@ static void amd_pstate_ut_check_freq(u32 index)
>  
>  		if (cpudata->boost_supported) {
>  			if ((policy->max == cpudata->max_freq) ||
> -					(policy->max == cpudata->nominal_freq))
> +					(policy->max == nominal_freq_khz))
>  				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
>  			else {
>  				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>  				pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
>  					__func__, cpu, policy->max, cpudata->max_freq,
> -					cpudata->nominal_freq);
> +					nominal_freq_khz);
>  				goto skip_test;
>  			}
>  		} else {

Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

[Mario Limonciello]

On 6/25/2024 08:41, Dhananjay Ugwekar wrote:
> cpudata->nominal_freq being in MHz whereas other frequencies being in
> KHz breaks the amd-pstate-ut frequency sanity check. This fixes it.
> 
> Fixes: 14eb1c96e3a3 ("cpufreq: amd-pstate: Add test module for amd-pstate driver")
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

The code change below looks fine to me, but I think the tag is wrong. 
It should go with the "fix" that caused the inconsistency.  Here is what
I think the correct tag should be:

Fixes: e4731baaf294 ("cpufreq: amd-pstate: Fix the inconsistency in max 
frequency units")

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/cpufreq/amd-pstate-ut.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
> index fc275d41d51e..66b73c308ce6 100644
> --- a/drivers/cpufreq/amd-pstate-ut.c
> +++ b/drivers/cpufreq/amd-pstate-ut.c
> @@ -202,6 +202,7 @@ static void amd_pstate_ut_check_freq(u32 index)
>   	int cpu = 0;
>   	struct cpufreq_policy *policy = NULL;
>   	struct amd_cpudata *cpudata = NULL;
> +	u32 nominal_freq_khz;
>   
>   	for_each_possible_cpu(cpu) {
>   		policy = cpufreq_cpu_get(cpu);
> @@ -209,13 +210,14 @@ static void amd_pstate_ut_check_freq(u32 index)
>   			break;
>   		cpudata = policy->driver_data;
>   
> -		if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
> -			(cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
> +		nominal_freq_khz = cpudata->nominal_freq*1000;
> +		if (!((cpudata->max_freq >= nominal_freq_khz) &&
> +			(nominal_freq_khz > cpudata->lowest_nonlinear_freq) &&
>   			(cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
>   			(cpudata->min_freq > 0))) {
>   			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>   			pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
> -				__func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
> +				__func__, cpu, cpudata->max_freq, nominal_freq_khz,
>   				cpudata->lowest_nonlinear_freq, cpudata->min_freq);
>   			goto skip_test;
>   		}
> @@ -229,13 +231,13 @@ static void amd_pstate_ut_check_freq(u32 index)
>   
>   		if (cpudata->boost_supported) {
>   			if ((policy->max == cpudata->max_freq) ||
> -					(policy->max == cpudata->nominal_freq))
> +					(policy->max == nominal_freq_khz))
>   				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
>   			else {
>   				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>   				pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
>   					__func__, cpu, policy->max, cpudata->max_freq,
> -					cpudata->nominal_freq);
> +					nominal_freq_khz);
>   				goto skip_test;
>   			}
>   		} else {

Re: [PATCH 2/2] cpufreq/amd-pstate: Fix the scaling_min/max_freq setting on shared memory CPPC systems

[Mario Limonciello]

On 6/25/2024 08:41, Dhananjay Ugwekar wrote:
> On shared memory CPPC systems, with amd_pstate=active mode, the change
> in scaling_min/max_freq doesn't get written to the shared memory
> region. Due to this, the writes to the scaling_min/max_freq sysfs file
> don't take effect. Fix this by propagating the scaling_min/max_freq
> changes to the shared memory region.
> 
> Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors")
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 9ad62dbe8bfb..7c1c96abe5bd 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -264,6 +264,15 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
>   			cpudata->epp_cached = epp;
>   	} else {
>   		perf_ctrls.energy_perf = epp;
> +		perf_ctrls.max_perf = cpudata->max_limit_perf;
> +		perf_ctrls.min_perf = cpudata->min_limit_perf;
> +		perf_ctrls.desired_perf = 0U;
> +
> +		ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);
> +		if (ret) {
> +			pr_debug("failed to set min max limits (%d)\n", ret);
> +			return ret;
> +		}

This feels like a handgrown implementation of amd_pstate_update_perf() 
(IE static call updated to cppc_update_perf).

Can you just call that instead?

>   		ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
>   		if (ret) {
>   			pr_debug("failed to set energy perf value (%d)\n", ret);
> @@ -1547,6 +1556,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>   	}
>   
>   	WRITE_ONCE(cpudata->cppc_req_cached, value);
> +

Spurious newline added here not relevant to this patch.

>   	amd_pstate_set_epp(cpudata, epp);
>   }
>   

Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

[Mario Limonciello]

On 6/25/2024 08:51, Dhananjay Ugwekar wrote:
> Minor modification, the commit subject is supposed to be
> "cpufreq/amd-pstate-ut: Handle the inconsistency between nominal_freq and other *_freq units"
> 
> The second half disappeared due to the word wrapping I guess.

I had some other feedback on the series, so when you submit a v2 can you 
try to fix the title on the first patch?

> 
> Regards,
> Dhananjay
> 
> On 6/25/2024 7:11 PM, Dhananjay Ugwekar wrote:
>> cpudata->nominal_freq being in MHz whereas other frequencies being in
>> KHz breaks the amd-pstate-ut frequency sanity check. This fixes it.
>>
>> Fixes: 14eb1c96e3a3 ("cpufreq: amd-pstate: Add test module for amd-pstate driver")
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>> ---
>>   drivers/cpufreq/amd-pstate-ut.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
>> index fc275d41d51e..66b73c308ce6 100644
>> --- a/drivers/cpufreq/amd-pstate-ut.c
>> +++ b/drivers/cpufreq/amd-pstate-ut.c
>> @@ -202,6 +202,7 @@ static void amd_pstate_ut_check_freq(u32 index)
>>   	int cpu = 0;
>>   	struct cpufreq_policy *policy = NULL;
>>   	struct amd_cpudata *cpudata = NULL;
>> +	u32 nominal_freq_khz;
>>   
>>   	for_each_possible_cpu(cpu) {
>>   		policy = cpufreq_cpu_get(cpu);
>> @@ -209,13 +210,14 @@ static void amd_pstate_ut_check_freq(u32 index)
>>   			break;
>>   		cpudata = policy->driver_data;
>>   
>> -		if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
>> -			(cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
>> +		nominal_freq_khz = cpudata->nominal_freq*1000;
>> +		if (!((cpudata->max_freq >= nominal_freq_khz) &&
>> +			(nominal_freq_khz > cpudata->lowest_nonlinear_freq) &&
>>   			(cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
>>   			(cpudata->min_freq > 0))) {
>>   			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>>   			pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
>> -				__func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
>> +				__func__, cpu, cpudata->max_freq, nominal_freq_khz,
>>   				cpudata->lowest_nonlinear_freq, cpudata->min_freq);
>>   			goto skip_test;
>>   		}
>> @@ -229,13 +231,13 @@ static void amd_pstate_ut_check_freq(u32 index)
>>   
>>   		if (cpudata->boost_supported) {
>>   			if ((policy->max == cpudata->max_freq) ||
>> -					(policy->max == cpudata->nominal_freq))
>> +					(policy->max == nominal_freq_khz))
>>   				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
>>   			else {
>>   				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>>   				pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
>>   					__func__, cpu, policy->max, cpudata->max_freq,
>> -					cpudata->nominal_freq);
>> +					nominal_freq_khz);
>>   				goto skip_test;
>>   			}
>>   		} else {

RE: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

[Meng, Li (Jassmine)]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Dhananjay:
Sorry for the delay.

I think the correction to this issue should be in function amd_pstae_init_freq() of file amd-pstate.c.
The value of norminal_freq should be consistent with other values(max_freq,min_freq etc.).

> -----Original Message-----
> From: Ugwekar, Dhananjay <Dhananjay.Ugwekar@amd.com>
> Sent: Tuesday, June 25, 2024 9:52 PM
> To: rafael@kernel.org; viresh.kumar@linaro.org; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>;
> skhan@linuxfoundation.org; Meng, Li (Jassmine) <Li.Meng@amd.com>;
> Huang, Ray <Ray.Huang@amd.com>
> Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency
>
> Minor modification, the commit subject is supposed to be
> "cpufreq/amd-pstate-ut: Handle the inconsistency between nominal_freq
> and other *_freq units"
>
> The second half disappeared due to the word wrapping I guess.
>
> Regards,
> Dhananjay
>
> On 6/25/2024 7:11 PM, Dhananjay Ugwekar wrote:
> > cpudata->nominal_freq being in MHz whereas other frequencies being in
> > KHz breaks the amd-pstate-ut frequency sanity check. This fixes it.
> >
> > Fixes: 14eb1c96e3a3 ("cpufreq: amd-pstate: Add test module for
> > amd-pstate driver")
> > Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> > ---
> >  drivers/cpufreq/amd-pstate-ut.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate-ut.c
> > b/drivers/cpufreq/amd-pstate-ut.c index fc275d41d51e..66b73c308ce6
> > 100644
> > --- a/drivers/cpufreq/amd-pstate-ut.c
> > +++ b/drivers/cpufreq/amd-pstate-ut.c
> > @@ -202,6 +202,7 @@ static void amd_pstate_ut_check_freq(u32 index)
> >     int cpu = 0;
> >     struct cpufreq_policy *policy = NULL;
> >     struct amd_cpudata *cpudata = NULL;
> > +   u32 nominal_freq_khz;
> >
> >     for_each_possible_cpu(cpu) {
> >             policy = cpufreq_cpu_get(cpu);
> > @@ -209,13 +210,14 @@ static void amd_pstate_ut_check_freq(u32 index)
> >                     break;
> >             cpudata = policy->driver_data;
> >
> > -           if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
> > -                   (cpudata->nominal_freq > cpudata-
> >lowest_nonlinear_freq) &&
> > +           nominal_freq_khz = cpudata->nominal_freq*1000;
> > +           if (!((cpudata->max_freq >= nominal_freq_khz) &&
> > +                   (nominal_freq_khz > cpudata-
> >lowest_nonlinear_freq) &&
> >                     (cpudata->lowest_nonlinear_freq > cpudata-
> >min_freq) &&
> >                     (cpudata->min_freq > 0))) {
> >                     amd_pstate_ut_cases[index].result =
> AMD_PSTATE_UT_RESULT_FAIL;
> >                     pr_err("%s cpu%d max=%d >= nominal=%d >
> lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
> > -                           __func__, cpu, cpudata->max_freq, cpudata-
> >nominal_freq,
> > +                           __func__, cpu, cpudata->max_freq,
> nominal_freq_khz,
> >                             cpudata->lowest_nonlinear_freq, cpudata-
> >min_freq);
> >                     goto skip_test;
> >             }
> > @@ -229,13 +231,13 @@ static void amd_pstate_ut_check_freq(u32 index)
> >
> >             if (cpudata->boost_supported) {
> >                     if ((policy->max == cpudata->max_freq) ||
> > -                                   (policy->max == cpudata-
> >nominal_freq))
> > +                                   (policy->max == nominal_freq_khz))
> >                             amd_pstate_ut_cases[index].result =
> AMD_PSTATE_UT_RESULT_PASS;
> >                     else {
> >                             amd_pstate_ut_cases[index].result =
> AMD_PSTATE_UT_RESULT_FAIL;
> >                             pr_err("%s cpu%d policy_max=%d should be
> equal cpu_max=%d or cpu_nominal=%d !\n",
> >                                     __func__, cpu, policy->max, cpudata-
> >max_freq,
> > -                                   cpudata->nominal_freq);
> > +                                   nominal_freq_khz);
> >                             goto skip_test;
> >                     }
> >             } else {

RE: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

[Gautham R.Shenoy]

"Meng, Li (Jassmine)" <Li.Meng@amd.com> writes:

Hello Jassmine,

>
> Hi Dhananjay:
> Sorry for the delay.
>
> I think the correction to this issue should be in function amd_pstae_init_freq() of file amd-pstate.c.
> The value of norminal_freq should be consistent with other
> values(max_freq,min_freq etc.).

Perry wanted nominal_freq to be in MHz since it is not exposed to the
user via any of the cpufreq sysfs interfaces.

IMO, it woyuld be good to to rename the variables to have their units
for better readability along with a bunch of other
cleanups/code deduplication. But can it be done separately ?


>
>> -----Original Message-----
>> From: Ugwekar, Dhananjay <Dhananjay.Ugwekar@amd.com>
>> Sent: Tuesday, June 25, 2024 9:52 PM
>> To: rafael@kernel.org; viresh.kumar@linaro.org; Shenoy, Gautham Ranjal
>> <gautham.shenoy@amd.com>; Limonciello, Mario
>> <Mario.Limonciello@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>;
>> skhan@linuxfoundation.org; Meng, Li (Jassmine) <Li.Meng@amd.com>;
>> Huang, Ray <Ray.Huang@amd.com>
>> Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency
>>
>> Minor modification, the commit subject is supposed to be
>> "cpufreq/amd-pstate-ut: Handle the inconsistency between nominal_freq
>> and other *_freq units"

How about:

"cpufreq/amd-pstate-ut: Convert nominal_freq to khz during comparisons"

Otherwise the patch looks good to me.

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

--
Thanks and Regards
gautham.

Re: [PATCH 2/2] cpufreq/amd-pstate: Fix the scaling_min/max_freq setting on shared memory CPPC systems

[Gautham R.Shenoy]

Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> writes:

> On shared memory CPPC systems, with amd_pstate=active mode, the change
> in scaling_min/max_freq doesn't get written to the shared memory
> region. Due to this, the writes to the scaling_min/max_freq sysfs file
> don't take effect. Fix this by propagating the scaling_min/max_freq
> changes to the shared memory region.
>
> Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors")
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Please add the following in your v2:

Reported-by: David Arcari <darcari@redhat.com>

> ---
>  drivers/cpufreq/amd-pstate.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 9ad62dbe8bfb..7c1c96abe5bd 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -264,6 +264,15 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
>  			cpudata->epp_cached = epp;
>  	} else {
>  		perf_ctrls.energy_perf = epp;
> +		perf_ctrls.max_perf = cpudata->max_limit_perf;
> +		perf_ctrls.min_perf = cpudata->min_limit_perf;
> +		perf_ctrls.desired_perf = 0U;
> +
> +		ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);
> +		if (ret) {
> +			pr_debug("failed to set min max limits (%d)\n", ret);
> +			return ret;
> +		}
>  		ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
>  		if (ret) {
>  			pr_debug("failed to set energy perf value (%d)\n", ret);
> @@ -1547,6 +1556,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>  	}
>  
>  	WRITE_ONCE(cpudata->cppc_req_cached, value);
> +
>  	amd_pstate_set_epp(cpudata, epp);
>  }
>  
> -- 

--
Thanks and Regards
gautham.

Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

[Dhananjay Ugwekar]

Hello Gautham,

On 6/26/2024 10:52 AM, Gautham R.Shenoy wrote:
> "Meng, Li (Jassmine)" <Li.Meng@amd.com> writes:
> 
> Hello Jassmine,
> 
>>
>> Hi Dhananjay:
>> Sorry for the delay.
>>
>> I think the correction to this issue should be in function amd_pstae_init_freq() of file amd-pstate.c.
>> The value of norminal_freq should be consistent with other
>> values(max_freq,min_freq etc.).
> 
> Perry wanted nominal_freq to be in MHz since it is not exposed to the
> user via any of the cpufreq sysfs interfaces.
> 
> IMO, it woyuld be good to to rename the variables to have their units
> for better readability along with a bunch of other
> cleanups/code deduplication. But can it be done separately ?

Yes that could be part of a separate cleanup patchset.

> 
> 
>>
>>> -----Original Message-----
>>> From: Ugwekar, Dhananjay <Dhananjay.Ugwekar@amd.com>
>>> Sent: Tuesday, June 25, 2024 9:52 PM
>>> To: rafael@kernel.org; viresh.kumar@linaro.org; Shenoy, Gautham Ranjal
>>> <gautham.shenoy@amd.com>; Limonciello, Mario
>>> <Mario.Limonciello@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>;
>>> skhan@linuxfoundation.org; Meng, Li (Jassmine) <Li.Meng@amd.com>;
>>> Huang, Ray <Ray.Huang@amd.com>
>>> Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency
>>>
>>> Minor modification, the commit subject is supposed to be
>>> "cpufreq/amd-pstate-ut: Handle the inconsistency between nominal_freq
>>> and other *_freq units"
> 
> How about:
> 
> "cpufreq/amd-pstate-ut: Convert nominal_freq to khz during comparisons"

Sure, this seems easier to understand and more concise. Will update in v2.

> 
> Otherwise the patch looks good to me.
> 
> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

Thanks for reviewing!

Regards,
Dhananjay

> 
> --
> Thanks and Regards
> gautham.
> 

Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

[Dhananjay Ugwekar]

Hello Mario,

On 6/25/2024 8:41 PM, Mario Limonciello wrote:
> On 6/25/2024 08:51, Dhananjay Ugwekar wrote:
>> Minor modification, the commit subject is supposed to be
>> "cpufreq/amd-pstate-ut: Handle the inconsistency between nominal_freq and other *_freq units"
>>
>> The second half disappeared due to the word wrapping I guess.
> 
> I had some other feedback on the series, so when you submit a v2 can you try to fix the title on the first patch?

Yup, will do!

Thanks,
Dhananjay

> 
>>
>> Regards,
>> Dhananjay
>>
>> On 6/25/2024 7:11 PM, Dhananjay Ugwekar wrote:
>>> cpudata->nominal_freq being in MHz whereas other frequencies being in
>>> KHz breaks the amd-pstate-ut frequency sanity check. This fixes it.
>>>
>>> Fixes: 14eb1c96e3a3 ("cpufreq: amd-pstate: Add test module for amd-pstate driver")
>>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>>> ---
>>>   drivers/cpufreq/amd-pstate-ut.c | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
>>> index fc275d41d51e..66b73c308ce6 100644
>>> --- a/drivers/cpufreq/amd-pstate-ut.c
>>> +++ b/drivers/cpufreq/amd-pstate-ut.c
>>> @@ -202,6 +202,7 @@ static void amd_pstate_ut_check_freq(u32 index)
>>>       int cpu = 0;
>>>       struct cpufreq_policy *policy = NULL;
>>>       struct amd_cpudata *cpudata = NULL;
>>> +    u32 nominal_freq_khz;
>>>         for_each_possible_cpu(cpu) {
>>>           policy = cpufreq_cpu_get(cpu);
>>> @@ -209,13 +210,14 @@ static void amd_pstate_ut_check_freq(u32 index)
>>>               break;
>>>           cpudata = policy->driver_data;
>>>   -        if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
>>> -            (cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
>>> +        nominal_freq_khz = cpudata->nominal_freq*1000;
>>> +        if (!((cpudata->max_freq >= nominal_freq_khz) &&
>>> +            (nominal_freq_khz > cpudata->lowest_nonlinear_freq) &&
>>>               (cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
>>>               (cpudata->min_freq > 0))) {
>>>               amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>>>               pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
>>> -                __func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
>>> +                __func__, cpu, cpudata->max_freq, nominal_freq_khz,
>>>                   cpudata->lowest_nonlinear_freq, cpudata->min_freq);
>>>               goto skip_test;
>>>           }
>>> @@ -229,13 +231,13 @@ static void amd_pstate_ut_check_freq(u32 index)
>>>             if (cpudata->boost_supported) {
>>>               if ((policy->max == cpudata->max_freq) ||
>>> -                    (policy->max == cpudata->nominal_freq))
>>> +                    (policy->max == nominal_freq_khz))
>>>                   amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
>>>               else {
>>>                   amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>>>                   pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
>>>                       __func__, cpu, policy->max, cpudata->max_freq,
>>> -                    cpudata->nominal_freq);
>>> +                    nominal_freq_khz);
>>>                   goto skip_test;
>>>               }
>>>           } else {
> 

Re: [PATCH 2/2] cpufreq/amd-pstate: Fix the scaling_min/max_freq setting on shared memory CPPC systems

[Dhananjay Ugwekar]

Hello Gautham,

On 6/26/2024 10:54 AM, Gautham R.Shenoy wrote:
> Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> writes:
> 
>> On shared memory CPPC systems, with amd_pstate=active mode, the change
>> in scaling_min/max_freq doesn't get written to the shared memory
>> region. Due to this, the writes to the scaling_min/max_freq sysfs file
>> don't take effect. Fix this by propagating the scaling_min/max_freq
>> changes to the shared memory region.
>>
>> Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors")
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> 
> Please add the following in your v2:
> 
> Reported-by: David Arcari <darcari@redhat.com>

Yup, will add.

Thanks,
Dhananjay

> 
>> ---
>>  drivers/cpufreq/amd-pstate.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 9ad62dbe8bfb..7c1c96abe5bd 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -264,6 +264,15 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
>>  			cpudata->epp_cached = epp;
>>  	} else {
>>  		perf_ctrls.energy_perf = epp;
>> +		perf_ctrls.max_perf = cpudata->max_limit_perf;
>> +		perf_ctrls.min_perf = cpudata->min_limit_perf;
>> +		perf_ctrls.desired_perf = 0U;
>> +
>> +		ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);
>> +		if (ret) {
>> +			pr_debug("failed to set min max limits (%d)\n", ret);
>> +			return ret;
>> +		}
>>  		ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
>>  		if (ret) {
>>  			pr_debug("failed to set energy perf value (%d)\n", ret);
>> @@ -1547,6 +1556,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>>  	}
>>  
>>  	WRITE_ONCE(cpudata->cppc_req_cached, value);
>> +
>>  	amd_pstate_set_epp(cpudata, epp);
>>  }
>>  
>> -- 
> 
> --
> Thanks and Regards
> gautham.

Re: [PATCH 1/2] cpufreq/amd-pstate-ut: Handle the inconsistency

[Dhananjay Ugwekar]

Hello Mario,

On 6/25/2024 8:35 PM, Mario Limonciello wrote:
> On 6/25/2024 08:41, Dhananjay Ugwekar wrote:
>> cpudata->nominal_freq being in MHz whereas other frequencies being in
>> KHz breaks the amd-pstate-ut frequency sanity check. This fixes it.
>>
>> Fixes: 14eb1c96e3a3 ("cpufreq: amd-pstate: Add test module for amd-pstate driver")
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> 
> The code change below looks fine to me, but I think the tag is wrong. It should go with the "fix" that caused the inconsistency.  Here is what
> I think the correct tag should be:
> 
> Fixes: e4731baaf294 ("cpufreq: amd-pstate: Fix the inconsistency in max frequency units")

Makes sense, will update the tag.

Thanks for reviewing!

Regards,
Dhananjay

> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> 
>> ---
>>   drivers/cpufreq/amd-pstate-ut.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
>> index fc275d41d51e..66b73c308ce6 100644
>> --- a/drivers/cpufreq/amd-pstate-ut.c
>> +++ b/drivers/cpufreq/amd-pstate-ut.c
>> @@ -202,6 +202,7 @@ static void amd_pstate_ut_check_freq(u32 index)
>>       int cpu = 0;
>>       struct cpufreq_policy *policy = NULL;
>>       struct amd_cpudata *cpudata = NULL;
>> +    u32 nominal_freq_khz;
>>         for_each_possible_cpu(cpu) {
>>           policy = cpufreq_cpu_get(cpu);
>> @@ -209,13 +210,14 @@ static void amd_pstate_ut_check_freq(u32 index)
>>               break;
>>           cpudata = policy->driver_data;
>>   -        if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
>> -            (cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
>> +        nominal_freq_khz = cpudata->nominal_freq*1000;
>> +        if (!((cpudata->max_freq >= nominal_freq_khz) &&
>> +            (nominal_freq_khz > cpudata->lowest_nonlinear_freq) &&
>>               (cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
>>               (cpudata->min_freq > 0))) {
>>               amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>>               pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
>> -                __func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
>> +                __func__, cpu, cpudata->max_freq, nominal_freq_khz,
>>                   cpudata->lowest_nonlinear_freq, cpudata->min_freq);
>>               goto skip_test;
>>           }
>> @@ -229,13 +231,13 @@ static void amd_pstate_ut_check_freq(u32 index)
>>             if (cpudata->boost_supported) {
>>               if ((policy->max == cpudata->max_freq) ||
>> -                    (policy->max == cpudata->nominal_freq))
>> +                    (policy->max == nominal_freq_khz))
>>                   amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
>>               else {
>>                   amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
>>                   pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
>>                       __func__, cpu, policy->max, cpudata->max_freq,
>> -                    cpudata->nominal_freq);
>> +                    nominal_freq_khz);
>>                   goto skip_test;
>>               }
>>           } else {
> 

Re: [PATCH 2/2] cpufreq/amd-pstate: Fix the scaling_min/max_freq setting on shared memory CPPC systems

[Dhananjay Ugwekar]

Hello Mario,

On 6/25/2024 8:39 PM, Mario Limonciello wrote:
> On 6/25/2024 08:41, Dhananjay Ugwekar wrote:
>> On shared memory CPPC systems, with amd_pstate=active mode, the change
>> in scaling_min/max_freq doesn't get written to the shared memory
>> region. Due to this, the writes to the scaling_min/max_freq sysfs file
>> don't take effect. Fix this by propagating the scaling_min/max_freq
>> changes to the shared memory region.
>>
>> Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors")
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>> ---
>>   drivers/cpufreq/amd-pstate.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 9ad62dbe8bfb..7c1c96abe5bd 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -264,6 +264,15 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
>>               cpudata->epp_cached = epp;
>>       } else {
>>           perf_ctrls.energy_perf = epp;
>> +        perf_ctrls.max_perf = cpudata->max_limit_perf;
>> +        perf_ctrls.min_perf = cpudata->min_limit_perf;
>> +        perf_ctrls.desired_perf = 0U;
>> +
>> +        ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);
>> +        if (ret) {
>> +            pr_debug("failed to set min max limits (%d)\n", ret);
>> +            return ret;
>> +        }
> 
> This feels like a handgrown implementation of amd_pstate_update_perf() (IE static call updated to cppc_update_perf).

Yes, I didn't notice it, better to call the existing function.

> 
> Can you just call that instead?
> 
>>           ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
>>           if (ret) {
>>               pr_debug("failed to set energy perf value (%d)\n", ret);
>> @@ -1547,6 +1556,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>>       }
>>         WRITE_ONCE(cpudata->cppc_req_cached, value);
>> +
> 
> Spurious newline added here not relevant to this patch.

Yes, will remove it

Regards,
Dhananjay

>>       amd_pstate_set_epp(cpudata, epp);
>>   }
>>   
>