[PATCH 0/5] fpga: enhancements to support non-dt code

[PATCH 0/5] fpga: enhancements to support non-dt code

[Alan Tull]

Set of patches that supports writing code that uses
FPGA regions without Device Tree overlays.

Main differences from the RFC:
* dropping the sysfs interface
* dropping support for more than one DT overlay on a region

Alan Tull (5):
  fpga-mgr: pass parameters for loading fpga in image info
  fpga-bridge: support getting bridge from device
  doc: fpga-mgr: separate getting/locking FPGA manager
  fpga-mgr: separate getting/locking FPGA manager
  fpga-region: separate out common code from dt specific code

 Documentation/fpga/fpga-mgr.txt  |  19 +-
 drivers/fpga/Kconfig             |  12 +-
 drivers/fpga/Makefile            |   1 +
 drivers/fpga/fpga-bridge.c       | 107 +++++++--
 drivers/fpga/fpga-mgr.c          |  56 ++++-
 drivers/fpga/fpga-region.c       | 473 +++++++--------------------------------
 drivers/fpga/fpga-region.h       |  50 +++++
 drivers/fpga/of-fpga-region.c    | 453 +++++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-bridge.h |   7 +-
 include/linux/fpga/fpga-mgr.h    |  17 ++
 10 files changed, 766 insertions(+), 429 deletions(-)
 create mode 100644 drivers/fpga/fpga-region.h
 create mode 100644 drivers/fpga/of-fpga-region.c

-- 
2.7.4

[PATCH 1/5] fpga-mgr: pass parameters for loading fpga in image info

[Alan Tull]

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
struct 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>
---
v1: changed order that methods are tried
---
 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..fde605b 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->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);
+	if (info->firmware_name)
+		return fpga_mgr_firmware_load(mgr, info, info->firmware_name);
+	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

[PATCH 2/5] fpga-bridge: support getting bridge from device

[Alan Tull]

Add two functions for getting the FPGA bridge from the device
rather than device tree node.  This is to enable writing code
that will support using FPGA bridges without device tree.
Rename one old function to make it clear that it is device
tree-ish.

* fpga_bridge_get
  Get the bridge given the device.

* fpga_bridges_get_to_list
  Given the device, get the bridge and add it to a list.

* of_fpga_bridges_get_to_list
  Renamed from priviously existing fpga_bridges_get_to_list.
  Given the device node, get the bridge and add it to a list.

Signed-off-by: Alan Tull <atull@kernel.org>
---
 drivers/fpga/fpga-bridge.c       | 107 +++++++++++++++++++++++++++++++--------
 drivers/fpga/fpga-region.c       |   8 +--
 include/linux/fpga/fpga-bridge.h |   7 ++-
 3 files changed, 95 insertions(+), 27 deletions(-)

diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
index 0f8b7dc..11fd40c 100644
--- a/drivers/fpga/fpga-bridge.c
+++ b/drivers/fpga/fpga-bridge.c
@@ -70,29 +70,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge)
 }
 EXPORT_SYMBOL_GPL(fpga_bridge_disable);
 
-/**
- * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
- *
- * @np: node pointer of a FPGA bridge
- * @info: fpga image specific information
- *
- * Return fpga_bridge struct if successful.
- * Return -EBUSY if someone already has a reference to the bridge.
- * Return -ENODEV if @np is not a FPGA Bridge.
- */
-struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
-				       struct fpga_image_info *info)
-
+struct fpga_bridge *__fpga_bridge_get(struct device *dev,
+				      struct fpga_image_info *info)
 {
-	struct device *dev;
 	struct fpga_bridge *bridge;
 	int ret = -ENODEV;
 
-	dev = class_find_device(fpga_bridge_class, NULL, np,
-				fpga_bridge_of_node_match);
-	if (!dev)
-		goto err_dev;
-
 	bridge = to_fpga_bridge(dev);
 	if (!bridge)
 		goto err_dev;
@@ -117,8 +100,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
 	put_device(dev);
 	return ERR_PTR(ret);
 }
+
+/**
+ * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
+ *
+ * @np: node pointer of a FPGA bridge
+ * @info: fpga image specific information
+ *
+ * Return fpga_bridge struct if successful.
+ * Return -EBUSY if someone already has a reference to the bridge.
+ * Return -ENODEV if @np is not a FPGA Bridge.
+ */
+struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
+				       struct fpga_image_info *info)
+{
+	struct device *dev;
+
+	dev = class_find_device(fpga_bridge_class, NULL, np,
+				fpga_bridge_of_node_match);
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	return __fpga_bridge_get(dev, info);
+}
 EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
 
+static int fpga_bridge_dev_match(struct device *dev, const void *data)
+{
+	return dev->parent == data;
+}
+
+/**
+ * fpga_bridge_get - get an exclusive reference to a fpga bridge
+ * @dev:	parent device that fpga bridge was registered with
+ *
+ * Given a device, get an exclusive reference to a fpga bridge.
+ *
+ * Return: fpga manager struct or IS_ERR() condition containing error code.
+ */
+struct fpga_bridge *fpga_bridge_get(struct device *dev,
+				    struct fpga_image_info *info)
+{
+	struct device *bridge_dev;
+
+	bridge_dev = class_find_device(fpga_bridge_class, NULL, dev,
+				       fpga_bridge_dev_match);
+	if (!bridge_dev)
+		return ERR_PTR(-ENODEV);
+
+	return __fpga_bridge_get(bridge_dev, info);
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_get);
+
 /**
  * fpga_bridge_put - release a reference to a bridge
  *
@@ -213,7 +246,7 @@ void fpga_bridges_put(struct list_head *bridge_list)
 EXPORT_SYMBOL_GPL(fpga_bridges_put);
 
 /**
- * fpga_bridges_get_to_list - get a bridge, add it to a list
+ * of_fpga_bridge_get_to_list - get a bridge, add it to a list
  *
  * @np: node pointer of a FPGA bridge
  * @info: fpga image specific information
@@ -223,14 +256,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);
  *
  * Return 0 for success, error code from of_fpga_bridge_get() othewise.
  */
