[PATCH v14 0/5] AMD Pstate Driver Core Performance Boost

[PATCH v14 0/5] AMD Pstate Driver Core Performance Boost

[Mario Limonciello]

This patch implements core-performance boost (CPB) for the amd-pstate
driver.

It's a follow on to v13 submitted by Perry:
https://lore.kernel.org/linux-pm/cover.1718988436.git.perry.yuan@amd.com/

v13->v14:
 * Drop cpb_boost global parameter and sysfs file.  It's unneeded right
   now; but may be introduced again in the future later.
 * Squash a few patches together due to the dropped parameter
 * Add a fix for incorrect boost value in sysfs during bootup caused by
   commit f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs
   using cpufreq_boost_set_sw()")
 * Correct some typographical errors
 * Add Co-developed-by tag for patches I worked on or modified

Mario Limonciello (1):
  cpufreq: Only disable boost during cpu online when using frequency
    tables

Perry Yuan (4):
  cpufreq: acpi: move MSR_K7_HWCR_CPB_DIS_BIT into msr-index.h
  cpufreq: amd-pstate: initialize core precision boost state
  cpufreq: amd-pstate: Cap the CPPC.max_perf to nominal_perf if CPB is
    off
  Documentation: cpufreq: amd-pstate: update doc for Per CPU boost
    control method

 Documentation/admin-guide/pm/amd-pstate.rst |  16 +++
 arch/x86/include/asm/msr-index.h            |   2 +
 drivers/cpufreq/acpi-cpufreq.c              |   2 -
 drivers/cpufreq/amd-pstate.c                | 120 ++++++++++++++++----
 drivers/cpufreq/amd-pstate.h                |   1 +
 drivers/cpufreq/cpufreq.c                   |   3 +-
 6 files changed, 120 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH v14 1/5] cpufreq: acpi: move MSR_K7_HWCR_CPB_DIS_BIT into msr-index.h

[Mario Limonciello]

From: Perry Yuan <perry.yuan@amd.com>

There are some other drivers also need to use the
MSR_K7_HWCR_CPB_DIS_BIT for CPB control bit, so it makes sense to move
the definition to a common header file to allow other driver to use it.

No intentional functional impact.

Suggested-by: Gautham Ranjal Shenoy <gautham.shenoy@amd.com>
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Acked-by: Huang Rui <ray.huang@amd.com>
Link: https://lore.kernel.org/r/78b6c75e6cffddce3e950dd543af6ae9f8eeccc3.1718988436.git.perry.yuan@amd.com
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/include/asm/msr-index.h | 2 ++
 drivers/cpufreq/acpi-cpufreq.c   | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index e022e6eb766c..384739d592af 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -781,6 +781,8 @@
 #define MSR_K7_HWCR_IRPERF_EN		BIT_ULL(MSR_K7_HWCR_IRPERF_EN_BIT)
 #define MSR_K7_FID_VID_CTL		0xc0010041
 #define MSR_K7_FID_VID_STATUS		0xc0010042
+#define MSR_K7_HWCR_CPB_DIS_BIT		25
+#define MSR_K7_HWCR_CPB_DIS		BIT_ULL(MSR_K7_HWCR_CPB_DIS_BIT)
 
 /* K6 MSRs */
 #define MSR_K6_WHCR			0xc0000082
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 37f1cdf46d29..2fc82831bddd 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -50,8 +50,6 @@ enum {
 #define AMD_MSR_RANGE		(0x7)
 #define HYGON_MSR_RANGE		(0x7)
 
-#define MSR_K7_HWCR_CPB_DIS	(1ULL << 25)
-
 struct acpi_cpufreq_data {
 	unsigned int resume;
 	unsigned int cpu_feature;
-- 
2.43.0

[PATCH v14 2/5] cpufreq: amd-pstate: initialize core precision boost state

[Mario Limonciello]

From: Perry Yuan <Perry.Yuan@amd.com>

The "Core Performance Boost (CPB) feature, when enabled in the BIOS,
allows the OS to control the highest performance for each individual
core. The active, passive and the guided modes of the amd-pstate driver
do support controlling the core frequency boost when this BIOS feature
is enabled. Additionally, the amd-pstate driver provides a sysfs
interface allowing the user to activate/deactivate this core performance
boost feature at runtime.

Add support for the set_boost callback in the active mode driver to
enable boost control via the cpufreq core. This ensures a consistent
boost control interface across all pstate modes, including passive
mode, guided mode, and active mode.

With this addition, all three pstate modes can support the same boost
control interface with unique interface and global CPB control. Each
CPU also supports individual boost control, allowing global CPB to
change all cores' boost states simultaneously. Specific CPUs can
update their boost states separately, ensuring all cores' boost
states are synchronized.

Cc: Oleksandr Natalenko <oleksandr@natalenko.name>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217931
Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 115 ++++++++++++++++++++++++++++-------
 drivers/cpufreq/amd-pstate.h |   1 +
 2 files changed, 95 insertions(+), 21 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index afcf398574f6..10cce7023208 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -679,43 +679,105 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 	cpufreq_cpu_put(policy);
 }
 
-static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state)
+static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
+	struct cppc_perf_ctrls perf_ctrls;
+	u32 highest_perf, nominal_perf, nominal_freq, max_freq;
 	int ret;
 
-	if (!cpudata->boost_supported) {
-		pr_err("Boost mode is not supported by this processor or SBIOS\n");
-		return -EINVAL;
+	highest_perf = READ_ONCE(cpudata->highest_perf);
+	nominal_perf = READ_ONCE(cpudata->nominal_perf);
+	nominal_freq = READ_ONCE(cpudata->nominal_freq);
+	max_freq = READ_ONCE(cpudata->max_freq);
+
+	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+		u64 value = READ_ONCE(cpudata->cppc_req_cached);
+
+		value &= ~GENMASK_ULL(7, 0);
+		value |= on ? highest_perf : nominal_perf;
+		WRITE_ONCE(cpudata->cppc_req_cached, value);
+
+		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+	} else {
+		perf_ctrls.max_perf = on ? highest_perf : nominal_perf;
+		ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);
+		if (ret) {
+			cpufreq_cpu_release(policy);
+			pr_debug("Failed to set max perf on CPU:%d. ret:%d\n",
+				cpudata->cpu, ret);
+			return ret;
+		}
 	}
 
-	if (state)
-		policy->cpuinfo.max_freq = cpudata->max_freq;
+	if (on)
+		policy->cpuinfo.max_freq = max_freq;
 	else
-		policy->cpuinfo.max_freq = cpudata->nominal_freq * 1000;
+		policy->cpuinfo.max_freq = nominal_freq * 1000;
 
 	policy->max = policy->cpuinfo.max_freq;
 
