On Mon, May 22, 2017 at 11:11 PM, Li, Yi <yi1...@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...@linux.intel.com> wrote: >> >> Hi Yi, >> >>> From: Yi Li <yi1...@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...@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 >>> >