[PATCH v3] remoteproc: imx_dsp_rproc: Add support for DSP-specific features

[PATCH v3] remoteproc: imx_dsp_rproc: Add support for DSP-specific features

[Iuliana Prodan (OSS)]

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

Some DSP firmware requires a FW_READY signal before proceeding, while
others do not.
Therefore, add support to handle i.MX DSP-specific features.

Implement handle_rsc callback to handle resource table parsing and to
process DSP-specific resource, to determine if waiting is needed.

Update imx_dsp_rproc_start() to handle this condition accordingly.

Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
---
Changes in v3:
- Reviews from Mathieu Poirier:
  - Added version and magic number to vendor-specific resource table entry.
  - Updated defines to maintain backward compatibility with a resource table that doesn't have a vendor-specific resource.
    - By default, wait for `fw_ready`, unless specified otherwise.
- Link to v2: https://lore.kernel.org/all/20250318215007.2109726-1-iuliana.prodan@oss.nxp.com

Changes in v2:
- Reviews from Mathieu Poirier:
  - Use vendor-specific resource table entry.
  - Implement resource handler specific to the i.MX DSP.
- Revise commit message to include recent updates.
- Link to v1: https://lore.kernel.org/all/20250305123923.514386-1-iuliana.prodan@oss.nxp.com/

 drivers/remoteproc/imx_dsp_rproc.c | 102 ++++++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index b9bb15970966..80d4470cc731 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -35,9 +35,17 @@ module_param_named(no_mailboxes, no_mailboxes, int, 0644);
 MODULE_PARM_DESC(no_mailboxes,
 		 "There is no mailbox between cores, so ignore remote proc reply after start, default is 0 (off).");
 
+/* Flag indicating that the remote is up and running */
 #define REMOTE_IS_READY				BIT(0)
+/* Flag indicating that the host should wait for a firmware-ready response */
+#define WAIT_FW_READY				BIT(1)
 #define REMOTE_READY_WAIT_MAX_RETRIES		500
 
+/* This flag is set in the DSP resource table's features field to indicate
+ * that the firmware requires the host NOT to wait for a FW_READY response.
+ */
+#define FEATURE_DONT_WAIT_FW_READY		BIT(0)
+
 /* att flags */
 /* DSP own area */
 #define ATT_OWN					BIT(31)
@@ -72,6 +80,10 @@ MODULE_PARM_DESC(no_mailboxes,
 
 #define IMX8ULP_SIP_HIFI_XRDC			0xc200000e
 
+#define FW_RSC_NXP_S_MAGIC			((uint32_t)'n' << 24 |	\
+						 (uint32_t)'x' << 16 |	\
+						 (uint32_t)'p' << 8 |	\
+						 (uint32_t)'s')
 /*
  * enum - Predefined Mailbox Messages
  *
@@ -136,6 +148,24 @@ struct imx_dsp_rproc_dcfg {
 	int (*reset)(struct imx_dsp_rproc *priv);
 };
 
+/**
+ * struct fw_rsc_imx_dsp - i.MX DSP specific info
+ *
+ * @len: length of the resource entry
+ * @magic_num: 32-bit magic number
+ * @version: version of data structure
+ * @features: feature flags supported by the i.MX DSP firmware
+ *
+ * This represents a DSP-specific resource in the firmware's
+ * resource table, providing information on supported features.
+ */
+struct fw_rsc_imx_dsp {
+	uint32_t len;
+	uint32_t magic_num;
+	uint32_t version;
+	uint32_t features;
+} __packed;
+
 static const struct imx_rproc_att imx_dsp_rproc_att_imx8qm[] = {
 	/* dev addr , sys addr  , size	    , flags */
 	{ 0x596e8000, 0x556e8000, 0x00008000, ATT_OWN },
@@ -300,6 +330,73 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
 	return -ETIMEDOUT;
 }
 
+/**
+ * imx_dsp_rproc_handle_rsc() - Handle DSP-specific resource table entries
+ * @rproc: remote processor instance
+ * @rsc_type: resource type identifier
+ * @rsc: pointer to the resource entry
+ * @offset: offset of the resource entry
+ * @avail: available space in the resource table
+ *
+ * Parse the DSP-specific resource entry and update flags accordingly.
+ * If the WAIT_FW_READY feature is set, the host must wait for the firmware
+ * to signal readiness before proceeding with execution.
+ *
+ * Return: RSC_HANDLED if processed successfully, RSC_IGNORED otherwise.
+ */
+static int imx_dsp_rproc_handle_rsc(struct rproc *rproc, u32 rsc_type,
+				    void *rsc, int offset, int avail)
+{
+	struct imx_dsp_rproc *priv = rproc->priv;
+	struct fw_rsc_imx_dsp *imx_dsp_rsc = rsc;
+	struct device *dev = rproc->dev.parent;
+	size_t expected_size;
+
+	if (!imx_dsp_rsc) {
+		dev_dbg(dev, "Invalid fw_rsc_imx_dsp.\n");
+		goto ignored;
+	}
+
+	/* Make sure resource isn't truncated */
+	expected_size = imx_dsp_rsc->len + sizeof(imx_dsp_rsc->len);
+	if (expected_size < sizeof(struct fw_rsc_imx_dsp)) {
+		dev_dbg(dev, "Resource fw_rsc_imx_dsp is truncated.\n");
+		goto ignored;
+	}
+
+	/*
+	 * If FW_RSC_NXP_S_MAGIC number is not found then
+	 * wait for fw_ready reply (default work flow)
+	 */
+	if (imx_dsp_rsc->magic_num != FW_RSC_NXP_S_MAGIC) {
+		dev_dbg(dev, "Invalid resource table magic number.\n");
+		goto ignored;
+	}
+
+	/*
+	 * For now, in struct fw_rsc_imx_dsp, version 0,
+	 * only FEATURE_DONT_WAIT_FW_READY is valid.
+	 *
+	 * When adding new features, please upgrade version.
+	 */
+	if (imx_dsp_rsc->version > 0) {
+		dev_warn(dev, "Unexpected fw_rsc_imx_dsp version %d.\n",
+			 imx_dsp_rsc->version);
+		goto ignored;
+	}
+
+	if (imx_dsp_rsc->features & FEATURE_DONT_WAIT_FW_READY)
+		priv->flags &= ~WAIT_FW_READY;
+	else
+		priv->flags |= WAIT_FW_READY;
+
+	return RSC_HANDLED;
+
+ignored:
+	priv->flags |= WAIT_FW_READY;
+	return RSC_IGNORED;
+}
+
 /*
  * Start function for rproc_ops
  *
@@ -335,8 +432,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 (priv->flags & WAIT_FW_READY)
+		return imx_dsp_rproc_ready(rproc);
 
 	return ret;
 }
@@ -936,6 +1033,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
 	.kick		= imx_dsp_rproc_kick,
 	.load		= imx_dsp_rproc_elf_load_segments,
 	.parse_fw	= imx_dsp_rproc_parse_fw,
+	.handle_rsc	= imx_dsp_rproc_handle_rsc,
 	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
 	.sanity_check	= rproc_elf_sanity_check,
 	.get_boot_addr	= rproc_elf_get_boot_addr,
-- 
2.25.1

Re: [PATCH v3] remoteproc: imx_dsp_rproc: Add support for DSP-specific features

[Frank Li]

On Thu, Apr 03, 2025 at 01:01:24PM +0300, Iuliana Prodan (OSS) wrote:
> From: Iuliana Prodan <iuliana.prodan@nxp.com>

subject: remoteproc: imx_dsp_rproc: add handle_rsc callback to handle DSP-specific features

> Some DSP firmware requires a FW_READY signal before proceeding, while
> others do not.
> Therefore, add support to handle i.MX DSP-specific features.

Add support to handle i.MX DSP-specific features because Some DSP firmware
requires a FW_READY signal before proceeding

>
> Implement handle_rsc callback to handle resource table parsing and to
> process DSP-specific resource, to determine if waiting is needed.


Implement the handle_rsc callback to parse the resource table and process
DSP-specific resources to determine if waiting is needed.

>
> Update imx_dsp_rproc_start() to handle this condition accordingly.
>
> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
> ---
> Changes in v3:
> - Reviews from Mathieu Poirier:
>   - Added version and magic number to vendor-specific resource table entry.
>   - Updated defines to maintain backward compatibility with a resource table that doesn't have a vendor-specific resource.
>     - By default, wait for `fw_ready`, unless specified otherwise.
> - Link to v2: https://lore.kernel.org/all/20250318215007.2109726-1-iuliana.prodan@oss.nxp.com
>
> Changes in v2:
> - Reviews from Mathieu Poirier:
>   - Use vendor-specific resource table entry.
>   - Implement resource handler specific to the i.MX DSP.
> - Revise commit message to include recent updates.
> - Link to v1: https://lore.kernel.org/all/20250305123923.514386-1-iuliana.prodan@oss.nxp.com/
>
>  drivers/remoteproc/imx_dsp_rproc.c | 102 ++++++++++++++++++++++++++++-
>  1 file changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index b9bb15970966..80d4470cc731 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -35,9 +35,17 @@ module_param_named(no_mailboxes, no_mailboxes, int, 0644);
>  MODULE_PARM_DESC(no_mailboxes,
>  		 "There is no mailbox between cores, so ignore remote proc reply after start, default is 0 (off).");
>
> +/* Flag indicating that the remote is up and running */
>  #define REMOTE_IS_READY				BIT(0)
> +/* Flag indicating that the host should wait for a firmware-ready response */
> +#define WAIT_FW_READY				BIT(1)
>  #define REMOTE_READY_WAIT_MAX_RETRIES		500
>
> +/* This flag is set in the DSP resource table's features field to indicate
> + * that the firmware requires the host NOT to wait for a FW_READY response.
> + */

