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

[Wendy Liang]

Hi Ben

On Sun, Sep 20, 2020 at 4:16 PM Ben Levinsky <BLEVINSK@xxxxxxxxxx> wrote:
>
> Hi All,
>
> > -----Original Message-----
> > From: Wendy Liang <sunnyliangjy@xxxxxxxxx>
> > Sent: Friday, September 18, 2020 6:53 PM
> > To: Michael Auchter <michael.auchter@xxxxxx>
> > Cc: Ben Levinsky <BLEVINSK@xxxxxxxxxx>; punit1.agrawal@xxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-remoteproc@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > Subject: Re: RE: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5
> > remoteproc driver
> >
> > HI Michael, Ben, Punit,
> >
> > On Fri, Sep 18, 2020 at 12:08 PM Michael Auchter <michael.auchter@xxxxxx>
> > wrote:
> > >
> > > 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@xxxxxx>
> > > > > Sent: Friday, September 18, 2020 9:07 AM
> > > > > To: Ben Levinsky <BLEVINSK@xxxxxxxxxx>
> > > > > Cc: devicetree@xxxxxxxxxxxxxxx; linux-remoteproc@xxxxxxxxxxxxxxx;
> > linux-
> > > > > kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > > > > 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?
> > >
> [Ben Levinsky] A way to do this, though it seems later comments show it is not an implementation to pursue, is use the RPU configuration API and present it via sysfs interface a la https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842232/Zynq+UltraScale+MPSoC+Power+Management+-+Linux+Kernel#ZynqUltraScale%EF%BC%8BMPSoCPowerManagement-LinuxKernel-EnableClock
> > > > 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.
> [Ben Levinsky] ok, noted. Can keep the configuration but still as wendy said just have lockstep property to denote lockstep mode in RPU and otherwise be split, for simplicity?
> > [Wendy] The TCMs are presented differently in the system depending on
> > if RPU is in
> > lockstep or split mode.
> >
> > Not sure if it is allowed to list TCMs registers properties for both
> > split mode and lockstep
> > mode in the same device node.
> >
> > Even though, driver can have this information in the code, but I feel
> > the device tree is a
> > better place for this information.
> > And also for predefined shared memories, you will need to know the RPU
> > op mode ahead,
> > so that you can specify which shared memories belong to which RPU.
> >
> > To dynamic setup the RPU mode, besides sysfs, setup, if remoteproc can
> > support
> > device tree overlay, the RPUs can be described with dtbo and loaded at
> > runtime.
> >
> > Just want to understand the case which needs to set  RPU mode at runtime?
> > I think testing can be one case.
> >
> [Ben Levinsky] for testing, so far it has been r50/1 split and r5 lockstep
[Wendy] I tried to understand the need to change the RPU mode at runtime.
What I can think of is for testing purposes.

Thanks,
Wendy

> > Best Regards,
> > Wendy
> >
> > > - 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.
> > >
> [Ben Levinsky] Ok, as Wendy suggested would instead the presence of a "lockstep=mode" property indicate lockstep mode and otherwise imply split mode?
> > > 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.
> > >
> [Ben Levinsky] Good Point. the loading of R5 1 while in lockstep is an uncovered corner case.. for this, before loading/starting or requesting memory the state of global rpu mode can be checked and this can act as a guard for probing a remoteproc instance for r5-1 if either is in lockstep and similar safeguard for firmware loading for R5-1 if in lockstep mode
>
> That is, add the lockstep property only if in lockstep mode and use the presence of it or lack thereof for subsequent, single R5-specific driver remoteproc R5 probes or firmware loading
>
> In addition to the above property and its behavior, would correcting the inconsistencies of the Documentation vs the split/lockstep code in the remoteproc r5 device tree binding, its corresponding remoteproc r5 driver address the above concerns as well as the memory handling as you noted earlier?
>
> Also in the next series I can point to a sample R5 application and device trees for the split mode and lockstep cases I used for testing in the cover letter.
>
> > > Thanks,
> > >  Michael