All of lore.kernel.org
 help / color / mirror / Atom feed
* FPGA Region enhancements and fixes
@ 2017-02-15 16:14 Alan Tull
  2017-02-15 16:14 ` [RFC 1/8] fpga-mgr: add a single function for fpga loading methods Alan Tull
                   ` (8 more replies)
  0 siblings, 9 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-15 16:14 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

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

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

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

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

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

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

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

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

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

Alan

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

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

-- 
2.7.4

^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC 1/8] fpga-mgr: add a single function for fpga loading methods
  2017-02-15 16:14 FPGA Region enhancements and fixes Alan Tull
@ 2017-02-15 16:14 ` Alan Tull
  2017-02-16  0:36   ` matthew.gerlach
  2017-02-15 16:14 ` [RFC 2/8] fpga-region: support more than one overlay per FPGA region Alan Tull
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-15 16:14 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

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

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

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

^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [RFC 2/8] fpga-region: support more than one overlay per FPGA region
  2017-02-15 16:14 FPGA Region enhancements and fixes Alan Tull
  2017-02-15 16:14 ` [RFC 1/8] fpga-mgr: add a single function for fpga loading methods Alan Tull
@ 2017-02-15 16:14 ` Alan Tull
  2017-02-16 16:50   ` matthew.gerlach
  2017-02-15 16:14 ` [RFC 3/8] fpga-bridge: add non-dt support Alan Tull
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-15 16:14 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

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

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

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

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

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

^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [RFC 3/8] fpga-bridge: add non-dt support
  2017-02-15 16:14 FPGA Region enhancements and fixes Alan Tull
  2017-02-15 16:14 ` [RFC 1/8] fpga-mgr: add a single function for fpga loading methods Alan Tull
  2017-02-15 16:14 ` [RFC 2/8] fpga-region: support more than one overlay per FPGA region Alan Tull
@ 2017-02-15 16:14 ` Alan Tull
  2017-02-15 16:14 ` [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager Alan Tull
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-15 16:14 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

Add functions that will support using FPGA bridges without
device tree.

* fpga_bridge_get
  Get the bridge given the device.

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

* fpga_bridges_get_to_list
  Given the device, 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       |  10 ++--
 include/linux/fpga/fpga-bridge.h |   7 ++-
 3 files changed, 96 insertions(+), 28 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 60d2947..3a36417 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -200,9 +200,9 @@ 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,
-				      reg_ovl->image_info,
-				      &region->bridge_list);
+	ret = of_fpga_bridge_get_to_list(region_np->parent,
+					 reg_ovl->image_info,
+					 &region->bridge_list);
 	if (ret == -EBUSY)
 		return ret;
 
@@ -225,8 +225,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, reg_ovl->image_info,
-					      &region->bridge_list);
+		ret = of_fpga_bridge_get_to_list(br, reg_ovl->image_info,
+						 &region->bridge_list);
 
 		/* If any of the bridges are in use, give up */
 		if (ret == -EBUSY) {
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

^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager
  2017-02-15 16:14 FPGA Region enhancements and fixes Alan Tull
                   ` (2 preceding siblings ...)
  2017-02-15 16:14 ` [RFC 3/8] fpga-bridge: add non-dt support Alan Tull
@ 2017-02-15 16:14 ` Alan Tull
  2017-02-17 17:14   ` Li, Yi
  2017-02-17 17:52   ` Moritz Fischer
  2017-02-15 16:14 ` [RFC 5/8] " Alan Tull
                   ` (4 subsequent siblings)
  8 siblings, 2 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-15 16:14 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

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

fpga_mgr_lock/unlock functions get/release mutex.

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

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

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

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

^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [RFC 5/8] fpga-mgr: separate getting/locking FPGA manager
  2017-02-15 16:14 FPGA Region enhancements and fixes Alan Tull
                   ` (3 preceding siblings ...)
  2017-02-15 16:14 ` [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager Alan Tull
@ 2017-02-15 16:14 ` Alan Tull
  2017-02-15 16:14 ` [RFC 6/8] fpga-region: separate out common code to allow non-dt support Alan Tull
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-15 16:14 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

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 b7c719a..3206a53 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 3a36417..a5d8112 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -142,7 +142,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.
@@ -265,10 +265,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, reg_ovl);
 	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);
@@ -289,6 +295,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);
 
@@ -296,6 +303,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

^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [RFC 6/8] fpga-region: separate out common code to allow non-dt support
  2017-02-15 16:14 FPGA Region enhancements and fixes Alan Tull
                   ` (4 preceding siblings ...)
  2017-02-15 16:14 ` [RFC 5/8] " Alan Tull
@ 2017-02-15 16:14 ` Alan Tull
  2017-02-15 16:14 ` [RFC 7/8] fpga-region: add sysfs interface Alan Tull
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-15 16:14 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

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

Functons exported:

of_fpga_region_find
* Find a fpga region, given the node pointer

fpga_region_alloc_image_info
fpga_region_free_image_info
* Alloc/free fpga_image_info struct

fpga_region_program_fpga
* Program an FPGA region

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

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

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

^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 16:14 FPGA Region enhancements and fixes Alan Tull
                   ` (5 preceding siblings ...)
  2017-02-15 16:14 ` [RFC 6/8] fpga-region: separate out common code to allow non-dt support Alan Tull
@ 2017-02-15 16:14 ` Alan Tull
  2017-02-15 17:21   ` Jason Gunthorpe
  2017-02-15 16:14 ` [RFC 8/8] doc: fpga: add sysfs document for fpga region Alan Tull
  2017-02-28 17:35 ` FPGA Region enhancements and fixes Alan Tull
  8 siblings, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-15 16:14 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

Add a sysfs interface to control programming FPGA.

Each fpga-region will get the following files which set values
in the fpga_image_info struct for that region.  More files will
need to be added as fpga_image_info expands.

firmware_name
* writing a name of a FPGA image file to firmware_name causes the
  FPGA region to write the FPGA

partial_config
* 0 : full reconfiguration
* 1 : partial reconfiguration

unfreeze_timeout
* Timeout for waiting for a freeze bridge to enable traffic

freeze_timeout
* Timeout for waiting for a freeze bridge to disable traffic

Signed-off-by: Alan Tull <atull@kernel.org>
---
 drivers/fpga/Kconfig       |   8 ++
 drivers/fpga/fpga-region.c | 241 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 249 insertions(+)

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index be9c23d..6455e02 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -21,6 +21,14 @@ config FPGA_REGION
 	  and the FPGA Bridges associated with either a reconfigurable
 	  region of an FPGA or a whole FPGA.
 
+config FPGA_REGION_SYSFS
+       bool "FPGA Region Sysfs"
+	depends on FPGA_REGION
+	help
+	  FPGA Region sysfs interface.  This creates sysfs file for each
+	  FPGA Region under /sys/class/fpga_region/ to show status and
+	  control programming FPGA regions.
+
 config OF_FPGA_REGION
 	tristate "FPGA Region Device Tree Overlay Support"
 	depends on FPGA_REGION
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 5690237..a63bc6c 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -105,6 +105,243 @@ EXPORT_SYMBOL_GPL(fpga_region_ovl_image_info);
 
 #endif /* CONFIG_OF_FPGA_REGION */
 
+#if IS_ENABLED(CONFIG_FPGA_REGION_SYSFS)
+
+struct fpga_image_info *image_info_from_region(struct fpga_region *region)
+{
+	struct fpga_image_info *image_info;
+
+	/* If region has an overlay, display image_info from overlay. */
+	image_info = fpga_region_ovl_image_info(region);
+	if (!image_info)
+		image_info = region->image_info;
+
+	return image_info;
+}
+
+/*
+ * Controlling a region both by sysfs and by device tree overlays is
+ * not supported.
+ */
+static ssize_t firmware_name_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_image_info *image_info;
+
+	image_info = image_info_from_region(region);
+
+	if (image_info && image_info->firmware_name)
+		return sprintf(buf, "%s\n", image_info->firmware_name);
+
+	return 0;
+}
+
+static ssize_t firmware_name_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	char *firmware_name;
+	size_t len;
+	int ret;
+
+	/*
+	 * Controlling a region both by sysfs and by device tree overlays is
+	 * not supported.
+	 */
+	if (fpga_region_ovl_image_info(region))
+		return -EINVAL;
+
+	if (!region->image_info) {
+		region->image_info = fpga_region_alloc_image_info(region);
+		if (!region->image_info)
+			return -ENOMEM;
+	}
+
+	firmware_name = devm_kzalloc(dev, count, GFP_KERNEL);
+	if (!firmware_name)
+		return -ENOMEM;
+	pr_err("count = %d\n", count);
+	/* lose terminating \n */
+	strcpy(firmware_name, buf);
+	len = strlen(firmware_name);
+	if (firmware_name[len - 1] == '\n')
+		firmware_name[len - 1] = 0;
+	if (firmware_name[0] == 0) {
+		devm_kfree(dev, firmware_name);
+		firmware_name = NULL;
+	}
+
+	/* Release previous firmware name (if any). Save current one. */
+	if (region->image_info->firmware_name)
+		devm_kfree(dev, region->image_info->firmware_name);
+	region->image_info->firmware_name = firmware_name;
+
+	if (firmware_name) {
+		ret = fpga_region_program_fpga(region, region->image_info);
+		if (ret)
+			dev_err(dev,
+				"FPGA programming failed with value %d\n", ret);
+	} else {
+		/*
+		 * Writing null string to firmware_name will disable and put
+		 * the bridges (if there were any bridges in the bridge list).
+		 */
+		fpga_bridges_disable(&region->bridge_list);
+		if (region->get_bridges)
+			fpga_bridges_put(&region->bridge_list);
+		fpga_region_free_image_info(region, region->image_info);
+		region->image_info = NULL;
+	}
+
+	return count;
+}
+
+static ssize_t partial_config_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_image_info *image_info;
+	int partial;
+
+	image_info = image_info_from_region(region);
+	if (!image_info)
+		return 0;
+
+	partial = !!(image_info->flags & FPGA_MGR_PARTIAL_RECONFIG);
+
+	return sprintf(buf, "%d\n", partial);
+}
+
+static ssize_t partial_config_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	unsigned long val;
+	int ret;
+
+	if (fpga_region_ovl_image_info(region))
+		return -EINVAL;
+
+	if (!region->image_info) {
+		region->image_info = fpga_region_alloc_image_info(region);
+		if (!region->image_info)
+			return -ENOMEM;
+	}
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val == 1)
+		region->image_info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
+	else if (val == 0)
+		region->image_info->flags &= ~FPGA_MGR_PARTIAL_RECONFIG;
+	else
+		return -EINVAL;
+
+	return count;
+}
+
+static ssize_t unfreeze_timeout_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_image_info *image_info;
+
+	image_info = image_info_from_region(region);
+	if (!image_info)
+		return 0;
+
+	return sprintf(buf, "%d\n", image_info->enable_timeout_us);
+}
+
+static ssize_t unfreeze_timeout_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf,
+				      size_t count)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	unsigned long val;
+	int ret;
+
+	if (fpga_region_ovl_image_info(region))
+		return -EINVAL;
+
+	if (!region->image_info) {
+		region->image_info = fpga_region_alloc_image_info(region);
+		if (!region->image_info)
+			return -ENOMEM;
+	}
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	region->image_info->enable_timeout_us = val;
+
+	return count;
+}
+
+static ssize_t freeze_timeout_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_image_info *image_info;
+
+	image_info = image_info_from_region(region);
+	if (!image_info)
+		return 0;
+
+	return sprintf(buf, "%d\n", image_info->disable_timeout_us);
+}
+
+static ssize_t freeze_timeout_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf,
+				    size_t count)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	unsigned long val;
+	int ret;
+
+	if (fpga_region_ovl_image_info(region))
+		return -EINVAL;
+
+	if (!region->image_info) {
+		region->image_info = fpga_region_alloc_image_info(region);
+		if (!region->image_info)
+			return -ENOMEM;
+	}
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	region->image_info->disable_timeout_us = val;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(firmware_name);
+static DEVICE_ATTR_RW(partial_config);
+static DEVICE_ATTR_RW(unfreeze_timeout);
+static DEVICE_ATTR_RW(freeze_timeout);
+
+static struct attribute *fpga_region_attrs[] = {
+	&dev_attr_firmware_name.attr,
+	&dev_attr_partial_config.attr,
+	&dev_attr_unfreeze_timeout.attr,
+	&dev_attr_freeze_timeout.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(fpga_region);
+
+#endif /* CONFIG_FPGA_REGION_SYSFS */
+
 /**
  * fpga_region_get - get an exclusive reference to a fpga region
  * @region: FPGA Region struct
@@ -288,6 +525,10 @@ static int __init fpga_region_init(void)
 	if (IS_ERR(fpga_region_class))
 		return PTR_ERR(fpga_region_class);
 
+#if IS_ENABLED(CONFIG_FPGA_REGION_SYSFS)
+	fpga_region_class->dev_groups = fpga_region_groups;
+#endif /* CONFIG_FPGA_REGION_SYSFS */
+
 	fpga_region_class->dev_release = fpga_region_dev_release;
 
 	return 0;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [RFC 8/8] doc: fpga: add sysfs document for fpga region
  2017-02-15 16:14 FPGA Region enhancements and fixes Alan Tull
                   ` (6 preceding siblings ...)
  2017-02-15 16:14 ` [RFC 7/8] fpga-region: add sysfs interface Alan Tull
@ 2017-02-15 16:14 ` Alan Tull
  2017-02-28 17:35 ` FPGA Region enhancements and fixes Alan Tull
  8 siblings, 0 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-15 16:14 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

Document the following added attributes for FPGA regions:
* firmware_name
* partial_config
* freeze_timeout
* unfreeze_timeout

Signed-off-by: Alan Tull <atull@kernel.org>
---
 Documentation/ABI/testing/sysfs-class-fpga-region | 26 +++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-region

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-region b/Documentation/ABI/testing/sysfs-class-fpga-region
new file mode 100644
index 0000000..4008fd6
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-fpga-region
@@ -0,0 +1,26 @@
+What:		/sys/class/fpga_region/<fpga>/firmware_name
+Date:		February 2017
+KernelVersion:	4.11
+Contact:	Alan Tull <atull@kernel.org>
+Description:	Write the name of FPGA image file on the firmware path to
+		program the FPGA.
+
+What:		/sys/class/fpga_region/<fpga>/partial_config
+Date:		February 2017
+KernelVersion:	4.11
+Contact:	Alan Tull <atull@kernel.org>
+Description:	Write 1 to specify FPGA parital configuration.
+		Write 0 to specify FPGA full configuration.
+
+What:		/sys/class/fpga_region/<fpga>/freeze_timeout
+Date:		February 2017
+KernelVersion:	4.11
+Contact:	Alan Tull <atull@kernel.org>
+Description:	Time in uSec to wait for a FPGA bridge to disable traffic.
+
+What:		/sys/class/fpga_region/<fpga>/unfreeze_timeout
+Date:		February 2017
+KernelVersion:	4.11
+Contact:	Alan Tull <atull@kernel.org>
+Description:	Time in uSec to wait for a FPGA bridge to enable traffic.
+
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 16:14 ` [RFC 7/8] fpga-region: add sysfs interface Alan Tull
@ 2017-02-15 17:21   ` Jason Gunthorpe
  2017-02-15 17:46     ` Alan Tull
  0 siblings, 1 reply; 63+ messages in thread
From: Jason Gunthorpe @ 2017-02-15 17:21 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 10:14:20AM -0600, Alan Tull wrote:
> Add a sysfs interface to control programming FPGA.
> 
> Each fpga-region will get the following files which set values
> in the fpga_image_info struct for that region.  More files will
> need to be added as fpga_image_info expands.
> 
> firmware_name
> * writing a name of a FPGA image file to firmware_name causes the
>   FPGA region to write the FPGA
> 
> partial_config
> * 0 : full reconfiguration
> * 1 : partial reconfiguration

This is really a property of the bitfile. It would be really nice to
have a saner system for describing the bitfiles that doesn't rely on
so much out of band stuff.

Eg when doing partial reconfiguration it would be really sane to have
some checks that the full bitfile is the correct basis for the partial
bitfile.

It also seems link Zynq needs an encrypted/not encrypted flag..

I wonder if we should require a Linux specific header on the bitfile
instead? That would make the bitfile self describing at least.

Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 17:21   ` Jason Gunthorpe
@ 2017-02-15 17:46     ` Alan Tull
  2017-02-15 17:55       ` Moritz Fischer
  2017-02-15 18:06       ` Jason Gunthorpe
  0 siblings, 2 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-15 17:46 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Moritz Fischer, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 11:21 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Feb 15, 2017 at 10:14:20AM -0600, Alan Tull wrote:
>> Add a sysfs interface to control programming FPGA.
>>
>> Each fpga-region will get the following files which set values
>> in the fpga_image_info struct for that region.  More files will
>> need to be added as fpga_image_info expands.
>>
>> firmware_name
>> * writing a name of a FPGA image file to firmware_name causes the
>>   FPGA region to write the FPGA
>>
>> partial_config
>> * 0 : full reconfiguration
>> * 1 : partial reconfiguration
>
> This is really a property of the bitfile. It would be really nice to
> have a saner system for describing the bitfiles that doesn't rely on
> so much out of band stuff.
>
> Eg when doing partial reconfiguration it would be really sane to have
> some checks that the full bitfile is the correct basis for the partial
> bitfile.
>
> It also seems link Zynq needs an encrypted/not encrypted flag..
>
> I wonder if we should require a Linux specific header on the bitfile
> instead? That would make the bitfile self describing at least.

Hi Jason,

I agree.  I've heard some discussions about adding a header.  We would
want it to not be manufacturer or fpga device specific. That would be
nice and would eliminate some of this struct.  We would need a tool to
add the header, given a bitstream and some info about the bitstream.
If the tool communicated seamlessly with vendor's tools that would be
nice, but that is complicated to get that to happen.  So far nobody
has posted their proposals to the mailing list.

Alan

>
> Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 17:46     ` Alan Tull
@ 2017-02-15 17:55       ` Moritz Fischer
  2017-02-15 18:06       ` Jason Gunthorpe
  1 sibling, 0 replies; 63+ messages in thread
From: Moritz Fischer @ 2017-02-15 17:55 UTC (permalink / raw)
  To: Alan Tull; +Cc: Jason Gunthorpe, linux-kernel, linux-fpga

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

On Wed, Feb 15, 2017 at 11:46:01AM -0600, Alan Tull wrote:
> On Wed, Feb 15, 2017 at 11:21 AM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
> > On Wed, Feb 15, 2017 at 10:14:20AM -0600, Alan Tull wrote:
> >> Add a sysfs interface to control programming FPGA.
> >>
> >> Each fpga-region will get the following files which set values
> >> in the fpga_image_info struct for that region.  More files will
> >> need to be added as fpga_image_info expands.
> >>
> >> firmware_name
> >> * writing a name of a FPGA image file to firmware_name causes the
> >>   FPGA region to write the FPGA
> >>
> >> partial_config
> >> * 0 : full reconfiguration
> >> * 1 : partial reconfiguration
> >
> > This is really a property of the bitfile. It would be really nice to
> > have a saner system for describing the bitfiles that doesn't rely on
> > so much out of band stuff.

Agreed.
> >
> > Eg when doing partial reconfiguration it would be really sane to have
> > some checks that the full bitfile is the correct basis for the partial
> > bitfile.
> >
> > It also seems link Zynq needs an encrypted/not encrypted flag..

Well, we could also run always at half rate and not benefit from faster
config for the non-encrypted case ;-)

