On Sun, Jul 08, 2018 at 03:28:16PM +0200, Greg KH wrote: > On Sat, Jul 07, 2018 at 10:19:40PM -0400, Jacob Feder wrote: > > Hi all, > > I have developed this driver for a Xilinx-provided IP block for their Zynq > > SoC. I fixed all of the checkpatch.pl problems I knew how to. If someone > > could chime in on how to fix the remaining few it would be appreciated. > > > > Also looking for feedback on general structure. It's my first driver (and > > patch submission) so I'm sure there are plenty of things to be improved on > > :). > > > > Functionally everything works OK except the performance isn't as good as I > > was hoping for. I have been testing it by operating the FIFO in loopback > > mode (AXI-Stream TX interface feeding back into RX interface) running on > > the XC7Z020 (7000 series) Zynq device. I am getting anything between > > 3-16MB/s depending on the amount of data transferred. The performance > > claimed by the PG080 datasheet is ~65MB/s. The CPU isn't under significant > > load (~12%) as reported by top so I don't think that's the bottleneck. > > > > Please +CC in responses as I'm not on the mailing list. > > > > Cheers > > > > > > This IP core has read and write AXI-Stream FIFOs, the contents of which can > > be accessed from the AXI4 memory-mapped interface. This is useful for > > transferring data from a processor into the FPGA fabric. The driver creates > > a character device that can be read/written to with standard > > open/read/write/close. > > Why not use the uio api, which allows userspace to mmap the memory of > the device and access it directly from userspace? That should make > things a lot faster, right? I thought about the UIO method but based on what I read it seemed like more of a hack (and also doesn't expose interrupts?). Whether it would make things faster I have no idea. > > Or if that doesn't work, what about the fpga API the kernel now has? > Would that work for this hardware? > I'm not totally sure what you're referring to here, but I think the FPGA kernel drivers are for downloading bitstreams to the FPGA (bitstream is equivalent to asm for fpgas), which isn't what I'm trying to do. > > > > > See Xilinx PG080 document for IP details. > > Do you have a link to that? If so, can you put it in here? > https://www.xilinx.com/support/documentation/ip_documentation/axi_fifo_mm_s/v4_1/pg080-axi-fifo-mm-s.pdf > > > > Currently supports only store-forward mode with a 32-bit > > AXI4-Lite interface. DOES NOT support: > > - cut-through mode > > - AXI4 (non-lite) > > > > Signed-off-by: Jacob Feder <jacobsfeder@xxxxxxxxx> > > --- > > drivers/staging/axisfifo/axis-fifo.c | 1296 ++++++++++++++++++++++++++++++++++ > > drivers/staging/axisfifo/axis-fifo.h | 119 ++++ > > 2 files changed, 1415 insertions(+) > > create mode 100644 drivers/staging/axisfifo/axis-fifo.c > > create mode 100644 drivers/staging/axisfifo/axis-fifo.h > > Why does a single .c file need a .h file? Good point... this can be consolidated :) > > I'll be glad to take this driver, as others can clean it up in-tree (I > think your locking is crazy and is probably causing a lot of performance > issues), but I need a TODO file for it listing what you think is needed > to do in order to get this out of the staging tree? I'm confused about why you don't like the locking - all I'm doing is locking on open() calls to prevent multiple userspace apps from reading/writing to the fifo simultaneously. This shouldn't reduce performance because the mutexes are only tested on open() not on read() or write(). Presumably the user is only opening once. I think locking is necessary - if the hardware registers are accessed in the wrong order it goes into an unknown state and must be reset (and will probably cause a kernel panic). > > Or, if you thin it should use the fpga or uio interface instead, maybe > it's just easier to redo it based on that? > > thanks, > > greg k-h I have made some slight modifications today. Let me know if there are any other things you think I should change. I can integrate those then resubmit a v2 for you to bring into the tree. In terms of TODO it's all ok as far as I'm concerned unless someone wants to look into the performance. (actually see below - I think this could explain why performance of my system is lower - xilinx is using some sort of DMA driver instead of io_remap) https://forums.xilinx.com/xlnx/attachments/xlnx/ELINUX/13011/2/Linux-DMA-In-Device-Drivers.pdf Thanks!! Cheers, Jacob _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
