[PATCH] intel_idle: make SPR C1 and C1E be independent

[PATCH] intel_idle: make SPR C1 and C1E be independent

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

This patch partially reverts the changes made by the following commit:

da0e58c038e6 intel_idle: add 'preferred_cstates' module argument

As that commit describes, on early Sapphire Rapids Xeon platforms the C1 and
C1E states were mutually exclusive, so that users could only have either C1 and
C6, or C1E and C6.

However, Intel firmware engineers managed to remove this limitation and make C1
and C1E to be completely independent, just like on previous Xeon platforms.

Therefore, this patch:
 * Removes commentary describing the old, and now non-existing SPR C1E
   limitation.
 * Marks SPR C1E as available by default.
 * Removes the 'preferred_cstates' parameter handling for SPR. Both C1 and
   C1E will be available regardless of 'preferred_cstates' value.

We expect that all SPR systems are shipping with new firmware, which includes
the C1/C1E improvement.

Cc: stable@vger.kernel.org # v5.18+
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/idle/intel_idle.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 424ef470223d..ba2b485a03ed 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -879,16 +879,6 @@ static struct cpuidle_state adl_l_cstates[] __initdata = {
 		.enter = NULL }
 };
 
-/*
- * On Sapphire Rapids Xeon C1 has to be disabled if C1E is enabled, and vice
- * versa. On SPR C1E is enabled only if "C1E promotion" bit is set in
- * MSR_IA32_POWER_CTL. But in this case there effectively no C1, because C1
- * requests are promoted to C1E. If the "C1E promotion" bit is cleared, then
- * both C1 and C1E requests end up with C1, so there is effectively no C1E.
- *
- * By default we enable C1 and disable C1E by marking it with
- * 'CPUIDLE_FLAG_UNUSABLE'.
- */
 static struct cpuidle_state spr_cstates[] __initdata = {
 	{
 		.name = "C1",
@@ -901,8 +891,7 @@ static struct cpuidle_state spr_cstates[] __initdata = {
 	{
 		.name = "C1E",
 		.desc = "MWAIT 0x01",
-		.flags = MWAIT2flg(0x01) | CPUIDLE_FLAG_ALWAYS_ENABLE |
-					   CPUIDLE_FLAG_UNUSABLE,
+		.flags = MWAIT2flg(0x01) | CPUIDLE_FLAG_ALWAYS_ENABLE,
 		.exit_latency = 2,
 		.target_residency = 4,
 		.enter = &intel_idle,
@@ -1724,17 +1713,6 @@ static void __init spr_idle_state_table_update(void)
 {
 	unsigned long long msr;
 
-	/* Check if user prefers C1E over C1. */
-	if ((preferred_states_mask & BIT(2)) &&
-	    !(preferred_states_mask & BIT(1))) {
-		/* Disable C1 and enable C1E. */
-		spr_cstates[0].flags |= CPUIDLE_FLAG_UNUSABLE;
-		spr_cstates[1].flags &= ~CPUIDLE_FLAG_UNUSABLE;
-
-		/* Enable C1E using the "C1E promotion" bit. */
-		c1e_promotion = C1E_PROMOTION_ENABLE;
-	}
-
 	/*
 	 * By default, the C6 state assumes the worst-case scenario of package
 	 * C6. However, if PC6 is disabled, we update the numbers to match
-- 
2.35.1

Re: [PATCH] intel_idle: make SPR C1 and C1E be independent

[Rafael J. Wysocki]

On Sat, Jul 16, 2022 at 8:27 AM Artem Bityutskiy <dedekind1@gmail.com> wrote:
>
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>
> This patch partially reverts the changes made by the following commit:
>
> da0e58c038e6 intel_idle: add 'preferred_cstates' module argument
>
> As that commit describes, on early Sapphire Rapids Xeon platforms the C1 and
> C1E states were mutually exclusive, so that users could only have either C1 and
> C6, or C1E and C6.
>
> However, Intel firmware engineers managed to remove this limitation and make C1
> and C1E to be completely independent, just like on previous Xeon platforms.
>
> Therefore, this patch:
>  * Removes commentary describing the old, and now non-existing SPR C1E
>    limitation.
>  * Marks SPR C1E as available by default.
>  * Removes the 'preferred_cstates' parameter handling for SPR. Both C1 and
>    C1E will be available regardless of 'preferred_cstates' value.
>
> We expect that all SPR systems are shipping with new firmware, which includes
> the C1/C1E improvement.
>
> Cc: stable@vger.kernel.org # v5.18+
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  drivers/idle/intel_idle.c | 24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
>
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 424ef470223d..ba2b485a03ed 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -879,16 +879,6 @@ static struct cpuidle_state adl_l_cstates[] __initdata = {
>                 .enter = NULL }
>  };
>
> -/*
> - * On Sapphire Rapids Xeon C1 has to be disabled if C1E is enabled, and vice
> - * versa. On SPR C1E is enabled only if "C1E promotion" bit is set in
> - * MSR_IA32_POWER_CTL. But in this case there effectively no C1, because C1
> - * requests are promoted to C1E. If the "C1E promotion" bit is cleared, then
> - * both C1 and C1E requests end up with C1, so there is effectively no C1E.
> - *
> - * By default we enable C1 and disable C1E by marking it with
> - * 'CPUIDLE_FLAG_UNUSABLE'.
> - */
>  static struct cpuidle_state spr_cstates[] __initdata = {
>         {
>                 .name = "C1",
> @@ -901,8 +891,7 @@ static struct cpuidle_state spr_cstates[] __initdata = {
>         {
>                 .name = "C1E",
>                 .desc = "MWAIT 0x01",
> -               .flags = MWAIT2flg(0x01) | CPUIDLE_FLAG_ALWAYS_ENABLE |
> -                                          CPUIDLE_FLAG_UNUSABLE,
> +               .flags = MWAIT2flg(0x01) | CPUIDLE_FLAG_ALWAYS_ENABLE,
>                 .exit_latency = 2,
>                 .target_residency = 4,
>                 .enter = &intel_idle,
> @@ -1724,17 +1713,6 @@ static void __init spr_idle_state_table_update(void)
>  {
>         unsigned long long msr;
>
> -       /* Check if user prefers C1E over C1. */
> -       if ((preferred_states_mask & BIT(2)) &&
> -           !(preferred_states_mask & BIT(1))) {
> -               /* Disable C1 and enable C1E. */
> -               spr_cstates[0].flags |= CPUIDLE_FLAG_UNUSABLE;
> -               spr_cstates[1].flags &= ~CPUIDLE_FLAG_UNUSABLE;
> -
> -               /* Enable C1E using the "C1E promotion" bit. */
> -               c1e_promotion = C1E_PROMOTION_ENABLE;
> -       }
> -
>         /*
>          * By default, the C6 state assumes the worst-case scenario of package
>          * C6. However, if PC6 is disabled, we update the numbers to match
> --

Applied as 5.20 material, thanks!