| Message ID | 1478732303-13718-10-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jason, On 11/09/2016 02:58 PM, Jason Gunthorpe wrote: > All drivers now use the sg interface so there is no reason to keep > the contiguous interface any more. > > Now that all drivers support this interface we can also export it. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > drivers/fpga/fpga-mgr.c | 62 +++++++------------------------------------ > include/linux/fpga/fpga-mgr.h | 7 ++--- > 2 files changed, 11 insertions(+), 58 deletions(-) > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > index c2491ffeabd3..4ba22925d9d5 100644 > --- a/drivers/fpga/fpga-mgr.c > +++ b/drivers/fpga/fpga-mgr.c > @@ -47,8 +47,8 @@ static struct class *fpga_mgr_class; > * > * Return: 0 on success, negative error code otherwise. > */ > -static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, > - struct sg_table *sgt) > +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, > + struct sg_table *sgt) > { > struct device *dev = &mgr->dev; > int ret; > @@ -92,52 +92,7 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, > > return 0; > } > - > -static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, u32 flags, > - const char *buf, size_t count) > -{ > - struct device *dev = &mgr->dev; > - int ret; > - > - /* > - * 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 receive an FPGA image. > - */ > - mgr->state = FPGA_MGR_STATE_WRITE_INIT; > - ret = mgr->mops->write_init(mgr, flags, buf, count); > - if (ret) { > - dev_err(dev, "Error preparing FPGA for writing\n"); > - mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR; > - return ret; > - } > - > - /* > - * Write the FPGA image to the FPGA. > - */ > - mgr->state = FPGA_MGR_STATE_WRITE; > - ret = mgr->mops->write(mgr, buf, count); > - if (ret) { > - dev_err(dev, "Error while writing image data to FPGA\n"); > - mgr->state = FPGA_MGR_STATE_WRITE_ERR; > - return ret; > - } > - > - /* > - * After all the FPGA image has been written, do the device specific > - * steps to finish and set the FPGA into operating mode. > - */ > - mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE; > - ret = mgr->mops->write_complete(mgr, flags); > - if (ret) { > - dev_err(dev, "Error after writing image data to FPGA\n"); > - mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR; > - return ret; > - } > - mgr->state = FPGA_MGR_STATE_OPERATING; > - > - return 0; > -} > +EXPORT_SYMBOL_GPL(fpga_mgr_buf_load_sg); > > /** > * fpga_mgr_buf_load - load fpga from image in buffer > @@ -163,9 +118,6 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf, > int index; > int rc; > > - if (!mgr->mops->write_init_sg || !mgr->mops->write_sg) > - return fpga_mgr_buf_load_mapped(mgr, flags, buf, count); > - > /* > * Convert the linear kernel pointer into a sg_table of pages for use > * by the driver. > @@ -226,6 +178,11 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags, > > mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ; > > + /* > + * FIXME: We do not need a vmap, just a page list, but > + * request_firmware has no way to give us that, so this needlessly > + * consumes vmalloc space. > + */ > ret = request_firmware(&fw, image_name, dev); > if (ret) { > mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR; > @@ -369,8 +326,7 @@ int fpga_mgr_register(struct device *dev, const char *name, > int id, ret; > > if (!mops || !mops->write_complete || !mops->state || > - ((!mops->write_init || !mops->write) && > - (!mops->write_init_sg || !mops->write_sg))) { > + !mops->write_init_sg || !mops->write_sg) { > dev_err(dev, "Attempt to register without fpga_manager_ops\n"); > return -EINVAL; > } > diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h > index 371b30ea60eb..5c698c8fe71b 100644 > --- a/include/linux/fpga/fpga-mgr.h > +++ b/include/linux/fpga/fpga-mgr.h > @@ -72,8 +72,6 @@ enum fpga_mgr_states { > /** > * struct fpga_manager_ops - ops for low level fpga manager drivers > * @state: returns an enum value of the FPGA's state > - * @write_init: prepare the FPGA to receive confuration data (linear memory) > - * @write: write count bytes of configuration data to the FPGA > * @write_init_sg: prepare the FPGA to receive confuration data (scatter list > * table) > * @write_sg: write count bytes of configuration data to the FPGA > @@ -86,9 +84,6 @@ enum fpga_mgr_states { > */ > struct fpga_manager_ops { > enum fpga_mgr_states (*state)(struct fpga_manager *mgr); > - int (*write_init)(struct fpga_manager *mgr, u32 flags, > - const char *buf, size_t count); > - int (*write)(struct fpga_manager *mgr, const char *buf, size_t count); > int (*write_init_sg)(struct fpga_manager *mgr, u32 flags, > struct sg_table *sgt); > int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt); > @@ -118,6 +113,8 @@ struct fpga_manager { > > int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, > const char *buf, size_t count); > +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, > + struct sg_table *sgt); > > int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags, > const char *image_name); I don't have any feeling either way about switching to scatter-gather. (Not zynq or socfpga user) But I do object to renaming the API. write_init() and write() do not imply a particular implementation, nor even that the buffer is coherent. I am working to merge an fpga manager which uses SPI to load the bitstream (see https://www.spinics.net/lists/arm-kernel/msg539328.html) Any dma in use there would come from the spi driver. write_init_sg, and write_sg don't make any sense in my case. Would it not make sense to keep the top level API the same? Thanks, Joshua.
> > struct fpga_manager_ops { > > enum fpga_mgr_states (*state)(struct fpga_manager *mgr); > > - int (*write_init)(struct fpga_manager *mgr, u32 flags, > > - const char *buf, size_t count); > > - int (*write)(struct fpga_manager *mgr, const char *buf, size_t count); > > int (*write_init_sg)(struct fpga_manager *mgr, u32 flags, > > struct sg_table *sgt); > > int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt); > > @@ -118,6 +113,8 @@ struct fpga_manager { > > > > int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, > > const char *buf, size_t count); > > +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, > > + struct sg_table *sgt); > > > > int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags, > > const char *image_name); > I don't have any feeling either way about switching to scatter-gather. > (Not zynq or socfpga user) > But I do object to renaming the API. > write_init() and write() do not imply a particular implementation, nor even that > the buffer is coherent. Neither the sg or old linear interface imply any particular underlying driver implementation. All that is being changed is how the list of physical pages gets passed to the driver. The linear interface requires them to be contiguously mapped (eg in a vmap) while the SG interface directly passes a list of physical page addresses. Any alogrithm that works with the old interface can run on the new interface, and the new interface can support much better options for DMA drivers, while not requiring the higher layers to perform a high order allocation (vmap or otherwise) to create the contiguous memory. The reason the old interface is being deleted here is so the fpga mgr API can be expanded to accept a sg list directly. Since we cannot convert a general sg list to linear memory the liner option must be totally removed. > I am working to merge an fpga manager which uses SPI to load the bitstream > (see https://www.spinics.net/lists/arm-kernel/msg539328.html) > Any dma in use there would come from the spi driver. write_init_sg, and write_sg > don't make any sense in my case. No, it still make lots of sense. SPI has been slowly transforming to use the same sort of SG scheme universally, including facing the client. (see 6ad45a27cbe343ec8d7888e5edf6335499a4b555) Some day your driver can just pass the SGs directly to spi and everything will be great. In the mean time it can do sg_miter_next to get mapped buffers. > Would it not make sense to keep the top level API the same? Fundamentally no. Jason
On 11/10/2016 08:33 AM, Jason Gunthorpe wrote: >>> struct fpga_manager_ops { >>> enum fpga_mgr_states (*state)(struct fpga_manager *mgr); >>> - int (*write_init)(struct fpga_manager *mgr, u32 flags, >>> - const char *buf, size_t count); >>> - int (*write)(struct fpga_manager *mgr, const char *buf, size_t count); >>> int (*write_init_sg)(struct fpga_manager *mgr, u32 flags, >>> struct sg_table *sgt); >>> int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt); >>> @@ -118,6 +113,8 @@ struct fpga_manager { >>> >>> int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, >>> const char *buf, size_t count); >>> +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, >>> + struct sg_table *sgt); >>> >>> int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags, >>> const char *image_name); >> I don't have any feeling either way about switching to scatter-gather. >> (Not zynq or socfpga user) >> But I do object to renaming the API. >> write_init() and write() do not imply a particular implementation, nor even that >> the buffer is coherent. > Neither the sg or old linear interface imply any particular > underlying driver implementation. > > All that is being changed is how the list of physical pages gets > passed to the driver. The linear interface requires them to be > contiguously mapped (eg in a vmap) while the SG interface > directly passes a list of physical page addresses. > > Any alogrithm that works with the old interface can run on the new > interface, and the new interface can support much better options for > DMA drivers, while not requiring the higher layers to perform a high > order allocation (vmap or otherwise) to create the contiguous memory. > > The reason the old interface is being deleted here is so the fpga mgr > API can be expanded to accept a sg list directly. Since we cannot > convert a general sg list to linear memory the liner option must be > totally removed. OK. That sounds reasonable. >> I am working to merge an fpga manager which uses SPI to load the bitstream >> (see https://www.spinics.net/lists/arm-kernel/msg539328.html) >> Any dma in use there would come from the spi driver. write_init_sg, and write_sg >> don't make any sense in my case. > No, it still make lots of sense. > > SPI has been slowly transforming to use the same sort of SG scheme > universally, including facing the client. (see > 6ad45a27cbe343ec8d7888e5edf6335499a4b555) Thanks for sharing that link. > > Some day your driver can just pass the SGs directly to spi and > everything will be great. > > In the mean time it can do sg_miter_next to get mapped buffers. ..so I have a way forward if this series gets merged. That was my main concern. And as a dma n00b, learning to use dma engine structures to do non-dma xfer was not very high on my list. >> Would it not make sense to keep the top level API the same? > Fundamentally no. > > Jason Joshua
On Thu, 10 Nov 2016, Joshua Clayton wrote: > > > On 11/10/2016 08:33 AM, Jason Gunthorpe wrote: > >>> struct fpga_manager_ops { > >>> enum fpga_mgr_states (*state)(struct fpga_manager *mgr); > >>> - int (*write_init)(struct fpga_manager *mgr, u32 flags, > >>> - const char *buf, size_t count); > >>> - int (*write)(struct fpga_manager *mgr, const char *buf, size_t count); > >>> int (*write_init_sg)(struct fpga_manager *mgr, u32 flags, > >>> struct sg_table *sgt); > >>> int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt); > >>> @@ -118,6 +113,8 @@ struct fpga_manager { > >>> > >>> int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, > >>> const char *buf, size_t count); > >>> +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, > >>> + struct sg_table *sgt); > >>> > >>> int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags, > >>> const char *image_name); > >> I don't have any feeling either way about switching to scatter-gather. > >> (Not zynq or socfpga user) > >> But I do object to renaming the API. > >> write_init() and write() do not imply a particular implementation, nor even that > >> the buffer is coherent. > > Neither the sg or old linear interface imply any particular > > underlying driver implementation. > > > > All that is being changed is how the list of physical pages gets > > passed to the driver. The linear interface requires them to be > > contiguously mapped (eg in a vmap) while the SG interface > > directly passes a list of physical page addresses. > > > > Any alogrithm that works with the old interface can run on the new > > interface, and the new interface can support much better options for > > DMA drivers, while not requiring the higher layers to perform a high > > order allocation (vmap or otherwise) to create the contiguous memory. > > > > The reason the old interface is being deleted here is so the fpga mgr > > API can be expanded to accept a sg list directly. Since we cannot > > convert a general sg list to linear memory the liner option must be > > totally removed. > OK. That sounds reasonable. > >> I am working to merge an fpga manager which uses SPI to load the bitstream > >> (see https://www.spinics.net/lists/arm-kernel/msg539328.html) > >> Any dma in use there would come from the spi driver. write_init_sg, and write_sg > >> don't make any sense in my case. > > No, it still make lots of sense. > > > > SPI has been slowly transforming to use the same sort of SG scheme > > universally, including facing the client. (see > > 6ad45a27cbe343ec8d7888e5edf6335499a4b555) > Thanks for sharing that link. > > > > Some day your driver can just pass the SGs directly to spi and > > everything will be great. > > > > In the mean time it can do sg_miter_next to get mapped buffers. > ..so I have a way forward if this series gets merged. > That was my main concern. > And as a dma n00b, learning to use dma engine structures to do > non-dma xfer was not very high on my list. > >> Would it not make sense to keep the top level API the same? > > Fundamentally no. NACK for now. Let's slow down here a bit. I don't see any rush in getting rid of the contiguous buffer interface. At the very least, my socfpga-a10 driver is using the old interface and has been just applied. And currently your socfpga changes are untested whereas my original driver has been in use and is known to work. Let's wait on that patch until we have quite a few FPGA's supported and can be sure that we won't miss the old interface. Alan > > > > Jason > Joshua >
On Sun, Nov 13, 2016 at 02:44:51PM -0600, atull wrote: > > >> Would it not make sense to keep the top level API the same? > > > Fundamentally no. > > NACK for now. Let's slow down here a bit. > > I don't see any rush in getting rid of the contiguous > buffer interface. As I explained to Joshua, until all the drivers support sg the public sg interface can not be sensibly added, so it actually is a hard blocker to making progress. Further, the longer things are left, the more code will depend on the broken interface and the harder it will be to ultimately fix. This is a good time because there are only 2 upstream drivers, and I can test one, you can test the other :) > At the very least, my socfpga-a10 driver is using the old > interface and has been just applied. And currently your > socfpga changes are untested whereas my original driver > has been in use and is known to work. Well, looks like a10 uses the same write code as socfpga, so the patch should trivially port over. Let me know if you need help. Are you able to test the modified socfpga? > Let's wait on that patch until we have quite a few FPGA's > supported and can be sure that we won't miss the old > interface. Well, I am sure :) There is no reason to need this contiguous virtual memory at the driver level. Our existing drivers demonstrate both how to do DMA and PIO from the SG list, there really are no other transfer modes supported by the kernel ... But even if you are not sure, keeping the unused dead API around is the exact opposite of the accepted kernel process. Read stable-api-nonsense.txt and understand how that applies here. The API can be revised again if a driver comes along that needs an improvement, but given how intrinsically broken huge contiguous allocations are, I can't forsee any situation where returning to this interface would be a good idea. Do you have comments on the other patches? Jason
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index c2491ffeabd3..4ba22925d9d5 100644 --- a/drivers/fpga/fpga-mgr.c +++ b/drivers/fpga/fpga-mgr.c @@ -47,8 +47,8 @@ static struct class *fpga_mgr_class; * * Return: 0 on success, negative error code otherwise. */ -static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, - struct sg_table *sgt) +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, + struct sg_table *sgt) { struct device *dev = &mgr->dev; int ret; @@ -92,52 +92,7 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, return 0; } - -static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, u32 flags, - const char *buf, size_t count) -{ - struct device *dev = &mgr->dev; - int ret; - - /* - * 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 receive an FPGA image. - */ - mgr->state = FPGA_MGR_STATE_WRITE_INIT; - ret = mgr->mops->write_init(mgr, flags, buf, count); - if (ret) { - dev_err(dev, "Error preparing FPGA for writing\n"); - mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR; - return ret; - } - - /* - * Write the FPGA image to the FPGA. - */ - mgr->state = FPGA_MGR_STATE_WRITE; - ret = mgr->mops->write(mgr, buf, count); - if (ret) { - dev_err(dev, "Error while writing image data to FPGA\n"); - mgr->state = FPGA_MGR_STATE_WRITE_ERR; - return ret; - } - - /* - * After all the FPGA image has been written, do the device specific - * steps to finish and set the FPGA into operating mode. - */ - mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE; - ret = mgr->mops->write_complete(mgr, flags); - if (ret) { - dev_err(dev, "Error after writing image data to FPGA\n"); - mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR; - return ret; - } - mgr->state = FPGA_MGR_STATE_OPERATING; - - return 0; -} +EXPORT_SYMBOL_GPL(fpga_mgr_buf_load_sg); /** * fpga_mgr_buf_load - load fpga from image in buffer @@ -163,9 +118,6 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf, int index; int rc; - if (!mgr->mops->write_init_sg || !mgr->mops->write_sg) - return fpga_mgr_buf_load_mapped(mgr, flags, buf, count); - /* * Convert the linear kernel pointer into a sg_table of pages for use * by the driver. @@ -226,6 +178,11 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags, mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ; + /* + * FIXME: We do not need a vmap, just a page list, but + * request_firmware has no way to give us that, so this needlessly + * consumes vmalloc space. + */ ret = request_firmware(&fw, image_name, dev); if (ret) { mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR; @@ -369,8 +326,7 @@ int fpga_mgr_register(struct device *dev, const char *name, int id, ret; if (!mops || !mops->write_complete || !mops->state || - ((!mops->write_init || !mops->write) && - (!mops->write_init_sg || !mops->write_sg))) { + !mops->write_init_sg || !mops->write_sg) { dev_err(dev, "Attempt to register without fpga_manager_ops\n"); return -EINVAL; } diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h index 371b30ea60eb..5c698c8fe71b 100644 --- a/include/linux/fpga/fpga-mgr.h +++ b/include/linux/fpga/fpga-mgr.h @@ -72,8 +72,6 @@ enum fpga_mgr_states { /** * struct fpga_manager_ops - ops for low level fpga manager drivers * @state: returns an enum value of the FPGA's state - * @write_init: prepare the FPGA to receive confuration data (linear memory) - * @write: write count bytes of configuration data to the FPGA * @write_init_sg: prepare the FPGA to receive confuration data (scatter list * table) * @write_sg: write count bytes of configuration data to the FPGA @@ -86,9 +84,6 @@ enum fpga_mgr_states { */ struct fpga_manager_ops { enum fpga_mgr_states (*state)(struct fpga_manager *mgr); - int (*write_init)(struct fpga_manager *mgr, u32 flags, - const char *buf, size_t count); - int (*write)(struct fpga_manager *mgr, const char *buf, size_t count); int (*write_init_sg)(struct fpga_manager *mgr, u32 flags, struct sg_table *sgt); int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt); @@ -118,6 +113,8 @@ struct fpga_manager { int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf, size_t count); +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags, + struct sg_table *sgt); int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags, const char *image_name);
All drivers now use the sg interface so there is no reason to keep the contiguous interface any more. Now that all drivers support this interface we can also export it. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- drivers/fpga/fpga-mgr.c | 62 +++++++------------------------------------ include/linux/fpga/fpga-mgr.h | 7 ++--- 2 files changed, 11 insertions(+), 58 deletions(-)