Re: [PATCH v3] remoteproc: imx_dsp_rproc: add module parameter to ignore ready flag from remote processor

[Iuliana Prodan <iuliana.prodan@nxp.com>]

On 1/18/2023 7:24 PM, Mathieu Poirier wrote:
> Hi Iuliana,
>
> On Tue, Jan 17, 2023 at 01:03:57PM +0200, Iuliana Prodan (OSS) wrote:
>> From: Iuliana Prodan <iuliana.prodan@nxp.com>
>>
>> There are cases when we want to test a simple "hello world"
>> application on the DSP and we don't have IPC between the cores.
>> Therefore, skip the wait for remote processor to start.
>>
>> Added "ignore_dsp_ready" flag while inserting the module to ignore
>> remote processor reply after start.
>> By default, this is off - do not ignore reply from rproc.
>>
>> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
>>
>> ---
>> Changes since v2
>> - s/ignoreready/ignore_dsp_ready
>>
>> Changes since v1
>> - change BIT(31) to BIT(1) for REMOTE_SKIP_WAIT
>>
>> ---
>>   drivers/remoteproc/imx_dsp_rproc.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
>> index 95da1cbefacf..22e2ef068c67 100644
>> --- a/drivers/remoteproc/imx_dsp_rproc.c
>> +++ b/drivers/remoteproc/imx_dsp_rproc.c
>> @@ -26,9 +26,20 @@
>>   #include "remoteproc_elf_helpers.h"
>>   #include "remoteproc_internal.h"
>>   
>> +#define IMX_DSP_IGNORE_REMOTE_READY		0
>> +
>> +/*
>> + * Module parameters
>> + */
>> +static unsigned int imx_dsp_rproc_ignore_ready = IMX_DSP_IGNORE_REMOTE_READY;
> Static variables are initialised to '0' and as such this is not needed.
>
>> +module_param_named(ignore_dsp_ready, imx_dsp_rproc_ignore_ready, int, 0644);
>> +MODULE_PARM_DESC(ignore_dsp_ready,
>> +		 "Ignore remote proc reply after start, default is 0 (off).");
>> +
>>   #define DSP_RPROC_CLK_MAX			5
>>   
>>   #define REMOTE_IS_READY				BIT(0)
>> +#define REMOTE_SKIP_WAIT			BIT(1)
>>   #define REMOTE_READY_WAIT_MAX_RETRIES		500
>>   
>>   /* att flags */
>> @@ -285,6 +296,9 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
>>   	if (!priv->rxdb_ch)
>>   		return 0;
>>   
>> +	if (priv->flags & REMOTE_SKIP_WAIT)
>> +		return 0;
>> +
> This looks very hackish to me...
>
> Here priv->rxdb_ch is valid and as such the DB mailbox has been setup, which
> contradicts the commit log where it is stated that "we don't have IPC between
> cores".  Moreover, the commit log mentions to "skip the wait for remote
> processor to start".  How can the remote processor executed an sample
> application if it is not ready?
>
> Lastly, is there even a need to call imx_dsp_rproc_mbox_init() if an IPC is not
> needed?
>
> I'm fine with the module parameter but would much rather see a solution that
> does not configure any kind of IPC related mechanic when it is not needed.
>
> Thanks,
> Mathieu
Hi Mathieu,

I've tested this with hello_world sample from Zephyr.
This was loaded on a hifi4 core from Linux using remoteproc. And, with 
this patch is working, otherwise I get "can't start rproc imx-dsp-rproc: 
-110:" because the ARM core is waiting for a reply from the hifi4.

I agree, I shouldn't initialize mbox if there is no IPC between the cores.
I'll fix this in a v4.

Thanks,
Iulia

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel