Re: [PATCH v2 03/11] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()

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

On Tue, Sep 03, 2024 at 03:36:53PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> 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@amd.com>

LGTM

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

--
Thanks and Regards
gautham.

> ---
> v1->v2:
>  * Rename variable from `highest_perf` to `numerator`
>  * Fail when unable to return boost numerator
>  * Move prototype behind CONFIG_ACPI_CPPC_LIB (lkp robot)
> ---
>  arch/x86/include/asm/processor.h |  3 ---
>  arch/x86/kernel/acpi/cppc.c      | 44 +++++++++++++++++++++++---------
>  drivers/cpufreq/acpi-cpufreq.c   | 12 ++++++---
>  include/acpi/cppc_acpi.h         |  5 ++++
>  4 files changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index a75a07f4931fd..775acbdea1a96 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -691,8 +691,6 @@ static inline u32 per_cpu_l2c_id(unsigned int cpu)
>  }
>  
>  #ifdef CONFIG_CPU_SUP_AMD
> -extern u32 amd_get_highest_perf(void);
> -
>  /*
>   * Issue a DIV 0/1 insn to clear any division data from previous DIV
>   * operations.
> @@ -705,7 +703,6 @@ static __always_inline void amd_clear_divider(void)
>  
>  extern void amd_check_microcode(void);
>  #else
> -static inline u32 amd_get_highest_perf(void)		{ return 0; }
>  static inline void amd_clear_divider(void)		{ }
>  static inline void amd_check_microcode(void)		{ }
>  #endif
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
> index 7ec8f2ce859c8..660cfeb6384ba 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -69,7 +69,7 @@ int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
>  static void amd_set_max_freq_ratio(void)
>  {
>  	struct cppc_perf_caps perf_caps;
> -	u64 highest_perf, nominal_perf;
> +	u64 numerator, nominal_perf;
>  	u64 perf_ratio;
>  	int rc;
>  
> @@ -79,15 +79,19 @@ static void amd_set_max_freq_ratio(void)
>  		return;
>  	}
>  
> -	highest_perf = amd_get_highest_perf();
> +	rc = amd_get_boost_ratio_numerator(0, &numerator);
> +	if (rc) {
> +		pr_debug("Could not retrieve highest performance (%d)\n", rc);
> +		return;
> +	}
>  	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;
>  	}
>  
> -	perf_ratio = div_u64(highest_perf * SCHED_CAPACITY_SCALE, nominal_perf);
> +	perf_ratio = div_u64(numerator * SCHED_CAPACITY_SCALE, nominal_perf);
>  	/* midpoint between max_boost and max_P */
>  	perf_ratio = (perf_ratio + SCHED_CAPACITY_SCALE) >> 1;
>  	if (!perf_ratio) {
> @@ -117,18 +121,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 409a7190a4a86..97861abc5f5b8 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -159,6 +159,7 @@ extern int cppc_get_epp_perf(int cpunum, u64 *epp_perf);
>  extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
>  extern int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);
>  extern int cppc_set_auto_sel(int cpu, bool enable);
> +extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
>  #else /* !CONFIG_ACPI_CPPC_LIB */
>  static inline int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
>  {
> @@ -232,6 +233,10 @@ static inline int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf
>  {
>  	return -EOPNOTSUPP;
>  }
> +static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
> +{
> +	return -EOPNOTSUPP;
> +}
>  #endif /* !CONFIG_ACPI_CPPC_LIB */
>  
>  #endif /* _CPPC_ACPI_H*/
> -- 
> 2.43.0
>