multi line comments should be
/*
 * This ..
 */
> +#define FEATURE_DONT_WAIT_FW_READY		BIT(0)
> +
>  /* att flags */
>  /* DSP own area */
>  #define ATT_OWN					BIT(31)
> @@ -72,6 +80,10 @@ MODULE_PARM_DESC(no_mailboxes,
>
>  #define IMX8ULP_SIP_HIFI_XRDC			0xc200000e
>
> +#define FW_RSC_NXP_S_MAGIC			((uint32_t)'n' << 24 |	\
> +						 (uint32_t)'x' << 16 |	\
> +						 (uint32_t)'p' << 8 |	\
> +						 (uint32_t)'s')
>  /*
>   * enum - Predefined Mailbox Messages
>   *
> @@ -136,6 +148,24 @@ struct imx_dsp_rproc_dcfg {
>  	int (*reset)(struct imx_dsp_rproc *priv);
>  };
>
> +/**
> + * struct fw_rsc_imx_dsp - i.MX DSP specific info
> + *
> + * @len: length of the resource entry
> + * @magic_num: 32-bit magic number
> + * @version: version of data structure
> + * @features: feature flags supported by the i.MX DSP firmware
> + *
> + * This represents a DSP-specific resource in the firmware's
> + * resource table, providing information on supported features.
> + */
> +struct fw_rsc_imx_dsp {
> +	uint32_t len;
> +	uint32_t magic_num;
> +	uint32_t version;
> +	uint32_t features;
> +} __packed;
> +
>  static const struct imx_rproc_att imx_dsp_rproc_att_imx8qm[] = {
>  	/* dev addr , sys addr  , size	    , flags */
>  	{ 0x596e8000, 0x556e8000, 0x00008000, ATT_OWN },
> @@ -300,6 +330,73 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
>  	return -ETIMEDOUT;
>  }
>
> +/**
> + * imx_dsp_rproc_handle_rsc() - Handle DSP-specific resource table entries
> + * @rproc: remote processor instance
> + * @rsc_type: resource type identifier
> + * @rsc: pointer to the resource entry
> + * @offset: offset of the resource entry
> + * @avail: available space in the resource table
> + *
> + * Parse the DSP-specific resource entry and update flags accordingly.
> + * If the WAIT_FW_READY feature is set, the host must wait for the firmware
> + * to signal readiness before proceeding with execution.
> + *
> + * Return: RSC_HANDLED if processed successfully, RSC_IGNORED otherwise.
> + */
> +static int imx_dsp_rproc_handle_rsc(struct rproc *rproc, u32 rsc_type,
> +				    void *rsc, int offset, int avail)
> +{
> +	struct imx_dsp_rproc *priv = rproc->priv;
> +	struct fw_rsc_imx_dsp *imx_dsp_rsc = rsc;
> +	struct device *dev = rproc->dev.parent;
> +	size_t expected_size;
> +

put
	priv->flags |= WAIT_FW_READY;

here,

all "goto ignored" can be replace with "return RSC_IGNORED"

> +	if (!imx_dsp_rsc) {
> +		dev_dbg(dev, "Invalid fw_rsc_imx_dsp.\n");
> +		goto ignored;
> +	}
> +
> +	/* Make sure resource isn't truncated */
> +	expected_size = imx_dsp_rsc->len + sizeof(imx_dsp_rsc->len);
> +	if (expected_size < sizeof(struct fw_rsc_imx_dsp)) {
> +		dev_dbg(dev, "Resource fw_rsc_imx_dsp is truncated.\n");
> +		goto ignored;
> +	}
> +
> +	/*
> +	 * If FW_RSC_NXP_S_MAGIC number is not found then
> +	 * wait for fw_ready reply (default work flow)
> +	 */
> +	if (imx_dsp_rsc->magic_num != FW_RSC_NXP_S_MAGIC) {
> +		dev_dbg(dev, "Invalid resource table magic number.\n");
> +		goto ignored;
> +	}
> +
> +	/*
> +	 * For now, in struct fw_rsc_imx_dsp, version 0,
> +	 * only FEATURE_DONT_WAIT_FW_READY is valid.
> +	 *
> +	 * When adding new features, please upgrade version.
> +	 */
> +	if (imx_dsp_rsc->version > 0) {
> +		dev_warn(dev, "Unexpected fw_rsc_imx_dsp version %d.\n",
> +			 imx_dsp_rsc->version);
> +		goto ignored;
> +	}
> +
> +	if (imx_dsp_rsc->features & FEATURE_DONT_WAIT_FW_READY)
> +		priv->flags &= ~WAIT_FW_READY;
> +	else
> +		priv->flags |= WAIT_FW_READY;

if set WAIT_FW_READY at beginning, else branch can be removed.

Frank
> +
> +	return RSC_HANDLED;
> +
> +ignored:
> +	priv->flags |= WAIT_FW_READY;
> +	return RSC_IGNORED;
> +}
> +
>  /*
>   * Start function for rproc_ops
>   *
> @@ -335,8 +432,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 (priv->flags & WAIT_FW_READY)
> +		return imx_dsp_rproc_ready(rproc);
>
>  	return ret;
>  }
> @@ -936,6 +1033,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
>  	.kick		= imx_dsp_rproc_kick,
>  	.load		= imx_dsp_rproc_elf_load_segments,
>  	.parse_fw	= imx_dsp_rproc_parse_fw,
> +	.handle_rsc	= imx_dsp_rproc_handle_rsc,
>  	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>  	.sanity_check	= rproc_elf_sanity_check,
>  	.get_boot_addr	= rproc_elf_get_boot_addr,
> --
> 2.25.1
>

Re: [PATCH v3] remoteproc: imx_dsp_rproc: Add support for DSP-specific features

[Iuliana Prodan]

Hi Frank,

On 4/3/2025 10:12 PM, Frank Li wrote:
> On Thu, Apr 03, 2025 at 01:01:24PM +0300, Iuliana Prodan (OSS) wrote:
>> From: Iuliana Prodan <iuliana.prodan@nxp.com>
> subject: remoteproc: imx_dsp_rproc: add handle_rsc callback to handle DSP-specific features
>
>> Some DSP firmware requires a FW_READY signal before proceeding, while
>> others do not.
>> Therefore, add support to handle i.MX DSP-specific features.
> Add support to handle i.MX DSP-specific features because Some DSP firmware
> requires a FW_READY signal before proceeding
>
>> Implement handle_rsc callback to handle resource table parsing and to
>> process DSP-specific resource, to determine if waiting is needed.
>
> Implement the handle_rsc callback to parse the resource table and process
> DSP-specific resources to determine if waiting is needed.
>
>> Update imx_dsp_rproc_start() to handle this condition accordingly.
>>
>> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
>> ---
>> Changes in v3:
>> - Reviews from Mathieu Poirier:
>>    - Added version and magic number to vendor-specific resource table entry.
>>    - Updated defines to maintain backward compatibility with a resource table that doesn't have a vendor-specific resource.
>>      - By default, wait for `fw_ready`, unless specified otherwise.
>> - Link to v2: https://lore.kernel.org/all/20250318215007.2109726-1-iuliana.prodan@oss.nxp.com
>>
>> Changes in v2:
>> - Reviews from Mathieu Poirier:
>>    - Use vendor-specific resource table entry.
>>    - Implement resource handler specific to the i.MX DSP.
>> - Revise commit message to include recent updates.
>> - Link to v1: https://lore.kernel.org/all/20250305123923.514386-1-iuliana.prodan@oss.nxp.com/
>>
>>   drivers/remoteproc/imx_dsp_rproc.c | 102 ++++++++++++++++++++++++++++-
>>   1 file changed, 100 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
>> index b9bb15970966..80d4470cc731 100644
>> --- a/drivers/remoteproc/imx_dsp_rproc.c
>> +++ b/drivers/remoteproc/imx_dsp_rproc.c
>> @@ -35,9 +35,17 @@ module_param_named(no_mailboxes, no_mailboxes, int, 0644);
>>   MODULE_PARM_DESC(no_mailboxes,
>>   		 "There is no mailbox between cores, so ignore remote proc reply after start, default is 0 (off).");
>>
>> +/* Flag indicating that the remote is up and running */
>>   #define REMOTE_IS_READY				BIT(0)
>> +/* Flag indicating that the host should wait for a firmware-ready response */
>> +#define WAIT_FW_READY				BIT(1)
>>   #define REMOTE_READY_WAIT_MAX_RETRIES		500
>>
>> +/* This flag is set in the DSP resource table's features field to indicate
>> + * that the firmware requires the host NOT to wait for a FW_READY response.
>> + */
> multi line comments should be
> /*
>   * This ..
>   */
>> +#define FEATURE_DONT_WAIT_FW_READY		BIT(0)
>> +
>>   /* att flags */
>>   /* DSP own area */
>>   #define ATT_OWN					BIT(31)
>> @@ -72,6 +80,10 @@ MODULE_PARM_DESC(no_mailboxes,
>>
>>   #define IMX8ULP_SIP_HIFI_XRDC			0xc200000e
>>
>> +#define FW_RSC_NXP_S_MAGIC			((uint32_t)'n' << 24 |	\
>> +						 (uint32_t)'x' << 16 |	\
>> +						 (uint32_t)'p' << 8 |	\
>> +						 (uint32_t)'s')
>>   /*
>>    * enum - Predefined Mailbox Messages
>>    *
>> @@ -136,6 +148,24 @@ struct imx_dsp_rproc_dcfg {
>>   	int (*reset)(struct imx_dsp_rproc *priv);
>>   };
>>
>> +/**
>> + * struct fw_rsc_imx_dsp - i.MX DSP specific info
>> + *
>> + * @len: length of the resource entry
>> + * @magic_num: 32-bit magic number
>> + * @version: version of data structure
>> + * @features: feature flags supported by the i.MX DSP firmware
>> + *
>> + * This represents a DSP-specific resource in the firmware's
>> + * resource table, providing information on supported features.
>> + */
>> +struct fw_rsc_imx_dsp {
>> +	uint32_t len;
>> +	uint32_t magic_num;
>> +	uint32_t version;
>> +	uint32_t features;
>> +} __packed;
>> +
>>   static const struct imx_rproc_att imx_dsp_rproc_att_imx8qm[] = {
>>   	/* dev addr , sys addr  , size	    , flags */
>>   	{ 0x596e8000, 0x556e8000, 0x00008000, ATT_OWN },
>> @@ -300,6 +330,73 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
>>   	return -ETIMEDOUT;
>>   }
>>
>> +/**
>> + * imx_dsp_rproc_handle_rsc() - Handle DSP-specific resource table entries
>> + * @rproc: remote processor instance
>> + * @rsc_type: resource type identifier
>> + * @rsc: pointer to the resource entry
>> + * @offset: offset of the resource entry
>> + * @avail: available space in the resource table
>> + *
>> + * Parse the DSP-specific resource entry and update flags accordingly.
>> + * If the WAIT_FW_READY feature is set, the host must wait for the firmware
>> + * to signal readiness before proceeding with execution.
>> + *
>> + * Return: RSC_HANDLED if processed successfully, RSC_IGNORED otherwise.
>> + */
>> +static int imx_dsp_rproc_handle_rsc(struct rproc *rproc, u32 rsc_type,
>> +				    void *rsc, int offset, int avail)
>> +{
>> +	struct imx_dsp_rproc *priv = rproc->priv;
>> +	struct fw_rsc_imx_dsp *imx_dsp_rsc = rsc;
>> +	struct device *dev = rproc->dev.parent;
>> +	size_t expected_size;
>> +
> put
> 	priv->flags |= WAIT_FW_READY;
>
> here,
>
> all "goto ignored" can be replace with "return RSC_IGNORED"

Thanks for reviewing.

I'll hold off for a couple of days before sending  v4.

Iulia

>> +	if (!imx_dsp_rsc) {
>> +		dev_dbg(dev, "Invalid fw_rsc_imx_dsp.\n");
>> +		goto ignored;
>> +	}
>> +
>> +	/* Make sure resource isn't truncated */
>> +	expected_size = imx_dsp_rsc->len + sizeof(imx_dsp_rsc->len);
>> +	if (expected_size < sizeof(struct fw_rsc_imx_dsp)) {
>> +		dev_dbg(dev, "Resource fw_rsc_imx_dsp is truncated.\n");
>> +		goto ignored;
>> +	}
>> +
>> +	/*
>> +	 * If FW_RSC_NXP_S_MAGIC number is not found then
>> +	 * wait for fw_ready reply (default work flow)
>> +	 */
>> +	if (imx_dsp_rsc->magic_num != FW_RSC_NXP_S_MAGIC) {
>> +		dev_dbg(dev, "Invalid resource table magic number.\n");
>> +		goto ignored;
>> +	}
>> +
>> +	/*
>> +	 * For now, in struct fw_rsc_imx_dsp, version 0,
>> +	 * only FEATURE_DONT_WAIT_FW_READY is valid.
>> +	 *
>> +	 * When adding new features, please upgrade version.
>> +	 */
>> +	if (imx_dsp_rsc->version > 0) {
>> +		dev_warn(dev, "Unexpected fw_rsc_imx_dsp version %d.\n",
>> +			 imx_dsp_rsc->version);
>> +		goto ignored;
>> +	}
>> +
>> +	if (imx_dsp_rsc->features & FEATURE_DONT_WAIT_FW_READY)
>> +		priv->flags &= ~WAIT_FW_READY;
>> +	else
>> +		priv->flags |= WAIT_FW_READY;
> if set WAIT_FW_READY at beginning, else branch can be removed.
>
> Frank
>> +
>> +	return RSC_HANDLED;
>> +
>> +ignored:
>> +	priv->flags |= WAIT_FW_READY;
>> +	return RSC_IGNORED;
>> +}
>> +
>>   /*
>>    * Start function for rproc_ops
>>    *
>> @@ -335,8 +432,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 (priv->flags & WAIT_FW_READY)
>> +		return imx_dsp_rproc_ready(rproc);
>>
>>   	return ret;
>>   }
>> @@ -936,6 +1033,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
>>   	.kick		= imx_dsp_rproc_kick,
>>   	.load		= imx_dsp_rproc_elf_load_segments,
>>   	.parse_fw	= imx_dsp_rproc_parse_fw,
>> +	.handle_rsc	= imx_dsp_rproc_handle_rsc,
>>   	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>>   	.sanity_check	= rproc_elf_sanity_check,
>>   	.get_boot_addr	= rproc_elf_get_boot_addr,
>> --
>> 2.25.1
>>

Re: [PATCH v3] remoteproc: imx_dsp_rproc: Add support for DSP-specific features

[Mathieu Poirier]

Good morning,

On Thu, Apr 03, 2025 at 01:01:24PM +0300, Iuliana Prodan (OSS) wrote:
> From: Iuliana Prodan <iuliana.prodan@nxp.com>
> 
> Some DSP firmware requires a FW_READY signal before proceeding, while
> others do not.
> Therefore, add support to handle i.MX DSP-specific features.
> 
> Implement handle_rsc callback to handle resource table parsing and to
> process DSP-specific resource, to determine if waiting is needed.
> 
> Update imx_dsp_rproc_start() to handle this condition accordingly.
> 
> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
> ---
> Changes in v3:
> - Reviews from Mathieu Poirier:
>   - Added version and magic number to vendor-specific resource table entry.
>   - Updated defines to maintain backward compatibility with a resource table that doesn't have a vendor-specific resource.
>     - By default, wait for `fw_ready`, unless specified otherwise.
> - Link to v2: https://lore.kernel.org/all/20250318215007.2109726-1-iuliana.prodan@oss.nxp.com
> 
> Changes in v2:
> - Reviews from Mathieu Poirier:
>   - Use vendor-specific resource table entry.
>   - Implement resource handler specific to the i.MX DSP.
> - Revise commit message to include recent updates.
> - Link to v1: https://lore.kernel.org/all/20250305123923.514386-1-iuliana.prodan@oss.nxp.com/
> 
>  drivers/remoteproc/imx_dsp_rproc.c | 102 ++++++++++++++++++++++++++++-
>  1 file changed, 100 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index b9bb15970966..80d4470cc731 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -35,9 +35,17 @@ module_param_named(no_mailboxes, no_mailboxes, int, 0644);
>  MODULE_PARM_DESC(no_mailboxes,
>  		 "There is no mailbox between cores, so ignore remote proc reply after start, default is 0 (off).");
>  
> +/* Flag indicating that the remote is up and running */
>  #define REMOTE_IS_READY				BIT(0)
> +/* Flag indicating that the host should wait for a firmware-ready response */
> +#define WAIT_FW_READY				BIT(1)
>  #define REMOTE_READY_WAIT_MAX_RETRIES		500
>  
> +/* This flag is set in the DSP resource table's features field to indicate
> + * that the firmware requires the host NOT to wait for a FW_READY response.
> + */
> +#define FEATURE_DONT_WAIT_FW_READY		BIT(0)
> +
>  /* att flags */
>  /* DSP own area */
>  #define ATT_OWN					BIT(31)
> @@ -72,6 +80,10 @@ MODULE_PARM_DESC(no_mailboxes,
>  
>  #define IMX8ULP_SIP_HIFI_XRDC			0xc200000e
>  
> +#define FW_RSC_NXP_S_MAGIC			((uint32_t)'n' << 24 |	\
> +						 (uint32_t)'x' << 16 |	\
> +						 (uint32_t)'p' << 8 |	\
> +						 (uint32_t)'s')
>  /*
>   * enum - Predefined Mailbox Messages
>   *
> @@ -136,6 +148,24 @@ struct imx_dsp_rproc_dcfg {
>  	int (*reset)(struct imx_dsp_rproc *priv);
>  };
>  
> +/**
> + * struct fw_rsc_imx_dsp - i.MX DSP specific info
> + *
> + * @len: length of the resource entry
> + * @magic_num: 32-bit magic number
> + * @version: version of data structure
> + * @features: feature flags supported by the i.MX DSP firmware
> + *
> + * This represents a DSP-specific resource in the firmware's
> + * resource table, providing information on supported features.
> + */
> +struct fw_rsc_imx_dsp {
> +	uint32_t len;
> +	uint32_t magic_num;
> +	uint32_t version;
> +	uint32_t features;
> +} __packed;
> +
>  static const struct imx_rproc_att imx_dsp_rproc_att_imx8qm[] = {
>  	/* dev addr , sys addr  , size	    , flags */
>  	{ 0x596e8000, 0x556e8000, 0x00008000, ATT_OWN },
> @@ -300,6 +330,73 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
>  	return -ETIMEDOUT;
>  }
>  
> +/**
> + * imx_dsp_rproc_handle_rsc() - Handle DSP-specific resource table entries
> + * @rproc: remote processor instance
> + * @rsc_type: resource type identifier
> + * @rsc: pointer to the resource entry
> + * @offset: offset of the resource entry
> + * @avail: available space in the resource table
> + *
> + * Parse the DSP-specific resource entry and update flags accordingly.
> + * If the WAIT_FW_READY feature is set, the host must wait for the firmware
> + * to signal readiness before proceeding with execution.
> + *
> + * Return: RSC_HANDLED if processed successfully, RSC_IGNORED otherwise.
> + */
> +static int imx_dsp_rproc_handle_rsc(struct rproc *rproc, u32 rsc_type,
> +				    void *rsc, int offset, int avail)
> +{
> +	struct imx_dsp_rproc *priv = rproc->priv;
> +	struct fw_rsc_imx_dsp *imx_dsp_rsc = rsc;
> +	struct device *dev = rproc->dev.parent;
> +	size_t expected_size;
> +
> +	if (!imx_dsp_rsc) {
> +		dev_dbg(dev, "Invalid fw_rsc_imx_dsp.\n");
> +		goto ignored;
> +	}
> +
> +	/* Make sure resource isn't truncated */
> +	expected_size = imx_dsp_rsc->len + sizeof(imx_dsp_rsc->len);

Something seems odd with this check... I don't see how adding
imx_dsp_rsc->len with 4 will give us any indication of the expected size.  To me
two checks are required here:

1) if (sizeof(*rsc) > avail)

