[PATCH 1/2] remoteproc: imx_dsp_rproc: Skip RP_MBOX_SUSPEND_SYSTEM when mailbox TX channel is uninitialized

[PATCH 1/2] remoteproc: imx_dsp_rproc: Skip RP_MBOX_SUSPEND_SYSTEM when mailbox TX channel is uninitialized

[Iuliana Prodan (OSS)]

From: Iuliana Prodan <iuliana.prodan@nxp.com>

Firmwares that do not use mailbox communication
(e.g., the hello_world sample) leave priv->tx_ch
as NULL. The current suspend logic unconditionally
sends RP_MBOX_SUSPEND_SYSTEM, which is invalid without
an initialized TX channel.

Detect the no_mailboxes case early and skip sending
the suspend message. Instead, proceed directly to
the runtime PM suspend path, which is the correct
behavior for firmwares that cannot respond to mailbox
requests.

Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
---
 drivers/remoteproc/imx_dsp_rproc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index f11662f9a12f..fc0470aa72c1 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -1308,6 +1308,15 @@ static int imx_dsp_suspend(struct device *dev)
 	if (rproc->state != RPROC_RUNNING)
 		goto out;
 
+	/*
+	 * No channel available for sending messages;
+	 * indicates no mailboxes present, so trigger PM runtime suspend
+	 */
+	if (!priv->tx_ch) {
+		dev_err(dev, "No initialized mbox tx channel\n");
+		goto out;
+	}
+
 	reinit_completion(&priv->pm_comp);
 
 	/* Tell DSP that suspend is happening */
-- 
2.34.1

[PATCH 2/2] remoteproc: imx_dsp_rproc: Wait for suspend ACK only if WAIT_FW_READY is set

[Iuliana Prodan (OSS)]

From: Iuliana Prodan <iuliana.prodan@nxp.com>

The DSP suspend path currently waits unconditionally
for a suspend ack from the firmware.
This breaks firmwares that do not implement the
mailbox-based READY handshake, as the DSP never
responds and system suspend fails with -EBUSY.

The driver already uses the WAIT_FW_READY flag to
indicate that the firmware supports the READY
handshake at boot. Apply the same logic during
suspend: only wait for the suspend ack when the
firmware is expected to support it.

Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
---
 drivers/remoteproc/imx_dsp_rproc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index fc0470aa72c1..e25dbe32ef79 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -1327,10 +1327,11 @@ static int imx_dsp_suspend(struct device *dev)
 	}
 
 	/*
-	 * DSP need to save the context at suspend.
-	 * Here waiting the response for DSP, then power can be disabled.
+	 * The DSP must save its context during suspend.
+	 * Wait for a response from the DSP if required before disabling power.
 	 */
-	if (!wait_for_completion_timeout(&priv->pm_comp, msecs_to_jiffies(100)))
+	if (priv->flags & WAIT_FW_READY &&
+	    !wait_for_completion_timeout(&priv->pm_comp, msecs_to_jiffies(100)))
 		return -EBUSY;
 
 out:
-- 
2.34.1

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: Wait for suspend ACK only if WAIT_FW_READY is set

[Bjorn Andersson]

On Tue, Nov 25, 2025 at 02:49:03PM +0200, Iuliana Prodan (OSS) wrote:
> From: Iuliana Prodan <iuliana.prodan@nxp.com>
> 
> The DSP suspend path currently waits unconditionally
> for a suspend ack from the firmware.
> This breaks firmwares that do not implement the
> mailbox-based READY handshake, as the DSP never
> responds and system suspend fails with -EBUSY.
> 

But if the firmware doesn't implement "mailbox-based READY handshake",
do you still want to send the RP_MBOX_SUSPEND_SYSTEM message?

If so, can you clarify here in the commit message that the firmware
expects the mailbox-based message, and only the "handshake" part should
be omitted.

If that part isn't implemented either, then I think you should fix the
code to not poke the mailbox in the first place.


Also, wrap your commit message at 75 characters, please.

