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
