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

[Mario Limonciello <mario.limonciello@xxxxxxx>]

On 8/27/2024 09:42, Gautham R. Shenoy wrote:
> 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?

Both good points.  Will fix for v2.

> --
> 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