| Message ID | 1495262948-1106-4-git-send-email-yi1.li@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
On Sat, May 20, 2017 at 1:49 AM, <yi1.li@linux.intel.com> wrote: Hi Yi, > From: Yi Li <yi1.li@linux.intel.com> > > Since the FPGA image are getting bigger in size, this add an new API > fpga_mgr_firmware_stream You could replace the guts of the current fpga_mgr_firmware_load() with this new API (keep the old name). Unless anyone can think of a reason to keep my old memory hog version. Assuming this all works :) > in FPGA manager, which will stream FPGA image in > 4KB trunks. chunks? > > Signed-off-by: Yi Li <yi1.li@linux.intel.com> > --- > drivers/fpga/fpga-mgr.c | 111 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/fpga/fpga-mgr.h | 4 ++ > 2 files changed, 115 insertions(+) > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > index 188ffef..420ee38 100644 > --- a/drivers/fpga/fpga-mgr.c > +++ b/drivers/fpga/fpga-mgr.c > @@ -27,6 +27,8 @@ > #include <linux/slab.h> > #include <linux/scatterlist.h> > #include <linux/highmem.h> > +#include <linux/sizes.h> > +#include <linux/driver_data.h> > > static DEFINE_IDA(fpga_mgr_ida); > static struct class *fpga_mgr_class; > @@ -196,6 +198,115 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, > return fpga_mgr_write_complete(mgr, info); > } > > +struct fpga_mgr_streaming_priv_params { This name is boggling my mind. :-) Something can be private or it can be params, but it seems like params aren't really private. And later it's referred to as 'params' although it's really private data. Could you rename and take off the _params? That way when I'm reading through this, I'll know that this is something that this code is really passing to itself and I won't mistakenly think that this is a struct that is being passed for the firmware code to consume. > + struct fpga_image_info *info; > + struct fpga_manager *mgr; > + loff_t offset; > + loff_t fw_size; > +}; > + Please add a comment here explaining that this is the callback function, called back from the firmware code for each 4K chunk of FPGA image. > +static int fpga_mgr_streaming_fw_cb(void *context, const struct firmware *fw, > + int err) > +{ > + int ret = -EINVAL; > + struct fpga_mgr_streaming_priv_params *params = And please call this 'priv' or something with 'priv' in the name? > + (struct fpga_mgr_streaming_priv_params *)context; > + struct fpga_image_info *info = params->info; > + struct fpga_manager *mgr = params->mgr; > + struct device *dev = &mgr->dev; > + > + params->fw_size = fw->size; Please skip a line here. > + /* > + * init. > + */ /* A one line comment should be like this, not 3 lines. */ > + if (params->offset == 0) { > + ret = fpga_mgr_write_init_buf(mgr, info, fw->data, fw->size); > + if (ret) > + return ret; > + } > + > + /* > + * Write the FPGA image to the FPGA. > + */ /* 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; > + return ret; > + } > + > + if (fw->size < SZ_4K) Define a macro before the function and use it everywhere SZ_4K means the same thing here (3 occurrences), such as #define FW_CHUNK_SZ SZ_4K > + ret = fpga_mgr_write_complete(mgr, info); > + > + return ret; > +} > + > +/** > + * fpga_mgr_firmware_stream - streaming firmware and load to fpga > + * @mgr: fpga manager > + * @info: fpga image specific information > + * @image_name: name of image file on the firmware search path > + * > + * Streaming 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) > +{ > + int ret; > + char *path = NULL; > + void *buf; > + size_t length = INT_MAX; I guess the firmware layer doesn't give us the size of the whole firmware, so length is used for the while() below, right? Too bad since everybody using streaming firmware is going to be writing this same type of while statement. > + struct device *dev = &mgr->dev; > + struct fpga_mgr_streaming_priv_params params = { priv > + .info = info, > + .mgr = mgr, > + .fw_size = 0, > + .offset = 0, > + }; > + > + const struct driver_data_req_params req_params = { > + DRIVER_DATA_DEFAULT_SYNC(fpga_mgr_streaming_fw_cb, ¶ms), > + .reqs = DRIVER_DATA_REQ_NO_CACHE | DRIVER_DATA_REQ_STREAMING, > + .alloc_buf_size = SZ_4K, FW_CHUNK_SZ or whatever name is appropriate. > + .alloc_buf = &buf, > + .img_offset = ¶ms.offset, > + .path = &path, > + }; > + > + buf = kmalloc(SZ_4K, GFP_KERNEL); > + if (!buf) { > + dev_err(dev, "%s: kmalloc buf failed\n", __func__); This message isn't needed. > + return -ENOMEM; > + } > + > + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ; > + while (length > 0) { This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ) && (length > 0));" since that's what it's really doing. > + ret = driver_data_request_sync(image_name, &req_params, dev); > + if (ret) { > + dev_err(dev, "Error reading firmware %d\n", ret); > + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR; > + return ret; > + } > + > + length -= params.fw_size; > + params.offset += params.fw_size; > + if (params.fw_size < SZ_4K) > + break; > + } > + > + kfree(buf); Please skip a line before return. > + return ret; > +} > +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream); Thanks for working on this! Alan > + > /** > * fpga_mgr_buf_load - load fpga from image in buffer > * @mgr: fpga manager > diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h > index b4ac24c..083e091 100644 > --- a/include/linux/fpga/fpga-mgr.h > +++ b/include/linux/fpga/fpga-mgr.h > @@ -143,6 +143,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); > + > struct fpga_manager *of_fpga_mgr_get(struct device_node *node); > > struct fpga_manager *fpga_mgr_get(struct device *dev); > -- > 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
hi Alan On 5/22/2017 4:09 PM, Alan Tull wrote: > On Sat, May 20, 2017 at 1:49 AM, <yi1.li@linux.intel.com> wrote: > > Hi Yi, > >> From: Yi Li <yi1.li@linux.intel.com> >> >> Since the FPGA image are getting bigger in size, this add an new API >> fpga_mgr_firmware_stream > You could replace the guts of the current fpga_mgr_firmware_load() > with this new API (keep the old name). Unless anyone can think of a > reason to keep my old memory hog version. Assuming this all works :) > >> in FPGA manager, which will stream FPGA image in >> 4KB trunks. > chunks? Oops. > >> Signed-off-by: Yi Li <yi1.li@linux.intel.com> >> --- >> drivers/fpga/fpga-mgr.c | 111 ++++++++++++++++++++++++++++++++++++++++++ >> include/linux/fpga/fpga-mgr.h | 4 ++ >> 2 files changed, 115 insertions(+) >> >> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c >> index 188ffef..420ee38 100644 >> --- a/drivers/fpga/fpga-mgr.c >> +++ b/drivers/fpga/fpga-mgr.c >> @@ -27,6 +27,8 @@ >> #include <linux/slab.h> >> #include <linux/scatterlist.h> >> #include <linux/highmem.h> >> +#include <linux/sizes.h> >> +#include <linux/driver_data.h> >> >> static DEFINE_IDA(fpga_mgr_ida); >> static struct class *fpga_mgr_class; >> @@ -196,6 +198,115 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, >> return fpga_mgr_write_complete(mgr, info); >> } >> >> +struct fpga_mgr_streaming_priv_params { > This name is boggling my mind. :-) Something can be private or it can > be params, but it seems like params aren't really private. And later > it's referred to as 'params' although it's really private data. Could > you rename and take off the _params? That way when I'm reading > through this, I'll know that this is something that this code is > really passing to itself and I won't mistakenly think that this is a > struct that is being passed for the firmware code to consume. Okay, will rename the structure to fpga_mgr_streaming_context? > >> + struct fpga_image_info *info; >> + struct fpga_manager *mgr; >> + loff_t offset; >> + loff_t fw_size; >> +}; >> + > Please add a comment here explaining that this is the callback > function, called back from the firmware code for each 4K chunk of FPGA > image. Sure, will do. >> +static int fpga_mgr_streaming_fw_cb(void *context, const struct firmware *fw, >> + int err) >> +{ >> + int ret = -EINVAL; >> + struct fpga_mgr_streaming_priv_params *params = > And please call this 'priv' or something with 'priv' in the name? Okay, struct fpga_mgr_streaming_context *streaming_context? > >> + (struct fpga_mgr_streaming_priv_params *)context; >> + struct fpga_image_info *info = params->info; >> + struct fpga_manager *mgr = params->mgr; >> + struct device *dev = &mgr->dev; >> + >> + params->fw_size = fw->size; > Please skip a line here. Thanks for catching that. > >> + /* >> + * init. >> + */ > /* A one line comment should be like this, not 3 lines. */ Thanks for catching that. > >> + if (params->offset == 0) { >> + ret = fpga_mgr_write_init_buf(mgr, info, fw->data, fw->size); >> + if (ret) >> + return ret; >> + } >> + >> + /* >> + * Write the FPGA image to the FPGA. >> + */ > /* 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; >> + return ret; >> + } >> + >> + if (fw->size < SZ_4K) > Define a macro before the function and use it everywhere SZ_4K means > the same thing here (3 occurrences), such as > > #define FW_CHUNK_SZ SZ_4K Okay. > >> + ret = fpga_mgr_write_complete(mgr, info); >> + >> + return ret; >> +} >> + >> +/** >> + * fpga_mgr_firmware_stream - streaming firmware and load to fpga >> + * @mgr: fpga manager >> + * @info: fpga image specific information >> + * @image_name: name of image file on the firmware search path >> + * >> + * Streaming 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) >> +{ >> + int ret; >> + char *path = NULL; >> + void *buf; >> + size_t length = INT_MAX; > I guess the firmware layer doesn't give us the size of the whole > firmware, so length is used for the while() below, right? Too bad > since everybody using streaming firmware is going to be writing this > same type of while statement. Yes, with the non-streaming new driver_data_request_xx case, it will give the firmware structure with the size of image length. But with the streaming case, we will not know the size of the image file until it reach to the end of the file. > >> + struct device *dev = &mgr->dev; >> + struct fpga_mgr_streaming_priv_params params = { > priv > >> + .info = info, >> + .mgr = mgr, >> + .fw_size = 0, >> + .offset = 0, >> + }; >> + >> + const struct driver_data_req_params req_params = { >> + DRIVER_DATA_DEFAULT_SYNC(fpga_mgr_streaming_fw_cb, ¶ms), >> + .reqs = DRIVER_DATA_REQ_NO_CACHE | DRIVER_DATA_REQ_STREAMING, >> + .alloc_buf_size = SZ_4K, > FW_CHUNK_SZ or whatever name is appropriate. > >> + .alloc_buf = &buf, >> + .img_offset = ¶ms.offset, >> + .path = &path, >> + }; >> + >> + buf = kmalloc(SZ_4K, GFP_KERNEL); >> + if (!buf) { >> + dev_err(dev, "%s: kmalloc buf failed\n", __func__); > This message isn't needed. Got it > >> + return -ENOMEM; >> + } >> + >> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ; >> + while (length > 0) { > This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ) > && (length > 0));" since that's what it's really doing. Yes, this is better, thanks. > >> + ret = driver_data_request_sync(image_name, &req_params, dev); >> + if (ret) { >> + dev_err(dev, "Error reading firmware %d\n", ret); >> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR; >> + return ret; >> + } >> + >> + length -= params.fw_size; >> + params.offset += params.fw_size; >> + if (params.fw_size < SZ_4K) >> + break; >> + } >> + >> + kfree(buf); > Please skip a line before return. > >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream); > Thanks for working on this! > > Alan > >> + >> /** >> * fpga_mgr_buf_load - load fpga from image in buffer >> * @mgr: fpga manager >> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h >> index b4ac24c..083e091 100644 >> --- a/include/linux/fpga/fpga-mgr.h >> +++ b/include/linux/fpga/fpga-mgr.h >> @@ -143,6 +143,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); >> + >> struct fpga_manager *of_fpga_mgr_get(struct device_node *node); >> >> struct fpga_manager *fpga_mgr_get(struct device *dev); >> -- >> 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
On Mon, May 22, 2017 at 11:11 PM, Li, Yi <yi1.li@linux.intel.com> wrote: > hi Alan > > > On 5/22/2017 4:09 PM, Alan Tull wrote: >> >> On Sat, May 20, 2017 at 1:49 AM, <yi1.li@linux.intel.com> wrote: >> >> Hi Yi, >> >>> From: Yi Li <yi1.li@linux.intel.com> >>> >>> Since the FPGA image are getting bigger in size, this add an new API >>> fpga_mgr_firmware_stream >> >> You could replace the guts of the current fpga_mgr_firmware_load() >> with this new API (keep the old name). Unless anyone can think of a >> reason to keep my old memory hog version. Assuming this all works :) >> >>> in FPGA manager, which will stream FPGA image in >>> 4KB trunks. >> >> chunks? > > Oops. >> >> >>> Signed-off-by: Yi Li <yi1.li@linux.intel.com> >>> --- >>> drivers/fpga/fpga-mgr.c | 111 >>> ++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/fpga/fpga-mgr.h | 4 ++ >>> 2 files changed, 115 insertions(+) >>> >>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c >>> index 188ffef..420ee38 100644 >>> --- a/drivers/fpga/fpga-mgr.c >>> +++ b/drivers/fpga/fpga-mgr.c >>> @@ -27,6 +27,8 @@ >>> #include <linux/slab.h> >>> #include <linux/scatterlist.h> >>> #include <linux/highmem.h> >>> +#include <linux/sizes.h> >>> +#include <linux/driver_data.h> >>> >>> static DEFINE_IDA(fpga_mgr_ida); >>> static struct class *fpga_mgr_class; >>> @@ -196,6 +198,115 @@ static int fpga_mgr_buf_load_mapped(struct >>> fpga_manager *mgr, >>> return fpga_mgr_write_complete(mgr, info); >>> } >>> >>> +struct fpga_mgr_streaming_priv_params { >> >> This name is boggling my mind. :-) Something can be private or it can >> be params, but it seems like params aren't really private. And later >> it's referred to as 'params' although it's really private data. Could >> you rename and take off the _params? That way when I'm reading >> through this, I'll know that this is something that this code is >> really passing to itself and I won't mistakenly think that this is a >> struct that is being passed for the firmware code to consume. > > > Okay, will rename the structure to fpga_mgr_streaming_context? That sounds good. >> >> >>> + struct fpga_image_info *info; >>> + struct fpga_manager *mgr; >>> + loff_t offset; >>> + loff_t fw_size; >>> +}; >>> + >> >> Please add a comment here explaining that this is the callback >> function, called back from the firmware code for each 4K chunk of FPGA >> image. > > > Sure, will do. >>> >>> +static int fpga_mgr_streaming_fw_cb(void *context, const struct firmware >>> *fw, >>> + int err) >>> +{ >>> + int ret = -EINVAL; >>> + struct fpga_mgr_streaming_priv_params *params = >> >> And please call this 'priv' or something with 'priv' in the name? > > > Okay, struct fpga_mgr_streaming_context *streaming_context? That's fine, especially since the firmware layer is calling it "context". >> >> >>> + (struct fpga_mgr_streaming_priv_params *)context; >>> + struct fpga_image_info *info = params->info; >>> + struct fpga_manager *mgr = params->mgr; >>> + struct device *dev = &mgr->dev; >>> + >>> + params->fw_size = fw->size; >> >> Please skip a line here. > > Thanks for catching that. >> >> >>> + /* >>> + * init. >>> + */ >> >> /* A one line comment should be like this, not 3 lines. */ > > Thanks for catching that. >> >> >>> + if (params->offset == 0) { >>> + ret = fpga_mgr_write_init_buf(mgr, info, fw->data, >>> fw->size); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + /* >>> + * Write the FPGA image to the FPGA. >>> + */ >> >> /* 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; >>> + return ret; >>> + } >>> + >>> + if (fw->size < SZ_4K) >> >> Define a macro before the function and use it everywhere SZ_4K means >> the same thing here (3 occurrences), such as >> >> #define FW_CHUNK_SZ SZ_4K > > Okay. >> >> >>> + ret = fpga_mgr_write_complete(mgr, info); >>> + >>> + return ret; >>> +} >>> + >>> +/** >>> + * fpga_mgr_firmware_stream - streaming firmware and load to fpga >>> + * @mgr: fpga manager >>> + * @info: fpga image specific information >>> + * @image_name: name of image file on the firmware search path >>> + * >>> + * Streaming 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) >>> +{ >>> + int ret; >>> + char *path = NULL; >>> + void *buf; >>> + size_t length = INT_MAX; >> >> I guess the firmware layer doesn't give us the size of the whole >> firmware, so length is used for the while() below, right? Too bad >> since everybody using streaming firmware is going to be writing this >> same type of while statement. > > Yes, with the non-streaming new driver_data_request_xx case, it will give > the firmware structure with the size of image length. But with the > streaming case, we will not know the size of the image file until it reach > to the end of the file. > >> >>> + struct device *dev = &mgr->dev; >>> + struct fpga_mgr_streaming_priv_params params = { >> >> priv >> >>> + .info = info, >>> + .mgr = mgr, >>> + .fw_size = 0, >>> + .offset = 0, >>> + }; >>> + >>> + const struct driver_data_req_params req_params = { >>> + DRIVER_DATA_DEFAULT_SYNC(fpga_mgr_streaming_fw_cb, >>> ¶ms), >>> + .reqs = DRIVER_DATA_REQ_NO_CACHE | >>> DRIVER_DATA_REQ_STREAMING, >>> + .alloc_buf_size = SZ_4K, >> >> FW_CHUNK_SZ or whatever name is appropriate. >> >>> + .alloc_buf = &buf, >>> + .img_offset = ¶ms.offset, >>> + .path = &path, >>> + }; >>> + >>> + buf = kmalloc(SZ_4K, GFP_KERNEL); >>> + if (!buf) { >>> + dev_err(dev, "%s: kmalloc buf failed\n", __func__); >> >> This message isn't needed. > > Got it >> >> >>> + return -ENOMEM; >>> + } >>> + >>> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ; >>> + while (length > 0) { >> >> This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ) >> && (length > 0));" since that's what it's really doing. > > Yes, this is better, thanks. > >> >>> + ret = driver_data_request_sync(image_name, &req_params, >>> dev); >>> + if (ret) { >>> + dev_err(dev, "Error reading firmware %d\n", ret); >>> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR; Also need: kfree(buf); >>> + return ret; >>> + } >>> + >>> + length -= params.fw_size; >>> + params.offset += params.fw_size; >>> + if (params.fw_size < SZ_4K) >>> + break; >>> + } >>> + >>> + kfree(buf); >> >> Please skip a line before return. >> >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream); >> >> Thanks for working on this! >> >> Alan >> >>> + >>> /** >>> * fpga_mgr_buf_load - load fpga from image in buffer >>> * @mgr: fpga manager >>> diff --git a/include/linux/fpga/fpga-mgr.h >>> b/include/linux/fpga/fpga-mgr.h >>> index b4ac24c..083e091 100644 >>> --- a/include/linux/fpga/fpga-mgr.h >>> +++ b/include/linux/fpga/fpga-mgr.h >>> @@ -143,6 +143,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); >>> + >>> struct fpga_manager *of_fpga_mgr_get(struct device_node *node); >>> >>> struct fpga_manager *fpga_mgr_get(struct device *dev); >>> -- >>> 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
On Tue, May 23, 2017 at 10:21 AM, Alan Tull <atull@kernel.org> wrote: >>>> + >>>> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ; >>>> + while (length > 0) { >>> >>> This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ) >>> && (length > 0));" since that's what it's really doing. >> >> Yes, this is better, thanks. >> >>> >>>> + ret = driver_data_request_sync(image_name, &req_params, >>>> dev); >>>> + if (ret) { >>>> + dev_err(dev, "Error reading firmware %d\n", ret); >>>> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR; > > Also need: kfree(buf); > >>>> + return ret; Or this could be a break instead of a return. >>>> + } >>>> + >>>> + length -= params.fw_size; >>>> + params.offset += params.fw_size; >>>> + if (params.fw_size < SZ_4K) >>>> + break; >>>> + } >>>> + >>>> + kfree(buf); >>> >>> Please skip a line before return. >>> >>>> + return ret; >>>> +} >>>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream); -- 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
On 5/23/2017 10:25 AM, Alan Tull wrote: > On Tue, May 23, 2017 at 10:21 AM, Alan Tull <atull@kernel.org> wrote: > >>>>> + >>>>> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ; >>>>> + while (length > 0) { >>>> This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ) >>>> && (length > 0));" since that's what it's really doing. >>> Yes, this is better, thanks. >>> >>>>> + ret = driver_data_request_sync(image_name, &req_params, >>>>> dev); >>>>> + if (ret) { >>>>> + dev_err(dev, "Error reading firmware %d\n", ret); >>>>> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR; >> Also need: kfree(buf); >> >>>>> + return ret; > Or this could be a break instead of a return. Thanks Alan, will fix it. >>>>> + } >>>>> + >>>>> + length -= params.fw_size; >>>>> + params.offset += params.fw_size; >>>>> + if (params.fw_size < SZ_4K) >>>>> + break; >>>>> + } >>>>> + >>>>> + kfree(buf); >>>> Please skip a line before return. >>>> >>>>> + return ret; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream); -- 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
On 5/23/2017 10:25 AM, Alan Tull wrote: > On Tue, May 23, 2017 at 10:21 AM, Alan Tull <atull@kernel.org> wrote: > >>>>> + >>>>> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ; >>>>> + while (length > 0) { >>>> This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ) >>>> && (length > 0));" since that's what it's really doing. >>> Yes, this is better, thanks. >>> >>>>> + ret = driver_data_request_sync(image_name, &req_params, >>>>> dev); >>>>> + if (ret) { >>>>> + dev_err(dev, "Error reading firmware %d\n", ret); >>>>> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR; >> Also need: kfree(buf); >> >>>>> + return ret; > Or this could be a break instead of a return. thanks Alan, will fix it. > >>>>> + } >>>>> + >>>>> + length -= params.fw_size; >>>>> + params.offset += params.fw_size; >>>>> + if (params.fw_size < SZ_4K) >>>>> + break; >>>>> + } >>>>> + >>>>> + kfree(buf); >>>> Please skip a line before return. >>>> >>>>> + return ret; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream); -- 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 188ffef..420ee38 100644 --- a/drivers/fpga/fpga-mgr.c +++ b/drivers/fpga/fpga-mgr.c @@ -27,6 +27,8 @@ #include <linux/slab.h> #include <linux/scatterlist.h> #include <linux/highmem.h> +#include <linux/sizes.h> +#include <linux/driver_data.h> static DEFINE_IDA(fpga_mgr_ida); static struct class *fpga_mgr_class; @@ -196,6 +198,115 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, return fpga_mgr_write_complete(mgr, info); } +struct fpga_mgr_streaming_priv_params { + struct fpga_image_info *info; + struct fpga_manager *mgr; + loff_t offset; + loff_t fw_size; +}; + +static int fpga_mgr_streaming_fw_cb(void *context, const struct firmware *fw, + int err) +{ + int ret = -EINVAL; + struct fpga_mgr_streaming_priv_params *params = + (struct fpga_mgr_streaming_priv_params *)context; + struct fpga_image_info *info = params->info; + struct fpga_manager *mgr = params->mgr; + struct device *dev = &mgr->dev; + + params->fw_size = fw->size; + /* + * init. + */ + if (params->offset == 0) { + ret = fpga_mgr_write_init_buf(mgr, info, fw->data, fw->size); + if (ret) + return ret; + } + + /* + * 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; + return ret; + } + + if (fw->size < SZ_4K) + ret = fpga_mgr_write_complete(mgr, info); + + return ret; +} + +/** + * fpga_mgr_firmware_stream - streaming firmware and load to fpga + * @mgr: fpga manager + * @info: fpga image specific information + * @image_name: name of image file on the firmware search path + * + * Streaming 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) +{ + int ret; + char *path = NULL; + void *buf; + size_t length = INT_MAX; + struct device *dev = &mgr->dev; + struct fpga_mgr_streaming_priv_params params = { + .info = info, + .mgr = mgr, + .fw_size = 0, + .offset = 0, + }; + + const struct driver_data_req_params req_params = { + DRIVER_DATA_DEFAULT_SYNC(fpga_mgr_streaming_fw_cb, ¶ms), + .reqs = DRIVER_DATA_REQ_NO_CACHE | DRIVER_DATA_REQ_STREAMING, + .alloc_buf_size = SZ_4K, + .alloc_buf = &buf, + .img_offset = ¶ms.offset, + .path = &path, + }; + + buf = kmalloc(SZ_4K, GFP_KERNEL); + if (!buf) { + dev_err(dev, "%s: kmalloc buf failed\n", __func__); + return -ENOMEM; + } + + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ; + while (length > 0) { + ret = driver_data_request_sync(image_name, &req_params, dev); + if (ret) { + dev_err(dev, "Error reading firmware %d\n", ret); + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR; + return ret; + } + + length -= params.fw_size; + params.offset += params.fw_size; + if (params.fw_size < SZ_4K) + break; + } + + kfree(buf); + return ret; +} +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream); + /** * fpga_mgr_buf_load - load fpga from image in buffer * @mgr: fpga manager diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h index b4ac24c..083e091 100644 --- a/include/linux/fpga/fpga-mgr.h +++ b/include/linux/fpga/fpga-mgr.h @@ -143,6 +143,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); + struct fpga_manager *of_fpga_mgr_get(struct device_node *node); struct fpga_manager *fpga_mgr_get(struct device *dev);