On Tue, 8 Apr 2025 at 02:47, Iuliana Prodan <iuliana.pro...@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.pro...@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.pro...@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.pro...@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.pro...@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 > >>