-	ret = freq_qos_update_request(&cpudata->req[1],
-				      policy->cpuinfo.max_freq);
-	if (ret < 0)
-		return ret;
+	if (cppc_state == AMD_PSTATE_PASSIVE) {
+		ret = freq_qos_update_request(&cpudata->req[1], policy->cpuinfo.max_freq);
+		if (ret < 0)
+			pr_debug("Failed to update freq constraint: CPU%d\n", cpudata->cpu);
+	}
 
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
-static void amd_pstate_boost_init(struct amd_cpudata *cpudata)
+static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state)
 {
-	u32 highest_perf, nominal_perf;
+	struct amd_cpudata *cpudata = policy->driver_data;
+	int ret;
 
-	highest_perf = READ_ONCE(cpudata->highest_perf);
-	nominal_perf = READ_ONCE(cpudata->nominal_perf);
+	if (!cpudata->boost_supported) {
+		pr_err("Boost mode is not supported by this processor or SBIOS\n");
+		return -EOPNOTSUPP;
+	}
+	mutex_lock(&amd_pstate_driver_lock);
+	ret = amd_pstate_cpu_boost_update(policy, state);
+	WRITE_ONCE(cpudata->boost_state, !ret ? state : false);
+	policy->boost_enabled = !ret ? state : false;
+	refresh_frequency_limits(policy);
+	mutex_unlock(&amd_pstate_driver_lock);
 
-	if (highest_perf <= nominal_perf)
-		return;
+	return ret;
+}
+
+static int amd_pstate_init_boost_support(struct amd_cpudata *cpudata)
+{
+	u64 boost_val;
+	int ret = -1;
 
-	cpudata->boost_supported = true;
+	/*
+	 * If platform has no CPB support or disable it, initialize current driver
+	 * boost_enabled state to be false, it is not an error for cpufreq core to handle.
+	 */
+	if (!cpu_feature_enabled(X86_FEATURE_CPB)) {
+		pr_debug_once("Boost CPB capabilities not present in the processor\n");
+		ret = 0;
+		goto exit_err;
+	}
+
+	/* at least one CPU supports CPB, even if others fail later on to set up */
 	current_pstate_driver->boost_enabled = true;
+
+	ret = rdmsrl_on_cpu(cpudata->cpu, MSR_K7_HWCR, &boost_val);
+	if (ret) {
+		pr_err_once("failed to read initial CPU boost state!\n");
+		ret = -EIO;
+		goto exit_err;
+	}
+
+	if (!(boost_val & MSR_K7_HWCR_CPB_DIS))
+		cpudata->boost_supported = true;
+
+	return 0;
+
+exit_err:
+	cpudata->boost_supported = false;
+	return ret;
 }
 
 static void amd_perf_ctl_reset(unsigned int cpu)
@@ -968,6 +1030,10 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 	if (ret)
 		goto free_cpudata1;
 
+	ret = amd_pstate_init_boost_support(cpudata);
+	if (ret)
+		goto free_cpudata1;
+
 	min_freq = READ_ONCE(cpudata->min_freq);
 	max_freq = READ_ONCE(cpudata->max_freq);
 
@@ -980,6 +1046,8 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 	policy->cpuinfo.min_freq = min_freq;
 	policy->cpuinfo.max_freq = max_freq;
 
+	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
+
 	/* It will be updated by governor */
 	policy->cur = policy->cpuinfo.min_freq;
 
@@ -1005,7 +1073,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 
 	policy->driver_data = cpudata;
 
-	amd_pstate_boost_init(cpudata);
 	if (!current_pstate_driver->adjust_perf)
 		current_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
 
@@ -1420,6 +1487,10 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	if (ret)
 		goto free_cpudata1;
 
+	ret = amd_pstate_init_boost_support(cpudata);
+	if (ret)
+		goto free_cpudata1;
+
 	min_freq = READ_ONCE(cpudata->min_freq);
 	max_freq = READ_ONCE(cpudata->max_freq);
 
@@ -1435,6 +1506,8 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	policy->min = policy->cpuinfo.min_freq;
 	policy->max = policy->cpuinfo.max_freq;
 
+	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
+
 	/*
 	 * Set the policy to provide a valid fallback value in case
 	 * the default cpufreq governor is neither powersave nor performance.
@@ -1456,7 +1529,6 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 			return ret;
 		WRITE_ONCE(cpudata->cppc_cap1_cached, value);
 	}
-	amd_pstate_boost_init(cpudata);
 
 	return 0;
 
@@ -1718,6 +1790,7 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
 	.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",
 	.attr		= amd_pstate_epp_attr,
 };
diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
index f80b33fa5d43..cc8bb2bc325a 100644
--- a/drivers/cpufreq/amd-pstate.h
+++ b/drivers/cpufreq/amd-pstate.h
@@ -100,6 +100,7 @@ struct amd_cpudata {
 	u64	cppc_cap1_cached;
 	bool	suspended;
 	s16	epp_default;
+	bool	boost_state;
 };
 
 #endif /* _LINUX_AMD_PSTATE_H */
-- 
2.43.0

[PATCH v14 3/5] cpufreq: amd-pstate: Cap the CPPC.max_perf to nominal_perf if CPB is off

