From: Iuliana Prodan <[email protected]>
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.
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 <[email protected]>
---
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
On Thu, Jul 13, 2023 at 2:15 AM Iuliana Prodan (OSS)
<[email protected]> wrote:
>
> From: Iuliana Prodan <[email protected]>
>
> 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.
> 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 <[email protected]>
Reviewed-by: Daniel Baluta <[email protected]>
Hi Iuliana,
On Thu, Jul 13, 2023 at 01:42:51AM +0300, Iuliana Prodan (OSS) wrote:
> From: Iuliana Prodan <[email protected]>
>
> 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?
> 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 <[email protected]>
>
> ---
> 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
>
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 <[email protected]>
>>
>> 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.
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.
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 <[email protected]>
>>
>> ---
>> This was discovered while testing openamp_rsc_table sample from Zephyr
>> repo (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fzephyrproject-rtos%2Fzephyr%2Ftree%2Fmain%2Fsamples%2Fsubsys%2Fipc%2Fopenamp_rsc_table&data=05%7C01%7Ciuliana.prodan%40nxp.com%7C4779cb20393e4af08a9408db86ec191e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638252120814415013%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=iCjvv8wr3sQ4CEXFcXDsW0VSw5RXr1ASw7LN2J08SXE%3D&reserved=0).
>>
>> 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
>>
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 <[email protected]>
> > >
> > > 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? And how does having a module flag to
characterize each FW implementation that springs up in the field can scale (and
be maintainable)?
>
> 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. 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.
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 <[email protected]>
> > >
> > > ---
> > > This was discovered while testing openamp_rsc_table sample from Zephyr
> > > repo (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fzephyrproject-rtos%2Fzephyr%2Ftree%2Fmain%2Fsamples%2Fsubsys%2Fipc%2Fopenamp_rsc_table&data=05%7C01%7Ciuliana.prodan%40nxp.com%7C4779cb20393e4af08a9408db86ec191e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638252120814415013%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=iCjvv8wr3sQ4CEXFcXDsW0VSw5RXr1ASw7LN2J08SXE%3D&reserved=0).
> > >
> > > 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
> > >
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>>
>>>> ---
>>>> 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
>>>>
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 <[email protected]>
> > > > >
> > > > > 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.
>
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 <[email protected]>
> > > > >
> > > > > ---
> > > > > 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
> > > > >
On 7/19/2023 6:47 PM, 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 <[email protected]>
>>>>>>
>>>>>> 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.
>>
> 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.
But why was this FW_READY added in the first place?
@S.J, can you, please, help here?
What is the use case for this custom message?
My proposal is to remove this reply.
This was added for a custom firmware/sample, that is not publicly
accessible - S.J, please correct me if I'm wrong.
Also, for imx_rproc (used for M4 or M7 secondary core) we don't have
this FW_READY reply.
>> 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 <[email protected]>
>>>>>>
>>>>>> ---
>>>>>> 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
>>>>>>