Received-SPF: pass (google.com: domain of linux-kernel+bounces-97317-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240301164227.339208-1-abdellatif.elkhlifi@arm.com>
 <20240301164227.339208-2-abdellatif.elkhlifi@arm.com> <ZeYWKVpeFm1+4mlT@p14s> <20240307194026.GA355455@e130802.arm.com>
In-Reply-To: <20240307194026.GA355455@e130802.arm.com>
From: Mathieu Poirier <mathieu.poirier@linaro.org>
Date: Fri, 8 Mar 2024 09:44:26 -0700
Message-ID: <CANLsYkzA20rQdTM6AOvFK=3o28GvcoRbckL=ri8RegHqyHaiCw@mail.gmail.com>
Subject: Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
To: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bjorn Andersson <andersson@kernel.org>, Rob Herring <robh+dt@kernel.org>, 
	Liviu Dudau <liviu.dudau@arm.com>, Sudeep Holla <sudeep.holla@arm.com>, 
	Lorenzo Pieralisi <lpieralisi@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Drew.Reed@arm.com, Adam.Johnston@arm.com, 
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-remoteproc@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

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.

> > > +/**
> > > + * 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.

>
> [1]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/remoteproc/remoteproc_core.c#L1947
> [2]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/base/firmware_loader/main.c#L863
>
> > > +module_platform_driver(arm_rproc_driver);
> > > +
> >
> > I am echoing Krzysztof view about how generic this driver name is.  This has to
> > be related to a family of processors or be made less generic in some way.  Have
> > a look at what TI did for their K3 lineup [1] - I would like to see the same
> > thing done here.
>
> Thank you, I'll take care of that and of all the other comments made.
>
> Cheers,
> Abdellatif