Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581426 > unrolled thread

FPGA Region enhancements and fixes

Back to article view | Back to linux.kernel

Contents

  FPGA Region enhancements and fixes Alan Tull <atull@kernel.org> - 2017-02-15 17:20 +0100
    [RFC 6/8] fpga-region: separate out common code to allow non-dt support Alan Tull <atull@kernel.org> - 2017-02-15 17:20 +0100
    [RFC 1/8] fpga-mgr: add a single function for fpga loading methods Alan Tull <atull@kernel.org> - 2017-02-15 17:20 +0100
      Re: [RFC 1/8] fpga-mgr: add a single function for fpga loading  methods matthew.gerlach@linux.intel.com - 2017-02-16 01:40 +0100
    [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager Alan Tull <atull@kernel.org> - 2017-02-15 17:20 +0100
      Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager "Li, Yi" <yi1.li@linux.intel.com> - 2017-02-17 18:20 +0100
        Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager Alan Tull <delicious.quinoa@gmail.com> - 2017-02-17 23:00 +0100
      Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager Moritz Fischer <mdf@kernel.org> - 2017-02-17 19:00 +0100
        Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager Alan Tull <delicious.quinoa@gmail.com> - 2017-02-17 23:10 +0100
    [RFC 2/8] fpga-region: support more than one overlay per FPGA region Alan Tull <atull@kernel.org> - 2017-02-15 17:20 +0100
      Re: [RFC 2/8] fpga-region: support more than one overlay per FPGA  region matthew.gerlach@linux.intel.com - 2017-02-16 18:00 +0100
        Re: [RFC 2/8] fpga-region: support more than one overlay per FPGA region Alan Tull <delicious.quinoa@gmail.com> - 2017-02-16 18:40 +0100
    Re: FPGA Region enhancements and fixes Alan Tull <delicious.quinoa@gmail.com> - 2017-02-28 18:40 +0100
      Re: FPGA Region enhancements and fixes Alan Tull <delicious.quinoa@gmail.com> - 2017-03-01 00:00 +0100

#1581426 — FPGA Region enhancements and fixes

This patchset intends to enable expanding the use of FPGA regions
beyond device tree overlays.  Also one fix for the existing DTO
implementation.  It's an RFC, looking for feedback, also I need
to do more testing and fix it working for modules.

Patch 1 adds a function so the caller could program the fpga from
either a scatter gather table, a buffer, or a firmware file.  The
parameters are passed in the fpga_image_info struct.  This way works,
but there may be a better or more widely accepted way.  Maybe they
should be a union?  If someone knows of a well written example in the
kernel for me to emulate, that would be really appreciated.

Patch 2 is a fix for if you write > 1 overlay to a region.
It keeps track of overlays in a list.

Patch 3 adds functions for working with FPGA bridges using the
device rather than the device node (for non DT use).

Patches 4-5 separate finding and locking a FPGA manager.  So someone
could get a reference for a FPGA manager without locking it for
exclusive use.

Patch 6 breaks up fpga-region.c into two files, moving the DT overlay
support to of-fpga-region.c.  The functions exported by fpga-region.c
will enable code that creates an FPGA region and tell it what manager
and bridge to use.  fpga-region.c doesn't do enumeration so whatever
code creates the region will also still have the responsibility to do
some enumeration after programming.

Patches 7-8 a sysfs interface to FPGA regions.  I'm sure this will be
controversial as discussions about FPGA userspace interfaces have
incited lively discussion in the past.  The nice thing about this
interface is that it handles the dance of disabling the bridge before
programming and reenabling it afterwards.  But no enumeration.
I post it as separate patch and document so the rest of the patches
could go forward while we hash out what a good non-DT interface
may look like if there is some other layer that is requesting
reprogramming and handling enumeration.

I've tested it lightly and believe that each patch separately
builds and works.

Known issues: doesn't work as a module anymore.  I'll be fixing that.

Alan

Alan Tull (8):
  fpga-mgr: add a single function for fpga loading methods
  fpga-region: support more than one overlay per FPGA region
  fpga-bridge: add non-dt support
  doc: fpga-mgr: separate getting/locking FPGA manager
  fpga-mgr: separate getting/locking FPGA manager
  fpga-region: separate out common code to allow non-dt support
  fpga-region: add sysfs interface
  doc: fpga: add sysfs document for fpga region

 Documentation/ABI/testing/sysfs-class-fpga-region |  26 +
 Documentation/fpga/fpga-mgr.txt                   |  19 +-
 drivers/fpga/Kconfig                              |  20 +-
 drivers/fpga/Makefile                             |   1 +
 drivers/fpga/fpga-bridge.c                        | 107 +++-
 drivers/fpga/fpga-mgr.c                           |  56 +-
 drivers/fpga/fpga-region.c                        | 725 ++++++++++------------
 drivers/fpga/fpga-region.h                        |  68 ++
 drivers/fpga/of-fpga-region.c                     | 510 +++++++++++++++
 include/linux/fpga/fpga-bridge.h                  |   7 +-
 include/linux/fpga/fpga-mgr.h                     |  17 +
 11 files changed, 1128 insertions(+), 428 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-region
 create mode 100644 drivers/fpga/fpga-region.h
 create mode 100644 drivers/fpga/of-fpga-region.c

-- 
2.7.4

[toc] | [next] | [standalone]

#1581428 — [RFC 6/8] fpga-region: separate out common code to allow non-dt support

FPGA region is a layer above the FPGA manager and FPGA bridge frameworks
that controls them both together.  The current implementation for FPGA
regions is dependent on device tree.  This commit separates that code into
common code and device tree specific code.  fpga-region.c remains as common
code and of-fpga-region.c is added to support device tree overlays.

Functons exported:

of_fpga_region_find
* Find a fpga region, given the node pointer

fpga_region_alloc_image_info
fpga_region_free_image_info
* Alloc/free fpga_image_info struct

fpga_region_program_fpga
* Program an FPGA region

* fpga_region_register
* fpga_region_unregister
  Create/remove a FPGA region.  Caller will supply the region
  struct initialized with a pointer to a FPGA manager and
  a method to get the FPGA bridges.

Signed-off-by: Alan Tull <atull@kernel.org>
---
 drivers/fpga/Kconfig          |  12 +-
 drivers/fpga/Makefile         |   1 +
 drivers/fpga/fpga-region.c    | 558 +++++++-----------------------------------
 drivers/fpga/fpga-region.h    |  68 +++++
 drivers/fpga/of-fpga-region.c | 510 ++++++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-mgr.h |   6 +-
 6 files changed, 678 insertions(+), 477 deletions(-)
 create mode 100644 drivers/fpga/fpga-region.h
 create mode 100644 drivers/fpga/of-fpga-region.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index ce861a2..be9c23d 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -15,10 +15,18 @@ if FPGA
 
 config FPGA_REGION
 	tristate "FPGA Region"
-	depends on OF && FPGA_BRIDGE
+	depends on FPGA_BRIDGE
+	help
+	  FPGA Region common code.  A FPGA Region controls a FPGA Manager
+	  and the FPGA Bridges associated with either a reconfigurable
+	  region of an FPGA or a whole FPGA.
+
+config OF_FPGA_REGION
+	tristate "FPGA Region Device Tree Overlay Support"
+	depends on FPGA_REGION
 	help
 	  FPGA Regions allow loading FPGA images under control of
-	  the Device Tree.
+	  Device Tree Overlays.
 
 config FPGA_MGR_SOCFPGA
 	tristate "Altera SOCFPGA FPGA Manager"
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 8df07bc..fb88fb0 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_ALTERA_FREEZE_BRIDGE)	+= altera-freeze-bridge.o
 
 # High Level Interfaces
 obj-$(CONFIG_FPGA_REGION)		+= fpga-region.o
+obj-$(CONFIG_OF_FPGA_REGION)		+= of-fpga-region.o
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index a5d8112..5690237 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -22,64 +22,58 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
-#include <linux/of_platform.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include "fpga-region.h"
 
-/**
- * struct fpga_region - FPGA Region structure
- * @dev: FPGA Region device
- * @mutex: enforces exclusive reference to region
- * @bridge_list: list of FPGA bridges specified in region
- * @overlays: list of struct region_overlay_info
- */
-struct fpga_region {
-	struct device dev;
-	struct mutex mutex; /* for exclusive reference to region */
-	struct list_head bridge_list;
-	struct list_head overlays;
-};
+static DEFINE_IDA(fpga_region_ida);
+struct class *fpga_region_class;
 
-/**
- * struct region_overlay: info regarding overlays applied to the region
- * @node: list node
- * @overlay: pointer to overlay
- * @image_info: fpga image specific information parsed from overlay.  Is NULL if
- *        overlay doesn't program FPGA.
- */
+/* todo: prevent programming parent region */
 
-struct region_overlay {
-	struct list_head node;
-	struct device_node *overlay;
+struct fpga_image_info *fpga_region_alloc_image_info(struct fpga_region *region)
+{
+	struct device *dev = &region->dev;
 	struct fpga_image_info *image_info;
-};
 
-/* Lock for list of overlays */
-spinlock_t overlay_list_lock;
+	image_info = devm_kzalloc(dev, sizeof(*image_info), GFP_KERNEL);
+	if (!image_info)
+		return ERR_PTR(-ENOMEM);
 
-#define to_fpga_region(d) container_of(d, struct fpga_region, dev)
+	return image_info;
+}
+EXPORT_SYMBOL_GPL(fpga_region_alloc_image_info);
 
