On Fri 15 May 12:46 PDT 2020, Mathieu Poirier wrote:

On Wed, May 13, 2020 at 07:10:55PM -0700, Bjorn Andersson wrote:

quoted

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

quoted

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.

I reflected long and hard about doing just that...

quoted

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

Exactly - duplication and maintenance are my prime concern.  Right now some
functions in the OFFLINE -> RUNNING are clearly not needed when dealing with a
DETACHED -> RUNNING scenarios, but with I am convinced people will find ways to
do something creative with the callbacks.

I'm sure there are problems out there that will require creative
solutions, but I would prefer that we keep things easy to reason about
and ensure that as new problems arise we can evolve the framework.

In the end I fear the new functions
we spin off to deal with DETACHED -> RUNNING scenarios will end up looking very
similar to the current implementation.

In those scenarios I don't see a problem with the platform drivers
having functions of common code shared between ops->start and
ops->attach.

With that in mind I simply did all the work in remoteproc_internal.h and left
the core functions intact.

We can try spinning off new functions in the next revision, just to test my
theory and see how much gets duplicated.

Looking forward to it!

quoted

quoted

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.

As I said above, I'll try spinning off new functions in the next revision.  From
there we can decide how best to move forward.

quoted

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.

At this time ops->start() doesn't have to be specified... But as you say it
won't do much good and this is something we can easily spot when reviewing
patches.

Presumably after implementing this support we should check during
registration that there's either a start or an attach ops specified. And
if there's no start we shouldn't allow the RUNNING->OFFLINE transition.

Thanks for the review,

Thanks for working on this and sorry that it took me time really digest
this.

Regards,
Bjorn

Mathieu

quoted

Regards,
Bjorn

quoted

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