Thanks Alan for the comments.
On 3/13/2017 1:00 PM, Alan Tull wrote:
On Thu, Mar 9, 2017 at 6:18 PM, <yi1...@linux.intel.com> wrote:
Hi Yi,
Thanks for your RFC. I believe this functionality is badly needed.
I had a few comments about the chunk size and some nits about comments below...
From: Yi Li <yi1...@linux.intel.com>
Add fpga_mgr_firmware_stream API, which can load and program firmware
in trucks to FPGA instead of the whole file.
Signed-off-by: Yi Li <yi1...@linux.intel.com>
---
drivers/fpga/fpga-mgr.c | 77 +++++++++++++++++++++++++++++++++++++++++++
include/linux/fpga/fpga-mgr.h | 4 +++
2 files changed, 81 insertions(+)
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 3206a53..bb55b80 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -27,10 +27,15 @@
#include <linux/slab.h>
#include <linux/scatterlist.h>
#include <linux/highmem.h>
+#include <linux/sizes.h>
static DEFINE_IDA(fpga_mgr_ida);
static struct class *fpga_mgr_class;
+static int streamsize = SZ_4K;
+module_param(streamsize, int, 0664);
+MODULE_PARM_DESC(streamsize, "buffer size for firmware streaming");
I think we could fix the chunk size at PAGE_SIZE unless someone can
state a reason for needing something different. Below I have one
reason why we will sometimes need > PAGE_SIZE.
+
/*
* Call the low level driver's write_init function. This will do the
* device-specific things to get the FPGA into the state where it is ready to
@@ -309,6 +314,78 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
}
EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
+/**
+ * fpga_mgr_firmware_load - request firmware and load to fpga
+ * @mgr: fpga manager
+ * @info: fpga image specific information
+ * @image_name: name of image file on the firmware search path
+ *
+ * Request an FPGA image using the firmware class, then write out to the FPGA.
+ * Update the state before each step to provide info on what step failed if
+ * there is a failure. This code assumes the caller got the mgr pointer
+ * from of_fpga_mgr_get() or fpga_mgr_get() and checked that it is not an error
+ * code.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
+ const char *image_name)
No need for both fpga_mgr_firmware_load and fpga_mgr_firmware_stream.
Just replace the old fpga_mgr_firmware_load with this function.
Sure, will do.
+{
+ struct device *dev = &mgr->dev;
+ const struct firmware *fw = NULL;
+ int ret;
+ size_t size = INT_MAX, offset = 0;
+ bool start_flag = 1;
+
+ dev_info(dev, "writing %s to %s with buffer size %d\n",
+ image_name, mgr->name, streamsize);
+
+ mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
+
+ while (size > 0) {
+ ret = stream_firmware(&fw, image_name, dev, offset, streamsize);
+ if (ret) {
+ dev_err(dev, "Error reading firmware %d\n", ret);
+ mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
+ break;
+ }
+
+ /*
+ * init.
+ */
We don't really need this comment.
+ if (start_flag) {
+ ret = fpga_mgr_write_init_buf(mgr, info, fw->data,
+ fw->size);
When the fpga manager is registered, one of the ops is
initial_header_size, which specifies how much buffer the write_init
needs. So this call could fail if initial_header_size < streamsize.
I suggest that you make the streamsize equal to the smallest multiple
of PAGE_SIZE that is >= initial_header_size.
Agree, we can fix the streamsize to something bigger than
initial_header_size for the bitstream file. Another reason to have the
variable streamsize is for the performance, although with Altera Cyclone
V FPGA CvP I did not see much difference when increasing the buffer
size, since the bottleneck is with programming through PCIe and the time
spent with firmware file reading is negligible.
+ start_flag = 0;
+ if (ret)
+ break;
+ }
+
+ /*
+ * Write the FPGA image to the FPGA.
+ */
Or this comment.
+ mgr->state = FPGA_MGR_STATE_WRITE;
+ ret = mgr->mops->write(mgr, fw->data, fw->size);
+ if (ret) {
+ dev_err(dev, "Error while writing image data to
FPGA\n");
+ mgr->state = FPGA_MGR_STATE_WRITE_ERR;
+ break;
+ }
+
+ offset += fw->size;
+ size -= fw->size;
+ if (fw->size < streamsize)
+ break;
+ }
+
+ ret = fpga_mgr_write_complete(mgr, info);
+
+ release_firmware(fw);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);
+
int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
{
if (info->firmware_name)
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 0f5072c..a25362e 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -151,6 +151,10 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
struct fpga_image_info *info,
const char *image_name);
+int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
+ const char *image_name);
Don't need both fpga_mgr_firmware_load and fpga_mgr_firmware_stream,
so no need to change the header.
+
int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
int fpga_mgr_lock(struct fpga_manager *mgr);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
