RE: [PATCH v2 1/1] remoteproc: add support for co-processor booted before kernel

[Loic PALLARDY]

> -----Original Message-----
> From: linux-remoteproc-owner@xxxxxxxxxxxxxxx <linux-remoteproc-
> owner@xxxxxxxxxxxxxxx> On Behalf Of Bjorn Andersson
> Sent: mardi 12 novembre 2019 18:33
> To: Loic PALLARDY <loic.pallardy@xxxxxx>
> Cc: ohad@xxxxxxxxxx; linux-remoteproc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Arnaud POULIQUEN <arnaud.pouliquen@xxxxxx>;
> benjamin.gaignard@xxxxxxxxxx; Fabien DESSENNE
> <fabien.dessenne@xxxxxx>; s-anna@xxxxxx
> Subject: Re: [PATCH v2 1/1] remoteproc: add support for co-processor
> booted before kernel
> 
> On Tue 12 Nov 00:40 PST 2019, Loic PALLARDY wrote:
> 
> > Hi Bjorn,
> >
> > Thanks for your review.
> >
> > > -----Original Message-----
> > > From: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > > Sent: samedi 9 novembre 2019 02:20
> > > To: Loic PALLARDY <loic.pallardy@xxxxxx>
> > > Cc: ohad@xxxxxxxxxx; linux-remoteproc@xxxxxxxxxxxxxxx; linux-
> > > kernel@xxxxxxxxxxxxxxx; Arnaud POULIQUEN
> <arnaud.pouliquen@xxxxxx>;
> > > benjamin.gaignard@xxxxxxxxxx; Fabien DESSENNE
> > > <fabien.dessenne@xxxxxx>; s-anna@xxxxxx
> > > Subject: Re: [PATCH v2 1/1] remoteproc: add support for co-processor
> > > booted before kernel
> > >
> > > On Mon 04 Nov 02:49 PST 2019, Loic Pallardy wrote:
> > >
> > > > Remote processor could boot independently or be started before Linux
> > > > kernel by bootloader or any firmware.
> > > > This patch introduces a new property in rproc core, named preloaded,
> > > > to be able to allocate resources and sub-devices like vdev and to
> > > > synchronize with current state without loading firmware from file
> system.
> > > > It is platform driver responsibility to implement the right firmware
> > > > load ops according to HW specificities.
> > > >
> > >
> > > Is it just preloaded or already started?
> > At the beginning, this properties was indeed to support an already
> > started coprocessor, but discussing with Suman few months ago, we
> > detected some cases for which firmware may be loaded before kernel
> > start (firmware located in embedded flash or loaded by bootloaders) or
> > may be loaded by a dedicated driver interface before rproc framework
> > be called.
> > As rproc framework is mainly responsible for firmware loading and
> > resource allocation, I named this property preloaded to cover as much
> > as possible all cases.
> 
> Cool, then I like this patch :)
> 
> > >
> > > > Signed-off-by: Loic Pallardy <loic.pallardy@xxxxxx>
> > > >
> > > > ---
> > > > Change from v1:
> > > > - Keep bool in struct rproc
> > > > ---
> > > >  drivers/remoteproc/remoteproc_core.c | 37
> > > +++++++++++++++++++++++++++---------
> > > >  include/linux/remoteproc.h           |  2 ++
> > > >  2 files changed, 30 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/remoteproc_core.c
> > > b/drivers/remoteproc/remoteproc_core.c
> > > > index 3c5fbbbfb0f1..7eaf0f949afa 100644
> > > > --- a/drivers/remoteproc/remoteproc_core.c
> > > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > > @@ -1372,7 +1372,11 @@ 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 (fw)
> > > > +		dev_info(dev, "Booting fw image %s, size %zd\n", name,
> > > > +			 fw->size);
> > > > +	else
> > > > +		dev_info(dev, "Synchronizing with preloaded co-
> > > processor\n");
> > > >
> > > >  	/*
> > > >  	 * if enabling an IOMMU isn't relevant for this rproc, this is
> > > > @@ -1728,7 +1732,7 @@ static void rproc_crash_handler_work(struct
> > > work_struct *work)
> > > >   */
> > > >  int rproc_boot(struct rproc *rproc)
> > > >  {
> > > > -	const struct firmware *firmware_p;
> > > > +	const struct firmware *firmware_p = NULL;
> > > >  	struct device *dev;
> > > >  	int ret;
> > > >
> > > > @@ -1759,11 +1763,17 @@ int rproc_boot(struct rproc *rproc)
> > > >
> > > >  	dev_info(dev, "powering up %s\n", rproc->name);
> > > >
> > > > -	/* load firmware */
> > > > -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> > > > -	if (ret < 0) {
> > > > -		dev_err(dev, "request_firmware failed: %d\n", ret);
> > > > -		goto downref_rproc;
> > > > +	if (!rproc->preloaded) {
> > > > +		/* load firmware */
> > > > +		ret = request_firmware(&firmware_p, rproc->firmware,
> > > dev);
> > > > +		if (ret < 0) {
> > > > +			dev_err(dev, "request_firmware failed: %d\n", ret);
> > > > +			goto downref_rproc;
> > > > +		}
> > > > +	} else {
> > > > +		/* set firmware name to null as unknown */
> > > > +		kfree(rproc->firmware);
> > > > +		rproc->firmware = NULL;
> > >
> > > What happens when the remoteproc crashes? What happens if I stop it
> and
> > > try to start it again?
> > Exactly, it should be stopped, then firmware should be provided and
> > coprocessor restarted thanks to sysfs interface
> >
> 
> But in both these cases you rely on the firmware being persistent in
> some memory - be it some protected portion of RAM or some ROM?
> 
> I.e. with this patch, and not setting "firmware" the remote is expected
> to just boot again on the same firmware.

Today no, we need to reload a firmware, but indeed it is an option is we consider the code located in a protected memory.
I have another patch to clean-up recovery operation, for example generating a core dump without automatically restarting the coprocessor...
I'll rebase and share it too.

> 
> > >
> > > >  	}
> > > >
> > > >  	ret = rproc_fw_boot(rproc, firmware_p);
> > > > @@ -1917,8 +1927,17 @@ int rproc_add(struct rproc *rproc)
> > > >  	/* create debugfs entries */
> > > >  	rproc_create_debug_dir(rproc);
> > > >
> > > > -	/* if rproc is marked always-on, request it to boot */
> > > > -	if (rproc->auto_boot) {
> > > > +	if (rproc->preloaded) {
> > > > +		/*
> > > > +		 * If rproc is marked already booted, no need to wait
> > > > +		 * for firmware.
> > > > +		 * Just handle associated resources and start sub devices
> > > > +		 */
> > > > +		ret = rproc_boot(rproc);
> > >
> > > This will trickle down to your remoteproc driver's start() callback. If
> > > you really mean that "preloaded" means "already started", then I
> presume
> > > you're having some logic in your start() to turn it into a nop?
> > Yes it is the responsibility of the driver to know in which state coprocessor
> is.
> > Depending on the use case, driver could:
> > - do nothing if coprocessor already running
> > - boot coprocessor, if only firmware was preloaded
> > - reboot coprocessor to restart its execution on the preloaded firmware
> >
> 
> I'm slightly worried about the cases of finding an already booted
> coprocessor, as this often means that we have to make assumptions about
> clock states etc.

Yes that part is very platform driver specific and it will be driver responsibility to preserve or to update associated clocks.

Regards,
Loic

> 
> But with the focus of supporting persistent firmware, I like this.
> 
> > >
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +	} else if (rproc->auto_boot) {
> > > > +		/* if rproc is marked always-on, request it to boot */
> > > >  		ret = rproc_trigger_auto_boot(rproc);
> > > >  		if (ret < 0)
> > > >  			return ret;
> > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > > index 16ad66683ad0..b68fbd576a77 100644
> > > > --- a/include/linux/remoteproc.h
> > > > +++ b/include/linux/remoteproc.h
> > > > @@ -479,6 +479,7 @@ struct rproc_dump_segment {
> > > >   * @table_sz: size of @cached_table
> > > >   * @has_iommu: flag to indicate if remote processor is behind an MMU
> > > >   * @auto_boot: flag to indicate if remote processor should be auto-
> started
> > > > + * @preloaded: remote processor has been preloaded before start
> > > sequence
> > >
> > > I think this should be "skip_firmware_load", or if you really mean that
> > > the bootloader started the remote process perhaps this should be
> > > "@fw_booted: remote processor was booted by firmware" (or
> something
> > > similar).
> > I'm fine with "skip_firmware_load" or "skip_fw_load" to have a shorter
> name.
> > I'll also add "preload" in patch title.
> >
> 
> skip_fw_load sounds good.
> 
> Thanks,
> Bjorn
> 
> > Regards,
> > Loic
> >
> > >
> > > Regards,
> > > Bjorn
> > >
> > > >   * @dump_segments: list of segments in the firmware
> > > >   * @nb_vdev: number of vdev currently handled by rproc
> > > >   */
> > > > @@ -512,6 +513,7 @@ struct rproc {
> > > >  	size_t table_sz;
> > > >  	bool has_iommu;
> > > >  	bool auto_boot;
> > > > +	bool preloaded;
> > > >  	struct list_head dump_segments;
> > > >  	int nb_vdev;
> > > >  };
> > > > --
> > > > 2.7.4
> > > >