Messages in this thread | | | Date | Tue, 12 Jul 2022 12:08:43 -0500 | Subject | Re: [PATCH v2 02/14] cpufreq: amd-pstate: enable AMD Precision Boost mode switch | From | Nathan Fontenot <> |
| |
On 7/11/22 23:15, Yuan, Perry wrote:>>> --- a/drivers/cpufreq/amd-pstate.c >>> +++ b/drivers/cpufreq/amd-pstate.c >>> @@ -122,6 +122,7 @@ struct amd_cpudata { >>> >>> u64 freq; >>> bool boost_supported; >>> + u64 cppc_hw_conf_cached; >>> }; >>> >>> static inline int pstate_enable(bool enable) @@ -438,18 +439,27 @@ >>> static int amd_pstate_set_boost(struct cpufreq_policy *policy, int >>> state) { >>> struct amd_cpudata *cpudata = policy->driver_data; >>> int ret; >>> + u64 value; >>> >>> if (!cpudata->boost_supported) { >>> pr_err("Boost mode is not supported by this processor or >> SBIOS\n"); >>> return -EINVAL; >>> } >>> >>> - if (state) >>> + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, >> &value); >>> + if (ret) >>> + return ret; >>> + >>> + if (state) { >>> + value |= AMD_CPPC_PRECISION_BOOST_ENABLED; >>> policy->cpuinfo.max_freq = cpudata->max_freq; >>> - else >>> + } else { >>> + value &= ~AMD_CPPC_PRECISION_BOOST_ENABLED; >>> policy->cpuinfo.max_freq = cpudata->nominal_freq; >>> - >>> + } >>> policy->max = policy->cpuinfo.max_freq; >>> + WRITE_ONCE(cpudata->cppc_hw_conf_cached, value); >> >> Does the entire MSR value need to be cached? We only care about the >> boost enabled bit so it may be better to just cache that. >> >> -Nathan > > I think the whole MSR value should be cached, because it has no bad impact to the hardware or driver . > And it is simple to do that, when we need to check other bits in the hardware configuration MSR in future, we can still use this cached value as well. > Dose it make sense ? > > Perry. >
The MSR controls much more than boost enabling, my concern is someone else starting to update bits in the MSR and now we have a stale MSR value. Just caching the boost enabled bit is really aimed at possible future bug prevention.
-Nathan
| |