| Message ID | 20230712224251.26482-1-iuliana.prodan@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] remoteproc: imx_dsp_rproc: add mandatory find_loaded_rsc_table op | expand |
On Thu, Jul 13, 2023 at 2:15 AM Iuliana Prodan (OSS) <iuliana.prodan@oss.nxp.com> wrote: > > From: Iuliana Prodan <iuliana.prodan@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. > 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@nxp.com> Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Hi Iuliana, On Thu, Jul 13, 2023 at 01:42:51AM +0300, Iuliana Prodan (OSS) wrote: > From: Iuliana Prodan <iuliana.prodan@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? > 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@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 >
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@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. 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 <iuliana.prodan@nxp.com> >> >> --- >> 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 <iuliana.prodan@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? 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 <iuliana.prodan@nxp.com> > > > > > > --- > > > 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 <iuliana.prodan@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@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 >>>>
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@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. > 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@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 > > > > >
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 <iuliana.prodan@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. >> > 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 <iuliana.prodan@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 >>>>>>
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@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. 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@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 >>>>>>
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);