Message-ID: <20241029091734.3288005-2-nava.kishore.manne@amd.com>
Date: Tue, 29 Oct 2024 14:47:34 +0530
From: Nava kishore Manne <nava.kishore.manne@....com>
To: <git@....com>, <mdf@...nel.org>, <hao.wu@...el.com>, <yilun.xu@...el.com>,
	<trix@...hat.com>, <robh@...nel.org>, <saravanak@...gle.com>,
	<nava.kishore.manne@....com>, <linux-kernel@...r.kernel.org>,
	<linux-fpga@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

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@....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(&region->dev.kobj);
+		region->miscdev.fops = &fpga_region_fops;
+		ret = misc_register(&region->miscdev);
+		if (ret) {
+			pr_err("fpga-region: failed to register misc device.\n");
+			goto err_remove;
+		}
+	}
+
 	ret = device_register(&region->dev);
 	if (ret) {
+		misc_deregister(&region->miscdev);
 		put_device(&region->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(&region->miscdev);
 	device_unregister(&region->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, &region_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@....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

Powered by blists - more mailing lists