| Message ID | 1532672551-22146-1-git-send-email-appana.durga.rao@xilinx.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] fpga: fpga-mgr: Add readback support | expand |
On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com> wrote: Hi Appana, There should be some documentation for the debugfs added under Documentation/driver-api/fpga/ Also there are a lot of #ifdefs that were added due to the CONFIG_FPGA_MGR_DEBUG_FS. This has caused a kernel robot complaint. The way to fix that is to have a separate c file for the debugfs implementation. I see a lot of other kernel drivers have done it this way. I have an implementation that I haven't submitted yet, it exposes different functionality (exposing the image firmware file name and writing to the image file). It's on the downstream github.com/altera-opensource repo [1]. I'll clean this up and submit it to the mailing list, it may take a minute for me to get to that. Once that's done, your added functionality can be a patch on top of that. Alan [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-4.17/drivers/fpga/fpga-mgr-debugfs.c https://github.com/altera-opensource/linux-socfpga/blob/socfpga-4.17/drivers/fpga/fpga-mgr-debugfs.h > Inorder to debug issues with fpga's users would > like to read the fpga configuration information. > This patch adds readback support for fpga configuration data > in the framework through debugfs interface. > > Usage: > cat /sys/kernel/debug/fpga/fpga0/image > > Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com> > --- > Changes for v4: > --> None. > Changes for v3: > --> None. > Changes for v2: > --> Fixed debug attribute path and name as suggested by Alan > --> Add config entry for DEBUG as suggested by Alan > --> Fixed trival coding style issues. > > drivers/fpga/Kconfig | 7 +++++ > drivers/fpga/fpga-mgr.c | 68 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/fpga/fpga-mgr.h | 5 ++++ > 3 files changed, 80 insertions(+) > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > index 53d3f55..838ad4e 100644 > --- a/drivers/fpga/Kconfig > +++ b/drivers/fpga/Kconfig > @@ -11,6 +11,13 @@ menuconfig FPGA > > if FPGA > > +config FPGA_MGR_DEBUG_FS > + tristate "FPGA Debug fs" > + select DEBUG_FS > + help > + FPGA manager debug provides support for reading fpga configuration > + information. > + > config FPGA_MGR_SOCFPGA > tristate "Altera SOCFPGA FPGA Manager" > depends on ARCH_SOCFPGA || COMPILE_TEST > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > index 9939d2c..4bea860 100644 > --- a/drivers/fpga/fpga-mgr.c > +++ b/drivers/fpga/fpga-mgr.c > @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr) > } > EXPORT_SYMBOL_GPL(fpga_mgr_put); > > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS > +#include <linux/debugfs.h> > + > +static int fpga_mgr_read(struct seq_file *s, void *data) > +{ > + struct fpga_manager *mgr = (struct fpga_manager *)s->private; > + int ret = 0; > + > + if (!mgr->mops->read) > + return -ENOENT; > + > + if (!mutex_trylock(&mgr->ref_mutex)) > + return -EBUSY; > + > + if (mgr->state != FPGA_MGR_STATE_OPERATING) { > + ret = -EPERM; > + goto err_unlock; > + } > + > + /* Read the FPGA configuration data from the fabric */ > + ret = mgr->mops->read(mgr, s); > + if (ret) > + dev_err(&mgr->dev, "Error while reading configuration data from FPGA\n"); > + > +err_unlock: > + mutex_unlock(&mgr->ref_mutex); > + > + return ret; > +} > + > +static int fpga_mgr_read_open(struct inode *inode, struct file *file) > +{ > + return single_open(file, fpga_mgr_read, inode->i_private); > +} > + > +static const struct file_operations fpga_mgr_ops_image = { > + .owner = THIS_MODULE, > + .open = fpga_mgr_read_open, > + .read = seq_read, > +}; > +#endif > + > /** > * fpga_mgr_lock - Lock FPGA manager for exclusive use > * @mgr: fpga manager > @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name, > if (ret) > goto error_device; > > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS > + struct dentry *d, *parent; > + > + mgr->dir = debugfs_create_dir("fpga", NULL); > + if (!mgr->dir) > + goto error_device; > + > + parent = mgr->dir; > + d = debugfs_create_dir(mgr->dev.kobj.name, parent); > + if (!d) { > + debugfs_remove_recursive(parent); > + goto error_device; > + } > + > + parent = d; > + d = debugfs_create_file("image", 0644, parent, mgr, > + &fpga_mgr_ops_image); > + if (!d) { > + debugfs_remove_recursive(mgr->dir); > + goto error_device; > + } > +#endif > + > dev_info(&mgr->dev, "%s registered\n", mgr->name); > > return 0; > @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev) > > dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name); > > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS > + debugfs_remove_recursive(mgr->dir); > +#endif > /* > * If the low level driver provides a method for putting fpga into > * a desired state upon unregister, do it. > diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h > index 3c6de23..e9e17a9 100644 > --- a/include/linux/fpga/fpga-mgr.h > +++ b/include/linux/fpga/fpga-mgr.h > @@ -114,6 +114,7 @@ struct fpga_image_info { > * @write: write count bytes of configuration data to the FPGA > * @write_sg: write the scatter list of configuration data to the FPGA > * @write_complete: set FPGA to operating state after writing is done > + * @read: optional: read FPGA configuration information > * @fpga_remove: optional: Set FPGA into a specific state during driver remove > * @groups: optional attribute groups. > * > @@ -131,6 +132,7 @@ struct fpga_manager_ops { > int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt); > int (*write_complete)(struct fpga_manager *mgr, > struct fpga_image_info *info); > + int (*read)(struct fpga_manager *mgr, struct seq_file *s); > void (*fpga_remove)(struct fpga_manager *mgr); > const struct attribute_group **groups; > }; > @@ -151,6 +153,9 @@ struct fpga_manager { > enum fpga_mgr_states state; > const struct fpga_manager_ops *mops; > void *priv; > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS > + struct dentry *dir; > +#endif > }; > > #define to_fpga_manager(d) container_of(d, struct fpga_manager, 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 -- 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, Did you get a chance to send your framework changes to upstream? @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series?? Please let me know your thoughts on this. Regards, Kedar. > On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao > <appana.durga.rao@xilinx.com> wrote: > > Hi Appana, > > There should be some documentation for the debugfs added under > Documentation/driver-api/fpga/ > > Also there are a lot of #ifdefs that were added due to the > CONFIG_FPGA_MGR_DEBUG_FS. This has caused a kernel robot complaint. > The way to fix that is to have a separate c file for the debugfs implementation. > I see a lot of other kernel drivers have done it this way. I have an > implementation that I haven't submitted yet, it exposes different functionality > (exposing the image firmware file name and writing to the image file). It's on > the downstream github.com/altera-opensource repo [1]. I'll clean this up and > submit it to the mailing list, it may take a minute for me to get to that. > Once that's done, your added functionality can be a patch on top of that. > > Alan > > [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga- > 4.17/drivers/fpga/fpga-mgr-debugfs.c > https://github.com/altera-opensource/linux-socfpga/blob/socfpga- > 4.17/drivers/fpga/fpga-mgr-debugfs.h > > > > Inorder to debug issues with fpga's users would like to read the fpga > > configuration information. > > This patch adds readback support for fpga configuration data in the > > framework through debugfs interface. > > > > Usage: > > cat /sys/kernel/debug/fpga/fpga0/image > > > > Signed-off-by: Appana Durga Kedareswara rao > > <appana.durga.rao@xilinx.com> > > --- > > Changes for v4: > > --> None. > > Changes for v3: > > --> None. > > Changes for v2: > > --> Fixed debug attribute path and name as suggested by Alan Add > > --> config entry for DEBUG as suggested by Alan Fixed trival coding > > --> style issues. > > > > drivers/fpga/Kconfig | 7 +++++ > > drivers/fpga/fpga-mgr.c | 68 > +++++++++++++++++++++++++++++++++++++++++++ > > include/linux/fpga/fpga-mgr.h | 5 ++++ > > 3 files changed, 80 insertions(+) > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index > > 53d3f55..838ad4e 100644 > > --- a/drivers/fpga/Kconfig > > +++ b/drivers/fpga/Kconfig > > @@ -11,6 +11,13 @@ menuconfig FPGA > > > > if FPGA > > > > +config FPGA_MGR_DEBUG_FS > > + tristate "FPGA Debug fs" > > + select DEBUG_FS > > + help > > + FPGA manager debug provides support for reading fpga configuration > > + information. > > + > > config FPGA_MGR_SOCFPGA > > tristate "Altera SOCFPGA FPGA Manager" > > depends on ARCH_SOCFPGA || COMPILE_TEST diff --git > > a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index > > 9939d2c..4bea860 100644 > > --- a/drivers/fpga/fpga-mgr.c > > +++ b/drivers/fpga/fpga-mgr.c > > @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr) } > > EXPORT_SYMBOL_GPL(fpga_mgr_put); > > > > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS > > +#include <linux/debugfs.h> > > + > > +static int fpga_mgr_read(struct seq_file *s, void *data) { > > + struct fpga_manager *mgr = (struct fpga_manager *)s->private; > > + int ret = 0; > > + > > + if (!mgr->mops->read) > > + return -ENOENT; > > + > > + if (!mutex_trylock(&mgr->ref_mutex)) > > + return -EBUSY; > > + > > + if (mgr->state != FPGA_MGR_STATE_OPERATING) { > > + ret = -EPERM; > > + goto err_unlock; > > + } > > + > > + /* Read the FPGA configuration data from the fabric */ > > + ret = mgr->mops->read(mgr, s); > > + if (ret) > > + dev_err(&mgr->dev, "Error while reading configuration > > + data from FPGA\n"); > > + > > +err_unlock: > > + mutex_unlock(&mgr->ref_mutex); > > + > > + return ret; > > +} > > + > > +static int fpga_mgr_read_open(struct inode *inode, struct file *file) > > +{ > > + return single_open(file, fpga_mgr_read, inode->i_private); } > > + > > +static const struct file_operations fpga_mgr_ops_image = { > > + .owner = THIS_MODULE, > > + .open = fpga_mgr_read_open, > > + .read = seq_read, > > +}; > > +#endif > > + > > /** > > * fpga_mgr_lock - Lock FPGA manager for exclusive use > > * @mgr: fpga manager > > @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const > char *name, > > if (ret) > > goto error_device; > > > > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS > > + struct dentry *d, *parent; > > + > > + mgr->dir = debugfs_create_dir("fpga", NULL); > > + if (!mgr->dir) > > + goto error_device; > > + > > + parent = mgr->dir; > > + d = debugfs_create_dir(mgr->dev.kobj.name, parent); > > + if (!d) { > > + debugfs_remove_recursive(parent); > > + goto error_device; > > + } > > + > > + parent = d; > > + d = debugfs_create_file("image", 0644, parent, mgr, > > + &fpga_mgr_ops_image); > > + if (!d) { > > + debugfs_remove_recursive(mgr->dir); > > + goto error_device; > > + } > > +#endif > > + > > dev_info(&mgr->dev, "%s registered\n", mgr->name); > > > > return 0; > > @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev) > > > > dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name); > > > > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS > > + debugfs_remove_recursive(mgr->dir); > > +#endif > > /* > > * If the low level driver provides a method for putting fpga into > > * a desired state upon unregister, do it. > > diff --git a/include/linux/fpga/fpga-mgr.h > > b/include/linux/fpga/fpga-mgr.h index 3c6de23..e9e17a9 100644 > > --- a/include/linux/fpga/fpga-mgr.h > > +++ b/include/linux/fpga/fpga-mgr.h > > @@ -114,6 +114,7 @@ struct fpga_image_info { > > * @write: write count bytes of configuration data to the FPGA > > * @write_sg: write the scatter list of configuration data to the FPGA > > * @write_complete: set FPGA to operating state after writing is done > > + * @read: optional: read FPGA configuration information > > * @fpga_remove: optional: Set FPGA into a specific state during driver > remove > > * @groups: optional attribute groups. > > * > > @@ -131,6 +132,7 @@ struct fpga_manager_ops { > > int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt); > > int (*write_complete)(struct fpga_manager *mgr, > > struct fpga_image_info *info); > > + int (*read)(struct fpga_manager *mgr, struct seq_file *s); > > void (*fpga_remove)(struct fpga_manager *mgr); > > const struct attribute_group **groups; }; @@ -151,6 +153,9 @@ > > struct fpga_manager { > > enum fpga_mgr_states state; > > const struct fpga_manager_ops *mops; > > void *priv; > > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS > > + struct dentry *dir; > > +#endif > > }; > > > > #define to_fpga_manager(d) container_of(d, struct fpga_manager, 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 Kedar & Moritz, On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote: > Hi Alan, > > Did you get a chance to send your framework changes to upstream? > @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series?? > Please let me know your thoughts on this. > > Regards, > Kedar. I'd like to see some mechanism added as well. Our CvP driver needs a way to load images to the FPGA over the PCIe bus. It wasn't clear to me from the discussion on Alan's patchset[1] that the debugfs was acceptable to the mainline. I'd be happy to resurrect that patchset with the suggested changes. Since debugfs isn't enabled for production, are there any alternatives? Alan sent a RFC [2] for loading FIT images through the sysfs. The RFC described a FIT image that included FPGA image specific information to be included with the image (for systems running without device tree like our PCIe bus FPGA CvP). Unfortunately, I believe this has the same uphill battle that the Device Tree Overlay patches[3] have to getting accepted. Thor [1] https://patchwork.kernel.org/patch/10566865/ [2] https://patchwork.kernel.org/patch/9860183/ https://patchwork.kernel.org/patch/9860193/ https://patchwork.kernel.org/patch/9860191/ https://patchwork.kernel.org/patch/9860189/ https://patchwork.kernel.org/patch/9860187/ [3] https://lore.kernel.org/patchwork/cover/511851/ >> On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao >> <appana.durga.rao@xilinx.com> wrote: >> >> Hi Appana, >> >> There should be some documentation for the debugfs added under >> Documentation/driver-api/fpga/ >> >> Also there are a lot of #ifdefs that were added due to the >> CONFIG_FPGA_MGR_DEBUG_FS. This has caused a kernel robot complaint. >> The way to fix that is to have a separate c file for the debugfs implementation. >> I see a lot of other kernel drivers have done it this way. I have an >> implementation that I haven't submitted yet, it exposes different functionality >> (exposing the image firmware file name and writing to the image file). It's on >> the downstream github.com/altera-opensource repo [1]. I'll clean this up and >> submit it to the mailing list, it may take a minute for me to get to that. >> Once that's done, your added functionality can be a patch on top of that. >> >> Alan >> >> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga- >> 4.17/drivers/fpga/fpga-mgr-debugfs.c >> https://github.com/altera-opensource/linux-socfpga/blob/socfpga- >> 4.17/drivers/fpga/fpga-mgr-debugfs.h >> >> >>> Inorder to debug issues with fpga's users would like to read the fpga >>> configuration information. >>> This patch adds readback support for fpga configuration data in the >>> framework through debugfs interface. >>> >>> Usage: >>> cat /sys/kernel/debug/fpga/fpga0/image >>> >>> Signed-off-by: Appana Durga Kedareswara rao >>> <appana.durga.rao@xilinx.com> >>> --- >>> Changes for v4: >>> --> None. >>> Changes for v3: >>> --> None. >>> Changes for v2: >>> --> Fixed debug attribute path and name as suggested by Alan Add >>> --> config entry for DEBUG as suggested by Alan Fixed trival coding >>> --> style issues. >>> >>> drivers/fpga/Kconfig | 7 +++++ >>> drivers/fpga/fpga-mgr.c | 68 >> +++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/fpga/fpga-mgr.h | 5 ++++ >>> 3 files changed, 80 insertions(+) >>> >>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index >>> 53d3f55..838ad4e 100644 >>> --- a/drivers/fpga/Kconfig >>> +++ b/drivers/fpga/Kconfig >>> @@ -11,6 +11,13 @@ menuconfig FPGA >>> >>> if FPGA >>> >>> +config FPGA_MGR_DEBUG_FS >>> + tristate "FPGA Debug fs" >>> + select DEBUG_FS >>> + help >>> + FPGA manager debug provides support for reading fpga configuration >>> + information. >>> + >>> config FPGA_MGR_SOCFPGA >>> tristate "Altera SOCFPGA FPGA Manager" >>> depends on ARCH_SOCFPGA || COMPILE_TEST diff --git >>> a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index >>> 9939d2c..4bea860 100644 >>> --- a/drivers/fpga/fpga-mgr.c >>> +++ b/drivers/fpga/fpga-mgr.c >>> @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr) } >>> EXPORT_SYMBOL_GPL(fpga_mgr_put); >>> >>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS >>> +#include <linux/debugfs.h> >>> + >>> +static int fpga_mgr_read(struct seq_file *s, void *data) { >>> + struct fpga_manager *mgr = (struct fpga_manager *)s->private; >>> + int ret = 0; >>> + >>> + if (!mgr->mops->read) >>> + return -ENOENT; >>> + >>> + if (!mutex_trylock(&mgr->ref_mutex)) >>> + return -EBUSY; >>> + >>> + if (mgr->state != FPGA_MGR_STATE_OPERATING) { >>> + ret = -EPERM; >>> + goto err_unlock; >>> + } >>> + >>> + /* Read the FPGA configuration data from the fabric */ >>> + ret = mgr->mops->read(mgr, s); >>> + if (ret) >>> + dev_err(&mgr->dev, "Error while reading configuration >>> + data from FPGA\n"); >>> + >>> +err_unlock: >>> + mutex_unlock(&mgr->ref_mutex); >>> + >>> + return ret; >>> +} >>> + >>> +static int fpga_mgr_read_open(struct inode *inode, struct file *file) >>> +{ >>> + return single_open(file, fpga_mgr_read, inode->i_private); } >>> + >>> +static const struct file_operations fpga_mgr_ops_image = { >>> + .owner = THIS_MODULE, >>> + .open = fpga_mgr_read_open, >>> + .read = seq_read, >>> +}; >>> +#endif >>> + >>> /** >>> * fpga_mgr_lock - Lock FPGA manager for exclusive use >>> * @mgr: fpga manager >>> @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const >> char *name, >>> if (ret) >>> goto error_device; >>> >>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS >>> + struct dentry *d, *parent; >>> + >>> + mgr->dir = debugfs_create_dir("fpga", NULL); >>> + if (!mgr->dir) >>> + goto error_device; >>> + >>> + parent = mgr->dir; >>> + d = debugfs_create_dir(mgr->dev.kobj.name, parent); >>> + if (!d) { >>> + debugfs_remove_recursive(parent); >>> + goto error_device; >>> + } >>> + >>> + parent = d; >>> + d = debugfs_create_file("image", 0644, parent, mgr, >>> + &fpga_mgr_ops_image); >>> + if (!d) { >>> + debugfs_remove_recursive(mgr->dir); >>> + goto error_device; >>> + } >>> +#endif >>> + >>> dev_info(&mgr->dev, "%s registered\n", mgr->name); >>> >>> return 0; >>> @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev) >>> >>> dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name); >>> >>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS >>> + debugfs_remove_recursive(mgr->dir); >>> +#endif >>> /* >>> * If the low level driver provides a method for putting fpga into >>> * a desired state upon unregister, do it. >>> diff --git a/include/linux/fpga/fpga-mgr.h >>> b/include/linux/fpga/fpga-mgr.h index 3c6de23..e9e17a9 100644 >>> --- a/include/linux/fpga/fpga-mgr.h >>> +++ b/include/linux/fpga/fpga-mgr.h >>> @@ -114,6 +114,7 @@ struct fpga_image_info { >>> * @write: write count bytes of configuration data to the FPGA >>> * @write_sg: write the scatter list of configuration data to the FPGA >>> * @write_complete: set FPGA to operating state after writing is done >>> + * @read: optional: read FPGA configuration information >>> * @fpga_remove: optional: Set FPGA into a specific state during driver >> remove >>> * @groups: optional attribute groups. >>> * >>> @@ -131,6 +132,7 @@ struct fpga_manager_ops { >>> int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt); >>> int (*write_complete)(struct fpga_manager *mgr, >>> struct fpga_image_info *info); >>> + int (*read)(struct fpga_manager *mgr, struct seq_file *s); >>> void (*fpga_remove)(struct fpga_manager *mgr); >>> const struct attribute_group **groups; }; @@ -151,6 +153,9 @@ >>> struct fpga_manager { >>> enum fpga_mgr_states state; >>> const struct fpga_manager_ops *mops; >>> void *priv; >>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS >>> + struct dentry *dir; >>> +#endif >>> }; >>> >>> #define to_fpga_manager(d) container_of(d, struct fpga_manager, 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
Thor, On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote: > Hi Kedar & Moritz, > > On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote: > > Hi Alan, > > > > Did you get a chance to send your framework changes to upstream? No they weren't upstreamed. > > @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series?? > > Please let me know your thoughts on this. Alan had some comments RE: #defines, I'll have to take another look. > > > > Regards, > > Kedar. > > > I'd like to see some mechanism added as well. Our CvP driver needs a way to > load images to the FPGA over the PCIe bus. Can you elaborate a bit on the CvP use-case and how that would work? Who would use the device how after loading the bitstream? Generally there are several use cases that I have collected mentally over the years: I) DFL use case: - Mixed-set of drivers: Kernel and Userspace - FPGA logic is discoverable through DFL - Userspace application wants to reprogram FPGA II) DT configfs use case: - Mixed-set of drivers: Kernel and Userspace - FPGA logic is *not* discoverable (hence DT overlay) - Userspace application wants to reprogram FPGA III) Thomas' case: - Kernel only drivers (pcie bridge, pcie drivers, ...) - FPGA logic is fully discoverable (i.e. PCIe endpoint implemented in FPGA, connected to SoC via PCIe) - Userspace application wants to reprogram FPGA IV) VFIO case: - Usually exposes either entire device via vfio-pci or part via vfio-mdev - Loading (basic) bitstream at boot from flash - vfio-mdev case can use FPGA region interface + ioctl - Full VFIO case is similar to III) How does your CvP use case fit in? Collecting all the use-cases would help with moving forward on coming up with an API :) > > It wasn't clear to me from the discussion on Alan's patchset[1] that the > debugfs was acceptable to the mainline. I'd be happy to resurrect that > patchset with the suggested changes. Back then we decided to not move forward with the debugfs patchset since it's essentially cat foo.bin > /dev/xdevcfg / cat bar.rbf > /dev/fpga0 in disguise. Which is why I vetoed it back then. > Since debugfs isn't enabled for production, are there any alternatives? > > Alan sent a RFC [2] for loading FIT images through the sysfs. > > The RFC described a FIT image that included FPGA image specific information > to be included with the image (for systems running without device tree like > our PCIe bus FPGA CvP). Yeah I had originally suggested that as a mechanim to make FPGA images discoverable by the kernel. I still think the idea has merit, however it will run into the same issues that the configfs interface ran into w.r.t using dt-overlays. Generally I'd like to see a solution that exposes the *same* interface to DT and not DT systems to userspace. Using FIT headers one could go ahead and design something along the lines of what DFL is doing, except for instead of parsing the DFL in the logic, one would parse the FIT header to create subdevices. > Unfortunately, I believe this has the same uphill battle that the Device > Tree Overlay patches[3] have to getting accepted. See above. While I'm happy to discuss this more I atm don't have the bandwidth to push the DT work any further. Thanks, Moritz
Hi Moritz, On 9/27/19 1:23 PM, Moritz Fischer wrote: > Thor, > > On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote: >> Hi Kedar & Moritz, >> >> On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote: >>> Hi Alan, >>> >>> Did you get a chance to send your framework changes to upstream? > No they weren't upstreamed. > >>> @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series?? >>> Please let me know your thoughts on this. > > Alan had some comments RE: #defines, I'll have to take another look. >>> >>> Regards, >>> Kedar. >> >> >> I'd like to see some mechanism added as well. Our CvP driver needs a way to >> load images to the FPGA over the PCIe bus. > > Can you elaborate a bit on the CvP use-case and how that would work? Who > would use the device how after loading the bitstream? > > Generally there are several use cases that I have collected mentally > over the years: > > I) DFL use case: > - Mixed-set of drivers: Kernel and Userspace > - FPGA logic is discoverable through DFL > - Userspace application wants to reprogram FPGA > > II) DT configfs use case: > - Mixed-set of drivers: Kernel and Userspace > - FPGA logic is *not* discoverable (hence DT overlay) > - Userspace application wants to reprogram FPGA > > III) Thomas' case: > - Kernel only drivers (pcie bridge, pcie drivers, ...) > - FPGA logic is fully discoverable (i.e. PCIe endpoint > implemented in FPGA, connected to SoC via PCIe) > - Userspace application wants to reprogram FPGA > > IV) VFIO case: > - Usually exposes either entire device via vfio-pci or part via > vfio-mdev > - Loading (basic) bitstream at boot from flash > - vfio-mdev case can use FPGA region interface + ioctl > - Full VFIO case is similar to III) > > How does your CvP use case fit in? Collecting all the use-cases would > help with moving forward on coming up with an API :) > The CvP case is the same as III) Thomas' case. The FPGA configuration bitstream is downloaded over the PCIe. The one difference in my case is that there isn't an SoC. This is a Intel host processor connecting to a non-SoC Stratix10/Arria10. The non-SoC A10/S10, boots a minimal image (CvP) setting up the peripheral pins and enabling the PCIe endpoint for CvP downloads. The host can then download bitstreams using the FPGA Manager through debugFS and when the bitstream finishes downloading and the FPGA enters User Mode, the functionality is available for the host to use. >> >> It wasn't clear to me from the discussion on Alan's patchset[1] that the >> debugfs was acceptable to the mainline. I'd be happy to resurrect that >> patchset with the suggested changes. > > Back then we decided to not move forward with the debugfs patchset since > it's essentially cat foo.bin > /dev/xdevcfg / cat bar.rbf > /dev/fpga0 > in disguise. Which is why I vetoed it back then. > >> Since debugfs isn't enabled for production, are there any alternatives? >> >> Alan sent a RFC [2] for loading FIT images through the sysfs. >> >> The RFC described a FIT image that included FPGA image specific information >> to be included with the image (for systems running without device tree like >> our PCIe bus FPGA CvP). > > Yeah I had originally suggested that as a mechanim to make FPGA images > discoverable by the kernel. I still think the idea has merit, however it > will run into the same issues that the configfs interface ran into w.r.t > using dt-overlays. > > Generally I'd like to see a solution that exposes the *same* interface > to DT and not DT systems to userspace. > > Using FIT headers one could go ahead and design something along the > lines of what DFL is doing, except for instead of parsing the DFL in the > logic, one would parse the FIT header to create subdevices. > >> Unfortunately, I believe this has the same uphill battle that the Device >> Tree Overlay patches[3] have to getting accepted. > > See above. While I'm happy to discuss this more I atm don't have the > bandwidth to push the DT work any further. Understood. I'm still coming up to speed on the variations of FPGA enablement but I'm happy to help where I can. Thanks. > > Thanks, > Moritz >
Hi Thor, On Mon, Oct 07, 2019 at 01:06:51PM -0500, Thor Thayer wrote: > Hi Moritz, > > On 9/27/19 1:23 PM, Moritz Fischer wrote: > > Thor, > > > > On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote: > > > Hi Kedar & Moritz, > > > > > > On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote: > > > > Hi Alan, > > > > > > > > Did you get a chance to send your framework changes to upstream? > > No they weren't upstreamed. > > > > > > @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series?? > > > > Please let me know your thoughts on this. > > > > Alan had some comments RE: #defines, I'll have to take another look. > > > > > > > > Regards, > > > > Kedar. > > > > > > > > > I'd like to see some mechanism added as well. Our CvP driver needs a way to > > > load images to the FPGA over the PCIe bus. > > > > Can you elaborate a bit on the CvP use-case and how that would work? Who > > would use the device how after loading the bitstream? > > > > Generally there are several use cases that I have collected mentally > > over the years: > > > > I) DFL use case: > > - Mixed-set of drivers: Kernel and Userspace > > - FPGA logic is discoverable through DFL > > - Userspace application wants to reprogram FPGA > > > > II) DT configfs use case: > > - Mixed-set of drivers: Kernel and Userspace > > - FPGA logic is *not* discoverable (hence DT overlay) > > - Userspace application wants to reprogram FPGA > > > > III) Thomas' case: > > - Kernel only drivers (pcie bridge, pcie drivers, ...) > > - FPGA logic is fully discoverable (i.e. PCIe endpoint > > implemented in FPGA, connected to SoC via PCIe) > > - Userspace application wants to reprogram FPGA > > > > IV) VFIO case: > > - Usually exposes either entire device via vfio-pci or part via > > vfio-mdev > > - Loading (basic) bitstream at boot from flash > > - vfio-mdev case can use FPGA region interface + ioctl > > - Full VFIO case is similar to III) > > > > How does your CvP use case fit in? Collecting all the use-cases would > > help with moving forward on coming up with an API :) > > > The CvP case is the same as III) Thomas' case. The FPGA configuration > bitstream is downloaded over the PCIe. > > The one difference in my case is that there isn't an SoC. This is a Intel > host processor connecting to a non-SoC Stratix10/Arria10. The non-SoC > A10/S10, boots a minimal image (CvP) setting up the peripheral pins and > enabling the PCIe endpoint for CvP downloads. > > The host can then download bitstreams using the FPGA Manager through debugFS > and when the bitstream finishes downloading and the FPGA enters User Mode, > the functionality is available for the host to use. I am generally confused by this driver. How does it work exactly? What happens after altera-cvp binds a PCI device? You can use it to download a bitstream (say we had the debugfs interface), and then what happens next? How do I use the device? It already has a PCI driver bound to it at that point? What happens next? Please tell me that not the only use-case for this is /dev/mem :) Thomas' use-case is different in that behind the FPGA device there are actual other *discoverable* PCI devices that will get enumerated and bind to separate drivers. Thanks, Moritz PS: I'll be out this week on vacation starting tmr so responses might be delayed
Hi Moritz, On 10/7/19 4:20 PM, Moritz Fischer wrote: > Hi Thor, > > On Mon, Oct 07, 2019 at 01:06:51PM -0500, Thor Thayer wrote: >> Hi Moritz, >> >> On 9/27/19 1:23 PM, Moritz Fischer wrote: >>> Thor, >>> >>> On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote: >>>> Hi Kedar & Moritz, >>>> >>>> On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote: >>>>> Hi Alan, >>>>> >>>>> Did you get a chance to send your framework changes to upstream? >>> No they weren't upstreamed. >>> >>>>> @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series?? >>>>> Please let me know your thoughts on this. >>> >>> Alan had some comments RE: #defines, I'll have to take another look. >>>>> >>>>> Regards, >>>>> Kedar. >>>> >>>> >>>> I'd like to see some mechanism added as well. Our CvP driver needs a way to >>>> load images to the FPGA over the PCIe bus. >>> >>> Can you elaborate a bit on the CvP use-case and how that would work? Who >>> would use the device how after loading the bitstream? >>> >>> Generally there are several use cases that I have collected mentally >>> over the years: >>> >>> I) DFL use case: >>> - Mixed-set of drivers: Kernel and Userspace >>> - FPGA logic is discoverable through DFL >>> - Userspace application wants to reprogram FPGA >>> >>> II) DT configfs use case: >>> - Mixed-set of drivers: Kernel and Userspace >>> - FPGA logic is *not* discoverable (hence DT overlay) >>> - Userspace application wants to reprogram FPGA >>> >>> III) Thomas' case: >>> - Kernel only drivers (pcie bridge, pcie drivers, ...) >>> - FPGA logic is fully discoverable (i.e. PCIe endpoint >>> implemented in FPGA, connected to SoC via PCIe) >>> - Userspace application wants to reprogram FPGA >>> >>> IV) VFIO case: >>> - Usually exposes either entire device via vfio-pci or part via >>> vfio-mdev >>> - Loading (basic) bitstream at boot from flash >>> - vfio-mdev case can use FPGA region interface + ioctl >>> - Full VFIO case is similar to III) >>> >>> How does your CvP use case fit in? Collecting all the use-cases would >>> help with moving forward on coming up with an API :) >>> >> The CvP case is the same as III) Thomas' case. The FPGA configuration >> bitstream is downloaded over the PCIe. >> >> The one difference in my case is that there isn't an SoC. This is a Intel >> host processor connecting to a non-SoC Stratix10/Arria10. The non-SoC >> A10/S10, boots a minimal image (CvP) setting up the peripheral pins and >> enabling the PCIe endpoint for CvP downloads. >> >> The host can then download bitstreams using the FPGA Manager through debugFS >> and when the bitstream finishes downloading and the FPGA enters User Mode, >> the functionality is available for the host to use. > > I am generally confused by this driver. How does it work exactly? What > happens after altera-cvp binds a PCI device? > > You can use it to download a bitstream (say we had the debugfs > interface), and then what happens next? How do I use the device? It > already has a PCI driver bound to it at that point? Sorry for the delay. In the CvP case, I reboot the host device leaving the FPGA board powered so the new PCI interface is enumerated. There may be a better way. I'll need to research Thomas' use case. There may be some good lessons to learn there. Thanks, Thor > > What happens next? > > Please tell me that not the only use-case for this is /dev/mem :) > > Thomas' use-case is different in that behind the FPGA device there are > actual other *discoverable* PCI devices that will get enumerated and > bind to separate drivers. > > Thanks, > Moritz > > PS: I'll be out this week on vacation starting tmr so responses might be delayed >
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index 53d3f55..838ad4e 100644 --- a/drivers/fpga/Kconfig +++ b/drivers/fpga/Kconfig @@ -11,6 +11,13 @@ menuconfig FPGA if FPGA +config FPGA_MGR_DEBUG_FS + tristate "FPGA Debug fs" + select DEBUG_FS + help + FPGA manager debug provides support for reading fpga configuration + information. + config FPGA_MGR_SOCFPGA tristate "Altera SOCFPGA FPGA Manager" depends on ARCH_SOCFPGA || COMPILE_TEST diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index 9939d2c..4bea860 100644 --- a/drivers/fpga/fpga-mgr.c +++ b/drivers/fpga/fpga-mgr.c @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr) } EXPORT_SYMBOL_GPL(fpga_mgr_put); +#ifdef CONFIG_FPGA_MGR_DEBUG_FS +#include <linux/debugfs.h> + +static int fpga_mgr_read(struct seq_file *s, void *data) +{ + struct fpga_manager *mgr = (struct fpga_manager *)s->private; + int ret = 0; + + if (!mgr->mops->read) + return -ENOENT; + + if (!mutex_trylock(&mgr->ref_mutex)) + return -EBUSY; + + if (mgr->state != FPGA_MGR_STATE_OPERATING) { + ret = -EPERM; + goto err_unlock; + } + + /* Read the FPGA configuration data from the fabric */ + ret = mgr->mops->read(mgr, s); + if (ret) + dev_err(&mgr->dev, "Error while reading configuration data from FPGA\n"); + +err_unlock: + mutex_unlock(&mgr->ref_mutex); + + return ret; +} + +static int fpga_mgr_read_open(struct inode *inode, struct file *file) +{ + return single_open(file, fpga_mgr_read, inode->i_private); +} + +static const struct file_operations fpga_mgr_ops_image = { + .owner = THIS_MODULE, + .open = fpga_mgr_read_open, + .read = seq_read, +}; +#endif + /** * fpga_mgr_lock - Lock FPGA manager for exclusive use * @mgr: fpga manager @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name, if (ret) goto error_device; +#ifdef CONFIG_FPGA_MGR_DEBUG_FS + struct dentry *d, *parent; + + mgr->dir = debugfs_create_dir("fpga", NULL); + if (!mgr->dir) + goto error_device; + + parent = mgr->dir; + d = debugfs_create_dir(mgr->dev.kobj.name, parent); + if (!d) { + debugfs_remove_recursive(parent); + goto error_device; + } + + parent = d; + d = debugfs_create_file("image", 0644, parent, mgr, + &fpga_mgr_ops_image); + if (!d) { + debugfs_remove_recursive(mgr->dir); + goto error_device; + } +#endif + dev_info(&mgr->dev, "%s registered\n", mgr->name); return 0; @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev) dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name); +#ifdef CONFIG_FPGA_MGR_DEBUG_FS + debugfs_remove_recursive(mgr->dir); +#endif /* * If the low level driver provides a method for putting fpga into * a desired state upon unregister, do it. diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h index 3c6de23..e9e17a9 100644 --- a/include/linux/fpga/fpga-mgr.h +++ b/include/linux/fpga/fpga-mgr.h @@ -114,6 +114,7 @@ struct fpga_image_info { * @write: write count bytes of configuration data to the FPGA * @write_sg: write the scatter list of configuration data to the FPGA * @write_complete: set FPGA to operating state after writing is done + * @read: optional: read FPGA configuration information * @fpga_remove: optional: Set FPGA into a specific state during driver remove * @groups: optional attribute groups. * @@ -131,6 +132,7 @@ struct fpga_manager_ops { int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt); int (*write_complete)(struct fpga_manager *mgr, struct fpga_image_info *info); + int (*read)(struct fpga_manager *mgr, struct seq_file *s); void (*fpga_remove)(struct fpga_manager *mgr); const struct attribute_group **groups; }; @@ -151,6 +153,9 @@ struct fpga_manager { enum fpga_mgr_states state; const struct fpga_manager_ops *mops; void *priv; +#ifdef CONFIG_FPGA_MGR_DEBUG_FS + struct dentry *dir; +#endif }; #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
Inorder to debug issues with fpga's users would like to read the fpga configuration information. This patch adds readback support for fpga configuration data in the framework through debugfs interface. Usage: cat /sys/kernel/debug/fpga/fpga0/image Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com> --- Changes for v4: --> None. Changes for v3: --> None. Changes for v2: --> Fixed debug attribute path and name as suggested by Alan --> Add config entry for DEBUG as suggested by Alan --> Fixed trival coding style issues. drivers/fpga/Kconfig | 7 +++++ drivers/fpga/fpga-mgr.c | 68 +++++++++++++++++++++++++++++++++++++++++++ include/linux/fpga/fpga-mgr.h | 5 ++++ 3 files changed, 80 insertions(+)