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
>
