| Message ID | 20240726063819.2274324-2-nava.kishore.manne@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add user space interaction for FPGA programming | expand |
On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote: > Adds sysfs interface as part of the of-fpga-region. This newly added > sysfs interface uses Device Tree Overlay (DTO) files to configure/reprogram > an FPGA while an operating system is running.This solution will not change > the existing sequence When a DT overlay that targets an FPGA Region is > applied. > - Disable appropriate FPGA bridges. > - Program the FPGA using the FPGA manager. > - Enable the FPGA bridges. > - The Device Tree overlay is accepted into the live tree. > - Child devices are populated. > > When the overlay is removed, the child nodes will be removed, and the FPGA > Region will disable the bridges. > > Usage: > To configure/reprogram an FPGA region: > echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load IIRC, last time we are considering some generic interface for both OF & non-OF FPGA region, but this is still OF specific. Thanks, Yilun
Hi Yilun, > -----Original Message----- > From: Xu Yilun <yilun.xu@linux.intel.com> > Sent: Monday, July 29, 2024 9:27 PM > To: Manne, Nava kishore <nava.kishore.manne@amd.com> > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com; > yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; > saravanak@google.com; linux-fpga@vger.kernel.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support for FPGA > configuration > > On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote: > > Adds sysfs interface as part of the of-fpga-region. This newly added > > sysfs interface uses Device Tree Overlay (DTO) files to > > configure/reprogram an FPGA while an operating system is running.This > > solution will not change the existing sequence When a DT overlay that > > targets an FPGA Region is applied. > > - Disable appropriate FPGA bridges. > > - Program the FPGA using the FPGA manager. > > - Enable the FPGA bridges. > > - The Device Tree overlay is accepted into the live tree. > > - Child devices are populated. > > > > When the overlay is removed, the child nodes will be removed, and the > > FPGA Region will disable the bridges. > > > > Usage: > > To configure/reprogram an FPGA region: > > echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load > > IIRC, last time we are considering some generic interface for both OF & non- > OF FPGA region, but this is still OF specific. > At AMD, we exclusively use OF for FPGA configuration/reconfiguration, utilizing overlay files as outlined in the fpga-region.txt documentation. However, some devices, like dfl.c those relying solely on the FPGA region, do not use OF. For these non-OF devices, should we expect them to follow the fpga-region.txt guidelines for FPGA configuration/reconfiguration? If so, it may be advantageous to develop a common interface for both OF and non-OF. If not, it might be more appropriate to establish distinct interfaces to cater to their specific requirements. Regards, Navakishore.
On Thu, Aug 01, 2024 at 04:25:42AM +0000, Manne, Nava kishore wrote: > Hi Yilun, > > > -----Original Message----- > > From: Xu Yilun <yilun.xu@linux.intel.com> > > Sent: Monday, July 29, 2024 9:27 PM > > To: Manne, Nava kishore <nava.kishore.manne@amd.com> > > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com; > > yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; > > saravanak@google.com; linux-fpga@vger.kernel.org; linux- > > kernel@vger.kernel.org; devicetree@vger.kernel.org > > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support for FPGA > > configuration > > > > On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote: > > > Adds sysfs interface as part of the of-fpga-region. This newly added > > > sysfs interface uses Device Tree Overlay (DTO) files to > > > configure/reprogram an FPGA while an operating system is running.This > > > solution will not change the existing sequence When a DT overlay that > > > targets an FPGA Region is applied. > > > - Disable appropriate FPGA bridges. > > > - Program the FPGA using the FPGA manager. > > > - Enable the FPGA bridges. > > > - The Device Tree overlay is accepted into the live tree. > > > - Child devices are populated. > > > > > > When the overlay is removed, the child nodes will be removed, and the > > > FPGA Region will disable the bridges. > > > > > > Usage: > > > To configure/reprogram an FPGA region: > > > echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load > > > > IIRC, last time we are considering some generic interface for both OF & non- > > OF FPGA region, but this is still OF specific. > > > At AMD, we exclusively use OF for FPGA configuration/reconfiguration, utilizing overlay files as outlined in the fpga-region.txt documentation. > However, some devices, like dfl.c those relying solely on the FPGA region, do not use OF. > For these non-OF devices, should we expect them to follow the fpga-region.txt guidelines for FPGA configuration/reconfiguration? I assume it is Documentation/devicetree/bindings/fpga/fpga-region.yaml. No, Non-OF devices don't have to follow the DT binding. > If so, it may be advantageous to develop a common interface for both OF and non-OF. > If not, it might be more appropriate to establish distinct interfaces to cater to their specific requirements. I think each vendor may have specific way for device enumeration, but that doesn't mean we need distinct user interfaces. For all FPGA devices, we should avoid the situation that the HW is changed but system SW knows nothing. So the common needs are: - Find out and remove all devices within the fpga region before reprograming. - Re-enumerate devices in fpga region after reprograming. I expect the fpga region class could generally enforce a flow for the reprograming interface. And of-fpga-region could specifically implement it using DT overlay. Thanks, Yilun
> -----Original Message----- > From: Xu Yilun <yilun.xu@linux.intel.com> > Sent: Monday, August 5, 2024 11:51 PM > To: Manne, Nava kishore <nava.kishore.manne@amd.com> > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com; > yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com; > linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support for FPGA > configuration > > On Thu, Aug 01, 2024 at 04:25:42AM +0000, Manne, Nava kishore wrote: > > Hi Yilun, > > > > > -----Original Message----- > > > From: Xu Yilun <yilun.xu@linux.intel.com> > > > Sent: Monday, July 29, 2024 9:27 PM > > > To: Manne, Nava kishore <nava.kishore.manne@amd.com> > > > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; > > > hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com; > > > robh@kernel.org; saravanak@google.com; linux-fpga@vger.kernel.org; > > > linux- kernel@vger.kernel.org; devicetree@vger.kernel.org > > > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support > > > for FPGA configuration > > > > > > On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote: > > > > Adds sysfs interface as part of the of-fpga-region. This newly > > > > added sysfs interface uses Device Tree Overlay (DTO) files to > > > > configure/reprogram an FPGA while an operating system is > > > > running.This solution will not change the existing sequence When a > > > > DT overlay that targets an FPGA Region is applied. > > > > - Disable appropriate FPGA bridges. > > > > - Program the FPGA using the FPGA manager. > > > > - Enable the FPGA bridges. > > > > - The Device Tree overlay is accepted into the live tree. > > > > - Child devices are populated. > > > > > > > > When the overlay is removed, the child nodes will be removed, and > > > > the FPGA Region will disable the bridges. > > > > > > > > Usage: > > > > To configure/reprogram an FPGA region: > > > > echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load > > > > > > IIRC, last time we are considering some generic interface for both > > > OF & non- OF FPGA region, but this is still OF specific. > > > > > At AMD, we exclusively use OF for FPGA configuration/reconfiguration, utilizing > overlay files as outlined in the fpga-region.txt documentation. > > However, some devices, like dfl.c those relying solely on the FPGA region, do not > use OF. > > For these non-OF devices, should we expect them to follow the fpga-region.txt > guidelines for FPGA configuration/reconfiguration? > > I assume it is Documentation/devicetree/bindings/fpga/fpga-region.yaml. > > No, Non-OF devices don't have to follow the DT binding. > > > If so, it may be advantageous to develop a common interface for both OF and > non-OF. > > If not, it might be more appropriate to establish distinct interfaces to cater to their > specific requirements. > > I think each vendor may have specific way for device enumeration, but that doesn't > mean we need distinct user interfaces. For all FPGA devices, we should avoid the > situation that the HW is changed but system SW knows nothing. So the common > needs are: > > - Find out and remove all devices within the fpga region before > reprograming. > - Re-enumerate devices in fpga region after reprograming. > > I expect the fpga region class could generally enforce a flow for the reprograming > interface. And of-fpga-region could specifically implement it using DT overlay. > To address the vendor-specific nature(either of or non-of) of device enumeration in FPGA regions, As you suggested, we can develop a common programming interface that abstracts these vendor-specifc differences. This can be achieved by integrating vendor-specific callbacks(ex: of and non-of) for device configuration, enumeration and removal to fpga-region. I have outlined the top-level framework changes here: diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index b364a929425c..7d4b755dc8e0 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -213,6 +213,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info * region->compat_id = info->compat_id; region->priv = info->priv; region->get_bridges = info->get_bridges; + region->region_ops = info->region_ops; mutex_init(®ion->mutex); INIT_LIST_HEAD(®ion->bridge_list); @@ -257,17 +258,46 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full); */ struct fpga_region * fpga_region_register(struct device *parent, struct fpga_manager *mgr, + struct fpga_region_ops *region_ops, int (*get_bridges)(struct fpga_region *)) { struct fpga_region_info info = { 0 }; info.mgr = mgr; info.get_bridges = get_bridges; + info.region_ops = region_ops; return fpga_region_register_full(parent, &info); } EXPORT_SYMBOL_GPL(fpga_region_register); +static int fpga_region_device_open(struct inode *inode, struct file *file) { + struct miscdevice *miscdev = file->private_data; + struct fpga_region *region = container_of(miscdev, struct fpga_region, +miscdev); + + file->private_data = region; + + return 0; +} + +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { + char __user *argp = (char __user *)arg; + struct fpga_region *region = (struct fpga_region *)(file->private_data); + int err; + + switch (cmd) { + case FPGA_REGION_IOCTL_LOAD: + err = region->region_ops->fpga_region_config_enumerate (region, argp); + break; + case FPGA_REGION_IOCTL_REMOVE: + err = region->region_ops->fpga_region_remove(region, argp); + break; + case FPGA_REGION_IOCTL_STATUS: + err = region->region_ops->fpga_region_status(region, argp); + default: + err = -ENOTTY; +} + /** * fpga_region_unregister - unregister an FPGA region * @region: FPGA region diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h index 9d4d32909340..725fdcbab3d8 100644 --- a/include/linux/fpga/fpga-region.h +++ b/include/linux/fpga/fpga-region.h @@ -9,6 +9,20 @@ struct fpga_region; +/** + * struct fpga_region_ops - ops for low level FPGA region ops for +device + * enumeration/removal + * @region_status: returns the FPGA region status + * @region_config_enumeration: Configure and enumerate the FPGA region. + * @region_remove: Remove all devices within the fpga region + * (which are added as part of the enumeration). + */ +struct fpga_region_ops { + int (*region_status)(struct fpga_region *bridge); + int (*region_config_enumeration)(struct fpga_region *region, void *args); + void (*region_remove)(struct fpga_region *region, void *args); }; + /** * struct fpga_region_info - collection of parameters an FPGA Region * @mgr: fpga region manager @@ -26,6 +40,7 @@ struct fpga_region_info { struct fpga_compat_id *compat_id; void *priv; int (*get_bridges)(struct fpga_region *region); + struct fpga_region_ops *region_ops; }; /** @@ -48,6 +63,7 @@ struct fpga_region { struct fpga_compat_id *compat_id; void *priv; int (*get_bridges)(struct fpga_region *region); + struct fpga_region_ops *region_ops; }; #define to_fpga_region(d) container_of(d, struct fpga_region, dev) In this approach, we utilized an IOCTL-based user interface, but it doesn't have to be confined to IOCTL. We can also use sysfs or configfs, or other appropriate options as we finalized on it. This call-backs approach works for both OF and non-OF devices. If this aligns with your expectations, I can do the necessary changes for one vendor specific interface (of-fpga-region.c) devices and submit the RFC patch shortly. Regards, Navakishore.
On Tue, Sep 17, 2024 at 11:16:08AM +0000, Manne, Nava kishore wrote: > > -----Original Message----- > > From: Xu Yilun <yilun.xu@linux.intel.com> > > Sent: Monday, August 5, 2024 11:51 PM > > To: Manne, Nava kishore <nava.kishore.manne@amd.com> > > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com; > > yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com; > > linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org; > > devicetree@vger.kernel.org > > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support for FPGA > > configuration > > > > On Thu, Aug 01, 2024 at 04:25:42AM +0000, Manne, Nava kishore wrote: > > > Hi Yilun, > > > > > > > -----Original Message----- > > > > From: Xu Yilun <yilun.xu@linux.intel.com> > > > > Sent: Monday, July 29, 2024 9:27 PM > > > > To: Manne, Nava kishore <nava.kishore.manne@amd.com> > > > > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; > > > > hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com; > > > > robh@kernel.org; saravanak@google.com; linux-fpga@vger.kernel.org; > > > > linux- kernel@vger.kernel.org; devicetree@vger.kernel.org > > > > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support > > > > for FPGA configuration > > > > > > > > On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote: > > > > > Adds sysfs interface as part of the of-fpga-region. This newly > > > > > added sysfs interface uses Device Tree Overlay (DTO) files to > > > > > configure/reprogram an FPGA while an operating system is > > > > > running.This solution will not change the existing sequence When a > > > > > DT overlay that targets an FPGA Region is applied. > > > > > - Disable appropriate FPGA bridges. > > > > > - Program the FPGA using the FPGA manager. > > > > > - Enable the FPGA bridges. > > > > > - The Device Tree overlay is accepted into the live tree. > > > > > - Child devices are populated. > > > > > > > > > > When the overlay is removed, the child nodes will be removed, and > > > > > the FPGA Region will disable the bridges. > > > > > > > > > > Usage: > > > > > To configure/reprogram an FPGA region: > > > > > echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load > > > > > > > > IIRC, last time we are considering some generic interface for both > > > > OF & non- OF FPGA region, but this is still OF specific. > > > > > > > At AMD, we exclusively use OF for FPGA configuration/reconfiguration, utilizing > > overlay files as outlined in the fpga-region.txt documentation. > > > However, some devices, like dfl.c those relying solely on the FPGA region, do not > > use OF. > > > For these non-OF devices, should we expect them to follow the fpga-region.txt > > guidelines for FPGA configuration/reconfiguration? > > > > I assume it is Documentation/devicetree/bindings/fpga/fpga-region.yaml. > > > > No, Non-OF devices don't have to follow the DT binding. > > > > > If so, it may be advantageous to develop a common interface for both OF and > > non-OF. > > > If not, it might be more appropriate to establish distinct interfaces to cater to their > > specific requirements. > > > > I think each vendor may have specific way for device enumeration, but that doesn't > > mean we need distinct user interfaces. For all FPGA devices, we should avoid the > > situation that the HW is changed but system SW knows nothing. So the common > > needs are: > > > > - Find out and remove all devices within the fpga region before > > reprograming. > > - Re-enumerate devices in fpga region after reprograming. > > > > I expect the fpga region class could generally enforce a flow for the reprograming > > interface. And of-fpga-region could specifically implement it using DT overlay. > > > > To address the vendor-specific nature(either of or non-of) of device enumeration > in FPGA regions, As you suggested, we can develop a common programming > interface that abstracts these vendor-specifc differences. This can be achieved > by integrating vendor-specific callbacks(ex: of and non-of) for device configuration, > enumeration and removal to fpga-region. > > I have outlined the top-level framework changes here: > > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index b364a929425c..7d4b755dc8e0 100644 > --- a/drivers/fpga/fpga-region.c > +++ b/drivers/fpga/fpga-region.c > @@ -213,6 +213,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info * > region->compat_id = info->compat_id; > region->priv = info->priv; > region->get_bridges = info->get_bridges; > + region->region_ops = info->region_ops; > > mutex_init(®ion->mutex); > INIT_LIST_HEAD(®ion->bridge_list); > @@ -257,17 +258,46 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full); > */ > struct fpga_region * > fpga_region_register(struct device *parent, struct fpga_manager *mgr, > + struct fpga_region_ops *region_ops, > int (*get_bridges)(struct fpga_region *)) { > struct fpga_region_info info = { 0 }; > > info.mgr = mgr; > info.get_bridges = get_bridges; > + info.region_ops = region_ops; > > return fpga_region_register_full(parent, &info); } EXPORT_SYMBOL_GPL(fpga_region_register); > > +static int fpga_region_device_open(struct inode *inode, struct file *file) { > + struct miscdevice *miscdev = file->private_data; > + struct fpga_region *region = container_of(miscdev, struct fpga_region, > +miscdev); > + > + file->private_data = region; > + > + return 0; > +} > + > +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { > + char __user *argp = (char __user *)arg; > + struct fpga_region *region = (struct fpga_region *)(file->private_data); > + int err; > + > + switch (cmd) { > + case FPGA_REGION_IOCTL_LOAD: > + err = region->region_ops->fpga_region_config_enumerate (region, argp); Not sure "void *args" is a proposal or something yet to be decided. I think we should try best not to give up parameter type and have a clear API definition. > + break; > + case FPGA_REGION_IOCTL_REMOVE: > + err = region->region_ops->fpga_region_remove(region, argp); > + break; > + case FPGA_REGION_IOCTL_STATUS: > + err = region->region_ops->fpga_region_status(region, argp); > + default: > + err = -ENOTTY; > +} > + > /** > * fpga_region_unregister - unregister an FPGA region > * @region: FPGA region > diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h > index 9d4d32909340..725fdcbab3d8 100644 > --- a/include/linux/fpga/fpga-region.h > +++ b/include/linux/fpga/fpga-region.h > @@ -9,6 +9,20 @@ > > struct fpga_region; > > +/** > + * struct fpga_region_ops - ops for low level FPGA region ops for > +device > + * enumeration/removal > + * @region_status: returns the FPGA region status > + * @region_config_enumeration: Configure and enumerate the FPGA region. region config could be a common existing operation, fpga_region_program_fpga(). So maybe only enumeration is needed? > + * @region_remove: Remove all devices within the fpga region > + * (which are added as part of the enumeration). > + */ > +struct fpga_region_ops { > + int (*region_status)(struct fpga_region *bridge); > + int (*region_config_enumeration)(struct fpga_region *region, void *args); > + void (*region_remove)(struct fpga_region *region, void *args); }; > + > /** > * struct fpga_region_info - collection of parameters an FPGA Region > * @mgr: fpga region manager > @@ -26,6 +40,7 @@ struct fpga_region_info { > struct fpga_compat_id *compat_id; > void *priv; > int (*get_bridges)(struct fpga_region *region); > + struct fpga_region_ops *region_ops; > }; > > /** > @@ -48,6 +63,7 @@ struct fpga_region { > struct fpga_compat_id *compat_id; > void *priv; > int (*get_bridges)(struct fpga_region *region); > + struct fpga_region_ops *region_ops; > }; > > #define to_fpga_region(d) container_of(d, struct fpga_region, dev) > > In this approach, we utilized an IOCTL-based user interface, but it doesn't have > to be confined to IOCTL. We can also use sysfs or configfs, or other appropriate > options as we finalized on it. > > This call-backs approach works for both OF and non-OF devices. > If this aligns with your expectations, I can do the necessary changes There are still much to discuss, but yes this is a good start. Thanks, Yilun > for one vendor specific interface (of-fpga-region.c) devices and submit > the RFC patch shortly. > > > Regards, > Navakishore.
> -----Original Message----- > From: Xu Yilun <yilun.xu@linux.intel.com> > Sent: Wednesday, September 18, 2024 1:50 PM > To: Manne, Nava kishore <nava.kishore.manne@amd.com> > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com; > yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com; > linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support for FPGA > configuration > > On Tue, Sep 17, 2024 at 11:16:08AM +0000, Manne, Nava kishore wrote: > > > -----Original Message----- > > > From: Xu Yilun <yilun.xu@linux.intel.com> > > > Sent: Monday, August 5, 2024 11:51 PM > > > To: Manne, Nava kishore <nava.kishore.manne@amd.com> > > > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; > > > hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com; > > > robh@kernel.org; saravanak@google.com; linux-fpga@vger.kernel.org; > > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org > > > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support > > > for FPGA configuration > > > > > > On Thu, Aug 01, 2024 at 04:25:42AM +0000, Manne, Nava kishore wrote: > > > > Hi Yilun, > > > > > > > > > -----Original Message----- > > > > > From: Xu Yilun <yilun.xu@linux.intel.com> > > > > > Sent: Monday, July 29, 2024 9:27 PM > > > > > To: Manne, Nava kishore <nava.kishore.manne@amd.com> > > > > > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; > > > > > hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com; > > > > > robh@kernel.org; saravanak@google.com; > > > > > linux-fpga@vger.kernel.org; > > > > > linux- kernel@vger.kernel.org; devicetree@vger.kernel.org > > > > > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface > > > > > support for FPGA configuration > > > > > > > > > > On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote: > > > > > > Adds sysfs interface as part of the of-fpga-region. This newly > > > > > > added sysfs interface uses Device Tree Overlay (DTO) files to > > > > > > configure/reprogram an FPGA while an operating system is > > > > > > running.This solution will not change the existing sequence > > > > > > When a DT overlay that targets an FPGA Region is applied. > > > > > > - Disable appropriate FPGA bridges. > > > > > > - Program the FPGA using the FPGA manager. > > > > > > - Enable the FPGA bridges. > > > > > > - The Device Tree overlay is accepted into the live tree. > > > > > > - Child devices are populated. > > > > > > > > > > > > When the overlay is removed, the child nodes will be removed, > > > > > > and the FPGA Region will disable the bridges. > > > > > > > > > > > > Usage: > > > > > > To configure/reprogram an FPGA region: > > > > > > echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load > > > > > > > > > > IIRC, last time we are considering some generic interface for > > > > > both OF & non- OF FPGA region, but this is still OF specific. > > > > > > > > > At AMD, we exclusively use OF for FPGA > > > > configuration/reconfiguration, utilizing > > > overlay files as outlined in the fpga-region.txt documentation. > > > > However, some devices, like dfl.c those relying solely on the FPGA > > > > region, do not > > > use OF. > > > > For these non-OF devices, should we expect them to follow the > > > > fpga-region.txt > > > guidelines for FPGA configuration/reconfiguration? > > > > > > I assume it is Documentation/devicetree/bindings/fpga/fpga-region.yaml. > > > > > > No, Non-OF devices don't have to follow the DT binding. > > > > > > > If so, it may be advantageous to develop a common interface for > > > > both OF and > > > non-OF. > > > > If not, it might be more appropriate to establish distinct > > > > interfaces to cater to their > > > specific requirements. > > > > > > I think each vendor may have specific way for device enumeration, > > > but that doesn't mean we need distinct user interfaces. For all FPGA > > > devices, we should avoid the situation that the HW is changed but > > > system SW knows nothing. So the common needs are: > > > > > > - Find out and remove all devices within the fpga region before > > > reprograming. > > > - Re-enumerate devices in fpga region after reprograming. > > > > > > I expect the fpga region class could generally enforce a flow for > > > the reprograming interface. And of-fpga-region could specifically implement it > using DT overlay. > > > > > > > To address the vendor-specific nature(either of or non-of) of device > > enumeration in FPGA regions, As you suggested, we can develop a common > > programming interface that abstracts these vendor-specifc differences. > > This can be achieved by integrating vendor-specific callbacks(ex: of > > and non-of) for device configuration, enumeration and removal to fpga-region. > > > > I have outlined the top-level framework changes here: > > > > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > > index b364a929425c..7d4b755dc8e0 100644 > > --- a/drivers/fpga/fpga-region.c > > +++ b/drivers/fpga/fpga-region.c > > @@ -213,6 +213,7 @@ fpga_region_register_full(struct device *parent, const > struct fpga_region_info * > > region->compat_id = info->compat_id; > > region->priv = info->priv; > > region->get_bridges = info->get_bridges; > > + region->region_ops = info->region_ops; > > > > mutex_init(®ion->mutex); > > INIT_LIST_HEAD(®ion->bridge_list); > > @@ -257,17 +258,46 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full); > > */ > > struct fpga_region * > > fpga_region_register(struct device *parent, struct fpga_manager *mgr, > > + struct fpga_region_ops *region_ops, > > int (*get_bridges)(struct fpga_region *)) { > > struct fpga_region_info info = { 0 }; > > > > info.mgr = mgr; > > info.get_bridges = get_bridges; > > + info.region_ops = region_ops; > > > > return fpga_region_register_full(parent, &info); } > > EXPORT_SYMBOL_GPL(fpga_region_register); > > > > +static int fpga_region_device_open(struct inode *inode, struct file *file) { > > + struct miscdevice *miscdev = file->private_data; > > + struct fpga_region *region = container_of(miscdev, struct > > +fpga_region, miscdev); > > + > > + file->private_data = region; > > + > > + return 0; > > +} > > + > > +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, unsigned > long arg) { > > + char __user *argp = (char __user *)arg; > > + struct fpga_region *region = (struct fpga_region *)(file->private_data); > > + int err; > > + > > + switch (cmd) { > > + case FPGA_REGION_IOCTL_LOAD: > > + err = region->region_ops->fpga_region_config_enumerate (region, > > +argp); > > Not sure "void *args" is a proposal or something yet to be decided. > I think we should try best not to give up parameter type and have a clear API > definition. > I agree fixing the parameter type is necessary, and I will work on Implementing this change in the RFC. Following our discussion, If any additional modifications or clarifications are required, I will ensure to incorporate them in the later stages. > > + break; > > + case FPGA_REGION_IOCTL_REMOVE: > > + err = region->region_ops->fpga_region_remove(region, argp); > > + break; > > + case FPGA_REGION_IOCTL_STATUS: > > + err = region->region_ops->fpga_region_status(region, argp); > > + default: > > + err = -ENOTTY; > > +} > > + > > /** > > * fpga_region_unregister - unregister an FPGA region > > * @region: FPGA region > > diff --git a/include/linux/fpga/fpga-region.h > > b/include/linux/fpga/fpga-region.h > > index 9d4d32909340..725fdcbab3d8 100644 > > --- a/include/linux/fpga/fpga-region.h > > +++ b/include/linux/fpga/fpga-region.h > > @@ -9,6 +9,20 @@ > > > > struct fpga_region; > > > > +/** > > + * struct fpga_region_ops - ops for low level FPGA region ops for > > +device > > + * enumeration/removal > > + * @region_status: returns the FPGA region status > > + * @region_config_enumeration: Configure and enumerate the FPGA region. > > region config could be a common existing operation, fpga_region_program_fpga(). > So maybe only enumeration is needed? > I agree configuration and enumeration can be handled separately. However, for some vendors, pre-configuration details such as port IDs(AFU) and lower-level connected device information needs to be set before configuration. To accommodate both vendor-specific pre and post configuration (enumerations), I chose to use a single API for handling both FPGA configuration and enumeration. > > + * @region_remove: Remove all devices within the fpga region > > + * (which are added as part of the enumeration). > > + */ > > +struct fpga_region_ops { > > + int (*region_status)(struct fpga_region *bridge); > > + int (*region_config_enumeration)(struct fpga_region *region, void *args); > > + void (*region_remove)(struct fpga_region *region, void *args); }; > > + > > /** > > * struct fpga_region_info - collection of parameters an FPGA Region > > * @mgr: fpga region manager > > @@ -26,6 +40,7 @@ struct fpga_region_info { > > struct fpga_compat_id *compat_id; > > void *priv; > > int (*get_bridges)(struct fpga_region *region); > > + struct fpga_region_ops *region_ops; > > }; > > > > /** > > @@ -48,6 +63,7 @@ struct fpga_region { > > struct fpga_compat_id *compat_id; > > void *priv; > > int (*get_bridges)(struct fpga_region *region); > > + struct fpga_region_ops *region_ops; > > }; > > > > #define to_fpga_region(d) container_of(d, struct fpga_region, dev) > > > > In this approach, we utilized an IOCTL-based user interface, but it > > doesn't have to be confined to IOCTL. We can also use sysfs or > > configfs, or other appropriate options as we finalized on it. > > > > This call-backs approach works for both OF and non-OF devices. > > If this aligns with your expectations, I can do the necessary changes > > There are still much to discuss, but yes this is a good start. > Thanks for providing the quick confirmation will post RFC soon. Regards, Navakishore
diff --git a/Documentation/ABI/testing/sysfs-class-of-fpga-region b/Documentation/ABI/testing/sysfs-class-of-fpga-region new file mode 100644 index 000000000000..aeb4e3be4ff3 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-of-fpga-region @@ -0,0 +1,30 @@ +What: /sys/class/fpga_region/<region>/device/load +Date: July 2024 +KernelVersion: 6.10 +Contact: Nava kishore Manne <nava.kishore.manne@amd.com> +Description: (WO) Configure/Reprogram an FPGA region. + It uses Device Tree Overlay (DTO) files to configurer (or) + reprogram an FPGA. While an operating system is running. + The bitstream and the relevant DTO file has to be located + on the appropriate firmware path, typically, /lib/firmware. + For example, when user pass the option "echo fpga.dtbo" the + file /lib/firmware/fpga.dtbo must be present. + +What: /sys/class/fpga_region/<region>/device/remove +Date: July 2024 +KernelVersion: 6.10 +Contact: Nava kishore Manne <nava.kishore.manne@amd.com> +Description: (WO) Revert the changes added by the load interface + It revert and free an overlay changeset added by the load + interface. + +What: /sys/class/fpga_region/<region>/device/status +Date: July 2024 +KernelVersion: 6.10 +Contact: Nava kishore Manne <nava.kishore.manne@amd.com> +Description: (RO) Status of the FPGA region + This file is used to check the status of the FPGA region. + This is a list of strings for the supported status. + + * applied = FPGA is programmed and operating + * unapplied = Error while programing the FPGA diff --git a/MAINTAINERS b/MAINTAINERS index c0a3d9e93689..384f1d6f3af9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8795,6 +8795,7 @@ L: linux-fpga@vger.kernel.org S: Maintained Q: http://patchwork.kernel.org/project/linux-fpga/list/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/fpga/linux-fpga.git +F: Documentation/ABI/testing/sysfs-class-of-fpga-region F: Documentation/devicetree/bindings/fpga/ F: Documentation/driver-api/fpga/ F: Documentation/fpga/ diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index 753cd142503e..0733db1347ea 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -14,6 +14,7 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/of.h> static DEFINE_IDA(fpga_region_ida); static const struct class fpga_region_class; @@ -192,6 +193,7 @@ struct fpga_region * __fpga_region_register_full(struct device *parent, const struct fpga_region_info *info, struct module *owner) { + struct device_node *np = parent->of_node; struct fpga_region *region; int id, ret = 0; @@ -225,7 +227,7 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info region->dev.of_node = parent->of_node; region->dev.id = id; - ret = dev_set_name(®ion->dev, "region%d", id); + ret = dev_set_name(®ion->dev, "%s", of_node_full_name(np)); if (ret) goto err_remove; diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c index 8526a5a86f0c..edcc4c23a4b4 100644 --- a/drivers/fpga/of-fpga-region.c +++ b/drivers/fpga/of-fpga-region.c @@ -5,6 +5,7 @@ * Copyright (C) 2013-2016 Altera Corporation * Copyright (C) 2017 Intel Corporation */ +#include <linux/firmware.h> #include <linux/fpga/fpga-bridge.h> #include <linux/fpga/fpga-mgr.h> #include <linux/fpga/fpga-region.h> @@ -347,6 +348,7 @@ static int of_fpga_region_notify(struct notifier_block *nb, unsigned long action, void *arg) { struct of_overlay_notify_data *nd = arg; + struct fpga_overlay_image_info *ovcs; struct fpga_region *region; int ret; @@ -371,6 +373,10 @@ static int of_fpga_region_notify(struct notifier_block *nb, if (!region) return NOTIFY_OK; + ovcs = ®ion->ovcs; + if (!ovcs->fw) + return NOTIFY_STOP; + ret = 0; switch (action) { case OF_OVERLAY_PRE_APPLY: @@ -394,6 +400,91 @@ static struct notifier_block fpga_region_of_nb = { .notifier_call = of_fpga_region_notify, }; +static ssize_t load_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct fpga_region *region = to_fpga_region(dev); + struct fpga_overlay_image_info *ovcs = ®ion->ovcs; + char *s; + int err; + + /* if it's set do not allow changes */ + if (ovcs->ovcs_id) + return -EPERM; + + /* copy to path buffer (and make sure it's always zero terminated */ + count = snprintf(ovcs->path, sizeof(ovcs->path) - 1, "%s", buf); + ovcs->path[sizeof(ovcs->path) - 1] = '\0'; + + /* strip trailing newlines */ + s = ovcs->path + strlen(ovcs->path); + while (s > ovcs->path && *--s == '\n') + *s = '\0'; + + err = request_firmware(&ovcs->fw, ovcs->path, NULL); + if (err != 0) + goto out_err; + + err = of_overlay_fdt_apply((void *)ovcs->fw->data, ovcs->fw->size, + &ovcs->ovcs_id, NULL); + if (err < 0) { + pr_err("%s: Failed to create overlay (err=%d)\n", + __func__, err); + release_firmware(ovcs->fw); + goto out_err; + } + + return count; +out_err: + ovcs->path[0] = '\0'; + ovcs->ovcs_id = 0; + ovcs->fw = NULL; + + return err; +} + +static ssize_t remove_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct fpga_region *region = to_fpga_region(dev); + struct fpga_overlay_image_info *ovcs = ®ion->ovcs; + + if (!ovcs->ovcs_id) + return -EPERM; + + of_overlay_remove(&ovcs->ovcs_id); + release_firmware(ovcs->fw); + + ovcs->path[0] = '\0'; + ovcs->ovcs_id = 0; + ovcs->fw = NULL; + + return count; +} + +static ssize_t status_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct fpga_region *region = to_fpga_region(dev); + struct fpga_overlay_image_info *ovcs = ®ion->ovcs; + + return sprintf(buf, "%s\n", ovcs->ovcs_id > 0 ? + "applied" : "unapplied"); +} + +static DEVICE_ATTR_WO(load); +static DEVICE_ATTR_WO(remove); +static DEVICE_ATTR_RO(status); + +static struct attribute *of_fpga_region_attrs[] = { + &dev_attr_load.attr, + &dev_attr_remove.attr, + &dev_attr_status.attr, + NULL, +}; + +ATTRIBUTE_GROUPS(of_fpga_region); + static int of_fpga_region_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -440,6 +531,7 @@ static struct platform_driver of_fpga_region_driver = { .driver = { .name = "of-fpga-region", .of_match_table = of_match_ptr(fpga_region_of_match), + .dev_groups = of_fpga_region_groups, }, }; diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h index 5fbc05fe70a6..36143301b49b 100644 --- a/include/linux/fpga/fpga-region.h +++ b/include/linux/fpga/fpga-region.h @@ -9,6 +9,19 @@ struct fpga_region; +/** + * struct fpga_overlay_image_info - information specific to an FPGA Overlay + * image. + * @fw: firmware of coeff table. + * @path: path of FPGA overlay image firmware file. + * @ovcs_id: overlay changeset id. + */ +struct fpga_overlay_image_info { + const struct firmware *fw; + char path[PATH_MAX]; + int ovcs_id; +}; + /** * struct fpga_region_info - collection of parameters an FPGA Region * @mgr: fpga region manager @@ -37,6 +50,7 @@ struct fpga_region_info { * @info: FPGA image info * @compat_id: FPGA region id for compatibility check. * @ops_owner: module containing the get_bridges function + * @ovcs: FPGA overlay image info * @priv: private data * @get_bridges: optional function to get bridges to a list */ @@ -48,6 +62,7 @@ struct fpga_region { struct fpga_image_info *info; struct fpga_compat_id *compat_id; struct module *ops_owner; + struct fpga_overlay_image_info ovcs; void *priv; int (*get_bridges)(struct fpga_region *region); };
Adds sysfs interface as part of the of-fpga-region. This newly added sysfs interface uses Device Tree Overlay (DTO) files to configure/reprogram an FPGA while an operating system is running.This solution will not change the existing sequence When a DT overlay that targets an FPGA Region is applied. - Disable appropriate FPGA bridges. - Program the FPGA using the FPGA manager. - Enable the FPGA bridges. - The Device Tree overlay is accepted into the live tree. - Child devices are populated. When the overlay is removed, the child nodes will be removed, and the FPGA Region will disable the bridges. Usage: To configure/reprogram an FPGA region: echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load To remove an FPGA region: echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/remove To get an FPGA region status: cat /sys/class/fpga_region/<region>/device/status Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com> --- .../ABI/testing/sysfs-class-of-fpga-region | 30 ++++++ MAINTAINERS | 1 + drivers/fpga/fpga-region.c | 4 +- drivers/fpga/of-fpga-region.c | 92 +++++++++++++++++++ include/linux/fpga/fpga-region.h | 15 +++ 5 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-class-of-fpga-region