Skip to contentSkip to conversation

cpuidle: riscv-sbi: Fix CPU_PM_CPU_IDLE_ENTER_xyz() macro usage

PATCH In review Anup Patel 4 participants 5 messages Jul 18, 2022 – Sep 23, 2022

Subsystems (3)
RISC-V CPU idle time management framework Cpuidle driver - RISC-V sbi
lore
Share & tools
lore

Anup Patel

author
#0 lore raw

PATCH 1/1 cpuidle: riscv-sbi: Fix CPU_PM_CPU_IDLE_ENTER_xyz() macro usage

Currently, we are using CPU_PM_CPU_IDLE_ENTER_PARAM() for all SBI HSM suspend types so retentive suspend types are also treated non-retentive and kernel will do redundant additional work for these states.

The BIT[31] of SBI HSM suspend types allows us to differentiate between retentive and non-retentive suspend types so we should use this BIT to call appropriate CPU_PM_CPU_IDLE_ENTER_xyz() macro.

Fixes 6abf32f1d9c5 ("cpuidle: Add RISC-V SBI CPU idle driver")
Signed-off-by Anup Patel <[email protected]>
---
 drivers/cpuidle/cpuidle-riscv-sbi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/cpuidle/cpuidle-riscv-sbi.c +6 -1
index 1151e5e2ba82..33c92fec4365 100644
@@ -97,8 +97,13 @@ static int sbi_cpuidle_enter_state(struct cpuidle_device *dev,
 				   struct cpuidle_driver *drv, int idx)
 {
 	u32 *states = __this_cpu_read(sbi_cpuidle_data.states);
+	u32 state = states[idx];
 
-	return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend, idx, states[idx]);
+	if (state & SBI_HSM_SUSP_NON_RET_BIT)
+		return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend, idx, state);
+	else
+		return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(sbi_suspend,
+							     idx, state);
 }
 
 static int __sbi_enter_domain_idle_state(struct cpuidle_device *dev,
Signature · 1 line
2.34.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 36 lines · Anup Patel

Currently, we are using CPU_PM_CPU_IDLE_ENTER_PARAM() for all SBI HSM suspend types so retentive suspend types are also treated non-retentive and kernel will do redundant additional work for these states.

The BIT[31] of SBI HSM suspend types allows us to differentiate between retentive and non-retentive suspend types so we should use this BIT to call appropriate CPU_PM_CPU_IDLE_ENTER_xyz() macro.

Fixes 6abf32f1d9c5 ("cpuidle: Add RISC-V SBI CPU idle driver")
Signed-off-by Anup Patel <[email protected]>
---
 drivers/cpuidle/cpuidle-riscv-sbi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/cpuidle/cpuidle-riscv-sbi.c +6 -1
index 1151e5e2ba82..33c92fec4365 100644
@@ -97,8 +97,13 @@ static int sbi_cpuidle_enter_state(struct cpuidle_device *dev,
 				   struct cpuidle_driver *drv, int idx)
 {
 	u32 *states = __this_cpu_read(sbi_cpuidle_data.states);
+	u32 state = states[idx];
 
-	return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend, idx, states[idx]);
+	if (state & SBI_HSM_SUSP_NON_RET_BIT)
+		return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend, idx, state);
+	else
+		return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(sbi_suspend,
+							     idx, state);
 }
 
 static int __sbi_enter_domain_idle_state(struct cpuidle_device *dev,
Signature · 1 line
2.34.1
Reviewed-by Andrew Jones <[email protected]>
maintainer
#2 lore raw
In reply to Anup Patel

Hi Palmer,

Quoted 11 lines · Anup Patel

Currently, we are using CPU_PM_CPU_IDLE_ENTER_PARAM() for all SBI HSM suspend types so retentive suspend types are also treated non-retentive and kernel will do redundant additional work for these states.

The BIT[31] of SBI HSM suspend types allows us to differentiate between retentive and non-retentive suspend types so we should use this BIT to call appropriate CPU_PM_CPU_IDLE_ENTER_xyz() macro.

Fixes 6abf32f1d9c5 ("cpuidle: Add RISC-V SBI CPU idle driver")
Signed-off-by Anup Patel <[email protected]>

Can you please take this patch through the RISC-V tree ?

Regards,
Anup

Quoted 26 lines
---
 drivers/cpuidle/cpuidle-riscv-sbi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/cpuidle/cpuidle-riscv-sbi.c +6 -2
index 1151e5e2ba82..33c92fec4365 100644
@@ -97,8 +97,13 @@ static int sbi_cpuidle_enter_state(struct cpuidle_device *dev,
                                   struct cpuidle_driver *drv, int idx)
 {
        u32 *states = __this_cpu_read(sbi_cpuidle_data.states);
+       u32 state = states[idx];
 
-       return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend, idx, states[idx]);
+       if (state & SBI_HSM_SUSP_NON_RET_BIT)
+               return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend, idx, state);
+       else
+               return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(sbi_suspend,
+                                                            idx, state);
 }
 
 static int __sbi_enter_domain_idle_state(struct cpuidle_device *dev,
--

2.34.1

maintainer
#3 lore raw
Quoted 8 lines · On Sun, 28 Aug 2022 19:59:45 PDT (-0700), [email protected] wrote:

Hi Palmer,

Quoted 5 lines · Anup Patel

Currently, we are using CPU_PM_CPU_IDLE_ENTER_PARAM() for all SBI HSM suspend types so retentive suspend types are also treated non-retentive and kernel will do redundant additional work for these states.

Quoted 8 lines
Quoted 6 lines

The BIT[31] of SBI HSM suspend types allows us to differentiate between retentive and non-retentive suspend types so we should use this BIT to call appropriate CPU_PM_CPU_IDLE_ENTER_xyz() macro.

Fixes 6abf32f1d9c5 ("cpuidle: Add RISC-V SBI CPU idle driver")
Signed-off-by Anup Patel <[email protected]>

Can you please take this patch through the RISC-V tree ?

Sorry I missed this, I didn't realize you wanted me to merge it. It's on fixes.

Quoted 30 lines

Regards,
Anup

Quoted 26 lines
---
 drivers/cpuidle/cpuidle-riscv-sbi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/cpuidle/cpuidle-riscv-sbi.c +6 -2
index 1151e5e2ba82..33c92fec4365 100644
@@ -97,8 +97,13 @@ static int sbi_cpuidle_enter_state(struct cpuidle_device *dev,
                                   struct cpuidle_driver *drv, int idx)
 {
        u32 *states = __this_cpu_read(sbi_cpuidle_data.states);
+       u32 state = states[idx];
 
-       return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend, idx, states[idx]);
+       if (state & SBI_HSM_SUSP_NON_RET_BIT)
+               return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend, idx, state);
+       else
+               return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(sbi_suspend,
+                                                            idx, state);
 }
 
 static int __sbi_enter_domain_idle_state(struct cpuidle_device *dev,
--

2.34.1

maintainer
#4 lore raw
Quoted 13 lines · Palmer Dabbelt
Quoted 12 lines · On Sun, 28 Aug 2022 19:59:45 PDT (-0700), [email protected] wrote:

Hi Palmer,

Quoted 9 lines · Anup Patel

Currently, we are using CPU_PM_CPU_IDLE_ENTER_PARAM() for all SBI HSM suspend types so retentive suspend types are also treated non-retentive and kernel will do redundant additional work for these states.

The BIT[31] of SBI HSM suspend types allows us to differentiate between retentive and non-retentive suspend types so we should use this BIT to call appropriate CPU_PM_CPU_IDLE_ENTER_xyz() macro.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Fixes 6abf32f1d9c5 ("cpuidle: Add RISC-V SBI CPU idle driver")
Signed-off-by Anup Patel <[email protected]>

Can you please take this patch through the RISC-V tree ?

Sorry I missed this, I didn't realize you wanted me to merge it. It's on fixes.

Actually from reading this again, it's not really a fix: the kernel is correct without this change, it's just going to save/restore when it doesn't need to. So I'm queuing this into for-next instead.

Quoted 27 lines
Quoted 27 lines

Regards,
Anup

Quoted 22 lines
---
 drivers/cpuidle/cpuidle-riscv-sbi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/cpuidle/cpuidle-riscv-sbi.c +6 -1
index 1151e5e2ba82..33c92fec4365 100644
@@ -97,8 +97,13 @@ static int sbi_cpuidle_enter_state(struct cpuidle_device *dev,
                                   struct cpuidle_driver *drv, int idx)
 {
        u32 *states = __this_cpu_read(sbi_cpuidle_data.states);
+       u32 state = states[idx];
 
-       return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend, idx, states[idx]);
+       if (state & SBI_HSM_SUSP_NON_RET_BIT)
+               return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend, idx, state);
+       else
+               return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(sbi_suspend,
+                                                            idx, state);
 }
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/cpuidle/cpuidle-riscv-sbi.c +0 -1
 static int __sbi_enter_domain_idle_state(struct cpuidle_device *dev,
--

2.34.1

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv