Re: [PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()

[Gautham R. Shenoy]

Hello Mario,

On Mon, Aug 26, 2024 at 04:13:52PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@xxxxxxx>
> 
> The function name is ambiguous because it returns an intermediate value
> for calculating maximum frequency rather than the CPPC 'Highest Perf'
> register.
> 
> Rename the function to clarify its use and allow the function to return
> errors. Adjust the consumer in acpi-cpufreq to catch errors.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
[..snip..]

> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -79,11 +79,13 @@ static void amd_set_max_freq_ratio(void)
>  		return;
>  	}
>  
> -	highest_perf = amd_get_highest_perf();
> +	rc = amd_get_boost_ratio_numerator(0, &highest_perf);

The variable is still named highest_perf, here! I suppose that will
change in a subsequent patch?



> +	if (rc)
> +		pr_debug("Could not retrieve highest performance\n");

I understand that amd_get_boost_ratio_numerator() always returns a 0
in this patch and thus rc == 0, which means we never enter this "if"
condition.

However, when rc is non-zero, shouldn't this function return after
printing the debug message?

--
Thanks and Regards
gautham.




>  	nominal_perf = perf_caps.nominal_perf;
>  
> -	if (!highest_perf || !nominal_perf) {
> -		pr_debug("Could not retrieve highest or nominal performance\n");
> +	if (!nominal_perf) {
> +		pr_debug("Could not retrieve nominal performance\n");
>  		return;
>  	}
>  
> @@ -117,18 +119,34 @@ void init_freq_invariance_cppc(void)
>  	mutex_unlock(&freq_invariance_lock);
>  }
>  
> -u32 amd_get_highest_perf(void)
> +/**
> + * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
> + * @cpu: CPU to get numerator for.
> + * @numerator: Output variable for numerator.
> + *
> + * Determine the numerator to use for calculating the boost ratio on
> + * a CPU. On systems that support preferred cores, this will be a hardcoded
> + * value. On other systems this will the highest performance register value.
> + *
> + * Return: 0 for success, negative error code otherwise.
> + */
> +int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
>  {
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
>  
>  	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
> -			       (c->x86_model >= 0x70 && c->x86_model < 0x80)))
> -		return 166;
> +			       (c->x86_model >= 0x70 && c->x86_model < 0x80))) {
> +		*numerator = 166;
> +		return 0;
> +	}
>  
>  	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
> -		return 166;
> +			       (c->x86_model >= 0x40 && c->x86_model < 0x70))) {
> +		*numerator = 166;
> +		return 0;
> +	}
> +	*numerator = 255;
>  
> -	return 255;
> +	return 0;
>  }
> -EXPORT_SYMBOL_GPL(amd_get_highest_perf);
> +EXPORT_SYMBOL_GPL(amd_get_boost_ratio_numerator);
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index a8ca625a98b89..0f04feb6cafaf 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -642,10 +642,16 @@ static u64 get_max_boost_ratio(unsigned int cpu)
>  		return 0;
>  	}
>  
> -	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> -		highest_perf = amd_get_highest_perf();
> -	else
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
> +		ret = amd_get_boost_ratio_numerator(cpu, &highest_perf);
> +		if (ret) {
> +			pr_debug("CPU%d: Unable to get boost ratio numerator (%d)\n",
> +				 cpu, ret);
> +			return 0;
> +		}
> +	} else {
>  		highest_perf = perf_caps.highest_perf;
> +	}
>  
>  	nominal_perf = perf_caps.nominal_perf;
>  
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 930b6afba6f4d..f25a881cd46dd 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -136,6 +136,12 @@ struct cppc_cpudata {
>  	cpumask_var_t shared_cpu_map;
>  };
>  
> +#ifdef CONFIG_CPU_SUP_AMD
> +extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
> +#else /* !CONFIG_CPU_SUP_AMD */
> +static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
> +#endif /* !CONFIG_CPU_SUP_AMD */
> +
>  #ifdef CONFIG_ACPI_CPPC_LIB
>  extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
>  extern int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf);
> -- 
> 2.43.0
>