Re: [PATCH] remoteproc: imx_dsp_rproc: conditionally wait for FW_READY

[Mathieu Poirier]

Good morning,

On Wed, Mar 05, 2025 at 02:39:23PM +0200, Iuliana Prodan (OSS) wrote:
> From: Iuliana Prodan <iuliana.prodan@xxxxxxx>
> 
> Some DSP firmware requires a FW_READY signal before proceeding,
> while others do not.
> Introduce imx_dsp_rproc_wait_fw_ready() to check the resource table
> and determine if waiting is needed.
> 
> Use the WAIT_FW_READY flag (bit 1) to distinguish cases where
> waiting is required, as bit 0 is reserved for VIRTIO_RPMSG_F_NS
> in OpenAMP and mentioned in rpmsg documentation (not used in Linux,
> so far).

VIRTIO_RPMSG_F_NS is used in [1].

[1]. https://elixir.bootlin.com/linux/v6.14-rc5/source/drivers/rpmsg/virtio_rpmsg_bus.c#L1051

> This flag is set by the remote processor in the dfeatures member of
> struct fw_rsc_vdev, indicating supported virtio device features.
> 
> Update imx_dsp_rproc_start() to handle this condition accordingly.
> 
> Signed-off-by: Iuliana Prodan <iuliana.prodan@xxxxxxx>
> ---
>  drivers/remoteproc/imx_dsp_rproc.c | 84 +++++++++++++++++++++++++++---
>  1 file changed, 77 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index b9bb15970966..8eefaee28061 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright 2021 NXP */
> +/* Copyright 2021, 2025 NXP */
>  
>  #include <dt-bindings/firmware/imx/rsrc.h>
>  #include <linux/arm-smccc.h>
> @@ -38,6 +38,15 @@ MODULE_PARM_DESC(no_mailboxes,
>  #define REMOTE_IS_READY				BIT(0)
>  #define REMOTE_READY_WAIT_MAX_RETRIES		500
>  
> +/*
> + * This flag is set by the remote processor in the dfeatures member of
> + * struct fw_rsc_vdev, indicating supported virtio device features
> + *
> + * Use bit 1 since bit 0 is used for VIRTIO_RPMSG_F_NS
> + * in OpenAMP and mentioned in kernel's rpmsg documentation
> + */
> +#define WAIT_FW_READY				BIT(1)
> +
>  /* att flags */
>  /* DSP own area */
>  #define ATT_OWN					BIT(31)
> @@ -300,13 +309,74 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
>  	return -ETIMEDOUT;
>  }
>  
> +/*
> + * Determines whether we should wait for a FW_READY reply
> + * from the remote processor.
> + *
> + * This function inspects the resource table associated with the remote
> + * processor to check if the firmware has indicated that waiting
> + * for a FW_READY signal is necessary.
> + * By default, wait for FW_READY unless an RSC_VDEV explicitly
> + * indicates otherwise.
> + *
> + * Return:
> + *   - true: If we should wait for FW READY
> + *   - false: If FW_READY wait is not required
> + */
> +static bool imx_dsp_rproc_wait_fw_ready(struct rproc *rproc)
> +{
> +	struct device *dev = &rproc->dev;
> +	struct fw_rsc_hdr *hdr;
> +	struct fw_rsc_vdev *rsc;
> +	int i, offset, avail;
> +
> +	/*
> +	 * If there is no resource table, wait for FW_READY
> +	 * unless no_mailboxes module param is used
> +	 */
> +	if (!rproc->table_ptr)
> +		return true;
> +
> +	/* Iterate over each resource entry in the resource table */
> +	for (i = 0; i < rproc->table_ptr->num; i++) {
> +		offset = rproc->table_ptr->offset[i];
> +		hdr = (void *)rproc->table_ptr + offset;
> +		avail = rproc->table_sz - offset - sizeof(*hdr);
> +
> +		/* Ensure the resource table is not truncated */
> +		if (avail < 0) {
> +			dev_err(dev, "Resource table is truncated\n");
> +			return true;
> +		}
> +
> +		/* Check if the resource type is a virtio device */
> +		if (hdr->type == RSC_VDEV) {
> +			rsc = (struct fw_rsc_vdev *)((void *)hdr + sizeof(*hdr));
> +
> +			/* vdev does not require waiting for FW_READY */
> +			return !!(rsc->dfeatures & WAIT_FW_READY);

>From a virtIO perspective where one virtIO device pertains to one virtIO driver,
your approach is valid.  From a remoteproc perspecrtive though, we have one
virtIO driver [2] used by several implementation (NXP, ST, TI, ...).  So far,
information conveyed by rsc->dfeatures was applicable to all implementation and
things need to remain that way.  Otherwise, it is a matter of time before custom
and global features start clashing.

Using rsc->dfeatures in the way you do above means the resource table in the FW
image needs to be mofidied.  As such, you could take advantage of the vendor
specific resource table entry already supported by the remoteproc framework [3].
>From there you provide a resource handler specific to the iMX DSP driver and
things just work.  Moreover, you wouldn't have to parse the whole resource table
every time imx_dsp_rproc_start() is called.

Hopefully this works for you.

Thanks,
Mathieu

[2]. https://elixir.bootlin.com/linux/v6.14-rc5/source/drivers/rpmsg/virtio_rpmsg_bus.c#L1054
[3]. https://elixir.bootlin.com/linux/v6.14-rc5/source/drivers/remoteproc/remoteproc_core.c#L1044

> +		}
> +	}
> +
> +	/*
> +	 * By default, wait for the FW_READY
> +	 * unless a vdev entry disables it
> +	 */
> +	return true;
> +}
> +
>  /*
>   * Start function for rproc_ops
>   *
> - * There is a handshake for start procedure: when DSP starts, it
> - * will send a doorbell message to this driver, then the
> - * REMOTE_IS_READY flags is set, then driver will kick

> - * a message to DSP.
> + * The start procedure involves a handshake: when the DSP starts, it
> + * sends a doorbell message to this driver, which sets the
> + * REMOTE_IS_READY flag. The driver then sends a message to the DSP.
> + *
> + * Before proceeding, the driver checks if it needs to wait for a
> + * firmware ready reply using imx_dsp_rproc_wait_fw_ready().
> + * If waiting is required, it calls imx_dsp_rproc_ready() to complete
> + * the initialization.
> + * If waiting is not required, the start function returns.
>   */
>  static int imx_dsp_rproc_start(struct rproc *rproc)
>  {
> @@ -335,8 +405,8 @@ static int imx_dsp_rproc_start(struct rproc *rproc)
>  
>  	if (ret)
>  		dev_err(dev, "Failed to enable remote core!\n");
> -	else
> -		ret = imx_dsp_rproc_ready(rproc);
> +	else if (imx_dsp_rproc_wait_fw_ready(rproc))
> +		return imx_dsp_rproc_ready(rproc);
>  
>  	return ret;
>  }
> -- 
> 2.25.1
>