Re: [PATCH v10 3/8] add fpga manager core
On Thu 2015-08-13 12:37:27, at...@opensource.altera.com wrote: > From: Alan Tull > > API to support programming FPGA. I'd do s/fpga/FPGA/ in the comments, too. Otherwise looks ok to me. Acked-by: Pavel Machek -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v10 3/8] add fpga manager core
Hi Alan, On Fri, Aug 14, 2015 at 8:46 AM, atull wrote: > On Fri, 14 Aug 2015, atull wrote: > >> On Fri, 14 Aug 2015, Moritz Fischer wrote: >> >> > Hi Alan, >> > >> > I've updated my Zynq driver (it can be found in an older version >> > against your v8 in the Xilinx tree, too) >> > >> > https://github.com/mfischer/linux/tree/alan-fpga-mgr-v10 >> >> Since we are both already using this and have been for a while now, I hope it >> can go up into the mainstream instead of continuing to exist only in Altera >> and Xilinx's git trees. Yeah, that was definitely my intention. I just held off submitting my driver for mainline, because your patchset was still sort of a moving target. And that would be like the 3rd layer of dependencies :-) The reason for inclusion into the Xilinx tree was so people can play around with it already. >> > > Hi Moritz, > > I fetched your git tree and took a look at your low level driver. > > I had a some feedback. write_complete() is a blocking call, waiting for the > FPGA to go into operating state and timing out (ETIMEDOUT) if necessary. The > fpga-mgr.c framework is assuming that when write_complete exits with status 0, > that means that the FPGA is in operating state. That's why it's proper for us > to add "mgr->state = FPGA_MGR_STATE_OPERATING" after write_complete returns > success as you noted. My suggestion is that your write_complete() should > check > status in this way. Whatever error codes it returns will get propagated. Fair enough, I had misunderstood the API then :-) Another option would have been to have the sysfs function actually query the state function instead of using the cached mgr->state value. I'll fix my driver ;-) I'll probably do something like #define zynq_fpga_poll_timeout(priv, addr, val, cond, sleep_us, timeout_us) \ readl_poll_timeout(priv->io_base + addr, val, cond, sleep_us, \ timeout_us) > > Also, I'm wondering how the simple-fpga-bus stuff looks to you now that you've > had it for a little while. To be honest I haven't played much with it aside from making sure it works. I had to submit another patchset for the Zynq's reset controller to make it work. The whole dt overlay is pretty cool, but the syntax took some getting used to. > > Thank, > Alan Thanks for your feedback, Moritz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v10 3/8] add fpga manager core
On Fri, 14 Aug 2015, atull wrote: > On Fri, 14 Aug 2015, Moritz Fischer wrote: > > > Hi Alan, > > > > I've updated my Zynq driver (it can be found in an older version > > against your v8 in the Xilinx tree, too) > > > > https://github.com/mfischer/linux/tree/alan-fpga-mgr-v10 > > Since we are both already using this and have been for a while now, I hope it > can go up into the mainstream instead of continuing to exist only in Altera > and Xilinx's git trees. > Hi Moritz, I fetched your git tree and took a look at your low level driver. I had a some feedback. write_complete() is a blocking call, waiting for the FPGA to go into operating state and timing out (ETIMEDOUT) if necessary. The fpga-mgr.c framework is assuming that when write_complete exits with status 0, that means that the FPGA is in operating state. That's why it's proper for us to add "mgr->state = FPGA_MGR_STATE_OPERATING" after write_complete returns success as you noted. My suggestion is that your write_complete() should check status in this way. Whatever error codes it returns will get propagated. Also, I'm wondering how the simple-fpga-bus stuff looks to you now that you've had it for a little while. Thank, Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v10 3/8] add fpga manager core
On Fri, 14 Aug 2015, Moritz Fischer wrote:
> Hi Alan,
>
> I've updated my Zynq driver (it can be found in an older version
> against your v8 in the Xilinx tree, too)
>
> https://github.com/mfischer/linux/tree/alan-fpga-mgr-v10
Since we are both already using this and have been for a while now, I hope it
can go up into the mainstream instead of continuing to exist only in Altera
and Xilinx's git trees.
>
> to use your v10 version of the patch. Either I'm using the API wrong ,
> or it never gets to the 'operating state'.
I'm sure you are doing it right.
> > + }
> > +
> > + /*
> > +* 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;
> > + }
> Maybe I'm misunderstanding something here. Shouldn't we set mgr->state
> = FPGA_MGR_STATE_OPERATING
> here, seen that the _show function below uses the mgr->state?
The FPGA gets programmed, but state wasn't getting updated.
Should have "mgr->state = FPGA_MGR_STATE_OPERATING" here.
Will add in v11.
Thanks for the review and the ack. If you see anything else that seems
wrong, please let me know.
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v10 3/8] add fpga manager core
Hi Alan, I've updated my Zynq driver (it can be found in an older version against your v8 in the Xilinx tree, too) https://github.com/mfischer/linux/tree/alan-fpga-mgr-v10 to use your v10 version of the patch. Either I'm using the API wrong , or it never gets to the 'operating state'. Comments inline below. On Thu, Aug 13, 2015 at 10:37 AM, wrote: > From: Alan Tull > > API to support programming FPGA. > > The following functions are exported as GPL: > * fpga_mgr_buf_load >Load fpga from image in buffer > > * fpga_mgr_firmware_load >Request firmware and load it to the FPGA. > > * fpga_mgr_register > * fpga_mgr_unregister >FPGA device drivers can be added by calling >fpga_mgr_register() to register a set of >fpga_manager_ops to do device specific stuff. > > * of_fpga_mgr_get > * fpga_mgr_put >Get/put a reference to a fpga manager. > > The following sysfs files are created: > * /sys/class/fpga_manager//name > Name of low level driver. > > * /sys/class/fpga_manager//state > State of fpga manager > > Signed-off-by: Alan Tull > Acked-by: Michal Simek > --- > v2: s/mangager/manager/ > check for invalid request_nr > add fpga reset interface > rework the state code > remove FPGA_MGR_FAIL flag > add _ERR states to fpga manager states enum > low level state op now returns a state enum value > initialize framework state from driver state op > remove unused fpga read stuff > merge sysfs.c into fpga-mgr.c > move suspend/resume from bus.c to fpga-mgr.c > > v3: Add struct device to fpga_manager (not as a pointer) > Add to_fpga_manager > Don't get irq in fpga-mgr.c (let low level driver do it) > remove from struct fpga_manager: nr, np, parent > get rid of fpga_mgr_get_new_minor() > simplify fpga_manager_register: > reorder parameters > use dev instead of pdev > get rid of code that used to make more sense when this > was a char driver, don't alloc_chrdev_region > use a mutex instead of flags > > v4: Move to drivers/staging > > v5: Make some things be static > Kconfig: add 'if FPGA' > Makefile: s/fpga-mgr-core.o/fpga-mgr.o/ > clean up includes > use enum fpga_mgr_states instead of int > static const char *state_str > use DEVICE_ATTR_RO/RW/WO and ATTRIBUTE_GROUPS > return -EINVAL instead of BUG_ON > move ida_simple_get after kzalloc > clean up fpga_mgr_remove > fpga-mgr.h: remove '#if IS_ENABLED(CONFIG_FPGA)' > add kernel-doc documentation > Move header to new include/linux/fpga folder > static const struct fpga_mgr_ops > dev_info msg simplified > > v6: no statically allocated string for image_name > kernel doc fixes > changes regarding enabling SYSFS for fpga mgr > Makefile cleanup > > v7: no change in this patch for v7 of the patchset > > v8: no change in this patch for v8 of the patchset > > v9: remove writable attributes 'firmware' and 'reset' > remove suspend/resume support for now > remove list of fpga managers; use class device iterators instead > simplify locking by giving out only one ref exclusively > add device tree support > add flags > par down API into fewer functions > update copyright year > rename some functions for clarity > clean up unnecessary #includes > add some error messages > write_init may need to look at buffer header, so add to params > > v10: Make this a tristate in Kconfig > pass flags parameter to write_complete > use BIT(0) in macro > move to drivers/fpga > fpga_manager_get/put call module_try_get/module_put > --- > drivers/Kconfig |2 + > drivers/Makefile |1 + > drivers/fpga/Kconfig | 14 ++ > drivers/fpga/Makefile |8 + > drivers/fpga/fpga-mgr.c | 381 > + > include/linux/fpga/fpga-mgr.h | 127 ++ > 6 files changed, 533 insertions(+) > create mode 100644 drivers/fpga/Kconfig > create mode 100644 drivers/fpga/Makefile > create mode 100644 drivers/fpga/fpga-mgr.c > create mode 100644 include/linux/fpga/fpga-mgr.h > > diff --git a/drivers/Kconfig b/drivers/Kconfig > index 6e973b8..2683346 100644 > --- a/drivers/Kconfig > +++ b/drivers/Kconfig > @@ -184,4 +184,6 @@ source "drivers/android/Kconfig" > > source "drivers/nvdimm/Kconfig" > > +source "drivers/fpga/Kconfig" > + > endmenu > diff --git a/drivers/Makefile b/drivers/Makefile > index b64b49f..832a6e0 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -165,3 +165,4 @@ obj-$(CONFIG_RAS) += ras/ > obj-$(CONFIG_THUNDERBOLT) += thunderbolt/ > obj-$(CONFIG_CORESIGHT)+= hwtracing/coresight/ > obj-$(CONFIG_ANDROID) += android/ > +obj-$(CONFIG_FPGA) += fpga/ > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > new file mode 100644 > index 000..f1f1f6d > --- /dev/null > +++ b/drivers/fpga/Kconfig >
[PATCH v10 3/8] add fpga manager core
From: Alan Tull API to support programming FPGA. The following functions are exported as GPL: * fpga_mgr_buf_load Load fpga from image in buffer * fpga_mgr_firmware_load Request firmware and load it to the FPGA. * fpga_mgr_register * fpga_mgr_unregister FPGA device drivers can be added by calling fpga_mgr_register() to register a set of fpga_manager_ops to do device specific stuff. * of_fpga_mgr_get * fpga_mgr_put Get/put a reference to a fpga manager. The following sysfs files are created: * /sys/class/fpga_manager//name Name of low level driver. * /sys/class/fpga_manager//state State of fpga manager Signed-off-by: Alan Tull Acked-by: Michal Simek --- v2: s/mangager/manager/ check for invalid request_nr add fpga reset interface rework the state code remove FPGA_MGR_FAIL flag add _ERR states to fpga manager states enum low level state op now returns a state enum value initialize framework state from driver state op remove unused fpga read stuff merge sysfs.c into fpga-mgr.c move suspend/resume from bus.c to fpga-mgr.c v3: Add struct device to fpga_manager (not as a pointer) Add to_fpga_manager Don't get irq in fpga-mgr.c (let low level driver do it) remove from struct fpga_manager: nr, np, parent get rid of fpga_mgr_get_new_minor() simplify fpga_manager_register: reorder parameters use dev instead of pdev get rid of code that used to make more sense when this was a char driver, don't alloc_chrdev_region use a mutex instead of flags v4: Move to drivers/staging v5: Make some things be static Kconfig: add 'if FPGA' Makefile: s/fpga-mgr-core.o/fpga-mgr.o/ clean up includes use enum fpga_mgr_states instead of int static const char *state_str use DEVICE_ATTR_RO/RW/WO and ATTRIBUTE_GROUPS return -EINVAL instead of BUG_ON move ida_simple_get after kzalloc clean up fpga_mgr_remove fpga-mgr.h: remove '#if IS_ENABLED(CONFIG_FPGA)' add kernel-doc documentation Move header to new include/linux/fpga folder static const struct fpga_mgr_ops dev_info msg simplified v6: no statically allocated string for image_name kernel doc fixes changes regarding enabling SYSFS for fpga mgr Makefile cleanup v7: no change in this patch for v7 of the patchset v8: no change in this patch for v8 of the patchset v9: remove writable attributes 'firmware' and 'reset' remove suspend/resume support for now remove list of fpga managers; use class device iterators instead simplify locking by giving out only one ref exclusively add device tree support add flags par down API into fewer functions update copyright year rename some functions for clarity clean up unnecessary #includes add some error messages write_init may need to look at buffer header, so add to params v10: Make this a tristate in Kconfig pass flags parameter to write_complete use BIT(0) in macro move to drivers/fpga fpga_manager_get/put call module_try_get/module_put --- drivers/Kconfig |2 + drivers/Makefile |1 + drivers/fpga/Kconfig | 14 ++ drivers/fpga/Makefile |8 + drivers/fpga/fpga-mgr.c | 381 + include/linux/fpga/fpga-mgr.h | 127 ++ 6 files changed, 533 insertions(+) create mode 100644 drivers/fpga/Kconfig create mode 100644 drivers/fpga/Makefile create mode 100644 drivers/fpga/fpga-mgr.c create mode 100644 include/linux/fpga/fpga-mgr.h diff --git a/drivers/Kconfig b/drivers/Kconfig index 6e973b8..2683346 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -184,4 +184,6 @@ source "drivers/android/Kconfig" source "drivers/nvdimm/Kconfig" +source "drivers/fpga/Kconfig" + endmenu diff --git a/drivers/Makefile b/drivers/Makefile index b64b49f..832a6e0 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -165,3 +165,4 @@ obj-$(CONFIG_RAS) += ras/ obj-$(CONFIG_THUNDERBOLT) += thunderbolt/ obj-$(CONFIG_CORESIGHT)+= hwtracing/coresight/ obj-$(CONFIG_ANDROID) += android/ +obj-$(CONFIG_FPGA) += fpga/ diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig new file mode 100644 index 000..f1f1f6d --- /dev/null +++ b/drivers/fpga/Kconfig @@ -0,0 +1,14 @@ +# +# FPGA framework configuration +# + +menu "FPGA Configuration Support" + +config FPGA + tristate "FPGA Configuration Framework" + help + Say Y here if you want support for configuring FPGAs from the + kernel. The FPGA framework adds a FPGA manager class and FPGA + manager drivers. + +endmenu diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile new file mode 100644 index 000..3313c52 --- /dev/null +++ b/drivers/fpga/Makefile @@ -0,0 +1,8 @@ +# +# Makefile for the fpga framework and fpga manager drivers. +# + +# Core FPGA Manager Framework +obj