> The driver already uses the WAIT_FW_READY flag to
> indicate that the firmware supports the READY
> handshake at boot. Apply the same logic during
> suspend: only wait for the suspend ack when the
> firmware is expected to support it.
> 
> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
> ---
>  drivers/remoteproc/imx_dsp_rproc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index fc0470aa72c1..e25dbe32ef79 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -1327,10 +1327,11 @@ static int imx_dsp_suspend(struct device *dev)
>  	}
>  
>  	/*
> -	 * DSP need to save the context at suspend.
> -	 * Here waiting the response for DSP, then power can be disabled.
> +	 * The DSP must save its context during suspend.

Please double check that this comment reflect above conclusion.

Regards,
Bjorn

> +	 * Wait for a response from the DSP if required before disabling power.
>  	 */
> -	if (!wait_for_completion_timeout(&priv->pm_comp, msecs_to_jiffies(100)))
> +	if (priv->flags & WAIT_FW_READY &&
> +	    !wait_for_completion_timeout(&priv->pm_comp, msecs_to_jiffies(100)))
>  		return -EBUSY;
>  
>  out:
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: Wait for suspend ACK only if WAIT_FW_READY is set

[Iuliana Prodan]

On 11/26/2025 10:21 PM, Bjorn Andersson wrote:
> On Tue, Nov 25, 2025 at 02:49:03PM +0200, Iuliana Prodan (OSS) wrote:
>> From: Iuliana Prodan <iuliana.prodan@nxp.com>
>>
>> The DSP suspend path currently waits unconditionally
>> for a suspend ack from the firmware.
>> This breaks firmwares that do not implement the
>> mailbox-based READY handshake, as the DSP never
>> responds and system suspend fails with -EBUSY.
>>
> 
> But if the firmware doesn't implement "mailbox-based READY handshake",
> do you still want to send the RP_MBOX_SUSPEND_SYSTEM message?
> 
If FEATURE_DONT_WAIT_FW_READY is set, it means the remote does not send 
any feedback to the host and does not handle specific messages. In this 
case, it’s better not to send the message at all.

BTW, to avoid confusion with FW_READY macros, I’ll add a new patch to 
rename these macros to FW_CONFIRMATION, since they are used both for 
boot confirmation and other firmware acknowledgments.

> If so, can you clarify here in the commit message that the firmware
> expects the mailbox-based message, and only the "handshake" part should
> be omitted.
> 
> If that part isn't implemented either, then I think you should fix the
> code to not poke the mailbox in the first place.
> 
Yes, will fix in v2.

> 
> Also, wrap your commit message at 75 characters, please.
> 
>> The driver already uses the WAIT_FW_READY flag to
>> indicate that the firmware supports the READY
>> handshake at boot. Apply the same logic during
>> suspend: only wait for the suspend ack when the
>> firmware is expected to support it.
>>
>> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
>> ---
>>   drivers/remoteproc/imx_dsp_rproc.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
>> index fc0470aa72c1..e25dbe32ef79 100644
>> --- a/drivers/remoteproc/imx_dsp_rproc.c
>> +++ b/drivers/remoteproc/imx_dsp_rproc.c
>> @@ -1327,10 +1327,11 @@ static int imx_dsp_suspend(struct device *dev)
>>   	}
>>   
>>   	/*
>> -	 * DSP need to save the context at suspend.
>> -	 * Here waiting the response for DSP, then power can be disabled.
>> +	 * The DSP must save its context during suspend.
> 
> Please double check that this comment reflect above conclusion.
> 
This is valid for some firmware, like Xtensa Audio Firmware (XAF), where 
we use both firmware-ready replay and the messages are handled on the 
remote side, where the DSP context is saved.

> Regards,
> Bjorn
> 
>> +	 * Wait for a response from the DSP if required before disabling power.
>>   	 */
>> -	if (!wait_for_completion_timeout(&priv->pm_comp, msecs_to_jiffies(100)))
>> +	if (priv->flags & WAIT_FW_READY &&
>> +	    !wait_for_completion_timeout(&priv->pm_comp, msecs_to_jiffies(100)))
>>   		return -EBUSY;
>>   
>>   out:
>> -- 
>> 2.34.1
>>

Re: [PATCH 1/2] remoteproc: imx_dsp_rproc: Skip RP_MBOX_SUSPEND_SYSTEM when mailbox TX channel is uninitialized

