diff mbox series

[RFC,1/1] of-fpga-region: Add sysfs interface support for FPGA configuration

Message ID 20240726063819.2274324-2-nava.kishore.manne@amd.com (mailing list archive)
State New
Headers show
Series Add user space interaction for FPGA programming | expand

Commit Message

Manne, Nava kishore July 26, 2024, 6:38 a.m. UTC
Adds sysfs interface as part of the of-fpga-region. This newly added
sysfs interface uses Device Tree Overlay (DTO) files to configure/reprogram
an FPGA while an operating system is running.This solution will not change
the existing sequence When a DT overlay that targets an FPGA Region is
applied.
	- Disable appropriate FPGA bridges.
	- Program the FPGA using the FPGA manager.
	- Enable the FPGA bridges.
	- The Device Tree overlay is accepted into the live tree.
	- Child devices are populated.

When the overlay is removed, the child nodes will be removed, and the FPGA
Region will disable the bridges.

Usage:
To configure/reprogram an FPGA region:
echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load

To remove an FPGA region:
echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/remove

To get an FPGA region status:
cat /sys/class/fpga_region/<region>/device/status

Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
---
 .../ABI/testing/sysfs-class-of-fpga-region    | 30 ++++++
 MAINTAINERS                                   |  1 +
 drivers/fpga/fpga-region.c                    |  4 +-
 drivers/fpga/of-fpga-region.c                 | 92 +++++++++++++++++++
 include/linux/fpga/fpga-region.h              | 15 +++
 5 files changed, 141 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-of-fpga-region

Comments

Xu Yilun July 29, 2024, 3:56 p.m. UTC | #1
On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote:
> Adds sysfs interface as part of the of-fpga-region. This newly added
> sysfs interface uses Device Tree Overlay (DTO) files to configure/reprogram
> an FPGA while an operating system is running.This solution will not change
> the existing sequence When a DT overlay that targets an FPGA Region is
> applied.
> 	- Disable appropriate FPGA bridges.
> 	- Program the FPGA using the FPGA manager.
> 	- Enable the FPGA bridges.
> 	- The Device Tree overlay is accepted into the live tree.
> 	- Child devices are populated.
> 
> When the overlay is removed, the child nodes will be removed, and the FPGA
> Region will disable the bridges.
> 
> Usage:
> To configure/reprogram an FPGA region:
> echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load

IIRC, last time we are considering some generic interface for both OF &
non-OF FPGA region, but this is still OF specific.

Thanks,
Yilun
Manne, Nava kishore Aug. 1, 2024, 4:25 a.m. UTC | #2
Hi Yilun,

> -----Original Message-----
> From: Xu Yilun <yilun.xu@linux.intel.com>
> Sent: Monday, July 29, 2024 9:27 PM
> To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com;
> yilun.xu@intel.com; trix@redhat.com; robh@kernel.org;
> saravanak@google.com; linux-fpga@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support for FPGA
> configuration
> 
> On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote:
> > Adds sysfs interface as part of the of-fpga-region. This newly added
> > sysfs interface uses Device Tree Overlay (DTO) files to
> > configure/reprogram an FPGA while an operating system is running.This
> > solution will not change the existing sequence When a DT overlay that
> > targets an FPGA Region is applied.
> > 	- Disable appropriate FPGA bridges.
> > 	- Program the FPGA using the FPGA manager.
> > 	- Enable the FPGA bridges.
> > 	- The Device Tree overlay is accepted into the live tree.
> > 	- Child devices are populated.
> >
> > When the overlay is removed, the child nodes will be removed, and the
> > FPGA Region will disable the bridges.
> >
> > Usage:
> > To configure/reprogram an FPGA region:
> > echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load
> 
> IIRC, last time we are considering some generic interface for both OF & non-
> OF FPGA region, but this is still OF specific.
> 
At AMD, we exclusively use OF for FPGA configuration/reconfiguration, utilizing overlay files as outlined in the fpga-region.txt documentation.
However, some devices, like dfl.c those relying solely on the FPGA region, do not use OF. 
For these non-OF devices, should we expect them to follow the fpga-region.txt guidelines for FPGA configuration/reconfiguration?
If so, it may be advantageous to develop a common interface for both OF and non-OF.
If not, it might be more appropriate to establish distinct interfaces to cater to their specific requirements.

