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

[Arnaud POULIQUEN]

Hi Juliana

On 7/19/23 17:47, Mathieu Poirier wrote:
> On Tue, Jul 18, 2023 at 07:44:49PM +0300, Iuliana Prodan wrote:
>> 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@xxxxxxx>
>>>>>>
>>>>>> 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.

It makes sense to me to adapt the sample in Zephyr to add this synchronization
under a config flag (I am the author of this sample).

FYI, this sample is currently only running on the STM32MP15 boards for
communication with Linux. For these boards, there is no need for synchronization
before starting the IPC communication.
So extending the sample to support a second board will make it more generic :)

Regards,
Arnaud

>>
> 
> Right, we can't expect firmware written for a totally different OS to work out
> of the box on Linux, and vice versa.
> 
>>>   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.
>>
> 
> What happens when a third, fourth and fifth protocol variation get introduced?
> Adding flags just doesn't scale.
> 
>>>> 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?
>>
> 
> The only way forward is to come up with a standard specification to describe the
> protocol to use, the same way it is done for virtIO for example.
> 
>> 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@xxxxxxx>
>>>>>>
>>>>>> ---
>>>>>> 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
>>>>>>