[PATCH v2 0/2] firmware: arm_scmi: add pm ops for scmi_power_control

[PATCH v2 0/2] firmware: arm_scmi: add pm ops for scmi_power_control

[Peng Fan]

Move information could be found in patch 2 commit log, hope it is clear

When testing on i.MX95, two consecutive suspend message send to the Linux
agent, Linux will suspend(by the 1st suspend message) and wake up(by the
2nd suspend message).

The ARM SCMI spec does not allow for filtering of which messages an agent
wants to get on the system power protocol. To i.MX95, as we use mailbox
to receive message, and the mailbox supports wake up(interrupt is always
enabled), so linux will also get a repeated suspend message. This will
cause Linux to wake (and should then go back into suspend).

This patchset is to make the 2nd suspend message could suspend linux
again.

So why SCMI firmware couldn't block the 2nd suspend message from being
sent to Linux agent? Per checking with our SCMI firmware owner:
The SM(System Manager) does not know exactly when Linux is in suspend.
There are no handshakes that clearly tell the SM this. The flow should
be, if in suspend and you send a suspend (or graceful reset/power off)
it will wake and then do the request action

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Changes in v2:
- Simplify code follow Dan's comments for patch 1
- Rewrite commit log for patch 2 to make it clear
- Rebased to next-20250703
- Link to v1: https://lore.kernel.org/r/20250620-scmi-pm-v1-0-c2f02cae5122@nxp.com

---
Peng Fan (2):
      firmware: arm_scmi: bus: Add pm ops
      firmware: arm_scmi: power_control: Set SCMI_SYSPOWER_IDLE in PM resume

 drivers/firmware/arm_scmi/bus.c                | 33 ++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/scmi_power_control.c | 24 +++++++++++++++----
 2 files changed, 52 insertions(+), 5 deletions(-)
---
base-commit: 38e1f239ba67d7a34d6b5e300fd61cb6fdffc831
change-id: 20250620-scmi-pm-8f6170769230

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH v2 1/2] firmware: arm_scmi: bus: Add pm ops

[Peng Fan]

Take platform_pm_ops as reference. Add pm ops for scmi_bus_type,
then the scmi devices under scmi bus could have their own hooks for
suspend, resume function.

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

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 1adef03894751dae9bb752b8c7f86e5d01c5d4fd..b6ade837ecea34f147fc1b734c55eafecca5ae0c 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -323,6 +323,38 @@ static struct attribute *scmi_device_attributes_attrs[] = {
 };
 ATTRIBUTE_GROUPS(scmi_device_attributes);
 
