Message ID | 20241029091734.3288005-2-nava.kishore.manne@amd.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | Add user space interaction for FPGA programming | expand |
On Thu, Dec 19, 2024 at 09:47:12AM +0000, Manne, Nava kishore wrote: > Hi Yilun, > > > -----Original Message----- > > From: Xu Yilun <yilun.xu@linux.intel.com> > > Sent: Tuesday, December 10, 2024 2:34 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-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; > > devicetree@vger.kernel.org > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime > > FPGA programming > > > > On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote: > > > Hi Yilun, > > > > > > > -----Original Message----- > > > > From: Xu Yilun <yilun.xu@linux.intel.com> > > > > Sent: Wednesday, November 27, 2024 7:20 AM > > > > 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-kernel@vger.kernel.org; > > > > linux-fpga@vger.kernel.org; devicetree@vger.kernel.org > > > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface > > > > for runtime FPGA programming > > > > > > > > > > > + * 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 *region); > > > > > > > + int (*region_config_enumeration)(struct fpga_region *region, > > > > > > > + struct fpga_region_config_info > > *config_info); > > > > > > > > > > > > My current concern is still about this combined API, it just > > > > > > offloads all work to low level, but we have some common flows. > > > > > > That's why we introduce a common FPGA reprograming API. > > > > > > > > > > > > I didn't see issue about the vendor specific pre configuration. > > > > > > They are generally needed to initialize the struct > > > > > > fpga_image_info, which is a common structure for > > fpga_region_program_fpga(). > > > > > > > > > > > > For port IDs(AFU) inputs for DFL, I think it could also be > > > > > > changed (Don't have to be implemented in this patchset). > > > > > > Previously DFL provides an uAPI for the whole device, so it > > > > > > needs a port_id input to position which fpga_region within the > > > > > > device for programming. But now, we are introducing a per > > > > > > fpga_region programming interface, IIUC port_id > > > > should not be needed anymore. > > > > > > > > > > > > The combined API is truly simple for leveraging the existing > > > > > > of-fpga-region overlay apply mechanism. But IMHO that flow > > > > > > doesn't fit our new uAPI well. That flow is to adapt the generic > > > > > > configfs overlay interface, which comes to a dead end as you mentioned. > > > > > > > > > > > > My gut feeling for the generic programing flow should be: > > > > > > > > > > > > 1. Program the image to HW. > > > > > > 2. Enumerate the programmed image (apply the DT overlay) > > > > > > > > > > > > Why we have to: > > > > > > > > > > > > 1. Start enumeration. > > > > > > 2. On pre enumeration, programe the image. > > > > > > 3. Real enumeration. > > > > > > > > > > > > > > > > I agree with the approach of leveraging vendor-specific callbacks > > > > > to handle the distinct phases of the FPGA programming process. > > > > > Here's the proposed flow. > > > > > > > > > > Pre-Configuration: > > > > > A vendor-specific callback extracts the required pre-configuration > > > > > details and initializes struct fpga_image_info. This ensures that > > > > > all vendor-specific > > > > > > > > Since we need to construct the fpga_image_info, initialize multiple > > > > field as needed, I'm wondering if configfs could be a solution for the uAPI? > > > > > > > > > > A configfs uAPI isn't necessary, we can manage this using the proposed IOCTL > > flow. > > > The POC code looks as follows. > > > > I prefer more to configfs cause it provides standard FS way to create the > > fpga_image_info object, e.g. which attributes are visible for OF/non-OF region, which > > attributes come from image blob and can only be RO, etc. > > > > Of couse ioctl() could achieve the same goal but would add much more specific rules > > (maybe flags/types) for user to follow. > > > > Agreed. Using ConfigFS is preferable because it provides a standardized filesystem > interface for creating and managing the fpga_image_info object. > > The proposed new user interface is outlined as follows: > > # Mount ConfigFS filesystem > mount -t configfs none /sys/kernel/config > > # Upload Configuration and Load the Bitstream for the Targeted FPGA Region. > > Configuration File Upload: > Upload the configuration file containing the necessary metadata or settings required > for configuring the FPGA region. This file may vary based on the vendor and includes > important details specific to the vendor's requirements. > > Vendor-Specific Callback: > A vendor-specific callback function extracts the relevant configuration data from the file. > The format and contents of the configuration file can differ between vendors. The callback > then initializes the struct fpga_image_info, ensuring all vendor-specific requirements are > satisfied. > > Device-Specific Considerations: > For Open Firmware (OF) devices, fpga.dtbo files are used instead of fpga_config files. > These .dtbo files contain all necessary information to populate the fpga_image_info. > For non-OF devices, a vendor specific fpga.config files are used to provide the required > data for initializing the fpga_image_info. non-OF fpga images usually don't contain fpga_image_info data (e.g. enable/disable_timeout_us). I think we don't have to force users embed these data in fpga image, provide additional configfs attributes to input these data is possible. For some FPGA regions (e.g. OF), these attributes could be RO, some could be RW, depends on different FPGA region drivers. So I think we may have a Configuration File Upload interface, like: echo "config_file" > /sys/kernel/config/fpga/<region>/image Some additional parameter interfaces, like: echo 10000 > /sys/kernel/config/fpga/<region>/enable_timeout ... And a Configuration interface, like: # programming echo 1 > /sys/kernel/config/fpga/<region>/config # removing echo 0 > /sys/kernel/config/fpga/<region>/config How do you think? Thanks, Yilun > > FPGA Configuration: > Once the configuration details are extracted and the fpga_image_info structure is initialized, > the FPGA can be programmed accordingly. > > echo "config_file" > /sys/kernel/config/fpga/<region>/config > > > # Check the status of "region" > cat /sys/kernel/config/fpga/<region>/status > > # Remove "region" > echo "remove" > /sys/kernel/config/fpga/<region>/remove > > Looking forward to your feedback. > > Regards, > Navakishore. >
On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote: > Introduces an IOCTL interface within the fpga-region subsystem, > providing a generic and standardized mechanism for configuring (or) > reprogramming FPGAs during runtime. The newly added interface supports > both OF (Open Firmware) and non-OF devices, leveraging vendor-specific > callbacks (e.g., configuration + enumeration, removal, and status) to > accommodate a wide range of device specific configurations. > > The IOCTL interface ensures compatibility with both OF and non-OF > devices, allowing for seamless FPGA reprogramming across diverse > platforms. > > Vendor-specific callbacks are integrated into the interface, enabling > custom FPGA configuration + enumeration, removal, and status reporting > mechanisms, ensuring flexibility for vendor implementations. > > This solution enhances FPGA runtime management, supporting various device > types and vendors, while ensuring compatibility with the current FPGA > configuration flow. > > Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com> > --- > Changes for v2: > - As discussed with Yilun, the implementation has been modified to utilize a > callback approach, enabling seamless handling of both OF and non-OF devices. > > - As suggested by Yilun in the POC code, we have moved away from using void *args > as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are > utilizing the fpga_region_config_info structure to gather user inputs. Currently, > this structure is implemented to support only OF devices, but we intend to extend > it by incorporating new members to accommodate non-OF devices in the future. > > drivers/fpga/fpga-region.c | 110 +++++++++++++++++++++++++++++++ > drivers/fpga/of-fpga-region.c | 91 ++++++++++++++++++++++++- > include/linux/fpga/fpga-region.h | 32 +++++++++ > include/uapi/linux/fpga-region.h | 51 ++++++++++++++ > 4 files changed, 283 insertions(+), 1 deletion(-) > create mode 100644 include/uapi/linux/fpga-region.h > > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > index 753cd142503e..c6bea3c99a69 100644 > --- a/drivers/fpga/fpga-region.c > +++ b/drivers/fpga/fpga-region.c > @@ -8,6 +8,7 @@ > #include <linux/fpga/fpga-bridge.h> > #include <linux/fpga/fpga-mgr.h> > #include <linux/fpga/fpga-region.h> > +#include <linux/fpga-region.h> > #include <linux/idr.h> > #include <linux/kernel.h> > #include <linux/list.h> > @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = { > }; > ATTRIBUTE_GROUPS(fpga_region); > > +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 int fpga_region_device_release(struct inode *inode, struct file *file) > +{ > + return 0; > +} > + > +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + int err; > + void __user *argp = (void __user *)arg; > + struct fpga_region_config_info config_info; > + struct fpga_region *region = (struct fpga_region *)(file->private_data); > + > + switch (cmd) { > + case FPGA_REGION_IOCTL_LOAD: > + if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) > + return -EFAULT; > + > + err = region->region_ops->region_config_enumeration(region, &config_info); > + > + break; > + case FPGA_REGION_IOCTL_REMOVE: > + if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) > + return -EFAULT; > + > + err = region->region_ops->region_remove(region, &config_info); > + > + break; > + case FPGA_REGION_IOCTL_STATUS: > + unsigned int status; > + > + status = region->region_ops->region_status(region); > + > + if (copy_to_user((void __user *)arg, &status, sizeof(status))) > + err = -EFAULT; > + break; > + default: > + err = -ENOTTY; > + } > + > + return err; > +} > + > +static const struct file_operations fpga_region_fops = { > + .owner = THIS_MODULE, > + .open = fpga_region_device_open, > + .release = fpga_region_device_release, > + .unlocked_ioctl = fpga_region_device_ioctl, > + .compat_ioctl = fpga_region_device_ioctl, > +}; > + > /** > * __fpga_region_register_full - create and register an FPGA Region device > * @parent: device parent > @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info > if (ret) > goto err_remove; > > + if (info->region_ops) { > + region->region_ops = info->region_ops; > + region->miscdev.minor = MISC_DYNAMIC_MINOR; > + region->miscdev.name = kobject_name(®ion->dev.kobj); > + region->miscdev.fops = &fpga_region_fops; > + ret = misc_register(®ion->miscdev); > + if (ret) { > + pr_err("fpga-region: failed to register misc device.\n"); > + goto err_remove; > + } > + } > + > ret = device_register(®ion->dev); > if (ret) { > + misc_deregister(®ion->miscdev); > put_device(®ion->dev); > return ERR_PTR(ret); > } > @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr, > } > EXPORT_SYMBOL_GPL(__fpga_region_register); > > +/** > + * __fpga_region_register_with_ops - create and register an FPGA Region device > + * with user interface call-backs. > + * @parent: device parent > + * @mgr: manager that programs this region > + * @region_ops: ops for low level FPGA region for device enumeration/removal > + * @priv: of-fpga-region private data > + * @get_bridges: optional function to get bridges to a list > + * @owner: module containing the get_bridges function > + * > + * This simple version of the register function should be sufficient for most users. > + * The fpga_region_register_full() function is available for users that need to > + * pass additional, optional parameters. > + * > + * Return: struct fpga_region or ERR_PTR() > + */ > +struct fpga_region * > +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr, > + const struct fpga_region_ops *region_ops, > + void *priv, > + int (*get_bridges)(struct fpga_region *), > + struct module *owner) > +{ > + struct fpga_region_info info = { 0 }; > + > + info.mgr = mgr; > + info.priv = priv; > + info.get_bridges = get_bridges; > + info.region_ops = region_ops; > + > + return __fpga_region_register_full(parent, &info, owner); > +} > +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops); > + > /** > * fpga_region_unregister - unregister an FPGA region > * @region: FPGA region > @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register); > */ > void fpga_region_unregister(struct fpga_region *region) > { > + misc_deregister(®ion->miscdev); > device_unregister(®ion->dev); > } > EXPORT_SYMBOL_GPL(fpga_region_unregister); > diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c > index 8526a5a86f0c..63fe56e0466f 100644 > --- a/drivers/fpga/of-fpga-region.c > +++ b/drivers/fpga/of-fpga-region.c > @@ -8,6 +8,8 @@ > #include <linux/fpga/fpga-bridge.h> > #include <linux/fpga/fpga-mgr.h> > #include <linux/fpga/fpga-region.h> > +#include <linux/firmware.h> > +#include <linux/fpga-region.h> > #include <linux/idr.h> > #include <linux/kernel.h> > #include <linux/list.h> > @@ -18,6 +20,20 @@ > #include <linux/slab.h> > #include <linux/spinlock.h> > > +/** > + * struct of_fpga_region_priv - Private data structure > + * image. > + * @dev: Device data structure > + * @fw: firmware of coeff table. > + * @path: path of FPGA overlay image firmware file. > + * @ovcs_id: overlay changeset id. > + */ > +struct of_fpga_region_priv { > + struct device *dev; > + const struct firmware *fw; > + int ovcs_id; > +}; > + > static const struct of_device_id fpga_region_of_match[] = { > { .compatible = "fpga-region", }, > {}, > @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = { > .notifier_call = of_fpga_region_notify, > }; > > +static int of_fpga_region_status(struct fpga_region *region) > +{ > + struct of_fpga_region_priv *ovcs = region->priv; > + > + if (ovcs->ovcs_id) > + return FPGA_REGION_HAS_PL; Could you help specify what is PL? > + > + return FPGA_REGION_EMPTY; > +} > + > +static int of_fpga_region_config_enumeration(struct fpga_region *region, > + struct fpga_region_config_info *config_info) > +{ > + struct of_fpga_region_priv *ovcs = region->priv; > + int err; > + > + /* if it's set do not allow changes */ > + if (ovcs->ovcs_id) > + return -EPERM; > + > + err = request_firmware(&ovcs->fw, config_info->firmware_name, 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 0; > + > +out_err: > + ovcs->ovcs_id = 0; > + ovcs->fw = NULL; > + > + return err; > +} > + > +static int of_fpga_region_config_remove(struct fpga_region *region, > + struct fpga_region_config_info *config_info) > +{ > + struct of_fpga_region_priv *ovcs = region->priv; > + > + if (!ovcs->ovcs_id) > + return -EPERM; > + > + of_overlay_remove(&ovcs->ovcs_id); > + release_firmware(ovcs->fw); > + > + ovcs->ovcs_id = 0; > + ovcs->fw = NULL; > + > + return 0; > +} > + > +static const struct fpga_region_ops region_ops = { > + .region_status = of_fpga_region_status, > + .region_config_enumeration = of_fpga_region_config_enumeration, > + .region_remove = of_fpga_region_config_remove, > +}; > + > static int of_fpga_region_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > + struct of_fpga_region_priv *priv; > struct fpga_region *region; > struct fpga_manager *mgr; > int ret; > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->dev = dev; > + > /* Find the FPGA mgr specified by region or parent region. */ > mgr = of_fpga_region_get_mgr(np); > if (IS_ERR(mgr)) > return -EPROBE_DEFER; > > - region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges); > + region = fpga_region_register_with_ops(dev, mgr, ®ion_ops, priv, > + of_fpga_region_get_bridges); > if (IS_ERR(region)) { > ret = PTR_ERR(region); > goto eprobe_mgr_put; > diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h > index 5fbc05fe70a6..3a3ba6dbb5e1 100644 > --- a/include/linux/fpga/fpga-region.h > +++ b/include/linux/fpga/fpga-region.h > @@ -6,15 +6,35 @@ > #include <linux/device.h> > #include <linux/fpga/fpga-mgr.h> > #include <linux/fpga/fpga-bridge.h> > +#include <linux/fpga-region.h> > +#include <linux/miscdevice.h> > > 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 *region); > + int (*region_config_enumeration)(struct fpga_region *region, > + struct fpga_region_config_info *config_info); My current concern is still about this combined API, it just offloads all work to low level, but we have some common flows. That's why we introduce a common FPGA reprograming API. I didn't see issue about the vendor specific pre configuration. They are generally needed to initialize the struct fpga_image_info, which is a common structure for fpga_region_program_fpga(). For port IDs(AFU) inputs for DFL, I think it could also be changed (Don't have to be implemented in this patchset). Previously DFL provides an uAPI for the whole device, so it needs a port_id input to position which fpga_region within the device for programming. But now, we are introducing a per fpga_region programming interface, IIUC port_id should not be needed anymore. The combined API is truly simple for leveraging the existing of-fpga-region overlay apply mechanism. But IMHO that flow doesn't fit our new uAPI well. That flow is to adapt the generic configfs overlay interface, which comes to a dead end as you mentioned. My gut feeling for the generic programing flow should be: 1. Program the image to HW. 2. Enumerate the programmed image (apply the DT overlay) Why we have to: 1. Start enumeration. 2. On pre enumeration, programe the image. 3. Real enumeration. Thanks, Yilun > + int (*region_remove)(struct fpga_region *region, > + struct fpga_region_config_info *config_info); > +}; > + > /** > * struct fpga_region_info - collection of parameters an FPGA Region > * @mgr: fpga region manager > * @compat_id: FPGA region id for compatibility check. > * @priv: fpga region private data > * @get_bridges: optional function to get bridges to a list > + * @fpga_region_ops: ops for low level FPGA region ops for device > + * enumeration/removal > * > * fpga_region_info contains parameters for the register_full function. > * These are separated into an info structure because they some are optional > @@ -26,6 +46,7 @@ struct fpga_region_info { > struct fpga_compat_id *compat_id; > void *priv; > int (*get_bridges)(struct fpga_region *region); > + const struct fpga_region_ops *region_ops; > }; > > /** > @@ -39,6 +60,8 @@ struct fpga_region_info { > * @ops_owner: module containing the get_bridges function > * @priv: private data > * @get_bridges: optional function to get bridges to a list > + * @fpga_region_ops: ops for low level FPGA region ops for device > + * enumeration/removal > */ > struct fpga_region { > struct device dev; > @@ -50,6 +73,8 @@ struct fpga_region { > struct module *ops_owner; > void *priv; > int (*get_bridges)(struct fpga_region *region); > + const struct fpga_region_ops *region_ops; > + struct miscdevice miscdev; > }; > > #define to_fpga_region(d) container_of(d, struct fpga_region, dev) > @@ -71,6 +96,13 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info > struct fpga_region * > __fpga_region_register(struct device *parent, struct fpga_manager *mgr, > int (*get_bridges)(struct fpga_region *), struct module *owner); > +#define fpga_region_register_with_ops(parent, mgr, region_ops, priv, get_bridges) \ > + __fpga_region_register_with_ops(parent, mgr, region_ops, priv, get_bridges, THIS_MODULE) > +struct fpga_region * > +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr, > + const struct fpga_region_ops *region_ops, void *priv, > + int (*get_bridges)(struct fpga_region *), > + struct module *owner); > void fpga_region_unregister(struct fpga_region *region); > > #endif /* _FPGA_REGION_H */ > diff --git a/include/uapi/linux/fpga-region.h b/include/uapi/linux/fpga-region.h > new file mode 100644 > index 000000000000..88ade83daf61 > --- /dev/null > +++ b/include/uapi/linux/fpga-region.h > @@ -0,0 +1,51 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > +/* > + * Header File for FPGA Region User API > + * > + * Copyright (C) 2024 Advanced Micro Devices, Inc. > + * > + * Author: Manne, Nava kishore <nava.kishore.manne@amd.com> > + */ > + > +#ifndef _UAPI_LINUX_FPGA_REGION_H > +#define _UAPI_LINUX_FPGA_REGION_H > + > +#include <linux/ioctl.h> > +#include <linux/limits.h> > +#include <linux/types.h> > + > +/* IOCTLs for fpga region file descriptor */ > +#define FPGA_REGION_MAGIC_NUMBER 'f' > +#define FPGA_REGION_BASE 0 > + > +/** > + * FPGA_REGION_IOCTL_LOAD - _IOW(FPGA_REGION_MAGIC, 0, > + * struct fpga_region_config_info) > + * > + * FPGA_REGION_IOCTL_REMOVE - _IOW(FPGA_REGION_MAGIC, 1, > + * struct fpga_region_config_info) > + * > + * Driver does Configuration/Reconfiguration based on Region ID and > + * Buffer (Image) provided by caller. > + * Return: 0 on success, -errno on failure. > + */ > +struct fpga_region_config_info { /* Input */ > + char firmware_name[NAME_MAX]; /* Firmware file name */ > +}; > + > +/* > + * FPGA Region Control IOCTLs. > + */ > +#define FPGA_REGION_MAGIC 'f' > +#define FPGA_IOW(num, dtype) _IOW(FPGA_REGION_MAGIC, num, dtype) > +#define FPGA_IOR(num, dtype) _IOR(FPGA_REGION_MAGIC, num, dtype) > + > +#define FPGA_REGION_IOCTL_LOAD FPGA_IOW(0, __u32) > +#define FPGA_REGION_IOCTL_REMOVE FPGA_IOW(1, __u32) > +#define FPGA_REGION_IOCTL_STATUS FPGA_IOR(2, __u32) > + > +/* Region status possibilities returned by FPGA_REGION_IOCTL_STATUS ioctl */ > +#define FPGA_REGION_HAS_PL 0 /* if the region has PL logic */ > +#define FPGA_REGION_EMPTY 1 /* If the region is empty */ > + > +#endif /* _UAPI_LINUX_FPGA_REGION_H */ > -- > 2.34.1 > >
Hi Yilun, > -----Original Message----- > From: Xu Yilun <yilun.xu@linux.intel.com> > Sent: Tuesday, November 19, 2024 9:45 AM > 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-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime > FPGA programming > > On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote: > > Introduces an IOCTL interface within the fpga-region subsystem, > > providing a generic and standardized mechanism for configuring (or) > > reprogramming FPGAs during runtime. The newly added interface supports > > both OF (Open Firmware) and non-OF devices, leveraging vendor-specific > > callbacks (e.g., configuration + enumeration, removal, and status) to > > accommodate a wide range of device specific configurations. > > > > The IOCTL interface ensures compatibility with both OF and non-OF > > devices, allowing for seamless FPGA reprogramming across diverse > > platforms. > > > > Vendor-specific callbacks are integrated into the interface, enabling > > custom FPGA configuration + enumeration, removal, and status reporting > > mechanisms, ensuring flexibility for vendor implementations. > > > > This solution enhances FPGA runtime management, supporting various > > device types and vendors, while ensuring compatibility with the > > current FPGA configuration flow. > > > > Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com> > > --- > > Changes for v2: > > - As discussed with Yilun, the implementation has been modified to > > utilize a callback approach, enabling seamless handling of both OF and non-OF > devices. > > > > - As suggested by Yilun in the POC code, we have moved away from > > using void *args as a parameter for ICOTL inputs to obtain the > > required user inputs. Instead, we are utilizing the > > fpga_region_config_info structure to gather user inputs. Currently, > > this structure is implemented to support only OF devices, but we intend to extend > it by incorporating new members to accommodate non-OF devices in the future. > > > > drivers/fpga/fpga-region.c | 110 +++++++++++++++++++++++++++++++ > > drivers/fpga/of-fpga-region.c | 91 ++++++++++++++++++++++++- > > include/linux/fpga/fpga-region.h | 32 +++++++++ > > include/uapi/linux/fpga-region.h | 51 ++++++++++++++ > > 4 files changed, 283 insertions(+), 1 deletion(-) create mode 100644 > > include/uapi/linux/fpga-region.h > > > > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > > index 753cd142503e..c6bea3c99a69 100644 > > --- a/drivers/fpga/fpga-region.c > > +++ b/drivers/fpga/fpga-region.c > > @@ -8,6 +8,7 @@ > > #include <linux/fpga/fpga-bridge.h> > > #include <linux/fpga/fpga-mgr.h> > > #include <linux/fpga/fpga-region.h> > > +#include <linux/fpga-region.h> > > #include <linux/idr.h> > > #include <linux/kernel.h> > > #include <linux/list.h> > > @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = { > > }; ATTRIBUTE_GROUPS(fpga_region); > > > > +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 int fpga_region_device_release(struct inode *inode, struct > > +file *file) { > > + return 0; > > +} > > + > > +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, > > + unsigned long arg) > > +{ > > + int err; > > + void __user *argp = (void __user *)arg; > > + struct fpga_region_config_info config_info; > > + struct fpga_region *region = (struct fpga_region > > +*)(file->private_data); > > + > > + switch (cmd) { > > + case FPGA_REGION_IOCTL_LOAD: > > + if (copy_from_user(&config_info, argp, sizeof(struct > fpga_region_config_info))) > > + return -EFAULT; > > + > > + err = region->region_ops->region_config_enumeration(region, > > +&config_info); > > + > > + break; > > + case FPGA_REGION_IOCTL_REMOVE: > > + if (copy_from_user(&config_info, argp, sizeof(struct > fpga_region_config_info))) > > + return -EFAULT; > > + > > + err = region->region_ops->region_remove(region, &config_info); > > + > > + break; > > + case FPGA_REGION_IOCTL_STATUS: > > + unsigned int status; > > + > > + status = region->region_ops->region_status(region); > > + > > + if (copy_to_user((void __user *)arg, &status, sizeof(status))) > > + err = -EFAULT; > > + break; > > + default: > > + err = -ENOTTY; > > + } > > + > > + return err; > > +} > > + > > +static const struct file_operations fpga_region_fops = { > > + .owner = THIS_MODULE, > > + .open = fpga_region_device_open, > > + .release = fpga_region_device_release, > > + .unlocked_ioctl = fpga_region_device_ioctl, > > + .compat_ioctl = fpga_region_device_ioctl, > > +}; > > + > > /** > > * __fpga_region_register_full - create and register an FPGA Region device > > * @parent: device parent > > @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const > struct fpga_region_info > > if (ret) > > goto err_remove; > > > > + if (info->region_ops) { > > + region->region_ops = info->region_ops; > > + region->miscdev.minor = MISC_DYNAMIC_MINOR; > > + region->miscdev.name = kobject_name(®ion->dev.kobj); > > + region->miscdev.fops = &fpga_region_fops; > > + ret = misc_register(®ion->miscdev); > > + if (ret) { > > + pr_err("fpga-region: failed to register misc device.\n"); > > + goto err_remove; > > + } > > + } > > + > > ret = device_register(®ion->dev); > > if (ret) { > > + misc_deregister(®ion->miscdev); > > put_device(®ion->dev); > > return ERR_PTR(ret); > > } > > @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, > > struct fpga_manager *mgr, } > > EXPORT_SYMBOL_GPL(__fpga_region_register); > > > > +/** > > + * __fpga_region_register_with_ops - create and register an FPGA > > +Region device > > + * with user interface call-backs. > > + * @parent: device parent > > + * @mgr: manager that programs this region > > + * @region_ops: ops for low level FPGA region for device > > +enumeration/removal > > + * @priv: of-fpga-region private data > > + * @get_bridges: optional function to get bridges to a list > > + * @owner: module containing the get_bridges function > > + * > > + * This simple version of the register function should be sufficient for most users. > > + * The fpga_region_register_full() function is available for users > > +that need to > > + * pass additional, optional parameters. > > + * > > + * Return: struct fpga_region or ERR_PTR() */ struct fpga_region * > > +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager > *mgr, > > + const struct fpga_region_ops *region_ops, > > + void *priv, > > + int (*get_bridges)(struct fpga_region *), > > + struct module *owner) > > +{ > > + struct fpga_region_info info = { 0 }; > > + > > + info.mgr = mgr; > > + info.priv = priv; > > + info.get_bridges = get_bridges; > > + info.region_ops = region_ops; > > + > > + return __fpga_region_register_full(parent, &info, owner); } > > +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops); > > + > > /** > > * fpga_region_unregister - unregister an FPGA region > > * @region: FPGA region > > @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register); > > */ > > void fpga_region_unregister(struct fpga_region *region) { > > + misc_deregister(®ion->miscdev); > > device_unregister(®ion->dev); > > } > > EXPORT_SYMBOL_GPL(fpga_region_unregister); > > diff --git a/drivers/fpga/of-fpga-region.c > > b/drivers/fpga/of-fpga-region.c index 8526a5a86f0c..63fe56e0466f > > 100644 > > --- a/drivers/fpga/of-fpga-region.c > > +++ b/drivers/fpga/of-fpga-region.c > > @@ -8,6 +8,8 @@ > > #include <linux/fpga/fpga-bridge.h> > > #include <linux/fpga/fpga-mgr.h> > > #include <linux/fpga/fpga-region.h> > > +#include <linux/firmware.h> > > +#include <linux/fpga-region.h> > > #include <linux/idr.h> > > #include <linux/kernel.h> > > #include <linux/list.h> > > @@ -18,6 +20,20 @@ > > #include <linux/slab.h> > > #include <linux/spinlock.h> > > > > +/** > > + * struct of_fpga_region_priv - Private data structure > > + * image. > > + * @dev: Device data structure > > + * @fw: firmware of coeff table. > > + * @path: path of FPGA overlay image firmware file. > > + * @ovcs_id: overlay changeset id. > > + */ > > +struct of_fpga_region_priv { > > + struct device *dev; > > + const struct firmware *fw; > > + int ovcs_id; > > +}; > > + > > static const struct of_device_id fpga_region_of_match[] = { > > { .compatible = "fpga-region", }, > > {}, > > @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = { > > .notifier_call = of_fpga_region_notify, }; > > > > +static int of_fpga_region_status(struct fpga_region *region) { > > + struct of_fpga_region_priv *ovcs = region->priv; > > + > > + if (ovcs->ovcs_id) > > + return FPGA_REGION_HAS_PL; > > Could you help specify what is PL? We will replace "PL" (Programmable Logic) with FPGA_REGION_HAS_CONFIGURED for better clarity. > > + > > + return FPGA_REGION_EMPTY; > > +} > > + > > +static int of_fpga_region_config_enumeration(struct fpga_region *region, > > + struct fpga_region_config_info *config_info) > { > > + struct of_fpga_region_priv *ovcs = region->priv; > > + int err; > > + > > + /* if it's set do not allow changes */ > > + if (ovcs->ovcs_id) > > + return -EPERM; > > + > > + err = request_firmware(&ovcs->fw, config_info->firmware_name, 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 0; > > + > > +out_err: > > + ovcs->ovcs_id = 0; > > + ovcs->fw = NULL; > > + > > + return err; > > +} > > + > > +static int of_fpga_region_config_remove(struct fpga_region *region, > > + struct fpga_region_config_info *config_info) { > > + struct of_fpga_region_priv *ovcs = region->priv; > > + > > + if (!ovcs->ovcs_id) > > + return -EPERM; > > + > > + of_overlay_remove(&ovcs->ovcs_id); > > + release_firmware(ovcs->fw); > > + > > + ovcs->ovcs_id = 0; > > + ovcs->fw = NULL; > > + > > + return 0; > > +} > > + > > +static const struct fpga_region_ops region_ops = { > > + .region_status = of_fpga_region_status, > > + .region_config_enumeration = of_fpga_region_config_enumeration, > > + .region_remove = of_fpga_region_config_remove, }; > > + > > static int of_fpga_region_probe(struct platform_device *pdev) { > > struct device *dev = &pdev->dev; > > struct device_node *np = dev->of_node; > > + struct of_fpga_region_priv *priv; > > struct fpga_region *region; > > struct fpga_manager *mgr; > > int ret; > > > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->dev = dev; > > + > > /* Find the FPGA mgr specified by region or parent region. */ > > mgr = of_fpga_region_get_mgr(np); > > if (IS_ERR(mgr)) > > return -EPROBE_DEFER; > > > > - region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges); > > + region = fpga_region_register_with_ops(dev, mgr, ®ion_ops, priv, > > + of_fpga_region_get_bridges); > > if (IS_ERR(region)) { > > ret = PTR_ERR(region); > > goto eprobe_mgr_put; > > diff --git a/include/linux/fpga/fpga-region.h > > b/include/linux/fpga/fpga-region.h > > index 5fbc05fe70a6..3a3ba6dbb5e1 100644 > > --- a/include/linux/fpga/fpga-region.h > > +++ b/include/linux/fpga/fpga-region.h > > @@ -6,15 +6,35 @@ > > #include <linux/device.h> > > #include <linux/fpga/fpga-mgr.h> > > #include <linux/fpga/fpga-bridge.h> > > +#include <linux/fpga-region.h> > > +#include <linux/miscdevice.h> > > > > 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 *region); > > + int (*region_config_enumeration)(struct fpga_region *region, > > + struct fpga_region_config_info *config_info); > > My current concern is still about this combined API, it just offloads all work to low > level, but we have some common flows. That's why we introduce a common FPGA > reprograming API. > > I didn't see issue about the vendor specific pre configuration. They are generally > needed to initialize the struct fpga_image_info, which is a common structure for > fpga_region_program_fpga(). > > For port IDs(AFU) inputs for DFL, I think it could also be changed (Don't have to be > implemented in this patchset). Previously DFL provides an uAPI for the whole > device, so it needs a port_id input to position which fpga_region within the device for > programming. But now, we are introducing a per fpga_region programming interface, > IIUC port_id should not be needed anymore. > > The combined API is truly simple for leveraging the existing of-fpga-region overlay > apply mechanism. But IMHO that flow doesn't fit our new uAPI well. That flow is to > adapt the generic configfs overlay interface, which comes to a dead end as you > mentioned. > > My gut feeling for the generic programing flow should be: > > 1. Program the image to HW. > 2. Enumerate the programmed image (apply the DT overlay) > > Why we have to: > > 1. Start enumeration. > 2. On pre enumeration, programe the image. > 3. Real enumeration. > I agree with the approach of leveraging vendor-specific callbacks to handle the distinct phases of the FPGA programming process. Here's the proposed flow. Pre-Configuration: A vendor-specific callback extracts the required pre-configuration details and initializes struct fpga_image_info. This ensures that all vendor-specific requirements are handled before proceeding to the programming phase. Programming: The common API fpga_region_program_fpga() is used to program the image to hardware. This standardizes the programming logic, minimizing duplication and ensuring consistency across implementations. Enumeration: A vendor-specific callback is used for real enumeration, enabling hardware specific customization while keeping the flow flexible and adaptable This approach provides a clean separation of responsibilities, with vendor-specific logic confined to the pre-configuration and enumeration phases, while the programming phase leverages a common API. It simplifies maintenance and aligns well with the Program -> Enumerate flow. Looking forward to your feedback. Regards, Navakishore.
On 2024-11-19 05:14, Xu Yilun wrote: > On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote: >> Introduces an IOCTL interface within the fpga-region subsystem, >> providing a generic and standardized mechanism for configuring (or) >> reprogramming FPGAs during runtime. The newly added interface supports >> both OF (Open Firmware) and non-OF devices, leveraging vendor-specific >> callbacks (e.g., configuration + enumeration, removal, and status) to >> accommodate a wide range of device specific configurations. >> >> The IOCTL interface ensures compatibility with both OF and non-OF >> devices, allowing for seamless FPGA reprogramming across diverse >> platforms. >> >> Vendor-specific callbacks are integrated into the interface, enabling >> custom FPGA configuration + enumeration, removal, and status reporting >> mechanisms, ensuring flexibility for vendor implementations. >> >> This solution enhances FPGA runtime management, supporting various device >> types and vendors, while ensuring compatibility with the current FPGA >> configuration flow. >> >> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com> >> --- >> Changes for v2: >> - As discussed with Yilun, the implementation has been modified to utilize a >> callback approach, enabling seamless handling of both OF and non-OF devices. >> >> - As suggested by Yilun in the POC code, we have moved away from using void *args >> as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are >> utilizing the fpga_region_config_info structure to gather user inputs. Currently, >> this structure is implemented to support only OF devices, but we intend to extend >> it by incorporating new members to accommodate non-OF devices in the future. >> >> drivers/fpga/fpga-region.c | 110 +++++++++++++++++++++++++++++++ >> drivers/fpga/of-fpga-region.c | 91 ++++++++++++++++++++++++- >> include/linux/fpga/fpga-region.h | 32 +++++++++ >> include/uapi/linux/fpga-region.h | 51 ++++++++++++++ >> 4 files changed, 283 insertions(+), 1 deletion(-) >> create mode 100644 include/uapi/linux/fpga-region.h >> >> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c >> index 753cd142503e..c6bea3c99a69 100644 >> --- a/drivers/fpga/fpga-region.c >> +++ b/drivers/fpga/fpga-region.c >> @@ -8,6 +8,7 @@ >> #include <linux/fpga/fpga-bridge.h> >> #include <linux/fpga/fpga-mgr.h> >> #include <linux/fpga/fpga-region.h> >> +#include <linux/fpga-region.h> >> #include <linux/idr.h> >> #include <linux/kernel.h> >> #include <linux/list.h> >> @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = { >> }; >> ATTRIBUTE_GROUPS(fpga_region); >> >> +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 int fpga_region_device_release(struct inode *inode, struct file *file) >> +{ >> + return 0; >> +} >> + >> +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, >> + unsigned long arg) >> +{ >> + int err; >> + void __user *argp = (void __user *)arg; >> + struct fpga_region_config_info config_info; >> + struct fpga_region *region = (struct fpga_region *)(file->private_data); >> + >> + switch (cmd) { >> + case FPGA_REGION_IOCTL_LOAD: >> + if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) >> + return -EFAULT; >> + >> + err = region->region_ops->region_config_enumeration(region, &config_info); >> + >> + break; >> + case FPGA_REGION_IOCTL_REMOVE: >> + if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) >> + return -EFAULT; >> + >> + err = region->region_ops->region_remove(region, &config_info); >> + >> + break; >> + case FPGA_REGION_IOCTL_STATUS: >> + unsigned int status; >> + >> + status = region->region_ops->region_status(region); >> + >> + if (copy_to_user((void __user *)arg, &status, sizeof(status))) >> + err = -EFAULT; >> + break; >> + default: >> + err = -ENOTTY; >> + } >> + >> + return err; >> +} >> + >> +static const struct file_operations fpga_region_fops = { >> + .owner = THIS_MODULE, >> + .open = fpga_region_device_open, >> + .release = fpga_region_device_release, >> + .unlocked_ioctl = fpga_region_device_ioctl, >> + .compat_ioctl = fpga_region_device_ioctl, >> +}; >> + >> /** >> * __fpga_region_register_full - create and register an FPGA Region device >> * @parent: device parent >> @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info >> if (ret) >> goto err_remove; >> >> + if (info->region_ops) { >> + region->region_ops = info->region_ops; >> + region->miscdev.minor = MISC_DYNAMIC_MINOR; >> + region->miscdev.name = kobject_name(®ion->dev.kobj); >> + region->miscdev.fops = &fpga_region_fops; >> + ret = misc_register(®ion->miscdev); >> + if (ret) { >> + pr_err("fpga-region: failed to register misc device.\n"); >> + goto err_remove; >> + } >> + } >> + >> ret = device_register(®ion->dev); >> if (ret) { >> + misc_deregister(®ion->miscdev); >> put_device(®ion->dev); >> return ERR_PTR(ret); >> } >> @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr, >> } >> EXPORT_SYMBOL_GPL(__fpga_region_register); >> >> +/** >> + * __fpga_region_register_with_ops - create and register an FPGA Region device >> + * with user interface call-backs. >> + * @parent: device parent >> + * @mgr: manager that programs this region >> + * @region_ops: ops for low level FPGA region for device enumeration/removal >> + * @priv: of-fpga-region private data >> + * @get_bridges: optional function to get bridges to a list >> + * @owner: module containing the get_bridges function >> + * >> + * This simple version of the register function should be sufficient for most users. >> + * The fpga_region_register_full() function is available for users that need to >> + * pass additional, optional parameters. >> + * >> + * Return: struct fpga_region or ERR_PTR() >> + */ >> +struct fpga_region * >> +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr, >> + const struct fpga_region_ops *region_ops, >> + void *priv, >> + int (*get_bridges)(struct fpga_region *), >> + struct module *owner) >> +{ >> + struct fpga_region_info info = { 0 }; >> + >> + info.mgr = mgr; >> + info.priv = priv; >> + info.get_bridges = get_bridges; >> + info.region_ops = region_ops; >> + >> + return __fpga_region_register_full(parent, &info, owner); >> +} >> +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops); >> + >> /** >> * fpga_region_unregister - unregister an FPGA region >> * @region: FPGA region >> @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register); >> */ >> void fpga_region_unregister(struct fpga_region *region) >> { >> + misc_deregister(®ion->miscdev); >> device_unregister(®ion->dev); >> } >> EXPORT_SYMBOL_GPL(fpga_region_unregister); >> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c >> index 8526a5a86f0c..63fe56e0466f 100644 >> --- a/drivers/fpga/of-fpga-region.c >> +++ b/drivers/fpga/of-fpga-region.c >> @@ -8,6 +8,8 @@ >> #include <linux/fpga/fpga-bridge.h> >> #include <linux/fpga/fpga-mgr.h> >> #include <linux/fpga/fpga-region.h> >> +#include <linux/firmware.h> >> +#include <linux/fpga-region.h> >> #include <linux/idr.h> >> #include <linux/kernel.h> >> #include <linux/list.h> >> @@ -18,6 +20,20 @@ >> #include <linux/slab.h> >> #include <linux/spinlock.h> >> >> +/** >> + * struct of_fpga_region_priv - Private data structure >> + * image. >> + * @dev: Device data structure >> + * @fw: firmware of coeff table. >> + * @path: path of FPGA overlay image firmware file. >> + * @ovcs_id: overlay changeset id. >> + */ >> +struct of_fpga_region_priv { >> + struct device *dev; >> + const struct firmware *fw; >> + int ovcs_id; >> +}; >> + >> static const struct of_device_id fpga_region_of_match[] = { >> { .compatible = "fpga-region", }, >> {}, >> @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = { >> .notifier_call = of_fpga_region_notify, >> }; >> >> +static int of_fpga_region_status(struct fpga_region *region) >> +{ >> + struct of_fpga_region_priv *ovcs = region->priv; >> + >> + if (ovcs->ovcs_id) >> + return FPGA_REGION_HAS_PL; > > Could you help specify what is PL? > >> + >> + return FPGA_REGION_EMPTY; >> +} >> + >> +static int of_fpga_region_config_enumeration(struct fpga_region *region, >> + struct fpga_region_config_info *config_info) >> +{ >> + struct of_fpga_region_priv *ovcs = region->priv; >> + int err; >> + >> + /* if it's set do not allow changes */ >> + if (ovcs->ovcs_id) >> + return -EPERM; >> + >> + err = request_firmware(&ovcs->fw, config_info->firmware_name, 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 0; >> + >> +out_err: >> + ovcs->ovcs_id = 0; >> + ovcs->fw = NULL; >> + >> + return err; >> +} >> + >> +static int of_fpga_region_config_remove(struct fpga_region *region, >> + struct fpga_region_config_info *config_info) >> +{ >> + struct of_fpga_region_priv *ovcs = region->priv; >> + >> + if (!ovcs->ovcs_id) >> + return -EPERM; >> + >> + of_overlay_remove(&ovcs->ovcs_id); >> + release_firmware(ovcs->fw); >> + >> + ovcs->ovcs_id = 0; >> + ovcs->fw = NULL; >> + >> + return 0; >> +} >> + >> +static const struct fpga_region_ops region_ops = { >> + .region_status = of_fpga_region_status, >> + .region_config_enumeration = of_fpga_region_config_enumeration, >> + .region_remove = of_fpga_region_config_remove, >> +}; >> + >> static int of_fpga_region_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct device_node *np = dev->of_node; >> + struct of_fpga_region_priv *priv; >> struct fpga_region *region; >> struct fpga_manager *mgr; >> int ret; >> >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + priv->dev = dev; >> + >> /* Find the FPGA mgr specified by region or parent region. */ >> mgr = of_fpga_region_get_mgr(np); >> if (IS_ERR(mgr)) >> return -EPROBE_DEFER; >> >> - region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges); >> + region = fpga_region_register_with_ops(dev, mgr, ®ion_ops, priv, >> + of_fpga_region_get_bridges); >> if (IS_ERR(region)) { >> ret = PTR_ERR(region); >> goto eprobe_mgr_put; >> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h >> index 5fbc05fe70a6..3a3ba6dbb5e1 100644 >> --- a/include/linux/fpga/fpga-region.h >> +++ b/include/linux/fpga/fpga-region.h >> @@ -6,15 +6,35 @@ >> #include <linux/device.h> >> #include <linux/fpga/fpga-mgr.h> >> #include <linux/fpga/fpga-bridge.h> >> +#include <linux/fpga-region.h> >> +#include <linux/miscdevice.h> >> >> 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 *region); >> + int (*region_config_enumeration)(struct fpga_region *region, >> + struct fpga_region_config_info *config_info); > > My current concern is still about this combined API, it just offloads > all work to low level, but we have some common flows. That's why we > introduce a common FPGA reprograming API. > > I didn't see issue about the vendor specific pre configuration. They > are generally needed to initialize the struct fpga_image_info, which > is a common structure for fpga_region_program_fpga(). > > For port IDs(AFU) inputs for DFL, I think it could also be changed > (Don't have to be implemented in this patchset). Previously DFL > provides an uAPI for the whole device, so it needs a port_id input to > position which fpga_region within the device for programming. But now, > we are introducing a per fpga_region programming interface, IIUC port_id > should not be needed anymore. > > The combined API is truly simple for leveraging the existing > of-fpga-region overlay apply mechanism. But IMHO that flow doesn't fit > our new uAPI well. That flow is to adapt the generic configfs overlay > interface, which comes to a dead end as you mentioned. > > My gut feeling for the generic programing flow should be: > > 1. Program the image to HW. > 2. Enumerate the programmed image (apply the DT overlay) > > Why we have to: > > 1. Start enumeration. > 2. On pre enumeration, programe the image. > 3. Real enumeration. I'm currently working on an RFC to propose a rework of the fpga subsystem in order to make it more aligned with the device model. One of the ideas I'm experimenting with is having a bus (struct bus_type) for fpga regions (devices) so that we can have region drivers that could handle internal device enumeration/management whenever a new region is configured on the fabric. Does this make sense in your opinions? Concerning the reconfiguration, wouldn't it be cleaner to use a per-region sysfs interface at fpga-region level? It would still work for both OF & non-OF cases. Thanks, Marco
> > > + * 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 *region); > > > + int (*region_config_enumeration)(struct fpga_region *region, > > > + struct fpga_region_config_info *config_info); > > > > My current concern is still about this combined API, it just offloads all work to low > > level, but we have some common flows. That's why we introduce a common FPGA > > reprograming API. > > > > I didn't see issue about the vendor specific pre configuration. They are generally > > needed to initialize the struct fpga_image_info, which is a common structure for > > fpga_region_program_fpga(). > > > > For port IDs(AFU) inputs for DFL, I think it could also be changed (Don't have to be > > implemented in this patchset). Previously DFL provides an uAPI for the whole > > device, so it needs a port_id input to position which fpga_region within the device for > > programming. But now, we are introducing a per fpga_region programming interface, > > IIUC port_id should not be needed anymore. > > > > The combined API is truly simple for leveraging the existing of-fpga-region overlay > > apply mechanism. But IMHO that flow doesn't fit our new uAPI well. That flow is to > > adapt the generic configfs overlay interface, which comes to a dead end as you > > mentioned. > > > > My gut feeling for the generic programing flow should be: > > > > 1. Program the image to HW. > > 2. Enumerate the programmed image (apply the DT overlay) > > > > Why we have to: > > > > 1. Start enumeration. > > 2. On pre enumeration, programe the image. > > 3. Real enumeration. > > > > I agree with the approach of leveraging vendor-specific callbacks to handle > the distinct phases of the FPGA programming process. > Here's the proposed flow. > > Pre-Configuration: > A vendor-specific callback extracts the required pre-configuration details > and initializes struct fpga_image_info. This ensures that all vendor-specific Since we need to construct the fpga_image_info, initialize multiple field as needed, I'm wondering if configfs could be a solution for the uAPI? > requirements are handled before proceeding to the programming phase. > > Programming: > The common API fpga_region_program_fpga() is used to program the image > to hardware. This standardizes the programming logic, minimizing duplication > and ensuring consistency across implementations. > > Enumeration: > A vendor-specific callback is used for real enumeration, enabling hardware > specific customization while keeping the flow flexible and adaptable > > This approach provides a clean separation of responsibilities, with > vendor-specific logic confined to the pre-configuration and enumeration > phases, while the programming phase leverages a common API. > It simplifies maintenance and aligns well with the Program -> Enumerate flow. Generally I'm good to this flow. Thanks, Yilun > > Looking forward to your feedback. > > > Regards, > Navakishore.
On Mon, Nov 25, 2024 at 12:26:06PM +0100, Marco Pagani wrote: > > > On 2024-11-19 05:14, Xu Yilun wrote: > > On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote: > >> Introduces an IOCTL interface within the fpga-region subsystem, > >> providing a generic and standardized mechanism for configuring (or) > >> reprogramming FPGAs during runtime. The newly added interface supports > >> both OF (Open Firmware) and non-OF devices, leveraging vendor-specific > >> callbacks (e.g., configuration + enumeration, removal, and status) to > >> accommodate a wide range of device specific configurations. > >> > >> The IOCTL interface ensures compatibility with both OF and non-OF > >> devices, allowing for seamless FPGA reprogramming across diverse > >> platforms. > >> > >> Vendor-specific callbacks are integrated into the interface, enabling > >> custom FPGA configuration + enumeration, removal, and status reporting > >> mechanisms, ensuring flexibility for vendor implementations. > >> > >> This solution enhances FPGA runtime management, supporting various device > >> types and vendors, while ensuring compatibility with the current FPGA > >> configuration flow. > >> > >> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com> > >> --- > >> Changes for v2: > >> - As discussed with Yilun, the implementation has been modified to utilize a > >> callback approach, enabling seamless handling of both OF and non-OF devices. > >> > >> - As suggested by Yilun in the POC code, we have moved away from using void *args > >> as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are > >> utilizing the fpga_region_config_info structure to gather user inputs. Currently, > >> this structure is implemented to support only OF devices, but we intend to extend > >> it by incorporating new members to accommodate non-OF devices in the future. > >> > >> drivers/fpga/fpga-region.c | 110 +++++++++++++++++++++++++++++++ > >> drivers/fpga/of-fpga-region.c | 91 ++++++++++++++++++++++++- > >> include/linux/fpga/fpga-region.h | 32 +++++++++ > >> include/uapi/linux/fpga-region.h | 51 ++++++++++++++ > >> 4 files changed, 283 insertions(+), 1 deletion(-) > >> create mode 100644 include/uapi/linux/fpga-region.h > >> > >> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > >> index 753cd142503e..c6bea3c99a69 100644 > >> --- a/drivers/fpga/fpga-region.c > >> +++ b/drivers/fpga/fpga-region.c > >> @@ -8,6 +8,7 @@ > >> #include <linux/fpga/fpga-bridge.h> > >> #include <linux/fpga/fpga-mgr.h> > >> #include <linux/fpga/fpga-region.h> > >> +#include <linux/fpga-region.h> > >> #include <linux/idr.h> > >> #include <linux/kernel.h> > >> #include <linux/list.h> > >> @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = { > >> }; > >> ATTRIBUTE_GROUPS(fpga_region); > >> > >> +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 int fpga_region_device_release(struct inode *inode, struct file *file) > >> +{ > >> + return 0; > >> +} > >> + > >> +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, > >> + unsigned long arg) > >> +{ > >> + int err; > >> + void __user *argp = (void __user *)arg; > >> + struct fpga_region_config_info config_info; > >> + struct fpga_region *region = (struct fpga_region *)(file->private_data); > >> + > >> + switch (cmd) { > >> + case FPGA_REGION_IOCTL_LOAD: > >> + if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) > >> + return -EFAULT; > >> + > >> + err = region->region_ops->region_config_enumeration(region, &config_info); > >> + > >> + break; > >> + case FPGA_REGION_IOCTL_REMOVE: > >> + if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) > >> + return -EFAULT; > >> + > >> + err = region->region_ops->region_remove(region, &config_info); > >> + > >> + break; > >> + case FPGA_REGION_IOCTL_STATUS: > >> + unsigned int status; > >> + > >> + status = region->region_ops->region_status(region); > >> + > >> + if (copy_to_user((void __user *)arg, &status, sizeof(status))) > >> + err = -EFAULT; > >> + break; > >> + default: > >> + err = -ENOTTY; > >> + } > >> + > >> + return err; > >> +} > >> + > >> +static const struct file_operations fpga_region_fops = { > >> + .owner = THIS_MODULE, > >> + .open = fpga_region_device_open, > >> + .release = fpga_region_device_release, > >> + .unlocked_ioctl = fpga_region_device_ioctl, > >> + .compat_ioctl = fpga_region_device_ioctl, > >> +}; > >> + > >> /** > >> * __fpga_region_register_full - create and register an FPGA Region device > >> * @parent: device parent > >> @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info > >> if (ret) > >> goto err_remove; > >> > >> + if (info->region_ops) { > >> + region->region_ops = info->region_ops; > >> + region->miscdev.minor = MISC_DYNAMIC_MINOR; > >> + region->miscdev.name = kobject_name(®ion->dev.kobj); > >> + region->miscdev.fops = &fpga_region_fops; > >> + ret = misc_register(®ion->miscdev); > >> + if (ret) { > >> + pr_err("fpga-region: failed to register misc device.\n"); > >> + goto err_remove; > >> + } > >> + } > >> + > >> ret = device_register(®ion->dev); > >> if (ret) { > >> + misc_deregister(®ion->miscdev); > >> put_device(®ion->dev); > >> return ERR_PTR(ret); > >> } > >> @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr, > >> } > >> EXPORT_SYMBOL_GPL(__fpga_region_register); > >> > >> +/** > >> + * __fpga_region_register_with_ops - create and register an FPGA Region device > >> + * with user interface call-backs. > >> + * @parent: device parent > >> + * @mgr: manager that programs this region > >> + * @region_ops: ops for low level FPGA region for device enumeration/removal > >> + * @priv: of-fpga-region private data > >> + * @get_bridges: optional function to get bridges to a list > >> + * @owner: module containing the get_bridges function > >> + * > >> + * This simple version of the register function should be sufficient for most users. > >> + * The fpga_region_register_full() function is available for users that need to > >> + * pass additional, optional parameters. > >> + * > >> + * Return: struct fpga_region or ERR_PTR() > >> + */ > >> +struct fpga_region * > >> +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr, > >> + const struct fpga_region_ops *region_ops, > >> + void *priv, > >> + int (*get_bridges)(struct fpga_region *), > >> + struct module *owner) > >> +{ > >> + struct fpga_region_info info = { 0 }; > >> + > >> + info.mgr = mgr; > >> + info.priv = priv; > >> + info.get_bridges = get_bridges; > >> + info.region_ops = region_ops; > >> + > >> + return __fpga_region_register_full(parent, &info, owner); > >> +} > >> +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops); > >> + > >> /** > >> * fpga_region_unregister - unregister an FPGA region > >> * @region: FPGA region > >> @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register); > >> */ > >> void fpga_region_unregister(struct fpga_region *region) > >> { > >> + misc_deregister(®ion->miscdev); > >> device_unregister(®ion->dev); > >> } > >> EXPORT_SYMBOL_GPL(fpga_region_unregister); > >> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c > >> index 8526a5a86f0c..63fe56e0466f 100644 > >> --- a/drivers/fpga/of-fpga-region.c > >> +++ b/drivers/fpga/of-fpga-region.c > >> @@ -8,6 +8,8 @@ > >> #include <linux/fpga/fpga-bridge.h> > >> #include <linux/fpga/fpga-mgr.h> > >> #include <linux/fpga/fpga-region.h> > >> +#include <linux/firmware.h> > >> +#include <linux/fpga-region.h> > >> #include <linux/idr.h> > >> #include <linux/kernel.h> > >> #include <linux/list.h> > >> @@ -18,6 +20,20 @@ > >> #include <linux/slab.h> > >> #include <linux/spinlock.h> > >> > >> +/** > >> + * struct of_fpga_region_priv - Private data structure > >> + * image. > >> + * @dev: Device data structure > >> + * @fw: firmware of coeff table. > >> + * @path: path of FPGA overlay image firmware file. > >> + * @ovcs_id: overlay changeset id. > >> + */ > >> +struct of_fpga_region_priv { > >> + struct device *dev; > >> + const struct firmware *fw; > >> + int ovcs_id; > >> +}; > >> + > >> static const struct of_device_id fpga_region_of_match[] = { > >> { .compatible = "fpga-region", }, > >> {}, > >> @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = { > >> .notifier_call = of_fpga_region_notify, > >> }; > >> > >> +static int of_fpga_region_status(struct fpga_region *region) > >> +{ > >> + struct of_fpga_region_priv *ovcs = region->priv; > >> + > >> + if (ovcs->ovcs_id) > >> + return FPGA_REGION_HAS_PL; > > > > Could you help specify what is PL? > > > >> + > >> + return FPGA_REGION_EMPTY; > >> +} > >> + > >> +static int of_fpga_region_config_enumeration(struct fpga_region *region, > >> + struct fpga_region_config_info *config_info) > >> +{ > >> + struct of_fpga_region_priv *ovcs = region->priv; > >> + int err; > >> + > >> + /* if it's set do not allow changes */ > >> + if (ovcs->ovcs_id) > >> + return -EPERM; > >> + > >> + err = request_firmware(&ovcs->fw, config_info->firmware_name, 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 0; > >> + > >> +out_err: > >> + ovcs->ovcs_id = 0; > >> + ovcs->fw = NULL; > >> + > >> + return err; > >> +} > >> + > >> +static int of_fpga_region_config_remove(struct fpga_region *region, > >> + struct fpga_region_config_info *config_info) > >> +{ > >> + struct of_fpga_region_priv *ovcs = region->priv; > >> + > >> + if (!ovcs->ovcs_id) > >> + return -EPERM; > >> + > >> + of_overlay_remove(&ovcs->ovcs_id); > >> + release_firmware(ovcs->fw); > >> + > >> + ovcs->ovcs_id = 0; > >> + ovcs->fw = NULL; > >> + > >> + return 0; > >> +} > >> + > >> +static const struct fpga_region_ops region_ops = { > >> + .region_status = of_fpga_region_status, > >> + .region_config_enumeration = of_fpga_region_config_enumeration, > >> + .region_remove = of_fpga_region_config_remove, > >> +}; > >> + > >> static int of_fpga_region_probe(struct platform_device *pdev) > >> { > >> struct device *dev = &pdev->dev; > >> struct device_node *np = dev->of_node; > >> + struct of_fpga_region_priv *priv; > >> struct fpga_region *region; > >> struct fpga_manager *mgr; > >> int ret; > >> > >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > >> + if (!priv) > >> + return -ENOMEM; > >> + > >> + priv->dev = dev; > >> + > >> /* Find the FPGA mgr specified by region or parent region. */ > >> mgr = of_fpga_region_get_mgr(np); > >> if (IS_ERR(mgr)) > >> return -EPROBE_DEFER; > >> > >> - region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges); > >> + region = fpga_region_register_with_ops(dev, mgr, ®ion_ops, priv, > >> + of_fpga_region_get_bridges); > >> if (IS_ERR(region)) { > >> ret = PTR_ERR(region); > >> goto eprobe_mgr_put; > >> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h > >> index 5fbc05fe70a6..3a3ba6dbb5e1 100644 > >> --- a/include/linux/fpga/fpga-region.h > >> +++ b/include/linux/fpga/fpga-region.h > >> @@ -6,15 +6,35 @@ > >> #include <linux/device.h> > >> #include <linux/fpga/fpga-mgr.h> > >> #include <linux/fpga/fpga-bridge.h> > >> +#include <linux/fpga-region.h> > >> +#include <linux/miscdevice.h> > >> > >> 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 *region); > >> + int (*region_config_enumeration)(struct fpga_region *region, > >> + struct fpga_region_config_info *config_info); > > > > My current concern is still about this combined API, it just offloads > > all work to low level, but we have some common flows. That's why we > > introduce a common FPGA reprograming API. > > > > I didn't see issue about the vendor specific pre configuration. They > > are generally needed to initialize the struct fpga_image_info, which > > is a common structure for fpga_region_program_fpga(). > > > > For port IDs(AFU) inputs for DFL, I think it could also be changed > > (Don't have to be implemented in this patchset). Previously DFL > > provides an uAPI for the whole device, so it needs a port_id input to > > position which fpga_region within the device for programming. But now, > > we are introducing a per fpga_region programming interface, IIUC port_id > > should not be needed anymore. > > > > The combined API is truly simple for leveraging the existing > > of-fpga-region overlay apply mechanism. But IMHO that flow doesn't fit > > our new uAPI well. That flow is to adapt the generic configfs overlay > > interface, which comes to a dead end as you mentioned. > > > > My gut feeling for the generic programing flow should be: > > > > 1. Program the image to HW. > > 2. Enumerate the programmed image (apply the DT overlay) > > > > Why we have to: > > > > 1. Start enumeration. > > 2. On pre enumeration, programe the image. > > 3. Real enumeration. > > I'm currently working on an RFC to propose a rework of the fpga > subsystem in order to make it more aligned with the device model. One of > the ideas I'm experimenting with is having a bus (struct bus_type) for > fpga regions (devices) so that we can have region drivers that could > handle internal device enumeration/management whenever a new region is > configured on the fabric. Does this make sense in your opinions? mm.. I didn't fully understand the need to have a region driver, what's the issue to solve? Thanks, Yilun > > Concerning the reconfiguration, wouldn't it be cleaner to use a > per-region sysfs interface at fpga-region level? It would still work > for both OF & non-OF cases. > > Thanks, > Marco > >
Hi Yilun, > -----Original Message----- > From: Xu Yilun <yilun.xu@linux.intel.com> > Sent: Wednesday, November 27, 2024 7:20 AM > 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-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime > FPGA programming > > > > > + * 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 *region); > > > > + int (*region_config_enumeration)(struct fpga_region *region, > > > > + struct fpga_region_config_info *config_info); > > > > > > My current concern is still about this combined API, it just > > > offloads all work to low level, but we have some common flows. > > > That's why we introduce a common FPGA reprograming API. > > > > > > I didn't see issue about the vendor specific pre configuration. They > > > are generally needed to initialize the struct fpga_image_info, which > > > is a common structure for fpga_region_program_fpga(). > > > > > > For port IDs(AFU) inputs for DFL, I think it could also be changed > > > (Don't have to be implemented in this patchset). Previously DFL > > > provides an uAPI for the whole device, so it needs a port_id input > > > to position which fpga_region within the device for programming. But > > > now, we are introducing a per fpga_region programming interface, IIUC port_id > should not be needed anymore. > > > > > > The combined API is truly simple for leveraging the existing > > > of-fpga-region overlay apply mechanism. But IMHO that flow doesn't > > > fit our new uAPI well. That flow is to adapt the generic configfs > > > overlay interface, which comes to a dead end as you mentioned. > > > > > > My gut feeling for the generic programing flow should be: > > > > > > 1. Program the image to HW. > > > 2. Enumerate the programmed image (apply the DT overlay) > > > > > > Why we have to: > > > > > > 1. Start enumeration. > > > 2. On pre enumeration, programe the image. > > > 3. Real enumeration. > > > > > > > I agree with the approach of leveraging vendor-specific callbacks to > > handle the distinct phases of the FPGA programming process. > > Here's the proposed flow. > > > > Pre-Configuration: > > A vendor-specific callback extracts the required pre-configuration > > details and initializes struct fpga_image_info. This ensures that all > > vendor-specific > > Since we need to construct the fpga_image_info, initialize multiple field as needed, > I'm wondering if configfs could be a solution for the uAPI? > A configfs uAPI isn't necessary, we can manage this using the proposed IOCTL flow. The POC code looks as follows. static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct fpga_region *region = (struct fpga_region *)(file->private_data); struct fpga_region_config_info config_info; void __user *argp = (void __user *)arg; struct device *dev = ®ion->dev; struct fpga_image_info *info; int err; switch (cmd) { case FPGA_REGION_IOCTL_LOAD: if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) return -EFAULT; info = fpga_image_info_alloc(dev); if (!info) return ERR_PTR(-ENOMEM); /* A vendor-specific callback extracts the required pre-configuration * details and initializes struct fpga_image_info. This ensures that all * vendor-specific requirements are handled before proceeding to * the programming phase. */ err = region->region_ops->region_preconfig(region, &config_info, info); if (err) return err; /* The common API fpga_region_program_fpga() is used to program * the image to hardware. */ region->info = info; err = fpga_region_program_fpga(region); if (err) { fpga_image_info_free(info); region->info = NULL; } /* A vendor-specific callback is used for real enumeration, enabling * hardware specific customization. */ err = region->region_ops->region_enumeration(region, &config_info); break; case FPGA_REGION_IOCTL_REMOVE: if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) return -EFAULT; err = region->region_ops->region_remove(region, &config_info); if (err) return err; fpga_image_info_free(region->info); break; case FPGA_REGION_IOCTL_STATUS: unsigned int status; status = region->region_ops->region_status(region); if (copy_to_user((void __user *)arg, &status, sizeof(status))) err = -EFAULT; break; default: err = -ENOTTY; } return err; } Regards, Navakishore.
On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote: > Hi Yilun, > > > -----Original Message----- > > From: Xu Yilun <yilun.xu@linux.intel.com> > > Sent: Wednesday, November 27, 2024 7:20 AM > > 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-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; > > devicetree@vger.kernel.org > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime > > FPGA programming > > > > > > > + * 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 *region); > > > > > + int (*region_config_enumeration)(struct fpga_region *region, > > > > > + struct fpga_region_config_info *config_info); > > > > > > > > My current concern is still about this combined API, it just > > > > offloads all work to low level, but we have some common flows. > > > > That's why we introduce a common FPGA reprograming API. > > > > > > > > I didn't see issue about the vendor specific pre configuration. They > > > > are generally needed to initialize the struct fpga_image_info, which > > > > is a common structure for fpga_region_program_fpga(). > > > > > > > > For port IDs(AFU) inputs for DFL, I think it could also be changed > > > > (Don't have to be implemented in this patchset). Previously DFL > > > > provides an uAPI for the whole device, so it needs a port_id input > > > > to position which fpga_region within the device for programming. But > > > > now, we are introducing a per fpga_region programming interface, IIUC port_id > > should not be needed anymore. > > > > > > > > The combined API is truly simple for leveraging the existing > > > > of-fpga-region overlay apply mechanism. But IMHO that flow doesn't > > > > fit our new uAPI well. That flow is to adapt the generic configfs > > > > overlay interface, which comes to a dead end as you mentioned. > > > > > > > > My gut feeling for the generic programing flow should be: > > > > > > > > 1. Program the image to HW. > > > > 2. Enumerate the programmed image (apply the DT overlay) > > > > > > > > Why we have to: > > > > > > > > 1. Start enumeration. > > > > 2. On pre enumeration, programe the image. > > > > 3. Real enumeration. > > > > > > > > > > I agree with the approach of leveraging vendor-specific callbacks to > > > handle the distinct phases of the FPGA programming process. > > > Here's the proposed flow. > > > > > > Pre-Configuration: > > > A vendor-specific callback extracts the required pre-configuration > > > details and initializes struct fpga_image_info. This ensures that all > > > vendor-specific > > > > Since we need to construct the fpga_image_info, initialize multiple field as needed, > > I'm wondering if configfs could be a solution for the uAPI? > > > > A configfs uAPI isn't necessary, we can manage this using the proposed IOCTL flow. > The POC code looks as follows. I prefer more to configfs cause it provides standard FS way to create the fpga_image_info object, e.g. which attributes are visible for OF/non-OF region, which attributes come from image blob and can only be RO, etc. Of couse ioctl() could achieve the same goal but would add much more specific rules (maybe flags/types) for user to follow. Thanks, Yilun > > static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > { > struct fpga_region *region = (struct fpga_region *)(file->private_data); > struct fpga_region_config_info config_info; > void __user *argp = (void __user *)arg; > struct device *dev = ®ion->dev; > struct fpga_image_info *info; > int err; > > switch (cmd) { > case FPGA_REGION_IOCTL_LOAD: > if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) > return -EFAULT; > > info = fpga_image_info_alloc(dev); > if (!info) > return ERR_PTR(-ENOMEM); > > /* A vendor-specific callback extracts the required pre-configuration > * details and initializes struct fpga_image_info. This ensures that all > * vendor-specific requirements are handled before proceeding to > * the programming phase. > */ > err = region->region_ops->region_preconfig(region, &config_info, info); > if (err) > return err; > > /* The common API fpga_region_program_fpga() is used to program > * the image to hardware. > */ > region->info = info; > err = fpga_region_program_fpga(region); > if (err) { > fpga_image_info_free(info); > region->info = NULL; > } > > /* A vendor-specific callback is used for real enumeration, enabling > * hardware specific customization. > */ > err = region->region_ops->region_enumeration(region, &config_info); > > break; > > case FPGA_REGION_IOCTL_REMOVE: > if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) > return -EFAULT; > > err = region->region_ops->region_remove(region, &config_info); > if (err) > return err; > > fpga_image_info_free(region->info); > > break; > > case FPGA_REGION_IOCTL_STATUS: > unsigned int status; > > status = region->region_ops->region_status(region); > > if (copy_to_user((void __user *)arg, &status, sizeof(status))) > err = -EFAULT; > > break; > > default: > err = -ENOTTY; > } > > return err; > } > > Regards, > Navakishore.
Hi Yilun, > -----Original Message----- > From: Xu Yilun <yilun.xu@linux.intel.com> > Sent: Tuesday, December 10, 2024 2:34 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-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime > FPGA programming > > On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote: > > Hi Yilun, > > > > > -----Original Message----- > > > From: Xu Yilun <yilun.xu@linux.intel.com> > > > Sent: Wednesday, November 27, 2024 7:20 AM > > > 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-kernel@vger.kernel.org; > > > linux-fpga@vger.kernel.org; devicetree@vger.kernel.org > > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface > > > for runtime FPGA programming > > > > > > > > > + * 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 *region); > > > > > > + int (*region_config_enumeration)(struct fpga_region *region, > > > > > > + struct fpga_region_config_info > *config_info); > > > > > > > > > > My current concern is still about this combined API, it just > > > > > offloads all work to low level, but we have some common flows. > > > > > That's why we introduce a common FPGA reprograming API. > > > > > > > > > > I didn't see issue about the vendor specific pre configuration. > > > > > They are generally needed to initialize the struct > > > > > fpga_image_info, which is a common structure for > fpga_region_program_fpga(). > > > > > > > > > > For port IDs(AFU) inputs for DFL, I think it could also be > > > > > changed (Don't have to be implemented in this patchset). > > > > > Previously DFL provides an uAPI for the whole device, so it > > > > > needs a port_id input to position which fpga_region within the > > > > > device for programming. But now, we are introducing a per > > > > > fpga_region programming interface, IIUC port_id > > > should not be needed anymore. > > > > > > > > > > The combined API is truly simple for leveraging the existing > > > > > of-fpga-region overlay apply mechanism. But IMHO that flow > > > > > doesn't fit our new uAPI well. That flow is to adapt the generic > > > > > configfs overlay interface, which comes to a dead end as you mentioned. > > > > > > > > > > My gut feeling for the generic programing flow should be: > > > > > > > > > > 1. Program the image to HW. > > > > > 2. Enumerate the programmed image (apply the DT overlay) > > > > > > > > > > Why we have to: > > > > > > > > > > 1. Start enumeration. > > > > > 2. On pre enumeration, programe the image. > > > > > 3. Real enumeration. > > > > > > > > > > > > > I agree with the approach of leveraging vendor-specific callbacks > > > > to handle the distinct phases of the FPGA programming process. > > > > Here's the proposed flow. > > > > > > > > Pre-Configuration: > > > > A vendor-specific callback extracts the required pre-configuration > > > > details and initializes struct fpga_image_info. This ensures that > > > > all vendor-specific > > > > > > Since we need to construct the fpga_image_info, initialize multiple > > > field as needed, I'm wondering if configfs could be a solution for the uAPI? > > > > > > > A configfs uAPI isn't necessary, we can manage this using the proposed IOCTL > flow. > > The POC code looks as follows. > > I prefer more to configfs cause it provides standard FS way to create the > fpga_image_info object, e.g. which attributes are visible for OF/non-OF region, which > attributes come from image blob and can only be RO, etc. > > Of couse ioctl() could achieve the same goal but would add much more specific rules > (maybe flags/types) for user to follow. > Agreed. Using ConfigFS is preferable because it provides a standardized filesystem interface for creating and managing the fpga_image_info object. The proposed new user interface is outlined as follows: # Mount ConfigFS filesystem mount -t configfs none /sys/kernel/config # Upload Configuration and Load the Bitstream for the Targeted FPGA Region. Configuration File Upload: Upload the configuration file containing the necessary metadata or settings required for configuring the FPGA region. This file may vary based on the vendor and includes important details specific to the vendor's requirements. Vendor-Specific Callback: A vendor-specific callback function extracts the relevant configuration data from the file. The format and contents of the configuration file can differ between vendors. The callback then initializes the struct fpga_image_info, ensuring all vendor-specific requirements are satisfied. Device-Specific Considerations: For Open Firmware (OF) devices, fpga.dtbo files are used instead of fpga_config files. These .dtbo files contain all necessary information to populate the fpga_image_info. For non-OF devices, a vendor specific fpga.config files are used to provide the required data for initializing the fpga_image_info. FPGA Configuration: Once the configuration details are extracted and the fpga_image_info structure is initialized, the FPGA can be programmed accordingly. echo "config_file" > /sys/kernel/config/fpga/<region>/config # Check the status of "region" cat /sys/kernel/config/fpga/<region>/status # Remove "region" echo "remove" > /sys/kernel/config/fpga/<region>/remove Looking forward to your feedback. Regards, Navakishore.
On 2024-11-28 02:34, Xu Yilun wrote: > On Mon, Nov 25, 2024 at 12:26:06PM +0100, Marco Pagani wrote: >> On 2024-11-19 05:14, Xu Yilun wrote: >>> On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote: >>>> Introduces an IOCTL interface within the fpga-region subsystem, >>>> providing a generic and standardized mechanism for configuring (or) >>>> reprogramming FPGAs during runtime. The newly added interface supports >>>> both OF (Open Firmware) and non-OF devices, leveraging vendor-specific >>>> callbacks (e.g., configuration + enumeration, removal, and status) to >>>> accommodate a wide range of device specific configurations. >>>> >>>> The IOCTL interface ensures compatibility with both OF and non-OF >>>> devices, allowing for seamless FPGA reprogramming across diverse >>>> platforms. >>>> >>>> Vendor-specific callbacks are integrated into the interface, enabling >>>> custom FPGA configuration + enumeration, removal, and status reporting >>>> mechanisms, ensuring flexibility for vendor implementations. >>>> >>>> This solution enhances FPGA runtime management, supporting various device >>>> types and vendors, while ensuring compatibility with the current FPGA >>>> configuration flow. >>>> >>>> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com> >>>> --- >>>> Changes for v2: >>>> - As discussed with Yilun, the implementation has been modified to utilize a >>>> callback approach, enabling seamless handling of both OF and non-OF devices. >>>> >>>> - As suggested by Yilun in the POC code, we have moved away from using void *args >>>> as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are >>>> utilizing the fpga_region_config_info structure to gather user inputs. Currently, >>>> this structure is implemented to support only OF devices, but we intend to extend >>>> it by incorporating new members to accommodate non-OF devices in the future. >>>> >>>> drivers/fpga/fpga-region.c | 110 +++++++++++++++++++++++++++++++ >>>> drivers/fpga/of-fpga-region.c | 91 ++++++++++++++++++++++++- >>>> include/linux/fpga/fpga-region.h | 32 +++++++++ >>>> include/uapi/linux/fpga-region.h | 51 ++++++++++++++ >>>> 4 files changed, 283 insertions(+), 1 deletion(-) >>>> create mode 100644 include/uapi/linux/fpga-region.h >>>> >>>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c >>>> index 753cd142503e..c6bea3c99a69 100644 >>>> --- a/drivers/fpga/fpga-region.c >>>> +++ b/drivers/fpga/fpga-region.c >>>> @@ -8,6 +8,7 @@ >>>> #include <linux/fpga/fpga-bridge.h> >>>> #include <linux/fpga/fpga-mgr.h> >>>> #include <linux/fpga/fpga-region.h> >>>> +#include <linux/fpga-region.h> >>>> #include <linux/idr.h> >>>> #include <linux/kernel.h> >>>> #include <linux/list.h> >>>> @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = { >>>> }; >>>> ATTRIBUTE_GROUPS(fpga_region); >>>> >>>> +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 int fpga_region_device_release(struct inode *inode, struct file *file) >>>> +{ >>>> + return 0; >>>> +} >>>> + >>>> +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, >>>> + unsigned long arg) >>>> +{ >>>> + int err; >>>> + void __user *argp = (void __user *)arg; >>>> + struct fpga_region_config_info config_info; >>>> + struct fpga_region *region = (struct fpga_region *)(file->private_data); >>>> + >>>> + switch (cmd) { >>>> + case FPGA_REGION_IOCTL_LOAD: >>>> + if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) >>>> + return -EFAULT; >>>> + >>>> + err = region->region_ops->region_config_enumeration(region, &config_info); >>>> + >>>> + break; >>>> + case FPGA_REGION_IOCTL_REMOVE: >>>> + if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) >>>> + return -EFAULT; >>>> + >>>> + err = region->region_ops->region_remove(region, &config_info); >>>> + >>>> + break; >>>> + case FPGA_REGION_IOCTL_STATUS: >>>> + unsigned int status; >>>> + >>>> + status = region->region_ops->region_status(region); >>>> + >>>> + if (copy_to_user((void __user *)arg, &status, sizeof(status))) >>>> + err = -EFAULT; >>>> + break; >>>> + default: >>>> + err = -ENOTTY; >>>> + } >>>> + >>>> + return err; >>>> +} >>>> + >>>> +static const struct file_operations fpga_region_fops = { >>>> + .owner = THIS_MODULE, >>>> + .open = fpga_region_device_open, >>>> + .release = fpga_region_device_release, >>>> + .unlocked_ioctl = fpga_region_device_ioctl, >>>> + .compat_ioctl = fpga_region_device_ioctl, >>>> +}; >>>> + >>>> /** >>>> * __fpga_region_register_full - create and register an FPGA Region device >>>> * @parent: device parent >>>> @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info >>>> if (ret) >>>> goto err_remove; >>>> >>>> + if (info->region_ops) { >>>> + region->region_ops = info->region_ops; >>>> + region->miscdev.minor = MISC_DYNAMIC_MINOR; >>>> + region->miscdev.name = kobject_name(®ion->dev.kobj); >>>> + region->miscdev.fops = &fpga_region_fops; >>>> + ret = misc_register(®ion->miscdev); >>>> + if (ret) { >>>> + pr_err("fpga-region: failed to register misc device.\n"); >>>> + goto err_remove; >>>> + } >>>> + } >>>> + >>>> ret = device_register(®ion->dev); >>>> if (ret) { >>>> + misc_deregister(®ion->miscdev); >>>> put_device(®ion->dev); >>>> return ERR_PTR(ret); >>>> } >>>> @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr, >>>> } >>>> EXPORT_SYMBOL_GPL(__fpga_region_register); >>>> >>>> +/** >>>> + * __fpga_region_register_with_ops - create and register an FPGA Region device >>>> + * with user interface call-backs. >>>> + * @parent: device parent >>>> + * @mgr: manager that programs this region >>>> + * @region_ops: ops for low level FPGA region for device enumeration/removal >>>> + * @priv: of-fpga-region private data >>>> + * @get_bridges: optional function to get bridges to a list >>>> + * @owner: module containing the get_bridges function >>>> + * >>>> + * This simple version of the register function should be sufficient for most users. >>>> + * The fpga_region_register_full() function is available for users that need to >>>> + * pass additional, optional parameters. >>>> + * >>>> + * Return: struct fpga_region or ERR_PTR() >>>> + */ >>>> +struct fpga_region * >>>> +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr, >>>> + const struct fpga_region_ops *region_ops, >>>> + void *priv, >>>> + int (*get_bridges)(struct fpga_region *), >>>> + struct module *owner) >>>> +{ >>>> + struct fpga_region_info info = { 0 }; >>>> + >>>> + info.mgr = mgr; >>>> + info.priv = priv; >>>> + info.get_bridges = get_bridges; >>>> + info.region_ops = region_ops; >>>> + >>>> + return __fpga_region_register_full(parent, &info, owner); >>>> +} >>>> +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops); >>>> + >>>> /** >>>> * fpga_region_unregister - unregister an FPGA region >>>> * @region: FPGA region >>>> @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register); >>>> */ >>>> void fpga_region_unregister(struct fpga_region *region) >>>> { >>>> + misc_deregister(®ion->miscdev); >>>> device_unregister(®ion->dev); >>>> } >>>> EXPORT_SYMBOL_GPL(fpga_region_unregister); >>>> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c >>>> index 8526a5a86f0c..63fe56e0466f 100644 >>>> --- a/drivers/fpga/of-fpga-region.c >>>> +++ b/drivers/fpga/of-fpga-region.c >>>> @@ -8,6 +8,8 @@ >>>> #include <linux/fpga/fpga-bridge.h> >>>> #include <linux/fpga/fpga-mgr.h> >>>> #include <linux/fpga/fpga-region.h> >>>> +#include <linux/firmware.h> >>>> +#include <linux/fpga-region.h> >>>> #include <linux/idr.h> >>>> #include <linux/kernel.h> >>>> #include <linux/list.h> >>>> @@ -18,6 +20,20 @@ >>>> #include <linux/slab.h> >>>> #include <linux/spinlock.h> >>>> >>>> +/** >>>> + * struct of_fpga_region_priv - Private data structure >>>> + * image. >>>> + * @dev: Device data structure >>>> + * @fw: firmware of coeff table. >>>> + * @path: path of FPGA overlay image firmware file. >>>> + * @ovcs_id: overlay changeset id. >>>> + */ >>>> +struct of_fpga_region_priv { >>>> + struct device *dev; >>>> + const struct firmware *fw; >>>> + int ovcs_id; >>>> +}; >>>> + >>>> static const struct of_device_id fpga_region_of_match[] = { >>>> { .compatible = "fpga-region", }, >>>> {}, >>>> @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = { >>>> .notifier_call = of_fpga_region_notify, >>>> }; >>>> >>>> +static int of_fpga_region_status(struct fpga_region *region) >>>> +{ >>>> + struct of_fpga_region_priv *ovcs = region->priv; >>>> + >>>> + if (ovcs->ovcs_id) >>>> + return FPGA_REGION_HAS_PL; >>> >>> Could you help specify what is PL? >>> >>>> + >>>> + return FPGA_REGION_EMPTY; >>>> +} >>>> + >>>> +static int of_fpga_region_config_enumeration(struct fpga_region *region, >>>> + struct fpga_region_config_info *config_info) >>>> +{ >>>> + struct of_fpga_region_priv *ovcs = region->priv; >>>> + int err; >>>> + >>>> + /* if it's set do not allow changes */ >>>> + if (ovcs->ovcs_id) >>>> + return -EPERM; >>>> + >>>> + err = request_firmware(&ovcs->fw, config_info->firmware_name, 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 0; >>>> + >>>> +out_err: >>>> + ovcs->ovcs_id = 0; >>>> + ovcs->fw = NULL; >>>> + >>>> + return err; >>>> +} >>>> + >>>> +static int of_fpga_region_config_remove(struct fpga_region *region, >>>> + struct fpga_region_config_info *config_info) >>>> +{ >>>> + struct of_fpga_region_priv *ovcs = region->priv; >>>> + >>>> + if (!ovcs->ovcs_id) >>>> + return -EPERM; >>>> + >>>> + of_overlay_remove(&ovcs->ovcs_id); >>>> + release_firmware(ovcs->fw); >>>> + >>>> + ovcs->ovcs_id = 0; >>>> + ovcs->fw = NULL; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct fpga_region_ops region_ops = { >>>> + .region_status = of_fpga_region_status, >>>> + .region_config_enumeration = of_fpga_region_config_enumeration, >>>> + .region_remove = of_fpga_region_config_remove, >>>> +}; >>>> + >>>> static int of_fpga_region_probe(struct platform_device *pdev) >>>> { >>>> struct device *dev = &pdev->dev; >>>> struct device_node *np = dev->of_node; >>>> + struct of_fpga_region_priv *priv; >>>> struct fpga_region *region; >>>> struct fpga_manager *mgr; >>>> int ret; >>>> >>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>>> + if (!priv) >>>> + return -ENOMEM; >>>> + >>>> + priv->dev = dev; >>>> + >>>> /* Find the FPGA mgr specified by region or parent region. */ >>>> mgr = of_fpga_region_get_mgr(np); >>>> if (IS_ERR(mgr)) >>>> return -EPROBE_DEFER; >>>> >>>> - region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges); >>>> + region = fpga_region_register_with_ops(dev, mgr, ®ion_ops, priv, >>>> + of_fpga_region_get_bridges); >>>> if (IS_ERR(region)) { >>>> ret = PTR_ERR(region); >>>> goto eprobe_mgr_put; >>>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h >>>> index 5fbc05fe70a6..3a3ba6dbb5e1 100644 >>>> --- a/include/linux/fpga/fpga-region.h >>>> +++ b/include/linux/fpga/fpga-region.h >>>> @@ -6,15 +6,35 @@ >>>> #include <linux/device.h> >>>> #include <linux/fpga/fpga-mgr.h> >>>> #include <linux/fpga/fpga-bridge.h> >>>> +#include <linux/fpga-region.h> >>>> +#include <linux/miscdevice.h> >>>> >>>> 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 *region); >>>> + int (*region_config_enumeration)(struct fpga_region *region, >>>> + struct fpga_region_config_info *config_info); >>> >>> My current concern is still about this combined API, it just offloads >>> all work to low level, but we have some common flows. That's why we >>> introduce a common FPGA reprograming API. >>> >>> I didn't see issue about the vendor specific pre configuration. They >>> are generally needed to initialize the struct fpga_image_info, which >>> is a common structure for fpga_region_program_fpga(). >>> >>> For port IDs(AFU) inputs for DFL, I think it could also be changed >>> (Don't have to be implemented in this patchset). Previously DFL >>> provides an uAPI for the whole device, so it needs a port_id input to >>> position which fpga_region within the device for programming. But now, >>> we are introducing a per fpga_region programming interface, IIUC port_id >>> should not be needed anymore. >>> >>> The combined API is truly simple for leveraging the existing >>> of-fpga-region overlay apply mechanism. But IMHO that flow doesn't fit >>> our new uAPI well. That flow is to adapt the generic configfs overlay >>> interface, which comes to a dead end as you mentioned. >>> >>> My gut feeling for the generic programing flow should be: >>> >>> 1. Program the image to HW. >>> 2. Enumerate the programmed image (apply the DT overlay) >>> >>> Why we have to: >>> >>> 1. Start enumeration. >>> 2. On pre enumeration, programe the image. >>> 3. Real enumeration. >> >> I'm currently working on an RFC to propose a rework of the fpga >> subsystem in order to make it more aligned with the device model. One of >> the ideas I'm experimenting with is having a bus (struct bus_type) for >> fpga regions (devices) so that we can have region drivers that could >> handle internal device enumeration/management whenever a new region is >> configured on the fabric. Does this make sense in your opinions? > > mm.. I didn't fully understand the need to have a region driver, what's > the issue to solve? > Sorry for the late reply. The general idea is to handle regions in a way that is more aligned with the device model without having to resort to extra ops and additional devices. Having an fpga bus would allow us to handle enumeration using proper region drivers (in the device model sense of the term, i.e., struct device_driver) instead of derived region devices. On second thought, I think having a reconfiguration interface at the fpga manager level is sounder than having it at the region level (one for each region). With that in place, the fpga manager could request a firmware image, parse it, write the content into the fpga configuration memory, and then instantiate the region devices and add them to its fpga bus. Then, if there is a match, a specific region driver can handle the enumeration within the new region. What do you think? Thanks, Marco
> >> I'm currently working on an RFC to propose a rework of the fpga > >> subsystem in order to make it more aligned with the device model. One of > >> the ideas I'm experimenting with is having a bus (struct bus_type) for > >> fpga regions (devices) so that we can have region drivers that could > >> handle internal device enumeration/management whenever a new region is > >> configured on the fabric. Does this make sense in your opinions? > > > > mm.. I didn't fully understand the need to have a region driver, what's > > the issue to solve? > > > > Sorry for the late reply. The general idea is to handle regions in a way > that is more aligned with the device model without having to resort to > extra ops and additional devices. > > Having an fpga bus would allow us to handle enumeration using proper > region drivers (in the device model sense of the term, i.e., struct > device_driver) instead of derived region devices. > > On second thought, I think having a reconfiguration interface at the > fpga manager level is sounder than having it at the region level (one > for each region). I don't think so. A firmware image may contain enumeration info, e.g. of-fpga-region. And I think the fpga-region should parse these enumeration info rather than fpga manager. fpga manager should only deal with content writing stuff and not be exposed to user. > > With that in place, the fpga manager could request a firmware image, > parse it, write the content into the fpga configuration memory, and then > instantiate the region devices and add them to its fpga bus. Then, if I think an fpga-region is always there no matter it is cleared, being reprogrammed, or working. So I don't think an fpga-region needs to be re-instantated. The sub devices inside fpga-region needs re-instantating. That's also why I'm hesitating to fpga bus. Thanks, Yilun > there is a match, a specific region driver can handle the enumeration > within the new region. > > What do you think? > > Thanks, > Marco >
Hi Yilun, > -----Original Message----- > From: Xu Yilun <yilun.xu@linux.intel.com> > Sent: Sunday, March 19, 2023 9:08 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-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime > FPGA programming > > On Thu, Dec 19, 2024 at 09:47:12AM +0000, Manne, Nava kishore wrote: > > Hi Yilun, > > > > > -----Original Message----- > > > From: Xu Yilun <yilun.xu@linux.intel.com> > > > Sent: Tuesday, December 10, 2024 2:34 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-kernel@vger.kernel.org; > > > linux-fpga@vger.kernel.org; devicetree@vger.kernel.org > > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface > > > for runtime FPGA programming > > > > > > On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote: > > > > Hi Yilun, > > > > > > > > > -----Original Message----- > > > > > From: Xu Yilun <yilun.xu@linux.intel.com> > > > > > Sent: Wednesday, November 27, 2024 7:20 AM > > > > > 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-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; > > > > > devicetree@vger.kernel.org > > > > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL > > > > > interface for runtime FPGA programming > > > > > > > > > > > > > + * 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 *region); > > > > > > > > + int (*region_config_enumeration)(struct fpga_region *region, > > > > > > > > + struct fpga_region_config_info > > > *config_info); > > > > > > > > > > > > > > My current concern is still about this combined API, it just > > > > > > > offloads all work to low level, but we have some common flows. > > > > > > > That's why we introduce a common FPGA reprograming API. > > > > > > > > > > > > > > I didn't see issue about the vendor specific pre configuration. > > > > > > > They are generally needed to initialize the struct > > > > > > > fpga_image_info, which is a common structure for > > > fpga_region_program_fpga(). > > > > > > > > > > > > > > For port IDs(AFU) inputs for DFL, I think it could also be > > > > > > > changed (Don't have to be implemented in this patchset). > > > > > > > Previously DFL provides an uAPI for the whole device, so it > > > > > > > needs a port_id input to position which fpga_region within > > > > > > > the device for programming. But now, we are introducing a > > > > > > > per fpga_region programming interface, IIUC port_id > > > > > should not be needed anymore. > > > > > > > > > > > > > > The combined API is truly simple for leveraging the existing > > > > > > > of-fpga-region overlay apply mechanism. But IMHO that flow > > > > > > > doesn't fit our new uAPI well. That flow is to adapt the > > > > > > > generic configfs overlay interface, which comes to a dead end as you > mentioned. > > > > > > > > > > > > > > My gut feeling for the generic programing flow should be: > > > > > > > > > > > > > > 1. Program the image to HW. > > > > > > > 2. Enumerate the programmed image (apply the DT overlay) > > > > > > > > > > > > > > Why we have to: > > > > > > > > > > > > > > 1. Start enumeration. > > > > > > > 2. On pre enumeration, programe the image. > > > > > > > 3. Real enumeration. > > > > > > > > > > > > > > > > > > > I agree with the approach of leveraging vendor-specific > > > > > > callbacks to handle the distinct phases of the FPGA programming process. > > > > > > Here's the proposed flow. > > > > > > > > > > > > Pre-Configuration: > > > > > > A vendor-specific callback extracts the required > > > > > > pre-configuration details and initializes struct > > > > > > fpga_image_info. This ensures that all vendor-specific > > > > > > > > > > Since we need to construct the fpga_image_info, initialize > > > > > multiple field as needed, I'm wondering if configfs could be a solution for the > uAPI? > > > > > > > > > > > > > A configfs uAPI isn't necessary, we can manage this using the > > > > proposed IOCTL > > > flow. > > > > The POC code looks as follows. > > > > > > I prefer more to configfs cause it provides standard FS way to > > > create the fpga_image_info object, e.g. which attributes are visible > > > for OF/non-OF region, which attributes come from image blob and can only be > RO, etc. > > > > > > Of couse ioctl() could achieve the same goal but would add much more > > > specific rules (maybe flags/types) for user to follow. > > > > > > > Agreed. Using ConfigFS is preferable because it provides a > > standardized filesystem interface for creating and managing the fpga_image_info > object. > > > > The proposed new user interface is outlined as follows: > > > > # Mount ConfigFS filesystem > > mount -t configfs none /sys/kernel/config > > > > # Upload Configuration and Load the Bitstream for the Targeted FPGA Region. > > > > Configuration File Upload: > > Upload the configuration file containing the necessary metadata or > > settings required for configuring the FPGA region. This file may vary > > based on the vendor and includes important details specific to the vendor's > requirements. > > > > Vendor-Specific Callback: > > A vendor-specific callback function extracts the relevant configuration data from the > file. > > The format and contents of the configuration file can differ between > > vendors. The callback then initializes the struct fpga_image_info, > > ensuring all vendor-specific requirements are satisfied. > > > > Device-Specific Considerations: > > For Open Firmware (OF) devices, fpga.dtbo files are used instead of fpga_config > files. > > These .dtbo files contain all necessary information to populate the > fpga_image_info. > > For non-OF devices, a vendor specific fpga.config files are used to > > provide the required data for initializing the fpga_image_info. > > non-OF fpga images usually don't contain fpga_image_info data (e.g. > enable/disable_timeout_us). I think we don't have to force users embed these data in > fpga image, provide additional configfs attributes to input these data is possible. For > some FPGA regions (e.g. OF), these attributes could be RO, some could be RW, > depends on different FPGA region drivers. > > So I think we may have a Configuration File Upload interface, like: > > echo "config_file" > /sys/kernel/config/fpga/<region>/image > > Some additional parameter interfaces, like: > > echo 10000 > /sys/kernel/config/fpga/<region>/enable_timeout > ... > > And a Configuration interface, like: > > # programming > echo 1 > /sys/kernel/config/fpga/<region>/config > # removing > echo 0 > /sys/kernel/config/fpga/<region>/config > > How do you think? > I agree and am actively working on the POC changes. I will submit the RFC patches at the earliest. Regards, Navakishore.
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index 753cd142503e..c6bea3c99a69 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -8,6 +8,7 @@ #include <linux/fpga/fpga-bridge.h> #include <linux/fpga/fpga-mgr.h> #include <linux/fpga/fpga-region.h> +#include <linux/fpga-region.h> #include <linux/idr.h> #include <linux/kernel.h> #include <linux/list.h> @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = { }; ATTRIBUTE_GROUPS(fpga_region); +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 int fpga_region_device_release(struct inode *inode, struct file *file) +{ + return 0; +} + +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + int err; + void __user *argp = (void __user *)arg; + struct fpga_region_config_info config_info; + struct fpga_region *region = (struct fpga_region *)(file->private_data); + + switch (cmd) { + case FPGA_REGION_IOCTL_LOAD: + if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) + return -EFAULT; + + err = region->region_ops->region_config_enumeration(region, &config_info); + + break; + case FPGA_REGION_IOCTL_REMOVE: + if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info))) + return -EFAULT; + + err = region->region_ops->region_remove(region, &config_info); + + break; + case FPGA_REGION_IOCTL_STATUS: + unsigned int status; + + status = region->region_ops->region_status(region); + + if (copy_to_user((void __user *)arg, &status, sizeof(status))) + err = -EFAULT; + break; + default: + err = -ENOTTY; + } + + return err; +} + +static const struct file_operations fpga_region_fops = { + .owner = THIS_MODULE, + .open = fpga_region_device_open, + .release = fpga_region_device_release, + .unlocked_ioctl = fpga_region_device_ioctl, + .compat_ioctl = fpga_region_device_ioctl, +}; + /** * __fpga_region_register_full - create and register an FPGA Region device * @parent: device parent @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info if (ret) goto err_remove; + if (info->region_ops) { + region->region_ops = info->region_ops; + region->miscdev.minor = MISC_DYNAMIC_MINOR; + region->miscdev.name = kobject_name(®ion->dev.kobj); + region->miscdev.fops = &fpga_region_fops; + ret = misc_register(®ion->miscdev); + if (ret) { + pr_err("fpga-region: failed to register misc device.\n"); + goto err_remove; + } + } + ret = device_register(®ion->dev); if (ret) { + misc_deregister(®ion->miscdev); put_device(®ion->dev); return ERR_PTR(ret); } @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr, } EXPORT_SYMBOL_GPL(__fpga_region_register); +/** + * __fpga_region_register_with_ops - create and register an FPGA Region device + * with user interface call-backs. + * @parent: device parent + * @mgr: manager that programs this region + * @region_ops: ops for low level FPGA region for device enumeration/removal + * @priv: of-fpga-region private data + * @get_bridges: optional function to get bridges to a list + * @owner: module containing the get_bridges function + * + * This simple version of the register function should be sufficient for most users. + * The fpga_region_register_full() function is available for users that need to + * pass additional, optional parameters. + * + * Return: struct fpga_region or ERR_PTR() + */ +struct fpga_region * +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr, + const struct fpga_region_ops *region_ops, + void *priv, + int (*get_bridges)(struct fpga_region *), + struct module *owner) +{ + struct fpga_region_info info = { 0 }; + + info.mgr = mgr; + info.priv = priv; + info.get_bridges = get_bridges; + info.region_ops = region_ops; + + return __fpga_region_register_full(parent, &info, owner); +} +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops); + /** * fpga_region_unregister - unregister an FPGA region * @region: FPGA region @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register); */ void fpga_region_unregister(struct fpga_region *region) { + misc_deregister(®ion->miscdev); device_unregister(®ion->dev); } EXPORT_SYMBOL_GPL(fpga_region_unregister); diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c index 8526a5a86f0c..63fe56e0466f 100644 --- a/drivers/fpga/of-fpga-region.c +++ b/drivers/fpga/of-fpga-region.c @@ -8,6 +8,8 @@ #include <linux/fpga/fpga-bridge.h> #include <linux/fpga/fpga-mgr.h> #include <linux/fpga/fpga-region.h> +#include <linux/firmware.h> +#include <linux/fpga-region.h> #include <linux/idr.h> #include <linux/kernel.h> #include <linux/list.h> @@ -18,6 +20,20 @@ #include <linux/slab.h> #include <linux/spinlock.h> +/** + * struct of_fpga_region_priv - Private data structure + * image. + * @dev: Device data structure + * @fw: firmware of coeff table. + * @path: path of FPGA overlay image firmware file. + * @ovcs_id: overlay changeset id. + */ +struct of_fpga_region_priv { + struct device *dev; + const struct firmware *fw; + int ovcs_id; +}; + static const struct of_device_id fpga_region_of_match[] = { { .compatible = "fpga-region", }, {}, @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = { .notifier_call = of_fpga_region_notify, }; +static int of_fpga_region_status(struct fpga_region *region) +{ + struct of_fpga_region_priv *ovcs = region->priv; + + if (ovcs->ovcs_id) + return FPGA_REGION_HAS_PL; + + return FPGA_REGION_EMPTY; +} + +static int of_fpga_region_config_enumeration(struct fpga_region *region, + struct fpga_region_config_info *config_info) +{ + struct of_fpga_region_priv *ovcs = region->priv; + int err; + + /* if it's set do not allow changes */ + if (ovcs->ovcs_id) + return -EPERM; + + err = request_firmware(&ovcs->fw, config_info->firmware_name, 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 0; + +out_err: + ovcs->ovcs_id = 0; + ovcs->fw = NULL; + + return err; +} + +static int of_fpga_region_config_remove(struct fpga_region *region, + struct fpga_region_config_info *config_info) +{ + struct of_fpga_region_priv *ovcs = region->priv; + + if (!ovcs->ovcs_id) + return -EPERM; + + of_overlay_remove(&ovcs->ovcs_id); + release_firmware(ovcs->fw); + + ovcs->ovcs_id = 0; + ovcs->fw = NULL; + + return 0; +} + +static const struct fpga_region_ops region_ops = { + .region_status = of_fpga_region_status, + .region_config_enumeration = of_fpga_region_config_enumeration, + .region_remove = of_fpga_region_config_remove, +}; + static int of_fpga_region_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; + struct of_fpga_region_priv *priv; struct fpga_region *region; struct fpga_manager *mgr; int ret; + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->dev = dev; + /* Find the FPGA mgr specified by region or parent region. */ mgr = of_fpga_region_get_mgr(np); if (IS_ERR(mgr)) return -EPROBE_DEFER; - region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges); + region = fpga_region_register_with_ops(dev, mgr, ®ion_ops, priv, + of_fpga_region_get_bridges); if (IS_ERR(region)) { ret = PTR_ERR(region); goto eprobe_mgr_put; diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h index 5fbc05fe70a6..3a3ba6dbb5e1 100644 --- a/include/linux/fpga/fpga-region.h +++ b/include/linux/fpga/fpga-region.h @@ -6,15 +6,35 @@ #include <linux/device.h> #include <linux/fpga/fpga-mgr.h> #include <linux/fpga/fpga-bridge.h> +#include <linux/fpga-region.h> +#include <linux/miscdevice.h> 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 *region); + int (*region_config_enumeration)(struct fpga_region *region, + struct fpga_region_config_info *config_info); + int (*region_remove)(struct fpga_region *region, + struct fpga_region_config_info *config_info); +}; + /** * struct fpga_region_info - collection of parameters an FPGA Region * @mgr: fpga region manager * @compat_id: FPGA region id for compatibility check. * @priv: fpga region private data * @get_bridges: optional function to get bridges to a list + * @fpga_region_ops: ops for low level FPGA region ops for device + * enumeration/removal * * fpga_region_info contains parameters for the register_full function. * These are separated into an info structure because they some are optional @@ -26,6 +46,7 @@ struct fpga_region_info { struct fpga_compat_id *compat_id; void *priv; int (*get_bridges)(struct fpga_region *region); + const struct fpga_region_ops *region_ops; }; /** @@ -39,6 +60,8 @@ struct fpga_region_info { * @ops_owner: module containing the get_bridges function * @priv: private data * @get_bridges: optional function to get bridges to a list + * @fpga_region_ops: ops for low level FPGA region ops for device + * enumeration/removal */ struct fpga_region { struct device dev; @@ -50,6 +73,8 @@ struct fpga_region { struct module *ops_owner; void *priv; int (*get_bridges)(struct fpga_region *region); + const struct fpga_region_ops *region_ops; + struct miscdevice miscdev; }; #define to_fpga_region(d) container_of(d, struct fpga_region, dev) @@ -71,6 +96,13 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info struct fpga_region * __fpga_region_register(struct device *parent, struct fpga_manager *mgr, int (*get_bridges)(struct fpga_region *), struct module *owner); +#define fpga_region_register_with_ops(parent, mgr, region_ops, priv, get_bridges) \ + __fpga_region_register_with_ops(parent, mgr, region_ops, priv, get_bridges, THIS_MODULE) +struct fpga_region * +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr, + const struct fpga_region_ops *region_ops, void *priv, + int (*get_bridges)(struct fpga_region *), + struct module *owner); void fpga_region_unregister(struct fpga_region *region); #endif /* _FPGA_REGION_H */ diff --git a/include/uapi/linux/fpga-region.h b/include/uapi/linux/fpga-region.h new file mode 100644 index 000000000000..88ade83daf61 --- /dev/null +++ b/include/uapi/linux/fpga-region.h @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +/* + * Header File for FPGA Region User API + * + * Copyright (C) 2024 Advanced Micro Devices, Inc. + * + * Author: Manne, Nava kishore <nava.kishore.manne@amd.com> + */ + +#ifndef _UAPI_LINUX_FPGA_REGION_H +#define _UAPI_LINUX_FPGA_REGION_H + +#include <linux/ioctl.h> +#include <linux/limits.h> +#include <linux/types.h> + +/* IOCTLs for fpga region file descriptor */ +#define FPGA_REGION_MAGIC_NUMBER 'f' +#define FPGA_REGION_BASE 0 + +/** + * FPGA_REGION_IOCTL_LOAD - _IOW(FPGA_REGION_MAGIC, 0, + * struct fpga_region_config_info) + * + * FPGA_REGION_IOCTL_REMOVE - _IOW(FPGA_REGION_MAGIC, 1, + * struct fpga_region_config_info) + * + * Driver does Configuration/Reconfiguration based on Region ID and + * Buffer (Image) provided by caller. + * Return: 0 on success, -errno on failure. + */ +struct fpga_region_config_info { /* Input */ + char firmware_name[NAME_MAX]; /* Firmware file name */ +}; + +/* + * FPGA Region Control IOCTLs. + */ +#define FPGA_REGION_MAGIC 'f' +#define FPGA_IOW(num, dtype) _IOW(FPGA_REGION_MAGIC, num, dtype) +#define FPGA_IOR(num, dtype) _IOR(FPGA_REGION_MAGIC, num, dtype) + +#define FPGA_REGION_IOCTL_LOAD FPGA_IOW(0, __u32) +#define FPGA_REGION_IOCTL_REMOVE FPGA_IOW(1, __u32) +#define FPGA_REGION_IOCTL_STATUS FPGA_IOR(2, __u32) + +/* Region status possibilities returned by FPGA_REGION_IOCTL_STATUS ioctl */ +#define FPGA_REGION_HAS_PL 0 /* if the region has PL logic */ +#define FPGA_REGION_EMPTY 1 /* If the region is empty */ + +#endif /* _UAPI_LINUX_FPGA_REGION_H */
Introduces an IOCTL interface within the fpga-region subsystem, providing a generic and standardized mechanism for configuring (or) reprogramming FPGAs during runtime. The newly added interface supports both OF (Open Firmware) and non-OF devices, leveraging vendor-specific callbacks (e.g., configuration + enumeration, removal, and status) to accommodate a wide range of device specific configurations. The IOCTL interface ensures compatibility with both OF and non-OF devices, allowing for seamless FPGA reprogramming across diverse platforms. Vendor-specific callbacks are integrated into the interface, enabling custom FPGA configuration + enumeration, removal, and status reporting mechanisms, ensuring flexibility for vendor implementations. This solution enhances FPGA runtime management, supporting various device types and vendors, while ensuring compatibility with the current FPGA configuration flow. Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com> --- Changes for v2: - As discussed with Yilun, the implementation has been modified to utilize a callback approach, enabling seamless handling of both OF and non-OF devices. - As suggested by Yilun in the POC code, we have moved away from using void *args as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are utilizing the fpga_region_config_info structure to gather user inputs. Currently, this structure is implemented to support only OF devices, but we intend to extend it by incorporating new members to accommodate non-OF devices in the future. drivers/fpga/fpga-region.c | 110 +++++++++++++++++++++++++++++++ drivers/fpga/of-fpga-region.c | 91 ++++++++++++++++++++++++- include/linux/fpga/fpga-region.h | 32 +++++++++ include/uapi/linux/fpga-region.h | 51 ++++++++++++++ 4 files changed, 283 insertions(+), 1 deletion(-) create mode 100644 include/uapi/linux/fpga-region.h