1. Patchew
  2. linux
  3. View series

[PATCH] firmware: arm_scmi: power_control: support suspend command

Peng Fan (OSS) posted 1 patch 1 year, 9 months ago
There is a newer version of this series
.../firmware/arm_scmi/scmi_power_control.c    | 20 ++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
[PATCH] firmware: arm_scmi: power_control: support suspend command
Posted by Peng Fan (OSS) 1 year, 9 months ago 
From: Peng Fan <peng.fan@nxp.com>

Support System suspend notification. Using a work struct to call
pm_suspend. There is no way to pass suspend level to pm_suspend,
so use MEM as of now.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../firmware/arm_scmi/scmi_power_control.c    | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/scmi_power_control.c b/drivers/firmware/arm_scmi/scmi_power_control.c
index 6eb7d2a4b6b1..beafca9957c7 100644
--- a/drivers/firmware/arm_scmi/scmi_power_control.c
+++ b/drivers/firmware/arm_scmi/scmi_power_control.c
@@ -50,6 +50,7 @@
 #include <linux/reboot.h>
 #include <linux/scmi_protocol.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 #include <linux/time64.h>
 #include <linux/timer.h>
 #include <linux/types.h>
@@ -90,6 +91,7 @@ struct scmi_syspower_conf {
 	struct notifier_block reboot_nb;
 
 	struct delayed_work forceful_work;
+	struct work_struct suspend_work;
 };
 
 #define userspace_nb_to_sconf(x)	\
@@ -249,6 +251,9 @@ static void scmi_request_graceful_transition(struct scmi_syspower_conf *sc,
 	case SCMI_SYSTEM_WARMRESET:
 		orderly_reboot();
 		break;
+	case SCMI_SYSTEM_SUSPEND:
+		schedule_work(&sc->suspend_work);
+		break;
 	default:
 		break;
 	}
@@ -277,7 +282,8 @@ static int scmi_userspace_notifier(struct notifier_block *nb,
 	struct scmi_system_power_state_notifier_report *er = data;
 	struct scmi_syspower_conf *sc = userspace_nb_to_sconf(nb);
 
-	if (er->system_state >= SCMI_SYSTEM_POWERUP) {
+	if (er->system_state >= SCMI_SYSTEM_MAX ||
+	    er->system_state == SCMI_SYSTEM_POWERUP) {
 		dev_err(sc->dev, "Ignoring unsupported system_state: 0x%X\n",
 			er->system_state);
 		return NOTIFY_DONE;
@@ -315,6 +321,16 @@ static int scmi_userspace_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+static void scmi_suspend_work_func(struct work_struct *work)
+{
+	struct scmi_syspower_conf *sc =
+		container_of(work, struct scmi_syspower_conf, suspend_work);
+
+	pm_suspend(PM_SUSPEND_MEM);
+
+	sc->state = SCMI_SYSPOWER_IDLE;
+}
+
 static int scmi_syspower_probe(struct scmi_device *sdev)
 {
 	int ret;
@@ -338,6 +354,8 @@ static int scmi_syspower_probe(struct scmi_device *sdev)
 	sc->userspace_nb.notifier_call = &scmi_userspace_notifier;
 	sc->dev = &sdev->dev;
 
+	INIT_WORK(&sc->suspend_work, scmi_suspend_work_func);
+
 	return handle->notify_ops->devm_event_notifier_register(sdev,
 							   SCMI_PROTOCOL_SYSTEM,
 					 SCMI_EVENT_SYSTEM_POWER_STATE_NOTIFIER,
-- 
2.37.1
Re: [PATCH] firmware: arm_scmi: power_control: support suspend command
Posted by Sudeep Holla 1 year, 9 months ago 
On Mon, Apr 15, 2024 at 06:11:41PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Support System suspend notification. Using a work struct to call
> pm_suspend. There is no way to pass suspend level to pm_suspend,
> so use MEM as of now.
>

While the change itself is simple and no-controversial, I am bit worried
about:

1. The choice of S2R(MEM) by default - not sure if different system
   prefer different things. The userspace can configure whatever default
   behaviour expected as S2R IIUC, so should be OK. I need to check though.

2. The userspace needs to keep the wakeup source enabled always which
   I need to check if it is feasible on all the platforms. If wakeups
   are not configured properly and suspend is triggered, the system can
   never resume back.

We may need to mention above points at-least as part of commit log. I
would wait for some feedback from SCMI users.

--
Regards,
Sudeep
Re: [PATCH] firmware: arm_scmi: power_control: support suspend command
Posted by Cristian Marussi 1 year, 9 months ago 
On Mon, Apr 15, 2024 at 06:11:41PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Support System suspend notification. Using a work struct to call
> pm_suspend. There is no way to pass suspend level to pm_suspend,
> so use MEM as of now.
> 

Hi Peng,

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../firmware/arm_scmi/scmi_power_control.c    | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 

This LGTM in general, the only obsservation here is that while on
shutdown by issuing a orderly_reboot() we in fact ask PID_1 to start a
shutdown from userspace, when triggering a system suspend with pm_suspend()
we do not involve userspace in the process, but I dont think there is another
way indeed.

Thanks,
Cristian

> diff --git a/drivers/firmware/arm_scmi/scmi_power_control.c b/drivers/firmware/arm_scmi/scmi_power_control.c
> index 6eb7d2a4b6b1..beafca9957c7 100644
> --- a/drivers/firmware/arm_scmi/scmi_power_control.c
> +++ b/drivers/firmware/arm_scmi/scmi_power_control.c
> @@ -50,6 +50,7 @@
>  #include <linux/reboot.h>
>  #include <linux/scmi_protocol.h>
>  #include <linux/slab.h>
> +#include <linux/suspend.h>
>  #include <linux/time64.h>
>  #include <linux/timer.h>
>  #include <linux/types.h>
> @@ -90,6 +91,7 @@ struct scmi_syspower_conf {
>  	struct notifier_block reboot_nb;
>  
>  	struct delayed_work forceful_work;
> +	struct work_struct suspend_work;
>  };
>  
>  #define userspace_nb_to_sconf(x)	\
> @@ -249,6 +251,9 @@ static void scmi_request_graceful_transition(struct scmi_syspower_conf *sc,
>  	case SCMI_SYSTEM_WARMRESET:
>  		orderly_reboot();
>  		break;
> +	case SCMI_SYSTEM_SUSPEND:
> +		schedule_work(&sc->suspend_work);
> +		break;
>  	default:
>  		break;
>  	}
> @@ -277,7 +282,8 @@ static int scmi_userspace_notifier(struct notifier_block *nb,
>  	struct scmi_system_power_state_notifier_report *er = data;
>  	struct scmi_syspower_conf *sc = userspace_nb_to_sconf(nb);
>  
> -	if (er->system_state >= SCMI_SYSTEM_POWERUP) {
> +	if (er->system_state >= SCMI_SYSTEM_MAX ||
> +	    er->system_state == SCMI_SYSTEM_POWERUP) {
>  		dev_err(sc->dev, "Ignoring unsupported system_state: 0x%X\n",
>  			er->system_state);
>  		return NOTIFY_DONE;
> @@ -315,6 +321,16 @@ static int scmi_userspace_notifier(struct notifier_block *nb,
>  	return NOTIFY_OK;
>  }
>  
> +static void scmi_suspend_work_func(struct work_struct *work)
> +{
> +	struct scmi_syspower_conf *sc =
> +		container_of(work, struct scmi_syspower_conf, suspend_work);
> +
> +	pm_suspend(PM_SUSPEND_MEM);
> +
> +	sc->state = SCMI_SYSPOWER_IDLE;
> +}
> +
>  static int scmi_syspower_probe(struct scmi_device *sdev)
>  {
>  	int ret;
> @@ -338,6 +354,8 @@ static int scmi_syspower_probe(struct scmi_device *sdev)
>  	sc->userspace_nb.notifier_call = &scmi_userspace_notifier;
>  	sc->dev = &sdev->dev;
>  
> +	INIT_WORK(&sc->suspend_work, scmi_suspend_work_func);
> +
>  	return handle->notify_ops->devm_event_notifier_register(sdev,
>  							   SCMI_PROTOCOL_SYSTEM,
>  					 SCMI_EVENT_SYSTEM_POWER_STATE_NOTIFIER,
> -- 
> 2.37.1
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.