[Mario Limonciello]

From: Perry Yuan <Perry.Yuan@amd.com>

When Core Performance Boost is disabled by the user, the
CPPC_REQ.max_perf should not exceed the nominal_perf since by definition
the frequencies between nominal_perf and the highest_perf are in the
boost range. Fix this in amd_pstate_update()

Acked-by: Huang Rui <ray.huang@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
Link: https://lore.kernel.org/r/66f55232be01092c423f0523f68b82b80c293943.1718988436.git.perry.yuan@amd.com
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 10cce7023208..6992f6169919 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -514,6 +514,7 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
 	unsigned long max_freq;
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
 	u64 prev = READ_ONCE(cpudata->cppc_req_cached);
+	u32 nominal_perf = READ_ONCE(cpudata->nominal_perf);
 	u64 value = prev;
 
 	min_perf = clamp_t(unsigned long, min_perf, cpudata->min_limit_perf,
@@ -536,6 +537,10 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
 	value &= ~AMD_CPPC_DES_PERF(~0L);
 	value |= AMD_CPPC_DES_PERF(des_perf);
 
+	/* limit the max perf when core performance boost feature is disabled */
+	if (!cpudata->boost_supported)
+		max_perf = min_t(unsigned long, nominal_perf, max_perf);
+
 	value &= ~AMD_CPPC_MAX_PERF(~0L);
 	value |= AMD_CPPC_MAX_PERF(max_perf);
 
-- 
2.43.0

[PATCH v14 4/5] Documentation: cpufreq: amd-pstate: update doc for Per CPU boost control method

[Mario Limonciello]

From: Perry Yuan <perry.yuan@amd.com>

Updates the documentation in `amd-pstate.rst` to include information about
the per CPU boost control feature. Users can now enable or disable the
Core Performance Boost (CPB) feature on individual CPUs using the `boost`
sysfs attribute.

Signed-off-by: Perry Yuan <perry.yuan@amd.com>
Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 Documentation/admin-guide/pm/amd-pstate.rst | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
index f5ee81419a93..d0324d44f548 100644
--- a/Documentation/admin-guide/pm/amd-pstate.rst
+++ b/Documentation/admin-guide/pm/amd-pstate.rst
@@ -281,6 +281,22 @@ integer values defined between 0 to 255 when EPP feature is enabled by platform
 firmware, if EPP feature is disabled, driver will ignore the written value
 This attribute is read-write.
 
+``boost``
+The `boost` sysfs attribute provides control over the CPU core
+performance boost, allowing users to manage the maximum frequency limitation
+of the CPU. This attribute can be used to enable or disable the boost feature
+on individual CPUs.
+
+When the boost feature is enabled, the CPU can dynamically increase its frequency
+beyond the base frequency, providing enhanced performance for demanding workloads.
+On the other hand, disabling the boost feature restricts the CPU to operate at the
+base frequency, which may be desirable in certain scenarios to prioritize power
+efficiency or manage temperature.
+
+To manipulate the `boost` attribute, users can write a value of `0` to disable the
+boost or `1` to enable it, for the respective CPU using the sysfs path
+`/sys/devices/system/cpu/cpuX/cpufreq/boost`, where `X` represents the CPU number.
+
 Other performance and frequency values can be read back from
 ``/sys/devices/system/cpu/cpuX/acpi_cppc/``, see :ref:`cppc_sysfs`.
 
-- 
2.43.0

Re: [PATCH v14 4/5] Documentation: cpufreq: amd-pstate: update doc for Per CPU boost control method

[Gautham R.Shenoy]

Mario Limonciello <mario.limonciello@amd.com> writes:

> From: Perry Yuan <perry.yuan@amd.com>
>
> Updates the documentation in `amd-pstate.rst` to include information about
> the per CPU boost control feature. Users can now enable or disable the
> Core Performance Boost (CPB) feature on individual CPUs using the `boost`
> sysfs attribute.
>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>


LGTM

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

> ---
>  Documentation/admin-guide/pm/amd-pstate.rst | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index f5ee81419a93..d0324d44f548 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -281,6 +281,22 @@ integer values defined between 0 to 255 when EPP feature is enabled by platform
>  firmware, if EPP feature is disabled, driver will ignore the written value
>  This attribute is read-write.
>  
> +``boost``
> +The `boost` sysfs attribute provides control over the CPU core
> +performance boost, allowing users to manage the maximum frequency limitation
> +of the CPU. This attribute can be used to enable or disable the boost feature
> +on individual CPUs.
> +
> +When the boost feature is enabled, the CPU can dynamically increase its frequency
> +beyond the base frequency, providing enhanced performance for demanding workloads.
> +On the other hand, disabling the boost feature restricts the CPU to operate at the
> +base frequency, which may be desirable in certain scenarios to prioritize power
> +efficiency or manage temperature.
> +
> +To manipulate the `boost` attribute, users can write a value of `0` to disable the
> +boost or `1` to enable it, for the respective CPU using the sysfs path
> +`/sys/devices/system/cpu/cpuX/cpufreq/boost`, where `X` represents the CPU number.
> +
>  Other performance and frequency values can be read back from
>  ``/sys/devices/system/cpu/cpuX/acpi_cppc/``, see :ref:`cppc_sysfs`.
>  
> -- 
> 2.43.0

[PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Mario Limonciello]

The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy
boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost
policy incorrectly when boost has been enabled by the platform firmware
initially even if a driver sets the policy up.

This is because there are no discrete entries in the frequency table.
Update that code to only run when a frequency table is present.

Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
Cc: Sibi Sankar <quic_sibis@quicinc.com>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Dhruva Gole <d-gole@ti.com>
Cc: Yipeng Zou <zouyipeng@huawei.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
 drivers/cpufreq/cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1fdabb660231..32c119614710 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu)
 		}
 
 		/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
-		policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
+		if (policy->freq_table)
+			policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
 
 		/*
 		 * The initialization has succeeded and the policy is online.
-- 
2.43.0

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Viresh Kumar]

On 24-06-24, 16:34, Mario Limonciello wrote:
> The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy
> boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost
> policy incorrectly when boost has been enabled by the platform firmware
> initially even if a driver sets the policy up.
> 
> This is because there are no discrete entries in the frequency table.
> Update that code to only run when a frequency table is present.
> 
> Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> Cc: Sibi Sankar <quic_sibis@quicinc.com>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Dhruva Gole <d-gole@ti.com>
> Cc: Yipeng Zou <zouyipeng@huawei.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>  drivers/cpufreq/cpufreq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1fdabb660231..32c119614710 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu)
>  		}
>  
>  		/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
> -		policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
> +		if (policy->freq_table)
> +			policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);

I am not sure if I understand the problem properly here. Can you
please explain a bit in detail ?

-- 
viresh

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Mario Limonciello]

On 6/25/2024 01:30, Viresh Kumar wrote:
> On 24-06-24, 16:34, Mario Limonciello wrote:
>> The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy
>> boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost
>> policy incorrectly when boost has been enabled by the platform firmware
>> initially even if a driver sets the policy up.
>>
>> This is because there are no discrete entries in the frequency table.
>> Update that code to only run when a frequency table is present.
>>
>> Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> Cc: Sibi Sankar <quic_sibis@quicinc.com>
>> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: Dhruva Gole <d-gole@ti.com>
>> Cc: Yipeng Zou <zouyipeng@huawei.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>   drivers/cpufreq/cpufreq.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 1fdabb660231..32c119614710 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu)
>>   		}
>>   
>>   		/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
>> -		policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
>> +		if (policy->freq_table)
>> +			policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
> 
> I am not sure if I understand the problem properly here. Can you
> please explain a bit in detail ?
> 

The core issue is that there are drivers that have boost functionality 
but don't have a frequency table.  As pointed out by Gautham there are 
also drivers that have a frequency table but don't advertise boost 
pstates (CPUFREQ_BOOST_FREQ isn't set on any frequency).

So what happens is the driver may have choosen a policy to have boost 
enabled but when cpufreq_online() is called it gets "marked" disabled 
from this check introduced in f37a4d6b4a2c even though it's previously 
enabled.

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Viresh Kumar]

On 25-06-24, 07:31, Mario Limonciello wrote:
> The core issue is that there are drivers that have boost functionality but
> don't have a frequency table.  As pointed out by Gautham there are also
> drivers that have a frequency table but don't advertise boost pstates
> (CPUFREQ_BOOST_FREQ isn't set on any frequency).
> 
> So what happens is the driver may have choosen a policy to have boost
> enabled but when cpufreq_online() is called it gets "marked" disabled from
> this check introduced in f37a4d6b4a2c even though it's previously enabled.

And who was setting policy->boost_enabled to true before that ? Also
how will your patch fix the problem ? I don't see any other code
setting it too, unless request comes from sysfs, which would work even
now I think.

-- 
viresh

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Mario Limonciello]

On 6/25/2024 22:11, Viresh Kumar wrote:
> On 25-06-24, 07:31, Mario Limonciello wrote:
>> The core issue is that there are drivers that have boost functionality but
>> don't have a frequency table.  As pointed out by Gautham there are also
>> drivers that have a frequency table but don't advertise boost pstates
>> (CPUFREQ_BOOST_FREQ isn't set on any frequency).
>>
>> So what happens is the driver may have choosen a policy to have boost
>> enabled but when cpufreq_online() is called it gets "marked" disabled from
>> this check introduced in f37a4d6b4a2c even though it's previously enabled.
> 
> And who was setting policy->boost_enabled to true before that ? Also
> how will your patch fix the problem ? I don't see any other code
> setting it too, unless request comes from sysfs, which would work even
> now I think.
> 

The earlier patches in this series do that with amd-pstate.  Gautham had 
suggested a change to acpi-cpufreq for the same too.

However I tested Gautham's suggestion (which is in this thread) and I 
think it's the better way to do it than what I did in this v14 patch.

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Viresh Kumar]

On 25-06-24, 22:14, Mario Limonciello wrote:
> The earlier patches in this series do that with amd-pstate.  Gautham had
> suggested a change to acpi-cpufreq for the same too.

Ahh, since I wasn't cc'd, I missed that obvious part :)

The right fix would be this then I guess:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a45aac17c20f..9e5060b27864 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1431,7 +1431,8 @@ static int cpufreq_online(unsigned int cpu)
                }

                /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
-               policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
+               if (cpufreq_boost_enabled() && policy_has_boost_freq(policy))
+                       policy->boost_enabled = true;

                /*
                 * The initialization has succeeded and the policy is online.

-- 
viresh

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Mario Limonciello]

On 6/25/2024 22:17, Viresh Kumar wrote:
> On 25-06-24, 22:14, Mario Limonciello wrote:
>> The earlier patches in this series do that with amd-pstate.  Gautham had
>> suggested a change to acpi-cpufreq for the same too.
> 
> Ahh, since I wasn't cc'd, I missed that obvious part :)
> 
> The right fix would be this then I guess:
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index a45aac17c20f..9e5060b27864 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1431,7 +1431,8 @@ static int cpufreq_online(unsigned int cpu)
>                  }
> 
>                  /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
> -               policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
> +               if (cpufreq_boost_enabled() && policy_has_boost_freq(policy))
> +                       policy->boost_enabled = true;
> 
>                  /*
>                   * The initialization has succeeded and the policy is online.
> 

Yeah; that's effectively the same result as Gautham's suggestion.  He 
had just said to change policy_has_boost_freq() for the same.

I'll test it with yours and reconcile the better one to submit back out 
for v15.

Thank you for your feedback.

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Viresh Kumar]

On 25-06-24, 22:20, Mario Limonciello wrote:
> Yeah; that's effectively the same result as Gautham's suggestion.  He had
> just said to change policy_has_boost_freq() for the same.

I know. The problem is policy_has_boost_freq() (as its name suggests)
needs to check if the policy supports boost freqs (in a generic way)
and it is used at several other places and it would be wrong to hack
that routine to fix this problem.

All we want here is for the core to not touch boost_enabled if the
policy->init() function has already done so.

> I'll test it with yours and reconcile the better one to submit back out for
> v15.

You can send it separately to be honest, and it looks like a fix, so
Rafael should be able to get it merged a bit sooner. Add a fixes tag
too.

-- 
viresh

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Viresh Kumar]

On 26-06-24, 08:55, Viresh Kumar wrote:
> On 25-06-24, 22:20, Mario Limonciello wrote:
> > Yeah; that's effectively the same result as Gautham's suggestion.  He had
> > just said to change policy_has_boost_freq() for the same.
> 
> I know. The problem is policy_has_boost_freq() (as its name suggests)
> needs to check if the policy supports boost freqs (in a generic way)
> and it is used at several other places and it would be wrong to hack
> that routine to fix this problem.
> 
> All we want here is for the core to not touch boost_enabled if the
> policy->init() function has already done so.
> 
> > I'll test it with yours and reconcile the better one to submit back out for
> > v15.
> 
> You can send it separately to be honest, and it looks like a fix, so
> Rafael should be able to get it merged a bit sooner. Add a fixes tag
> too.

And maybe send patch for acpi-cpufreq and any other platform that is
broken too..

-- 
viresh

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Mario Limonciello]

On 6/25/2024 22:27, Viresh Kumar wrote:
> On 26-06-24, 08:55, Viresh Kumar wrote:
>> On 25-06-24, 22:20, Mario Limonciello wrote:
>>> Yeah; that's effectively the same result as Gautham's suggestion.  He had
>>> just said to change policy_has_boost_freq() for the same.
>>
>> I know. The problem is policy_has_boost_freq() (as its name suggests)
>> needs to check if the policy supports boost freqs (in a generic way)
>> and it is used at several other places and it would be wrong to hack
>> that routine to fix this problem.
>>
>> All we want here is for the core to not touch boost_enabled if the
>> policy->init() function has already done so.
>>
>>> I'll test it with yours and reconcile the better one to submit back out for
>>> v15.
>>
>> You can send it separately to be honest, and it looks like a fix, so
>> Rafael should be able to get it merged a bit sooner. Add a fixes tag
>> too.
> 
> And maybe send patch for acpi-cpufreq and any other platform that is
> broken too..
> 

I can take it all through the amd-pstate tree.
I'll put it at the front of the series.
I think as long as we can get it merged before ~rc6 it should be fine 
for the 6.11 merge window.

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Viresh Kumar]

On 25-06-24, 22:33, Mario Limonciello wrote:
> I can take it all through the amd-pstate tree.

Unless there is a dependency, we try to take the patches through the
PM tree itself as there can be conflicting patches there otherwise.

> I'll put it at the front of the series.
> I think as long as we can get it merged before ~rc6 it should be fine for
> the 6.11 merge window.

Since this is a fix, it may get into 6.10 itself.

-- 
viresh

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Mario Limonciello]

On 6/25/2024 22:44, Viresh Kumar wrote:
> On 25-06-24, 22:33, Mario Limonciello wrote:
>> I can take it all through the amd-pstate tree.
> 
> Unless there is a dependency, we try to take the patches through the
> PM tree itself as there can be conflicting patches there otherwise.

Right; but amd-pstate merges to the PM tree.

> 
>> I'll put it at the front of the series.
>> I think as long as we can get it merged before ~rc6 it should be fine for
>> the 6.11 merge window.
> 
> Since this is a fix, it may get into 6.10 itself.
> 

Good point.  I'll do some testing with your suggestion and send those 
two as 6.10 material and then the rest of this series at v15 for 6.11 
remaining material if we get it done in time.

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Gautham R.Shenoy]

Hello Mario,

Mario Limonciello <mario.limonciello@amd.com> writes:

> The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy
> boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost
> policy incorrectly when boost has been enabled by the platform firmware
> initially even if a driver sets the policy up.
>
> This is because there are no discrete entries in the frequency table.
> Update that code to only run when a frequency table is present.

Thanks for this fix.


There are also drivers such as acpi-cpufreq which have a frequency
table, but the boost Pstates are not advertised. Thus none of the table
entries have CPUFREQ_BOOST_FREQ set.


>
> Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> Cc: Sibi Sankar <quic_sibis@quicinc.com>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Dhruva Gole <d-gole@ti.com>
> Cc: Yipeng Zou <zouyipeng@huawei.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>  drivers/cpufreq/cpufreq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1fdabb660231..32c119614710 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu)
>  		}
>  
>  		/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
> -		policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
> +		if (policy->freq_table)
> +			policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
>  
>  		/*
>  		 * The initialization has succeeded and the policy is
>  		online.


How about something like the following:

---

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 37f1cdf46d29..be5f4d3e9c1d 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -140,6 +140,7 @@ static int set_boost(struct cpufreq_policy *policy, int val)
        pr_debug("CPU %*pbl: Core Boosting %s.\n",
                 cpumask_pr_args(policy->cpus), str_enabled_disabled(val));
 
+       policy->boost_enabled = val;
        return 0;
 }
 
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index 10e80d912b8d..f604389b9cd6 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -18,6 +18,10 @@ bool policy_has_boost_freq(struct cpufreq_policy *policy)
 {
        struct cpufreq_frequency_table *pos, *table = policy->freq_table;
 
+       /* The driver has explicitly advertised the boost-capabilities */
+       if (policy->boost_enabled)
+               return true;
+
        if (!table)
                return false;
 



