FPGA Region enhancements and fixes

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 <[email protected]>

---

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(-)

drivers/fpga/fpga-mgr.c +2 -0

index 86d2cb2..b7c719a 100644
@@ -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)
+{

drivers/fpga/fpga-mgr.c +0 -0

 static const char * const state_str[] = {
 	[FPGA_MGR_STATE_UNKNOWN] =		"unknown",
 	[FPGA_MGR_STATE_POWER_OFF] =		"power off",

drivers/fpga/fpga-region.c +7 -10

index 3222fdb..24f4ed5 100644
@@ -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);
 }
 
 /**

include/linux/fpga/fpga-mgr.h +0 -0

index 57beb5d..45df05a 100644
@@ -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)

include/linux/fpga/fpga-mgr.h +6 -0

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

Signature · 6 lines

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

---
drivers/fpga/fpga-region.c | 222 +++++++++++++++++++++++++++++++--------------

1 file changed, 155 insertions(+), 67 deletions(-)

drivers/fpga/fpga-region.c +8 -2

index 24f4ed5..60d2947 100644
@@ -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.

drivers/fpga/fpga-region.c +137 -65

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

Signature · 6 lines

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Alan,

Quoted 53 lines · Alan Tull

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 <[email protected]>

---
drivers/fpga/fpga-region.c | 222
+++++++++++++++++++++++++++++++--------------

1 file changed, 155 insertions(+), 67 deletions(-)

drivers/fpga/fpga-region.c +7 -2

index 24f4ed5..60d2947 100644
@@ -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.
+ */

drivers/fpga/fpga-region.c +6 -0

+
+struct region_overlay {
+       struct list_head node;
+       struct device_node *overlay;
+       struct fpga_image_info *image_info;
+};

drivers/fpga/fpga-region.c line 58

Quoted 3 lines

drivers/fpga/fpga-region.c +3 -0

+
+/* 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.

Quoted 354 lines

+
#define to_fpga_region(d) container_of(d, struct fpga_region, dev)

static DEFINE_IDA(fpga_region_ida);

drivers/fpga/fpga-region.c +1 -1

@@ -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

drivers/fpga/fpga-region.c +1 -1

@@ -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;

drivers/fpga/fpga-region.c +2 -1

@@ -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;

drivers/fpga/fpga-region.c +3 -3

@@ -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 */

drivers/fpga/fpga-region.c +7 -5

@@ -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;

drivers/fpga/fpga-region.c +1 -1

@@ -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;

drivers/fpga/fpga-region.c +1 -1

@@ -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;

drivers/fpga/fpga-region.c +0 -7

@@ -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.

*/

drivers/fpga/fpga-region.c +1 -2

-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)
{

drivers/fpga/fpga-region.c +1 -7

+       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 */

drivers/fpga/fpga-region.c +14 -7

-       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);

drivers/fpga/fpga-region.c +2 -4

-       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");

drivers/fpga/fpga-region.c +1 -1

-               return -EINVAL;
+       /* If overlay is not programming the FPGA, don't need FPGA image

info */

drivers/fpga/fpga-region.c +5 -3

+       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

drivers/fpga/fpga-region.c +4 -0

+        * firmware here would be ambiguous.
+        */
+       if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
+               dev_err(dev, "error: specified firmware and

external-fpga-config");

drivers/fpga/fpga-region.c +3 -3

+               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");

drivers/fpga/fpga-region.c +2 -1

-               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

drivers/fpga/fpga-region.c +4 -0

+        * 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");

drivers/fpga/fpga-region.c +12 -1

+               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.
+ */

drivers/fpga/fpga-region.c +37 -0

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

}

