| Message ID | 1489105090-4996-3-git-send-email-yi1.li@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Mar 9, 2017 at 6:18 PM, <yi1.li@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.li@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.li@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. > +{ > + 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. > + 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.li@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.li@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.li@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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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"); + /* * 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) +{ + 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. + */ + if (start_flag) { + ret = fpga_mgr_write_init_buf(mgr, info, fw->data, + fw->size); + start_flag = 0; + if (ret) + break; + } + + /* + * Write the FPGA image to the FPGA. + */ + 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); + int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info); int fpga_mgr_lock(struct fpga_manager *mgr);