> -- 
> 2.43.0

Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Mario Limonciello]

On 6/25/2024 06:55, Gautham R.Shenoy wrote:
> 
> Hello Mario,
> 
> Mario Limonciello <mario.limonciello@amd.com> writes:
> 
>> The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy
>> boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost
>> policy incorrectly when boost has been enabled by the platform firmware
>> initially even if a driver sets the policy up.
>>
>> This is because there are no discrete entries in the frequency table.
>> Update that code to only run when a frequency table is present.
> 
> Thanks for this fix.
> 
> 
> There are also drivers such as acpi-cpufreq which have a frequency
> table, but the boost Pstates are not advertised. Thus none of the table
> entries have CPUFREQ_BOOST_FREQ set.
> 
> 
>>
>> Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> Cc: Sibi Sankar <quic_sibis@quicinc.com>
>> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: Dhruva Gole <d-gole@ti.com>
>> Cc: Yipeng Zou <zouyipeng@huawei.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>   drivers/cpufreq/cpufreq.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 1fdabb660231..32c119614710 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu)
>>   		}
>>   
>>   		/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
>> -		policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
>> +		if (policy->freq_table)
>> +			policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
>>   
>>   		/*
>>   		 * The initialization has succeeded and the policy is
>>   		online.
> 
> 
> How about something like the following:
> 
> ---
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 37f1cdf46d29..be5f4d3e9c1d 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -140,6 +140,7 @@ static int set_boost(struct cpufreq_policy *policy, int val)
>          pr_debug("CPU %*pbl: Core Boosting %s.\n",
>                   cpumask_pr_args(policy->cpus), str_enabled_disabled(val));
>   
> +       policy->boost_enabled = val;
>          return 0;
>   }
>   
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index 10e80d912b8d..f604389b9cd6 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -18,6 +18,10 @@ bool policy_has_boost_freq(struct cpufreq_policy *policy)
>   {
>          struct cpufreq_frequency_table *pos, *table = policy->freq_table;
>   
> +       /* The driver has explicitly advertised the boost-capabilities */
> +       if (policy->boost_enabled)
> +               return true;
> +
>          if (!table)
>                  return false;
>   
> 
> 
> 
>> -- 
>> 2.43.0

Yeah I think this works too.  Let's see see what Viresh thinks.