-static DEFINE_IDA(fpga_region_ida);
-static struct class *fpga_region_class;
+void fpga_region_free_image_info(struct fpga_region *region,
+				 struct fpga_image_info *image_info)
+{
+	struct device *dev = &region->dev;
 
-static const struct of_device_id fpga_region_of_match[] = {
-	{ .compatible = "fpga-region", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, fpga_region_of_match);
+	if (!image_info)
+		return;
 
+	if (image_info->firmware_name)
+		devm_kfree(dev, image_info->firmware_name);
+
+	devm_kfree(dev, image_info);
+}
+EXPORT_SYMBOL_GPL(fpga_region_free_image_info);
+
+#if IS_ENABLED(CONFIG_OF_FPGA_REGION)
 static int fpga_region_of_node_match(struct device *dev, const void *data)
 {
 	return dev->of_node == data;
 }
 
 /**
- * fpga_region_find - find FPGA region
+ * of_fpga_region_find - find FPGA region
  * @np: device node of FPGA Region
+ *
  * Caller will need to put_device(&region->dev) when done.
+ *
  * Returns FPGA Region struct or NULL
  */
-static struct fpga_region *fpga_region_find(struct device_node *np)
+struct fpga_region *of_fpga_region_find(struct device_node *np)
 {
 	struct device *dev;
 
@@ -90,6 +84,26 @@ static struct fpga_region *fpga_region_find(struct device_node *np)
 
 	return to_fpga_region(dev);
 }
+EXPORT_SYMBOL_GPL(of_fpga_region_find);
+
+/*
+ * If a region has overlays, only the first overlay can program the FPGA
+ * so only the first overlay will have image info.
+ */
+struct fpga_image_info *fpga_region_ovl_image_info(struct fpga_region *region)
+{
+	struct region_overlay *reg_ovl;
+
+	if (list_empty(&region->overlays))
+		return NULL;
+
+	reg_ovl = list_first_entry(&region->overlays, typeof(*reg_ovl), node);
+
+	return reg_ovl->image_info;
+}
+EXPORT_SYMBOL_GPL(fpga_region_ovl_image_info);
+
+#endif /* CONFIG_OF_FPGA_REGION */
 
 /**
  * fpga_region_get - get an exclusive reference to a fpga region
@@ -111,9 +125,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)
 	}
 
 	get_device(dev);
-	of_node_get(dev->of_node);
 	if (!try_module_get(dev->parent->driver->owner)) {
-		of_node_put(dev->of_node);
 		put_device(dev);
 		mutex_unlock(&region->mutex);
 		return ERR_PTR(-ENODEV);
@@ -136,109 +148,11 @@ static void fpga_region_put(struct fpga_region *region)
 	dev_dbg(&region->dev, "put\n");
 
 	module_put(dev->parent->driver->owner);
-	of_node_put(dev->of_node);
 	put_device(dev);
 	mutex_unlock(&region->mutex);
 }
 
 /**
- * fpga_region_get_manager - get reference for FPGA manager
- * @region: FPGA region
- *
- * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
- *
- * Caller should call fpga_mgr_put() when done with manager.
- *
- * Return: fpga manager struct or IS_ERR() condition containing error code.
- */
-static struct fpga_manager *fpga_region_get_manager(struct fpga_region *region)
-{
-	struct device *dev = &region->dev;
-	struct device_node *np = dev->of_node;
-	struct device_node  *mgr_node;
-	struct fpga_manager *mgr;
-
-	of_node_get(np);
-	while (np) {
-		if (of_device_is_compatible(np, "fpga-region")) {
-			mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
-			if (mgr_node) {
-				mgr = of_fpga_mgr_get(mgr_node);
-				of_node_put(np);
-				return mgr;
-			}
-		}
-		np = of_get_next_parent(np);
-	}
-	of_node_put(np);
-
-	return ERR_PTR(-EINVAL);
-}
-
-/**
- * fpga_region_get_bridges - create a list of bridges
- * @region: FPGA region
- * @reg_ovl: overlay applied to the region
- *
- * Create a list of bridges including the parent bridge and the bridges
- * specified by "fpga-bridges" property.  Note that the
- * fpga_bridges_enable/disable/put functions are all fine with an empty list
- * if that happens.
- *
- * Caller should call fpga_bridges_put(&region->bridge_list) when
- * done with the bridges.
- *
- * Return 0 for success (even if there are no bridges specified)
- * or -EBUSY if any of the bridges are in use.
- */
-static int fpga_region_get_bridges(struct fpga_region *region,
-				   struct region_overlay *reg_ovl)
-{
-	struct device *dev = &region->dev;
-	struct device_node *region_np = dev->of_node;
-	struct device_node *br, *np, *parent_br = NULL;
-	int i, ret;
-
-	/* If parent is a bridge, add to list */
-	ret = of_fpga_bridge_get_to_list(region_np->parent,
-					 reg_ovl->image_info,
-					 &region->bridge_list);
-	if (ret == -EBUSY)
-		return ret;
-
-	if (!ret)
-		parent_br = region_np->parent;
-
-	/* If overlay has a list of bridges, use it. */
-	if (of_parse_phandle(reg_ovl->overlay, "fpga-bridges", 0))
-		np = reg_ovl->overlay;
-	else
-		np = region_np;
-
-	for (i = 0; ; i++) {
-		br = of_parse_phandle(np, "fpga-bridges", i);
-		if (!br)
-			break;
-
-		/* If parent bridge is in list, skip it. */
-		if (br == parent_br)
-			continue;
-
-		/* If node is a bridge, get it and add to list */
-		ret = of_fpga_bridge_get_to_list(br, reg_ovl->image_info,
-						 &region->bridge_list);
-
-		/* If any of the bridges are in use, give up */
-		if (ret == -EBUSY) {
-			fpga_bridges_put(&region->bridge_list);
-			return -EBUSY;
-		}
-	}
-
-	return 0;
-}
-
-/**
  * fpga_region_program_fpga - program FPGA
  * @region: FPGA region that is receiving an overlay
  * @reg_ovl: region overlay with fpga_image_info parsed from overlay
@@ -247,10 +161,9 @@ static int fpga_region_get_bridges(struct fpga_region *region,
  *
  * Return: 0 for success or negative error code.
  */
-static int fpga_region_program_fpga(struct fpga_region *region,
-				    struct region_overlay *reg_ovl)
+int fpga_region_program_fpga(struct fpga_region *region,
+			     struct fpga_image_info *image_info)
 {
-	struct fpga_manager *mgr;
 	int ret;
 
 	region = fpga_region_get(region);
@@ -259,22 +172,22 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		return PTR_ERR(region);
 	}
 
-	mgr = fpga_region_get_manager(region);
-	if (IS_ERR(mgr)) {
-		pr_err("failed to get fpga region manager\n");
-		return PTR_ERR(mgr);
-	}
-
-	ret = fpga_mgr_lock(mgr);
-	if (ret) {
-		pr_err("FPGA manager is busy\n");
-		goto err_put_mgr;
+	ret = fpga_mgr_lock(region->mgr);
+	if (ret < 0) {
+		pr_err("fpga manager is busy\n");
+		goto err_put_region;
 	}
 
-	ret = fpga_region_get_bridges(region, reg_ovl);
-	if (ret) {
-		pr_err("failed to get fpga region bridges\n");
-		goto err_unlock_mgr;
+	/*
+	 * In some cases, we already have a list of bridges in the
+	 * fpga region struct.  Or we don't have any bridges.
+	 */
+	if (region->get_bridges) {
+		ret = region->get_bridges(region, image_info);
+		if (ret) {
+			pr_err("failed to get fpga region bridges\n");
+			goto err_unlock_mgr;
+		}
 	}
 
 	ret = fpga_bridges_disable(&region->bridge_list);
@@ -283,7 +196,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_br;
 	}
 
-	ret = fpga_mgr_load(mgr, reg_ovl->image_info);
+	ret = fpga_mgr_load(region->mgr, image_info);
 	if (ret) {
 		pr_err("failed to load fpga image\n");
 		goto err_put_br;
@@ -295,309 +208,39 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_br;
 	}
 
-	fpga_mgr_unlock(mgr);
-	fpga_mgr_put(mgr);
+	region->image_info = image_info;
+
+	fpga_mgr_unlock(region->mgr);
 	fpga_region_put(region);
 
 	return 0;
 
 err_put_br:
-	fpga_bridges_put(&region->bridge_list);
+	if (region->get_bridges)
+		fpga_bridges_put(&region->bridge_list);
 err_unlock_mgr:
-	fpga_mgr_unlock(mgr);
-err_put_mgr:
-	fpga_mgr_put(mgr);
+	fpga_mgr_unlock(region->mgr);
+err_put_region:
 	fpga_region_put(region);
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(fpga_region_program_fpga);
 
-/**
- * child_regions_with_firmware
- * @overlay: device node of the overlay
- *
- * If the overlay adds child FPGA regions, they are not allowed to have
- * firmware-name property.
- *
- * Return 0 for OK or -EINVAL if child FPGA region adds firmware-name.
- */
-static int child_regions_with_firmware(struct device_node *overlay)
+int fpga_region_register(struct device *dev, struct fpga_region *region)
 {
-	struct device_node *child_region;
-	const char *child_firmware_name;
-	int ret = 0;
-
-	of_node_get(overlay);
-
-	child_region = of_find_matching_node(overlay, fpga_region_of_match);
-	while (child_region) {
-		if (!of_property_read_string(child_region, "firmware-name",
-					     &child_firmware_name)) {
-			ret = -EINVAL;
-			break;
-		}
-		child_region = of_find_matching_node(child_region,
-						     fpga_region_of_match);
-	}
-
-	of_node_put(child_region);
-
-	if (ret)
-		pr_err("firmware-name not allowed in child FPGA region: %s",
-		       child_region->full_name);
-
-	return ret;
-}
-
-/**
- * fpga_region_parse_ov - parse and check overlay applied to region
- *
- * @region: FPGA region
- * @overlay: overlay applied to the FPGA region
- *
- * Given an overlay applied to a FPGA region, parse the FPGA image specific
- * info in the overlay and do some checking.
- *
- * Returns:
- *   NULL if overlay doesn't direct us to program the FPGA.
- *   fpga_image_info struct if there is an image to program.
- *   error code for invalid overlay.
- */
-static struct fpga_image_info *fpga_region_parse_ov(struct fpga_region *region,
-						    struct device_node *overlay)
-{
-	struct device *dev = &region->dev;
-	struct fpga_image_info *info;
-	int ret;
-
-	/*
-	 * Reject overlay if child FPGA Regions added in the overlay have
-	 * firmware-name property (would mean that an FPGA region that has
-	 * not been added to the live tree yet is doing FPGA programming).
-	 */
-	ret = child_regions_with_firmware(overlay);
-	if (ret)
-		return ERR_PTR(ret);
-
-	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
-	if (!info)
-		return ERR_PTR(-ENOMEM);
-
-	if (of_property_read_bool(overlay, "partial-fpga-config"))
-		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
-
-	if (of_property_read_bool(overlay, "external-fpga-config"))
-		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
-
-	of_property_read_string(overlay, "firmware-name", &info->firmware_name);
-
-	of_property_read_u32(overlay, "region-unfreeze-timeout-us",
-			     &info->enable_timeout_us);
-
-	of_property_read_u32(overlay, "region-freeze-timeout-us",
-			     &info->disable_timeout_us);
-
-	/* If overlay is not programming the FPGA, don't need FPGA image info */
-	if (!info->firmware_name) {
-		devm_kfree(dev, info);
-		return NULL;
-	}
-
-	/*
-	 * If overlay informs us FPGA was externally programmed, specifying
-	 * firmware here would be ambiguous.
-	 */
-	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
-		dev_err(dev, "error: specified firmware and external-fpga-config");
-		devm_kfree(dev, info);
-		return ERR_PTR(-EINVAL);
-	}
-
-	/*
-	 * The first overlay to a region may reprogram the FPGA and specify how
-	 * to program the fpga (fpga_image_info).  Subsequent overlays can be
-	 * can add/modify child node properties if that is useful.
-	 */
-	if (!list_empty(&region->overlays)) {
-		dev_err(dev, "Only 1st DTO to a region may program a FPGA.\n");
-		devm_kfree(dev, info);
-		return ERR_PTR(-EINVAL);
-	}
-
-	return info;
-}
-
-/**
- * fpga_region_notify_pre_apply - pre-apply overlay notification
- *
- * @region: FPGA region that the overlay will be applied to
- * @nd: overlay notification data
- *
- * Called when an overlay targeted to a FPGA Region is about to be applied.
- * Parses the overlay for properties that influence how the FPGA will be
- * programmed and does some checking. If the checks pass, programs the FPGA.
- *
- * If the overlay that breaks the rules, notifier returns an error and the
- * overlay is rejected, preventing it from being added to the main tree.
- *
- * Return: 0 for success or negative error code for failure.
- */
-static int fpga_region_notify_pre_apply(struct fpga_region *region,
-					struct of_overlay_notify_data *nd)
-{
-	struct device *dev = &region->dev;
-	struct region_overlay *reg_ovl;
-	struct fpga_image_info *info;
-	unsigned long flags;
-	int ret;
-
-	info = fpga_region_parse_ov(region, nd->overlay);
-	if (IS_ERR(info))
-		return PTR_ERR(info);
-
-	reg_ovl = devm_kzalloc(dev, sizeof(*reg_ovl), GFP_KERNEL);
-	if (!reg_ovl)
-		return -ENOMEM;
-
-	reg_ovl->overlay = nd->overlay;
-	reg_ovl->image_info = info;
-
-	if (info) {
-		ret = fpga_region_program_fpga(region, reg_ovl);
-		if (ret)
-			goto pre_a_err;
-	}
-
-	spin_lock_irqsave(&overlay_list_lock, flags);
-	list_add_tail(&reg_ovl->node, &region->overlays);
-	spin_unlock_irqrestore(&overlay_list_lock, flags);
-
-	return 0;
-
-pre_a_err:
-	if (info)
-		devm_kfree(dev, info);
-	devm_kfree(dev, reg_ovl);
-	return ret;
-}
-
-/**
- * fpga_region_notify_post_remove - post-remove overlay notification
- *
- * @region: FPGA region that was targeted by the overlay that was removed
- * @nd: overlay notification data
- *
- * Called after an overlay has been removed if the overlay's target was a
- * FPGA region.
- */
-static void fpga_region_notify_post_remove(struct fpga_region *region,
-					   struct of_overlay_notify_data *nd)
-{
-	struct region_overlay *reg_ovl;
-	unsigned long flags;
-
-	reg_ovl = list_last_entry(&region->overlays, typeof(*reg_ovl), node);
-
-	fpga_bridges_disable(&region->bridge_list);
-	fpga_bridges_put(&region->bridge_list);
-
-	spin_lock_irqsave(&overlay_list_lock, flags);
-	list_del(&reg_ovl->node);
-	spin_unlock_irqrestore(&overlay_list_lock, flags);
-
-	if (reg_ovl->image_info)
-		devm_kfree(&region->dev, reg_ovl->image_info);
-
-	devm_kfree(&region->dev, reg_ovl);
-}
-
-/**
- * of_fpga_region_notify - reconfig notifier for dynamic DT changes
- * @nb:		notifier block
- * @action:	notifier action
- * @arg:	reconfig data
- *
- * This notifier handles programming a FPGA when a "firmware-name" property is
- * added to a fpga-region.
- *
- * Returns NOTIFY_OK or error if FPGA programming fails.
- */
-static int of_fpga_region_notify(struct notifier_block *nb,
-				 unsigned long action, void *arg)
-{
-	struct of_overlay_notify_data *nd = arg;
-	struct fpga_region *region;
-	int ret;
-
-	switch (action) {
-	case OF_OVERLAY_PRE_APPLY:
-		pr_debug("%s OF_OVERLAY_PRE_APPLY\n", __func__);
-		break;
-	case OF_OVERLAY_POST_APPLY:
-		pr_debug("%s OF_OVERLAY_POST_APPLY\n", __func__);
-		return NOTIFY_OK;       /* not for us */
-	case OF_OVERLAY_PRE_REMOVE:
-		pr_debug("%s OF_OVERLAY_PRE_REMOVE\n", __func__);
-		return NOTIFY_OK;       /* not for us */
-	case OF_OVERLAY_POST_REMOVE:
-		pr_debug("%s OF_OVERLAY_POST_REMOVE\n", __func__);
-		break;
-	default:			/* should not happen */
-		return NOTIFY_OK;
-	}
-
-	region = fpga_region_find(nd->target);
-	if (!region)
-		return NOTIFY_OK;
-
-	ret = 0;
-	switch (action) {
-	case OF_OVERLAY_PRE_APPLY:
-		ret = fpga_region_notify_pre_apply(region, nd);
-		break;
-
-	case OF_OVERLAY_POST_REMOVE:
-		fpga_region_notify_post_remove(region, nd);
-		break;
-	}
-
-	put_device(&region->dev);
-
-	if (ret)
-		return notifier_from_errno(ret);
-
-	return NOTIFY_OK;
-}
-
-static struct notifier_block fpga_region_of_nb = {
-	.notifier_call = of_fpga_region_notify,
-};
-
-static int fpga_region_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
-	struct fpga_region *region;
 	int id, ret = 0;
 
-	region = kzalloc(sizeof(*region), GFP_KERNEL);
-	if (!region)
-		return -ENOMEM;
-
 	id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL);
-	if (id < 0) {
-		ret = id;
-		goto err_kfree;
-	}
+	if (id < 0)
+		return id;
 
 	mutex_init(&region->mutex);
 	INIT_LIST_HEAD(&region->bridge_list);
-	INIT_LIST_HEAD(&region->overlays);
-
 	device_initialize(&region->dev);
 	region->dev.class = fpga_region_class;
 	region->dev.parent = dev;
-	region->dev.of_node = np;
+	region->dev.of_node = dev->of_node;
 	region->dev.id = id;
 	dev_set_drvdata(dev, region);
 
@@ -609,82 +252,49 @@ static int fpga_region_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_remove;
 
-	of_platform_populate(np, fpga_region_of_match, NULL, &region->dev);
-
 	dev_info(dev, "FPGA Region probed\n");
 
 	return 0;
 
 err_remove:
 	ida_simple_remove(&fpga_region_ida, id);
-err_kfree:
-	kfree(region);
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(fpga_region_register);
 
-static int fpga_region_remove(struct platform_device *pdev)
+int fpga_region_unregister(struct fpga_region *region)
 {
-	struct fpga_region *region = platform_get_drvdata(pdev);
-
 	device_unregister(&region->dev);
 
 	return 0;
 }
-
-static struct platform_driver fpga_region_driver = {
-	.probe = fpga_region_probe,
-	.remove = fpga_region_remove,
-	.driver = {
-		.name	= "fpga-region",
-		.of_match_table = of_match_ptr(fpga_region_of_match),
-	},
-};
+EXPORT_SYMBOL_GPL(fpga_region_unregister);
 
 static void fpga_region_dev_release(struct device *dev)
 {
 	struct fpga_region *region = to_fpga_region(dev);
 
 	ida_simple_remove(&fpga_region_ida, region->dev.id);
-	kfree(region);
 }
 
 /**
  * fpga_region_init - init function for fpga_region class
- * Creates the fpga_region class and registers a reconfig notifier.
+ * Creates the fpga_region class.
  */
 static int __init fpga_region_init(void)
 {
-	int ret;
-
 	fpga_region_class = class_create(THIS_MODULE, "fpga_region");
 	if (IS_ERR(fpga_region_class))
 		return PTR_ERR(fpga_region_class);
 
 	fpga_region_class->dev_release = fpga_region_dev_release;
 
-	ret = of_overlay_notifier_register(&fpga_region_of_nb);
-	if (ret)
-		goto err_class;
-
-	ret = platform_driver_register(&fpga_region_driver);
-	if (ret)
-		goto err_plat;
-
 	return 0;
-
-err_plat:
-	of_overlay_notifier_unregister(&fpga_region_of_nb);
-err_class:
-	class_destroy(fpga_region_class);
-	ida_destroy(&fpga_region_ida);
-	return ret;
 }
 
 static void __exit fpga_region_exit(void)
 {
-	platform_driver_unregister(&fpga_region_driver);
-	of_overlay_notifier_unregister(&fpga_region_of_nb);
 	class_destroy(fpga_region_class);
 	ida_destroy(&fpga_region_ida);
 }
