Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1599666
| From | Alan Tull <delicious.quinoa@gmail.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [RFC 2/2] fpga manager: Add fpga_mgr_firmware_stream API |
| Date | 2017-03-13 19:10 +0100 |
| Message-ID | <tkCzT-4kS-1@gated-at.bofh.it> (permalink) |
| References | <tjgBr-4eD-3@gated-at.bofh.it> <tjgBr-4eD-1@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
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
>
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[RFC 0/2] Add streaming API for firmware and FPGA manager yi1.li@linux.intel.com - 2017-03-10 01:30 +0100
[RFC 2/2] fpga manager: Add fpga_mgr_firmware_stream API yi1.li@linux.intel.com - 2017-03-10 01:30 +0100
Re: [RFC 2/2] fpga manager: Add fpga_mgr_firmware_stream API Alan Tull <delicious.quinoa@gmail.com> - 2017-03-13 19:10 +0100
Re: [RFC 2/2] fpga manager: Add fpga_mgr_firmware_stream API "Li, Yi" <yi1.li@linux.intel.com> - 2017-03-13 20:10 +0100
[RFC 1/2] firmware class: Add stream_firmware API. yi1.li@linux.intel.com - 2017-03-10 01:30 +0100
Re: [RFC 1/2] firmware class: Add stream_firmware API. matthew.gerlach@linux.intel.com - 2017-03-10 18:50 +0100
Re: [RFC 1/2] firmware class: Add stream_firmware API. "Li, Yi" <yi1.li@linux.intel.com> - 2017-03-10 20:30 +0100
Re: [RFC 1/2] firmware class: Add stream_firmware API. matthew.gerlach@linux.intel.com - 2017-03-13 22:10 +0100
Re: [RFC 1/2] firmware class: Add stream_firmware API. "Li, Yi" <yi1.li@linux.intel.com> - 2017-03-14 17:20 +0100
Re: [RFC 1/2] firmware class: Add stream_firmware API. matthew.gerlach@linux.intel.com - 2017-03-14 18:00 +0100
Re: [RFC 1/2] firmware class: Add stream_firmware API. Alan Tull <delicious.quinoa@gmail.com> - 2017-03-20 19:10 +0100
Re: [RFC 1/2] firmware class: Add stream_firmware API. Alan Tull <delicious.quinoa@gmail.com> - 2017-03-20 19:40 +0100
Re: [RFC 1/2] firmware class: Add stream_firmware API. "Li, Yi" <yi1.li@linux.intel.com> - 2017-03-22 23:10 +0100
Re: [RFC 1/2] firmware class: Add stream_firmware API. Alan Tull <delicious.quinoa@gmail.com> - 2017-03-23 01:40 +0100
Re: [RFC 0/2] Add streaming API for firmware and FPGA manager matthew.gerlach@linux.intel.com - 2017-03-10 18:20 +0100
csiph-web