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

From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>,
	Bjorn Andersson <andersson@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Liviu Dudau <liviu.dudau@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
Subject: Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
Date: Mon, 25 Mar 2024 17:13:39 +0000	[thread overview]
Message-ID: <20240325171339.GA368569@e130802.arm.com> (raw)
In-Reply-To: <CANLsYkzdfP8Np-XwPDt=GBNLYiSypd8tNdb29KUwr+tyi7gJEA@mail.gmail.com>

Hi Mathieu,

> > > > > > > > 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).
> > > > > > > >
> > > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > > >
> > > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > > on the SoC.
> > > > > >
> > > > >
> > > > > Ok
> > > > >
> > > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > > to share memory with the remote core and also being able to access the remote
> > > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > > of adding remoteproc support for Corstone.
> > > > > >
> > > > >
> > > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > > memory, preventing the A core from copying the firmware image to the proper
> > > > > location.
> > > > >
> > > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > > firmware image has been preloaded into memory?
> > > >
> > > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > > >
> > >
> > > Very well.  I am willing to continue with this driver but it does so little that
> > > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > > the HW is fixed.  The choice is yours.
> > >
> >
> > I think Robin has raised few points that need clarification. I think it was
> > done as part of DT binding patch. I share those concerns and I wanted to
> > reaching to the same concerns by starting the questions I asked on corstone
> > device tree changes.
> >
> 
> I also agree with Robin's point of view.  Proceeding with an initial
> driver with minimal functionality doesn't preclude having complete
> bindings.  But that said and as I pointed out, it might be better to
> wait for the HW to be fixed before moving forward.

We checked with the HW teams. The missing features will be implemented but
this will take time.

The foundation driver as it is right now is still valuable for people wanting to
know how to power control Corstone external systems in a future proof manner
(even in the incomplete state). We prefer to address all the review comments
made so it can be merged. This includes making the DT binding as complete as
possible as you advised. Then, once the HW is ready, I'll implement the comms
and the FW reload part. Is that OK please ?

Cheers,
Abdellatif