| Message ID | 20250219210302.442954-18-superm1@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | amd-pstate cleanups | expand |
On 2/20/2025 2:33 AM, Mario Limonciello wrote: > From: Mario Limonciello <mario.limonciello@amd.com> > > The CPPC enable register is configured as "write once". That is > any future writes don't actually do anything. > > Because of this, all the cleanup paths that currently exist for > CPPC disable are non-effective. > > Rework CPPC enable to only enable after all the CAP registers have > been read to avoid enabling CPPC on CPUs with invalid _CPC or > unpopulated MSRs. > > As the register is write once, remove all cleanup paths as well. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v4: > * Remove unnecessary amd_pstate_update_perf() call during online > * Remove unnecessary if (ret) ret. > * Drop amd_pstate_cpu_resume() > * Drop unnecessary derefs > v3: > * Fixup for suspend/resume issue > --- > drivers/cpufreq/amd-pstate.c | 187 +++++++++++------------------------ > 1 file changed, 55 insertions(+), 132 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index fa9c581c68a39..f152636cecbeb 100644 [Snip] > -static inline int msr_cppc_enable(bool enable) > +static int shmem_cppc_enable(struct cpufreq_policy *policy) > { > - int ret, cpu; > - unsigned long logical_proc_id_mask = 0; > - > - /* > - * MSR_AMD_CPPC_ENABLE is write-once, once set it cannot be cleared. > - */ > - if (!enable) > - return 0; > - > - if (enable == cppc_enabled) > - return 0; > - > - for_each_present_cpu(cpu) { > - unsigned long logical_id = topology_logical_package_id(cpu); > - > - if (test_bit(logical_id, &logical_proc_id_mask)) > - continue; > - > - set_bit(logical_id, &logical_proc_id_mask); > - > - ret = wrmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_ENABLE, > - enable); > - if (ret) > - return ret; > - } > - > - cppc_enabled = enable; > - return 0; > -} > - > -static int shmem_cppc_enable(bool enable) > -{ > - int cpu, ret = 0; > struct cppc_perf_ctrls perf_ctrls; > + int ret; > > - if (enable == cppc_enabled) > - return 0; > - > - for_each_present_cpu(cpu) { > - ret = cppc_set_enable(cpu, enable); > - if (ret) > - return ret; > + ret = cppc_set_enable(policy->cpu, 1); > + if (ret) > + return ret; > > - /* 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(cpu, &perf_ctrls); > - if (ret) > - return ret; > - } > + /* 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() > } > > - 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) > @@ -1115,28 +1060,6 @@ static void amd_pstate_cpu_exit(struct cpufreq_policy *policy) > kfree(cpudata); > } > > -static int amd_pstate_cpu_resume(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 ret; > -} > - > -static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy) > -{ > - int ret; > - > - ret = amd_pstate_cppc_enable(false); > - if (ret) > - pr_err("failed to disable amd-pstate during suspend, return %d\n", ret); > - > - return ret; > -} > - > /* Sysfs attributes */ > > /* > @@ -1228,8 +1151,10 @@ static ssize_t show_energy_performance_available_preferences( > static ssize_t store_energy_performance_preference( > struct cpufreq_policy *policy, const char *buf, size_t count) > { > + struct amd_cpudata *cpudata = policy->driver_data; > char str_preference[21]; > ssize_t ret; > + u8 epp; > > ret = sscanf(buf, "%20s", str_preference); > if (ret != 1) > @@ -1239,7 +1164,29 @@ static ssize_t store_energy_performance_preference( > if (ret < 0) > return -EINVAL; > > - ret = amd_pstate_set_energy_pref_index(policy, ret); > + if (!ret) > + epp = cpudata->epp_default; > + else > + epp = epp_values[ret]; > + > + if (epp > 0 && policy->policy == CPUFREQ_POLICY_PERFORMANCE) { > + pr_debug("EPP cannot be set under performance policy\n"); > + return -EBUSY; > + } > + > + if (trace_amd_pstate_epp_perf_enabled()) { > + union perf_cached perf = cpudata->perf; > + > + trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf, > + epp, > + FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cpudata->cppc_req_cached), > + FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cpudata->cppc_req_cached), > + policy->boost_enabled, > + FIELD_GET(AMD_CPPC_EPP_PERF_MASK, > + cpudata->cppc_req_cached) != epp); We are doing the tracing in amd_pstate_set_epp() as well right?, Isnt this one redundant? > + } > + > + ret = amd_pstate_set_epp(policy, epp); > > return ret ? ret : count; > } > @@ -1272,7 +1219,6 @@ static ssize_t show_energy_performance_preference( > > static void amd_pstate_driver_cleanup(void) > { > - amd_pstate_cppc_enable(false); > cppc_state = AMD_PSTATE_DISABLE; > current_pstate_driver = NULL; > } > @@ -1306,14 +1252,6 @@ static int amd_pstate_register_driver(int mode) > > cppc_state = mode; > > - ret = amd_pstate_cppc_enable(true); > - if (ret) { > - pr_err("failed to enable cppc during amd-pstate driver registration, return %d\n", > - ret); > - amd_pstate_driver_cleanup(); > - return ret; > - } > - > /* at least one CPU supports CPB */ > current_pstate_driver->boost_enabled = cpu_feature_enabled(X86_FEATURE_CPB); > > @@ -1553,11 +1491,15 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) > policy->cpuinfo.max_freq = policy->max = perf_to_freq(perf, > cpudata->nominal_freq, > perf.highest_perf); > + policy->driver_data = cpudata; > + > + ret = amd_pstate_cppc_enable(policy); I think we missed cppc_enable in "amd_pstate_cpu_init". Confirmed this by booting with "amd_pstate=passive" Also, one weird behavior I saw while testing this part, if we boot with "amd_pstate=passive" initially, the MSR_AMD_CPPC_ENABLE register is 0. But after I run the amd-pstate-ut (which fails the check_amd_pstate_enabled() test the first time) the MSR_AMD_CPPC_ENABLE gets set to 1. But I didnt see any code in amd-pstate-ut that sets it. We can ignore this quirk for now, just mentioned to see if you have any idea about this. > + if (ret) > + goto free_cpudata1; > > /* It will be updated by governor */ > policy->cur = policy->cpuinfo.min_freq; > > - policy->driver_data = cpudata; > > policy->boost_enabled = READ_ONCE(cpudata->boost_supported); > > @@ -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? > > return 0; > + > } > > static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy) > @@ -1691,11 +1623,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy) > static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) > { > struct amd_cpudata *cpudata = policy->driver_data; > - int ret; > - > - /* avoid suspending when EPP is not enabled */ > - if (cppc_state != AMD_PSTATE_ACTIVE) > - return 0; > > /* invalidate to ensure it's rewritten during resume */ > cpudata->cppc_req_cached = 0; > @@ -1703,11 +1630,6 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) > /* set this flag to avoid setting core offline*/ > cpudata->suspended = true; > > - /* disable CPPC in lowlevel firmware */ > - ret = amd_pstate_cppc_enable(false); > - if (ret) > - pr_err("failed to suspend, return %d\n", ret); > - > return 0; > } > > @@ -1716,8 +1638,12 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy) > struct amd_cpudata *cpudata = policy->driver_data; > > if (cpudata->suspended) { > + int ret; > + > /* enable amd pstate from suspend state*/ > - amd_pstate_epp_reenable(policy); > + ret = amd_pstate_epp_update_limit(policy); > + if (ret) > + return ret; > > cpudata->suspended = false; > } > @@ -1732,8 +1658,6 @@ static struct cpufreq_driver amd_pstate_driver = { > .fast_switch = amd_pstate_fast_switch, > .init = amd_pstate_cpu_init, > .exit = amd_pstate_cpu_exit, > - .suspend = amd_pstate_cpu_suspend, > - .resume = amd_pstate_cpu_resume, > .set_boost = amd_pstate_set_boost, > .update_limits = amd_pstate_update_limits, > .name = "amd-pstate", > @@ -1748,8 +1672,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = { > .exit = amd_pstate_epp_cpu_exit, > .offline = amd_pstate_epp_cpu_offline, > .online = amd_pstate_epp_cpu_online, > - .suspend = amd_pstate_epp_suspend, > - .resume = amd_pstate_epp_resume, > + .suspend = amd_pstate_epp_suspend, > + .resume = amd_pstate_epp_resume, Spurious whitespace change? > .update_limits = amd_pstate_update_limits, > .set_boost = amd_pstate_set_boost, > .name = "amd-pstate-epp", > @@ -1900,7 +1824,6 @@ static int __init amd_pstate_init(void) > > global_attr_free: > cpufreq_unregister_driver(current_pstate_driver); > - amd_pstate_cppc_enable(false); > return ret; > } > device_initcall(amd_pstate_init);
>> + /* 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) >> @@ -1115,28 +1060,6 @@ static void amd_pstate_cpu_exit(struct cpufreq_policy *policy) >> kfree(cpudata); >> } >> >> -static int amd_pstate_cpu_resume(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 ret; >> -} >> - >> -static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy) >> -{ >> - int ret; >> - >> - ret = amd_pstate_cppc_enable(false); >> - if (ret) >> - pr_err("failed to disable amd-pstate during suspend, return %d\n", ret); >> - >> - return ret; >> -} >> - >> /* Sysfs attributes */ >> >> /* >> @@ -1228,8 +1151,10 @@ static ssize_t show_energy_performance_available_preferences( >> static ssize_t store_energy_performance_preference( >> struct cpufreq_policy *policy, const char *buf, size_t count) >> { >> + struct amd_cpudata *cpudata = policy->driver_data; >> char str_preference[21]; >> ssize_t ret; >> + u8 epp; >> >> ret = sscanf(buf, "%20s", str_preference); >> if (ret != 1) >> @@ -1239,7 +1164,29 @@ static ssize_t store_energy_performance_preference( >> if (ret < 0) >> return -EINVAL; >> >> - ret = amd_pstate_set_energy_pref_index(policy, ret); >> + if (!ret) >> + epp = cpudata->epp_default; >> + else >> + epp = epp_values[ret]; >> + >> + if (epp > 0 && policy->policy == CPUFREQ_POLICY_PERFORMANCE) { >> + pr_debug("EPP cannot be set under performance policy\n"); >> + return -EBUSY; >> + } >> + >> + if (trace_amd_pstate_epp_perf_enabled()) { >> + union perf_cached perf = cpudata->perf; >> + >> + trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf, >> + epp, >> + FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cpudata->cppc_req_cached), >> + FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cpudata->cppc_req_cached), >> + policy->boost_enabled, >> + FIELD_GET(AMD_CPPC_EPP_PERF_MASK, >> + cpudata->cppc_req_cached) != epp); > > We are doing the tracing in amd_pstate_set_epp() as well right?, Isnt this one redundant? Yup! Great catch. > >> + } >> + >> + ret = amd_pstate_set_epp(policy, epp); >> >> return ret ? ret : count; >> } >> @@ -1272,7 +1219,6 @@ static ssize_t show_energy_performance_preference( >> >> static void amd_pstate_driver_cleanup(void) >> { >> - amd_pstate_cppc_enable(false); >> cppc_state = AMD_PSTATE_DISABLE; >> current_pstate_driver = NULL; >> } >> @@ -1306,14 +1252,6 @@ static int amd_pstate_register_driver(int mode) >> >> cppc_state = mode; >> >> - ret = amd_pstate_cppc_enable(true); >> - if (ret) { >> - pr_err("failed to enable cppc during amd-pstate driver registration, return %d\n", >> - ret); >> - amd_pstate_driver_cleanup(); >> - return ret; >> - } >> - >> /* at least one CPU supports CPB */ >> current_pstate_driver->boost_enabled = cpu_feature_enabled(X86_FEATURE_CPB); >> >> @@ -1553,11 +1491,15 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) >> policy->cpuinfo.max_freq = policy->max = perf_to_freq(perf, >> cpudata->nominal_freq, >> perf.highest_perf); >> + policy->driver_data = cpudata; >> + >> + ret = amd_pstate_cppc_enable(policy); > > I think we missed cppc_enable in "amd_pstate_cpu_init". Confirmed this by booting with "amd_pstate=passive" Yeah; true. I will add this. > > Also, one weird behavior I saw while testing this part, if we boot with "amd_pstate=passive" > initially, the MSR_AMD_CPPC_ENABLE register is 0. But after I run the amd-pstate-ut (which fails > the check_amd_pstate_enabled() test the first time) the MSR_AMD_CPPC_ENABLE gets set to 1. But I > didnt see any code in amd-pstate-ut that sets it. We can ignore this quirk for now, just > mentioned to see if you have any idea about this. amd-pstate-ut will change modes, so I expect this is the reason it happens. It's also the reason I didn't catch it. I always started in active when I was testing switching to passive. > >> + if (ret) >> + goto free_cpudata1; >> >> /* It will be updated by governor */ >> policy->cur = policy->cpuinfo.min_freq; >> >> - policy->driver_data = cpudata; >> >> policy->boost_enabled = READ_ONCE(cpudata->boost_supported); >> >> @@ -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. > >> >> return 0; >> + >> } >> >> static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy) >> @@ -1691,11 +1623,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy) >> static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) >> { >> struct amd_cpudata *cpudata = policy->driver_data; >> - int ret; >> - >> - /* avoid suspending when EPP is not enabled */ >> - if (cppc_state != AMD_PSTATE_ACTIVE) >> - return 0; >> >> /* invalidate to ensure it's rewritten during resume */ >> cpudata->cppc_req_cached = 0; >> @@ -1703,11 +1630,6 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) >> /* set this flag to avoid setting core offline*/ >> cpudata->suspended = true; >> >> - /* disable CPPC in lowlevel firmware */ >> - ret = amd_pstate_cppc_enable(false); >> - if (ret) >> - pr_err("failed to suspend, return %d\n", ret); >> - >> return 0; >> } >> >> @@ -1716,8 +1638,12 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy) >> struct amd_cpudata *cpudata = policy->driver_data; >> >> if (cpudata->suspended) { >> + int ret; >> + >> /* enable amd pstate from suspend state*/ >> - amd_pstate_epp_reenable(policy); >> + ret = amd_pstate_epp_update_limit(policy); >> + if (ret) >> + return ret; >> >> cpudata->suspended = false; >> } >> @@ -1732,8 +1658,6 @@ static struct cpufreq_driver amd_pstate_driver = { >> .fast_switch = amd_pstate_fast_switch, >> .init = amd_pstate_cpu_init, >> .exit = amd_pstate_cpu_exit, >> - .suspend = amd_pstate_cpu_suspend, >> - .resume = amd_pstate_cpu_resume, >> .set_boost = amd_pstate_set_boost, >> .update_limits = amd_pstate_update_limits, >> .name = "amd-pstate", >> @@ -1748,8 +1672,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = { >> .exit = amd_pstate_epp_cpu_exit, >> .offline = amd_pstate_epp_cpu_offline, >> .online = amd_pstate_epp_cpu_online, >> - .suspend = amd_pstate_epp_suspend, >> - .resume = amd_pstate_epp_resume, >> + .suspend = amd_pstate_epp_suspend, >> + .resume = amd_pstate_epp_resume, > > Spurious whitespace change? > >> .update_limits = amd_pstate_update_limits, >> .set_boost = amd_pstate_set_boost, >> .name = "amd-pstate-epp", >> @@ -1900,7 +1824,6 @@ static int __init amd_pstate_init(void) >> >> global_attr_free: >> cpufreq_unregister_driver(current_pstate_driver); >> - amd_pstate_cppc_enable(false); >> return ret; >> } >> device_initcall(amd_pstate_init); >
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.
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index fa9c581c68a39..f152636cecbeb 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -85,7 +85,6 @@ static struct cpufreq_driver *current_pstate_driver; static struct cpufreq_driver amd_pstate_driver; static struct cpufreq_driver amd_pstate_epp_driver; static int cppc_state = AMD_PSTATE_UNDEFINED; -static bool cppc_enabled; static bool amd_pstate_prefcore = true; static struct quirk_entry *quirks; @@ -371,89 +370,35 @@ static int shmem_set_epp(struct cpufreq_policy *policy, u8 epp) return ret; } -static int amd_pstate_set_energy_pref_index(struct cpufreq_policy *policy, - int pref_index) +static inline int msr_cppc_enable(struct cpufreq_policy *policy) { - struct amd_cpudata *cpudata = policy->driver_data; - u8 epp; - - if (!pref_index) - epp = cpudata->epp_default; - else - epp = epp_values[pref_index]; - - if (epp > 0 && cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) { - pr_debug("EPP cannot be set under performance policy\n"); - return -EBUSY; - } - - return amd_pstate_set_epp(policy, epp); + return wrmsrl_safe_on_cpu(policy->cpu, MSR_AMD_CPPC_ENABLE, 1); } -static inline int msr_cppc_enable(bool enable) +static int shmem_cppc_enable(struct cpufreq_policy *policy) { - int ret, cpu; - unsigned long logical_proc_id_mask = 0; - - /* - * MSR_AMD_CPPC_ENABLE is write-once, once set it cannot be cleared. - */ - if (!enable) - return 0; - - if (enable == cppc_enabled) - return 0; - - for_each_present_cpu(cpu) { - unsigned long logical_id = topology_logical_package_id(cpu); - - if (test_bit(logical_id, &logical_proc_id_mask)) - continue; - - set_bit(logical_id, &logical_proc_id_mask); - - ret = wrmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_ENABLE, - enable); - if (ret) - return ret; - } - - cppc_enabled = enable; - return 0; -} - -static int shmem_cppc_enable(bool enable) -{ - int cpu, ret = 0; struct cppc_perf_ctrls perf_ctrls; + int ret; - if (enable == cppc_enabled) - return 0; - - for_each_present_cpu(cpu) { - ret = cppc_set_enable(cpu, enable); - if (ret) - return ret; + ret = cppc_set_enable(policy->cpu, 1); + if (ret) + return ret; - /* 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(cpu, &perf_ctrls); - if (ret) - return ret; - } + /* 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); } - 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) @@ -1115,28 +1060,6 @@ static void amd_pstate_cpu_exit(struct cpufreq_policy *policy) kfree(cpudata); } -static int amd_pstate_cpu_resume(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 ret; -} - -static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy) -{ - int ret; - - ret = amd_pstate_cppc_enable(false); - if (ret) - pr_err("failed to disable amd-pstate during suspend, return %d\n", ret); - - return ret; -} - /* Sysfs attributes */ /* @@ -1228,8 +1151,10 @@ static ssize_t show_energy_performance_available_preferences( static ssize_t store_energy_performance_preference( struct cpufreq_policy *policy, const char *buf, size_t count) { + struct amd_cpudata *cpudata = policy->driver_data; char str_preference[21]; ssize_t ret; + u8 epp; ret = sscanf(buf, "%20s", str_preference); if (ret != 1) @@ -1239,7 +1164,29 @@ static ssize_t store_energy_performance_preference( if (ret < 0) return -EINVAL; - ret = amd_pstate_set_energy_pref_index(policy, ret); + if (!ret) + epp = cpudata->epp_default; + else + epp = epp_values[ret]; + + if (epp > 0 && policy->policy == CPUFREQ_POLICY_PERFORMANCE) { + pr_debug("EPP cannot be set under performance policy\n"); + return -EBUSY; + } + + if (trace_amd_pstate_epp_perf_enabled()) { + union perf_cached perf = cpudata->perf; + + trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf, + epp, + FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cpudata->cppc_req_cached), + FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cpudata->cppc_req_cached), + policy->boost_enabled, + FIELD_GET(AMD_CPPC_EPP_PERF_MASK, + cpudata->cppc_req_cached) != epp); + } + + ret = amd_pstate_set_epp(policy, epp); return ret ? ret : count; } @@ -1272,7 +1219,6 @@ static ssize_t show_energy_performance_preference( static void amd_pstate_driver_cleanup(void) { - amd_pstate_cppc_enable(false); cppc_state = AMD_PSTATE_DISABLE; current_pstate_driver = NULL; } @@ -1306,14 +1252,6 @@ static int amd_pstate_register_driver(int mode) cppc_state = mode; - ret = amd_pstate_cppc_enable(true); - if (ret) { - pr_err("failed to enable cppc during amd-pstate driver registration, return %d\n", - ret); - amd_pstate_driver_cleanup(); - return ret; - } - /* at least one CPU supports CPB */ current_pstate_driver->boost_enabled = cpu_feature_enabled(X86_FEATURE_CPB); @@ -1553,11 +1491,15 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) policy->cpuinfo.max_freq = policy->max = perf_to_freq(perf, cpudata->nominal_freq, perf.highest_perf); + policy->driver_data = cpudata; + + ret = amd_pstate_cppc_enable(policy); + if (ret) + goto free_cpudata1; /* It will be updated by governor */ policy->cur = policy->cpuinfo.min_freq; - policy->driver_data = cpudata; policy->boost_enabled = READ_ONCE(cpudata->boost_supported); @@ -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; return 0; + } static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy) @@ -1691,11 +1623,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy) static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) { struct amd_cpudata *cpudata = policy->driver_data; - int ret; - - /* avoid suspending when EPP is not enabled */ - if (cppc_state != AMD_PSTATE_ACTIVE) - return 0; /* invalidate to ensure it's rewritten during resume */ cpudata->cppc_req_cached = 0; @@ -1703,11 +1630,6 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) /* set this flag to avoid setting core offline*/ cpudata->suspended = true; - /* disable CPPC in lowlevel firmware */ - ret = amd_pstate_cppc_enable(false); - if (ret) - pr_err("failed to suspend, return %d\n", ret); - return 0; } @@ -1716,8 +1638,12 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy) struct amd_cpudata *cpudata = policy->driver_data; if (cpudata->suspended) { + int ret; + /* enable amd pstate from suspend state*/ - amd_pstate_epp_reenable(policy); + ret = amd_pstate_epp_update_limit(policy); + if (ret) + return ret; cpudata->suspended = false; } @@ -1732,8 +1658,6 @@ static struct cpufreq_driver amd_pstate_driver = { .fast_switch = amd_pstate_fast_switch, .init = amd_pstate_cpu_init, .exit = amd_pstate_cpu_exit, - .suspend = amd_pstate_cpu_suspend, - .resume = amd_pstate_cpu_resume, .set_boost = amd_pstate_set_boost, .update_limits = amd_pstate_update_limits, .name = "amd-pstate", @@ -1748,8 +1672,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = { .exit = amd_pstate_epp_cpu_exit, .offline = amd_pstate_epp_cpu_offline, .online = amd_pstate_epp_cpu_online, - .suspend = amd_pstate_epp_suspend, - .resume = amd_pstate_epp_resume, + .suspend = amd_pstate_epp_suspend, + .resume = amd_pstate_epp_resume, .update_limits = amd_pstate_update_limits, .set_boost = amd_pstate_set_boost, .name = "amd-pstate-epp", @@ -1900,7 +1824,6 @@ static int __init amd_pstate_init(void) global_attr_free: cpufreq_unregister_driver(current_pstate_driver); - amd_pstate_cppc_enable(false); return ret; } device_initcall(amd_pstate_init);