/**

drivers/fpga/fpga-region.c +14 -2

@@ -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);

}

/**

drivers/fpga/fpga-region.c +1 -0

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

---
  Documentation/fpga/fpga-mgr.txt | 19 ++++++++++++++++++-
  1 file changed, 18 insertions(+), 1 deletion(-)

Documentation/fpga/fpga-mgr.txt +18 -1

index 78f197f..06d5d5b 100644
@@ -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);
 
 

hi Alan

Quoted 4 lines · Alan Tull

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

Quoted 8 lines

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

Quoted 53 lines

Signed-off-by Alan Tull <[email protected]>

---
  Documentation/fpga/fpga-mgr.txt | 19 ++++++++++++++++++-
  1 file changed, 18 insertions(+), 1 deletion(-)

Documentation/fpga/fpga-mgr.txt +0 -0

index 78f197f..06d5d5b 100644
@@ -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.

Documentation/fpga/fpga-mgr.txt +17 -0

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

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 <[email protected]>

---
 Documentation/fpga/fpga-mgr.txt | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Documentation/fpga/fpga-mgr.txt +0 -0

index 78f197f..06d5d5b 100644

Documentation/fpga/fpga-mgr.txt +17 -1

+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

Alan,

small nits inline

Quoted 5 lines

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.

Quoted 8 lines

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 <[email protected]>

Acked-by Moritz Fischer <[email protected]>

Quoted 9 lines

---
 Documentation/fpga/fpga-mgr.txt | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Documentation/fpga/fpga-mgr.txt +0 -0

index 78f197f..06d5d5b 100644

Documentation/fpga/fpga-mgr.txt To get/put a reference to a FPGA manager: · line 56

Quoted 5 lines

Documentation/fpga/fpga-mgr.txt +1 -1

@@ -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.

Quoted 51 lines

Documentation/fpga/fpga-mgr.txt +17 -1

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

Quoted 2 lines

2.7.4

Cheers,

Moritz

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

Quoted 6 lines · Alan Tull

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.

Quoted 7 lines

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

Quoted 11 lines · Jason Gunthorpe

Quoted 10 lines · Alan Tull

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

Quoted 7 lines

Quoted 3 lines

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.

Quoted 6 lines

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..

Quoted 2 lines

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.

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.

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 6 lines · Alan Tull

Quoted 3 lines

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()))));

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.

On Wed, Feb 15, 2017 at 12:06 PM, Jason Gunthorpe
<[email protected]> wrote:

Hi Jason,

Quoted 8 lines · Alan Tull

Quoted 7 lines

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.

Quoted 27 lines

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

Quoted 9 lines

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

Quoted 6 lines

[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..

Quoted 7 lines

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

Quoted 2 lines

Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

Quoted 4 lines · On Wed, Feb 15, 2017 at 12:07:15PM -0800, [email protected] 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,

Quoted 2 lines · Moritz Fischer

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,

Quoted 9 lines · Jason Gunthorpe

Quoted 2 lines · Moritz Fischer

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

Quoted 1 line

Can we force a specific alignment for the included data?

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

Quoted 1 line

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

Quoted 31 lines · Moritz Fischer

Jason,

Quoted 9 lines · Jason Gunthorpe

Quoted 2 lines · Moritz Fischer

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

Quoted 1 line

Can we force a specific alignment for the included data?

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

Quoted 1 line

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,

Quoted 1 line

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.

Quoted 33 lines · Alan Tull

Quoted 31 lines · Moritz Fischer

Jason,

Quoted 9 lines · Jason Gunthorpe

Quoted 2 lines · Moritz Fischer

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

Quoted 1 line

Can we force a specific alignment for the included data?

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

Quoted 1 line

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,

Quoted 8 lines

Quoted 1 line

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

Quoted 3 lines

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.

Quoted 7 lines · Alan Tull

Quoted 3 lines

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..

Quoted 38 lines

Hi Jason,

Quoted 13 lines · Jason Gunthorpe

Quoted 4 lines · On Wed, Feb 15, 2017 at 12:07:15PM -0800, [email protected] 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

Quoted 7 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

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.

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

Quoted 3 lines · Sundar

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.

Quoted 7 lines

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.

Quoted 6 lines

* 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.

Quoted 9 lines · Alan Tull

Quoted 6 lines · Sundar

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.

Quoted 4 lines

Quoted 4 lines

-- 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.

Quoted 7 lines

Quoted 7 lines

* 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

Quoted 10 lines · Moritz Fischer

Quoted 9 lines · Alan Tull

Quoted 6 lines · Sundar

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.

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

-- 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?

Quoted 14 lines

Quoted 7 lines

Quoted 7 lines

* 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,

Quoted 1 line

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

Quoted 42 lines · Alan Tull

Quoted 10 lines · Moritz Fischer

Quoted 9 lines · Alan Tull

Quoted 6 lines · Sundar

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.

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

-- 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?

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

* 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

Quoted 6 lines

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.

Hi Alan,

Quoted 43 lines · Alan Tull

Quoted 42 lines · Alan Tull

Quoted 10 lines · Moritz Fischer

Quoted 9 lines · Alan Tull

Quoted 6 lines · Sundar

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.

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

-- 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?

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

* 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

Quoted 8 lines

Quoted 6 lines

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

Quoted 8 lines

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

-----Original Message-----
From: Alan Tull [mailto:[email protected]]
Sent: Tuesday, February 21, 2017 10:33 AM
To: Moritz Fischer <[email protected]>
Cc: Nadathur, Sundar <[email protected]>; Yves Vandervennet <[email protected]>; Jason Gunthorpe <[email protected]>; [email protected]; linux-kernel <[email protected]>; [email protected]; Marek Vašut <[email protected]>
Subject: Re: [RFC 7/8] fpga-region: add sysfs interface

Quoted 3 lines · Moritz Fischer

Hi Alan,

On Sun, Feb 19, 2017 at 3:16 PM, Alan Tull <[email protected]>

Quoted 1 line · wrote:

Quoted 1 line

On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <[email protected]>

Quoted 3 lines · wrote:

Quoted 3 lines

Quoted 3 lines

Quoted 1 line · Moritz Fischer

On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines · Sundar

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

[...]

Quoted 2 lines

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?

Quoted 1 line

Quoted 1 line

On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <[email protected]>

Quoted 2 lines · wrote:

Quoted 2 lines

Quoted 2 lines

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.

Quoted 13 lines

Quoted 5 lines

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.

Quoted 2 lines

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

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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

Quoted 4 lines

Quoted 1 line

Quoted 1 line

On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <[email protected]>

Quoted 2 lines · wrote:

Quoted 2 lines

Quoted 2 lines

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.

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).

Quoted 2 lines · Moritz Fischer

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.

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.

Quoted 2 lines

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

Quoted 7 lines · Moritz Fischer

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.

Quoted 2 lines

Quoted 2 lines

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.

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?

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?

Quoted 2 lines · Moritz Fischer

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.

Quoted 3 lines

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.

Quoted 6 lines · Moritz Fischer

Quoted 3 lines

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..

Hi all,

Quoted 10 lines · Jason Gunthorpe

Quoted 2 lines · Moritz Fischer

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.

Quoted 3 lines

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,

(quoted diff) +37 -0

+                                          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.

Quoted 2 lines

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

On Tue, Feb 21, 2017 at 11:38 PM, Moritz Fischer
<[email protected]> wrote:

Hi Moritz,

Quoted 14 lines

Hi all,

Quoted 10 lines · Jason Gunthorpe

Quoted 2 lines · Moritz Fischer

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.

Quoted 7 lines

Quoted 3 lines

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...

Quoted 4 lines

Please disregard the horrible hackyness of this ...

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

Quoted 8 lines

<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.

Quoted 47 lines · Alan Tull

On Tue, Feb 21, 2017 at 11:38 PM, Moritz Fischer
<[email protected]> wrote:

Hi Moritz,

Quoted 14 lines

Hi all,

Quoted 10 lines · Jason Gunthorpe

Quoted 2 lines · Moritz Fischer

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.

Quoted 7 lines

Quoted 3 lines

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...

Quoted 4 lines

Please disregard the horrible hackyness of this ...

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

Quoted 8 lines

<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.

Quoted 2 lines

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

Quoted 2 lines

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

Quoted 80 lines · Moritz Fischer

Hi Alan,

Quoted 43 lines · Alan Tull

Quoted 42 lines · Alan Tull

Quoted 10 lines · Moritz Fischer

Quoted 9 lines · Alan Tull

Quoted 6 lines · Sundar

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.

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

-- 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?

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

* 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

Quoted 8 lines

Quoted 6 lines

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

Quoted 8 lines

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.

Quoted 3 lines

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

Quoted 4 lines

Cheers,

Moritz

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,

Quoted 3 lines · Alan Tull

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.

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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...

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

Quoted 4 lines · Alan Tull

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

Quoted 39 lines

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.

Quoted 32 lines

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