From: Huang Rui <ray.huang@amd.com>
To: "Karny, Wyes" <Wyes.Karny@amd.com>
Cc: "rafael@kernel.org" <rafael@kernel.org>,
"viresh.kumar@linaro.org" <viresh.kumar@linaro.org>,
"Limonciello, Mario" <Mario.Limonciello@amd.com>,
"Jinzhou.Su@amd.com" <Jinzhou.Su@amd.com>,
"Yuan, Perry" <Perry.Yuan@amd.com>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] cpufreq/amd-pstate: Write CPPC enable bit for each core
Date: Mon, 22 May 2023 16:16:35 +0800 [thread overview]
Message-ID: <ZGsk4zzBBVEQTZQ5@amd.com> (raw)
In-Reply-To: <20230522063325.80193-2-wyes.karny@amd.com>
On Mon, May 22, 2023 at 02:33:24PM +0800, Karny, Wyes wrote:
> ACPI specification [1] says: "CPPC Enable Register: If supported by the
> platform, OSPM writes a one to this register to enable CPPC on this
> processor."
>
> Make amd_pstate align with the specification.
>
> To do so, move amd_pstate_enable function to per-policy/per-core
> callbacks.
>
> Also remove per-cpu loop from cppc_enable, because it's called from
> per-policy/per-core callbacks and it was causing duplicate MSR writes.
> This will improve driver-load, suspend-resume and offline-online on
> shared memory system.
>
> [1]: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/08_Processor_Configuration_and_Control/declaring-processors.html#cppc-enable-register
>
> Fixes: e059c184da47 ("cpufreq: amd-pstate: Introduce the support for the processors with shared memory solution")
> Signed-off-by: Wyes Karny <wyes.karny@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 53 ++++++++++++++++++------------------
> 1 file changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 5a3d4aa0f45a..8c72f95ac315 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -226,29 +226,27 @@ static int amd_pstate_set_energy_pref_index(struct amd_cpudata *cpudata,
> return ret;
> }
>
> -static inline int pstate_enable(bool enable)
> +static inline int pstate_enable(int cpu, bool enable)
> {
> - return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable);
> + return wrmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_ENABLE, enable);
In the full MSR processors, the CPPCEnableRegister is per package, not per
thread. In share memory processors, it should be per thread.
Thanks,
Ray
> }
>
> -static int cppc_enable(bool enable)
> +static int cppc_enable(int cpu, bool enable)
> {
> - int cpu, ret = 0;
> + int ret = 0;
> struct cppc_perf_ctrls perf_ctrls;
>
> - for_each_present_cpu(cpu) {
> - ret = cppc_set_enable(cpu, enable);
> + ret = cppc_set_enable(cpu, enable);
> + 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(cpu, &perf_ctrls);
> - if (ret)
> - return ret;
> - }
> }
>
> return ret;
> @@ -256,9 +254,9 @@ static int cppc_enable(bool enable)
>
> DEFINE_STATIC_CALL(amd_pstate_enable, pstate_enable);
>
> -static inline int amd_pstate_enable(bool enable)
> +static inline int amd_pstate_enable(int cpu, bool enable)
> {
> - return static_call(amd_pstate_enable)(enable);
> + return static_call(amd_pstate_enable)(cpu, enable);
> }
>
> static int pstate_init_perf(struct amd_cpudata *cpudata)
> @@ -643,6 +641,8 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>
> cpudata->cpu = policy->cpu;
>
> + ret = amd_pstate_enable(policy->cpu, true);
> +
> ret = amd_pstate_init_perf(cpudata);
> if (ret)
> goto free_cpudata1;
> @@ -724,7 +724,7 @@ static int amd_pstate_cpu_resume(struct cpufreq_policy *policy)
> {
> int ret;
>
> - ret = amd_pstate_enable(true);
> + ret = amd_pstate_enable(policy->cpu, true);
> if (ret)
> pr_err("failed to enable amd-pstate during resume, return %d\n", ret);
>
> @@ -735,7 +735,7 @@ static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy)
> {
> int ret;
>
> - ret = amd_pstate_enable(false);
> + ret = amd_pstate_enable(policy->cpu, false);
> if (ret)
> pr_err("failed to disable amd-pstate during suspend, return %d\n", ret);
>
> @@ -841,7 +841,6 @@ static ssize_t show_energy_performance_preference(
>
> static void amd_pstate_driver_cleanup(void)
> {
> - amd_pstate_enable(false);
> cppc_state = AMD_PSTATE_DISABLE;
> current_pstate_driver = NULL;
> }
> @@ -1039,6 +1038,8 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
> cpudata->cpu = policy->cpu;
> cpudata->epp_policy = 0;
>
> + ret = amd_pstate_enable(policy->cpu, true);
> +
> ret = amd_pstate_init_perf(cpudata);
> if (ret)
> goto free_cpudata1;
> @@ -1180,13 +1181,13 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
> return 0;
> }
>
> -static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
> +static void amd_pstate_epp_reenable(int cpu, struct amd_cpudata *cpudata)
> {
> struct cppc_perf_ctrls perf_ctrls;
> u64 value, max_perf;
> int ret;
>
> - ret = amd_pstate_enable(true);
> + ret = amd_pstate_enable(cpu, true);
> if (ret)
> pr_err("failed to enable amd pstate during resume, return %d\n", ret);
>
> @@ -1209,7 +1210,7 @@ static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
> pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
>
> if (cppc_state == AMD_PSTATE_ACTIVE) {
> - amd_pstate_epp_reenable(cpudata);
> + amd_pstate_epp_reenable(policy->cpu, cpudata);
> cpudata->suspended = false;
> }
>
> @@ -1280,7 +1281,7 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
> cpudata->suspended = true;
>
> /* disable CPPC in lowlevel firmware */
> - ret = amd_pstate_enable(false);
> + ret = amd_pstate_enable(policy->cpu, false);
> if (ret)
> pr_err("failed to suspend, return %d\n", ret);
>
> @@ -1295,7 +1296,7 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
> mutex_lock(&amd_pstate_limits_lock);
>
> /* enable amd pstate from suspend state*/
> - amd_pstate_epp_reenable(cpudata);
> + amd_pstate_epp_reenable(policy->cpu, cpudata);
>
> mutex_unlock(&amd_pstate_limits_lock);
>
> @@ -1370,8 +1371,6 @@ static int __init amd_pstate_init(void)
> static_call_update(amd_pstate_update_perf, cppc_update_perf);
> }
>
> - /* enable amd pstate feature */
> - ret = amd_pstate_enable(true);
> if (ret) {
> pr_err("failed to enable with return %d\n", ret);
> return ret;
> --
> 2.34.1
>
next prev parent reply other threads:[~2023-05-22 8:18 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-22 6:33 [PATCH 0/2] amd-pstate: amd_pstate related fixes Wyes Karny
2023-05-22 6:33 ` [PATCH 1/2] cpufreq/amd-pstate: Write CPPC enable bit for each core Wyes Karny
2023-05-22 8:16 ` Huang Rui [this message]
2023-05-26 3:28 ` kernel test robot
2023-05-22 6:33 ` [PATCH 2/2] cpufreq/amd-pstate: Remove unnecessary active state checks Wyes Karny
2023-05-22 7:27 ` Yuan, Perry
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZGsk4zzBBVEQTZQ5@amd.com \
--to=ray.huang@amd.com \
--cc=Jinzhou.Su@amd.com \
--cc=Mario.Limonciello@amd.com \
--cc=Perry.Yuan@amd.com \
--cc=Wyes.Karny@amd.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.