> > I wonder if we should require a Linux specific header on the bitfile
> > instead? That would make the bitfile self describing at least.

> I agree.  I've heard some discussions about adding a header.  We would
> want it to not be manufacturer or fpga device specific. That would be
> nice and would eliminate some of this struct.  We would need a tool to
> add the header, given a bitstream and some info about the bitstream.
> If the tool communicated seamlessly with vendor's tools that would be
> nice, but that is complicated to get that to happen.  So far nobody
> has posted their proposals to the mailing list.

Well, there's not that many vendors out there. If we can figure out a
format and stick to it, keep it reasonably extensible, 'the vendors'
will eventually adopt it.

Cheers,

Moritz

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

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 17:46     ` Alan Tull
  2017-02-15 17:55       ` Moritz Fischer
@ 2017-02-15 18:06       ` Jason Gunthorpe
  2017-02-15 18:23         ` Alan Tull
  2017-02-15 21:20         ` Anatolij Gustschin
  1 sibling, 2 replies; 63+ messages in thread
From: Jason Gunthorpe @ 2017-02-15 18:06 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 11:46:01AM -0600, Alan Tull wrote:
> I agree.  I've heard some discussions about adding a header.  We would
> want it to not be manufacturer or fpga device specific. That would be
> nice and would eliminate some of this struct.  We would need a tool to
> add the header, given a bitstream and some info about the bitstream.
> If the tool communicated seamlessly with vendor's tools that would be
> nice, but that is complicated to get that to happen.  So far nobody
> has posted their proposals to the mailing list.

Okay, we've had success using a HTTP style plain text header for the
last 15 years. Here is a header example:

BIT/1.0
Bit-Order: reversed
Builder: jgg
Content-Length: 9987064
Date: Thu, 19 Jan 2017 06:22:42 GMT
Design: tluna
Device: 7k355t
GIT-TOT: 60da4e9e8e9610e490ddeb4e5880778938f80691
Package: ffg901
Pad: xxxx
Speed: 2
Speed-File: PRODUCTION 1.12 2014-09-11
Synplify-Ver: Pro I-2014.03-1 , Build 016R, Mar 24 2014
Xilinx-Ver: Vivado v.2016.1 (lin64) Build 1538259 Fri Apr  8 15:45:23 MDT 2016

[raw bitfile follows, start byte in the file is aligned for DMA]

The plaintext format allows a fair amount of flexibility, eg I could
include the linux header for partial/encrypt along with my usual
headers for identification.

So along those lines I'd suggest the basic Linux format to be

Linux_FPGA_BIT/1.0
FPGA-Device: xc7k355t-ffg901-2    # Allow the kernel driver to check, if it can
# Enable partial reconfiguration and require the full bitfile to have
# the ID 'xxx'
Partial-Reconfiguration-Basis-ID: xxxx
# This is a full bitfile with unique tag xxxx
FPGA-ID: xxxx 
Encrypted: yes/no   # Enable decryption if the driver needs to be told
Pad: xxxx           # Enough 'x' characters to align the bitfile

[raw bitfile follows, start byte in the file is aligned for DMA]

I can publish a version of my python script which produces these files
from typical Xilinx output..

The kernel could detect the bitfile starts with 'Linux_FPGA_BIT/1.0\n'
and then proceed to decode the header providing compat with the
current scheme.

This is usually the sort of stuff I'd punt to userspace, but since the
kernel is doing request_firmware it is hard to see how that is an
option in this case...

Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 18:06       ` Jason Gunthorpe
@ 2017-02-15 18:23         ` Alan Tull
  2017-02-15 18:31           ` Moritz Fischer
                             ` (2 more replies)
  2017-02-15 21:20         ` Anatolij Gustschin
  1 sibling, 3 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-15 18:23 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Moritz Fischer, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 12:06 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:

Hi Jason,

> On Wed, Feb 15, 2017 at 11:46:01AM -0600, Alan Tull wrote:
>> I agree.  I've heard some discussions about adding a header.  We would
>> want it to not be manufacturer or fpga device specific. That would be
>> nice and would eliminate some of this struct.  We would need a tool to
>> add the header, given a bitstream and some info about the bitstream.
>> If the tool communicated seamlessly with vendor's tools that would be
>> nice, but that is complicated to get that to happen.  So far nobody
>> has posted their proposals to the mailing list.
>
> Okay, we've had success using a HTTP style plain text header for the
> last 15 years. Here is a header example:
>
> BIT/1.0
> Bit-Order: reversed
> Builder: jgg
> Content-Length: 9987064
> Date: Thu, 19 Jan 2017 06:22:42 GMT
> Design: tluna
> Device: 7k355t
> GIT-TOT: 60da4e9e8e9610e490ddeb4e5880778938f80691
> Package: ffg901
> Pad: xxxx
> Speed: 2
> Speed-File: PRODUCTION 1.12 2014-09-11
> Synplify-Ver: Pro I-2014.03-1 , Build 016R, Mar 24 2014
> Xilinx-Ver: Vivado v.2016.1 (lin64) Build 1538259 Fri Apr  8 15:45:23 MDT 2016
>
> [raw bitfile follows, start byte in the file is aligned for DMA]
>
> The plaintext format allows a fair amount of flexibility, eg I could
> include the linux header for partial/encrypt along with my usual
> headers for identification.
>
> So along those lines I'd suggest the basic Linux format to be
>
> Linux_FPGA_BIT/1.0
> FPGA-Device: xc7k355t-ffg901-2    # Allow the kernel driver to check, if it can
> # Enable partial reconfiguration and require the full bitfile to have
> # the ID 'xxx'
> Partial-Reconfiguration-Basis-ID: xxxx
> # This is a full bitfile with unique tag xxxx
> FPGA-ID: xxxx
> Encrypted: yes/no   # Enable decryption if the driver needs to be told
> Pad: xxxx           # Enough 'x' characters to align the bitfile
>
> [raw bitfile follows, start byte in the file is aligned for DMA]
>
> I can publish a version of my python script which produces these files
> from typical Xilinx output..
>
> The kernel could detect the bitfile starts with 'Linux_FPGA_BIT/1.0\n'
> and then proceed to decode the header providing compat with the
> current scheme.
>
> This is usually the sort of stuff I'd punt to userspace, but since the
> kernel is doing request_firmware it is hard to see how that is an
> option in this case...

I like how extensible (and readable!) this is.  It wouldn't take much
kernel code to add this.  I'd like to see the python script.

Alan

>
> Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 18:23         ` Alan Tull
@ 2017-02-15 18:31           ` Moritz Fischer
  2017-02-15 19:49           ` Jason Gunthorpe
  2017-02-15 20:07           ` matthew.gerlach
  2 siblings, 0 replies; 63+ messages in thread
From: Moritz Fischer @ 2017-02-15 18:31 UTC (permalink / raw)
  To: Alan Tull; +Cc: Jason Gunthorpe, linux-kernel, linux-fpga

Hi Jason, Alan

On Wed, Feb 15, 2017 at 7:23 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:

>> This is usually the sort of stuff I'd punt to userspace, but since the
>> kernel is doing request_firmware it is hard to see how that is an
>> option in this case...
>
> I like how extensible (and readable!) this is.  It wouldn't take much
> kernel code to add this.  I'd like to see the python script.

We could also use something dts based like FIT in u-boot.
Just an idea. Downside is it would need a compiler (dtc)

Thanks,
Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 18:23         ` Alan Tull
  2017-02-15 18:31           ` Moritz Fischer
@ 2017-02-15 19:49           ` Jason Gunthorpe
  2017-02-15 22:49             ` Alan Tull
  2017-02-15 20:07           ` matthew.gerlach
  2 siblings, 1 reply; 63+ messages in thread
From: Jason Gunthorpe @ 2017-02-15 19:49 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 12:23:28PM -0600, Alan Tull wrote:
> > This is usually the sort of stuff I'd punt to userspace, but since the
> > kernel is doing request_firmware it is hard to see how that is an
> > option in this case...
> 
> I like how extensible (and readable!) this is.  It wouldn't take much
> kernel code to add this.  I'd like to see the python script.

Sure, attached

Jason

#!/usr/bin/env python
# COPYRIGHT (c) 2016 Obsidian Research Corporation.
# Permission is hereby granted, free of charge, to any person obtaining a copy
# of this software and associated documentation files (the "Software"), to deal
# in the Software without restriction, including without limitation the rights
# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
# copies of the Software, and to permit persons to whom the Software is
# furnished to do so, subject to the following conditions:

# The above copyright notice and this permission notice shall be included in
# all copies or substantial portions of the Software.

# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

import subprocess,re,string,os,stat,mmap,sys,base64;
import argparse
import time;

def timeRFC1123():
    # Python doesn't have this as a built in
    return subprocess.check_output(["date","-u",'+%a, %02d %b %Y %T GMT']).strip();

def getTOT():
    """Return the top of tree commit hash from git"""
    try:
        HEAD = subprocess.check_output(["git","rev-parse","--verify","HEAD"]).strip();
    except subprocess.CalledProcessError:
        return "??? %s"%(os.getcwd());

    dirty = subprocess.check_output(["git","diff","--stat"]).strip();
    if dirty:
        return "%s-dirty"%(HEAD);
    return HEAD;

def parseISEPar(f,fmts):
    """Read Xilinx ISE par log files to get relevant design information"""
    f = string.join(f.readlines());
    g = re.search('"([^"]+)" is an NCD, version [0-9.]+, device (\S+), package (\S+), speed -(\d+)',f).groups();
    fmts.update({"Design": g[0],
                 "Device": g[1],
                 "Package": g[2],
                 "Speed": g[3]});
    fmts["PAR-Ver"] = re.search("(Release \S+ par \S+(?: \(\S+\))?)\n",f).groups()[0]
    fmts["Speed-File"] = re.search('Device speed data version:\s+"([^"]+)"',f).groups()[0];

def parseVivadoTwr(f,fmts):
    """Read Vivado 'report_timing' log files to get relevant design information"""
    f = string.join(f.readlines(1024));
    g = re.search('Design\s+:\s+(\S+)',f).groups();
    fmts["Design"] = g[0];

    g = re.search('Speed File\s+:\s+-(\d+)\s+(.+)',f);
    if g is not None:
        g = g.groups();
        fmts["Speed"] = g[0];
        fmts["Speed-File"] = g[1];
        g = re.search('Device\s+:\s+(\S+)-(\S+)',f).groups();
        fmts["Device"] = g[0];
        fmts["Package"] = g[1];
    else:
        g = re.search('Part\s+:\s+Device=(\S+)\s+Package=(\S+)\s+Speed=-(\d+)\s+\((.+)\)',f).groups();
        fmts.update({"Device": g[0],
                     "Package": g[1],
                     "Speed": g[2],
                     "Speed-File": g[3]});

    g = re.search('Version\s+:\s+(.+)',f).groups();
    fmts["Xilinx-Ver"] = g[0];

def parseSrr(f,fmts):
    """Read Synplify log files to get relevent design information"""
    l = f.readline().strip();
    fmts["Synplify-Ver"] = re.match("#Build: Synplify (.*)",l).groups()[0];

def find_start(bitm):
    """Locate the start of the actual bitsream in a Xilinx .bit file.  Xilinx
       tools drop an Impact header in front of the sync word. The FPGA ignores
       everything prior to the sync word."""
    for I in range(len(bitm)):
        if (bitm[I] == '\xaa' and bitm[I+1] == '\x99' and
            bitm[I+2] == '\x55' and bitm[I+3] == '\x66'):
            return I;
    return 0;

def align_bitstream(fmts,alignment=8):
    """Adjust the header content so that the bitstream starts aligned. This is
    so we can mmap this file with the header and still DMA from it."""
    while True:
        hdr = ("YYBIT/1.0\n" +
               "\n".join("%s: %s"%(k,v) for k,v in sorted(fmts.iteritems())) +
               "\n\n");
        if len(hdr) % alignment == 0:
            return hdr;
        fmts["Pad"] = "x"*(alignment - ((len(hdr) + 6) % alignment));

def makeHeader(out,args):
    fmts = {
        "Builder": os.getenv("USERNAME",os.getenv("USER","???")),
        "Date": timeRFC1123(),
        "GIT-TOT": getTOT(),
        "Bit-Order": args.order,
    };
    for fn in args.logs:
        with open(fn) as F:
            if fn.endswith(".par"):
                parseISEPar(F,fmts);
            if fn.endswith(".srr"):
                parseSrr(F,fmts);
            if fn.endswith(".twr"):
                parseVivadoTwr(F,fmts);
            if fn.endswith(".tsr"):
                parseVivadoTwr(F,fmts);

    with open(args.bit) as bitf:
        bitlen = os.fstat(bitf.fileno())[stat.ST_SIZE];

        bitm = mmap.mmap(bitf.fileno(),bitlen,access=mmap.ACCESS_COPY);
        start = 0;

        # This is the format for our bit bang schemes. The pin labeled D0 is
        # taken from bit 7.
        if args.order == "reversed":
            for i in range(0,bitlen):
                v = ord(bitm[i]);
                bitm[i] = chr(((v & (1<<0)) << 7) |
                              ((v & (1<<1)) << 5) |
                              ((v & (1<<2)) << 3) |
                              ((v & (1<<3)) << 1) |
                              ((v & (1<<4)) >> 1) |
                              ((v & (1<<5)) >> 3) |
                              ((v & (1<<6)) >> 5) |
                              ((v & (1<<7)) >> 7));

        # This is the format DMA to devcfg on the Zynq wants, impact header
        # stripped, sync word in little endian and aligned.
        if args.order == "byte-reversed":
            start = find_start(bitm);
            for i in range(start,bitlen//4*4,4):
                bitm[i],bitm[i+1],bitm[i+2],bitm[i+3] = bitm[i+3],bitm[i+2],bitm[i+1],bitm[i];

        if start != 0:
            fmts["Impact-Header"] = base64.b64encode(bitm[:start]);

        fmts["Content-Length"] = bitlen - start;
        out.write(align_bitstream(fmts));

        out.write(bitm[start:]);

parser = argparse.ArgumentParser(description="Format a Xilinx .bit file into a ybf with the necessary headers")
parser.add_argument("--ybf",required=True,
                    help="Output filename");
parser.add_argument("--bit",required=True,
                    help="Input bit filename");
parser.add_argument("--archive",
                    help="Optional directory to place a timestamped hardlink");
parser.add_argument("--deps",
                    help="File to write makefile dependencies list to");
parser.add_argument("--order",default="reversed",
                    help="Byte or bit order to use for the raw data");
parser.add_argument('logs',nargs="+",
                    help="Log files to pull meta data out of")
args = parser.parse_args();

with open(args.ybf,"wt") as F:
    makeHeader(F,args);

if args.archive:
    os.link(args.ybf,os.path.join(args.archive,"%s-%s"%(os.path.basename(args.ybf),int(time.time()))));

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 18:23         ` Alan Tull
  2017-02-15 18:31           ` Moritz Fischer
  2017-02-15 19:49           ` Jason Gunthorpe