[Bjorn Andersson]

On Tue, Nov 25, 2025 at 02:49:02PM +0200, Iuliana Prodan (OSS) wrote:
> From: Iuliana Prodan <iuliana.prodan@nxp.com>
> 
> Firmwares that do not use mailbox communication
> (e.g., the hello_world sample) leave priv->tx_ch
> as NULL. The current suspend logic unconditionally
> sends RP_MBOX_SUSPEND_SYSTEM, which is invalid without
> an initialized TX channel.
> 
> Detect the no_mailboxes case early and skip sending
> the suspend message. Instead, proceed directly to
> the runtime PM suspend path, which is the correct
> behavior for firmwares that cannot respond to mailbox
> requests.

Please use the allotted 75 characters of width for your commit message.

> 
> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
> ---
>  drivers/remoteproc/imx_dsp_rproc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index f11662f9a12f..fc0470aa72c1 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -1308,6 +1308,15 @@ static int imx_dsp_suspend(struct device *dev)
>  	if (rproc->state != RPROC_RUNNING)
>  		goto out;
>  
> +	/*
> +	 * No channel available for sending messages;
> +	 * indicates no mailboxes present, so trigger PM runtime suspend
> +	 */
> +	if (!priv->tx_ch) {
> +		dev_err(dev, "No initialized mbox tx channel\n");

Commit message and comment above says this is "normal" behavior,
dev_err() indicates that it's not. Should this be a dev_info()?

That said, it's still a message every time you suspend, so perhaps even
omitting the print (or a dev_dbg()) makes more sense?

Regards,
Bjorn

> +		goto out;
> +	}
> +
>  	reinit_completion(&priv->pm_comp);
>  
>  	/* Tell DSP that suspend is happening */
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] remoteproc: imx_dsp_rproc: Skip RP_MBOX_SUSPEND_SYSTEM when mailbox TX channel is uninitialized

[Iuliana Prodan]

On 11/26/2025 10:14 PM, Bjorn Andersson wrote:
> On Tue, Nov 25, 2025 at 02:49:02PM +0200, Iuliana Prodan (OSS) wrote:
>> From: Iuliana Prodan <iuliana.prodan@nxp.com>
>>
>> Firmwares that do not use mailbox communication
>> (e.g., the hello_world sample) leave priv->tx_ch
>> as NULL. The current suspend logic unconditionally
>> sends RP_MBOX_SUSPEND_SYSTEM, which is invalid without
>> an initialized TX channel.
>>
>> Detect the no_mailboxes case early and skip sending
>> the suspend message. Instead, proceed directly to
>> the runtime PM suspend path, which is the correct
>> behavior for firmwares that cannot respond to mailbox
>> requests.
> 
> Please use the allotted 75 characters of width for your commit message.
> 
Right, will do in v2.

>>
>> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
>> ---
>>   drivers/remoteproc/imx_dsp_rproc.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
>> index f11662f9a12f..fc0470aa72c1 100644
>> --- a/drivers/remoteproc/imx_dsp_rproc.c
>> +++ b/drivers/remoteproc/imx_dsp_rproc.c
>> @@ -1308,6 +1308,15 @@ static int imx_dsp_suspend(struct device *dev)
>>   	if (rproc->state != RPROC_RUNNING)
>>   		goto out;
>>   
>> +	/*
>> +	 * No channel available for sending messages;
>> +	 * indicates no mailboxes present, so trigger PM runtime suspend
>> +	 */
>> +	if (!priv->tx_ch) {
>> +		dev_err(dev, "No initialized mbox tx channel\n");
> 
> Commit message and comment above says this is "normal" behavior,
> dev_err() indicates that it's not. Should this be a dev_info()?
> 
> That said, it's still a message every time you suspend, so perhaps even
> omitting the print (or a dev_dbg()) makes more sense?
There's no error here; it's just a use case.
dev_dbg makes more sense, I agree.

> 
> Regards,
> Bjorn
> 
>> +		goto out;
>> +	}
>> +
>>   	reinit_completion(&priv->pm_comp);
>>   
>>   	/* Tell DSP that suspend is happening */
>> -- 
>> 2.34.1
>>