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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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

> On Tue, May 05, 2020 at 05:33:41PM -0700, Bjorn Andersson wrote:
> > On Fri 24 Apr 13:01 PDT 2020, Mathieu Poirier wrote:
> > 
> > > 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(?)

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

> > 
> > Regards,
> > Bjorn
> > 
> > > +		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
> > >