@ 2017-02-15 20:07           ` matthew.gerlach
  2017-02-15 20:37             ` Jason Gunthorpe
  2 siblings, 1 reply; 63+ messages in thread
From: matthew.gerlach @ 2017-02-15 20:07 UTC (permalink / raw)
  To: Alan Tull; +Cc: Jason Gunthorpe, Moritz Fischer, linux-kernel, linux-fpga


Hi Jason, Alan, and Moritz,

On Wed, 15 Feb 2017, Alan Tull wrote:

> On Wed, Feb 15, 2017 at 12:06 PM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
>
> Hi Jason,
>
>> On Wed, Feb 15, 2017 at 11:46:01AM -0600, Alan Tull wrote:
>>> I agree.  I've heard some discussions about adding a header.  We would
>>> want it to not be manufacturer or fpga device specific. That would be
>>> nice and would eliminate some of this struct.  We would need a tool to
>>> add the header, given a bitstream and some info about the bitstream.
>>> If the tool communicated seamlessly with vendor's tools that would be
>>> nice, but that is complicated to get that to happen.  So far nobody
>>> has posted their proposals to the mailing list.

It seems pretty clear that there is a set of meta data associated with 
a fpga bitstream to allow that bit stream to be authenticated, decrypted, 
and configured into the fpga.  When using device tree based fpga 
configuration, the meta data has been put into a device tree or device
tree overlay that is separate from the bitstream itself.

It does seem reasonable to consider combining the meta data with actual 
bitstream data.  The benefit of combining the meta data with the bitstream 
is that it simplifies the userspace/kernel interface to a single file 
transfer instead of having a growing number of sysfs entries for the meta 
data.

>>
>> Okay, we've had success using a HTTP style plain text header for the
>> last 15 years. Here is a header example:
>>
>> BIT/1.0
>> Bit-Order: reversed
>> Builder: jgg
>> Content-Length: 9987064
>> Date: Thu, 19 Jan 2017 06:22:42 GMT
>> Design: tluna
>> Device: 7k355t
>> GIT-TOT: 60da4e9e8e9610e490ddeb4e5880778938f80691
>> Package: ffg901
>> Pad: xxxx
>> Speed: 2
>> Speed-File: PRODUCTION 1.12 2014-09-11
>> Synplify-Ver: Pro I-2014.03-1 , Build 016R, Mar 24 2014
>> Xilinx-Ver: Vivado v.2016.1 (lin64) Build 1538259 Fri Apr  8 15:45:23 MDT 2016
>>
>> [raw bitfile follows, start byte in the file is aligned for DMA]
>>
>> The plaintext format allows a fair amount of flexibility, eg I could
>> include the linux header for partial/encrypt along with my usual
>> headers for identification.
>>
>> So along those lines I'd suggest the basic Linux format to be
>>
>> Linux_FPGA_BIT/1.0
>> FPGA-Device: xc7k355t-ffg901-2    # Allow the kernel driver to check, if it can
>> # Enable partial reconfiguration and require the full bitfile to have
>> # the ID 'xxx'
>> Partial-Reconfiguration-Basis-ID: xxxx
>> # This is a full bitfile with unique tag xxxx
>> FPGA-ID: xxxx
>> Encrypted: yes/no   # Enable decryption if the driver needs to be told
>> Pad: xxxx           # Enough 'x' characters to align the bitfile


The format of the meta data associated with a fpga bitstream is certainly 
a subject on its own.  HTTP style plain text is definately easy to 
understand and more importantly it is extendable.  On the other hand, it 
seems dangerous to be doing a lot of string parsing in the kernel.  Is 
there already an example of kernel code parsing an extendable data format? 
Depending on how the kernel is configured, the kernel code can parse a 
device tree blob.  I also think someone mentioned the FIT format which is 
closely related to device tree format.

Matthew Gerlach

>>
>> [raw bitfile follows, start byte in the file is aligned for DMA]
>>
>> I can publish a version of my python script which produces these files
>> from typical Xilinx output..
>>
>> The kernel could detect the bitfile starts with 'Linux_FPGA_BIT/1.0\n'
>> and then proceed to decode the header providing compat with the
>> current scheme.
>>
>> This is usually the sort of stuff I'd punt to userspace, but since the
>> kernel is doing request_firmware it is hard to see how that is an
>> option in this case...
>
> I like how extensible (and readable!) this is.  It wouldn't take much
> kernel code to add this.  I'd like to see the python script.
>
> Alan
>
>>
>> Jason
> --
> 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
>

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 20:07           ` matthew.gerlach
@ 2017-02-15 20:37             ` Jason Gunthorpe
  2017-02-15 20:54               ` Moritz Fischer
  0 siblings, 1 reply; 63+ messages in thread
From: Jason Gunthorpe @ 2017-02-15 20:37 UTC (permalink / raw)
  To: matthew.gerlach; +Cc: Alan Tull, Moritz Fischer, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 12:07:15PM -0800, matthew.gerlach@linux.intel.com wrote:

> The format of the meta data associated with a fpga bitstream is certainly a
> subject on its own.  HTTP style plain text is definately easy to understand
> and more importantly it is extendable.  On the other hand, it seems
> dangerous to be doing a lot of string parsing in the kernel.

It is fairly close to binary parsing.. The process is

- Find the first occurance of \n\n, must be less than XX bytes
- Memcpy that from the sg list into a linear buffer
- Replace all \n with \0

To access a key:
- Case insensitive search for START + "Key: " or \0 + "Key: "
- Return as a string the part after the match

This isn't the sort of string parsing that typically gets you into
trouble. If we can't code the above correctly then we will screw up
safe binary parsing of strings too :)

Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 20:37             ` Jason Gunthorpe
@ 2017-02-15 20:54               ` Moritz Fischer
  2017-02-15 21:15                 ` Jason Gunthorpe
  2017-02-17 22:28                 ` Yves Vandervennet
  0 siblings, 2 replies; 63+ messages in thread
From: Moritz Fischer @ 2017-02-15 20:54 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: matthew.gerlach, Alan Tull, linux-kernel, linux-fpga

Hi Jason,

On Wed, Feb 15, 2017 at 12:37 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Feb 15, 2017 at 12:07:15PM -0800, matthew.gerlach@linux.intel.com wrote:
>
>> The format of the meta data associated with a fpga bitstream is certainly a
>> subject on its own.  HTTP style plain text is definately easy to understand
>> and more importantly it is extendable.  On the other hand, it seems
>> dangerous to be doing a lot of string parsing in the kernel.
>
> It is fairly close to binary parsing.. The process is
>
> - Find the first occurance of \n\n, must be less than XX bytes
> - Memcpy that from the sg list into a linear buffer
> - Replace all \n with \0
>
> To access a key:
> - Case insensitive search for START + "Key: " or \0 + "Key: "
> - Return as a string the part after the match
>
> This isn't the sort of string parsing that typically gets you into
> trouble. If we can't code the above correctly then we will screw up
> safe binary parsing of strings too :)

Well I don't know ;-) With something fdt based we already have parsers there,
compilers are already in tree. I'll take another look at the u-boot
code, I think their
FIT (Flattened Image Tree) would be a fairly good match for what we're
trying to do.

Cheers,
Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 20:54               ` Moritz Fischer
@ 2017-02-15 21:15                 ` Jason Gunthorpe
  2017-02-15 21:36                   ` Moritz Fischer
  2017-02-17 22:28                 ` Yves Vandervennet
  1 sibling, 1 reply; 63+ messages in thread
From: Jason Gunthorpe @ 2017-02-15 21:15 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: matthew.gerlach, Alan Tull, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 12:54:27PM -0800, Moritz Fischer wrote:

> Well I don't know ;-) With something fdt based we already have
> parsers there,

Not sure.. How does incbin work in DTB?

We have the FPGA in a s/g list so we cannot pass the entire file to
libfdt - is that consistent with incbin?

Can we force a specific alignment for the included data?

How complex will the userspace tool be to make the image?

Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 18:06       ` Jason Gunthorpe
  2017-02-15 18:23         ` Alan Tull
@ 2017-02-15 21:20         ` Anatolij Gustschin
  1 sibling, 0 replies; 63+ messages in thread
From: Anatolij Gustschin @ 2017-02-15 21:20 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Alan Tull, Moritz Fischer, linux-kernel, linux-fpga

Hi Jason,

On Wed, 15 Feb 2017 11:06:12 -0700
Jason Gunthorpe jgunthorpe@obsidianresearch.com wrote:
...
>The kernel could detect the bitfile starts with 'Linux_FPGA_BIT/1.0\n'
>and then proceed to decode the header providing compat with the
>current scheme.
>
>This is usually the sort of stuff I'd punt to userspace, but since the
>kernel is doing request_firmware it is hard to see how that is an
>option in this case...

Interesting. We have a requirement that the complete history of FPGA
configurations (basic configuration and the sequence of following
partial reconfigurations) should be readable from FPGA manager
framework on request. Each bitfile is associated with meta-data
and one should be able to read this meta-data back for complete
reconfiguration chain. When the kernel decodes a header, it could
optionally store the data and allow to read it back on request.

Thanks,
Anatolij

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 21:15                 ` Jason Gunthorpe
@ 2017-02-15 21:36                   ` Moritz Fischer
  2017-02-15 22:42                     ` Alan Tull
  0 siblings, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-15 21:36 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: matthew.gerlach, Alan Tull, linux-kernel, linux-fpga

Jason,

On Wed, Feb 15, 2017 at 1:15 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Feb 15, 2017 at 12:54:27PM -0800, Moritz Fischer wrote:
>
>> Well I don't know ;-) With something fdt based we already have
>> parsers there,
>
> Not sure.. How does incbin work in DTB?
>
> We have the FPGA in a s/g list so we cannot pass the entire file to
> libfdt - is that consistent with incbin?

Well you could attach the (for lack of better word) blob to the beginning,
instead of doing incbin

> Can we force a specific alignment for the included data?

I'd say probably, but haven't checked.

> How complex will the userspace tool be to make the image?

Userspace can be as complex as it needs to be, imho, if it makes
kernel space easier & safer.

I'll need to do some more reading over the weekend before I can make
more sensible comments :)

Thanks,

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 21:36                   ` Moritz Fischer
@ 2017-02-15 22:42                     ` Alan Tull
  2017-02-16  0:16                       ` Moritz Fischer
  0 siblings, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-15 22:42 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Jason Gunthorpe, matthew.gerlach, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 3:36 PM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:
> Jason,
>
> On Wed, Feb 15, 2017 at 1:15 PM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
>> On Wed, Feb 15, 2017 at 12:54:27PM -0800, Moritz Fischer wrote:
>>
>>> Well I don't know ;-) With something fdt based we already have
>>> parsers there,
>>
>> Not sure.. How does incbin work in DTB?
>>
>> We have the FPGA in a s/g list so we cannot pass the entire file to
>> libfdt - is that consistent with incbin?
>
> Well you could attach the (for lack of better word) blob to the beginning,
> instead of doing incbin
>
>> Can we force a specific alignment for the included data?
>
> I'd say probably, but haven't checked.
>
>> How complex will the userspace tool be to make the image?
>
> Userspace can be as complex as it needs to be, imho, if it makes
> kernel space easier & safer.
>
> I'll need to do some more reading over the weekend before I can make
> more sensible comments :)
>
> Thanks,
>
> Moritz

Another thought I have about this is that adding the header to
bitstreams can be a piece of independence from DT for systems that
aren't already using DT.  This includes x86 in Linux.  It also
includes other OS's that aren't using DT, they can reuse the same
image files without having to add dtc.  As much as I like DT, it is
something I'm having to think about.

