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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

  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@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




[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Photo Sharing]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux