Re: [PATCH v4] remoteproc: imx_dsp_rproc: Add support for DSP-specific features
From: Iuliana Prodan
Date: Tue Apr 15 2025 - 07:05:17 EST
On 4/14/2025 6:41 PM, Mathieu Poirier wrote:
On Thu, Apr 10, 2025 at 12:30:30AM +0300, Iuliana Prodan (OSS) wrote:
From: Iuliana Prodan <iuliana.prodan@xxxxxxx>
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@xxxxxxx>
---
Changes in v4:
- Reviews from Mathieu Poirier:
- Adjusted len to include the size of struct fw_rsc_imx_dsp.
- Updated len validation checks.
- Review from Frank Li:
- In imx_dsp_rproc_handle_rsc(), removed the goto ignored statement.
- In probe(), set flags to WAIT_FW_READY to ensure the host waits
for fw_ready when no vendor-specific resource is defined.
- Link to v3: https://lore.kernel.org/all/20250403100124.637889-1-iuliana.prodan@xxxxxxxxxxx/
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@xxxxxxxxxxx
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@xxxxxxxxxxx/
drivers/remoteproc/imx_dsp_rproc.c | 98 +++++++++++++++++++++++++++++-
1 file changed, 96 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index b9bb15970966..e4212e624a91 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -35,9 +35,18 @@ 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 +81,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 +149,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 +331,66 @@ 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;
+
+ if (!imx_dsp_rsc) {
+ dev_dbg(dev, "Invalid fw_rsc_imx_dsp.\n");
+ return RSC_IGNORED;
+ }
+
+ /* Make sure resource isn't truncated */
+ if (sizeof(struct fw_rsc_imx_dsp) > avail ||
We agree on that part.
+ sizeof(struct fw_rsc_imx_dsp) < imx_dsp_rsc->len) {
From the above, "sizeof(struct fw_rsc_imx_dsp) > imx_dsp_rsc->len" would be a
valid condition when it clearly isn't. I am still convinced the only
valid option is:
sizeof(struct fw_rsc_imx_dsp) != imx_dsp_rsc->len)
I am happy to change my mind but would need more information.
You're right, it should be sizeof(struct fw_rsc_imx_dsp) > imx_dsp_rsc->len.
The ->len comes from the remote size, while in Linux we need to check if
the length is at least sizeof(struct fw_rsc_imx_dsp).
This is for backwards compatibility - if someone changes the structure
on the remote side and increases the length, in Linux we can still load
that firmware, but probably not all features will be checked.
If you agree with this, I'll send a v5 with this fix - s/</>.
Thanks,
Iulia
Thanks,
Mathieu
+ dev_dbg(dev, "Resource fw_rsc_imx_dsp is truncated.\n");
+ return RSC_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");
+ return RSC_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);
+ return RSC_IGNORED;
+ }
+
+ if (imx_dsp_rsc->features & FEATURE_DONT_WAIT_FW_READY)
+ priv->flags &= ~WAIT_FW_READY;
+
+ return RSC_HANDLED;
+}
+
/*
* Start function for rproc_ops
*
@@ -335,8 +426,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 +1027,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,
@@ -1053,6 +1145,8 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
priv = rproc->priv;
priv->rproc = rproc;
priv->dsp_dcfg = dsp_dcfg;
+ /* By default, host waits for fw_ready reply */
+ priv->flags |= WAIT_FW_READY;
if (no_mailboxes)
imx_dsp_rproc_mbox_init = imx_dsp_rproc_mbox_no_alloc;
--
2.25.1