2) if (sizeof(*rsc) != imx_dsp_rsc->len)

Otherwise I'm good with this new revision.

Thanks,
Mathieu

> +	if (expected_size < sizeof(struct fw_rsc_imx_dsp)) {
> +		dev_dbg(dev, "Resource fw_rsc_imx_dsp is truncated.\n");
> +		goto ignored;
> +	}
> +
> +	/*
> +	 * If FW_RSC_NXP_S_MAGIC number is not found then
> +	 * wait for fw_ready reply (default work flow)
> +	 */
> +	if (imx_dsp_rsc->magic_num != FW_RSC_NXP_S_MAGIC) {
> +		dev_dbg(dev, "Invalid resource table magic number.\n");
> +		goto ignored;
> +	}
> +
> +	/*
> +	 * For now, in struct fw_rsc_imx_dsp, version 0,
> +	 * only FEATURE_DONT_WAIT_FW_READY is valid.
> +	 *
> +	 * When adding new features, please upgrade version.
> +	 */
> +	if (imx_dsp_rsc->version > 0) {
> +		dev_warn(dev, "Unexpected fw_rsc_imx_dsp version %d.\n",
> +			 imx_dsp_rsc->version);
> +		goto ignored;
> +	}
> +
> +	if (imx_dsp_rsc->features & FEATURE_DONT_WAIT_FW_READY)
> +		priv->flags &= ~WAIT_FW_READY;
> +	else
> +		priv->flags |= WAIT_FW_READY;
> +
> +	return RSC_HANDLED;
> +
> +ignored:
> +	priv->flags |= WAIT_FW_READY;
> +	return RSC_IGNORED;
> +}
> +
>  /*
>   * Start function for rproc_ops
>   *
> @@ -335,8 +432,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 (priv->flags & WAIT_FW_READY)
> +		return imx_dsp_rproc_ready(rproc);
>  
>  	return ret;
>  }
> @@ -936,6 +1033,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
>  	.kick		= imx_dsp_rproc_kick,
>  	.load		= imx_dsp_rproc_elf_load_segments,
>  	.parse_fw	= imx_dsp_rproc_parse_fw,
> +	.handle_rsc	= imx_dsp_rproc_handle_rsc,
>  	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>  	.sanity_check	= rproc_elf_sanity_check,
>  	.get_boot_addr	= rproc_elf_get_boot_addr,
> -- 
> 2.25.1
> 

Re: [PATCH v3] remoteproc: imx_dsp_rproc: Add support for DSP-specific features

[Iuliana Prodan]

Hello Mathieu,

On 4/7/2025 7:17 PM, Mathieu Poirier wrote:
> Good morning,
>
> On Thu, Apr 03, 2025 at 01:01:24PM +0300, Iuliana Prodan (OSS) wrote:
>> From: Iuliana Prodan <iuliana.prodan@nxp.com>
>>
>> Some DSP firmware requires a FW_READY signal before proceeding, while
>> others do not.
>> Therefore, add support to handle i.MX DSP-specific features.
>>
>> Implement handle_rsc callback to handle resource table parsing and to
>> process DSP-specific resource, to determine if waiting is needed.
>>
>> Update imx_dsp_rproc_start() to handle this condition accordingly.
>>
>> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
>> ---
>> Changes in v3:
>> - Reviews from Mathieu Poirier:
>>    - Added version and magic number to vendor-specific resource table entry.
>>    - Updated defines to maintain backward compatibility with a resource table that doesn't have a vendor-specific resource.
>>      - By default, wait for `fw_ready`, unless specified otherwise.
>> - Link to v2: https://lore.kernel.org/all/20250318215007.2109726-1-iuliana.prodan@oss.nxp.com
>>
>> Changes in v2:
>> - Reviews from Mathieu Poirier:
>>    - Use vendor-specific resource table entry.
>>    - Implement resource handler specific to the i.MX DSP.
>> - Revise commit message to include recent updates.
>> - Link to v1: https://lore.kernel.org/all/20250305123923.514386-1-iuliana.prodan@oss.nxp.com/
>>
>>   drivers/remoteproc/imx_dsp_rproc.c | 102 ++++++++++++++++++++++++++++-
>>   1 file changed, 100 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
>> index b9bb15970966..80d4470cc731 100644
>> --- a/drivers/remoteproc/imx_dsp_rproc.c
>> +++ b/drivers/remoteproc/imx_dsp_rproc.c
>> @@ -35,9 +35,17 @@ module_param_named(no_mailboxes, no_mailboxes, int, 0644);
>>   MODULE_PARM_DESC(no_mailboxes,
>>   		 "There is no mailbox between cores, so ignore remote proc reply after start, default is 0 (off).");
>>   
>> +/* Flag indicating that the remote is up and running */
>>   #define REMOTE_IS_READY				BIT(0)
>> +/* Flag indicating that the host should wait for a firmware-ready response */
>> +#define WAIT_FW_READY				BIT(1)
>>   #define REMOTE_READY_WAIT_MAX_RETRIES		500
>>   
>> +/* This flag is set in the DSP resource table's features field to indicate
>> + * that the firmware requires the host NOT to wait for a FW_READY response.
>> + */
>> +#define FEATURE_DONT_WAIT_FW_READY		BIT(0)
>> +
>>   /* att flags */
>>   /* DSP own area */
>>   #define ATT_OWN					BIT(31)
>> @@ -72,6 +80,10 @@ MODULE_PARM_DESC(no_mailboxes,
>>   
>>   #define IMX8ULP_SIP_HIFI_XRDC			0xc200000e
>>   
>> +#define FW_RSC_NXP_S_MAGIC			((uint32_t)'n' << 24 |	\
>> +						 (uint32_t)'x' << 16 |	\
>> +						 (uint32_t)'p' << 8 |	\
>> +						 (uint32_t)'s')
>>   /*
>>    * enum - Predefined Mailbox Messages
>>    *
>> @@ -136,6 +148,24 @@ struct imx_dsp_rproc_dcfg {
>>   	int (*reset)(struct imx_dsp_rproc *priv);
>>   };
>>   
>> +/**
>> + * struct fw_rsc_imx_dsp - i.MX DSP specific info
>> + *
>> + * @len: length of the resource entry
>> + * @magic_num: 32-bit magic number
>> + * @version: version of data structure
>> + * @features: feature flags supported by the i.MX DSP firmware
>> + *
>> + * This represents a DSP-specific resource in the firmware's
>> + * resource table, providing information on supported features.
>> + */
>> +struct fw_rsc_imx_dsp {
>> +	uint32_t len;
>> +	uint32_t magic_num;
>> +	uint32_t version;
>> +	uint32_t features;
>> +} __packed;
>> +
>>   static const struct imx_rproc_att imx_dsp_rproc_att_imx8qm[] = {
>>   	/* dev addr , sys addr  , size	    , flags */
>>   	{ 0x596e8000, 0x556e8000, 0x00008000, ATT_OWN },
>> @@ -300,6 +330,73 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
>>   	return -ETIMEDOUT;
>>   }
>>   
>> +/**
>> + * imx_dsp_rproc_handle_rsc() - Handle DSP-specific resource table entries
>> + * @rproc: remote processor instance
>> + * @rsc_type: resource type identifier
>> + * @rsc: pointer to the resource entry
>> + * @offset: offset of the resource entry
>> + * @avail: available space in the resource table
>> + *
>> + * Parse the DSP-specific resource entry and update flags accordingly.
>> + * If the WAIT_FW_READY feature is set, the host must wait for the firmware
>> + * to signal readiness before proceeding with execution.
>> + *
>> + * Return: RSC_HANDLED if processed successfully, RSC_IGNORED otherwise.
>> + */
>> +static int imx_dsp_rproc_handle_rsc(struct rproc *rproc, u32 rsc_type,
>> +				    void *rsc, int offset, int avail)
>> +{
>> +	struct imx_dsp_rproc *priv = rproc->priv;
>> +	struct fw_rsc_imx_dsp *imx_dsp_rsc = rsc;
>> +	struct device *dev = rproc->dev.parent;
>> +	size_t expected_size;
>> +
>> +	if (!imx_dsp_rsc) {
>> +		dev_dbg(dev, "Invalid fw_rsc_imx_dsp.\n");
>> +		goto ignored;
>> +	}
>> +
>> +	/* Make sure resource isn't truncated */
>> +	expected_size = imx_dsp_rsc->len + sizeof(imx_dsp_rsc->len);
> Something seems odd with this check... I don't see how adding
> imx_dsp_rsc->len with 4 will give us any indication of the expected size.
The fw_rsc_imx_dsp structure is based on Zephyr and OpenAMP ([1]).

The imx_dsp_rsc->len indicates the available resource size. Adding 4 
bytes (for uint32_t len member) gives the total structure size. If this 
does not match sizeof(struct fw_rsc_imx_dsp), the structure is incomplete.

I will also verify with avail and send a v4.

[1] 
https://github.com/OpenAMP/open-amp/blob/main/lib/include/openamp/remoteproc.h#L356

Thanks,
Iulia
>   To me
> two checks are required here:
>
> 1) if (sizeof(*rsc) > avail)
>
> 2) if (sizeof(*rsc) != imx_dsp_rsc->len)
>
> Otherwise I'm good with this new revision.
>
> Thanks,
> Mathieu
>
>> +	if (expected_size < sizeof(struct fw_rsc_imx_dsp)) {
>> +		dev_dbg(dev, "Resource fw_rsc_imx_dsp is truncated.\n");
>> +		goto ignored;
>> +	}
>> +
>> +	/*
>> +	 * If FW_RSC_NXP_S_MAGIC number is not found then
>> +	 * wait for fw_ready reply (default work flow)
>> +	 */
>> +	if (imx_dsp_rsc->magic_num != FW_RSC_NXP_S_MAGIC) {
>> +		dev_dbg(dev, "Invalid resource table magic number.\n");
>> +		goto ignored;
>> +	}
>> +
>> +	/*
>> +	 * For now, in struct fw_rsc_imx_dsp, version 0,
>> +	 * only FEATURE_DONT_WAIT_FW_READY is valid.
>> +	 *
>> +	 * When adding new features, please upgrade version.
>> +	 */
>> +	if (imx_dsp_rsc->version > 0) {
>> +		dev_warn(dev, "Unexpected fw_rsc_imx_dsp version %d.\n",
>> +			 imx_dsp_rsc->version);
>> +		goto ignored;
>> +	}
>> +
>> +	if (imx_dsp_rsc->features & FEATURE_DONT_WAIT_FW_READY)
>> +		priv->flags &= ~WAIT_FW_READY;
>> +	else
>> +		priv->flags |= WAIT_FW_READY;
>> +
>> +	return RSC_HANDLED;
>> +
>> +ignored:
>> +	priv->flags |= WAIT_FW_READY;
>> +	return RSC_IGNORED;
>> +}
>> +
>>   /*
>>    * Start function for rproc_ops
>>    *
>> @@ -335,8 +432,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 (priv->flags & WAIT_FW_READY)
>> +		return imx_dsp_rproc_ready(rproc);
>>   
>>   	return ret;
>>   }
>> @@ -936,6 +1033,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
>>   	.kick		= imx_dsp_rproc_kick,
>>   	.load		= imx_dsp_rproc_elf_load_segments,
>>   	.parse_fw	= imx_dsp_rproc_parse_fw,
>> +	.handle_rsc	= imx_dsp_rproc_handle_rsc,
>>   	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>>   	.sanity_check	= rproc_elf_sanity_check,
>>   	.get_boot_addr	= rproc_elf_get_boot_addr,
>> -- 
>> 2.25.1
>>

Re: [PATCH v3] remoteproc: imx_dsp_rproc: Add support for DSP-specific features

[Mathieu Poirier]

On Tue, 8 Apr 2025 at 02:47, Iuliana Prodan <iuliana.prodan@nxp.com> wrote:
>
> Hello Mathieu,
>
> On 4/7/2025 7:17 PM, Mathieu Poirier wrote:
> > Good morning,
> >
> > On Thu, Apr 03, 2025 at 01:01:24PM +0300, Iuliana Prodan (OSS) wrote:
> >> From: Iuliana Prodan <iuliana.prodan@nxp.com>
> >>
> >> Some DSP firmware requires a FW_READY signal before proceeding, while
> >> others do not.
> >> Therefore, add support to handle i.MX DSP-specific features.
> >>
> >> Implement handle_rsc callback to handle resource table parsing and to
> >> process DSP-specific resource, to determine if waiting is needed.
> >>
> >> Update imx_dsp_rproc_start() to handle this condition accordingly.
> >>
> >> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
> >> ---
> >> Changes in v3:
> >> - Reviews from Mathieu Poirier:
> >>    - Added version and magic number to vendor-specific resource table entry.
> >>    - Updated defines to maintain backward compatibility with a resource table that doesn't have a vendor-specific resource.
> >>      - By default, wait for `fw_ready`, unless specified otherwise.
> >> - Link to v2: https://lore.kernel.org/all/20250318215007.2109726-1-iuliana.prodan@oss.nxp.com
> >>
> >> Changes in v2:
> >> - Reviews from Mathieu Poirier:
> >>    - Use vendor-specific resource table entry.
> >>    - Implement resource handler specific to the i.MX DSP.
> >> - Revise commit message to include recent updates.
> >> - Link to v1: https://lore.kernel.org/all/20250305123923.514386-1-iuliana.prodan@oss.nxp.com/
> >>
> >>   drivers/remoteproc/imx_dsp_rproc.c | 102 ++++++++++++++++++++++++++++-
> >>   1 file changed, 100 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> >> index b9bb15970966..80d4470cc731 100644
> >> --- a/drivers/remoteproc/imx_dsp_rproc.c
> >> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> >> @@ -35,9 +35,17 @@ module_param_named(no_mailboxes, no_mailboxes, int, 0644);
> >>   MODULE_PARM_DESC(no_mailboxes,
> >>               "There is no mailbox between cores, so ignore remote proc reply after start, default is 0 (off).");
> >>
> >> +/* Flag indicating that the remote is up and running */
> >>   #define REMOTE_IS_READY                            BIT(0)
> >> +/* Flag indicating that the host should wait for a firmware-ready response */
> >> +#define WAIT_FW_READY                               BIT(1)
> >>   #define REMOTE_READY_WAIT_MAX_RETRIES              500
> >>
> >> +/* This flag is set in the DSP resource table's features field to indicate
> >> + * that the firmware requires the host NOT to wait for a FW_READY response.
> >> + */
> >> +#define FEATURE_DONT_WAIT_FW_READY          BIT(0)
> >> +
> >>   /* att flags */
> >>   /* DSP own area */
> >>   #define ATT_OWN                                    BIT(31)
> >> @@ -72,6 +80,10 @@ MODULE_PARM_DESC(no_mailboxes,
> >>
> >>   #define IMX8ULP_SIP_HIFI_XRDC                      0xc200000e
> >>
> >> +#define FW_RSC_NXP_S_MAGIC                  ((uint32_t)'n' << 24 |  \
> >> +                                             (uint32_t)'x' << 16 |  \
> >> +                                             (uint32_t)'p' << 8 |   \
> >> +                                             (uint32_t)'s')
> >>   /*
> >>    * enum - Predefined Mailbox Messages
> >>    *
> >> @@ -136,6 +148,24 @@ struct imx_dsp_rproc_dcfg {
> >>      int (*reset)(struct imx_dsp_rproc *priv);
> >>   };
> >>
> >> +/**
> >> + * struct fw_rsc_imx_dsp - i.MX DSP specific info
> >> + *
> >> + * @len: length of the resource entry
> >> + * @magic_num: 32-bit magic number
> >> + * @version: version of data structure
> >> + * @features: feature flags supported by the i.MX DSP firmware
> >> + *
> >> + * This represents a DSP-specific resource in the firmware's
> >> + * resource table, providing information on supported features.
> >> + */
> >> +struct fw_rsc_imx_dsp {
> >> +    uint32_t len;
> >> +    uint32_t magic_num;
> >> +    uint32_t version;
> >> +    uint32_t features;
> >> +} __packed;
> >> +
> >>   static const struct imx_rproc_att imx_dsp_rproc_att_imx8qm[] = {
> >>      /* dev addr , sys addr  , size      , flags */
> >>      { 0x596e8000, 0x556e8000, 0x00008000, ATT_OWN },
> >> @@ -300,6 +330,73 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
> >>      return -ETIMEDOUT;
> >>   }
> >>
> >> +/**
> >> + * imx_dsp_rproc_handle_rsc() - Handle DSP-specific resource table entries
> >> + * @rproc: remote processor instance
> >> + * @rsc_type: resource type identifier
> >> + * @rsc: pointer to the resource entry
> >> + * @offset: offset of the resource entry
> >> + * @avail: available space in the resource table
> >> + *
> >> + * Parse the DSP-specific resource entry and update flags accordingly.
> >> + * If the WAIT_FW_READY feature is set, the host must wait for the firmware
> >> + * to signal readiness before proceeding with execution.
> >> + *
> >> + * Return: RSC_HANDLED if processed successfully, RSC_IGNORED otherwise.
> >> + */
> >> +static int imx_dsp_rproc_handle_rsc(struct rproc *rproc, u32 rsc_type,
> >> +                                void *rsc, int offset, int avail)
> >> +{
> >> +    struct imx_dsp_rproc *priv = rproc->priv;
> >> +    struct fw_rsc_imx_dsp *imx_dsp_rsc = rsc;
> >> +    struct device *dev = rproc->dev.parent;
> >> +    size_t expected_size;
> >> +
> >> +    if (!imx_dsp_rsc) {
> >> +            dev_dbg(dev, "Invalid fw_rsc_imx_dsp.\n");
> >> +            goto ignored;
> >> +    }
> >> +
> >> +    /* Make sure resource isn't truncated */
> >> +    expected_size = imx_dsp_rsc->len + sizeof(imx_dsp_rsc->len);
> > Something seems odd with this check... I don't see how adding
> > imx_dsp_rsc->len with 4 will give us any indication of the expected size.
> The fw_rsc_imx_dsp structure is based on Zephyr and OpenAMP ([1]).
>
> The imx_dsp_rsc->len indicates the available resource size. Adding 4
> bytes (for uint32_t len member) gives the total structure size. If this
> does not match sizeof(struct fw_rsc_imx_dsp), the structure is incomplete.
>

Ok, but why is adding 4 to imx_dsp_rsc->len needed?  Why doesn't
imx_dsp_rsc->len already contain the right size?

The size of struct fw_rsc_imx_dsp is 16 byte.  From your
implementation, it seems like ->len is equal to 12 and 4 needs to be
added to make it 16.  To me ->len should be 16... What am I missing?

> I will also verify with avail and send a v4.
>
> [1]
> https://github.com/OpenAMP/open-amp/blob/main/lib/include/openamp/remoteproc.h#L356
>
> Thanks,
> Iulia
> >   To me
> > two checks are required here:
> >
> > 1) if (sizeof(*rsc) > avail)
> >
> > 2) if (sizeof(*rsc) != imx_dsp_rsc->len)
> >
> > Otherwise I'm good with this new revision.
> >
> > Thanks,
> > Mathieu
> >
> >> +    if (expected_size < sizeof(struct fw_rsc_imx_dsp)) {
> >> +            dev_dbg(dev, "Resource fw_rsc_imx_dsp is truncated.\n");
> >> +            goto ignored;
> >> +    }
> >> +
> >> +    /*
> >> +     * If FW_RSC_NXP_S_MAGIC number is not found then
> >> +     * wait for fw_ready reply (default work flow)
> >> +     */
> >> +    if (imx_dsp_rsc->magic_num != FW_RSC_NXP_S_MAGIC) {
> >> +            dev_dbg(dev, "Invalid resource table magic number.\n");
> >> +            goto ignored;
> >> +    }
> >> +
> >> +    /*
> >> +     * For now, in struct fw_rsc_imx_dsp, version 0,
> >> +     * only FEATURE_DONT_WAIT_FW_READY is valid.
> >> +     *
> >> +     * When adding new features, please upgrade version.
> >> +     */
> >> +    if (imx_dsp_rsc->version > 0) {
> >> +            dev_warn(dev, "Unexpected fw_rsc_imx_dsp version %d.\n",
> >> +                     imx_dsp_rsc->version);
> >> +            goto ignored;
> >> +    }
> >> +
> >> +    if (imx_dsp_rsc->features & FEATURE_DONT_WAIT_FW_READY)
> >> +            priv->flags &= ~WAIT_FW_READY;
> >> +    else
> >> +            priv->flags |= WAIT_FW_READY;
> >> +
> >> +    return RSC_HANDLED;
> >> +
> >> +ignored:
> >> +    priv->flags |= WAIT_FW_READY;
> >> +    return RSC_IGNORED;
> >> +}
> >> +
> >>   /*
> >>    * Start function for rproc_ops
> >>    *
> >> @@ -335,8 +432,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 (priv->flags & WAIT_FW_READY)
> >> +            return imx_dsp_rproc_ready(rproc);
> >>
> >>      return ret;
> >>   }
> >> @@ -936,6 +1033,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
> >>      .kick           = imx_dsp_rproc_kick,
> >>      .load           = imx_dsp_rproc_elf_load_segments,
> >>      .parse_fw       = imx_dsp_rproc_parse_fw,
> >> +    .handle_rsc     = imx_dsp_rproc_handle_rsc,
> >>      .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> >>      .sanity_check   = rproc_elf_sanity_check,
> >>      .get_boot_addr  = rproc_elf_get_boot_addr,
> >> --
> >> 2.25.1
> >>

Re: [PATCH v3] remoteproc: imx_dsp_rproc: Add support for DSP-specific features

[Iuliana Prodan]

On 4/8/2025 4:46 PM, Mathieu Poirier wrote:
> On Tue, 8 Apr 2025 at 02:47, Iuliana Prodan <iuliana.prodan@nxp.com> wrote:
>> Hello Mathieu,
>>
>> On 4/7/2025 7:17 PM, Mathieu Poirier wrote:
>>> Good morning,
>>>
>>> On Thu, Apr 03, 2025 at 01:01:24PM +0300, Iuliana Prodan (OSS) wrote:
>>>> From: Iuliana Prodan <iuliana.prodan@nxp.com>
>>>>
>>>> Some DSP firmware requires a FW_READY signal before proceeding, while
>>>> others do not.
>>>> Therefore, add support to handle i.MX DSP-specific features.
>>>>
>>>> Implement handle_rsc callback to handle resource table parsing and to
>>>> process DSP-specific resource, to determine if waiting is needed.
>>>>
>>>> Update imx_dsp_rproc_start() to handle this condition accordingly.
>>>>
>>>> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
>>>> ---
>>>> Changes in v3:
>>>> - Reviews from Mathieu Poirier:
>>>>     - Added version and magic number to vendor-specific resource table entry.
>>>>     - Updated defines to maintain backward compatibility with a resource table that doesn't have a vendor-specific resource.
>>>>       - By default, wait for `fw_ready`, unless specified otherwise.
>>>> - Link to v2: https://lore.kernel.org/all/20250318215007.2109726-1-iuliana.prodan@oss.nxp.com
>>>>
>>>> Changes in v2:
>>>> - Reviews from Mathieu Poirier:
>>>>     - Use vendor-specific resource table entry.
>>>>     - Implement resource handler specific to the i.MX DSP.
>>>> - Revise commit message to include recent updates.
>>>> - Link to v1: https://lore.kernel.org/all/20250305123923.514386-1-iuliana.prodan@oss.nxp.com/
>>>>
>>>>    drivers/remoteproc/imx_dsp_rproc.c | 102 ++++++++++++++++++++++++++++-
>>>>    1 file changed, 100 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
>>>> index b9bb15970966..80d4470cc731 100644
>>>> --- a/drivers/remoteproc/imx_dsp_rproc.c
>>>> +++ b/drivers/remoteproc/imx_dsp_rproc.c
>>>> @@ -35,9 +35,17 @@ module_param_named(no_mailboxes, no_mailboxes, int, 0644);
>>>>    MODULE_PARM_DESC(no_mailboxes,
>>>>                "There is no mailbox between cores, so ignore remote proc reply after start, default is 0 (off).");
>>>>
>>>> +/* Flag indicating that the remote is up and running */
>>>>    #define REMOTE_IS_READY                            BIT(0)
>>>> +/* Flag indicating that the host should wait for a firmware-ready response */
>>>> +#define WAIT_FW_READY                               BIT(1)
>>>>    #define REMOTE_READY_WAIT_MAX_RETRIES              500
>>>>
>>>> +/* This flag is set in the DSP resource table's features field to indicate
>>>> + * that the firmware requires the host NOT to wait for a FW_READY response.
>>>> + */
>>>> +#define FEATURE_DONT_WAIT_FW_READY          BIT(0)
>>>> +
>>>>    /* att flags */
>>>>    /* DSP own area */
>>>>    #define ATT_OWN                                    BIT(31)
>>>> @@ -72,6 +80,10 @@ MODULE_PARM_DESC(no_mailboxes,
>>>>
>>>>    #define IMX8ULP_SIP_HIFI_XRDC                      0xc200000e
>>>>
>>>> +#define FW_RSC_NXP_S_MAGIC                  ((uint32_t)'n' << 24 |  \
>>>> +                                             (uint32_t)'x' << 16 |  \
>>>> +                                             (uint32_t)'p' << 8 |   \
>>>> +                                             (uint32_t)'s')
>>>>    /*
>>>>     * enum - Predefined Mailbox Messages
>>>>     *
>>>> @@ -136,6 +148,24 @@ struct imx_dsp_rproc_dcfg {
>>>>       int (*reset)(struct imx_dsp_rproc *priv);
>>>>    };
>>>>
>>>> +/**
>>>> + * struct fw_rsc_imx_dsp - i.MX DSP specific info
>>>> + *
>>>> + * @len: length of the resource entry
>>>> + * @magic_num: 32-bit magic number
>>>> + * @version: version of data structure
>>>> + * @features: feature flags supported by the i.MX DSP firmware
>>>> + *
>>>> + * This represents a DSP-specific resource in the firmware's
>>>> + * resource table, providing information on supported features.
>>>> + */
>>>> +struct fw_rsc_imx_dsp {
>>>> +    uint32_t len;
>>>> +    uint32_t magic_num;
>>>> +    uint32_t version;
>>>> +    uint32_t features;
>>>> +} __packed;
>>>> +
>>>>    static const struct imx_rproc_att imx_dsp_rproc_att_imx8qm[] = {
>>>>       /* dev addr , sys addr  , size      , flags */
>>>>       { 0x596e8000, 0x556e8000, 0x00008000, ATT_OWN },
>>>> @@ -300,6 +330,73 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
>>>>       return -ETIMEDOUT;
>>>>    }
>>>>
>>>> +/**
>>>> + * imx_dsp_rproc_handle_rsc() - Handle DSP-specific resource table entries
>>>> + * @rproc: remote processor instance
>>>> + * @rsc_type: resource type identifier
>>>> + * @rsc: pointer to the resource entry
>>>> + * @offset: offset of the resource entry
>>>> + * @avail: available space in the resource table
>>>> + *
>>>> + * Parse the DSP-specific resource entry and update flags accordingly.
>>>> + * If the WAIT_FW_READY feature is set, the host must wait for the firmware
>>>> + * to signal readiness before proceeding with execution.
>>>> + *
>>>> + * Return: RSC_HANDLED if processed successfully, RSC_IGNORED otherwise.
>>>> + */
>>>> +static int imx_dsp_rproc_handle_rsc(struct rproc *rproc, u32 rsc_type,
>>>> +                                void *rsc, int offset, int avail)
>>>> +{
>>>> +    struct imx_dsp_rproc *priv = rproc->priv;
>>>> +    struct fw_rsc_imx_dsp *imx_dsp_rsc = rsc;
>>>> +    struct device *dev = rproc->dev.parent;
>>>> +    size_t expected_size;
>>>> +
>>>> +    if (!imx_dsp_rsc) {
>>>> +            dev_dbg(dev, "Invalid fw_rsc_imx_dsp.\n");
>>>> +            goto ignored;
>>>> +    }
>>>> +
>>>> +    /* Make sure resource isn't truncated */
>>>> +    expected_size = imx_dsp_rsc->len + sizeof(imx_dsp_rsc->len);
>>> Something seems odd with this check... I don't see how adding
>>> imx_dsp_rsc->len with 4 will give us any indication of the expected size.
>> The fw_rsc_imx_dsp structure is based on Zephyr and OpenAMP ([1]).
>>
>> The imx_dsp_rsc->len indicates the available resource size. Adding 4
>> bytes (for uint32_t len member) gives the total structure size. If this
>> does not match sizeof(struct fw_rsc_imx_dsp), the structure is incomplete.
>>
> Ok, but why is adding 4 to imx_dsp_rsc->len needed?  Why doesn't
> imx_dsp_rsc->len already contain the right size?
>
> The size of struct fw_rsc_imx_dsp is 16 byte.  From your
> implementation, it seems like ->len is equal to 12 and 4 needs to be
> added to make it 16.  To me ->len should be 16... What am I missing?

You're correct. In my implementation, len is 12, which represents the 
available
bytes for writing additional data such as the magic number, version,
and supported features. This is based on my understanding of the len member
from the OpenAMP [1] and how I applied it in Zephyr.


I will update the code to define len as the size of the struct 
fw_rsc_imx_dsp.

Thanks,
Iulia

[1] 
https://github.com/OpenAMP/open-amp/blob/main/lib/include/openamp/remoteproc.h#L356


>
>> I will also verify with avail and send a v4.
>>
>> [1]
>> https://github.com/OpenAMP/open-amp/blob/main/lib/include/openamp/remoteproc.h#L356
>>
>> Thanks,
>> Iulia
>>>    To me
>>> two checks are required here:
>>>
>>> 1) if (sizeof(*rsc) > avail)
>>>
>>> 2) if (sizeof(*rsc) != imx_dsp_rsc->len)
>>>
>>> Otherwise I'm good with this new revision.
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>> +    if (expected_size < sizeof(struct fw_rsc_imx_dsp)) {
>>>> +            dev_dbg(dev, "Resource fw_rsc_imx_dsp is truncated.\n");
>>>> +            goto ignored;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * If FW_RSC_NXP_S_MAGIC number is not found then
>>>> +     * wait for fw_ready reply (default work flow)
>>>> +     */
>>>> +    if (imx_dsp_rsc->magic_num != FW_RSC_NXP_S_MAGIC) {
>>>> +            dev_dbg(dev, "Invalid resource table magic number.\n");
>>>> +            goto ignored;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * For now, in struct fw_rsc_imx_dsp, version 0,
>>>> +     * only FEATURE_DONT_WAIT_FW_READY is valid.
>>>> +     *
>>>> +     * When adding new features, please upgrade version.
>>>> +     */
>>>> +    if (imx_dsp_rsc->version > 0) {
>>>> +            dev_warn(dev, "Unexpected fw_rsc_imx_dsp version %d.\n",
>>>> +                     imx_dsp_rsc->version);
>>>> +            goto ignored;
>>>> +    }
>>>> +
>>>> +    if (imx_dsp_rsc->features & FEATURE_DONT_WAIT_FW_READY)
>>>> +            priv->flags &= ~WAIT_FW_READY;
>>>> +    else
>>>> +            priv->flags |= WAIT_FW_READY;
>>>> +
>>>> +    return RSC_HANDLED;
>>>> +
>>>> +ignored:
>>>> +    priv->flags |= WAIT_FW_READY;
>>>> +    return RSC_IGNORED;
>>>> +}
>>>> +
>>>>    /*
>>>>     * Start function for rproc_ops
>>>>     *
>>>> @@ -335,8 +432,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 (priv->flags & WAIT_FW_READY)
>>>> +            return imx_dsp_rproc_ready(rproc);
>>>>
>>>>       return ret;
>>>>    }
>>>> @@ -936,6 +1033,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
>>>>       .kick           = imx_dsp_rproc_kick,
>>>>       .load           = imx_dsp_rproc_elf_load_segments,
>>>>       .parse_fw       = imx_dsp_rproc_parse_fw,
>>>> +    .handle_rsc     = imx_dsp_rproc_handle_rsc,
>>>>       .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>>>>       .sanity_check   = rproc_elf_sanity_check,
>>>>       .get_boot_addr  = rproc_elf_get_boot_addr,
>>>> --
>>>> 2.25.1
>>>>