diff --git a/drivers/fpga/fpga-region.h b/drivers/fpga/fpga-region.h
new file mode 100644
index 0000000..e041015
--- /dev/null
+++ b/drivers/fpga/fpga-region.h
@@ -0,0 +1,68 @@
+#include <linux/device.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/fpga/fpga-bridge.h>
+
+#ifndef _FPGA_REGION_H
+#define _FPGA_REGION_H
+
+/**
+ * struct fpga_region - FPGA Region structure
+ * @dev: FPGA Region device
+ * @mutex: enforces exclusive reference to region
+ * @bridge_list: list of FPGA bridges specified in region
+ * @overlays: list of struct region_overlay_info
+ * @mgr_dev: device of fpga manager
+ * @priv: private data
+ */
+struct fpga_region {
+	struct device dev;
+	struct mutex mutex; /* for exclusive reference to region */
+	struct list_head bridge_list;
+	struct fpga_manager *mgr;
+	struct fpga_image_info *image_info;
+	void *priv;
+	int (*get_bridges)(struct fpga_region *region,
+			   struct fpga_image_info *image_info);
+#if IS_ENABLED(CONFIG_OF_FPGA_REGION)
+	struct list_head overlays;
+#endif
+};
+
+#if IS_ENABLED(CONFIG_OF_FPGA_REGION)
+/**
+ * struct region_overlay: info regarding overlays applied to the region
+ * @node: list node
+ * @overlay: pointer to overlay
+ * @image_info: fpga image specific information parsed from overlay.  Is NULL if
+ *        overlay doesn't program FPGA.
+ */
+struct region_overlay {
+	struct list_head node;
+	struct device_node *overlay;
+	struct fpga_image_info *image_info;
+};
+#endif /* CONFIG_OF_FPGA_REGION */
+
+#define to_fpga_region(d) container_of(d, struct fpga_region, dev)
+
+#ifdef CONFIG_OF
+struct fpga_region *of_fpga_region_find(struct device_node *np);
+#else
+struct fpga_region *of_fpga_region_find(struct device_node *np)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF */
+
+struct fpga_image_info *fpga_region_alloc_image_info(
+				struct fpga_region *region);
+void fpga_region_free_image_info(struct fpga_region *region,
+				 struct fpga_image_info *image_info);
+
+int fpga_region_program_fpga(struct fpga_region *region,
+			     struct fpga_image_info *image_info);
+
+int fpga_region_register(struct device *dev, struct fpga_region *region);
+int fpga_region_unregister(struct fpga_region *region);
+
+#endif /* _FPGA_REGION_H */
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
new file mode 100644
index 0000000..af9de1e
--- /dev/null
+++ b/drivers/fpga/of-fpga-region.c
@@ -0,0 +1,510 @@
+/*
+ * FPGA Region - Device Tree support for FPGA programming under Linux
+ *
+ *  Copyright (C) 2013-2016 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/fpga/fpga-bridge.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include "fpga-region.h"
+
+/* Lock for list of overlays */
+spinlock_t overlay_list_lock;
+
+/**
+ * of_fpga_region_get_manager - get pointer for FPGA Manager
+ * @dev: FPGA region's device
+ *
+ * Get FPGA Manager from "fpga-mgr" property in region or in ancestor region.
+ *
+ * Caller should call fpga_mgr_put() when done with manager.
+ *
+ * Return: fpga manager struct or IS_ERR() condition containing error code.
+ */
+static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)
+{
+	struct device_node  *mgr_node;
+	struct fpga_manager *mgr;
+
+	of_node_get(np);
+	while (np) {
+		if (of_device_is_compatible(np, "fpga-region")) {
+			mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
+			if (mgr_node) {
+				mgr = of_fpga_mgr_get(mgr_node);
+				of_node_put(np);
+				return mgr;
+			}
+		}
+		np = of_get_next_parent(np);
+	}
+	of_node_put(np);
+
+	return ERR_PTR(-EINVAL);
+}
+
+/**
+ * of_fpga_region_get_bridges - create a list of bridges
+ * @region: FPGA region
+ * @image_info: FPGA image info
+ *
+ * Create a list of bridges including the parent bridge and the bridges
+ * specified by "fpga-bridges" property.  Note that the
+ * fpga_bridges_enable/disable/put functions are all fine with an empty list
+ * if that happens.
+ *
+ * Caller should call fpga_bridges_put(&region->bridge_list) when
+ * done with the bridges.
+ *
+ * Return 0 for success (even if there are no bridges specified)
+ * or -EBUSY if any of the bridges are in use.
+ */
+static int of_fpga_region_get_bridges(struct fpga_region *region,
+				      struct fpga_image_info *image_info)
+{
+	struct device *dev = &region->dev;
+	struct device_node *region_np = dev->of_node;
+	struct device_node *br, *np, *parent_br = NULL;
+	int i, ret;
+
+	/* If parent is a bridge, add to list */
+	ret = of_fpga_bridge_get_to_list(region_np->parent,
+					 image_info,
+					 &region->bridge_list);
+	if (ret == -EBUSY)
+		return ret;
+
+	if (!ret)
+		parent_br = region_np->parent;
+
+	/* If overlay has a list of bridges, use it. */
+	if (of_parse_phandle(image_info->overlay, "fpga-bridges", 0))
+		np = image_info->overlay;
+	else
+		np = region_np;
+
+	for (i = 0; ; i++) {
+		br = of_parse_phandle(np, "fpga-bridges", i);
+		if (!br)
+			break;
+
+		/* If parent bridge is in list, skip it. */
+		if (br == parent_br)
+			continue;
+
+		/* If node is a bridge, get it and add to list */
+		ret = of_fpga_bridge_get_to_list(br, image_info,
+						 &region->bridge_list);
+
+		/* If any of the bridges are in use, give up */
+		if (ret == -EBUSY) {
+			fpga_bridges_put(&region->bridge_list);
+			return -EBUSY;
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id fpga_region_of_match[] = {
+	{ .compatible = "fpga-region", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, fpga_region_of_match);
+
+/**
+ * child_regions_with_firmware
+ * @overlay: device node of the overlay
+ *
+ * If the overlay adds child FPGA regions, they are not allowed to have
+ * firmware-name property.
+ *
+ * Return 0 for OK or -EINVAL if child FPGA region adds firmware-name.
+ */
+static int child_regions_with_firmware(struct device_node *overlay)
+{
+	struct device_node *child_region;
+	const char *child_firmware_name;
+	int ret = 0;
+
+	of_node_get(overlay);
+
+	child_region = of_find_matching_node(overlay, fpga_region_of_match);
+	while (child_region) {
+		if (!of_property_read_string(child_region, "firmware-name",
+					     &child_firmware_name)) {
+			ret = -EINVAL;
+			break;
+		}
+		child_region = of_find_matching_node(child_region,
+						     fpga_region_of_match);
+	}
+
+	of_node_put(child_region);
+
+	if (ret)
+		pr_err("firmware-name not allowed in child FPGA region: %s",
+		       child_region->full_name);
+
+	return ret;
+}
+
+/**
+ * of_fpga_region_parse_ov - parse and check overlay applied to region
+ *
+ * @region: FPGA region
+ * @overlay: overlay applied to the FPGA region
+ *
+ * Given an overlay applied to a FPGA region, parse the FPGA image specific
+ * info in the overlay and do some checking.
+ *
+ * Returns:
+ *   NULL if overlay doesn't direct us to program the FPGA.
+ *   fpga_image_info struct if there is an image to program.
+ *   error code for invalid overlay.
+ */
+static struct fpga_image_info *of_fpga_region_parse_ov(
+						struct fpga_region *region,
+						struct device_node *overlay)
+{
+	struct device *dev = &region->dev;
+	struct fpga_image_info *info;
+	const char *firmware_name;
+	int ret;
+
+	/*
+	 * Reject overlay if child FPGA Regions added in the overlay have
+	 * firmware-name property (would mean that an FPGA region that has
+	 * not been added to the live tree yet is doing FPGA programming).
+	 */
+	ret = child_regions_with_firmware(overlay);
+	if (ret)
+		return ERR_PTR(ret);
+
+	info = fpga_region_alloc_image_info(region);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	info->overlay = overlay;
+
+	if (of_property_read_bool(overlay, "partial-fpga-config"))
+		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
+
+	if (of_property_read_bool(overlay, "external-fpga-config"))
+		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
+
+	if (!of_property_read_string(overlay, "firmware-name",
+				     &firmware_name)) {
+		info->firmware_name = devm_kzalloc(dev,
+						   strlen(firmware_name),
+						   GFP_KERNEL);
+		if (!info->firmware_name)
+			return ERR_PTR(-ENOMEM);
+
+		strcpy(info->firmware_name, firmware_name);
+	}
+
+	of_property_read_u32(overlay, "region-unfreeze-timeout-us",
+			     &info->enable_timeout_us);
+
+	of_property_read_u32(overlay, "region-freeze-timeout-us",
+			     &info->disable_timeout_us);
+
+	/* If overlay is not programming the FPGA, don't need FPGA image info */
+	if (!info->firmware_name) {
+		ret = 0;
+		goto ret_no_info;
+	}
+
+	/*
+	 * If overlay informs us FPGA was externally programmed, specifying
+	 * firmware here would be ambiguous.
+	 */
+	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
+		dev_err(dev, "error: specified firmware and external-fpga-config");
+		ret = -EINVAL;
+		goto ret_no_info;
+	}
+
+	/*
+	 * The first overlay to a region may reprogram the FPGA and specify how
+	 * to program the fpga (fpga_image_info).  Subsequent overlays can be
+	 * can add/modify child node properties if that is useful.
+	 */
+	if (!list_empty(&region->overlays)) {
+		dev_err(dev, "Only 1st DTO to a region may program a FPGA.\n");
+		ret = -EINVAL;
+		goto ret_no_info;
+	}
+
+	return info;
+ret_no_info:
+	fpga_region_free_image_info(region, info);
+	return ERR_PTR(ret);
+}
+
+static int of_fpga_region_list_add_ovl(struct fpga_region *region,
+				       struct device_node *overlay,
+				       struct fpga_image_info *info)
+{
+	unsigned long flags;
+	struct region_overlay *reg_ovl;
+
+	reg_ovl = devm_kzalloc(&region->dev, sizeof(*reg_ovl), GFP_KERNEL);
+	if (!reg_ovl)
+		return -ENOMEM;
+
+	reg_ovl->overlay = overlay;
+	reg_ovl->image_info = info;
+
+	spin_lock_irqsave(&overlay_list_lock, flags);
+	list_add_tail(&reg_ovl->node, &region->overlays);
+	spin_unlock_irqrestore(&overlay_list_lock, flags);
+
+	return 0;
+}
+
+static void of_fpga_region_list_rm_ovl(struct fpga_region *region,
+				       struct region_overlay *reg_ovl)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&overlay_list_lock, flags);
+	list_del(&reg_ovl->node);
+	spin_unlock_irqrestore(&overlay_list_lock, flags);
+
+	fpga_region_free_image_info(region, reg_ovl->image_info);
+	devm_kfree(&region->dev, reg_ovl);
+}
+
+/**
+ * of_fpga_region_notify_pre_apply - pre-apply overlay notification
+ *
+ * @region: FPGA region that the overlay will be applied to
+ * @nd: overlay notification data
+ *
+ * Called when an overlay targeted to a FPGA Region is about to be applied.
+ * Parses the overlay for properties that influence how the FPGA will be
+ * programmed and does some checking. If the checks pass, programs the FPGA.
+ *
+ * If the overlay that breaks the rules, notifier returns an error and the
+ * overlay is rejected, preventing it from being added to the main tree.
+ *
+ * Return: 0 for success or negative error code for failure.
+ */
+static int of_fpga_region_notify_pre_apply(struct fpga_region *region,
+					   struct of_overlay_notify_data *nd)
+{
+	struct fpga_image_info *info;
+	int ret;
+
+	info = of_fpga_region_parse_ov(region, nd->overlay);
+	if (IS_ERR(info))
+		return PTR_ERR(info);
+
+	if (info) {
+		ret = fpga_region_program_fpga(region, info);
+		if (ret)
+			goto pre_a_err;
+	}
+
+	ret = of_fpga_region_list_add_ovl(region, nd->overlay, info);
+	if (ret)
+		goto pre_a_err;
+
+	return 0;
+
+pre_a_err:
+	fpga_region_free_image_info(region, info);
+	return ret;
+}
+
+/**
+ * of_fpga_region_notify_post_remove - post-remove overlay notification
+ *
+ * @region: FPGA region that was targeted by the overlay that was removed
+ * @nd: overlay notification data
+ *
+ * Called after an overlay has been removed if the overlay's target was a
+ * FPGA region.
+ */
+static void of_fpga_region_notify_post_remove(struct fpga_region *region,
+					      struct of_overlay_notify_data *nd)
+{
+	struct region_overlay *reg_ovl;
+
+	reg_ovl = list_last_entry(&region->overlays, typeof(*reg_ovl), node);
+
+	fpga_bridges_disable(&region->bridge_list);
+	fpga_bridges_put(&region->bridge_list);
+
+	if (reg_ovl)
+		of_fpga_region_list_rm_ovl(region, reg_ovl);
+}
+
+/**
+ * of_fpga_region_notify - reconfig notifier for dynamic DT changes
+ * @nb:		notifier block
+ * @action:	notifier action
+ * @arg:	reconfig data
+ *
+ * This notifier handles programming a FPGA when a "firmware-name" property is
+ * added to a fpga-region.
+ *
+ * Returns NOTIFY_OK or error if FPGA programming fails.
+ */
+static int of_fpga_region_notify(struct notifier_block *nb,
+				 unsigned long action, void *arg)
+{
+	struct of_overlay_notify_data *nd = arg;
+	struct fpga_region *region;
+	int ret;
+
+	switch (action) {
+	case OF_OVERLAY_PRE_APPLY:
+		pr_debug("%s OF_OVERLAY_PRE_APPLY\n", __func__);
+		break;
+	case OF_OVERLAY_POST_APPLY:
+		pr_debug("%s OF_OVERLAY_POST_APPLY\n", __func__);
+		return NOTIFY_OK;       /* not for us */
+	case OF_OVERLAY_PRE_REMOVE:
+		pr_debug("%s OF_OVERLAY_PRE_REMOVE\n", __func__);
+		return NOTIFY_OK;       /* not for us */
+	case OF_OVERLAY_POST_REMOVE:
+		pr_debug("%s OF_OVERLAY_POST_REMOVE\n", __func__);
+		break;
+	default:			/* should not happen */
+		return NOTIFY_OK;
+	}
+
+	region = of_fpga_region_find(nd->target);
+	if (!region)
+		return NOTIFY_OK;
+
+	ret = 0;
+	switch (action) {
+	case OF_OVERLAY_PRE_APPLY:
+		ret = of_fpga_region_notify_pre_apply(region, nd);
+		break;
+
+	case OF_OVERLAY_POST_REMOVE:
+		of_fpga_region_notify_post_remove(region, nd);
+		break;
+	}
+
+	put_device(&region->dev);
+
+	if (ret)
+		return notifier_from_errno(ret);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block fpga_region_of_nb = {
+	.notifier_call = of_fpga_region_notify,
+};
+
+static int of_fpga_region_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct fpga_manager *mgr;
+	struct fpga_region *region;
+	int ret;
+
+	region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL);
+	if (!region)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&region->overlays);
+
+	/* Find the FPGA mgr specified by region or parent region. */
+	mgr = of_fpga_region_get_mgr(np);
+	if (IS_ERR(mgr)) {
+		ret = PTR_ERR(mgr);
+		goto err_kfree;
+	}
+	region->mgr = mgr;
+
+	/* Specify how to get bridges for this type of region. */
+	region->get_bridges = of_fpga_region_get_bridges;
+
+	ret = fpga_region_register(dev, region);
+	if (ret)
+		goto err_put_mgr;
+
+	of_platform_populate(np, fpga_region_of_match, NULL, &region->dev);
+	dev_info(dev, "FPGA Region probed\n");
+
+	return ret;
+
+err_put_mgr:
+	fpga_mgr_put(mgr);
+err_kfree:
+	devm_kfree(dev, region);
+
+	return ret;
+}
+
+static int of_fpga_region_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct fpga_region *region = platform_get_drvdata(pdev);
+
+	fpga_region_unregister(region);
+	devm_kfree(dev, region);
+
+	return 0;
+}
+
+static struct platform_driver fpga_region_driver = {
+	.probe = of_fpga_region_probe,
+	.remove = of_fpga_region_remove,
+	.driver = {
+		.name	= "fpga-region",
+		.of_match_table = of_match_ptr(fpga_region_of_match),
+	},
+};
+
+static int __init of_fpga_region_notifier_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&fpga_region_driver);
+	if (ret)
+		return ret;
+
+	return of_overlay_notifier_register(&fpga_region_of_nb);
+}
+
+static void __exit of_fpga_region_notifier_exit(void)
+{
+	of_overlay_notifier_unregister(&fpga_region_of_nb);
+	platform_driver_unregister(&fpga_region_driver);
+}
+
+device_initcall(of_fpga_region_notifier_init);
+module_exit(of_fpga_region_notifier_exit);
+
+MODULE_DESCRIPTION("OF FPGA Region");
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index ae970ca..0f5072c 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -80,15 +80,19 @@ enum fpga_mgr_states {
  * @sgt: scatter/gather table containing FPGA image
  * @buf: contiguous buffer containing FPGA image
  * @count: size of buf
+ * @overlay: Device Tree overlay
  */
 struct fpga_image_info {
 	u32 flags;
 	u32 enable_timeout_us;
 	u32 disable_timeout_us;
-	const char *firmware_name;
+	char *firmware_name;
 	struct sg_table *sgt;
 	const char *buf;
 	size_t count;
+#ifdef CONFIG_OF
+	struct device_node *overlay;
+#endif
 };
 
 /**
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1581429 — [RFC 1/8] fpga-mgr: add a single function for fpga loading methods

Currently fpga-mgr.c has three methods for loading FPGA's depending on how
the FPGA image is presented: in a sg table, as a single buffer, or as a
firmware file.  This commit adds these parameters to the fpga_image_info
stuct and adds a single function that can accept the image as any of these
three.  This allows code to be written that could use any of the three
methods.

Signed-off-by: Alan Tull <atull@kernel.org>
---
 drivers/fpga/fpga-mgr.c       | 12 ++++++++++++
 drivers/fpga/fpga-region.c    | 17 +++++++----------
 include/linux/fpga/fpga-mgr.h | 10 ++++++++++
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 86d2cb2..b7c719a 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -309,6 +309,18 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
 
+int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
+{
+	if (info->firmware_name)
+		return fpga_mgr_firmware_load(mgr, info, info->firmware_name);
+	if (info->sgt)
+		return fpga_mgr_buf_load_sg(mgr, info, info->sgt);
+	if (info->buf && info->count)
+		return fpga_mgr_buf_load(mgr, info, info->buf, info->count);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_load);
+
 static const char * const state_str[] = {
 	[FPGA_MGR_STATE_UNKNOWN] =		"unknown",
 	[FPGA_MGR_STATE_POWER_OFF] =		"power off",
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 3222fdb..24f4ed5 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -223,14 +223,11 @@ static int fpga_region_get_bridges(struct fpga_region *region,
 /**
  * fpga_region_program_fpga - program FPGA
  * @region: FPGA region
- * @firmware_name: name of FPGA image firmware file
  * @overlay: device node of the overlay
- * Program an FPGA using information in the device tree.
- * Function assumes that there is a firmware-name property.
+ * Program an FPGA using information in the region's fpga image info.
  * Return 0 for success or negative error code.
  */
 static int fpga_region_program_fpga(struct fpga_region *region,
-				    const char *firmware_name,
 				    struct device_node *overlay)
 {
 	struct fpga_manager *mgr;
@@ -260,7 +257,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_br;
 	}
 
-	ret = fpga_mgr_firmware_load(mgr, region->info, firmware_name);
+	ret = fpga_mgr_load(mgr, region->info);
 	if (ret) {
 		pr_err("failed to load fpga image\n");
 		goto err_put_br;
@@ -351,7 +348,6 @@ static int child_regions_with_firmware(struct device_node *overlay)
 static int fpga_region_notify_pre_apply(struct fpga_region *region,
 					struct of_overlay_notify_data *nd)
 {
-	const char *firmware_name = NULL;
 	struct fpga_image_info *info;
 	int ret;
 
@@ -373,7 +369,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 	if (of_property_read_bool(nd->overlay, "external-fpga-config"))
 		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
 
-	of_property_read_string(nd->overlay, "firmware-name", &firmware_name);
+	of_property_read_string(nd->overlay, "firmware-name",
+				&info->firmware_name);
 
 	of_property_read_u32(nd->overlay, "region-unfreeze-timeout-us",
 			     &info->enable_timeout_us);
@@ -382,7 +379,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 			     &info->disable_timeout_us);
 
 	/* If FPGA was externally programmed, don't specify firmware */
-	if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && firmware_name) {
+	if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && info->firmware_name) {
 		pr_err("error: specified firmware and external-fpga-config");
 		return -EINVAL;
 	}
@@ -392,12 +389,12 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 		return 0;
 
 	/* If we got this far, we should be programming the FPGA */
-	if (!firmware_name) {
+	if (!info->firmware_name) {
 		pr_err("should specify firmware-name or external-fpga-config\n");
 		return -EINVAL;
 	}
 
-	return fpga_region_program_fpga(region, firmware_name, nd->overlay);
+	return fpga_region_program_fpga(region, nd->overlay);
 }
 
 /**
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 57beb5d..45df05a 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -76,11 +76,19 @@ enum fpga_mgr_states {
  * @flags: boolean flags as defined above
  * @enable_timeout_us: maximum time to enable traffic through bridge (uSec)
  * @disable_timeout_us: maximum time to disable traffic through bridge (uSec)
+ * @firmware_name: name of FPGA image firmware file
+ * @sgt: scatter/gather table containing FPGA image
+ * @buf: contiguous buffer containing FPGA image
+ * @count: size of buf
  */
 struct fpga_image_info {
 	u32 flags;
 	u32 enable_timeout_us;
 	u32 disable_timeout_us;
+	const char *firmware_name;
+	struct sg_table *sgt;
+	const char *buf;
+	size_t count;
 };
 
 /**
@@ -139,6 +147,8 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 			   struct fpga_image_info *info,
 			   const char *image_name);
 
+int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
+
 struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
 
 struct fpga_manager *fpga_mgr_get(struct device *dev);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1582162 — Re: [RFC 1/8] fpga-mgr: add a single function for fpga loading methods

Hi Alan,


On Wed, 15 Feb 2017, Alan Tull wrote:

> Currently fpga-mgr.c has three methods for loading FPGA's depending on how
> the FPGA image is presented: in a sg table, as a single buffer, or as a
> firmware file.  This commit adds these parameters to the fpga_image_info
> stuct and adds a single function that can accept the image as any of these
> three.  This allows code to be written that could use any of the three
> methods.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
> drivers/fpga/fpga-mgr.c       | 12 ++++++++++++
> drivers/fpga/fpga-region.c    | 17 +++++++----------
> include/linux/fpga/fpga-mgr.h | 10 ++++++++++
> 3 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 86d2cb2..b7c719a 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -309,6 +309,18 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
> }
> EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
>
> +int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
> +{
> +	if (info->firmware_name)
> +		return fpga_mgr_firmware_load(mgr, info, info->firmware_name);
> +	if (info->sgt)
> +		return fpga_mgr_buf_load_sg(mgr, info, info->sgt);
> +	if (info->buf && info->count)
> +		return fpga_mgr_buf_load(mgr, info, info->buf, info->count);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_load);
> +

I like this cleaner api.

> static const char * const state_str[] = {
> 	[FPGA_MGR_STATE_UNKNOWN] =		"unknown",
> 	[FPGA_MGR_STATE_POWER_OFF] =		"power off",
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index 3222fdb..24f4ed5 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -223,14 +223,11 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> /**
>  * fpga_region_program_fpga - program FPGA
>  * @region: FPGA region
> - * @firmware_name: name of FPGA image firmware file
>  * @overlay: device node of the overlay
> - * Program an FPGA using information in the device tree.
> - * Function assumes that there is a firmware-name property.
> + * Program an FPGA using information in the region's fpga image info.
>  * Return 0 for success or negative error code.
>  */
> static int fpga_region_program_fpga(struct fpga_region *region,
> -				    const char *firmware_name,
> 				    struct device_node *overlay)
> {
> 	struct fpga_manager *mgr;
> @@ -260,7 +257,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
> 		goto err_put_br;
> 	}
>
> -	ret = fpga_mgr_firmware_load(mgr, region->info, firmware_name);
> +	ret = fpga_mgr_load(mgr, region->info);
> 	if (ret) {
> 		pr_err("failed to load fpga image\n");
> 		goto err_put_br;
> @@ -351,7 +348,6 @@ static int child_regions_with_firmware(struct device_node *overlay)
> static int fpga_region_notify_pre_apply(struct fpga_region *region,
> 					struct of_overlay_notify_data *nd)
> {
> -	const char *firmware_name = NULL;
> 	struct fpga_image_info *info;
> 	int ret;
>
> @@ -373,7 +369,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
> 	if (of_property_read_bool(nd->overlay, "external-fpga-config"))
> 		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
>
> -	of_property_read_string(nd->overlay, "firmware-name", &firmware_name);
> +	of_property_read_string(nd->overlay, "firmware-name",
> +				&info->firmware_name);
>
> 	of_property_read_u32(nd->overlay, "region-unfreeze-timeout-us",
> 			     &info->enable_timeout_us);
> @@ -382,7 +379,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
> 			     &info->disable_timeout_us);
>
> 	/* If FPGA was externally programmed, don't specify firmware */
> -	if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && firmware_name) {
> +	if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && info->firmware_name) {
> 		pr_err("error: specified firmware and external-fpga-config");
> 		return -EINVAL;
> 	}
> @@ -392,12 +389,12 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
> 		return 0;
>
> 	/* If we got this far, we should be programming the FPGA */
> -	if (!firmware_name) {
> +	if (!info->firmware_name) {
> 		pr_err("should specify firmware-name or external-fpga-config\n");
> 		return -EINVAL;
> 	}
>
> -	return fpga_region_program_fpga(region, firmware_name, nd->overlay);
> +	return fpga_region_program_fpga(region, nd->overlay);
> }
>
> /**
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 57beb5d..45df05a 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -76,11 +76,19 @@ enum fpga_mgr_states {
>  * @flags: boolean flags as defined above
>  * @enable_timeout_us: maximum time to enable traffic through bridge (uSec)
>  * @disable_timeout_us: maximum time to disable traffic through bridge (uSec)
> + * @firmware_name: name of FPGA image firmware file
> + * @sgt: scatter/gather table containing FPGA image
> + * @buf: contiguous buffer containing FPGA image
> + * @count: size of buf
>  */
> struct fpga_image_info {
> 	u32 flags;
> 	u32 enable_timeout_us;
> 	u32 disable_timeout_us;

We really need to address your patch adds the config_complete_timout_us.
It seems like it would conflict with this patch.


> +	const char *firmware_name;
> +	struct sg_table *sgt;
> +	const char *buf;
> +	size_t count;
> };
>
> /**
> @@ -139,6 +147,8 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
> 			   struct fpga_image_info *info,
> 			   const char *image_name);
>
> +int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
> +
> struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>
> struct fpga_manager *fpga_mgr_get(struct device *dev);
> -- 
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1581430 — [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager

Document that getting a reference to a FPGA Manager has been
separated from locking the FPGA Mangager for use.

fpga_mgr_lock/unlock functions get/release mutex.

of_fpga_mgr_get, fpga_mgr_get, and fpga_mgr_put no longer lock
the FPGA manager mutex.

This makes it more straigtforward to save a reference to
a FPGA manager and only attempting to lock it when programming
the FPGA.

Signed-off-by: Alan Tull <atull@kernel.org>
---
 Documentation/fpga/fpga-mgr.txt | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
index 78f197f..06d5d5b 100644
--- a/Documentation/fpga/fpga-mgr.txt
+++ b/Documentation/fpga/fpga-mgr.txt
@@ -53,13 +53,26 @@ To get/put a reference to a FPGA manager:
 	struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
 	struct fpga_manager *fpga_mgr_get(struct device *dev);
 
-Given a DT node or device, get an exclusive reference to a FPGA manager.
+Given a DT node or device, get an reference to a FPGA manager.  Pointer
+can be saved until you are ready to program the FPGA.
 
 	void fpga_mgr_put(struct fpga_manager *mgr);
 
 Release the reference.
 
 
+To get exclusive control of a FPGA manager:
+-------------------------------------------
+
+	int fpga_mgr_lock(struct fpga_magager *mgr);
+
+Call fpga_mgr_lock and verify that it returns 0 before attempting to
+program the FPGA.
+
+	void fpga_mgr_unlock(struct fpga_magager *mgr);
+
+Call fpga_mgr_unlock when done programming the FPGA.
+
 To register or unregister the low level FPGA-specific driver:
 -------------------------------------------------------------
 
@@ -95,11 +108,13 @@ int ret;
 
 /* Get exclusive control of FPGA manager */
 struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
+ret = fpga_mgr_lock(mgr);
 
 /* Load the buffer to the FPGA */
 ret = fpga_mgr_buf_load(mgr, &info, buf, count);
 
 /* Release the FPGA manager */
+fpga_mgr_unlock(mgr);
 fpga_mgr_put(mgr);
 
 
@@ -124,11 +139,13 @@ int ret;
 
 /* Get exclusive control of FPGA manager */
 struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
+ret = fpga_mgr_lock(mgr);
 
 /* Get the firmware image (path) and load it to the FPGA */
 ret = fpga_mgr_firmware_load(mgr, &info, path);
 
 /* Release the FPGA manager */
+fpga_mgr_unlock(mgr);
 fpga_mgr_put(mgr);
 
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1583610 — Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager

hi Alan


On 2/15/2017 10:14 AM, Alan Tull wrote:
> Document that getting a reference to a FPGA Manager has been
> separated from locking the FPGA Mangager for use.
>
> fpga_mgr_lock/unlock functions get/release mutex.
>
> of_fpga_mgr_get, fpga_mgr_get, and fpga_mgr_put no longer lock
> the FPGA manager mutex.
>
> This makes it more straigtforward to save a reference to
> a FPGA manager and only attempting to lock it when programming
> the FPGA.

New to the FPGA world, but I like the idea of shorter lock. Separating 
the lock from fpga_mgr_get will give underline FPGA device drivers more 
flexibility to acquire the mgr pointer.
One newbie question, since the underline FPGA device driver does the 
fpga_mgr_register during probe, each manager instance belongs to that 
FPGA device only. What's the use to keep tracking the usage reference 
with fpga_mgr_put/get function, or is it enough to increase/decrease dev 
reference count in fpga_mgr_register/unregister function?

Thanks,
Yi

>
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
>   Documentation/fpga/fpga-mgr.txt | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
> index 78f197f..06d5d5b 100644
> --- a/Documentation/fpga/fpga-mgr.txt
> +++ b/Documentation/fpga/fpga-mgr.txt
> @@ -53,13 +53,26 @@ To get/put a reference to a FPGA manager:
>   	struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>   	struct fpga_manager *fpga_mgr_get(struct device *dev);
>   
> -Given a DT node or device, get an exclusive reference to a FPGA manager.
> +Given a DT node or device, get an reference to a FPGA manager.  Pointer
> +can be saved until you are ready to program the FPGA.
>   
>   	void fpga_mgr_put(struct fpga_manager *mgr);
>   
>   Release the reference.
>   
>   
> +To get exclusive control of a FPGA manager:
> +-------------------------------------------
> +
> +	int fpga_mgr_lock(struct fpga_magager *mgr);
> +
> +Call fpga_mgr_lock and verify that it returns 0 before attempting to
> +program the FPGA.
> +
> +	void fpga_mgr_unlock(struct fpga_magager *mgr);
> +
> +Call fpga_mgr_unlock when done programming the FPGA.
> +
>   To register or unregister the low level FPGA-specific driver:
>   -------------------------------------------------------------
>   
> @@ -95,11 +108,13 @@ int ret;
>   
>   /* Get exclusive control of FPGA manager */
>   struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
> +ret = fpga_mgr_lock(mgr);
>   
>   /* Load the buffer to the FPGA */
>   ret = fpga_mgr_buf_load(mgr, &info, buf, count);
>   
>   /* Release the FPGA manager */
> +fpga_mgr_unlock(mgr);
>   fpga_mgr_put(mgr);
>   
>   
> @@ -124,11 +139,13 @@ int ret;
>   
>   /* Get exclusive control of FPGA manager */
>   struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
> +ret = fpga_mgr_lock(mgr);
>   
>   /* Get the firmware image (path) and load it to the FPGA */
>   ret = fpga_mgr_firmware_load(mgr, &info, path);
>   
>   /* Release the FPGA manager */
> +fpga_mgr_unlock(mgr);
>   fpga_mgr_put(mgr);
>   
>   

[toc] | [prev] | [next] | [standalone]

#1583765 — Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager

On Fri, Feb 17, 2017 at 11:14 AM, Li, Yi <yi1.li@linux.intel.com> wrote:
> hi Alan
>
>
> On 2/15/2017 10:14 AM, Alan Tull wrote:
>>
>> Document that getting a reference to a FPGA Manager has been
>> separated from locking the FPGA Mangager for use.
>>
>> fpga_mgr_lock/unlock functions get/release mutex.
>>
>> of_fpga_mgr_get, fpga_mgr_get, and fpga_mgr_put no longer lock
>> the FPGA manager mutex.
>>
>> This makes it more straigtforward to save a reference to
>> a FPGA manager and only attempting to lock it when programming
>> the FPGA.
>
>
> New to the FPGA world, but I like the idea of shorter lock. Separating the
> lock from fpga_mgr_get will give underline FPGA device drivers more
> flexibility to acquire the mgr pointer.
> One newbie question, since the underline FPGA device driver does the
> fpga_mgr_register during probe, each manager instance belongs to that FPGA
> device only. What's the use to keep tracking the usage reference with
> fpga_mgr_put/get function, or is it enough to increase/decrease dev
> reference count in fpga_mgr_register/unregister function?

Hi Yi,

That's a good though but the code that creates the fpga mgr device might not be
the code that needs to hold a reference to it.  The device tree
implementation is an
example of this.  The fpga mgr devices are created completely
separately from the
fpga regions.

Alan

>
> Thanks,
> Yi
>
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
>> ---
>>   Documentation/fpga/fpga-mgr.txt | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/fpga/fpga-mgr.txt
>> b/Documentation/fpga/fpga-mgr.txt
>> index 78f197f..06d5d5b 100644
>> --- a/Documentation/fpga/fpga-mgr.txt
>> +++ b/Documentation/fpga/fpga-mgr.txt
>> @@ -53,13 +53,26 @@ To get/put a reference to a FPGA manager:
>>         struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>>         struct fpga_manager *fpga_mgr_get(struct device *dev);
>>   -Given a DT node or device, get an exclusive reference to a FPGA
>> manager.
>> +Given a DT node or device, get an reference to a FPGA manager.  Pointer
>> +can be saved until you are ready to program the FPGA.
>>         void fpga_mgr_put(struct fpga_manager *mgr);
>>     Release the reference.
>>     +To get exclusive control of a FPGA manager:
>> +-------------------------------------------
>> +
>> +       int fpga_mgr_lock(struct fpga_magager *mgr);
>> +
>> +Call fpga_mgr_lock and verify that it returns 0 before attempting to
>> +program the FPGA.
>> +
>> +       void fpga_mgr_unlock(struct fpga_magager *mgr);
>> +
>> +Call fpga_mgr_unlock when done programming the FPGA.
>> +
>>   To register or unregister the low level FPGA-specific driver:
>>   -------------------------------------------------------------
>>   @@ -95,11 +108,13 @@ int ret;
>>     /* Get exclusive control of FPGA manager */
>>   struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
>> +ret = fpga_mgr_lock(mgr);
>>     /* Load the buffer to the FPGA */
>>   ret = fpga_mgr_buf_load(mgr, &info, buf, count);
>>     /* Release the FPGA manager */
>> +fpga_mgr_unlock(mgr);
>>   fpga_mgr_put(mgr);
>>     @@ -124,11 +139,13 @@ int ret;
>>     /* Get exclusive control of FPGA manager */
>>   struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
>> +ret = fpga_mgr_lock(mgr);
>>     /* Get the firmware image (path) and load it to the FPGA */
>>   ret = fpga_mgr_firmware_load(mgr, &info, path);
>>     /* Release the FPGA manager */
>> +fpga_mgr_unlock(mgr);
>>   fpga_mgr_put(mgr);
>>
>
>

[toc] | [prev] | [next] | [standalone]

#1583626 — Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager

Alan,

small nits inline

On Wed, Feb 15, 2017 at 8:14 AM, Alan Tull <atull@kernel.org> wrote:
> Document that getting a reference to a FPGA Manager has been
> separated from locking the FPGA Mangager for use.
>
> fpga_mgr_lock/unlock functions get/release mutex.
>
> of_fpga_mgr_get, fpga_mgr_get, and fpga_mgr_put no longer lock
> the FPGA manager mutex.
>
> This makes it more straigtforward to save a reference to
> a FPGA manager and only attempting to lock it when programming
> the FPGA.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
Acked-by: Moritz Fischer <mdf@kernel.org>

> ---
>  Documentation/fpga/fpga-mgr.txt | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
> index 78f197f..06d5d5b 100644
> --- a/Documentation/fpga/fpga-mgr.txt
> +++ b/Documentation/fpga/fpga-mgr.txt
> @@ -53,13 +53,26 @@ To get/put a reference to a FPGA manager:
>         struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>         struct fpga_manager *fpga_mgr_get(struct device *dev);
>
> -Given a DT node or device, get an exclusive reference to a FPGA manager.
> +Given a DT node or device, get an reference to a FPGA manager.  Pointer

Nits: get *a* reference, 'A' pointer or 'The' pointer.

> +can be saved until you are ready to program the FPGA.
>
>         void fpga_mgr_put(struct fpga_manager *mgr);
>
>  Release the reference.
>
>
> +To get exclusive control of a FPGA manager:
> +-------------------------------------------
> +
> +       int fpga_mgr_lock(struct fpga_magager *mgr);
> +
> +Call fpga_mgr_lock and verify that it returns 0 before attempting to
> +program the FPGA.
> +
> +       void fpga_mgr_unlock(struct fpga_magager *mgr);
> +
> +Call fpga_mgr_unlock when done programming the FPGA.
> +
>  To register or unregister the low level FPGA-specific driver:
>  -------------------------------------------------------------
>
> @@ -95,11 +108,13 @@ int ret;
>
>  /* Get exclusive control of FPGA manager */
>  struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
> +ret = fpga_mgr_lock(mgr);
>
>  /* Load the buffer to the FPGA */
>  ret = fpga_mgr_buf_load(mgr, &info, buf, count);
>
>  /* Release the FPGA manager */
> +fpga_mgr_unlock(mgr);
>  fpga_mgr_put(mgr);
>
>
> @@ -124,11 +139,13 @@ int ret;
>
>  /* Get exclusive control of FPGA manager */
>  struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
> +ret = fpga_mgr_lock(mgr);
>
>  /* Get the firmware image (path) and load it to the FPGA */
>  ret = fpga_mgr_firmware_load(mgr, &info, path);
>
>  /* Release the FPGA manager */
> +fpga_mgr_unlock(mgr);
>  fpga_mgr_put(mgr);
>
>
> --
> 2.7.4
>

Cheers,

Moritz

[toc] | [prev] | [next] | [standalone]

#1583775 — Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager

On Fri, Feb 17, 2017 at 11:52 AM, Moritz Fischer <mdf@kernel.org> wrote:
> Alan,
>
> small nits inline

Hi Moritz,

Thanks for the review!

Alan

>
> On Wed, Feb 15, 2017 at 8:14 AM, Alan Tull <atull@kernel.org> wrote:
>> Document that getting a reference to a FPGA Manager has been
>> separated from locking the FPGA Mangager for use.
>>
>> fpga_mgr_lock/unlock functions get/release mutex.
>>
>> of_fpga_mgr_get, fpga_mgr_get, and fpga_mgr_put no longer lock
>> the FPGA manager mutex.
>>
>> This makes it more straigtforward to save a reference to
>> a FPGA manager and only attempting to lock it when programming
>> the FPGA.
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
> Acked-by: Moritz Fischer <mdf@kernel.org>
>
>> ---
>>  Documentation/fpga/fpga-mgr.txt | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
>> index 78f197f..06d5d5b 100644
>> --- a/Documentation/fpga/fpga-mgr.txt
>> +++ b/Documentation/fpga/fpga-mgr.txt
>> @@ -53,13 +53,26 @@ To get/put a reference to a FPGA manager:
>>         struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>>         struct fpga_manager *fpga_mgr_get(struct device *dev);
>>
>> -Given a DT node or device, get an exclusive reference to a FPGA manager.
>> +Given a DT node or device, get an reference to a FPGA manager.  Pointer
>
> Nits: get *a* reference, 'A' pointer or 'The' pointer.
>
>> +can be saved until you are ready to program the FPGA.
>>
>>         void fpga_mgr_put(struct fpga_manager *mgr);
>>
>>  Release the reference.
>>
>>
>> +To get exclusive control of a FPGA manager:
>> +-------------------------------------------
>> +
>> +       int fpga_mgr_lock(struct fpga_magager *mgr);
>> +
>> +Call fpga_mgr_lock and verify that it returns 0 before attempting to
>> +program the FPGA.
>> +
>> +       void fpga_mgr_unlock(struct fpga_magager *mgr);
>> +
>> +Call fpga_mgr_unlock when done programming the FPGA.
>> +
>>  To register or unregister the low level FPGA-specific driver:
>>  -------------------------------------------------------------
>>
>> @@ -95,11 +108,13 @@ int ret;
>>
>>  /* Get exclusive control of FPGA manager */
>>  struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
>> +ret = fpga_mgr_lock(mgr);
>>
>>  /* Load the buffer to the FPGA */
>>  ret = fpga_mgr_buf_load(mgr, &info, buf, count);
>>
>>  /* Release the FPGA manager */
>> +fpga_mgr_unlock(mgr);
>>  fpga_mgr_put(mgr);
>>
>>
>> @@ -124,11 +139,13 @@ int ret;
>>
>>  /* Get exclusive control of FPGA manager */
>>  struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
>> +ret = fpga_mgr_lock(mgr);
>>
>>  /* Get the firmware image (path) and load it to the FPGA */
>>  ret = fpga_mgr_firmware_load(mgr, &info, path);
>>
>>  /* Release the FPGA manager */
>> +fpga_mgr_unlock(mgr);
>>  fpga_mgr_put(mgr);
>>
>>
>> --
>> 2.7.4
>>
>
> Cheers,
>
> Moritz

[toc] | [prev] | [next] | [standalone]

#1581431 — [RFC 2/8] fpga-region: support more than one overlay per FPGA region

Currently if a user applies > 1 overlays to a region
and removes them, we get a slow warning from devm_kfree.
because the pointer to the FPGA image info was overwritten.

This commit adds a list to keep track of overlays applied
to each FPGA region.  Some rules are enforced:

 * Only allow the first overlay to a region to do FPGA
   programming.
 * Allow subsequent overlays to modify properties but
   not properties regarding FPGA programming.
 * To reprogram a region, first remove all previous
   overlays to the region.

Signed-off-by: Alan Tull <atull@kernel.org>
---
 drivers/fpga/fpga-region.c | 222 +++++++++++++++++++++++++++++++--------------
 1 file changed, 155 insertions(+), 67 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 24f4ed5..60d2947 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -31,15 +31,32 @@
  * @dev: FPGA Region device
  * @mutex: enforces exclusive reference to region
  * @bridge_list: list of FPGA bridges specified in region
- * @info: fpga image specific information
+ * @overlays: list of struct region_overlay_info
  */
 struct fpga_region {
 	struct device dev;
 	struct mutex mutex; /* for exclusive reference to region */
 	struct list_head bridge_list;
-	struct fpga_image_info *info;
+	struct list_head overlays;
 };
 
+/**
+ * struct region_overlay: info regarding overlays applied to the region
+ * @node: list node
+ * @overlay: pointer to overlay
+ * @image_info: fpga image specific information parsed from overlay.  Is NULL if
+ *        overlay doesn't program FPGA.
+ */
+
+struct region_overlay {
+	struct list_head node;
+	struct device_node *overlay;
+	struct fpga_image_info *image_info;
+};
+
+/* Lock for list of overlays */
+spinlock_t overlay_list_lock;
+
 #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
 
 static DEFINE_IDA(fpga_region_ida);
@@ -161,7 +178,7 @@ static struct fpga_manager *fpga_region_get_manager(struct fpga_region *region)
 /**
  * fpga_region_get_bridges - create a list of bridges
  * @region: FPGA region
- * @overlay: device node of the overlay
+ * @reg_ovl: overlay applied to the region
  *
  * Create a list of bridges including the parent bridge and the bridges
  * specified by "fpga-bridges" property.  Note that the
@@ -175,7 +192,7 @@ static struct fpga_manager *fpga_region_get_manager(struct fpga_region *region)
  * or -EBUSY if any of the bridges are in use.
  */
 static int fpga_region_get_bridges(struct fpga_region *region,
-				   struct device_node *overlay)
+				   struct region_overlay *reg_ovl)
 {
 	struct device *dev = &region->dev;
 	struct device_node *region_np = dev->of_node;
@@ -183,7 +200,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
 	int i, ret;
 
 	/* If parent is a bridge, add to list */
-	ret = fpga_bridge_get_to_list(region_np->parent, region->info,
+	ret = fpga_bridge_get_to_list(region_np->parent,
+				      reg_ovl->image_info,
 				      &region->bridge_list);
 	if (ret == -EBUSY)
 		return ret;
@@ -192,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
 		parent_br = region_np->parent;
 
 	/* If overlay has a list of bridges, use it. */
-	if (of_parse_phandle(overlay, "fpga-bridges", 0))
-		np = overlay;
+	if (of_parse_phandle(reg_ovl->overlay, "fpga-bridges", 0))
+		np = reg_ovl->overlay;
 	else
 		np = region_np;
 
@@ -207,7 +225,7 @@ static int fpga_region_get_bridges(struct fpga_region *region,
 			continue;
 
 		/* If node is a bridge, get it and add to list */
-		ret = fpga_bridge_get_to_list(br, region->info,
+		ret = fpga_bridge_get_to_list(br, reg_ovl->image_info,
 					      &region->bridge_list);
 
 		/* If any of the bridges are in use, give up */
@@ -222,13 +240,15 @@ static int fpga_region_get_bridges(struct fpga_region *region,
 
 /**
  * fpga_region_program_fpga - program FPGA
- * @region: FPGA region
- * @overlay: device node of the overlay
- * Program an FPGA using information in the region's fpga image info.
- * Return 0 for success or negative error code.
+ * @region: FPGA region that is receiving an overlay
+ * @reg_ovl: region overlay with fpga_image_info parsed from overlay
+ *
+ * Program an FPGA using information in a device tree overlay.
+ *
+ * Return: 0 for success or negative error code.
  */
 static int fpga_region_program_fpga(struct fpga_region *region,
-				    struct device_node *overlay)
+				    struct region_overlay *reg_ovl)
 {
 	struct fpga_manager *mgr;
 	int ret;
@@ -245,7 +265,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		return PTR_ERR(mgr);
 	}
 
-	ret = fpga_region_get_bridges(region, overlay);
+	ret = fpga_region_get_bridges(region, reg_ovl);
 	if (ret) {
 		pr_err("failed to get fpga region bridges\n");
 		goto err_put_mgr;
@@ -257,7 +277,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_br;
 	}
 
-	ret = fpga_mgr_load(mgr, region->info);
+	ret = fpga_mgr_load(mgr, reg_ovl->image_info);
 	if (ret) {
 		pr_err("failed to load fpga image\n");
 		goto err_put_br;
@@ -321,80 +341,135 @@ static int child_regions_with_firmware(struct device_node *overlay)
 }
 
 /**
- * fpga_region_notify_pre_apply - pre-apply overlay notification
- *
- * @region: FPGA region that the overlay was applied to
- * @nd: overlay notification data
- *
- * Called after when an overlay targeted to a FPGA Region is about to be
- * applied.  Function will check the properties that will be added to the FPGA
- * region.  If the checks pass, it will program the FPGA.
- *
- * The checks are:
- * The overlay must add either firmware-name or external-fpga-config property
- * to the FPGA Region.
+ * fpga_region_parse_ov - parse and check overlay applied to region
  *
- *   firmware-name        : program the FPGA
- *   external-fpga-config : FPGA is already programmed
- *
- * The overlay can add other FPGA regions, but child FPGA regions cannot have a
- * firmware-name property since those regions don't exist yet.
+ * @region: FPGA region
+ * @overlay: overlay applied to the FPGA region
  *
- * If the overlay that breaks the rules, notifier returns an error and the
- * overlay is rejected before it goes into the main tree.
+ * Given an overlay applied to a FPGA region, parse the FPGA image specific
+ * info in the overlay and do some checking.
  *
- * Returns 0 for success or negative error code for failure.
+ * Returns:
+ *   NULL if overlay doesn't direct us to program the FPGA.
+ *   fpga_image_info struct if there is an image to program.
+ *   error code for invalid overlay.
  */
-static int fpga_region_notify_pre_apply(struct fpga_region *region,
-					struct of_overlay_notify_data *nd)
+static struct fpga_image_info *fpga_region_parse_ov(struct fpga_region *region,
+						    struct device_node *overlay)
 {
+	struct device *dev = &region->dev;
 	struct fpga_image_info *info;
 	int ret;
 
-	info = devm_kzalloc(&region->dev, sizeof(*info), GFP_KERNEL);
-	if (!info)
-		return -ENOMEM;
-
-	region->info = info;
-
-	/* Reject overlay if child FPGA Regions have firmware-name property */
-	ret = child_regions_with_firmware(nd->overlay);
+	/*
+	 * Reject overlay if child FPGA Regions added in the overlay have
+	 * firmware-name property (would mean that an FPGA region that has
+	 * not been added to the live tree yet is doing FPGA programming).
+	 */
+	ret = child_regions_with_firmware(overlay);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
+
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
 
-	/* Read FPGA region properties from the overlay */
-	if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
+	if (of_property_read_bool(overlay, "partial-fpga-config"))
 		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
 
-	if (of_property_read_bool(nd->overlay, "external-fpga-config"))
+	if (of_property_read_bool(overlay, "external-fpga-config"))
 		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
 
-	of_property_read_string(nd->overlay, "firmware-name",
-				&info->firmware_name);
+	of_property_read_string(overlay, "firmware-name", &info->firmware_name);
 
-	of_property_read_u32(nd->overlay, "region-unfreeze-timeout-us",
+	of_property_read_u32(overlay, "region-unfreeze-timeout-us",
 			     &info->enable_timeout_us);
 
-	of_property_read_u32(nd->overlay, "region-freeze-timeout-us",
+	of_property_read_u32(overlay, "region-freeze-timeout-us",
 			     &info->disable_timeout_us);
 
-	/* If FPGA was externally programmed, don't specify firmware */
-	if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && info->firmware_name) {
-		pr_err("error: specified firmware and external-fpga-config");
-		return -EINVAL;
+	/* If overlay is not programming the FPGA, don't need FPGA image info */
+	if (!info->firmware_name) {
+		devm_kfree(dev, info);
+		return NULL;
 	}
 
-	/* FPGA is already configured externally.  We're done. */
-	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG)
-		return 0;
+	/*
+	 * If overlay informs us FPGA was externally programmed, specifying
+	 * firmware here would be ambiguous.
+	 */
+	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
+		dev_err(dev, "error: specified firmware and external-fpga-config");
+		devm_kfree(dev, info);
+		return ERR_PTR(-EINVAL);
+	}
 
-	/* If we got this far, we should be programming the FPGA */
-	if (!info->firmware_name) {
-		pr_err("should specify firmware-name or external-fpga-config\n");
-		return -EINVAL;
+	/*
+	 * The first overlay to a region may reprogram the FPGA and specify how
+	 * to program the fpga (fpga_image_info).  Subsequent overlays can be
+	 * can add/modify child node properties if that is useful.
+	 */
+	if (!list_empty(&region->overlays)) {
+		dev_err(dev, "Only 1st DTO to a region may program a FPGA.\n");
+		devm_kfree(dev, info);
+		return ERR_PTR(-EINVAL);
 	}
 
-	return fpga_region_program_fpga(region, nd->overlay);
+	return info;
+}
+
+/**
+ * fpga_region_notify_pre_apply - pre-apply overlay notification
+ *
+ * @region: FPGA region that the overlay will be applied to
+ * @nd: overlay notification data
+ *
+ * Called when an overlay targeted to a FPGA Region is about to be applied.
+ * Parses the overlay for properties that influence how the FPGA will be
+ * programmed and does some checking. If the checks pass, programs the FPGA.
+ *
+ * If the overlay that breaks the rules, notifier returns an error and the
+ * overlay is rejected, preventing it from being added to the main tree.
+ *
+ * Return: 0 for success or negative error code for failure.
+ */
+static int fpga_region_notify_pre_apply(struct fpga_region *region,
+					struct of_overlay_notify_data *nd)
+{
+	struct device *dev = &region->dev;
+	struct region_overlay *reg_ovl;
+	struct fpga_image_info *info;
+	unsigned long flags;
+	int ret;
+
+	info = fpga_region_parse_ov(region, nd->overlay);
+	if (IS_ERR(info))
+		return PTR_ERR(info);
+
+	reg_ovl = devm_kzalloc(dev, sizeof(*reg_ovl), GFP_KERNEL);
+	if (!reg_ovl)
+		return -ENOMEM;
+
+	reg_ovl->overlay = nd->overlay;
+	reg_ovl->image_info = info;
+
+	if (info) {
+		ret = fpga_region_program_fpga(region, reg_ovl);
+		if (ret)
+			goto pre_a_err;
+	}
+
+	spin_lock_irqsave(&overlay_list_lock, flags);
+	list_add_tail(&reg_ovl->node, &region->overlays);
+	spin_unlock_irqrestore(&overlay_list_lock, flags);
+
+	return 0;
+
+pre_a_err:
+	if (info)
+		devm_kfree(dev, info);
+	devm_kfree(dev, reg_ovl);
+	return ret;
 }
 
 /**
@@ -409,10 +484,22 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 static void fpga_region_notify_post_remove(struct fpga_region *region,
 					   struct of_overlay_notify_data *nd)
 {
+	struct region_overlay *reg_ovl;
+	unsigned long flags;
+
+	reg_ovl = list_last_entry(&region->overlays, typeof(*reg_ovl), node);
+
 	fpga_bridges_disable(&region->bridge_list);
 	fpga_bridges_put(&region->bridge_list);
-	devm_kfree(&region->dev, region->info);
-	region->info = NULL;
+
+	spin_lock_irqsave(&overlay_list_lock, flags);
+	list_del(&reg_ovl->node);
+	spin_unlock_irqrestore(&overlay_list_lock, flags);
+
+	if (reg_ovl->image_info)
+		devm_kfree(&region->dev, reg_ovl->image_info);
+
+	devm_kfree(&region->dev, reg_ovl);
 }
 
 /**
@@ -496,6 +583,7 @@ static int fpga_region_probe(struct platform_device *pdev)
 
 	mutex_init(&region->mutex);
 	INIT_LIST_HEAD(&region->bridge_list);
+	INIT_LIST_HEAD(&region->overlays);
 
 	device_initialize(&region->dev);
 	region->dev.class = fpga_region_class;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1582712 — Re: [RFC 2/8] fpga-region: support more than one overlay per FPGA region

Hi Alan,

On Wed, 15 Feb 2017, Alan Tull wrote:

> Currently if a user applies > 1 overlays to a region
> and removes them, we get a slow warning from devm_kfree.
> because the pointer to the FPGA image info was overwritten.
>
> This commit adds a list to keep track of overlays applied
> to each FPGA region.  Some rules are enforced:
>
> * Only allow the first overlay to a region to do FPGA
>   programming.
> * Allow subsequent overlays to modify properties but
>   not properties regarding FPGA programming.
> * To reprogram a region, first remove all previous
>   overlays to the region.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
> drivers/fpga/fpga-region.c | 222 +++++++++++++++++++++++++++++++--------------
> 1 file changed, 155 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index 24f4ed5..60d2947 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -31,15 +31,32 @@
>  * @dev: FPGA Region device
>  * @mutex: enforces exclusive reference to region
>  * @bridge_list: list of FPGA bridges specified in region
> - * @info: fpga image specific information
> + * @overlays: list of struct region_overlay_info
>  */
> struct fpga_region {
> 	struct device dev;
> 	struct mutex mutex; /* for exclusive reference to region */
> 	struct list_head bridge_list;
> -	struct fpga_image_info *info;
> +	struct list_head overlays;
> };
>
> +/**
> + * struct region_overlay: info regarding overlays applied to the region
> + * @node: list node
> + * @overlay: pointer to overlay
> + * @image_info: fpga image specific information parsed from overlay.  Is NULL if
> + *        overlay doesn't program FPGA.
> + */
> +
> +struct region_overlay {
> +	struct list_head node;
> +	struct device_node *overlay;
> +	struct fpga_image_info *image_info;
> +};
> +
> +/* Lock for list of overlays */
> +spinlock_t overlay_list_lock;

Each region has its own list of overlays.  Shouldn't each region have its 
own lock?  Otherwise only one region can get overlays applied at one time 
even though there may be more than one region and one fpga-mgr per region.


> +
> #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
>
> static DEFINE_IDA(fpga_region_ida);
> @@ -161,7 +178,7 @@ static struct fpga_manager *fpga_region_get_manager(struct fpga_region *region)
> /**
>  * fpga_region_get_bridges - create a list of bridges
>  * @region: FPGA region
> - * @overlay: device node of the overlay
> + * @reg_ovl: overlay applied to the region
>  *
>  * Create a list of bridges including the parent bridge and the bridges
>  * specified by "fpga-bridges" property.  Note that the
> @@ -175,7 +192,7 @@ static struct fpga_manager *fpga_region_get_manager(struct fpga_region *region)
>  * or -EBUSY if any of the bridges are in use.
>  */
> static int fpga_region_get_bridges(struct fpga_region *region,
> -				   struct device_node *overlay)
> +				   struct region_overlay *reg_ovl)
> {
> 	struct device *dev = &region->dev;
> 	struct device_node *region_np = dev->of_node;
> @@ -183,7 +200,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> 	int i, ret;
>
> 	/* If parent is a bridge, add to list */
> -	ret = fpga_bridge_get_to_list(region_np->parent, region->info,
> +	ret = fpga_bridge_get_to_list(region_np->parent,
> +				      reg_ovl->image_info,
> 				      &region->bridge_list);
> 	if (ret == -EBUSY)
> 		return ret;
> @@ -192,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> 		parent_br = region_np->parent;
>
> 	/* If overlay has a list of bridges, use it. */
> -	if (of_parse_phandle(overlay, "fpga-bridges", 0))
> -		np = overlay;
> +	if (of_parse_phandle(reg_ovl->overlay, "fpga-bridges", 0))
> +		np = reg_ovl->overlay;
> 	else
> 		np = region_np;
>
> @@ -207,7 +225,7 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> 			continue;
>
> 		/* If node is a bridge, get it and add to list */
> -		ret = fpga_bridge_get_to_list(br, region->info,
> +		ret = fpga_bridge_get_to_list(br, reg_ovl->image_info,
> 					      &region->bridge_list);
>
> 		/* If any of the bridges are in use, give up */
> @@ -222,13 +240,15 @@ static int fpga_region_get_bridges(struct fpga_region *region,
>
> /**
>  * fpga_region_program_fpga - program FPGA
> - * @region: FPGA region
> - * @overlay: device node of the overlay
> - * Program an FPGA using information in the region's fpga image info.
> - * Return 0 for success or negative error code.
> + * @region: FPGA region that is receiving an overlay
> + * @reg_ovl: region overlay with fpga_image_info parsed from overlay
> + *
> + * Program an FPGA using information in a device tree overlay.
> + *
> + * Return: 0 for success or negative error code.
>  */
> static int fpga_region_program_fpga(struct fpga_region *region,
> -				    struct device_node *overlay)
> +				    struct region_overlay *reg_ovl)
> {
> 	struct fpga_manager *mgr;
> 	int ret;
> @@ -245,7 +265,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
> 		return PTR_ERR(mgr);
> 	}
>
> -	ret = fpga_region_get_bridges(region, overlay);
> +	ret = fpga_region_get_bridges(region, reg_ovl);
> 	if (ret) {
> 		pr_err("failed to get fpga region bridges\n");
> 		goto err_put_mgr;
> @@ -257,7 +277,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
> 		goto err_put_br;
> 	}
>
> -	ret = fpga_mgr_load(mgr, region->info);
> +	ret = fpga_mgr_load(mgr, reg_ovl->image_info);
> 	if (ret) {
> 		pr_err("failed to load fpga image\n");
> 		goto err_put_br;
> @@ -321,80 +341,135 @@ static int child_regions_with_firmware(struct device_node *overlay)
> }
>
> /**
> - * fpga_region_notify_pre_apply - pre-apply overlay notification
> - *
> - * @region: FPGA region that the overlay was applied to
> - * @nd: overlay notification data
> - *
> - * Called after when an overlay targeted to a FPGA Region is about to be
> - * applied.  Function will check the properties that will be added to the FPGA
> - * region.  If the checks pass, it will program the FPGA.
> - *
> - * The checks are:
> - * The overlay must add either firmware-name or external-fpga-config property
> - * to the FPGA Region.
> + * fpga_region_parse_ov - parse and check overlay applied to region
>  *
> - *   firmware-name        : program the FPGA
> - *   external-fpga-config : FPGA is already programmed
> - *
> - * The overlay can add other FPGA regions, but child FPGA regions cannot have a
> - * firmware-name property since those regions don't exist yet.
> + * @region: FPGA region
> + * @overlay: overlay applied to the FPGA region
>  *
> - * If the overlay that breaks the rules, notifier returns an error and the
> - * overlay is rejected before it goes into the main tree.
> + * Given an overlay applied to a FPGA region, parse the FPGA image specific
> + * info in the overlay and do some checking.
>  *
> - * Returns 0 for success or negative error code for failure.
> + * Returns:
> + *   NULL if overlay doesn't direct us to program the FPGA.
> + *   fpga_image_info struct if there is an image to program.
> + *   error code for invalid overlay.
>  */
> -static int fpga_region_notify_pre_apply(struct fpga_region *region,
> -					struct of_overlay_notify_data *nd)
> +static struct fpga_image_info *fpga_region_parse_ov(struct fpga_region *region,
> +						    struct device_node *overlay)
> {
> +	struct device *dev = &region->dev;
> 	struct fpga_image_info *info;
> 	int ret;
>
> -	info = devm_kzalloc(&region->dev, sizeof(*info), GFP_KERNEL);
> -	if (!info)
> -		return -ENOMEM;
> -
> -	region->info = info;
> -
> -	/* Reject overlay if child FPGA Regions have firmware-name property */
> -	ret = child_regions_with_firmware(nd->overlay);
> +	/*
> +	 * Reject overlay if child FPGA Regions added in the overlay have
> +	 * firmware-name property (would mean that an FPGA region that has
> +	 * not been added to the live tree yet is doing FPGA programming).
> +	 */
> +	ret = child_regions_with_firmware(overlay);
> 	if (ret)
> -		return ret;
> +		return ERR_PTR(ret);
> +
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return ERR_PTR(-ENOMEM);
>
> -	/* Read FPGA region properties from the overlay */
> -	if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
> +	if (of_property_read_bool(overlay, "partial-fpga-config"))
> 		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
>
> -	if (of_property_read_bool(nd->overlay, "external-fpga-config"))
> +	if (of_property_read_bool(overlay, "external-fpga-config"))
> 		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
>
> -	of_property_read_string(nd->overlay, "firmware-name",
> -				&info->firmware_name);
> +	of_property_read_string(overlay, "firmware-name", &info->firmware_name);
>
> -	of_property_read_u32(nd->overlay, "region-unfreeze-timeout-us",
> +	of_property_read_u32(overlay, "region-unfreeze-timeout-us",
> 			     &info->enable_timeout_us);
>
> -	of_property_read_u32(nd->overlay, "region-freeze-timeout-us",
> +	of_property_read_u32(overlay, "region-freeze-timeout-us",
> 			     &info->disable_timeout_us);
>
> -	/* If FPGA was externally programmed, don't specify firmware */
> -	if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && info->firmware_name) {
> -		pr_err("error: specified firmware and external-fpga-config");
> -		return -EINVAL;
> +	/* If overlay is not programming the FPGA, don't need FPGA image info */
> +	if (!info->firmware_name) {
> +		devm_kfree(dev, info);
> +		return NULL;
> 	}
>
> -	/* FPGA is already configured externally.  We're done. */
> -	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG)
> -		return 0;
> +	/*
> +	 * If overlay informs us FPGA was externally programmed, specifying
> +	 * firmware here would be ambiguous.
> +	 */
> +	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
> +		dev_err(dev, "error: specified firmware and external-fpga-config");
> +		devm_kfree(dev, info);
> +		return ERR_PTR(-EINVAL);
> +	}
>
> -	/* If we got this far, we should be programming the FPGA */
> -	if (!info->firmware_name) {
> -		pr_err("should specify firmware-name or external-fpga-config\n");
> -		return -EINVAL;
> +	/*
> +	 * The first overlay to a region may reprogram the FPGA and specify how
> +	 * to program the fpga (fpga_image_info).  Subsequent overlays can be
> +	 * can add/modify child node properties if that is useful.
> +	 */
> +	if (!list_empty(&region->overlays)) {
> +		dev_err(dev, "Only 1st DTO to a region may program a FPGA.\n");
> +		devm_kfree(dev, info);
> +		return ERR_PTR(-EINVAL);
> 	}
>
> -	return fpga_region_program_fpga(region, nd->overlay);
> +	return info;
> +}
> +
> +/**
> + * fpga_region_notify_pre_apply - pre-apply overlay notification
> + *
> + * @region: FPGA region that the overlay will be applied to
> + * @nd: overlay notification data
> + *
> + * Called when an overlay targeted to a FPGA Region is about to be applied.
> + * Parses the overlay for properties that influence how the FPGA will be
> + * programmed and does some checking. If the checks pass, programs the FPGA.
> + *
> + * If the overlay that breaks the rules, notifier returns an error and the
> + * overlay is rejected, preventing it from being added to the main tree.
> + *
> + * Return: 0 for success or negative error code for failure.
> + */
> +static int fpga_region_notify_pre_apply(struct fpga_region *region,
> +					struct of_overlay_notify_data *nd)
> +{
> +	struct device *dev = &region->dev;
> +	struct region_overlay *reg_ovl;
> +	struct fpga_image_info *info;
> +	unsigned long flags;
> +	int ret;
> +
> +	info = fpga_region_parse_ov(region, nd->overlay);
> +	if (IS_ERR(info))
> +		return PTR_ERR(info);
> +
> +	reg_ovl = devm_kzalloc(dev, sizeof(*reg_ovl), GFP_KERNEL);
> +	if (!reg_ovl)
> +		return -ENOMEM;
> +
> +	reg_ovl->overlay = nd->overlay;
> +	reg_ovl->image_info = info;
> +
> +	if (info) {
> +		ret = fpga_region_program_fpga(region, reg_ovl);
> +		if (ret)
> +			goto pre_a_err;
> +	}
> +
> +	spin_lock_irqsave(&overlay_list_lock, flags);
> +	list_add_tail(&reg_ovl->node, &region->overlays);
> +	spin_unlock_irqrestore(&overlay_list_lock, flags);
> +
> +	return 0;
> +
> +pre_a_err:
> +	if (info)
> +		devm_kfree(dev, info);
> +	devm_kfree(dev, reg_ovl);
> +	return ret;
> }
>
> /**
> @@ -409,10 +484,22 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
> static void fpga_region_notify_post_remove(struct fpga_region *region,
> 					   struct of_overlay_notify_data *nd)
> {
> +	struct region_overlay *reg_ovl;
> +	unsigned long flags;
> +
> +	reg_ovl = list_last_entry(&region->overlays, typeof(*reg_ovl), node);
> +
> 	fpga_bridges_disable(&region->bridge_list);
> 	fpga_bridges_put(&region->bridge_list);
> -	devm_kfree(&region->dev, region->info);
> -	region->info = NULL;
> +
> +	spin_lock_irqsave(&overlay_list_lock, flags);
> +	list_del(&reg_ovl->node);
> +	spin_unlock_irqrestore(&overlay_list_lock, flags);
> +
> +	if (reg_ovl->image_info)
> +		devm_kfree(&region->dev, reg_ovl->image_info);
> +
> +	devm_kfree(&region->dev, reg_ovl);
> }
>
> /**
> @@ -496,6 +583,7 @@ static int fpga_region_probe(struct platform_device *pdev)
>
> 	mutex_init(&region->mutex);
> 	INIT_LIST_HEAD(&region->bridge_list);
> +	INIT_LIST_HEAD(&region->overlays);
>
> 	device_initialize(&region->dev);
> 	region->dev.class = fpga_region_class;
> -- 
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1582730 — Re: [RFC 2/8] fpga-region: support more than one overlay per FPGA region

On Thu, Feb 16, 2017 at 10:50 AM,  <matthew.gerlach@linux.intel.com> wrote:
>
>
> Hi Alan,
>
> On Wed, 15 Feb 2017, Alan Tull wrote:
>
>> Currently if a user applies > 1 overlays to a region
>> and removes them, we get a slow warning from devm_kfree.
>> because the pointer to the FPGA image info was overwritten.
>>
>> This commit adds a list to keep track of overlays applied
>> to each FPGA region.  Some rules are enforced:
>>
>> * Only allow the first overlay to a region to do FPGA
>>   programming.
>> * Allow subsequent overlays to modify properties but
>>   not properties regarding FPGA programming.
>> * To reprogram a region, first remove all previous
>>   overlays to the region.
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
>> ---
>> drivers/fpga/fpga-region.c | 222
>> +++++++++++++++++++++++++++++++--------------
>> 1 file changed, 155 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index 24f4ed5..60d2947 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -31,15 +31,32 @@
>>  * @dev: FPGA Region device
>>  * @mutex: enforces exclusive reference to region
>>  * @bridge_list: list of FPGA bridges specified in region
>> - * @info: fpga image specific information
>> + * @overlays: list of struct region_overlay_info
>>  */
>> struct fpga_region {
>>         struct device dev;
>>         struct mutex mutex; /* for exclusive reference to region */
>>         struct list_head bridge_list;
>> -       struct fpga_image_info *info;
>> +       struct list_head overlays;
>> };
>>
>> +/**
>> + * struct region_overlay: info regarding overlays applied to the region
>> + * @node: list node
>> + * @overlay: pointer to overlay
>> + * @image_info: fpga image specific information parsed from overlay.  Is
>> NULL if
>> + *        overlay doesn't program FPGA.
>> + */
>> +
>> +struct region_overlay {
>> +       struct list_head node;
>> +       struct device_node *overlay;
>> +       struct fpga_image_info *image_info;
>> +};
>> +
>> +/* Lock for list of overlays */
>> +spinlock_t overlay_list_lock;
>
>
> Each region has its own list of overlays.  Shouldn't each region have its
> own lock?  Otherwise only one region can get overlays applied at one time
> even though there may be more than one region and one fpga-mgr per region.

Hi Matthew,

I think the entire kernel is only taking one overlay at a time.  But
it's probably better for me to make that change anyway.

Alan

>
>
>> +
>> #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
>>
>> static DEFINE_IDA(fpga_region_ida);
>> @@ -161,7 +178,7 @@ static struct fpga_manager
>> *fpga_region_get_manager(struct fpga_region *region)
>> /**
>>  * fpga_region_get_bridges - create a list of bridges
>>  * @region: FPGA region
>> - * @overlay: device node of the overlay
>> + * @reg_ovl: overlay applied to the region
>>  *
>>  * Create a list of bridges including the parent bridge and the bridges
>>  * specified by "fpga-bridges" property.  Note that the
>> @@ -175,7 +192,7 @@ static struct fpga_manager
>> *fpga_region_get_manager(struct fpga_region *region)
>>  * or -EBUSY if any of the bridges are in use.
>>  */
>> static int fpga_region_get_bridges(struct fpga_region *region,
>> -                                  struct device_node *overlay)
>> +                                  struct region_overlay *reg_ovl)
>> {
>>         struct device *dev = &region->dev;
>>         struct device_node *region_np = dev->of_node;
>> @@ -183,7 +200,8 @@ static int fpga_region_get_bridges(struct fpga_region
>> *region,
>>         int i, ret;
>>
>>         /* If parent is a bridge, add to list */
>> -       ret = fpga_bridge_get_to_list(region_np->parent, region->info,
>> +       ret = fpga_bridge_get_to_list(region_np->parent,
>> +                                     reg_ovl->image_info,
>>                                       &region->bridge_list);
>>         if (ret == -EBUSY)
>>                 return ret;
>> @@ -192,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region
>> *region,
>>                 parent_br = region_np->parent;
>>
>>         /* If overlay has a list of bridges, use it. */
>> -       if (of_parse_phandle(overlay, "fpga-bridges", 0))
>> -               np = overlay;
>> +       if (of_parse_phandle(reg_ovl->overlay, "fpga-bridges", 0))
>> +               np = reg_ovl->overlay;
>>         else
>>                 np = region_np;
>>
>> @@ -207,7 +225,7 @@ static int fpga_region_get_bridges(struct fpga_region
>> *region,
>>                         continue;
>>
>>                 /* If node is a bridge, get it and add to list */
>> -               ret = fpga_bridge_get_to_list(br, region->info,
>> +               ret = fpga_bridge_get_to_list(br, reg_ovl->image_info,
>>                                               &region->bridge_list);
>>
>>                 /* If any of the bridges are in use, give up */
>> @@ -222,13 +240,15 @@ static int fpga_region_get_bridges(struct
>> fpga_region *region,
>>
>> /**
>>  * fpga_region_program_fpga - program FPGA
>> - * @region: FPGA region
>> - * @overlay: device node of the overlay
>> - * Program an FPGA using information in the region's fpga image info.
>> - * Return 0 for success or negative error code.
>> + * @region: FPGA region that is receiving an overlay
>> + * @reg_ovl: region overlay with fpga_image_info parsed from overlay
>> + *
>> + * Program an FPGA using information in a device tree overlay.
>> + *
>> + * Return: 0 for success or negative error code.
>>  */
>> static int fpga_region_program_fpga(struct fpga_region *region,
>> -                                   struct device_node *overlay)
>> +                                   struct region_overlay *reg_ovl)
>> {
>>         struct fpga_manager *mgr;
>>         int ret;
>> @@ -245,7 +265,7 @@ static int fpga_region_program_fpga(struct fpga_region
>> *region,
>>                 return PTR_ERR(mgr);
>>         }
>>
>> -       ret = fpga_region_get_bridges(region, overlay);
>> +       ret = fpga_region_get_bridges(region, reg_ovl);
>>         if (ret) {
>>                 pr_err("failed to get fpga region bridges\n");
>>                 goto err_put_mgr;
>> @@ -257,7 +277,7 @@ static int fpga_region_program_fpga(struct fpga_region
>> *region,
>>                 goto err_put_br;
>>         }
>>
>> -       ret = fpga_mgr_load(mgr, region->info);
>> +       ret = fpga_mgr_load(mgr, reg_ovl->image_info);
>>         if (ret) {
>>                 pr_err("failed to load fpga image\n");
>>                 goto err_put_br;
>> @@ -321,80 +341,135 @@ static int child_regions_with_firmware(struct
>> device_node *overlay)
>> }
>>
>> /**
>> - * fpga_region_notify_pre_apply - pre-apply overlay notification
>> - *
>> - * @region: FPGA region that the overlay was applied to
>> - * @nd: overlay notification data
>> - *
>> - * Called after when an overlay targeted to a FPGA Region is about to be
>> - * applied.  Function will check the properties that will be added to the
>> FPGA
>> - * region.  If the checks pass, it will program the FPGA.
>> - *
>> - * The checks are:
>> - * The overlay must add either firmware-name or external-fpga-config
>> property
>> - * to the FPGA Region.
>> + * fpga_region_parse_ov - parse and check overlay applied to region
>>  *
>> - *   firmware-name        : program the FPGA
>> - *   external-fpga-config : FPGA is already programmed
>> - *
>> - * The overlay can add other FPGA regions, but child FPGA regions cannot
>> have a
>> - * firmware-name property since those regions don't exist yet.
>> + * @region: FPGA region
>> + * @overlay: overlay applied to the FPGA region
>>  *
>> - * If the overlay that breaks the rules, notifier returns an error and
>> the
>> - * overlay is rejected before it goes into the main tree.
>> + * Given an overlay applied to a FPGA region, parse the FPGA image
>> specific
>> + * info in the overlay and do some checking.
>>  *
>> - * Returns 0 for success or negative error code for failure.
>> + * Returns:
>> + *   NULL if overlay doesn't direct us to program the FPGA.
>> + *   fpga_image_info struct if there is an image to program.
>> + *   error code for invalid overlay.
>>  */
>> -static int fpga_region_notify_pre_apply(struct fpga_region *region,
>> -                                       struct of_overlay_notify_data *nd)
>> +static struct fpga_image_info *fpga_region_parse_ov(struct fpga_region
>> *region,
>> +                                                   struct device_node
>> *overlay)
>> {
>> +       struct device *dev = &region->dev;
>>         struct fpga_image_info *info;
>>         int ret;
>>
>> -       info = devm_kzalloc(&region->dev, sizeof(*info), GFP_KERNEL);
>> -       if (!info)
>> -               return -ENOMEM;
>> -
>> -       region->info = info;
>> -
>> -       /* Reject overlay if child FPGA Regions have firmware-name
>> property */
>> -       ret = child_regions_with_firmware(nd->overlay);
>> +       /*
>> +        * Reject overlay if child FPGA Regions added in the overlay have
>> +        * firmware-name property (would mean that an FPGA region that has
>> +        * not been added to the live tree yet is doing FPGA programming).
>> +        */
>> +       ret = child_regions_with_firmware(overlay);
>>         if (ret)
>> -               return ret;
>> +               return ERR_PTR(ret);
>> +
>> +       info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> +       if (!info)
>> +               return ERR_PTR(-ENOMEM);
>>
>> -       /* Read FPGA region properties from the overlay */
>> -       if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
>> +       if (of_property_read_bool(overlay, "partial-fpga-config"))
>>                 info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
>>
>> -       if (of_property_read_bool(nd->overlay, "external-fpga-config"))
>> +       if (of_property_read_bool(overlay, "external-fpga-config"))
>>                 info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
>>
>> -       of_property_read_string(nd->overlay, "firmware-name",
>> -                               &info->firmware_name);
>> +       of_property_read_string(overlay, "firmware-name",
>> &info->firmware_name);
>>
>> -       of_property_read_u32(nd->overlay, "region-unfreeze-timeout-us",
>> +       of_property_read_u32(overlay, "region-unfreeze-timeout-us",
>>                              &info->enable_timeout_us);
>>
>> -       of_property_read_u32(nd->overlay, "region-freeze-timeout-us",
>> +       of_property_read_u32(overlay, "region-freeze-timeout-us",
>>                              &info->disable_timeout_us);
>>
>> -       /* If FPGA was externally programmed, don't specify firmware */
>> -       if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) &&
>> info->firmware_name) {
>> -               pr_err("error: specified firmware and
>> external-fpga-config");
>> -               return -EINVAL;
>> +       /* If overlay is not programming the FPGA, don't need FPGA image
>> info */
>> +       if (!info->firmware_name) {
>> +               devm_kfree(dev, info);
>> +               return NULL;
>>         }
>>
>> -       /* FPGA is already configured externally.  We're done. */
>> -       if (info->flags & FPGA_MGR_EXTERNAL_CONFIG)
>> -               return 0;
>> +       /*
>> +        * If overlay informs us FPGA was externally programmed,
>> specifying
>> +        * firmware here would be ambiguous.
>> +        */
>> +       if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
>> +               dev_err(dev, "error: specified firmware and
>> external-fpga-config");
>> +               devm_kfree(dev, info);
>> +               return ERR_PTR(-EINVAL);
>> +       }
>>
>> -       /* If we got this far, we should be programming the FPGA */
>> -       if (!info->firmware_name) {
>> -               pr_err("should specify firmware-name or
>> external-fpga-config\n");
>> -               return -EINVAL;
>> +       /*
>> +        * The first overlay to a region may reprogram the FPGA and
>> specify how
>> +        * to program the fpga (fpga_image_info).  Subsequent overlays can
>> be
>> +        * can add/modify child node properties if that is useful.
>> +        */
>> +       if (!list_empty(&region->overlays)) {
>> +               dev_err(dev, "Only 1st DTO to a region may program a
>> FPGA.\n");
>> +               devm_kfree(dev, info);
>> +               return ERR_PTR(-EINVAL);
>>         }
>>
>> -       return fpga_region_program_fpga(region, nd->overlay);
>> +       return info;
>> +}
>> +
>> +/**
>> + * fpga_region_notify_pre_apply - pre-apply overlay notification
>> + *
>> + * @region: FPGA region that the overlay will be applied to
>> + * @nd: overlay notification data
>> + *
>> + * Called when an overlay targeted to a FPGA Region is about to be
>> applied.
>> + * Parses the overlay for properties that influence how the FPGA will be
>> + * programmed and does some checking. If the checks pass, programs the
>> FPGA.
>> + *
>> + * If the overlay that breaks the rules, notifier returns an error and
>> the
>> + * overlay is rejected, preventing it from being added to the main tree.
>> + *
>> + * Return: 0 for success or negative error code for failure.
>> + */
>> +static int fpga_region_notify_pre_apply(struct fpga_region *region,
>> +                                       struct of_overlay_notify_data *nd)
>> +{
>> +       struct device *dev = &region->dev;
>> +       struct region_overlay *reg_ovl;
>> +       struct fpga_image_info *info;
>> +       unsigned long flags;
>> +       int ret;
>> +
>> +       info = fpga_region_parse_ov(region, nd->overlay);
>> +       if (IS_ERR(info))
>> +               return PTR_ERR(info);
>> +
>> +       reg_ovl = devm_kzalloc(dev, sizeof(*reg_ovl), GFP_KERNEL);
>> +       if (!reg_ovl)
>> +               return -ENOMEM;
>> +
>> +       reg_ovl->overlay = nd->overlay;
>> +       reg_ovl->image_info = info;
>> +
>> +       if (info) {
>> +               ret = fpga_region_program_fpga(region, reg_ovl);
>> +               if (ret)
>> +                       goto pre_a_err;
>> +       }
>> +
>> +       spin_lock_irqsave(&overlay_list_lock, flags);
>> +       list_add_tail(&reg_ovl->node, &region->overlays);
>> +       spin_unlock_irqrestore(&overlay_list_lock, flags);
>> +
>> +       return 0;
>> +
>> +pre_a_err:
>> +       if (info)
>> +               devm_kfree(dev, info);
>> +       devm_kfree(dev, reg_ovl);
>> +       return ret;
>> }
>>
>> /**
>> @@ -409,10 +484,22 @@ static int fpga_region_notify_pre_apply(struct
>> fpga_region *region,
>> static void fpga_region_notify_post_remove(struct fpga_region *region,
>>                                            struct of_overlay_notify_data
>> *nd)
>> {
>> +       struct region_overlay *reg_ovl;
>> +       unsigned long flags;
>> +
>> +       reg_ovl = list_last_entry(&region->overlays, typeof(*reg_ovl),
>> node);
>> +
>>         fpga_bridges_disable(&region->bridge_list);
>>         fpga_bridges_put(&region->bridge_list);
>> -       devm_kfree(&region->dev, region->info);
>> -       region->info = NULL;
>> +
>> +       spin_lock_irqsave(&overlay_list_lock, flags);
>> +       list_del(&reg_ovl->node);
>> +       spin_unlock_irqrestore(&overlay_list_lock, flags);
>> +
>> +       if (reg_ovl->image_info)
>> +               devm_kfree(&region->dev, reg_ovl->image_info);
>> +
>> +       devm_kfree(&region->dev, reg_ovl);
>> }
>>
>> /**
>> @@ -496,6 +583,7 @@ static int fpga_region_probe(struct platform_device
>> *pdev)
>>
>>         mutex_init(&region->mutex);
>>         INIT_LIST_HEAD(&region->bridge_list);
>> +       INIT_LIST_HEAD(&region->overlays);
>>
>>         device_initialize(&region->dev);
>>         region->dev.class = fpga_region_class;
>> --
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

[toc] | [prev] | [next] | [standalone]

#1589651

On Wed, Feb 15, 2017 at 10:14 AM, Alan Tull <atull@kernel.org> wrote:
> This patchset intends to enable expanding the use of FPGA regions
> beyond device tree overlays.  Also one fix for the existing DTO
> implementation.  It's an RFC, looking for feedback, also I need
> to do more testing and fix it working for modules.

There's a lot of stuff to look at here.  To make it easier, I've pushed a
branch to the linux-fpga repo.  Branch name is
next-20170228-rfc-atull-20170210

Alan

>
> Patch 1 adds a function so the caller could program the fpga from
> either a scatter gather table, a buffer, or a firmware file.  The
> parameters are passed in the fpga_image_info struct.  This way works,
> but there may be a better or more widely accepted way.  Maybe they
> should be a union?  If someone knows of a well written example in the
> kernel for me to emulate, that would be really appreciated.
>
> Patch 2 is a fix for if you write > 1 overlay to a region.
> It keeps track of overlays in a list.
>
> Patch 3 adds functions for working with FPGA bridges using the
> device rather than the device node (for non DT use).
>
> Patches 4-5 separate finding and locking a FPGA manager.  So someone
> could get a reference for a FPGA manager without locking it for
> exclusive use.
>
> Patch 6 breaks up fpga-region.c into two files, moving the DT overlay
> support to of-fpga-region.c.  The functions exported by fpga-region.c
> will enable code that creates an FPGA region and tell it what manager
> and bridge to use.  fpga-region.c doesn't do enumeration so whatever
> code creates the region will also still have the responsibility to do
> some enumeration after programming.
>
> Patches 7-8 a sysfs interface to FPGA regions.  I'm sure this will be
> controversial as discussions about FPGA userspace interfaces have
> incited lively discussion in the past.  The nice thing about this
> interface is that it handles the dance of disabling the bridge before
> programming and reenabling it afterwards.  But no enumeration.
> I post it as separate patch and document so the rest of the patches
> could go forward while we hash out what a good non-DT interface
> may look like if there is some other layer that is requesting
> reprogramming and handling enumeration.
>
> I've tested it lightly and believe that each patch separately
> builds and works.
>
> Known issues: doesn't work as a module anymore.  I'll be fixing that.
>
> Alan
>
> Alan Tull (8):
>   fpga-mgr: add a single function for fpga loading methods
>   fpga-region: support more than one overlay per FPGA region
>   fpga-bridge: add non-dt support
>   doc: fpga-mgr: separate getting/locking FPGA manager
>   fpga-mgr: separate getting/locking FPGA manager
>   fpga-region: separate out common code to allow non-dt support
>   fpga-region: add sysfs interface
>   doc: fpga: add sysfs document for fpga region
>
>  Documentation/ABI/testing/sysfs-class-fpga-region |  26 +
>  Documentation/fpga/fpga-mgr.txt                   |  19 +-
>  drivers/fpga/Kconfig                              |  20 +-
>  drivers/fpga/Makefile                             |   1 +
>  drivers/fpga/fpga-bridge.c                        | 107 +++-
>  drivers/fpga/fpga-mgr.c                           |  56 +-
>  drivers/fpga/fpga-region.c                        | 725 ++++++++++------------
>  drivers/fpga/fpga-region.h                        |  68 ++
>  drivers/fpga/of-fpga-region.c                     | 510 +++++++++++++++
>  include/linux/fpga/fpga-bridge.h                  |   7 +-
>  include/linux/fpga/fpga-mgr.h                     |  17 +
>  11 files changed, 1128 insertions(+), 428 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-region
>  create mode 100644 drivers/fpga/fpga-region.h
>  create mode 100644 drivers/fpga/of-fpga-region.c
>
> --
> 2.7.4
>

[toc] | [prev] | [next] | [standalone]

#1589853

On Tue, Feb 28, 2017 at 11:35 AM, Alan Tull <delicious.quinoa@gmail.com> wrote:
> On Wed, Feb 15, 2017 at 10:14 AM, Alan Tull <atull@kernel.org> wrote:
>> This patchset intends to enable expanding the use of FPGA regions
>> beyond device tree overlays.  Also one fix for the existing DTO
>> implementation.  It's an RFC, looking for feedback, also I need
>> to do more testing and fix it working for modules.
>
> There's a lot of stuff to look at here.  To make it easier, I've pushed a
> branch to the linux-fpga repo.  Branch name is
> next-20170228-rfc-atull-20170210
>
> Alan
>
>>
>> Patch 1 adds a function so the caller could program the fpga from
>> either a scatter gather table, a buffer, or a firmware file.  The
>> parameters are passed in the fpga_image_info struct.  This way works,
>> but there may be a better or more widely accepted way.  Maybe they
>> should be a union?  If someone knows of a well written example in the
>> kernel for me to emulate, that would be really appreciated.
>>
>> Patch 2 is a fix for if you write > 1 overlay to a region.
>> It keeps track of overlays in a list.
>>
>> Patch 3 adds functions for working with FPGA bridges using the
>> device rather than the device node (for non DT use).
>>
>> Patches 4-5 separate finding and locking a FPGA manager.  So someone
>> could get a reference for a FPGA manager without locking it for
>> exclusive use.
>>
>> Patch 6 breaks up fpga-region.c into two files, moving the DT overlay
>> support to of-fpga-region.c.  The functions exported by fpga-region.c
>> will enable code that creates an FPGA region and tell it what manager
>> and bridge to use.  fpga-region.c doesn't do enumeration so whatever
>> code creates the region will also still have the responsibility to do
>> some enumeration after programming.
>>
>> Patches 7-8 a sysfs interface to FPGA regions.  I'm sure this will be
>> controversial as discussions about FPGA userspace interfaces have
>> incited lively discussion in the past.  The nice thing about this
>> interface is that it handles the dance of disabling the bridge before
>> programming and reenabling it afterwards.  But no enumeration.
>> I post it as separate patch and document so the rest of the patches
>> could go forward while we hash out what a good non-DT interface
>> may look like if there is some other layer that is requesting
>> reprogramming and handling enumeration.
>>
>> I've tested it lightly and believe that each patch separately
>> builds and works.
>>
>> Known issues: doesn't work as a module anymore.  I'll be fixing that.

Actually just reran my tests and these *do* work as a modules.

Alan

>>
>> Alan
>>
>> Alan Tull (8):
>>   fpga-mgr: add a single function for fpga loading methods
>>   fpga-region: support more than one overlay per FPGA region
>>   fpga-bridge: add non-dt support
>>   doc: fpga-mgr: separate getting/locking FPGA manager
>>   fpga-mgr: separate getting/locking FPGA manager
>>   fpga-region: separate out common code to allow non-dt support
>>   fpga-region: add sysfs interface
>>   doc: fpga: add sysfs document for fpga region
>>
>>  Documentation/ABI/testing/sysfs-class-fpga-region |  26 +
>>  Documentation/fpga/fpga-mgr.txt                   |  19 +-
>>  drivers/fpga/Kconfig                              |  20 +-
>>  drivers/fpga/Makefile                             |   1 +
>>  drivers/fpga/fpga-bridge.c                        | 107 +++-
>>  drivers/fpga/fpga-mgr.c                           |  56 +-
>>  drivers/fpga/fpga-region.c                        | 725 ++++++++++------------
>>  drivers/fpga/fpga-region.h                        |  68 ++
>>  drivers/fpga/of-fpga-region.c                     | 510 +++++++++++++++
>>  include/linux/fpga/fpga-bridge.h                  |   7 +-
>>  include/linux/fpga/fpga-mgr.h                     |  17 +
>>  11 files changed, 1128 insertions(+), 428 deletions(-)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-region
>>  create mode 100644 drivers/fpga/fpga-region.h
>>  create mode 100644 drivers/fpga/of-fpga-region.c
>>
>> --
>> 2.7.4
>>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web