lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6c47b7c3-fc4b-43c3-ac4c-f739cc739343@nxp.com>
Date: Mon, 7 Apr 2025 14:16:32 +0300
From: Iuliana Prodan <iuliana.prodan@....com>
To: Frank Li <Frank.li@....com>
Cc: Mathieu Poirier <mathieu.poirier@...aro.org>,
 Bjorn Andersson <andersson@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>, "S.J. Wang" <shengjiu.wang@....com>,
 Fabio Estevam <festevam@...il.com>, Daniel Baluta <daniel.baluta@....com>,
 Mpuaudiosw <Mpuaudiosw@....com>, imx@...ts.linux.dev,
 linux-remoteproc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, Pengutronix Kernel Team
 <kernel@...gutronix.de>, "Iuliana Prodan (OSS)" <iuliana.prodan@....nxp.com>
Subject: Re: [PATCH v3] remoteproc: imx_dsp_rproc: Add support for
 DSP-specific features

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@....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@....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
>>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