Hi Ben,

On Sun, Sep 20, 2020 at 4:16 PM Ben Levinsky [off-list ref] wrote:

Hi All,

quoted

-----Original Message-----
From: Wendy Liang <redacted>
Sent: Friday, September 18, 2020 6:53 PM
To: Michael Auchter <redacted>
Cc: Ben Levinsky <redacted>; punit1.agrawal@toshiba.co.jp;
devicetree@vger.kernel.org; linux-remoteproc@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
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 [off-list ref]
wrote:

quoted

Hey Ben,

On Fri, Sep 18, 2020 at 06:01:19PM +0000, Ben Levinsky wrote:

quoted

Hi Michael, Punit,

quoted

-----Original Message-----
From: Michael Auchter <redacted>
Sent: Friday, September 18, 2020 9:07 AM
To: Ben Levinsky <redacted>
Cc: devicetree@vger.kernel.org; linux-remoteproc@vger.kernel.org;

linux-

quoted

quoted

quoted

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:

quoted

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

quoted

quoted

quoted

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.

quoted

quoted

- the zynqmp_remoteproc_r5 driver does not configure and memory

regions or the RPU. Let the Xilinx firmware sysfs interface handle this.

quoted

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?

quoted

[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

quoted

Best Regards,
Wendy

quoted

- How will someone know to configure the RPU mode and TCM mode via

sysfs?

quoted

- What happens when someone changes the RPU mode after remoteproc

has

quoted

  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?

quoted

quoted

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

[Wendy] As op mode is described in the device tree, in lockstep mode,
r5-1 doesn't need to show in the sysfs.

Thanks,
Wendy

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.

quoted

quoted

Thanks,
 Michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel