From: Peng Fan <[email protected]>
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 <[email protected]>
---
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
On Sun, Apr 28, 2024 at 03:51:05PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> 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 <[email protected]>
> ---
Have you tried triggering a suspend with this on your setup ?
Anyway, LGTM.
Reviewed-by: Cristian Marussi <[email protected]>
Thanks,
Cristian
> 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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
>
> 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 <[email protected]>
>
Thanks,
Peng.
> Thanks,
> Cristian
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 <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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