+#ifdef CONFIG_SUSPEND
+static int scmi_pm_suspend(struct device *dev)
+{
+	const struct device_driver *drv = dev->driver;
+
+	if (drv && drv->pm && drv->pm->suspend)
+		return drv->pm->suspend(dev);
+
+	return 0;
+}
+
+static int scmi_pm_resume(struct device *dev)
+{
+	const struct device_driver *drv = dev->driver;
+
+	if (drv && drv->pm && drv->pm->resume)
+		return drv->pm->resume(dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops scmi_dev_pm_ops = {
+	.suspend = scmi_pm_suspend,
+	.resume = scmi_pm_resume,
+};
+#else
+static const struct dev_pm_ops scmi_dev_pm_ops = {
+	.suspend = NULL,
+	.resume = NULL,
+};
+#endif
+
 const struct bus_type scmi_bus_type = {
 	.name =	"scmi_protocol",
 	.match = scmi_dev_match,
@@ -330,6 +362,7 @@ const struct bus_type scmi_bus_type = {
 	.remove = scmi_dev_remove,
 	.uevent	= scmi_device_uevent,
 	.dev_groups = scmi_device_attributes_groups,
+	.pm = &scmi_dev_pm_ops,
 };
 EXPORT_SYMBOL_GPL(scmi_bus_type);
 

-- 
2.37.1

Re: [PATCH v2 1/2] firmware: arm_scmi: bus: Add pm ops

[Sudeep Holla]

On Fri, Jul 04, 2025 at 11:09:35AM +0800, Peng Fan wrote:
> Take platform_pm_ops as reference. Add pm ops for scmi_bus_type,
> then the scmi devices under scmi bus could have their own hooks for
> suspend, resume function.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/bus.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 1adef03894751dae9bb752b8c7f86e5d01c5d4fd..b6ade837ecea34f147fc1b734c55eafecca5ae0c 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -323,6 +323,38 @@ static struct attribute *scmi_device_attributes_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(scmi_device_attributes);
>  
> +#ifdef CONFIG_SUSPEND
> +static int scmi_pm_suspend(struct device *dev)
> +{
> +	const struct device_driver *drv = dev->driver;
> +
> +	if (drv && drv->pm && drv->pm->suspend)
> +		return drv->pm->suspend(dev);
> +
> +	return 0;
> +}
> +
> +static int scmi_pm_resume(struct device *dev)
> +{
> +	const struct device_driver *drv = dev->driver;
> +
> +	if (drv && drv->pm && drv->pm->resume)
> +		return drv->pm->resume(dev);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops scmi_dev_pm_ops = {
> +	.suspend = scmi_pm_suspend,

I have use pm_sleep_ptr() and removed the below NULL based struct.
Have a look at for-next/scmi/updates and let me know if you are happy
with that.

-- 
Regards,
Sudeep

RE: [PATCH v2 1/2] firmware: arm_scmi: bus: Add pm ops

[Peng Fan]

> Subject: Re: [PATCH v2 1/2] firmware: arm_scmi: bus: Add pm ops
> 
> On Fri, Jul 04, 2025 at 11:09:35AM +0800, Peng Fan wrote:
> > Take platform_pm_ops as reference. Add pm ops for scmi_bus_type,
> then
> > the scmi devices under scmi bus could have their own hooks for
> > suspend, resume function.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/firmware/arm_scmi/bus.c | 33
> > +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/bus.c
> > b/drivers/firmware/arm_scmi/bus.c index
> >
> 1adef03894751dae9bb752b8c7f86e5d01c5d4fd..b6ade837ecea34f14
> 7fc1b734c55
> > eafecca5ae0c 100644
> > --- a/drivers/firmware/arm_scmi/bus.c
> > +++ b/drivers/firmware/arm_scmi/bus.c
> > @@ -323,6 +323,38 @@ static struct attribute
> > *scmi_device_attributes_attrs[] = {  };
> > ATTRIBUTE_GROUPS(scmi_device_attributes);
> >
> > +#ifdef CONFIG_SUSPEND
> > +static int scmi_pm_suspend(struct device *dev) {
> > +	const struct device_driver *drv = dev->driver;
> > +
> > +	if (drv && drv->pm && drv->pm->suspend)
> > +		return drv->pm->suspend(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int scmi_pm_resume(struct device *dev) {
> > +	const struct device_driver *drv = dev->driver;
> > +
> > +	if (drv && drv->pm && drv->pm->resume)
> > +		return drv->pm->resume(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops scmi_dev_pm_ops = {
> > +	.suspend = scmi_pm_suspend,
> 
> I have use pm_sleep_ptr() and removed the below NULL based struct.
> Have a look at for-next/scmi/updates and let me know if you are happy
> with that.

Thanks for the cleanup. Seems kernel build bot report a failure, not
sure related to this macro or else, I am still trying to see what happens.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep

[PATCH v2 2/2] firmware: arm_scmi: power_control: Set SCMI_SYSPOWER_IDLE in PM resume

[Peng Fan]

When the Linux SCMI agent is suspended and another agent sends a second
suspend message, the Linux agent wakes up. The expected behavior is for
the Linux agent to wake up and then suspend again.

However, due to a race condition, the second suspend notification wakes
the system, but the SCMI state is not yet set to SCMI_SYSPOWER_IDLE.
This is because interrupts (e.g., i.MX95 Message Unit interrupts, which are
always enabled as wakeup sources) are active before thaw_processes() is
called. As a result, scmi_userspace_notifier() runs too early and
ignores the suspend request since the state hasn't been updated yet.

This patch addresses the race by setting the SCMI state to
SCMI_SYSPOWER_IDLE earlier-during the device resume phase, before
thaw_processes() is invoked. This ensures that when the notifier is
triggered, the state check passes and the system can suspend again as
intended.

On i.MX95, there are two scenarios where the Cortex-A cluster issues a
WFI signal:
- Linux CPU idle
- Linux suspend
The hardware state machine handles power-off sequences, with SCP involved
in one step. However, SCP cannot distinguish between a CPU idle request
and a full suspend request-it only sees a generic cluster power-off.

As a result, the SCP cannot filter out the second suspend message. By
setting SCMI_SYSPOWER_IDLE early, we ensure the Linux agent can correctly
handle the second suspend request and re-enter suspend.

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

diff --git a/drivers/firmware/arm_scmi/scmi_power_control.c b/drivers/firmware/arm_scmi/scmi_power_control.c
index 21f467a92942883be66074c37c2cab08c3e8a5cc..d2cfd9d92e711f7247a13c7773c11c0a6e582876 100644
--- a/drivers/firmware/arm_scmi/scmi_power_control.c
+++ b/drivers/firmware/arm_scmi/scmi_power_control.c
@@ -46,6 +46,7 @@
 #include <linux/math.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/pm.h>
 #include <linux/printk.h>
 #include <linux/reboot.h>
 #include <linux/scmi_protocol.h>
@@ -324,12 +325,7 @@ static int scmi_userspace_notifier(struct notifier_block *nb,
 
 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)
@@ -354,6 +350,7 @@ static int scmi_syspower_probe(struct scmi_device *sdev)
 	sc->required_transition = SCMI_SYSTEM_MAX;
 	sc->userspace_nb.notifier_call = &scmi_userspace_notifier;
 	sc->dev = &sdev->dev;
+	dev_set_drvdata(&sdev->dev, sc);
 
 	INIT_WORK(&sc->suspend_work, scmi_suspend_work_func);
 
@@ -363,6 +360,20 @@ static int scmi_syspower_probe(struct scmi_device *sdev)
 						       NULL, &sc->userspace_nb);
 }
 
+static int scmi_system_power_resume(struct device *dev)
+{
+
+	struct scmi_syspower_conf *sc = dev_get_drvdata(dev);
+
+	sc->state = SCMI_SYSPOWER_IDLE;
+
+	return 0;
+}
+
+static const struct dev_pm_ops scmi_system_power_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, scmi_system_power_resume)
+};
+
 static const struct scmi_device_id scmi_id_table[] = {
 	{ SCMI_PROTOCOL_SYSTEM, "syspower" },
 	{ },
@@ -370,6 +381,9 @@ static const struct scmi_device_id scmi_id_table[] = {
 MODULE_DEVICE_TABLE(scmi, scmi_id_table);
 
 static struct scmi_driver scmi_system_power_driver = {
+	.driver	= {
+		.pm = &scmi_system_power_pmops,
+	},
 	.name = "scmi-system-power",
 	.probe = scmi_syspower_probe,
 	.id_table = scmi_id_table,

-- 
2.37.1

Re: [PATCH v2 0/2] firmware: arm_scmi: add pm ops for scmi_power_control

[Sudeep Holla]

On Fri, 04 Jul 2025 11:09:34 +0800, Peng Fan wrote:
> Move information could be found in patch 2 commit log, hope it is clear
> 
> When testing on i.MX95, two consecutive suspend message send to the Linux
> agent, Linux will suspend(by the 1st suspend message) and wake up(by the
> 2nd suspend message).
> 
> The ARM SCMI spec does not allow for filtering of which messages an agent
> wants to get on the system power protocol. To i.MX95, as we use mailbox
> to receive message, and the mailbox supports wake up(interrupt is always
> enabled), so linux will also get a repeated suspend message. This will
> cause Linux to wake (and should then go back into suspend).
> 
> [...]

Applied to sudeep.holla/linux (for-linux-next), thanks!
(with some commit message changes and removal of #ifdef)

[1/2] firmware: arm_scmi: bus: Add pm ops
      https://git.kernel.org/sudeep.holla/c/76e65f7a0e0f
[2/2] firmware: arm_scmi: power_control: Set SCMI_SYSPOWER_IDLE in PM resume
      https://git.kernel.org/sudeep.holla/c/9a0658d3991e

--
Regards,
Sudeep