-int fpga_bridge_get_to_list(struct device_node *np,
+int of_fpga_bridge_get_to_list(struct device_node *np,
+			       struct fpga_image_info *info,
+			       struct list_head *bridge_list)
+{
+	struct fpga_bridge *bridge;
+	unsigned long flags;
+
+	bridge = of_fpga_bridge_get(np, info);
+	if (IS_ERR(bridge))
+		return PTR_ERR(bridge);
+
+	spin_lock_irqsave(&bridge_list_lock, flags);
+	list_add(&bridge->node, bridge_list);
+	spin_unlock_irqrestore(&bridge_list_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_fpga_bridge_get_to_list);
+
+/**
+ * fpga_bridge_get_to_list - given device, get a bridge, add it to a list
+ *
+ * @dev: FPGA bridge device
+ * @info: fpga image specific information
+ * @bridge_list: list of FPGA bridges
+ *
+ * Get an exclusive reference to the bridge and and it to the list.
+ *
+ * Return 0 for success, error code from fpga_bridge_get() othewise.
+ */
+int fpga_bridge_get_to_list(struct device *dev,
 			    struct fpga_image_info *info,
 			    struct list_head *bridge_list)
 {
 	struct fpga_bridge *bridge;
 	unsigned long flags;
 
-	bridge = of_fpga_bridge_get(np, info);
+	bridge = fpga_bridge_get(dev, info);
 	if (IS_ERR(bridge))
 		return PTR_ERR(bridge);
 
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 24f4ed5..294556e 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -183,8 +183,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,
-				      &region->bridge_list);
+	ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
+					 &region->bridge_list);
 	if (ret == -EBUSY)
 		return ret;
 
@@ -207,8 +207,8 @@ 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,
-					      &region->bridge_list);
+		ret = of_fpga_bridge_get_to_list(br, region->info,
+						 &region->bridge_list);
 
 		/* If any of the bridges are in use, give up */
 		if (ret == -EBUSY) {
diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
index dba6e3c..9f6696b 100644
--- a/include/linux/fpga/fpga-bridge.h
+++ b/include/linux/fpga/fpga-bridge.h
@@ -42,6 +42,8 @@ struct fpga_bridge {
 
 struct fpga_bridge *of_fpga_bridge_get(struct device_node *node,
 				       struct fpga_image_info *info);
+struct fpga_bridge *fpga_bridge_get(struct device *dev,
+				    struct fpga_image_info *info);
 void fpga_bridge_put(struct fpga_bridge *bridge);
 int fpga_bridge_enable(struct fpga_bridge *bridge);
 int fpga_bridge_disable(struct fpga_bridge *bridge);
@@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);
 int fpga_bridges_enable(struct list_head *bridge_list);
 int fpga_bridges_disable(struct list_head *bridge_list);
 void fpga_bridges_put(struct list_head *bridge_list);
-int fpga_bridge_get_to_list(struct device_node *np,
+int fpga_bridge_get_to_list(struct device *dev,
 			    struct fpga_image_info *info,
 			    struct list_head *bridge_list);
+int of_fpga_bridge_get_to_list(struct device_node *np,
+			       struct fpga_image_info *info,
+			       struct list_head *bridge_list);
 
 int fpga_bridge_register(struct device *dev, const char *name,
 			 const struct fpga_bridge_ops *br_ops, void *priv);
-- 
2.7.4

[PATCH 3/5] doc: fpga-mgr: separate getting/locking FPGA manager

[Alan Tull]

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>
---
v1: fixed some nits
---
 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..0240caf 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 a reference to a FPGA manager.  This 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

[PATCH 4/5] fpga-mgr: separate getting/locking FPGA manager

[Alan Tull]

Add fpga_mgr_lock/unlock functions that get a mutex for
exclusive use.

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

This makes it more straightforward 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>
---
 drivers/fpga/fpga-mgr.c       | 44 +++++++++++++++++++++++++++++++------------
 drivers/fpga/fpga-region.c    | 13 +++++++++++--
 include/linux/fpga/fpga-mgr.h |  3 +++
 3 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index fde605b..f7c3648 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -376,28 +376,19 @@ ATTRIBUTE_GROUPS(fpga_mgr);
 struct fpga_manager *__fpga_mgr_get(struct device *dev)
 {
 	struct fpga_manager *mgr;
-	int ret = -ENODEV;
 
 	mgr = to_fpga_manager(dev);
 	if (!mgr)
 		goto err_dev;
 
-	/* Get exclusive use of fpga manager */
-	if (!mutex_trylock(&mgr->ref_mutex)) {
-		ret = -EBUSY;
-		goto err_dev;
-	}
-
 	if (!try_module_get(dev->parent->driver->owner))
-		goto err_ll_mod;
+		goto err_dev;
 
 	return mgr;
 
-err_ll_mod:
-	mutex_unlock(&mgr->ref_mutex);
 err_dev:
 	put_device(dev);
-	return ERR_PTR(ret);
+	return ERR_PTR(-ENODEV);
 }
 
 static int fpga_mgr_dev_match(struct device *dev, const void *data)
@@ -457,12 +448,41 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
 void fpga_mgr_put(struct fpga_manager *mgr)
 {
 	module_put(mgr->dev.parent->driver->owner);
-	mutex_unlock(&mgr->ref_mutex);
 	put_device(&mgr->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
 /**
+ * fpga_mgr_lock - Lock FPGA manager for exclusive use
+ * @mgr:	fpga manager
+ *
+ * Given a pointer to FPGA Manager (from fpga_mgr_get() or
+ * of_fpga_mgr_put()) attempt to get the mutex.
+ *
+ * Return: 0 for success or -EBUSY
+ */
+int fpga_mgr_lock(struct fpga_manager *mgr)
+{
+	if (!mutex_trylock(&mgr->ref_mutex)) {
+		dev_err(&mgr->dev, "FPGA manager is in use.\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_lock);
+
+/**
+ * fpga_mgr_unlock - Unlock FPGA manager
+ * @mgr:	fpga manager
+ */
+void fpga_mgr_unlock(struct fpga_manager *mgr)
+{
+	mutex_unlock(&mgr->ref_mutex);
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
+
+/**
  * fpga_mgr_register - register a low level fpga manager driver
  * @dev:	fpga manager device from pdev
  * @name:	fpga manager name
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 294556e..815f178 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -125,7 +125,7 @@ static void fpga_region_put(struct fpga_region *region)
 }
 
 /**
- * fpga_region_get_manager - get exclusive reference for FPGA manager
+ * fpga_region_get_manager - get reference for FPGA manager
  * @region: FPGA region
  *
  * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
@@ -245,10 +245,16 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		return PTR_ERR(mgr);
 	}
 
+	ret = fpga_mgr_lock(mgr);
+	if (ret) {
+		pr_err("FPGA manager is busy\n");
+		goto err_put_mgr;
+	}
+
 	ret = fpga_region_get_bridges(region, overlay);
 	if (ret) {
 		pr_err("failed to get fpga region bridges\n");
-		goto err_put_mgr;
+		goto err_unlock_mgr;
 	}
 
 	ret = fpga_bridges_disable(&region->bridge_list);
@@ -269,6 +275,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_br;
 	}
 
+	fpga_mgr_unlock(mgr);
 	fpga_mgr_put(mgr);
 	fpga_region_put(region);
 
@@ -276,6 +283,8 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 
 err_put_br:
 	fpga_bridges_put(&region->bridge_list);
+err_unlock_mgr:
+	fpga_mgr_unlock(mgr);
 err_put_mgr:
 	fpga_mgr_put(mgr);
 	fpga_region_put(region);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 45df05a..ae970ca 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -149,6 +149,9 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 
 int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
 
+int fpga_mgr_lock(struct fpga_manager *mgr);
+void fpga_mgr_unlock(struct fpga_manager *mgr);
+
 struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
 
 struct fpga_manager *fpga_mgr_get(struct device *dev);
-- 
2.7.4

[PATCH 5/5] fpga-region: separate out common code from dt specific code

[Alan Tull]

FPGA region is a layer above the FPGA manager and FPGA bridge
frameworks.  Currently, FPGA region is dependent on device tree.
This commit separates the device tree specific code from the
common code, separating fpga-region.c into fpga-region.c,
of-fpga-region.c, and fpga-region.h.

Functons exported from fpga-region.c:
* 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.

* 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

Signed-off-by: Alan Tull <atull@kernel.org>
---
 drivers/fpga/Kconfig          |  12 +-
 drivers/fpga/Makefile         |   1 +
 drivers/fpga/fpga-region.c    | 475 +++++++-----------------------------------
 drivers/fpga/fpga-region.h    |  50 +++++
 drivers/fpga/of-fpga-region.c | 453 ++++++++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-mgr.h |   6 +-
 6 files changed, 599 insertions(+), 398 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 815f178..c06f2f7 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -16,53 +16,64 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+/* todo: prevent programming if region has child regions or overlay applied */
+
 #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"
 
-/**
- * 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
- * @info: fpga image specific information
- */
-struct fpga_region {
-	struct device dev;
-	struct mutex mutex; /* for exclusive reference to region */
-	struct list_head bridge_list;
+static DEFINE_IDA(fpga_region_ida);
+struct class *fpga_region_class;
+
+struct fpga_image_info *fpga_region_alloc_image_info(struct fpga_region *region)
+{
+	struct device *dev = &region->dev;
 	struct fpga_image_info *info;
-};
 
-#define to_fpga_region(d) container_of(d, struct fpga_region, dev)
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
 
-static DEFINE_IDA(fpga_region_ida);
-static struct class *fpga_region_class;
+	return info;
+}
+EXPORT_SYMBOL_GPL(fpga_region_alloc_image_info);
 
-static const struct of_device_id fpga_region_of_match[] = {
-	{ .compatible = "fpga-region", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, fpga_region_of_match);
+void fpga_region_free_image_info(struct fpga_region *region,
+				 struct fpga_image_info *info)
+{
+	struct device *dev = &region->dev;
+
+	if (!info)
+		return;
+
+	if (info->firmware_name)
+		devm_kfree(dev, info->firmware_name);
 
+	devm_kfree(dev, 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;
 
@@ -73,6 +84,9 @@ static struct fpga_region *fpga_region_find(struct device_node *np)
 
 	return to_fpga_region(dev);
 }
+EXPORT_SYMBOL_GPL(of_fpga_region_find);
+
+#endif /* CONFIG_OF_FPGA_REGION */
 
 /**
  * fpga_region_get - get an exclusive reference to a fpga region
@@ -94,9 +108,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);
@@ -119,397 +131,103 @@ 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
- * @overlay: device node of the overlay
- *
- * 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 device_node *overlay)
-{
-	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, region->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(overlay, "fpga-bridges", 0))
-		np = 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, region->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
  * @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.
  */
-static int fpga_region_program_fpga(struct fpga_region *region,
-				    struct device_node *overlay)
+int fpga_region_program_fpga(struct fpga_region *region,
+			     struct fpga_image_info *info)
 {
-	struct fpga_manager *mgr;
+	struct device *dev = &region->dev;
 	int ret;
 
-	region = fpga_region_get(region);
-	if (IS_ERR(region)) {
-		pr_err("failed to get fpga region\n");
+	if (region->info) {
+		dev_err(dev, "region already programmed\n");
 		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);
+	region = fpga_region_get(region);
+	if (IS_ERR(region)) {
+		dev_err(dev, "failed to get fpga region\n");
+		return PTR_ERR(region);
 	}
 
-	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) {
+		dev_err(dev, "fpga manager is busy\n");
+		goto err_put_region;
 	}
 
-	ret = fpga_region_get_bridges(region, overlay);
-	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, info);
+		if (ret) {
+			dev_err(dev, "failed to get fpga region bridges\n");
+			goto err_unlock_mgr;
+		}
 	}
 
 	ret = fpga_bridges_disable(&region->bridge_list);
 	if (ret) {
-		pr_err("failed to disable region bridges\n");
+		dev_err(dev, "failed to disable region bridges\n");
 		goto err_put_br;
 	}
 
-	ret = fpga_mgr_load(mgr, region->info);
+	ret = fpga_mgr_load(region->mgr, info);
 	if (ret) {
-		pr_err("failed to load fpga image\n");
+		dev_err(dev, "failed to load fpga image\n");
 		goto err_put_br;
 	}
 
 	ret = fpga_bridges_enable(&region->bridge_list);
 	if (ret) {
-		pr_err("failed to enable region bridges\n");
+		dev_err(dev, "failed to enable region bridges\n");
 		goto err_put_br;
 	}
 
-	fpga_mgr_unlock(mgr);
-	fpga_mgr_put(mgr);
+	region->info = 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_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.
- *
- *   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.
- *
- * If the overlay that breaks the rules, notifier returns an error and the
- * overlay is rejected before it goes into the main tree.
- *
- * Returns 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 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);
-	if (ret)
-		return ret;
-
-	/* Read FPGA region properties from the overlay */
-	if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
-		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
-
-	if (of_property_read_bool(nd->overlay, "external-fpga-config"))
-		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
-
-	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);
-
-	of_property_read_u32(nd->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;
-	}
-
-	/* FPGA is already configured externally.  We're done. */
-	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG)
-		return 0;
-
-	/* 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;
-	}
-
-	return fpga_region_program_fpga(region, nd->overlay);
-}
-
-/**
- * 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)
-{
-	fpga_bridges_disable(&region->bridge_list);
-	fpga_bridges_put(&region->bridge_list);
-	devm_kfree(&region->dev, region->info);
-	region->info = NULL;
-}
-
-/**
- * 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);
-
 	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);
 
@@ -521,82 +239,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..472f2f9
--- /dev/null
+++ b/drivers/fpga/fpga-region.h
@@ -0,0 +1,50 @@
+#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 *info;
+	void *priv;
+	int (*get_bridges)(struct fpga_region *region,
+			   struct fpga_image_info *image_info);
+};
+
+#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..7809036
--- /dev/null
+++ b/drivers/fpga/of-fpga-region.c
@@ -0,0 +1,453 @@
+/*
+ * 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"
+
+/**
+ * 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_kstrdup(dev, firmware_name,
+						   GFP_KERNEL);
+		if (!info->firmware_name)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	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;
+	}
+
+	return info;
+ret_no_info:
+	fpga_region_free_image_info(region, info);
+	return ERR_PTR(ret);
+}
+
+/**
+ * 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;
+
+	if (region->info) {
+		dev_err(&region->dev, "Region already has overlay applied.\n");
+		return -EINVAL;
+	}
+
+	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;
+	}
+
+	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)
+{
+	fpga_bridges_disable(&region->bridge_list);
+	fpga_bridges_put(&region->bridge_list);
+	fpga_region_free_image_info(region, region->info);
+	region->info = NULL;
+}
+
+/**
+ * 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;
+
+	/* 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

Re: [PATCH 0/5] fpga: enhancements to support non-dt code

[Alan Tull]

On Mon, Mar 13, 2017 at 4:53 PM, Alan Tull <atull@kernel.org> wrote:
> Set of patches that supports writing code that uses
> FPGA regions without Device Tree overlays.

OK here's where I try to clarify my intention for this code.  And
beg for reviews :)

This patchset is intended as a stepping stone towards supporting
PCIe FPGAs by allowing fpga regions to be created and used in
without device tree overlays.  Without it, I can see a lot of
code coming down the line where people have to reinvent things that
aren't called fpga-regions but ultimately are doing the same thing.

The idea is that if someone has some whizzbang scheme,
that somehow knows "Oh I have a FPGA here and some bridges
and I want to program it without getting a kernel oops", then that
code can use fpga_region_register() to create an fpga region device and
tell it what fpga manager to use and give it a method for getting
bridges.  There is an example of usage in of_fpga_region_probe().
Then fpga_region_program_fpga() is available for programming
the fpga, including handling enabling/disabling the bridges.

Alan

>
> Main differences from the RFC:
> * dropping the sysfs interface
> * dropping support for more than one DT overlay on a region
>
> Alan Tull (5):
>   fpga-mgr: pass parameters for loading fpga in image info
>   fpga-bridge: support getting bridge from device
>   doc: fpga-mgr: separate getting/locking FPGA manager
>   fpga-mgr: separate getting/locking FPGA manager
>   fpga-region: separate out common code from dt specific code
>
>  Documentation/fpga/fpga-mgr.txt  |  19 +-
>  drivers/fpga/Kconfig             |  12 +-
>  drivers/fpga/Makefile            |   1 +
>  drivers/fpga/fpga-bridge.c       | 107 +++++++--
>  drivers/fpga/fpga-mgr.c          |  56 ++++-
>  drivers/fpga/fpga-region.c       | 473 +++++++--------------------------------
>  drivers/fpga/fpga-region.h       |  50 +++++
>  drivers/fpga/of-fpga-region.c    | 453 +++++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-bridge.h |   7 +-
>  include/linux/fpga/fpga-mgr.h    |  17 ++
>  10 files changed, 766 insertions(+), 429 deletions(-)
>  create mode 100644 drivers/fpga/fpga-region.h
>  create mode 100644 drivers/fpga/of-fpga-region.c
>
> --
> 2.7.4
>

Re: [PATCH 0/5] fpga: enhancements to support non-dt code

[Moritz Fischer]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

On Fri, Mar 24, 2017 at 01:09:22PM -0500, Alan Tull wrote:
> On Mon, Mar 13, 2017 at 4:53 PM, Alan Tull <atull@kernel.org> wrote:
> > Set of patches that supports writing code that uses
> > FPGA regions without Device Tree overlays.
> 
> OK here's where I try to clarify my intention for this code.  And
> beg for reviews :)

It's on the todo list ;-)

Cheers,

Moritz

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH 0/5] fpga: enhancements to support non-dt code

[Alan Tull]

On Fri, Mar 24, 2017 at 2:11 PM, Moritz Fischer
<moritz.fischer.private@gmail.com> wrote:
> On Fri, Mar 24, 2017 at 01:09:22PM -0500, Alan Tull wrote:
>> On Mon, Mar 13, 2017 at 4:53 PM, Alan Tull <atull@kernel.org> wrote:
>> > Set of patches that supports writing code that uses
>> > FPGA regions without Device Tree overlays.
>>
>> OK here's where I try to clarify my intention for this code.  And
>> beg for reviews :)
>
> It's on the todo list ;-)

Thanks!  I could post a branch on linux-fpga git if that's helpful.

Most of the patches are small.  The last patch is large as it is moving
code from fpga-region.c to a new header and new of-fpga-region.c.
I think this patchset will be useful for the current 'Intel FPGA Driver'
patchset.

Alan

>
> Cheers,
>
> Moritz

Re: [PATCH 4/5] fpga-mgr: separate getting/locking FPGA manager

[Moritz Fischer]

Hi Alan,

minor nits, inline

On Mon, Mar 13, 2017 at 04:53:32PM -0500, Alan Tull wrote:
> Add fpga_mgr_lock/unlock functions that get a mutex for
> exclusive use.
> 
> of_fpga_mgr_get, fpga_mgr_get, and fpga_mgr_put no longer lock
> the FPGA manager mutex.
> 
> This makes it more straightforward 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>
> ---
>  drivers/fpga/fpga-mgr.c       | 44 +++++++++++++++++++++++++++++++------------
>  drivers/fpga/fpga-region.c    | 13 +++++++++++--
>  include/linux/fpga/fpga-mgr.h |  3 +++
>  3 files changed, 46 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index fde605b..f7c3648 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -376,28 +376,19 @@ ATTRIBUTE_GROUPS(fpga_mgr);
>  struct fpga_manager *__fpga_mgr_get(struct device *dev)
>  {
>  	struct fpga_manager *mgr;
> -	int ret = -ENODEV;
>  
>  	mgr = to_fpga_manager(dev);
>  	if (!mgr)
>  		goto err_dev;
>  
> -	/* Get exclusive use of fpga manager */
> -	if (!mutex_trylock(&mgr->ref_mutex)) {
> -		ret = -EBUSY;
> -		goto err_dev;
> -	}
> -
>  	if (!try_module_get(dev->parent->driver->owner))
> -		goto err_ll_mod;
> +		goto err_dev;
>  
>  	return mgr;
>  
> -err_ll_mod:
> -	mutex_unlock(&mgr->ref_mutex);
>  err_dev:
>  	put_device(dev);
> -	return ERR_PTR(ret);
> +	return ERR_PTR(-ENODEV);
>  }
>  
>  static int fpga_mgr_dev_match(struct device *dev, const void *data)
> @@ -457,12 +448,41 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
>  void fpga_mgr_put(struct fpga_manager *mgr)
>  {
>  	module_put(mgr->dev.parent->driver->owner);
> -	mutex_unlock(&mgr->ref_mutex);
>  	put_device(&mgr->dev);
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_put);
>  
>  /**
> + * fpga_mgr_lock - Lock FPGA manager for exclusive use
> + * @mgr:	fpga manager
> + *
> + * Given a pointer to FPGA Manager (from fpga_mgr_get() or
> + * of_fpga_mgr_put()) attempt to get the mutex.
> + *
> + * Return: 0 for success or -EBUSY
> + */
> +int fpga_mgr_lock(struct fpga_manager *mgr)
> +{
> +	if (!mutex_trylock(&mgr->ref_mutex)) {
> +		dev_err(&mgr->dev, "FPGA manager is in use.\n");
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_lock);
> +
> +/**
> + * fpga_mgr_unlock - Unlock FPGA manager
> + * @mgr:	fpga manager
> + */
> +void fpga_mgr_unlock(struct fpga_manager *mgr)
> +{
> +	mutex_unlock(&mgr->ref_mutex);
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
> +
> +/**
>   * fpga_mgr_register - register a low level fpga manager driver
>   * @dev:	fpga manager device from pdev
>   * @name:	fpga manager name
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index 294556e..815f178 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -125,7 +125,7 @@ static void fpga_region_put(struct fpga_region *region)
>  }
>  
>  /**
> - * fpga_region_get_manager - get exclusive reference for FPGA manager
> + * fpga_region_get_manager - get reference for FPGA manager
>   * @region: FPGA region
>   *
>   * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
> @@ -245,10 +245,16 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>  		return PTR_ERR(mgr);
>  	}
>  
> +	ret = fpga_mgr_lock(mgr);
> +	if (ret) {
> +		pr_err("FPGA manager is busy\n");

Am I missing something here, or could you use dev_err(&mgr->dev, ...)
here?

> +		goto err_put_mgr;
> +	}
> +
>  	ret = fpga_region_get_bridges(region, overlay);
>  	if (ret) {
>  		pr_err("failed to get fpga region bridges\n");

Same here, (I know this is not part of this patch), maybe the above is
for consistency reasons then. Maybe I'm missing something.
> -		goto err_put_mgr;
> +		goto err_unlock_mgr;
>  	}
>  
>  	ret = fpga_bridges_disable(&region->bridge_list);
> @@ -269,6 +275,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>  		goto err_put_br;
>  	}
>  
> +	fpga_mgr_unlock(mgr);
>  	fpga_mgr_put(mgr);
>  	fpga_region_put(region);
>  
> @@ -276,6 +283,8 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>  
>  err_put_br:
>  	fpga_bridges_put(&region->bridge_list);
> +err_unlock_mgr:
> +	fpga_mgr_unlock(mgr);
>  err_put_mgr:
>  	fpga_mgr_put(mgr);
>  	fpga_region_put(region);
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 45df05a..ae970ca 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -149,6 +149,9 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>  
>  int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
>  
> +int fpga_mgr_lock(struct fpga_manager *mgr);
> +void fpga_mgr_unlock(struct fpga_manager *mgr);
> +
>  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

Cheers,

Moritz

Re: [PATCH 5/5] fpga-region: separate out common code from dt specific code

[Moritz Fischer]

Hi Alan,

first pass ... need to get back to it.

On Mon, Mar 13, 2017 at 04:53:33PM -0500, Alan Tull wrote:
> FPGA region is a layer above the FPGA manager and FPGA bridge
> frameworks.  Currently, FPGA region is dependent on device tree.
> This commit separates the device tree specific code from the
> common code, separating fpga-region.c into fpga-region.c,
> of-fpga-region.c, and fpga-region.h.
> 
> Functons exported from fpga-region.c:
> * 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.
> 
> * 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
> 
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
>  drivers/fpga/Kconfig          |  12 +-
>  drivers/fpga/Makefile         |   1 +
>  drivers/fpga/fpga-region.c    | 475 +++++++-----------------------------------
>  drivers/fpga/fpga-region.h    |  50 +++++
>  drivers/fpga/of-fpga-region.c | 453 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h |   6 +-
>  6 files changed, 599 insertions(+), 398 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

Doesn't this one now need depends on FPGA_REGION & OF ? Since
FPGA_REGION no longer depends on OF, or does FPGA_BRIDGE pull it in?

>  	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 815f178..c06f2f7 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -16,53 +16,64 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +/* todo: prevent programming if region has child regions or overlay applied */
> +
>  #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"
>  
> -/**
> - * 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
> - * @info: fpga image specific information
> - */
> -struct fpga_region {
> -	struct device dev;
> -	struct mutex mutex; /* for exclusive reference to region */
> -	struct list_head bridge_list;
> +static DEFINE_IDA(fpga_region_ida);
> +struct class *fpga_region_class;
> +
> +struct fpga_image_info *fpga_region_alloc_image_info(struct fpga_region *region)
> +{
> +	struct device *dev = &region->dev;
>  	struct fpga_image_info *info;
> -};
>  
> -#define to_fpga_region(d) container_of(d, struct fpga_region, dev)
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return ERR_PTR(-ENOMEM);
>  
> -static DEFINE_IDA(fpga_region_ida);
> -static struct class *fpga_region_class;
> +	return info;
> +}
> +EXPORT_SYMBOL_GPL(fpga_region_alloc_image_info);
>  
> -static const struct of_device_id fpga_region_of_match[] = {
> -	{ .compatible = "fpga-region", },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, fpga_region_of_match);
> +void fpga_region_free_image_info(struct fpga_region *region,
> +				 struct fpga_image_info *info)
> +{
> +	struct device *dev = &region->dev;
> +
> +	if (!info)
> +		return;
> +
> +	if (info->firmware_name)
> +		devm_kfree(dev, info->firmware_name);
>  
> +	devm_kfree(dev, 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;
>  
> @@ -73,6 +84,9 @@ static struct fpga_region *fpga_region_find(struct device_node *np)
>  
>  	return to_fpga_region(dev);
>  }
> +EXPORT_SYMBOL_GPL(of_fpga_region_find);
> +
> +#endif /* CONFIG_OF_FPGA_REGION */
>  
>  /**
>   * fpga_region_get - get an exclusive reference to a fpga region
> @@ -94,9 +108,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);
> @@ -119,397 +131,103 @@ 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
> - * @overlay: device node of the overlay
> - *
> - * 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 device_node *overlay)
> -{
> -	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, region->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(overlay, "fpga-bridges", 0))
> -		np = 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, region->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
>   * @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.
>   */
> -static int fpga_region_program_fpga(struct fpga_region *region,
> -				    struct device_node *overlay)
> +int fpga_region_program_fpga(struct fpga_region *region,
> +			     struct fpga_image_info *info)
>  {
> -	struct fpga_manager *mgr;
> +	struct device *dev = &region->dev;
>  	int ret;
>  
> -	region = fpga_region_get(region);
> -	if (IS_ERR(region)) {
> -		pr_err("failed to get fpga region\n");
> +	if (region->info) {
> +		dev_err(dev, "region already programmed\n");
>  		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);
> +	region = fpga_region_get(region);
> +	if (IS_ERR(region)) {
> +		dev_err(dev, "failed to get fpga region\n");
> +		return PTR_ERR(region);
>  	}
>  
> -	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) {
> +		dev_err(dev, "fpga manager is busy\n");
> +		goto err_put_region;
>  	}
>  
> -	ret = fpga_region_get_bridges(region, overlay);
> -	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, info);
> +		if (ret) {
> +			dev_err(dev, "failed to get fpga region bridges\n");
> +			goto err_unlock_mgr;
> +		}
>  	}
>  
>  	ret = fpga_bridges_disable(&region->bridge_list);
>  	if (ret) {
> -		pr_err("failed to disable region bridges\n");
> +		dev_err(dev, "failed to disable region bridges\n");
>  		goto err_put_br;
>  	}
>  
> -	ret = fpga_mgr_load(mgr, region->info);
> +	ret = fpga_mgr_load(region->mgr, info);
>  	if (ret) {
> -		pr_err("failed to load fpga image\n");
> +		dev_err(dev, "failed to load fpga image\n");
>  		goto err_put_br;
>  	}
>  
>  	ret = fpga_bridges_enable(&region->bridge_list);
>  	if (ret) {
> -		pr_err("failed to enable region bridges\n");
> +		dev_err(dev, "failed to enable region bridges\n");
>  		goto err_put_br;
>  	}
>  
> -	fpga_mgr_unlock(mgr);
> -	fpga_mgr_put(mgr);
> +	region->info = 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_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.
> - *
> - *   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.
> - *
> - * If the overlay that breaks the rules, notifier returns an error and the
> - * overlay is rejected before it goes into the main tree.
> - *
> - * Returns 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 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);
> -	if (ret)
> -		return ret;
> -
> -	/* Read FPGA region properties from the overlay */
> -	if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
> -		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
> -
> -	if (of_property_read_bool(nd->overlay, "external-fpga-config"))
> -		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
> -
> -	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);
> -
> -	of_property_read_u32(nd->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;
> -	}
> -
> -	/* FPGA is already configured externally.  We're done. */
> -	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG)
> -		return 0;
> -
> -	/* 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;
> -	}
> -
> -	return fpga_region_program_fpga(region, nd->overlay);
> -}
> -
> -/**
> - * 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)
> -{
> -	fpga_bridges_disable(&region->bridge_list);
> -	fpga_bridges_put(&region->bridge_list);
> -	devm_kfree(&region->dev, region->info);
> -	region->info = NULL;
> -}
> -
> -/**
> - * 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);
> -
>  	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);
>  
> @@ -521,82 +239,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..472f2f9
> --- /dev/null
> +++ b/drivers/fpga/fpga-region.h
> @@ -0,0 +1,50 @@
> +#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 *info;
> +	void *priv;
> +	int (*get_bridges)(struct fpga_region *region,
> +			   struct fpga_image_info *image_info);
> +};
> +
> +#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..7809036
> --- /dev/null
> +++ b/drivers/fpga/of-fpga-region.c
> @@ -0,0 +1,453 @@
> +/*
> + * 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"
> +
> +/**
> + * 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;

Would a if (ret) do here? Otherwise what happens on if (ret)?
If you can replace the above if (ret == ...) the if (!ret) can go away.
> +
> +	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_kstrdup(dev, firmware_name,
> +						   GFP_KERNEL);
> +		if (!info->firmware_name)
> +			return ERR_PTR(-ENOMEM);
> +	}
> +
> +	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;
> +	}
> +
> +	return info;
> +ret_no_info:
> +	fpga_region_free_image_info(region, info);
> +	return ERR_PTR(ret);
> +}
> +
> +/**
> + * 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;
> +
> +	if (region->info) {
> +		dev_err(&region->dev, "Region already has overlay applied.\n");
> +		return -EINVAL;
> +	}
> +
> +	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;
> +	}
> +
> +	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)
> +{
> +	fpga_bridges_disable(&region->bridge_list);
> +	fpga_bridges_put(&region->bridge_list);
> +	fpga_region_free_image_info(region, region->info);
> +	region->info = NULL;
> +}
> +
> +/**
> + * 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;
> +
> +	/* 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
> 
> --
> 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

Cheers,

Moritz

Re: [PATCH 4/5] fpga-mgr: separate getting/locking FPGA manager

[Alan Tull]

On Wed, Apr 5, 2017 at 11:07 PM, Moritz Fischer <mdf@kernel.org> wrote:
> Hi Alan,
>
> minor nits, inline
>
> On Mon, Mar 13, 2017 at 04:53:32PM -0500, Alan Tull wrote:
>> Add fpga_mgr_lock/unlock functions that get a mutex for
>> exclusive use.
>>
>> of_fpga_mgr_get, fpga_mgr_get, and fpga_mgr_put no longer lock
>> the FPGA manager mutex.
>>
>> This makes it more straightforward 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>
>> ---
>>  drivers/fpga/fpga-mgr.c       | 44 +++++++++++++++++++++++++++++++------------
>>  drivers/fpga/fpga-region.c    | 13 +++++++++++--
>>  include/linux/fpga/fpga-mgr.h |  3 +++
>>  3 files changed, 46 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index fde605b..f7c3648 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -376,28 +376,19 @@ ATTRIBUTE_GROUPS(fpga_mgr);
>>  struct fpga_manager *__fpga_mgr_get(struct device *dev)
>>  {
>>       struct fpga_manager *mgr;
>> -     int ret = -ENODEV;
>>
>>       mgr = to_fpga_manager(dev);
>>       if (!mgr)
>>               goto err_dev;
>>
>> -     /* Get exclusive use of fpga manager */
>> -     if (!mutex_trylock(&mgr->ref_mutex)) {
>> -             ret = -EBUSY;
>> -             goto err_dev;
>> -     }
>> -
>>       if (!try_module_get(dev->parent->driver->owner))
>> -             goto err_ll_mod;
>> +             goto err_dev;
>>
>>       return mgr;
>>
>> -err_ll_mod:
>> -     mutex_unlock(&mgr->ref_mutex);
>>  err_dev:
>>       put_device(dev);
>> -     return ERR_PTR(ret);
>> +     return ERR_PTR(-ENODEV);
>>  }
>>
>>  static int fpga_mgr_dev_match(struct device *dev, const void *data)
>> @@ -457,12 +448,41 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
>>  void fpga_mgr_put(struct fpga_manager *mgr)
>>  {
>>       module_put(mgr->dev.parent->driver->owner);
>> -     mutex_unlock(&mgr->ref_mutex);
>>       put_device(&mgr->dev);
>>  }
>>  EXPORT_SYMBOL_GPL(fpga_mgr_put);
>>
>>  /**
>> + * fpga_mgr_lock - Lock FPGA manager for exclusive use
>> + * @mgr:     fpga manager
>> + *
>> + * Given a pointer to FPGA Manager (from fpga_mgr_get() or
>> + * of_fpga_mgr_put()) attempt to get the mutex.
>> + *
>> + * Return: 0 for success or -EBUSY
>> + */
>> +int fpga_mgr_lock(struct fpga_manager *mgr)
>> +{
>> +     if (!mutex_trylock(&mgr->ref_mutex)) {
>> +             dev_err(&mgr->dev, "FPGA manager is in use.\n");
>> +             return -EBUSY;
>> +     }
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_mgr_lock);
>> +
>> +/**
>> + * fpga_mgr_unlock - Unlock FPGA manager
>> + * @mgr:     fpga manager
>> + */
>> +void fpga_mgr_unlock(struct fpga_manager *mgr)
>> +{
>> +     mutex_unlock(&mgr->ref_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
>> +
>> +/**
>>   * fpga_mgr_register - register a low level fpga manager driver
>>   * @dev:     fpga manager device from pdev
>>   * @name:    fpga manager name
>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index 294556e..815f178 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -125,7 +125,7 @@ static void fpga_region_put(struct fpga_region *region)
>>  }
>>
>>  /**
>> - * fpga_region_get_manager - get exclusive reference for FPGA manager
>> + * fpga_region_get_manager - get reference for FPGA manager
>>   * @region: FPGA region
>>   *
>>   * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
>> @@ -245,10 +245,16 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>>               return PTR_ERR(mgr);
>>       }
>>
>> +     ret = fpga_mgr_lock(mgr);
>> +     if (ret) {
>> +             pr_err("FPGA manager is busy\n");
>
> Am I missing something here, or could you use dev_err(&mgr->dev, ...)
> here?

Hi Moritz,

I agree, I can go though and look for pr_* print messages and
change them to dev_* in functions where I have the device.

Thanks!
Alan

>
>> +             goto err_put_mgr;
>> +     }
>> +
>>       ret = fpga_region_get_bridges(region, overlay);
>>       if (ret) {
>>               pr_err("failed to get fpga region bridges\n");
>
> Same here, (I know this is not part of this patch), maybe the above is
> for consistency reasons then. Maybe I'm missing something.
>> -             goto err_put_mgr;
>> +             goto err_unlock_mgr;
>>       }
>>
>>       ret = fpga_bridges_disable(&region->bridge_list);
>> @@ -269,6 +275,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>>               goto err_put_br;
>>       }
>>
>> +     fpga_mgr_unlock(mgr);
>>       fpga_mgr_put(mgr);
>>       fpga_region_put(region);
>>
>> @@ -276,6 +283,8 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>>
>>  err_put_br:
>>       fpga_bridges_put(&region->bridge_list);
>> +err_unlock_mgr:
>> +     fpga_mgr_unlock(mgr);
>>  err_put_mgr:
>>       fpga_mgr_put(mgr);
>>       fpga_region_put(region);
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index 45df05a..ae970ca 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -149,6 +149,9 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>>
>>  int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
>>
>> +int fpga_mgr_lock(struct fpga_manager *mgr);
>> +void fpga_mgr_unlock(struct fpga_manager *mgr);
>> +
>>  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
>
> Cheers,
>
> Moritz

Re: [PATCH 5/5] fpga-region: separate out common code from dt specific code

[Alan Tull]

On Wed, Apr 5, 2017 at 11:25 PM, Moritz Fischer <mdf@kernel.org> wrote:
Hi Moritz,

> Hi Alan,
>
> first pass ... need to get back to it.

Thanks for reviewing!

>
> On Mon, Mar 13, 2017 at 04:53:33PM -0500, Alan Tull wrote:
>> FPGA region is a layer above the FPGA manager and FPGA bridge
>> frameworks.  Currently, FPGA region is dependent on device tree.
>> This commit separates the device tree specific code from the
>> common code, separating fpga-region.c into fpga-region.c,
>> of-fpga-region.c, and fpga-region.h.
>>
>> Functons exported from fpga-region.c:
>> * 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.
>>
>> * 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
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
>> ---
>>  drivers/fpga/Kconfig          |  12 +-
>>  drivers/fpga/Makefile         |   1 +
>>  drivers/fpga/fpga-region.c    | 475 +++++++-----------------------------------
>>  drivers/fpga/fpga-region.h    |  50 +++++
>>  drivers/fpga/of-fpga-region.c | 453 ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/fpga/fpga-mgr.h |   6 +-
>>  6 files changed, 599 insertions(+), 398 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
>
> Doesn't this one now need depends on FPGA_REGION & OF ? Since
> FPGA_REGION no longer depends on OF, or does FPGA_BRIDGE pull it in?

Yes.  I will revisit this.

>> +
>> +/**
>> + * 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;
>
> Would a if (ret) do here? Otherwise what happens on if (ret)?
> If you can replace the above if (ret == ...) the if (!ret) can go away.

I could have explained more clearly what this is doing.
of_fpga_bridge_get_to_list returns -ENODEV if the parent is not
a bridge or -EBUSY if it is a bridge that has already been gotten.
If the parent isn't a bridge, that's valid, this function should
continue and add bridges.

This function isn't new code and most of this patch isn't new.  It's
mostly moving code between files and doing some changes to
make that separation possible.  I've tried to make it easier by
breaking some of the functional changes to the previous smaller
patches.  Still thinking about how this could be made to be less
painful for reviewing.  I was thinking breaking this patch down further
by having one patch that does all the functional changes while
leaving the code in place in fpga-region.c.    That way the
functional changes will be clear (add a line/delete a line).  Then
another separate patch that actually creates the new
fpga-region.h and of-fpga-region.c files, moving stuff out of
fpga-region.c to them.

Alan

>> +
>> +     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;
>> +}