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@xxxxxxx>
> ---
> 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@xxxxxxx>
> + */
> +
> +#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
>
>
