Re: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver

[Michael Auchter <michael.auchter@ni.com>]

Hey Ben,

On Fri, Sep 18, 2020 at 06:01:19PM +0000, Ben Levinsky wrote:
> Hi Michael, Punit,
> 
> > -----Original Message-----
> > From: Michael Auchter <michael.auchter@ni.com>
> > Sent: Friday, September 18, 2020 9:07 AM
> > To: Ben Levinsky <BLEVINSK@xilinx.com>
> > Cc: devicetree@vger.kernel.org; linux-remoteproc@vger.kernel.org; linux-
> > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Subject: Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5
> > remoteproc driver
> > 
> > On Thu, Sep 17, 2020 at 10:50:42PM +0000, Ben Levinsky wrote:
> > > In addition to device tree, is there particular linker script you use
> > > for your R5 application? For example with OCM? As presently this
> > > driver only has DDR and TCM as supported regions to load into
> > 
> > The firmware is being loaded to TCM.
> > 
> > I'm able to use this driver to load and run my firmware on both R5
> > cores, but only after I change the incorrect:
> > 
> > 	rpu_mode = lockstep_mode
> > 
> > assignment to:
> > 
> > 	rpu_mode = lockstep_mode ? PM_RPU_MODE_LOCKSTEP
> > 				 : PM_RPU_MODE_SPLIT;
> There was a point raised by Punit that as "it is possible to set R5 to
> operatore in split or lock-step mode dynamically" which is true and
> can be done via sysfs and the Xilinx firmware kernel code.

I'm not familiar with this, and don't see an obvious way to do this
(from looking at drivers/firmware/xilinx/). Can you point me to this
code?

> A suggestion that might clean up the driver so that the whole
> rpu_mode, tcm_mode configuration can be simplified and pulled out of
> the driver:
> - as Punit suggested, remove the lockstep-mode property
> - the zynqmp_remoteproc_r5 driver ONLY loads firmware and does start/stop.
> - the zynqmp_remoteproc_r5 driver does not configure and memory regions or the RPU. Let the Xilinx firmware sysfs interface handle this.

I don't think this is a good approach.
- How will someone know to configure the RPU mode and TCM mode via sysfs?
- What happens when someone changes the RPU mode after remoteproc has
  already booted some firmware on it?
- What if the kernel is the one booting the R5, not the user?

Split vs. lockstep, IMO, needs to be specified as part of the device
tree, and this driver needs to handle configuring the RPU mode and TCM
modes appropriately.

Split vs. lockstep already necessitates different entries in the device
tree:
- In the binding, each core references its TCMs via the
  meta-memory-regions phandles, and the referenced nodes necessarily
  encode this size. In split mode, each core has access to 64K of
  TCMA/TCMB, while in lockstep R5 0 has access to 128K of TCMA/TCMB. So,
  the "xlnx,tcm" nodes' reg entries need to differ between lockstep and
  split.
- In lockstep mode, it does not make sense to have both r5@0 and r5@1
  child nodes: only r5@0 makes sense. Though, I just realized that I
  think this driver will currently permit that, and register two
  remoteprocs even in lockstep mode... What happens if someone tries to
  load firmware on to r5_1 when they're in lockstep mode? This should
  probably be prevented.

Thanks,
 Michael