Alan

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 19:49           ` Jason Gunthorpe
@ 2017-02-15 22:49             ` Alan Tull
  2017-02-15 23:07               ` Jason Gunthorpe
  0 siblings, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-15 22:49 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Moritz Fischer, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 1:49 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Feb 15, 2017 at 12:23:28PM -0600, Alan Tull wrote:
>> > This is usually the sort of stuff I'd punt to userspace, but since the
>> > kernel is doing request_firmware it is hard to see how that is an
>> > option in this case...
>>
>> I like how extensible (and readable!) this is.  It wouldn't take much
>> kernel code to add this.  I'd like to see the python script.
>
> Sure, attached
>
> Jason

Hi Jason,

Thanks for sharing this.

So this script takes the bitfile and its build logs as input, parses
the build logs for image information, does some manipulations on bit
order as needed, and adds the header.  So it's really doing (at least)
two things: adding header info and doing bitorder changes where needed
so that the kernel won't need to do it.

Alan

>
> #!/usr/bin/env python
> # COPYRIGHT (c) 2016 Obsidian Research Corporation.
> # Permission is hereby granted, free of charge, to any person obtaining a copy
> # of this software and associated documentation files (the "Software"), to deal
> # in the Software without restriction, including without limitation the rights
> # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> # copies of the Software, and to permit persons to whom the Software is
> # furnished to do so, subject to the following conditions:
>
> # The above copyright notice and this permission notice shall be included in
> # all copies or substantial portions of the Software.
>
> # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> # THE SOFTWARE.
>
> import subprocess,re,string,os,stat,mmap,sys,base64;
> import argparse
> import time;
>
> def timeRFC1123():
>     # Python doesn't have this as a built in
>     return subprocess.check_output(["date","-u",'+%a, %02d %b %Y %T GMT']).strip();
>
> def getTOT():
>     """Return the top of tree commit hash from git"""
>     try:
>         HEAD = subprocess.check_output(["git","rev-parse","--verify","HEAD"]).strip();
>     except subprocess.CalledProcessError:
>         return "??? %s"%(os.getcwd());
>
>     dirty = subprocess.check_output(["git","diff","--stat"]).strip();
>     if dirty:
>         return "%s-dirty"%(HEAD);
>     return HEAD;
>
> def parseISEPar(f,fmts):
>     """Read Xilinx ISE par log files to get relevant design information"""
>     f = string.join(f.readlines());
>     g = re.search('"([^"]+)" is an NCD, version [0-9.]+, device (\S+), package (\S+), speed -(\d+)',f).groups();
>     fmts.update({"Design": g[0],
>                  "Device": g[1],
>                  "Package": g[2],
>                  "Speed": g[3]});
>     fmts["PAR-Ver"] = re.search("(Release \S+ par \S+(?: \(\S+\))?)\n",f).groups()[0]
>     fmts["Speed-File"] = re.search('Device speed data version:\s+"([^"]+)"',f).groups()[0];
>
> def parseVivadoTwr(f,fmts):
>     """Read Vivado 'report_timing' log files to get relevant design information"""
>     f = string.join(f.readlines(1024));
>     g = re.search('Design\s+:\s+(\S+)',f).groups();
>     fmts["Design"] = g[0];
>
>     g = re.search('Speed File\s+:\s+-(\d+)\s+(.+)',f);
>     if g is not None:
>         g = g.groups();
>         fmts["Speed"] = g[0];
>         fmts["Speed-File"] = g[1];
>         g = re.search('Device\s+:\s+(\S+)-(\S+)',f).groups();
>         fmts["Device"] = g[0];
>         fmts["Package"] = g[1];
>     else:
>         g = re.search('Part\s+:\s+Device=(\S+)\s+Package=(\S+)\s+Speed=-(\d+)\s+\((.+)\)',f).groups();
>         fmts.update({"Device": g[0],
>                      "Package": g[1],
>                      "Speed": g[2],
>                      "Speed-File": g[3]});
>
>     g = re.search('Version\s+:\s+(.+)',f).groups();
>     fmts["Xilinx-Ver"] = g[0];
>
> def parseSrr(f,fmts):
>     """Read Synplify log files to get relevent design information"""
>     l = f.readline().strip();
>     fmts["Synplify-Ver"] = re.match("#Build: Synplify (.*)",l).groups()[0];
>
> def find_start(bitm):
>     """Locate the start of the actual bitsream in a Xilinx .bit file.  Xilinx
>        tools drop an Impact header in front of the sync word. The FPGA ignores
>        everything prior to the sync word."""
>     for I in range(len(bitm)):
>         if (bitm[I] == '\xaa' and bitm[I+1] == '\x99' and
>             bitm[I+2] == '\x55' and bitm[I+3] == '\x66'):
>             return I;
>     return 0;
>
> def align_bitstream(fmts,alignment=8):
>     """Adjust the header content so that the bitstream starts aligned. This is
>     so we can mmap this file with the header and still DMA from it."""
>     while True:
>         hdr = ("YYBIT/1.0\n" +
>                "\n".join("%s: %s"%(k,v) for k,v in sorted(fmts.iteritems())) +
>                "\n\n");
>         if len(hdr) % alignment == 0:
>             return hdr;
>         fmts["Pad"] = "x"*(alignment - ((len(hdr) + 6) % alignment));
>
> def makeHeader(out,args):
>     fmts = {
>         "Builder": os.getenv("USERNAME",os.getenv("USER","???")),
>         "Date": timeRFC1123(),
>         "GIT-TOT": getTOT(),
>         "Bit-Order": args.order,
>     };
>     for fn in args.logs:
>         with open(fn) as F:
>             if fn.endswith(".par"):
>                 parseISEPar(F,fmts);
>             if fn.endswith(".srr"):
>                 parseSrr(F,fmts);
>             if fn.endswith(".twr"):
>                 parseVivadoTwr(F,fmts);
>             if fn.endswith(".tsr"):
>                 parseVivadoTwr(F,fmts);
>
>     with open(args.bit) as bitf:
>         bitlen = os.fstat(bitf.fileno())[stat.ST_SIZE];
>
>         bitm = mmap.mmap(bitf.fileno(),bitlen,access=mmap.ACCESS_COPY);
>         start = 0;
>
>         # This is the format for our bit bang schemes. The pin labeled D0 is
>         # taken from bit 7.
>         if args.order == "reversed":
>             for i in range(0,bitlen):
>                 v = ord(bitm[i]);
>                 bitm[i] = chr(((v & (1<<0)) << 7) |
>                               ((v & (1<<1)) << 5) |
>                               ((v & (1<<2)) << 3) |
>                               ((v & (1<<3)) << 1) |
>                               ((v & (1<<4)) >> 1) |
>                               ((v & (1<<5)) >> 3) |
>                               ((v & (1<<6)) >> 5) |
>                               ((v & (1<<7)) >> 7));
>
>         # This is the format DMA to devcfg on the Zynq wants, impact header
>         # stripped, sync word in little endian and aligned.
>         if args.order == "byte-reversed":
>             start = find_start(bitm);
>             for i in range(start,bitlen//4*4,4):
>                 bitm[i],bitm[i+1],bitm[i+2],bitm[i+3] = bitm[i+3],bitm[i+2],bitm[i+1],bitm[i];
>
>         if start != 0:
>             fmts["Impact-Header"] = base64.b64encode(bitm[:start]);
>
>         fmts["Content-Length"] = bitlen - start;
>         out.write(align_bitstream(fmts));
>
>         out.write(bitm[start:]);
>
> parser = argparse.ArgumentParser(description="Format a Xilinx .bit file into a ybf with the necessary headers")
> parser.add_argument("--ybf",required=True,
>                     help="Output filename");
> parser.add_argument("--bit",required=True,
>                     help="Input bit filename");
> parser.add_argument("--archive",
>                     help="Optional directory to place a timestamped hardlink");
> parser.add_argument("--deps",
>                     help="File to write makefile dependencies list to");
> parser.add_argument("--order",default="reversed",
>                     help="Byte or bit order to use for the raw data");
> parser.add_argument('logs',nargs="+",
>                     help="Log files to pull meta data out of")
> args = parser.parse_args();
>
> with open(args.ybf,"wt") as F:
>     makeHeader(F,args);
>
> if args.archive:
>     os.link(args.ybf,os.path.join(args.archive,"%s-%s"%(os.path.basename(args.ybf),int(time.time()))));

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 22:49             ` Alan Tull
@ 2017-02-15 23:07               ` Jason Gunthorpe
  0 siblings, 0 replies; 63+ messages in thread
From: Jason Gunthorpe @ 2017-02-15 23:07 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 04:49:58PM -0600, Alan Tull wrote:

> So this script takes the bitfile and its build logs as input, parses
> the build logs for image information, does some manipulations on bit
> order as needed, and adds the header.  So it's really doing (at least)
> two things: adding header info and doing bitorder changes where needed
> so that the kernel won't need to do it.

Yes. This mangling is basically mandatory for Zynq due to how DevCfg
works, what Xilinx tools emit, and the desire to avoid copying the
bitfile.

Other cases are less essential, eg a gpio driver could do the
bit-reversal internally. We did the swap when writing the image
because the speed up was very noticable when the programming hardware
was a < 100MHz CPU.

It would be trivial to add Altera support, it really just needs a
similar build log parser for Altera's format to extract similar
information.

Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 22:42                     ` Alan Tull
@ 2017-02-16  0:16                       ` Moritz Fischer
  2017-02-16 17:47                         ` Alan Tull
  0 siblings, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-16  0:16 UTC (permalink / raw)
  To: Alan Tull; +Cc: Jason Gunthorpe, matthew.gerlach, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 2:42 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
> On Wed, Feb 15, 2017 at 3:36 PM, Moritz Fischer
> <moritz.fischer@ettus.com> wrote:
>> Jason,
>>
>> On Wed, Feb 15, 2017 at 1:15 PM, Jason Gunthorpe
>> <jgunthorpe@obsidianresearch.com> wrote:
>>> On Wed, Feb 15, 2017 at 12:54:27PM -0800, Moritz Fischer wrote:
>>>
>>>> Well I don't know ;-) With something fdt based we already have
>>>> parsers there,
>>>
>>> Not sure.. How does incbin work in DTB?
>>>
>>> We have the FPGA in a s/g list so we cannot pass the entire file to
>>> libfdt - is that consistent with incbin?
>>
>> Well you could attach the (for lack of better word) blob to the beginning,
>> instead of doing incbin
>>
>>> Can we force a specific alignment for the included data?
>>
>> I'd say probably, but haven't checked.
>>
>>> How complex will the userspace tool be to make the image?
>>
>> Userspace can be as complex as it needs to be, imho, if it makes
>> kernel space easier & safer.
>>
>> I'll need to do some more reading over the weekend before I can make
>> more sensible comments :)
>>
>> Thanks,
>>
>> Moritz
>
> Another thought I have about this is that adding the header to
> bitstreams can be a piece of independence from DT for systems that
> aren't already using DT.  This includes x86 in Linux.  It also
> includes other OS's that aren't using DT, they can reuse the same
> image files without having to add dtc.  As much as I like DT, it is
> something I'm having to think about.

Just to clarify:
I was proposing using the binary format of dts, not actually requiring
devicetree
for it to work. There's plenty of people running u-boot on x86 using FIT images
to boot.

W.r.t to Jason's script, it's there. Almost any company dealing with
Xilinx FPGAs
will have one of those. We have one, too. I recall having seen another one made
and shared by Mike @ topic.

While it's a good starting point ,I *really* don't like the idea
parsing user-land
provided strings in kernel space in a parser that we open-code.

Good discussion ;-)

Cheers,
Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 1/8] fpga-mgr: add a single function for fpga loading methods
  2017-02-15 16:14 ` [RFC 1/8] fpga-mgr: add a single function for fpga loading methods Alan Tull
@ 2017-02-16  0:36   ` matthew.gerlach
  0 siblings, 0 replies; 63+ messages in thread
From: matthew.gerlach @ 2017-02-16  0:36 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, Jason Gunthorpe, linux-kernel, linux-fpga

Hi Alan,


On Wed, 15 Feb 2017, Alan Tull wrote:

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

I like this cleaner api.

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

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


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

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 2/8] fpga-region: support more than one overlay per FPGA region
  2017-02-15 16:14 ` [RFC 2/8] fpga-region: support more than one overlay per FPGA region Alan Tull
@ 2017-02-16 16:50   ` matthew.gerlach
  2017-02-16 17:35     ` Alan Tull
  0 siblings, 1 reply; 63+ messages in thread
From: matthew.gerlach @ 2017-02-16 16:50 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, Jason Gunthorpe, linux-kernel, linux-fpga



Hi Alan,

On Wed, 15 Feb 2017, Alan Tull wrote:

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

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


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

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 2/8] fpga-region: support more than one overlay per FPGA region
  2017-02-16 16:50   ` matthew.gerlach
@ 2017-02-16 17:35     ` Alan Tull
  0 siblings, 0 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-16 17:35 UTC (permalink / raw)
  To: matthew.gerlach; +Cc: Moritz Fischer, Jason Gunthorpe, linux-kernel, linux-fpga

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

Hi Matthew,

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

Alan

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

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-16  0:16                       ` Moritz Fischer
@ 2017-02-16 17:47                         ` Alan Tull
  2017-02-16 17:56                           ` Jason Gunthorpe
  0 siblings, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-16 17:47 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Jason Gunthorpe, matthew.gerlach, linux-kernel, linux-fpga

On Wed, Feb 15, 2017 at 6:16 PM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:
> On Wed, Feb 15, 2017 at 2:42 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>> On Wed, Feb 15, 2017 at 3:36 PM, Moritz Fischer
>> <moritz.fischer@ettus.com> wrote:
>>> Jason,
>>>
>>> On Wed, Feb 15, 2017 at 1:15 PM, Jason Gunthorpe
>>> <jgunthorpe@obsidianresearch.com> wrote:
>>>> On Wed, Feb 15, 2017 at 12:54:27PM -0800, Moritz Fischer wrote:
>>>>
>>>>> Well I don't know ;-) With something fdt based we already have
>>>>> parsers there,
>>>>
>>>> Not sure.. How does incbin work in DTB?
>>>>
>>>> We have the FPGA in a s/g list so we cannot pass the entire file to
>>>> libfdt - is that consistent with incbin?
>>>
>>> Well you could attach the (for lack of better word) blob to the beginning,
>>> instead of doing incbin
>>>
>>>> Can we force a specific alignment for the included data?
>>>
>>> I'd say probably, but haven't checked.
>>>
>>>> How complex will the userspace tool be to make the image?
>>>
>>> Userspace can be as complex as it needs to be, imho, if it makes
>>> kernel space easier & safer.
>>>
>>> I'll need to do some more reading over the weekend before I can make
>>> more sensible comments :)
>>>
>>> Thanks,
>>>
>>> Moritz
>>
>> Another thought I have about this is that adding the header to
>> bitstreams can be a piece of independence from DT for systems that
>> aren't already using DT.  This includes x86 in Linux.  It also
>> includes other OS's that aren't using DT, they can reuse the same
>> image files without having to add dtc.  As much as I like DT, it is
>> something I'm having to think about.
>
> Just to clarify:
> I was proposing using the binary format of dts, not actually requiring
> devicetree
> for it to work. There's plenty of people running u-boot on x86 using FIT images
> to boot.

The FPGA images should not be required to have OS specific parts.
Some ahem non-Linux OS's that use FPGAs don't use device tree, so that
adds an extra complication for them unnecessarily.

>
> W.r.t to Jason's script, it's there. Almost any company dealing with
> Xilinx FPGAs
> will have one of those. We have one, too. I recall having seen another one made
> and shared by Mike @ topic.
>
> While it's a good starting point ,I *really* don't like the idea
> parsing user-land
> provided strings in kernel space in a parser that we open-code.

Why do you not like about it?  Jason posted some very clear practices
on how to do that properly and safely.

>
> Good discussion ;-)

Yes, I like it. :)

Alan

>
> Cheers,
> Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-16 17:47                         ` Alan Tull
@ 2017-02-16 17:56                           ` Jason Gunthorpe
  2017-02-16 18:11                             ` Moritz Fischer
  0 siblings, 1 reply; 63+ messages in thread
From: Jason Gunthorpe @ 2017-02-16 17:56 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, matthew.gerlach, linux-kernel, linux-fpga

On Thu, Feb 16, 2017 at 11:47:08AM -0600, Alan Tull wrote:

> > Just to clarify: I was proposing using the binary format of dts,
> > not actually requiring devicetree for it to work. There's plenty
> > of people running u-boot on x86 using FIT images to boot.
> 
> The FPGA images should not be required to have OS specific parts.
> Some ahem non-Linux OS's that use FPGAs don't use device tree, so that
> adds an extra complication for them unnecessarily.

Not just that, but we parse the bitfile headers in user space as well.

Requiring people to use libfdt pretty much kills the idea because of
its GPL license.

As I've shown the plain text headers can be produced in a scripting
langauge, and are trivially consumed without much trouble. IHMO this
makes it more likely there would be adoption..

Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-16 17:56                           ` Jason Gunthorpe
@ 2017-02-16 18:11                             ` Moritz Fischer
  0 siblings, 0 replies; 63+ messages in thread
From: Moritz Fischer @ 2017-02-16 18:11 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Alan Tull, matthew.gerlach, linux-kernel, linux-fpga

On Thu, Feb 16, 2017 at 9:56 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Thu, Feb 16, 2017 at 11:47:08AM -0600, Alan Tull wrote:
>
>> > Just to clarify: I was proposing using the binary format of dts,
>> > not actually requiring devicetree for it to work. There's plenty
>> > of people running u-boot on x86 using FIT images to boot.
>>
>> The FPGA images should not be required to have OS specific parts.
>> Some ahem non-Linux OS's that use FPGAs don't use device tree, so that
>> adds an extra complication for them unnecessarily.

That's a good point, I had assumed that pulling in a C library shouldn't
be an issue for any OS (u-boot can do it, BSD can do it, Linux can do it).
I have to admit I didn't think about Windows :)

> Not just that, but we parse the bitfile headers in user space as well.
>
> Requiring people to use libfdt pretty much kills the idea because of
> its GPL license.

<snip>

libfdt, however, is GPL/BSD dual-licensed.  That is, it may be used
either under the terms of the GPL, or under the terms of the 2-clause
BSD license (aka the ISC license).  The full terms of that license are
given in the copyright banners of each of the libfdt source files.
This is, in practice, equivalent to being BSD licensed, since the
terms of the BSD license are strictly more permissive than the GPL.

</snip>

> As I've shown the plain text headers can be produced in a scripting
> langauge, and are trivially consumed without much trouble. IHMO this
> makes it more likely there would be adoption..

If you provide a reasonably well documented format and make your tools
easy to use and integrate, I don't think that would be an issue.

I was really mainly concerned about the parsing userspace provided
strings in kernel being a security issue.

If everyone else feels http style plain text is the best we can do, so be it.

I wanted to look around and see how far I get with fdt, but anyway I'm quite
busy with other work at the moment. If someone comes around and writes
code that we can review, maybe that's better than not having a solution.

I'll see how far I get, maybe it turns out my proposal is a bad idea anyways
once I start writing actual code :)

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager
  2017-02-15 16:14 ` [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager Alan Tull
@ 2017-02-17 17:14   ` Li, Yi
  2017-02-17 21:55     ` Alan Tull
  2017-02-17 17:52   ` Moritz Fischer
  1 sibling, 1 reply; 63+ messages in thread
From: Li, Yi @ 2017-02-17 17:14 UTC (permalink / raw)
  To: Alan Tull, Moritz Fischer, Jason Gunthorpe; +Cc: linux-kernel, linux-fpga

hi Alan


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

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

Thanks,
Yi

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

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager
  2017-02-15 16:14 ` [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager Alan Tull
  2017-02-17 17:14   ` Li, Yi
@ 2017-02-17 17:52   ` Moritz Fischer
  2017-02-17 22:02     ` Alan Tull
  1 sibling, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-17 17:52 UTC (permalink / raw)
  To: Alan Tull; +Cc: Jason Gunthorpe, Linux Kernel Mailing List, linux-fpga

Alan,

small nits inline

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

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

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

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

Cheers,

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager
  2017-02-17 17:14   ` Li, Yi
@ 2017-02-17 21:55     ` Alan Tull
  0 siblings, 0 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-17 21:55 UTC (permalink / raw)
  To: Li, Yi; +Cc: Moritz Fischer, Jason Gunthorpe, linux-kernel, linux-fpga

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

Hi Yi,

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

Alan

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

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager
  2017-02-17 17:52   ` Moritz Fischer
@ 2017-02-17 22:02     ` Alan Tull
  0 siblings, 0 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-17 22:02 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Jason Gunthorpe, Linux Kernel Mailing List, linux-fpga

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

Hi Moritz,

Thanks for the review!

Alan

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

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-15 20:54               ` Moritz Fischer
  2017-02-15 21:15                 ` Jason Gunthorpe
@ 2017-02-17 22:28                 ` Yves Vandervennet
  2017-02-18  2:30                   ` Moritz Fischer
  1 sibling, 1 reply; 63+ messages in thread
From: Yves Vandervennet @ 2017-02-17 22:28 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Jason Gunthorpe, matthew.gerlach, Alan Tull, linux-kernel, linux-fpga

Moritz,

  whatever solution we decide to go with has to work with other OS'es. The 
last thing we want to do is to have wrappers that are Linux specific.

Yves

On Wed, 15 Feb 2017, Moritz Fischer wrote:

>>Hi Jason,
>>
>>On Wed, Feb 15, 2017 at 12:37 PM, Jason Gunthorpe
>><jgunthorpe@obsidianresearch.com> wrote:
>>> On Wed, Feb 15, 2017 at 12:07:15PM -0800, matthew.gerlach@linux.intel.com wrote:
>>>
>>>> The format of the meta data associated with a fpga bitstream is certainly a
>>>> subject on its own.  HTTP style plain text is definately easy to understand
>>>> and more importantly it is extendable.  On the other hand, it seems
>>>> dangerous to be doing a lot of string parsing in the kernel.
>>>
>>> It is fairly close to binary parsing.. The process is
>>>
>>> - Find the first occurance of \n\n, must be less than XX bytes
>>> - Memcpy that from the sg list into a linear buffer
>>> - Replace all \n with \0
>>>
>>> To access a key:
>>> - Case insensitive search for START + "Key: " or \0 + "Key: "
>>> - Return as a string the part after the match
>>>
>>> This isn't the sort of string parsing that typically gets you into
>>> trouble. If we can't code the above correctly then we will screw up
>>> safe binary parsing of strings too :)
>>
>>Well I don't know ;-) With something fdt based we already have parsers there,
>>compilers are already in tree. I'll take another look at the u-boot
>>code, I think their
>>FIT (Flattened Image Tree) would be a fairly good match for what we're
>>trying to do.
>>
>>Cheers,
>>Moritz
>>--
>>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
>>

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-17 22:28                 ` Yves Vandervennet
@ 2017-02-18  2:30                   ` Moritz Fischer
  2017-02-18 12:45                     ` Nadathur, Sundar
  0 siblings, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-18  2:30 UTC (permalink / raw)
  To: Yves Vandervennet
  Cc: Jason Gunthorpe, matthew.gerlach, Alan Tull, linux-kernel, linux-fpga

On Fri, Feb 17, 2017 at 04:28:37PM -0600, Yves Vandervennet wrote:
> Moritz,
> 
>   whatever solution we decide to go with has to work with other OS'es. The 
> last thing we want to do is to have wrappers that are Linux specific.

I do agree that we should make sure the format is reasonably well
documented. In my earlier email I pointed out several projects
successfully integrating libfdt.
There's nothing Linux specific about libfdt. FreeBSD uses it, U-Boot,
Qemu ...

I know nothing about how windows kernel development works, but I assume
however one goes about making FPGA programming work there, someone will
most likely have to write a kernel mode driver to take the job of the
fpga-mgr framework.
I assume this will be written in C or C++ or whatever people use these
days for kernel development so pulling in libfdt shouldn't be too hard
if we were to try that.

To be clear:
I did not suggest fdt to make it hard for other OSs, or because this is
my personal pet project. I think we're more likely to get it right by
reusing an existing format, with parsers that other people already
successfully use. It does not have to be fdt (I suggested that because
that was around), but  I do think we certainly can do better than
HTTP-eque plaintext headers.

Thanks,

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* RE: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-18  2:30                   ` Moritz Fischer
@ 2017-02-18 12:45                     ` Nadathur, Sundar
  2017-02-18 20:10                       ` Alan Tull
  0 siblings, 1 reply; 63+ messages in thread
From: Nadathur, Sundar @ 2017-02-18 12:45 UTC (permalink / raw)
  To: Moritz Fischer, Yves Vandervennet
  Cc: Jason Gunthorpe, matthew.gerlach, Alan Tull, linux-kernel, linux-fpga

On February 17, 2017 6:30 PM, Moritz Fischer wrote:
<quote>

On Fri, Feb 17, 2017 at 04:28:37PM -0600, Yves Vandervennet wrote:
> Moritz,
> 
>   whatever solution we decide to go with has to work with other OS'es. 
> The last thing we want to do is to have wrappers that are Linux specific.

I do agree that we should make sure the format is reasonably well documented. In my earlier email I pointed out several projects successfully integrating libfdt.
There's nothing Linux specific about libfdt. FreeBSD uses it, U-Boot, Qemu ...

I know nothing about how windows kernel development works, but I assume however one goes about making FPGA programming work there, someone will most likely have to write a kernel mode driver to take the job of the fpga-mgr framework.
I assume this will be written in C or C++ or whatever people use these days for kernel development so pulling in libfdt shouldn't be too hard if we were to try that.

To be clear:
I did not suggest fdt to make it hard for other OSs, or because this is my personal pet project. I think we're more likely to get it right by reusing an existing format, with parsers that other people already successfully use. It does not have to be fdt (I suggested that because that was around), but  I do think we certainly can do better than HTTP-eque plaintext headers.

Thanks,

Moritz
--
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
</endquote>

Hi all,
   Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples.

To elaborate, a TLV-based format has many advantages:
* It is highly extensible in many ways
   -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time.
   -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values.
   -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges.
* It is OS-independent.
* It can be easily parsed, in kernel or user space.
* It can be validated, in terms of Type values, acceptable lengths, etc.

It  is not directly human-readable but that can be easily addressed with a tool that parses TLVs. 

Compared to some other proposals:
* Compared to DTs, TLVs are OS-independent.
* Compared to strings as key-value pairs, TLVs can express structures/arrays, can be validated, etc. 

So, I suggest we use TLVs to express metadata in image files.

Thank you very much,
Sundar Nadathur 

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-18 12:45                     ` Nadathur, Sundar
@ 2017-02-18 20:10                       ` Alan Tull
  2017-02-18 20:45                         ` Moritz Fischer
  0 siblings, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-18 20:10 UTC (permalink / raw)
  To: Nadathur, Sundar
  Cc: Moritz Fischer, Yves Vandervennet, Jason Gunthorpe,
	matthew.gerlach, linux-kernel, linux-fpga

On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar
<sundar.nadathur@intel.com> wrote:

> Hi all,
>    Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples.
>
> To elaborate, a TLV-based format has many advantages:
> * It is highly extensible in many ways
>    -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time.
>    -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values.
>    -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges.
> * It is OS-independent.
> * It can be easily parsed, in kernel or user space.
> * It can be validated, in terms of Type values, acceptable lengths, etc.
>
> It  is not directly human-readable but that can be easily addressed with a tool that parses TLVs.
>
> Compared to some other proposals:
> * Compared to DTs, TLVs are OS-independent.
> * Compared to strings as key-value pairs, TLVs can express structures/arrays, can be validated, etc.
>
> So, I suggest we use TLVs to express metadata in image files.
>
> Thank you very much,
> Sundar Nadathur

Hi Sundar,

IIUC, each field is position dependent.  One of the strengths of key
value pairs is
that any key can be added in any order and ones that aren't applicable for
a particular architecture or use case can be left out.  If the header
is position
dependent, it becomes less flexible.  Once a field is added, we are
stuck with it
forever unless we drop it in some incremented version of the header format.

Alan

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-18 20:10                       ` Alan Tull
@ 2017-02-18 20:45                         ` Moritz Fischer
  2017-02-19 15:00                           ` Alan Tull
  0 siblings, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-18 20:45 UTC (permalink / raw)
  To: Alan Tull
  Cc: Nadathur, Sundar, Yves Vandervennet, Jason Gunthorpe,
	matthew.gerlach, linux-kernel, linux-fpga

On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote:
> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar
> <sundar.nadathur@intel.com> wrote:
> 
> > Hi all,
> >    Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples.
> >
> > To elaborate, a TLV-based format has many advantages:
> > * It is highly extensible in many ways
> >    -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time.
> >    -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values.
> >    -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges.
> > * It is OS-independent.
> > * It can be easily parsed, in kernel or user space.

Are there other users of the format? I have to admit I didn't look very
long, but couldn't find any libs / existing code at a first glance.

> > * It can be validated, in terms of Type values, acceptable lengths, etc.
> >
> > It  is not directly human-readable but that can be easily addressed with a tool that parses TLVs.
> >
> > Compared to some other proposals:
> > * Compared to DTs, TLVs are OS-independent.

That's just alternative facts here. Just because Linux uses fdt for
devicetree blobs it is *not* OS dependent. There are several (see
last email) non-Linux users of fdt / libfdt.

Thanks,

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-18 20:45                         ` Moritz Fischer
@ 2017-02-19 15:00                           ` Alan Tull
  2017-02-19 23:16                             ` Alan Tull
  0 siblings, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-19 15:00 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Nadathur, Sundar, Yves Vandervennet, Jason Gunthorpe,
	matthew.gerlach, linux-kernel, linux-fpga

On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:
> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote:
>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar
>> <sundar.nadathur@intel.com> wrote:
>>
>> > Hi all,
>> >    Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples.
>> >
>> > To elaborate, a TLV-based format has many advantages:
>> > * It is highly extensible in many ways
>> >    -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time.

Device tree can express arrays.

>> >    -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values.
>> >    -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges.
>> > * It is OS-independent.
>> > * It can be easily parsed, in kernel or user space.
>
> Are there other users of the format? I have to admit I didn't look very
> long, but couldn't find any libs / existing code at a first glance.

Is there a standard you are looking at?  Have you seen any use of TLV's
in the Linux kernel you could point to?

>
>> > * It can be validated, in terms of Type values, acceptable lengths, etc.
>> >
>> > It  is not directly human-readable but that can be easily addressed with a tool that parses TLVs.
>> >
>> > Compared to some other proposals:
>> > * Compared to DTs, TLVs are OS-independent.
>
> That's just alternative facts here. Just because Linux uses fdt for
> devicetree blobs it is *not* OS dependent. There are several (see
> last email) non-Linux users of fdt / libfdt.
>
> Thanks,
>
> Moritz

It is worth repeating that libdtc is GPL/BSD with the intent of
allowing proprietary code to use libdtc.  So license shouldn't be a barrier.

Using device tree in the header would give us a way of doing enumeration at
least for Linux, not sure if that kind of info can be used in Windows
in some way.

Alan

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-19 15:00                           ` Alan Tull
@ 2017-02-19 23:16                             ` Alan Tull
  2017-02-20 23:49                               ` Moritz Fischer
  0 siblings, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-19 23:16 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Nadathur, Sundar, Yves Vandervennet, Jason Gunthorpe,
	matthew.gerlach, linux-kernel, linux-fpga

On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> wrote:
> On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer
> <moritz.fischer@ettus.com> wrote:
>> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote:
>>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar
>>> <sundar.nadathur@intel.com> wrote:
>>>
>>> > Hi all,
>>> >    Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples.
>>> >
>>> > To elaborate, a TLV-based format has many advantages:
>>> > * It is highly extensible in many ways
>>> >    -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time.
>
> Device tree can express arrays.
>
>>> >    -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values.
>>> >    -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges.
>>> > * It is OS-independent.
>>> > * It can be easily parsed, in kernel or user space.
>>
>> Are there other users of the format? I have to admit I didn't look very
>> long, but couldn't find any libs / existing code at a first glance.
>
> Is there a standard you are looking at?  Have you seen any use of TLV's
> in the Linux kernel you could point to?
>
>>
>>> > * It can be validated, in terms of Type values, acceptable lengths, etc.
>>> >
>>> > It  is not directly human-readable but that can be easily addressed with a tool that parses TLVs.
>>> >
>>> > Compared to some other proposals:
>>> > * Compared to DTs, TLVs are OS-independent.
>>
>> That's just alternative facts here. Just because Linux uses fdt for
>> devicetree blobs it is *not* OS dependent. There are several (see
>> last email) non-Linux users of fdt / libfdt.
>>
>> Thanks,
>>
>> Moritz
>
> It is worth repeating that libdtc is GPL/BSD with the intent of
> allowing proprietary code to use libdtc.  So license shouldn't be a barrier.
>
> Using device tree in the header would give us a way of doing enumeration at
> least for Linux, not sure if that kind of info can be used in Windows
> in some way.

Actually, enumeration is the only advantage I see with DT.

Currently I like key/value pairs because they are easily implemented
and expandable without being rigid in any way.

If we use key/value pairs, we could pass in child device info
in one of the keys.  It could be either a device tree overlay or an
ACPI overlay.  Or could just be left out.  So platforms that
aren't already using DT wouldn't have to.  Platforms that
are have a smooth road to enumeration.

Alan

>
> Alan

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-19 23:16                             ` Alan Tull
@ 2017-02-20 23:49                               ` Moritz Fischer
  2017-02-21 18:33                                 ` Alan Tull
  0 siblings, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-20 23:49 UTC (permalink / raw)
  To: Alan Tull
  Cc: Nadathur, Sundar, Yves Vandervennet, Jason Gunthorpe,
	matthew.gerlach, linux-kernel, linux-fpga, Marek Vašut

Hi Alan,

On Sun, Feb 19, 2017 at 3:16 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>> On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer
>> <moritz.fischer@ettus.com> wrote:
>>> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote:
>>>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar
>>>> <sundar.nadathur@intel.com> wrote:
>>>>
>>>> > Hi all,
>>>> >    Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples.
>>>> >
>>>> > To elaborate, a TLV-based format has many advantages:
>>>> > * It is highly extensible in many ways
>>>> >    -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time.
>>
>> Device tree can express arrays.
>>
>>>> >    -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values.
>>>> >    -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges.
>>>> > * It is OS-independent.
>>>> > * It can be easily parsed, in kernel or user space.
>>>
>>> Are there other users of the format? I have to admit I didn't look very
>>> long, but couldn't find any libs / existing code at a first glance.
>>
>> Is there a standard you are looking at?  Have you seen any use of TLV's
>> in the Linux kernel you could point to?
>>
>>>
>>>> > * It can be validated, in terms of Type values, acceptable lengths, etc.
>>>> >
>>>> > It  is not directly human-readable but that can be easily addressed with a tool that parses TLVs.
>>>> >
>>>> > Compared to some other proposals:
>>>> > * Compared to DTs, TLVs are OS-independent.
>>>
>>> That's just alternative facts here. Just because Linux uses fdt for
>>> devicetree blobs it is *not* OS dependent. There are several (see
>>> last email) non-Linux users of fdt / libfdt.
>>>
>>> Thanks,
>>>
>>> Moritz
>>
>> It is worth repeating that libdtc is GPL/BSD with the intent of
>> allowing proprietary code to use libdtc.  So license shouldn't be a barrier.
>>
>> Using device tree in the header would give us a way of doing enumeration at
>> least for Linux, not sure if that kind of info can be used in Windows
>> in some way.
>
> Actually, enumeration is the only advantage I see with DT.

Which seems to some point a separate issue to passing in image
specific info such as
encrypted or not, compressed or not or build info metadata.

So I think in general we can still separate this out into:
- Image specific values
- Reconfiguration specific values

> Currently I like key/value pairs because they are easily implemented
> and expandable without being rigid in any way.
>
> If we use key/value pairs, we could pass in child device info
> in one of the keys.  It could be either a device tree overlay or an
> ACPI overlay.  Or could just be left out.  So platforms that
> aren't already using DT wouldn't have to.  Platforms that
> are have a smooth road to enumeration.

I'm not sure if you can bundle up enumeration info *with* the image
since you might e.g.
load the same image (i.e. same header) into different FPGAs and the
required update to
the kernel state, i.e. live tree or ACPI would depend entirely on
which FPGA you loaded
the image into w.r.t busses it's connected to etc.

I do think this info cannot be image specific, but needs to be passed
in via something
external such as a dt overlay.

Cheers,

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-20 23:49                               ` Moritz Fischer
@ 2017-02-21 18:33                                 ` Alan Tull
  2017-02-22  3:13                                   ` Nadathur, Sundar
  2017-02-27 20:09                                   ` Alan Tull
  0 siblings, 2 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-21 18:33 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Nadathur, Sundar, Yves Vandervennet, Jason Gunthorpe,
	matthew.gerlach, linux-kernel, linux-fpga, Marek Vašut

On Mon, Feb 20, 2017 at 5:49 PM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:
> Hi Alan,
>
> On Sun, Feb 19, 2017 at 3:16 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>>> On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer
>>> <moritz.fischer@ettus.com> wrote:
>>>> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote:
>>>>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar
>>>>> <sundar.nadathur@intel.com> wrote:
>>>>>
>>>>> > Hi all,
>>>>> >    Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples.
>>>>> >
>>>>> > To elaborate, a TLV-based format has many advantages:
>>>>> > * It is highly extensible in many ways
>>>>> >    -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time.
>>>
>>> Device tree can express arrays.
>>>
>>>>> >    -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values.
>>>>> >    -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges.
>>>>> > * It is OS-independent.
>>>>> > * It can be easily parsed, in kernel or user space.
>>>>
>>>> Are there other users of the format? I have to admit I didn't look very
>>>> long, but couldn't find any libs / existing code at a first glance.
>>>
>>> Is there a standard you are looking at?  Have you seen any use of TLV's
>>> in the Linux kernel you could point to?
>>>
>>>>
>>>>> > * It can be validated, in terms of Type values, acceptable lengths, etc.
>>>>> >
>>>>> > It  is not directly human-readable but that can be easily addressed with a tool that parses TLVs.
>>>>> >
>>>>> > Compared to some other proposals:
>>>>> > * Compared to DTs, TLVs are OS-independent.
>>>>
>>>> That's just alternative facts here. Just because Linux uses fdt for
>>>> devicetree blobs it is *not* OS dependent. There are several (see
>>>> last email) non-Linux users of fdt / libfdt.
>>>>
>>>> Thanks,
>>>>
>>>> Moritz
>>>
>>> It is worth repeating that libdtc is GPL/BSD with the intent of
>>> allowing proprietary code to use libdtc.  So license shouldn't be a barrier.
>>>
>>> Using device tree in the header would give us a way of doing enumeration at
>>> least for Linux, not sure if that kind of info can be used in Windows
>>> in some way.
>>
>> Actually, enumeration is the only advantage I see with DT.
>
> Which seems to some point a separate issue to passing in image
> specific info such as
> encrypted or not, compressed or not or build info metadata.
>
> So I think in general we can still separate this out into:
> - Image specific values
> - Reconfiguration specific values
>
>> Currently I like key/value pairs because they are easily implemented
>> and expandable without being rigid in any way.
>>
>> If we use key/value pairs, we could pass in child device info
>> in one of the keys.  It could be either a device tree overlay or an
>> ACPI overlay.  Or could just be left out.  So platforms that
>> aren't already using DT wouldn't have to.  Platforms that
>> are have a smooth road to enumeration.
>
> I'm not sure if you can bundle up enumeration info *with* the image
> since you might e.g.
> load the same image (i.e. same header) into different FPGAs and the
> required update to
> the kernel state, i.e. live tree or ACPI would depend entirely on
> which FPGA you loaded
> the image into w.r.t busses it's connected to etc.
>
> I do think this info cannot be image specific, but needs to be passed
> in via something
> external such as a dt overlay.

Yes, I think you are right about that.

Thanks!
Alan

>
> Cheers,
>
> Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* RE: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-21 18:33                                 ` Alan Tull
@ 2017-02-22  3:13                                   ` Nadathur, Sundar
  2017-02-22  3:49                                     ` Moritz Fischer
  2017-02-27 20:09                                   ` Alan Tull
  1 sibling, 1 reply; 63+ messages in thread
From: Nadathur, Sundar @ 2017-02-22  3:13 UTC (permalink / raw)
  To: Alan Tull, Moritz Fischer
  Cc: Yves Vandervennet, Jason Gunthorpe, matthew.gerlach,
	linux-kernel, linux-fpga, Marek Vašut



> -----Original Message-----
> From: Alan Tull [mailto:delicious.quinoa@gmail.com]
> Sent: Tuesday, February 21, 2017 10:33 AM
> To: Moritz Fischer <moritz.fischer@ettus.com>
> Cc: Nadathur, Sundar <sundar.nadathur@intel.com>; Yves Vandervennet
> <yves.vandervennet@linux.intel.com>; Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com>; matthew.gerlach@linux.intel.com;
> linux-kernel <linux-kernel@vger.kernel.org>; linux-fpga@vger.kernel.org;
> Marek Vašut <marex@denx.de>
> Subject: Re: [RFC 7/8] fpga-region: add sysfs interface
> 
> On Mon, Feb 20, 2017 at 5:49 PM, Moritz Fischer
> <moritz.fischer@ettus.com> wrote:
> > Hi Alan,
> >
> > On Sun, Feb 19, 2017 at 3:16 PM, Alan Tull <delicious.quinoa@gmail.com>
> wrote:
> >> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com>
> wrote:
> >>> On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer
> >>> <moritz.fischer@ettus.com> wrote:
> >>>> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote:
> >>>>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar
> >>>>> <sundar.nadathur@intel.com> wrote:
> >>>>>
> >>>>> > Hi all,
> >>>>> >    Interesting discussion. The discussion so far has brought out many
> concerns such as OS independence. There is an existing format, well-known
> to developers, with widespread support, and which is quite extensible: Type-
> Length-Value triples.
> >>> [...]
> >>>> Are there other users of the format? I have to admit I didn't look
> >>>> very long, but couldn't find any libs / existing code at a first glance.
> >>>
> >>> Is there a standard you are looking at?  Have you seen any use of
> >>> TLV's in the Linux kernel you could point to?	

Here are some examples of TLVs in the Linux kernel:
http://lxr.free-electrons.com/source/net/ipv6/exthdrs.c <-- includes TLV parsing code
http://lxr.free-electrons.com/source/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c 
http://lxr.free-electrons.com/source/include/sound/tlv.h 

In addition, some protocols like LLDP are defined in terms of TLVs. E.g.
http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/i40e/i40e_dcb.h?v=4.4 

> >> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com>
> wrote:
> >>> It is worth repeating that libdtc is GPL/BSD with the intent of
> >>> allowing proprietary code to use libdtc.  So license shouldn't be a barrier.
It is better to check with Windows folks before concluding this. 

> >> If we use key/value pairs, we could pass in child device info in one
> >> of the keys.  It could be either a device tree overlay or an ACPI
> >> overlay.  Or could just be left out.  So platforms that aren't
> >> already using DT wouldn't have to.  Platforms that are have a smooth
> >> road to enumeration.
> >
> > I'm not sure if you can bundle up enumeration info *with* the image
> > since you might e.g.
> > load the same image (i.e. same header) into different FPGAs and the
> > required update to the kernel state, i.e. live tree or ACPI would
> > depend entirely on which FPGA you loaded the image into w.r.t busses
> > it's connected to etc.
> >
> > I do think this info cannot be image specific, but needs to be passed
> > in via something external such as a dt overlay.
> 
> Yes, I think you are right about that.
> 
> Thanks!
> Alan

I agree that device enumeration should be separated out from the metadata format considerations. 

I got some feedback that not everybody may be familiar with TLVs. To make the proposal more clear and specific, let me add more information here.
* We represent every datum of interest with its Type (which indicates what it is), a Length (how many bytes it takes) and a Value (its actual value, taking as many bytes as the Length field indicates.)  
* The exact lengths of the Type and Length fields are up to us, but let us say they are 4 bytes each, for concreteness. As an example, say we want to express the function in the FPGA (crypto, compress, etc.) as a UUID (128 bits long) compliant with RFC 4122. We could have a Type of say 0x00000050 (4 bytes in all) to indicate Function UUIDs, and a Length field of 0x00000010 (16 bytes) and a value of say 3d8814d8-4ecc-4030-8415-0dea4e5e829a . 
* A Type may indicate that its value is another TLV, thus allowing nested TLVs. The nested TLV may be an array of TLVs (all of same Type) or a structure (TLVs of different Types). 

With a Key-Value Pair, if the parser comes across an unknown key (such as when an old parser comes across newer metadata), it would not know how to skip that KVP and move onto the next one. With TLVs, that flexibility is built in. Further, we can use bits in the Type field to indicate that it is mandatory, i.e., if the parser does not understand it, it should error out rather than skip it silently. This degree of forward compatibility is difficult to achieve with other formats. 

Thanks,
Sundar

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22  3:13                                   ` Nadathur, Sundar
@ 2017-02-22  3:49                                     ` Moritz Fischer
  2017-02-22  5:12                                       ` Jason Gunthorpe
  0 siblings, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-22  3:49 UTC (permalink / raw)
  To: Nadathur, Sundar
  Cc: Alan Tull, Yves Vandervennet, Jason Gunthorpe, matthew.gerlach,
	linux-kernel, linux-fpga, Marek Vašut

Hi Sundar,

On Tue, Feb 21, 2017 at 7:13 PM, Nadathur, Sundar
<sundar.nadathur@intel.com> wrote:

>> >>> Is there a standard you are looking at?  Have you seen any use of
>> >>> TLV's in the Linux kernel you could point to?
>
> Here are some examples of TLVs in the Linux kernel:
> http://lxr.free-electrons.com/source/net/ipv6/exthdrs.c <-- includes TLV parsing code
> http://lxr.free-electrons.com/source/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c
> http://lxr.free-electrons.com/source/include/sound/tlv.h
>
> In addition, some protocols like LLDP are defined in terms of TLVs. E.g.
> http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/i40e/i40e_dcb.h?v=4.4

Thanks for the examples, looks interesting. I'll do some reading.

>> >> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com>
>> wrote:
>> >>> It is worth repeating that libdtc is GPL/BSD with the intent of
>> >>> allowing proprietary code to use libdtc.  So license shouldn't be a barrier.
> It is better to check with Windows folks before concluding this.

About whether they can link C code? Or whether BSD licensed code is an issue?

> I agree that device enumeration should be separated out from the metadata format considerations.
>
> I got some feedback that not everybody may be familiar with TLVs. To make the proposal more clear and specific, let me add more information here.
> * We represent every datum of interest with its Type (which indicates what it is), a Length (how many bytes it takes) and a Value (its actual value, taking as many bytes as the Length field indicates.)
> * The exact lengths of the Type and Length fields are up to us, but let us say they are 4 bytes each, for concreteness. As an example, say we want to express the function in the FPGA (crypto, compress, etc.) as a UUID (128 bits long) compliant with RFC 4122. We could have a Type of say 0x00000050 (4 bytes in all) to indicate Function UUIDs, and a Length field of 0x00000010 (16 bytes) and a value of say 3d8814d8-4ecc-4030-8415-0dea4e5e829a .
> * A Type may indicate that its value is another TLV, thus allowing nested TLVs. The nested TLV may be an array of TLVs (all of same Type) or a structure (TLVs of different Types).
>
> With a Key-Value Pair, if the parser comes across an unknown key (such as when an old parser comes across newer metadata), it would not know how to skip that KVP and move onto the next one. With TLVs, that flexibility is built in. Further, we can use bits in the Type field to indicate that it is mandatory, i.e., if the parser does not understand it, it should error out rather than skip it silently. This degree of forward compatibility is difficult to achieve with other formats.

fdt does this out of the box, too. So far I've seen nothing fdt
couldn't do (or doesn't do let's rather say).

Thanks for clarifying the TLV stuff,

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22  3:49                                     ` Moritz Fischer
@ 2017-02-22  5:12                                       ` Jason Gunthorpe
  2017-02-22  5:38                                         ` Moritz Fischer
  0 siblings, 1 reply; 63+ messages in thread
From: Jason Gunthorpe @ 2017-02-22  5:12 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Nadathur, Sundar, Alan Tull, Yves Vandervennet, matthew.gerlach,
	linux-kernel, linux-fpga, Marek Va??ut

On Tue, Feb 21, 2017 at 07:49:19PM -0800, Moritz Fischer wrote:

> fdt does this out of the box, too. So far I've seen nothing fdt
> couldn't do (or doesn't do let's rather say).

tlv/fdt/http headers are all essentially exactly the same
thing. Key/value pairs with various encoding schemes.

I don't think we don't need a tree of data, so fdt is overkill.

tlv is not substantially easier to parse correctly than the
structured plain text headers.. It is just in binary so it can
represent binary-ish things better.

So far the only thing we know we need is a 'bool' for encrypted and a
stringish guid thing for partial reconfiguration.

The other stuff I've always used is pretty much all textual.

Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22  5:12                                       ` Jason Gunthorpe
@ 2017-02-22  5:38                                         ` Moritz Fischer
  2017-02-22  5:46                                           ` Nadathur, Sundar
  2017-02-22 16:33                                           ` Alan Tull
  0 siblings, 2 replies; 63+ messages in thread
From: Moritz Fischer @ 2017-02-22  5:38 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Nadathur, Sundar, Alan Tull, Yves Vandervennet, matthew.gerlach,
	linux-kernel, linux-fpga, Marek Va??ut

Hi all,

On Tue, Feb 21, 2017 at 9:12 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Tue, Feb 21, 2017 at 07:49:19PM -0800, Moritz Fischer wrote:
>
>> fdt does this out of the box, too. So far I've seen nothing fdt
>> couldn't do (or doesn't do let's rather say).
>
> tlv/fdt/http headers are all essentially exactly the same
> thing. Key/value pairs with various encoding schemes.
>
> I don't think we don't need a tree of data, so fdt is overkill.
>
> tlv is not substantially easier to parse correctly than the
> structured plain text headers.. It is just in binary so it can
> represent binary-ish things better.

TLV Seems easy enough. To give an update, I played with fdt a bit to see
how far I get in half an hour. I got bool / int / strings to work
quite fast (~30mins).
Please disregard the horrible hackyness of this ...

For simplicity I stuck the header on top of my bitfile with:

<snip>
/dts-v1/;

/{
        description = "Test";
        compressed = <0>;
        encrypted = <1>;
};
</snip>

$ dtc -o header.dtb header.dts

$ cat header.dtb mybitfile.bin > /lib/firmware/bitfile_header.bin

+ static int __fpga_mgr_blob_to_image_info(const void *blob,
+                                          struct fpga_image_info *info)
+ {
+         int root_offset;
+         const char *desc;
+         const uint32_t *_compressed, *_encrypted;
+         int compressed, encrypted;
+
+         if (fdt_check_header(blob)) {
+                 pr_err("Invalid device tree blob header\n");
+                 return -EINVAL;
+         }
+
+         root_offset = fdt_path_offset(blob, "/");
+         if (root_offset < 0) {
+                 pr_err("Invalid root offset\n");
+                 return -EINVAL;
+         }
+
+         desc = fdt_getprop(blob, root_offset, "description", NULL);
+
+         _compressed = fdt_getprop(blob, root_offset, "compressed", NULL);
+         if (_compressed)
+                 compressed = fdt32_to_cpu(*_compressed);
+
+         _encrypted = fdt_getprop(blob, root_offset, "encrypted", NULL);
+         if (_encrypted)
+                 encrypted = fdt32_to_cpu(*_encrypted);
+
+         if (desc)
+                 pr_info("%s: description=%s\n", __func__, desc);
+
+         if (_encrypted && _compressed)
+                 pr_info("%s: compressed? %s encrypted? %s\n", __func__,
+                         compressed ? "Yes" : "No", encrypted ? "Yes" : "No");
+
+         return 0;
+ }

Which gave me:

<snip>

[   19.325182] fpga_manager fpga0: writing bitfile_header.bin to
Xilinx Zynq FPGA Manager
[   20.091222] __fpga_mgr_blob_to_image_info: description=Test
[   20.096730] __fpga_mgr_blob_to_image_info: compressed? No encrypted? Yes

</snip>

So I'm fairly convinced I can make this work, TVLs seem like it could work, too.

> So far the only thing we know we need is a 'bool' for encrypted and a
> stringish guid thing for partial reconfiguration.

Yeah, shouldn't be hard to implement either way.

Cheers,

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* RE: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22  5:38                                         ` Moritz Fischer
@ 2017-02-22  5:46                                           ` Nadathur, Sundar
  2017-02-22  6:05                                             ` Moritz Fischer
  2017-02-22 16:33                                           ` Alan Tull
  1 sibling, 1 reply; 63+ messages in thread
From: Nadathur, Sundar @ 2017-02-22  5:46 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe
  Cc: Alan Tull, Yves Vandervennet, matthew.gerlach, linux-kernel,
	linux-fpga, Marek Va??ut

On February 21, 2017 9:39 PM, Moritz Fischer wrote:

> TLV Seems easy enough. To give an update, I played with fdt a bit to see how
> far I get in half an hour. I got bool / int / strings to work quite fast (~30mins).
> Please disregard the horrible hackyness of this ...
> [...]
> So I'm fairly convinced I can make this work, TVLs seem like it could work,
> too.
> 
> > So far the only thing we know we need is a 'bool' for encrypted and a
> > stringish guid thing for partial reconfiguration.

These things have a way of growing beyond their original anticipated needs. 

> Yeah, shouldn't be hard to implement either way.
> 
> Cheers,
> 
> Moritz

Say we upstream a metadata parser. Subsequently, an FPGA image is released with an additional metadata field that the upstreamed version does not handle. How will this be handled if the metadata were in FDT or KVP format?

Thanks,
Sundar

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22  5:46                                           ` Nadathur, Sundar
@ 2017-02-22  6:05                                             ` Moritz Fischer
  2017-02-22 16:44                                               ` Jason Gunthorpe
  0 siblings, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-22  6:05 UTC (permalink / raw)
  To: Nadathur, Sundar
  Cc: Jason Gunthorpe, Alan Tull, Yves Vandervennet, matthew.gerlach,
	linux-kernel, linux-fpga, Marek Va??ut

On Tue, Feb 21, 2017 at 9:46 PM, Nadathur, Sundar
<sundar.nadathur@intel.com> wrote:
> On February 21, 2017 9:39 PM, Moritz Fischer wrote:
>
>> TLV Seems easy enough. To give an update, I played with fdt a bit to see how
>> far I get in half an hour. I got bool / int / strings to work quite fast (~30mins).
>> Please disregard the horrible hackyness of this ...
>> [...]
>> So I'm fairly convinced I can make this work, TVLs seem like it could work,
>> too.
>>
>> > So far the only thing we know we need is a 'bool' for encrypted and a
>> > stringish guid thing for partial reconfiguration.
>
> These things have a way of growing beyond their original anticipated needs.

True. But yeah, not sure about the requirement for a tree, maybe it is overkill.

> Say we upstream a metadata parser. Subsequently, an FPGA image is released with an additional metadata field that the upstreamed version does not handle. How will this be handled if the metadata were in FDT or KVP format?

The code above will gently ignore it, as I said I spent about half an hour on
writing that, just to prove to myself it can be done easily.
Logically I don't see anything wrong with ignoring features from the future.
But if one insisted one could make a compatibility number part of the
required properties I suppose and error out instead. There are examples
of optional properties in the devicetree parsing code in the kernel.

That being said older drivers / fpga-mgr  will not deal with newer features.
TLV / KV or whatever doesn't change this fact, or am I missing something?

Adding new properties to devicetrees is a well known exercise to cope with
newer versions or variations of hardware and happens all the time in the kernel.
Older kernels will just ignore them.

Thanks,

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22  5:38                                         ` Moritz Fischer
  2017-02-22  5:46                                           ` Nadathur, Sundar
@ 2017-02-22 16:33                                           ` Alan Tull
  2017-02-22 16:44                                             ` Moritz Fischer
  1 sibling, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-22 16:33 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Jason Gunthorpe, Nadathur, Sundar, Yves Vandervennet,
	matthew.gerlach, linux-kernel, linux-fpga, Marek Va??ut

On Tue, Feb 21, 2017 at 11:38 PM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:

Hi Moritz,

> Hi all,
>
> On Tue, Feb 21, 2017 at 9:12 PM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
>> On Tue, Feb 21, 2017 at 07:49:19PM -0800, Moritz Fischer wrote:
>>
>>> fdt does this out of the box, too. So far I've seen nothing fdt
>>> couldn't do (or doesn't do let's rather say).
>>
>> tlv/fdt/http headers are all essentially exactly the same
>> thing. Key/value pairs with various encoding schemes.
>>
>> I don't think we don't need a tree of data, so fdt is overkill.
>>
>> tlv is not substantially easier to parse correctly than the
>> structured plain text headers.. It is just in binary so it can
>> represent binary-ish things better.
>
> TLV Seems easy enough. To give an update, I played with fdt a bit to see
> how far I get in half an hour. I got bool / int / strings to work
> quite fast (~30mins).

Thanks for doing this fast piece of exploratory coding.  It does
confirm that for Linux, using fdt is pretty straightforward here.
However...

> Please disregard the horrible hackyness of this ...
>
> For simplicity I stuck the header on top of my bitfile with:
>
> <snip>
> /dts-v1/;
>
> /{
>         description = "Test";
>         compressed = <0>;
>         encrypted = <1>;
> };

I understand that this is a simplified example, but it looks a lot
like KVP which then gets compiled by dtc.

If we do KVP or TLV we get skip using dtc, which would be nice for non-dt
OS's using the same images.

Also, the license of libfdt allows the use by proprietary
os's, but that's not true for dtc.

Alan

> </snip>
>
> $ dtc -o header.dtb header.dts
>
> $ cat header.dtb mybitfile.bin > /lib/firmware/bitfile_header.bin
>
> + static int __fpga_mgr_blob_to_image_info(const void *blob,
> +                                          struct fpga_image_info *info)
> + {
> +         int root_offset;
> +         const char *desc;
> +         const uint32_t *_compressed, *_encrypted;
> +         int compressed, encrypted;
> +
> +         if (fdt_check_header(blob)) {
> +                 pr_err("Invalid device tree blob header\n");
> +                 return -EINVAL;
> +         }
> +
> +         root_offset = fdt_path_offset(blob, "/");
> +         if (root_offset < 0) {
> +                 pr_err("Invalid root offset\n");
> +                 return -EINVAL;
> +         }
> +
> +         desc = fdt_getprop(blob, root_offset, "description", NULL);
> +
> +         _compressed = fdt_getprop(blob, root_offset, "compressed", NULL);
> +         if (_compressed)
> +                 compressed = fdt32_to_cpu(*_compressed);
> +
> +         _encrypted = fdt_getprop(blob, root_offset, "encrypted", NULL);
> +         if (_encrypted)
> +                 encrypted = fdt32_to_cpu(*_encrypted);
> +
> +         if (desc)
> +                 pr_info("%s: description=%s\n", __func__, desc);
> +
> +         if (_encrypted && _compressed)
> +                 pr_info("%s: compressed? %s encrypted? %s\n", __func__,
> +                         compressed ? "Yes" : "No", encrypted ? "Yes" : "No");
> +
> +         return 0;
> + }
>
> Which gave me:
>
> <snip>
>
> [   19.325182] fpga_manager fpga0: writing bitfile_header.bin to
> Xilinx Zynq FPGA Manager
> [   20.091222] __fpga_mgr_blob_to_image_info: description=Test
> [   20.096730] __fpga_mgr_blob_to_image_info: compressed? No encrypted? Yes
>
> </snip>
>
> So I'm fairly convinced I can make this work, TVLs seem like it could work, too.
>
>> So far the only thing we know we need is a 'bool' for encrypted and a
>> stringish guid thing for partial reconfiguration.
>
> Yeah, shouldn't be hard to implement either way.
>
> Cheers,
>
> Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22  6:05                                             ` Moritz Fischer
@ 2017-02-22 16:44                                               ` Jason Gunthorpe
  2017-02-22 17:50                                                 ` Moritz Fischer
  0 siblings, 1 reply; 63+ messages in thread
From: Jason Gunthorpe @ 2017-02-22 16:44 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Nadathur, Sundar, Alan Tull, Yves Vandervennet, matthew.gerlach,
	linux-kernel, linux-fpga, Marek Va??ut

On Tue, Feb 21, 2017 at 10:05:42PM -0800, Moritz Fischer wrote:

> That being said older drivers / fpga-mgr  will not deal with newer features.
> TLV / KV or whatever doesn't change this fact, or am I missing something?

Often a scheme will have an OPTIONAL and REQUIRED flag for each
value. If a REQUIRED value is present but the parser does not
support it then the parse fails.

For instance, we could mark Encrypted as required, and a zynq driver
that does not support the Encrypted tag would not load the bitfile
rather than try to load it wrongly.

This can be forced into any of the approaches with various levels of
hackery.

Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22 16:33                                           ` Alan Tull
@ 2017-02-22 16:44                                             ` Moritz Fischer
  2017-02-22 16:52                                               ` Alan Tull
  0 siblings, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-22 16:44 UTC (permalink / raw)
  To: Alan Tull
  Cc: Jason Gunthorpe, Nadathur, Sundar, Yves Vandervennet,
	matthew.gerlach, linux-kernel, linux-fpga, Marek Va??ut

On Wed, Feb 22, 2017 at 8:33 AM, Alan Tull <delicious.quinoa@gmail.com> wrote:
> On Tue, Feb 21, 2017 at 11:38 PM, Moritz Fischer
> <moritz.fischer@ettus.com> wrote:
>
> Hi Moritz,
>
>> Hi all,
>>
>> On Tue, Feb 21, 2017 at 9:12 PM, Jason Gunthorpe
>> <jgunthorpe@obsidianresearch.com> wrote:
>>> On Tue, Feb 21, 2017 at 07:49:19PM -0800, Moritz Fischer wrote:
>>>
>>>> fdt does this out of the box, too. So far I've seen nothing fdt
>>>> couldn't do (or doesn't do let's rather say).
>>>
>>> tlv/fdt/http headers are all essentially exactly the same
>>> thing. Key/value pairs with various encoding schemes.
>>>
>>> I don't think we don't need a tree of data, so fdt is overkill.
>>>
>>> tlv is not substantially easier to parse correctly than the
>>> structured plain text headers.. It is just in binary so it can
>>> represent binary-ish things better.
>>
>> TLV Seems easy enough. To give an update, I played with fdt a bit to see
>> how far I get in half an hour. I got bool / int / strings to work
>> quite fast (~30mins).
>
> Thanks for doing this fast piece of exploratory coding.  It does
> confirm that for Linux, using fdt is pretty straightforward here.
> However...
>
>> Please disregard the horrible hackyness of this ...
>>
>> For simplicity I stuck the header on top of my bitfile with:
>>
>> <snip>
>> /dts-v1/;
>>
>> /{
>>         description = "Test";
>>         compressed = <0>;
>>         encrypted = <1>;
>> };
>
> I understand that this is a simplified example, but it looks a lot
> like KVP which then gets compiled by dtc.
>
> If we do KVP or TLV we get skip using dtc, which would be nice for non-dt
> OS's using the same images.

I used dtc for pure lazyness. Writing a blob to a file using libfdt is
about as much
code as parsing it. Even with KVP or TLV you have some code that needs
to encode / pack your header into a file.

libfdt has an example that creates an empty tree. Write that to a file, done.

1: Create empty tree
https://github.com/dgibson/dtc/blob/master/libfdt/fdt_empty_tree.c

2: fopen / fwrite, done

> Also, the license of libfdt allows the use by proprietary
> os's, but that's not true for dtc.

Why would that be an issue, you don't need to link anything to run
dtc. That being
said as I pointed out above you don not have to actually use dtc if the values
are known ahead of time (like in our case). What you'd get from using dtc is to
encode arbitrary values (for the types supported).

Cheers,

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22 16:44                                             ` Moritz Fischer
@ 2017-02-22 16:52                                               ` Alan Tull
  0 siblings, 0 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-22 16:52 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Jason Gunthorpe, Nadathur, Sundar, Yves Vandervennet,
	matthew.gerlach, linux-kernel, linux-fpga, Marek Va??ut

On Wed, Feb 22, 2017 at 10:44 AM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:
> On Wed, Feb 22, 2017 at 8:33 AM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>> On Tue, Feb 21, 2017 at 11:38 PM, Moritz Fischer
>> <moritz.fischer@ettus.com> wrote:
>>
>> Hi Moritz,
>>
>>> Hi all,
>>>
>>> On Tue, Feb 21, 2017 at 9:12 PM, Jason Gunthorpe
>>> <jgunthorpe@obsidianresearch.com> wrote:
>>>> On Tue, Feb 21, 2017 at 07:49:19PM -0800, Moritz Fischer wrote:
>>>>
>>>>> fdt does this out of the box, too. So far I've seen nothing fdt
>>>>> couldn't do (or doesn't do let's rather say).
>>>>
>>>> tlv/fdt/http headers are all essentially exactly the same
>>>> thing. Key/value pairs with various encoding schemes.
>>>>
>>>> I don't think we don't need a tree of data, so fdt is overkill.
>>>>
>>>> tlv is not substantially easier to parse correctly than the
>>>> structured plain text headers.. It is just in binary so it can
>>>> represent binary-ish things better.
>>>
>>> TLV Seems easy enough. To give an update, I played with fdt a bit to see
>>> how far I get in half an hour. I got bool / int / strings to work
>>> quite fast (~30mins).
>>
>> Thanks for doing this fast piece of exploratory coding.  It does
>> confirm that for Linux, using fdt is pretty straightforward here.
>> However...
>>
>>> Please disregard the horrible hackyness of this ...
>>>
>>> For simplicity I stuck the header on top of my bitfile with:
>>>
>>> <snip>
>>> /dts-v1/;
>>>
>>> /{
>>>         description = "Test";
>>>         compressed = <0>;
>>>         encrypted = <1>;
>>> };
>>
>> I understand that this is a simplified example, but it looks a lot
>> like KVP which then gets compiled by dtc.
>>
>> If we do KVP or TLV we get skip using dtc, which would be nice for non-dt
>> OS's using the same images.
>
> I used dtc for pure lazyness. Writing a blob to a file using libfdt is
> about as much
> code as parsing it.

Thanks for that clarification.  I haven't used libfdt myself.  That
takes care of the license issue I brought up below.

I have heard that MS is averse to using DT, but I'm not clear
about why other than that it isn't native to Windows already.

Alan

> Even with KVP or TLV you have some code that needs
> to encode / pack your header into a file.
>
> libfdt has an example that creates an empty tree. Write that to a file, done.
>
> 1: Create empty tree
> https://github.com/dgibson/dtc/blob/master/libfdt/fdt_empty_tree.c
>
> 2: fopen / fwrite, done
>
>> Also, the license of libfdt allows the use by proprietary
>> os's, but that's not true for dtc.
>
> Why would that be an issue, you don't need to link anything to run
> dtc. That being
> said as I pointed out above you don not have to actually use dtc if the values
> are known ahead of time (like in our case). What you'd get from using dtc is to
> encode arbitrary values (for the types supported).
>
> Cheers,
>
> Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22 16:44                                               ` Jason Gunthorpe
@ 2017-02-22 17:50                                                 ` Moritz Fischer
  2017-02-22 17:54                                                   ` Jason Gunthorpe
  0 siblings, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-22 17:50 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Nadathur, Sundar, Alan Tull, Yves Vandervennet, matthew.gerlach,
	linux-kernel, linux-fpga, Marek Va??ut

Jason,

On Wed, Feb 22, 2017 at 8:44 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Tue, Feb 21, 2017 at 10:05:42PM -0800, Moritz Fischer wrote:
>
>> That being said older drivers / fpga-mgr  will not deal with newer features.
>> TLV / KV or whatever doesn't change this fact, or am I missing something?
>
> Often a scheme will have an OPTIONAL and REQUIRED flag for each
> value. If a REQUIRED value is present but the parser does not
> support it then the parse fails.

Oh, of course. I'm a dorque :) Obvioiusly better customer experience like that.

> For instance, we could mark Encrypted as required, and a zynq driver
> that does not support the Encrypted tag would not load the bitfile
> rather than try to load it wrongly.

Funny you'd mention that. Thanks to your patch the zynq driver won't ;-)
I ran into that while testing, hehe.

> This can be forced into any of the approaches with various levels of
> hackery.

Yeah doesn't seem to hard,

Cheers

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22 17:50                                                 ` Moritz Fischer
@ 2017-02-22 17:54                                                   ` Jason Gunthorpe
  2017-02-22 17:57                                                     ` Moritz Fischer
  0 siblings, 1 reply; 63+ messages in thread
From: Jason Gunthorpe @ 2017-02-22 17:54 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Nadathur, Sundar, Alan Tull, Yves Vandervennet, matthew.gerlach,
	linux-kernel, linux-fpga, Marek Va??ut

On Wed, Feb 22, 2017 at 09:50:54AM -0800, Moritz Fischer wrote:

> > For instance, we could mark Encrypted as required, and a zynq driver
> > that does not support the Encrypted tag would not load the bitfile
> > rather than try to load it wrongly.
> 
> Funny you'd mention that. Thanks to your patch the zynq driver won't ;-)
> I ran into that while testing, hehe.

Really? You mean there is no sync word? That really surprises me..

Jason

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-22 17:54                                                   ` Jason Gunthorpe
@ 2017-02-22 17:57                                                     ` Moritz Fischer
  0 siblings, 0 replies; 63+ messages in thread
From: Moritz Fischer @ 2017-02-22 17:57 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Nadathur, Sundar, Alan Tull, Yves Vandervennet, matthew.gerlach,
	linux-kernel, linux-fpga, Marek Va??ut

On Wed, Feb 22, 2017 at 9:54 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Feb 22, 2017 at 09:50:54AM -0800, Moritz Fischer wrote:
>
>> > For instance, we could mark Encrypted as required, and a zynq driver
>> > that does not support the Encrypted tag would not load the bitfile
>> > rather than try to load it wrongly.
>>
>> Funny you'd mention that. Thanks to your patch the zynq driver won't ;-)
>> I ran into that while testing, hehe.
>
> Really? You mean there is no sync word? That really surprises me..

Oh wait, that might have been because I didn't drop the header, now
that I think about it,
it probably still has the sync word. I thought it might have been
encrypted (which makes no sense
of course ... )

Ignore the noise :D

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-21 18:33                                 ` Alan Tull
  2017-02-22  3:13                                   ` Nadathur, Sundar
@ 2017-02-27 20:09                                   ` Alan Tull
  2017-02-27 22:49                                     ` Moritz Fischer
  1 sibling, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-27 20:09 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Nadathur, Sundar, Yves Vandervennet, Jason Gunthorpe,
	matthew.gerlach, linux-kernel, linux-fpga, Marek Vašut

On Tue, Feb 21, 2017 at 12:33 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
> On Mon, Feb 20, 2017 at 5:49 PM, Moritz Fischer
> <moritz.fischer@ettus.com> wrote:
>> Hi Alan,
>>
>> On Sun, Feb 19, 2017 at 3:16 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>>> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>>>> On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer
>>>> <moritz.fischer@ettus.com> wrote:
>>>>> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote:
>>>>>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar
>>>>>> <sundar.nadathur@intel.com> wrote:
>>>>>>
>>>>>> > Hi all,
>>>>>> >    Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples.
>>>>>> >
>>>>>> > To elaborate, a TLV-based format has many advantages:
>>>>>> > * It is highly extensible in many ways
>>>>>> >    -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time.
>>>>
>>>> Device tree can express arrays.
>>>>
>>>>>> >    -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values.
>>>>>> >    -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges.
>>>>>> > * It is OS-independent.
>>>>>> > * It can be easily parsed, in kernel or user space.
>>>>>
>>>>> Are there other users of the format? I have to admit I didn't look very
>>>>> long, but couldn't find any libs / existing code at a first glance.
>>>>
>>>> Is there a standard you are looking at?  Have you seen any use of TLV's
>>>> in the Linux kernel you could point to?
>>>>
>>>>>
>>>>>> > * It can be validated, in terms of Type values, acceptable lengths, etc.
>>>>>> >
>>>>>> > It  is not directly human-readable but that can be easily addressed with a tool that parses TLVs.
>>>>>> >
>>>>>> > Compared to some other proposals:
>>>>>> > * Compared to DTs, TLVs are OS-independent.
>>>>>
>>>>> That's just alternative facts here. Just because Linux uses fdt for
>>>>> devicetree blobs it is *not* OS dependent. There are several (see
>>>>> last email) non-Linux users of fdt / libfdt.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Moritz
>>>>
>>>> It is worth repeating that libdtc is GPL/BSD with the intent of
>>>> allowing proprietary code to use libdtc.  So license shouldn't be a barrier.
>>>>
>>>> Using device tree in the header would give us a way of doing enumeration at
>>>> least for Linux, not sure if that kind of info can be used in Windows
>>>> in some way.
>>>
>>> Actually, enumeration is the only advantage I see with DT.
>>
>> Which seems to some point a separate issue to passing in image
>> specific info such as
>> encrypted or not, compressed or not or build info metadata.
>>
>> So I think in general we can still separate this out into:
>> - Image specific values
>> - Reconfiguration specific values
>>
>>> Currently I like key/value pairs because they are easily implemented
>>> and expandable without being rigid in any way.
>>>
>>> If we use key/value pairs, we could pass in child device info
>>> in one of the keys.  It could be either a device tree overlay or an
>>> ACPI overlay.  Or could just be left out.  So platforms that
>>> aren't already using DT wouldn't have to.  Platforms that
>>> are have a smooth road to enumeration.
>>
>> I'm not sure if you can bundle up enumeration info *with* the image
>> since you might e.g.
>> load the same image (i.e. same header) into different FPGAs and the
>> required update to
>> the kernel state, i.e. live tree or ACPI would depend entirely on
>> which FPGA you loaded
>> the image into w.r.t busses it's connected to etc.
>>
>> I do think this info cannot be image specific, but needs to be passed
>> in via something
>> external such as a dt overlay.
>
> Yes, I think you are right about that.

Hi Moritz,

Actually now I think that enumeration data in the FPGA image should
work even for cases with > 1 FPGA.

First case: embedded FPGA.  The hardware has one FPGA.  The image is
designed for a specific board, so there's no problem including the
enumeration in the image.

Second case: embedded FPGA + a PCIe FPGA.  The image will be specific
as to whether it goes into the embedded FPGA or the PCIe one.

Third case: multiple PCIe FPGAs.  The enumeration base will be the
PCIe bus of the individual FPGA.  If the FPGAs don't have unique pin
connections, then the images could go on any of the PCie FPGAs.  If
there are unique pin connections, then the image will be specific to
the FPGA and having the enumeration data in the image is that much
more helpful for keeping things straight.  Part of the header could
specify which specific FPGA it should go on if it is restricted.

Of course if the FPGAs have > 1 PR regions, most FPGA architectures do
not have relocatable images so those images will be specific for the
PR region but not specific to the FPGA except as otherwise noted
above.

So again, including enumeration data in the bitstream should work
unless I'm missing something.  What am I missing here?

Alan

>
> Thanks!
> Alan
>
>>
>> Cheers,
>>
>> Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-27 20:09                                   ` Alan Tull
@ 2017-02-27 22:49                                     ` Moritz Fischer
  2017-02-28  0:04                                       ` matthew.gerlach
  0 siblings, 1 reply; 63+ messages in thread
From: Moritz Fischer @ 2017-02-27 22:49 UTC (permalink / raw)
  To: Alan Tull
  Cc: Nadathur, Sundar, Yves Vandervennet, Jason Gunthorpe,
	matthew.gerlach, linux-kernel, linux-fpga, Marek Vašut

Alan,

On Mon, Feb 27, 2017 at 12:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:


> First case: embedded FPGA.  The hardware has one FPGA.  The image is
> designed for a specific board, so there's no problem including the
> enumeration in the image.

Agreed.

> Second case: embedded FPGA + a PCIe FPGA.  The image will be specific
> as to whether it goes into the embedded FPGA or the PCIe one.

Agreed.


> Third case: multiple PCIe FPGAs.  The enumeration base will be the
> PCIe bus of the individual FPGA.  If the FPGAs don't have unique pin
> connections, then the images could go on any of the PCie FPGAs.  If
> there are unique pin connections, then the image will be specific to
> the FPGA and having the enumeration data in the image is that much
> more helpful for keeping things straight.  Part of the header could
> specify which specific FPGA it should go on if it is restricted.

Agreed.

> Of course if the FPGAs have > 1 PR regions, most FPGA architectures do
> not have relocatable images so those images will be specific for the
> PR region but not specific to the FPGA except as otherwise noted
> above.
>
> So again, including enumeration data in the bitstream should work
> unless I'm missing something.  What am I missing here?

If you enumeration base is sufficiently smart, I suppose that can work.
What you'd probably want is some sort of extension to the platform bus?

I really need to take another look at how non-dt systems enumerate to
give better feedback on this.

Cheers,

Moritz

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [RFC 7/8] fpga-region: add sysfs interface
  2017-02-27 22:49                                     ` Moritz Fischer
@ 2017-02-28  0:04                                       ` matthew.gerlach
  0 siblings, 0 replies; 63+ messages in thread
From: matthew.gerlach @ 2017-02-28  0:04 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Alan Tull, Nadathur, Sundar, Yves Vandervennet, Jason Gunthorpe,
	linux-kernel, linux-fpga, Marek Vašut



On Mon, 27 Feb 2017, Moritz Fischer wrote:

> Alan,
>
> On Mon, Feb 27, 2017 at 12:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>
>
>> First case: embedded FPGA.  The hardware has one FPGA.  The image is
>> designed for a specific board, so there's no problem including the
>> enumeration in the image.
>
> Agreed.
>
>> Second case: embedded FPGA + a PCIe FPGA.  The image will be specific
>> as to whether it goes into the embedded FPGA or the PCIe one.
>
> Agreed.
>
>
>> Third case: multiple PCIe FPGAs.  The enumeration base will be the
>> PCIe bus of the individual FPGA.  If the FPGAs don't have unique pin
>> connections, then the images could go on any of the PCie FPGAs.  If
>> there are unique pin connections, then the image will be specific to
>> the FPGA and having the enumeration data in the image is that much
>> more helpful for keeping things straight.  Part of the header could
>> specify which specific FPGA it should go on if it is restricted.
>
> Agreed.
>
>> Of course if the FPGAs have > 1 PR regions, most FPGA architectures do
>> not have relocatable images so those images will be specific for the
>> PR region but not specific to the FPGA except as otherwise noted
>> above.
>>
>> So again, including enumeration data in the bitstream should work
>> unless I'm missing something.  What am I missing here?
>
> If you enumeration base is sufficiently smart, I suppose that can work.
> What you'd probably want is some sort of extension to the platform bus?

I think there is merit it considering the platform bus as part of the 
overal enumeration strategy.  When using device trees, the platform bus is 
involved.  I have also seen other folks enumerate over a platform bus 
based on data in a format other than device tree.  I have experimented 
with instantiating platform devices from my pcie driver, but I didn't 
completely work, which could be related to using a fairly old 3.10 kernel.

Matthew Gerlach

>
> I really need to take another look at how non-dt systems enumerate to
> give better feedback on this.
>
> Cheers,
>
> Moritz
> --
> 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
>

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: FPGA Region enhancements and fixes
  2017-02-15 16:14 FPGA Region enhancements and fixes Alan Tull
                   ` (7 preceding siblings ...)
  2017-02-15 16:14 ` [RFC 8/8] doc: fpga: add sysfs document for fpga region Alan Tull
@ 2017-02-28 17:35 ` Alan Tull
  2017-02-28 22:03   ` Alan Tull
  8 siblings, 1 reply; 63+ messages in thread
From: Alan Tull @ 2017-02-28 17:35 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

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

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

Alan

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

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: FPGA Region enhancements and fixes
  2017-02-28 17:35 ` FPGA Region enhancements and fixes Alan Tull
@ 2017-02-28 22:03   ` Alan Tull
  0 siblings, 0 replies; 63+ messages in thread
From: Alan Tull @ 2017-02-28 22:03 UTC (permalink / raw)
  To: Moritz Fischer, Jason Gunthorpe; +Cc: Alan Tull, linux-kernel, linux-fpga

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

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

Alan

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

^ permalink raw reply	[flat|nested] 63+ messages in thread

end of thread, other threads:[~2017-02-28 22:57 UTC | newest]

Thread overview: 63+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-15 16:14 FPGA Region enhancements and fixes Alan Tull
2017-02-15 16:14 ` [RFC 1/8] fpga-mgr: add a single function for fpga loading methods Alan Tull
2017-02-16  0:36   ` matthew.gerlach
2017-02-15 16:14 ` [RFC 2/8] fpga-region: support more than one overlay per FPGA region Alan Tull
2017-02-16 16:50   ` matthew.gerlach
2017-02-16 17:35     ` Alan Tull
2017-02-15 16:14 ` [RFC 3/8] fpga-bridge: add non-dt support Alan Tull
2017-02-15 16:14 ` [RFC 4/8] doc: fpga-mgr: separate getting/locking FPGA manager Alan Tull
2017-02-17 17:14   ` Li, Yi
2017-02-17 21:55     ` Alan Tull
2017-02-17 17:52   ` Moritz Fischer
2017-02-17 22:02     ` Alan Tull
2017-02-15 16:14 ` [RFC 5/8] " Alan Tull
2017-02-15 16:14 ` [RFC 6/8] fpga-region: separate out common code to allow non-dt support Alan Tull
2017-02-15 16:14 ` [RFC 7/8] fpga-region: add sysfs interface Alan Tull
2017-02-15 17:21   ` Jason Gunthorpe
2017-02-15 17:46     ` Alan Tull
2017-02-15 17:55       ` Moritz Fischer
2017-02-15 18:06       ` Jason Gunthorpe
2017-02-15 18:23         ` Alan Tull
2017-02-15 18:31           ` Moritz Fischer
2017-02-15 19:49           ` Jason Gunthorpe
2017-02-15 22:49             ` Alan Tull
2017-02-15 23:07               ` Jason Gunthorpe
2017-02-15 20:07           ` matthew.gerlach
2017-02-15 20:37             ` Jason Gunthorpe
2017-02-15 20:54               ` Moritz Fischer
2017-02-15 21:15                 ` Jason Gunthorpe
2017-02-15 21:36                   ` Moritz Fischer
2017-02-15 22:42                     ` Alan Tull
2017-02-16  0:16                       ` Moritz Fischer
2017-02-16 17:47                         ` Alan Tull
2017-02-16 17:56                           ` Jason Gunthorpe
2017-02-16 18:11                             ` Moritz Fischer
2017-02-17 22:28                 ` Yves Vandervennet
2017-02-18  2:30                   ` Moritz Fischer
2017-02-18 12:45                     ` Nadathur, Sundar
2017-02-18 20:10                       ` Alan Tull
2017-02-18 20:45                         ` Moritz Fischer
2017-02-19 15:00                           ` Alan Tull
2017-02-19 23:16                             ` Alan Tull
2017-02-20 23:49                               ` Moritz Fischer
2017-02-21 18:33                                 ` Alan Tull
2017-02-22  3:13                                   ` Nadathur, Sundar
2017-02-22  3:49                                     ` Moritz Fischer
2017-02-22  5:12                                       ` Jason Gunthorpe
2017-02-22  5:38                                         ` Moritz Fischer
2017-02-22  5:46                                           ` Nadathur, Sundar
2017-02-22  6:05                                             ` Moritz Fischer
2017-02-22 16:44                                               ` Jason Gunthorpe
2017-02-22 17:50                                                 ` Moritz Fischer
2017-02-22 17:54                                                   ` Jason Gunthorpe
2017-02-22 17:57                                                     ` Moritz Fischer
2017-02-22 16:33                                           ` Alan Tull
2017-02-22 16:44                                             ` Moritz Fischer
2017-02-22 16:52                                               ` Alan Tull
2017-02-27 20:09                                   ` Alan Tull
2017-02-27 22:49                                     ` Moritz Fischer
2017-02-28  0:04                                       ` matthew.gerlach
2017-02-15 21:20         ` Anatolij Gustschin
2017-02-15 16:14 ` [RFC 8/8] doc: fpga: add sysfs document for fpga region Alan Tull
2017-02-28 17:35 ` FPGA Region enhancements and fixes Alan Tull
2017-02-28 22:03   ` Alan Tull

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.