Re: [PATCH v4 17/19] cpufreq/amd-pstate: Rework CPPC enabling

[Dhananjay Ugwekar]

On 2/25/2025 5:29 AM, Mario Limonciello wrote:
>>> +    /* Enable autonomous mode for EPP */
>>> +    if (cppc_state == AMD_PSTATE_ACTIVE) {
>>> +        /* Set desired perf as zero to allow EPP firmware control */
>>> +        perf_ctrls.desired_perf = 0;
>>> +        ret = cppc_set_perf(policy->cpu, &perf_ctrls);
>>
>> I'm thinking do we need this "setting of desired_perf" as a part of shmem_cppc_enable,
>> one thing is we're not doing it in the "msr_" counterpart
>> also, I guess this would be taken care as part of amd_pstate_epp_set_policy()->amd_pstate_epp_update_limit()->amd_pstate_update_perf()
> 
> Great point, agreed will drop it.
> 
>>
>>>       }
>>>   -    cppc_enabled = enable;
>>>       return ret;
>>>   }
>>>     DEFINE_STATIC_CALL(amd_pstate_cppc_enable, msr_cppc_enable);
>>>   -static inline int amd_pstate_cppc_enable(bool enable)
>>> +static inline int amd_pstate_cppc_enable(struct cpufreq_policy *policy)
>>>   {
>>> -    return static_call(amd_pstate_cppc_enable)(enable);
>>> +    return static_call(amd_pstate_cppc_enable)(policy);
>>>   }
>>>     static int msr_init_perf(struct amd_cpudata *cpudata)
[Snip]
>>>   @@ -1649,31 +1591,21 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
>>>       return 0;
>>>   }
>>>   -static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
>>> -{
>>> -    int ret;
>>> -
>>> -    ret = amd_pstate_cppc_enable(true);
>>> -    if (ret)
>>> -        pr_err("failed to enable amd pstate during resume, return %d\n", ret);
>>> -
>>> -
>>> -    return amd_pstate_epp_update_limit(policy);
>>> -}
>>> -
>>>   static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
>>>   {
>>>       struct amd_cpudata *cpudata = policy->driver_data;
>>>       int ret;
>>>   -    pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
>>> +    pr_debug("AMD CPU Core %d going online\n", policy->cpu);
>>>   -    ret = amd_pstate_epp_reenable(policy);
>>> +    ret = amd_pstate_cppc_enable(policy);
>>>       if (ret)
>>>           return ret;
>>> +
>>>       cpudata->suspended = false;
>>
>> Do we need this here?, shouldn't only resume() have this statement?
> 
> The reason I had in mind for it was this sequence:
> * Suspend
> * CPU goes offline
> * CPU goes online
> * Resume
> 
> But I don't think that's realistic even with parallel boot.  I will drop this.

Also I have one doubt, why do we need to keep track if the system is suspended ?

Won't the idle subsystem have safeguards to prevent CPU offline while the system 
is being suspended ? Haven't gone through that code, just checking if you have an 
idea about it.