Re: [PATCH v3 05/14] remoteproc: Refactor function rproc_fw_boot()

On Fri 08 May 14:27 PDT 2020, Mathieu Poirier wrote:

On Tue, May 05, 2020 at 05:33:41PM -0700, Bjorn Andersson wrote:

quoted

On Fri 24 Apr 13:01 PDT 2020, Mathieu Poirier wrote:

quoted

Refactor function rproc_fw_boot() in order to better reflect the work
that is done when supporting scenarios where the remoteproc core is
synchronising with a remote processor.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index a02593b75bec..e90a21de9de1 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c

@@ -1370,9 +1370,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 }
 
 /*
- * take a firmware and boot a remote processor with it.
+ * boot or synchronise with a remote processor.
  */
-static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
+static int rproc_actuate_device(struct rproc *rproc, const struct firmware *fw)

Per patch 4 this function will if rproc_needs_syncing() be called with
fw == NULL, it's not obvious to me that the various operations on "fw"
in this function are valid anymore.

That is right, all firmware related operations in this function are found in
remoteproc_internal.h where the value of rproc->sync_with_mcu is checked before
moving forward. That allows us to avoid introducing a new function similar to
rproc_fw_boot() but without firmware operations or peppering the code with if
statements.

As I wrote in my other reply, the two mechanisms seems to consist of the
following steps:

boot the core:
1) request firmware
2) prepare device
3) parse fw
4) handle resources
5) allocate carveouts
6) load segments
7) find resource table
8) prepare subdevices
9) power on
10) start subdevices

sync:
1) prepare device (?)
2) handle resources
3) allocate carveouts (?)
4) prepare subdevices
5) attach
6) start subdevices

Rather than relying on the state flag and missing ops will turn the
first list into the second list I conceptually prefer having two
separate functions that are easy to reason about.

But I haven't done any refactoring or implemented this, so in practice
the two might just be a lot of duplication(?)

quoted

quoted

 {
 	struct device *dev = &rproc->dev;
 	const char *name = rproc->firmware;

@@ -1382,7 +1382,9 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	if (ret)
 		return ret;
 
-	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
+	if (!rproc_needs_syncing(rproc))

Can't we make this check on fw, to make the relationship "if we where
passed a firmware object, we're going to load and boot that firmware"?

It can but I specifically decided to use rproc_needs_syncing() to be consistent
with the rest of the patchset.  That way all we need to do is grep for
rproc_needs_syncing to get all the places where a decision about synchronising
with a remote processor is made.

Conceptually we have a single "to sync or not to sync", but I think
we're invoking rproc_needs_syncing() 8 times during rproc_fw_boot() and
each of those operations may or may not do anything.

There are certain operations where I see it makes sense for a driver to
either implement or not, but I think that e.g. for a rproc in OFFLINE
state we should just require ops->start to be specified - because it
doesn't make sense to enter rproc_start() if ops->start is a nop.

Regards,
Bjorn

quoted

Regards,
Bjorn

quoted

+		dev_info(dev, "Booting fw image %s, size %zd\n",
+			 name, fw->size);
 
 	/*
 	 * if enabling an IOMMU isn't relevant for this rproc, this is

@@ -1818,7 +1820,7 @@ int rproc_boot(struct rproc *rproc)
 		}
 	}
 
-	ret = rproc_fw_boot(rproc, firmware_p);
+	ret = rproc_actuate_device(rproc, firmware_p);
 
 	release_firmware(firmware_p);
 

-- 
2.20.1