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

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

[Peng Fan (OSS)]

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.

- The choice of S2R(MEM) by default. The userspace can configure whatever
  default behaviour expected as S2R, if issuing suspend from userspace.

- The userspace needs to keep the wakeup source enabled, otherwise the
  system may never resume back.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Update commit log
 Add comment for suspend_work

 .../firmware/arm_scmi/scmi_power_control.c    | 21 ++++++++++++++++++-
 1 file changed, 20 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..21f467a92942 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>
@@ -78,6 +79,7 @@ enum scmi_syspower_state {
  * @reboot_nb: A notifier_block optionally used to track reboot progress
  * @forceful_work: A worker used to trigger a forceful transition once a
  *		   graceful has timed out.
+ * @suspend_work: A worker used to trigger system suspend
  */
 struct scmi_syspower_conf {
 	struct device *dev;
@@ -90,6 +92,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 +252,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 +283,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 +322,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 +355,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Cristian Marussi]

On Sun, Apr 28, 2024 at 03:51:05PM +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.
> 
> - The choice of S2R(MEM) by default. The userspace can configure whatever
>   default behaviour expected as S2R, if issuing suspend from userspace.
> 
> - The userspace needs to keep the wakeup source enabled, otherwise the
>   system may never resume back.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Have you tried triggering a suspend with this on your setup ?

Anyway, LGTM.
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH V2] firmware: arm_scmi: power_control: support
> suspend command
> 
> On Sun, Apr 28, 2024 at 03:51:05PM +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.
> >
> > - The choice of S2R(MEM) by default. The userspace can configure whatever
> >   default behaviour expected as S2R, if issuing suspend from userspace.
> >
> > - The userspace needs to keep the wakeup source enabled, otherwise the
> >   system may never resume back.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> 
> Have you tried triggering a suspend with this on your setup ?

Yes, this feature is already in NXP internal git tree and tested.

> 
> Anyway, LGTM.
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> 

Thanks,
Peng.
> Thanks,
> Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

Hi Sudeep,

> Subject: [PATCH V2] firmware: arm_scmi: power_control: support suspend
> command

Is this patch good for you to pick up?

Thanks,
Peng.

> 
> 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.
> 
> - The choice of S2R(MEM) by default. The userspace can configure whatever
>   default behaviour expected as S2R, if issuing suspend from userspace.
> 
> - The userspace needs to keep the wakeup source enabled, otherwise the
>   system may never resume back.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>  Update commit log
>  Add comment for suspend_work
> 
>  .../firmware/arm_scmi/scmi_power_control.c    | 21 ++++++++++++++++++-
>  1 file changed, 20 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..21f467a92942 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>
> @@ -78,6 +79,7 @@ enum scmi_syspower_state {
>   * @reboot_nb: A notifier_block optionally used to track reboot progress
>   * @forceful_work: A worker used to trigger a forceful transition once a
>   *		   graceful has timed out.
> + * @suspend_work: A worker used to trigger system suspend
>   */
>  struct scmi_syspower_conf {
>  	struct device *dev;
> @@ -90,6 +92,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 +252,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 +283,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 +322,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 +355,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Sun, 28 Apr 2024 15:51:05 +0800, Peng Fan (OSS) wrote:
> 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.
>
> - The choice of S2R(MEM) by default. The userspace can configure whatever
>   default behaviour expected as S2R, if issuing suspend from userspace.
>
> [...]

Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!

[1/1] firmware: arm_scmi: power_control: support suspend command
      https://git.kernel.org/sudeep.holla/c/dd22cc907a74
--
Regards,
Sudeep