Re: [PATCH 08/15] cpufreq/amd-pstate: store all values in cpudata struct in khz

["Gautham R. Shenoy" <gautham.shenoy@amd.com>]

On Thu, Dec 05, 2024 at 04:28:40PM -0600, Mario Limonciello wrote:
> Storing values in the cpudata structure in different units leads
> to confusion and hardcoded conversions elsewhere.  After ratios are
> calculated store everything in khz for any future use. Adjust all
> relevant consumers for this change as well.
> 
> Suggested-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Thanks for cleaning this up.

This also makes the code in-line with the comment for cpudata->nominal_freq which says

 * @nominal_freq: the frequency (in khz) that mapped to nominal_perf

Couple of minor nits. See below.

> ---
>  drivers/cpufreq/amd-pstate-ut.c | 12 +++++-------
>  drivers/cpufreq/amd-pstate.c    | 30 +++++++++++++++---------------
>  2 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
> index a261d7300951e..3a0a380c3590c 100644
> --- a/drivers/cpufreq/amd-pstate-ut.c
> +++ b/drivers/cpufreq/amd-pstate-ut.c
> @@ -207,7 +207,6 @@ 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);
> @@ -215,14 +214,13 @@ static void amd_pstate_ut_check_freq(u32 index)
>  			break;
>  		cpudata = policy->driver_data;
>  
> -		nominal_freq_khz = cpudata->nominal_freq*1000;
> -		if (!((cpudata->max_freq >= nominal_freq_khz) &&
> -			(nominal_freq_khz > cpudata->lowest_nonlinear_freq) &&
> +		if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
> +			(cpudata->nominal_freq > 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, nominal_freq_khz,
> +				__func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
>  				cpudata->lowest_nonlinear_freq, cpudata->min_freq);
>  			goto skip_test;
>  		}
> @@ -236,13 +234,13 @@ static void amd_pstate_ut_check_freq(u32 index)
>  
>  		if (cpudata->boost_supported) {
>  			if ((policy->max == cpudata->max_freq) ||
> -					(policy->max == nominal_freq_khz))
> +					(policy->max == cpudata->nominal_freq))
>  				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,
> -					nominal_freq_khz);
> +					cpudata->nominal_freq);
>  				goto skip_test;
>  			}
>  		} else {
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ce70d1bfa55d0..464db6745d84e 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -739,8 +739,8 @@ static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
>  
>  	if (on)
>  		policy->cpuinfo.max_freq = max_freq;
> -	else if (policy->cpuinfo.max_freq > nominal_freq * 1000)
> -		policy->cpuinfo.max_freq = nominal_freq * 1000;
> +	else if (policy->cpuinfo.max_freq > nominal_freq)
> +		policy->cpuinfo.max_freq = nominal_freq;
>  
>  	policy->max = policy->cpuinfo.max_freq;
>  
> @@ -940,29 +940,29 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>  		return ret;
>  
>  	if (quirks && quirks->lowest_freq)
> -		min_freq = quirks->lowest_freq * 1000;
> +		min_freq = quirks->lowest_freq;
>  	else
> -		min_freq = cppc_perf.lowest_freq * 1000;
> +		min_freq = cppc_perf.lowest_freq;
>  
>  	if (quirks && quirks->nominal_freq)
> -		nominal_freq = quirks->nominal_freq ;
> +		nominal_freq = quirks->nominal_freq;
>  	else
>  		nominal_freq = cppc_perf.nominal_freq;
>  
>  	nominal_perf = READ_ONCE(cpudata->nominal_perf);
>  
>  	boost_ratio = div_u64(cpudata->highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf);
> -	max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
> +	max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT);
>  
>  	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
>  	lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
>  					 nominal_perf);
> -	lowest_nonlinear_freq = (nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
> +	lowest_nonlinear_freq = (nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT);
>  
> -	WRITE_ONCE(cpudata->min_freq, min_freq);
> -	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
> -	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
> -	WRITE_ONCE(cpudata->max_freq, max_freq);
> +	WRITE_ONCE(cpudata->min_freq, min_freq * 1000);
> +	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq * 1000);
> +	WRITE_ONCE(cpudata->nominal_freq, nominal_freq * 1000);
> +	WRITE_ONCE(cpudata->max_freq, max_freq * 1000);
>  
>  	/**
>  	 * Below values need to be initialized correctly, otherwise driver will fail to load
> @@ -970,15 +970,15 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>  	 * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
>  	 * Check _CPC in ACPI table objects if any values are incorrect
>  	 */
> -	if (min_freq <= 0 || max_freq <= 0 || nominal_freq <= 0 || min_freq > max_freq) {

> +	if (min_freq <= 0 || max_freq <= 0 || cpudata->nominal_freq <= 0 || min_freq > max_freq) {
>  		pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n",
> -			min_freq, max_freq, nominal_freq * 1000);
> +			min_freq, max_freq, cpudata->nominal_freq);
>  		return -EINVAL;


We don't need this change. This looks odd since we are using min_freq
and max_freq from local variables, but nominal_freq from cpudata.

We may just as well use the local variables in all the cases since
"cpudata->nominal_freq = nominal_freq * 1000", and thus won't make any
difference to the checks above.


>  	}
>  
> -	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq * 1000) {
                                                                         ^^^^^^^^^^^^^^^^^^^^

Simply replace this with nominal_freq ?

Because both lowest_nonlinear_freq and nominal_freq are in MHz at this
point.

> +	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > cpudata->nominal_freq) {
>  		pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n",
> -			lowest_nonlinear_freq, min_freq, nominal_freq * 1000);
> +			lowest_nonlinear_freq, min_freq, cpudata->nominal_freq);
                                                         ^^^^^^^^^^^^^^^^^^^^^^

Likewise, this can just be nominal_freq
>  		return -EINVAL;
>  	}
>  
> -- 
> 2.43.0
> 

Other than that,
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.