Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver

[Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>]

Hi Mathieu,

On Fri, Mar 08, 2024 at 09:44:26AM -0700, Mathieu Poirier wrote:
> On Thu, 7 Mar 2024 at 12:40, Abdellatif El Khlifi
> <abdellatif.elkhlifi@arm.com> wrote:
> >
> > Hi Mathieu,
> >
> > > > +   do {
> > > > +           state_reg = readl(priv->reset_cfg.state_reg);
> > > > +           *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> > > > +
> > > > +           if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> > > > +                   dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> > > > +                           *rst_ack);
> > > > +                   return -EINVAL;
> > > > +           }
> > > > +
> > > > +           /* expected ACK value read */
> > > > +           if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
> > >
> > > I'm not sure why the second condition in this if() statement is needed.  As far
> > > as I can tell the first condition will trigger and the second one won't be
> > > reached.
> >
> > The second condition takes care of the following: exp_ack and  *rst_ack are both 0.
> > This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
> > we expect the RST_ACK to be 00 afterwards.
> >
> 
> This is the kind of conditions that definitely deserve documentation.
> Please split the conditions in two different if() statements and add a
> comment to explain what is going on.

Thanks, I'll address that.

> 
> > > > +/**
> > > > + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> > > > + * @rproc: pointer to the remote processor object
> > > > + * @fw: pointer to the firmware
> > > > + *
> > > > + * Does nothing currently.
> > > > + *
> > > > + * Return:
> > > > + *
> > > > + * 0 for success.
> > > > + */
> > > > +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
> > > > +{
> > >
> > > What is the point of doing rproc_of_parse_firmware() if the firmware image is
> > > not loaded to memory?  Does the remote processor have some kind of default ROM
> > > image to run if it doesn't find anything in memory?
> >
> > Yes, the remote processor has a default FW image already loaded by default.
> >
> 
> That too would have mandated a comment - otherwise people looking at
> the code are left wondering, as I did.
> 
> > rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename
> > provided.
> >
> > Please correct me if I'm wrong.
> 
> You are correct, the remoteproc subsystem expects a firmware image to
> be provided _and_ loaded into memory.  Providing a dummy image just to
> get the remote processor booted is a hack, but simply because the
> subsystem isn't tailored to handle this use case.  So I am left
> wondering what the plans are for this driver, i.e is this a real
> scenario that needs to be addressed or just an initial patchset to get
> a foundation for the driver.
> 
> In the former case we need to start talking about refactoring the
> subsystem so that it properly handles remote processors that don't
> need a firmware image.  In the latter case I'd rather see a patchset
> where the firmware image is loaded into RAM.

This is an initial patchset for allowing to turn on and off the remote processor.
The FW is already loaded before the Corstone-1000 SoC is powered on and this
is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
(emulator).

The plan for the driver is as follows:

Step 1: provide a foundation driver capable of turning the core on/off

Step 2: provide mailbox support for comms

Step 3: provide FW reload capability

Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can share memory with
the remote core.

I'm happy to provide more explanation in the commit log to reflect this status.

Is it OK that we go with step 1 as a foundation please ?

Cheers
Abdellatif

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel