Re: [PATCH v3 08/21] cpufreq: amd: add acpi cppc function as the backend for legacy processors

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 10/29/2021 08:02, Huang Rui wrote:
> In some old Zen based processors, they are using the shared memory that
> exposed from ACPI SBIOS.

I don't think this is only "old" processors.  I think there are "new" 
processors that just don't happen to implement the MSR too.

> 
> Signed-off-by: Jinzhou Su <Jinzhou.Su@amd.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 58 ++++++++++++++++++++++++++++++++----
>   1 file changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 55ff03f85608..d399938d6d85 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -73,6 +73,19 @@ static inline int pstate_enable(bool enable)
>   	return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable ? 1 : 0);
>   }
>   
> +static int cppc_enable(bool enable)
> +{
> +	int cpu, ret = 0;
> +
> +	for_each_online_cpu(cpu) {

I wonder if this should also be changed to present CPU instead of 
offline CPU.  Otherwise could this turn into a situation that the user 
starts with some CPU's offlined and enables them later but this doesn't 
end up applying to the CPUs that were started offlined and changed?

> +		ret = cppc_set_enable(cpu, enable ? 1 : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return ret;
> +}
> +
>   DEFINE_STATIC_CALL(amd_pstate_enable, pstate_enable);
>   
>   static inline int amd_pstate_enable(bool enable)
> @@ -103,6 +116,24 @@ static int pstate_init_perf(struct amd_cpudata *cpudata)
>   	return 0;
>   }
>   
> +static int cppc_init_perf(struct amd_cpudata *cpudata)
> +{
> +	struct cppc_perf_caps cppc_perf;
> +
> +	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> +	if (ret)
> +		return ret;
> +
> +	WRITE_ONCE(cpudata->highest_perf, amd_get_highest_perf());
> +
> +	WRITE_ONCE(cpudata->nominal_perf, cppc_perf.nominal_perf);
> +	WRITE_ONCE(cpudata->lowest_nonlinear_perf,
> +		   cppc_perf.lowest_nonlinear_perf);
> +	WRITE_ONCE(cpudata->lowest_perf, cppc_perf.lowest_perf);
> +
> +	return 0;
> +}
> +
>   DEFINE_STATIC_CALL(amd_pstate_init_perf, pstate_init_perf);
>   
>   static inline int amd_pstate_init_perf(struct amd_cpudata *cpudata)
> @@ -120,6 +151,19 @@ static void pstate_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
>   			      READ_ONCE(cpudata->cppc_req_cached));
>   }
>   
> +static void cppc_update_perf(struct amd_cpudata *cpudata,
> +			     u32 min_perf, u32 des_perf,
> +			     u32 max_perf, bool fast_switch)
> +{
> +	struct cppc_perf_ctrls perf_ctrls;
> +
> +	perf_ctrls.max_perf = max_perf;
> +	perf_ctrls.min_perf = min_perf;
> +	perf_ctrls.desired_perf = des_perf;
> +
> +	cppc_set_perf(cpudata->cpu, &perf_ctrls);
> +}
> +
>   DEFINE_STATIC_CALL(amd_pstate_update_perf, pstate_update_perf);
>   
>   static inline void amd_pstate_update_perf(struct amd_cpudata *cpudata,
> @@ -346,7 +390,8 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>   	/* It will be updated by governor */
>   	policy->cur = policy->cpuinfo.min_freq;
>   
> -	policy->fast_switch_possible = true;
> +	if (boot_cpu_has(X86_FEATURE_AMD_CPPC))
> +		policy->fast_switch_possible = true;
>   
>   	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
>   				   FREQ_QOS_MIN, policy->cpuinfo.min_freq);
> @@ -397,7 +442,6 @@ static struct cpufreq_driver amd_pstate_driver = {
>   	.flags		= CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
>   	.verify		= amd_pstate_verify,
>   	.target		= amd_pstate_target,
> -	.adjust_perf    = amd_pstate_adjust_perf,
>   	.init		= amd_pstate_cpu_init,
>   	.exit		= amd_pstate_cpu_exit,
>   	.name		= "amd-pstate",
> @@ -421,10 +465,14 @@ static int __init amd_pstate_init(void)
>   		return -EEXIST;
>   
>   	/* capability check */
> -	if (!boot_cpu_has(X86_FEATURE_AMD_CPPC)) {
> -		pr_debug("%s, AMD CPPC MSR based functionality is not supported\n",
> +	if (boot_cpu_has(X86_FEATURE_AMD_CPPC)) {
> +		pr_debug("%s, AMD CPPC MSR based functionality is supported\n",
>   			 __func__);
> -		return -ENODEV;
> +		amd_pstate_driver.adjust_perf = amd_pstate_adjust_perf;
> +	} else {
> +		static_call_update(amd_pstate_enable, cppc_enable);
> +		static_call_update(amd_pstate_init_perf, cppc_init_perf);
> +		static_call_update(amd_pstate_update_perf, cppc_update_perf);
>   	}
>   
>   	/* enable amd pstate feature */
>