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

[Iuliana Prodan]

On 7/18/2023 6:48 PM, Mathieu Poirier wrote:
> On Tue, Jul 18, 2023 at 11:30:43AM +0300, Iuliana Prodan wrote:
>> Hi Mathieu,
>>
>> On 7/17/2023 8:34 PM, Mathieu Poirier wrote:
>>> Hi Iuliana,
>>>
>>> On Thu, Jul 13, 2023 at 01:42:51AM +0300, Iuliana Prodan (OSS) wrote:
>>>> From: Iuliana Prodan <iuliana.prodan at nxp.com>
>>>>
>>>> There are cases when we want to test samples that do not
>>>> reply with FW READY message, after fw is loaded and the
>>>> remote processor started.
>>> This seems like a bug to me - where is this FW comes from?
>> The firmware is a generic sample from Zephyr repo: https://github.com/zephyrproject-rtos/zephyr/tree/main/samples/subsys/ipc/openamp_rsc_table
>>
>> There is no bug, this is how the application was written.
> But how did it ever worked before?

It never worked on this kind of samples (and it was never tested like this).
We used only applications written by us (NXP) with the 
requirements/limitations we know we have.
Now, we want to use also generic firmware/samples (from Zephyr) and we 
face other kind of limitations.

>   And how does having a module flag to
> characterize each FW implementation that springs up in the field can scale (and
> be maintainable)?

I believe the FW_READY reply is a limitation introduced by imx_dsp_rproc.
We cannot expect all firmware to give a FW_READY reply.
So, to keep both usecases (internal firmware and generic sample) I added 
this flag.

>> Rather than modifying a generic sample for i.MX usecase, I prefer doing an
>> "insmod imx_dsp_rproc.ko ignore_dsp_ready=1" just for this sample.
> We already have a "no_mailbox" flag for cases where the FW doesn't need to
> communicate with the main processor.
"no_mailbox" - no IPC between cores;
"ignore_dsp_ready" - we have IPC between cores, but the remote processor 
doesn't send a fw_ready reply
These two can be combine, but for "no_mailbox" will do some useless 
memory allocations.

When I added the "no_mailbox" flag, the problem was still FW_READY.
>   What happens when some FW implementation
> requires a three-way handshake?  How many flags do we spin off?
>
> As I said above this approach is not sustainable.  I suggest to either fix the
> FW (it doesn't work with upstream in its present form anyway) or start using the
> config space as described here [1] to dynamically probe the characteristics of
> the FW being loaded.  Whichever option you chose, the FW needs to be updated and
> the former is a lot more simple.
I don't think I can modify a generic sample, used on other targets to 
send a FW_READY reply.
How will it be handled on other platforms, if their *_rproc are not 
expecting this kind of message?

Thanks,
Iulia

> Thanks,
> Mathieu
>
> [1]. https://elixir.bootlin.com/linux/latest/source/include/linux/remoteproc.h#L298
>
>> Thanks,
>> Iulia
>>
>>>> In these cases, do not wait for a confirmation from the remote processor
>>>> at 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 at nxp.com>
>>>>
>>>> ---
>>>> This was discovered while testing openamp_rsc_table sample from Zephyr
>>>> repo (https://github.com/zephyrproject-rtos/zephyr/tree/main/samples/subsys/ipc/openamp_rsc_table).
>>>>
>>>> We have IPC, but the remote proc doesn't send a FW_READY reply.
>>>> ---
>>>>    drivers/remoteproc/imx_dsp_rproc.c | 15 +++++++++++++++
>>>>    1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
>>>> index b5634507d953..ed89de2f3b98 100644
>>>> --- a/drivers/remoteproc/imx_dsp_rproc.c
>>>> +++ b/drivers/remoteproc/imx_dsp_rproc.c
>>>> @@ -36,7 +36,13 @@ 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).");
>>>> +static unsigned int imx_dsp_rproc_ignore_ready;
>>>> +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 REMOTE_IS_READY				BIT(0)
>>>> +#define REMOTE_IGNORE_READY_REPLY	BIT(1)
>>>>    #define REMOTE_READY_WAIT_MAX_RETRIES		500
>>>>    /* att flags */
>>>> @@ -296,6 +302,12 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
>>>>    	if (!priv->rxdb_ch)
>>>>    		return 0;
>>>> +	/*
>>>> +	 * FW_READY reply is optional/ignored, so don't wait for it.
>>>> +	 */
>>>> +	if (priv->flags & REMOTE_IGNORE_READY_REPLY)
>>>> +		return 0;
>>>> +
>>>>    	for (i = 0; i < REMOTE_READY_WAIT_MAX_RETRIES; i++) {
>>>>    		if (priv->flags & REMOTE_IS_READY)
>>>>    			return 0;
>>>> @@ -1119,6 +1131,9 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
>>>>    	else
>>>>    		imx_dsp_rproc_mbox_init = imx_dsp_rproc_mbox_alloc;
>>>> +	if (imx_dsp_rproc_ignore_ready)
>>>> +		priv->flags |= REMOTE_IGNORE_READY_REPLY;
>>>> +
>>>>    	dev_set_drvdata(dev, rproc);
>>>>    	INIT_WORK(&priv->rproc_work, imx_dsp_rproc_vq_work);
>>>> -- 
>>>> 2.17.1
>>>>