Regards,
Navakishore.
Xu Yilun Aug. 5, 2024, 6:20 p.m. UTC | #3
On Thu, Aug 01, 2024 at 04:25:42AM +0000, Manne, Nava kishore wrote:
> Hi Yilun,
> 
> > -----Original Message-----
> > From: Xu Yilun <yilun.xu@linux.intel.com>
> > Sent: Monday, July 29, 2024 9:27 PM
> > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com;
> > yilun.xu@intel.com; trix@redhat.com; robh@kernel.org;
> > saravanak@google.com; linux-fpga@vger.kernel.org; linux-
> > kernel@vger.kernel.org; devicetree@vger.kernel.org
> > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support for FPGA
> > configuration
> > 
> > On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote:
> > > Adds sysfs interface as part of the of-fpga-region. This newly added
> > > sysfs interface uses Device Tree Overlay (DTO) files to
> > > configure/reprogram an FPGA while an operating system is running.This
> > > solution will not change the existing sequence When a DT overlay that
> > > targets an FPGA Region is applied.
> > > 	- Disable appropriate FPGA bridges.
> > > 	- Program the FPGA using the FPGA manager.
> > > 	- Enable the FPGA bridges.
> > > 	- The Device Tree overlay is accepted into the live tree.
> > > 	- Child devices are populated.
> > >
> > > When the overlay is removed, the child nodes will be removed, and the
> > > FPGA Region will disable the bridges.
> > >
> > > Usage:
> > > To configure/reprogram an FPGA region:
> > > echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load
> > 
> > IIRC, last time we are considering some generic interface for both OF & non-
> > OF FPGA region, but this is still OF specific.
> > 
> At AMD, we exclusively use OF for FPGA configuration/reconfiguration, utilizing overlay files as outlined in the fpga-region.txt documentation.
> However, some devices, like dfl.c those relying solely on the FPGA region, do not use OF. 
> For these non-OF devices, should we expect them to follow the fpga-region.txt guidelines for FPGA configuration/reconfiguration?

I assume it is Documentation/devicetree/bindings/fpga/fpga-region.yaml.

No, Non-OF devices don't have to follow the DT binding.

> If so, it may be advantageous to develop a common interface for both OF and non-OF.
> If not, it might be more appropriate to establish distinct interfaces to cater to their specific requirements.

I think each vendor may have specific way for device enumeration, but
that doesn't mean we need distinct user interfaces. For all FPGA
devices, we should avoid the situation that the HW is changed but
system SW knows nothing. So the common needs are:

 - Find out and remove all devices within the fpga region before
   reprograming.
 - Re-enumerate devices in fpga region after reprograming.

I expect the fpga region class could generally enforce a flow for
the reprograming interface. And of-fpga-region could specifically
implement it using DT overlay.

Thanks,
Yilun
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-class-of-fpga-region b/Documentation/ABI/testing/sysfs-class-of-fpga-region
new file mode 100644
index 000000000000..aeb4e3be4ff3
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-of-fpga-region
@@ -0,0 +1,30 @@ 
+What:		/sys/class/fpga_region/<region>/device/load
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Nava kishore Manne <nava.kishore.manne@amd.com>
+Description:	(WO) Configure/Reprogram an FPGA region.
+		It uses Device Tree Overlay (DTO) files to configurer (or)
+		reprogram an FPGA. While an operating system is running.
+		The bitstream and the relevant DTO file has to be located
+		on the appropriate firmware path, typically, /lib/firmware.
+		For example, when user pass the option "echo fpga.dtbo"	the
+		file /lib/firmware/fpga.dtbo must be present.
+
+What:		/sys/class/fpga_region/<region>/device/remove
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Nava kishore Manne <nava.kishore.manne@amd.com>
+Description:	(WO) Revert the changes added by the load interface
+		It revert and free an overlay changeset added by the load
+		interface.
+
+What:		/sys/class/fpga_region/<region>/device/status
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Nava kishore Manne <nava.kishore.manne@amd.com>
+Description:	(RO) Status of the FPGA region
+		This file is used to check the status of the FPGA region.
+		This is a list of strings for the supported status.
+
+		* applied	= FPGA is programmed and operating
+		* unapplied	= Error while programing the FPGA
diff --git a/MAINTAINERS b/MAINTAINERS
index c0a3d9e93689..384f1d6f3af9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8795,6 +8795,7 @@  L:	linux-fpga@vger.kernel.org
 S:	Maintained
 Q:	http://patchwork.kernel.org/project/linux-fpga/list/
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/fpga/linux-fpga.git
+F:	Documentation/ABI/testing/sysfs-class-of-fpga-region
 F:	Documentation/devicetree/bindings/fpga/
 F:	Documentation/driver-api/fpga/
 F:	Documentation/fpga/
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 753cd142503e..0733db1347ea 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -14,6 +14,7 @@ 
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/of.h>
 
 static DEFINE_IDA(fpga_region_ida);
 static const struct class fpga_region_class;
@@ -192,6 +193,7 @@  struct fpga_region *
 __fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
 			    struct module *owner)
 {
+	struct device_node *np = parent->of_node;
 	struct fpga_region *region;
 	int id, ret = 0;
 
@@ -225,7 +227,7 @@  __fpga_region_register_full(struct device *parent, const struct fpga_region_info
 	region->dev.of_node = parent->of_node;
 	region->dev.id = id;
 
-	ret = dev_set_name(&region->dev, "region%d", id);
+	ret = dev_set_name(&region->dev, "%s", of_node_full_name(np));
 	if (ret)
 		goto err_remove;
 
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index 8526a5a86f0c..edcc4c23a4b4 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -5,6 +5,7 @@ 
  *  Copyright (C) 2013-2016 Altera Corporation
  *  Copyright (C) 2017 Intel Corporation
  */
+#include <linux/firmware.h>
 #include <linux/fpga/fpga-bridge.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/fpga/fpga-region.h>
@@ -347,6 +348,7 @@  static int of_fpga_region_notify(struct notifier_block *nb,
 				 unsigned long action, void *arg)
 {
 	struct of_overlay_notify_data *nd = arg;
+	struct fpga_overlay_image_info *ovcs;
 	struct fpga_region *region;
 	int ret;
 
@@ -371,6 +373,10 @@  static int of_fpga_region_notify(struct notifier_block *nb,
 	if (!region)
 		return NOTIFY_OK;
 
+	ovcs = &region->ovcs;
+	if (!ovcs->fw)
+		return NOTIFY_STOP;
+
 	ret = 0;
 	switch (action) {
 	case OF_OVERLAY_PRE_APPLY:
@@ -394,6 +400,91 @@  static struct notifier_block fpga_region_of_nb = {
 	.notifier_call = of_fpga_region_notify,
 };
 
+static ssize_t load_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_overlay_image_info *ovcs = &region->ovcs;
+	char *s;
+	int err;
+
+	/* if it's set do not allow changes */
+	if (ovcs->ovcs_id)
+		return -EPERM;
+
+	/* copy to path buffer (and make sure it's always zero terminated */
+	count = snprintf(ovcs->path, sizeof(ovcs->path) - 1, "%s", buf);
+	ovcs->path[sizeof(ovcs->path) - 1] = '\0';
+
+	/* strip trailing newlines */
+	s = ovcs->path + strlen(ovcs->path);
+	while (s > ovcs->path && *--s == '\n')
+		*s = '\0';
+
+	err = request_firmware(&ovcs->fw, ovcs->path, NULL);
+	if (err != 0)
+		goto out_err;
+
+	err = of_overlay_fdt_apply((void *)ovcs->fw->data, ovcs->fw->size,
+				   &ovcs->ovcs_id, NULL);
+	if (err < 0) {
+		pr_err("%s: Failed to create overlay (err=%d)\n",
+		       __func__, err);
+		release_firmware(ovcs->fw);
+		goto out_err;
+	}
+
+	return count;
+out_err:
+	ovcs->path[0] = '\0';
+	ovcs->ovcs_id = 0;
+	ovcs->fw = NULL;
+
+	return err;
+}
+
+static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_overlay_image_info *ovcs = &region->ovcs;
+
+	if (!ovcs->ovcs_id)
+		return -EPERM;
+
+	of_overlay_remove(&ovcs->ovcs_id);
+	release_firmware(ovcs->fw);
+
+	ovcs->path[0] = '\0';
+	ovcs->ovcs_id = 0;
+	ovcs->fw = NULL;
+
+	return count;
+}
+
+static ssize_t status_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_overlay_image_info *ovcs = &region->ovcs;
+
+	return sprintf(buf, "%s\n", ovcs->ovcs_id > 0 ?
+		       "applied" : "unapplied");
+}
+
+static DEVICE_ATTR_WO(load);
+static DEVICE_ATTR_WO(remove);
+static DEVICE_ATTR_RO(status);
+
+static struct attribute *of_fpga_region_attrs[] = {
+	&dev_attr_load.attr,
+	&dev_attr_remove.attr,
+	&dev_attr_status.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(of_fpga_region);
+
 static int of_fpga_region_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -440,6 +531,7 @@  static struct platform_driver of_fpga_region_driver = {
 	.driver = {
 		.name	= "of-fpga-region",
 		.of_match_table = of_match_ptr(fpga_region_of_match),
+		.dev_groups = of_fpga_region_groups,
 	},
 };
 
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index 5fbc05fe70a6..36143301b49b 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -9,6 +9,19 @@ 
 
 struct fpga_region;
 
+/**
+ * struct fpga_overlay_image_info - information specific to an FPGA Overlay
+ * image.
+ * @fw: firmware of coeff table.
+ * @path: path of FPGA overlay image firmware file.
+ * @ovcs_id: overlay changeset id.
+ */
+struct fpga_overlay_image_info {
+	const struct firmware *fw;
+	char path[PATH_MAX];
+	int ovcs_id;
+};
+
 /**
  * struct fpga_region_info - collection of parameters an FPGA Region
  * @mgr: fpga region manager
@@ -37,6 +50,7 @@  struct fpga_region_info {
  * @info: FPGA image info
  * @compat_id: FPGA region id for compatibility check.
  * @ops_owner: module containing the get_bridges function
+ * @ovcs: FPGA overlay image info
  * @priv: private data
  * @get_bridges: optional function to get bridges to a list
  */
@@ -48,6 +62,7 @@  struct fpga_region {
 	struct fpga_image_info *info;
 	struct fpga_compat_id *compat_id;
 	struct module *ops_owner;
+	struct fpga_overlay_image_info ovcs;
 	void *priv;
 	int (*get_bridges)(struct fpga_region *region);
 };