linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree
@ 2017-04-20 14:09 Alan Tull
  2017-04-20 14:09 ` [PATCH v2 01/16] doc: fpga: update documents for the FPGA API Alan Tull
                   ` (16 more replies)
  0 siblings, 17 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

The current FPGA layer has two problems which this
patchset addresses:

* We now have 3 functions for programming a FPGA, depending
on whether the image is in a sg list, a buffer, or firmware.
So callers have to decide where the image will be or have
to write extra code to maintain flexibilty.

* users who aren't using device tree are left to write their
own code that is essentially a rewrite of FPGA region.

The major things this patchset accomplishes is:
* Change the fpga-mgr api to have one fpga_mgr_load functon
  instead of three.

* Separate common FPGA region code from DT support

* Additions for using bridges where DT is not used.

The end result of this v2 is not hugely different from v1.
This patchset has > patches than v1, but that is because I respun
it to make it easier to see where I'm making code changes verses
where code is just moving to a different file.  So there are a
lot of little patches that make code changes; these patches are
separate from the one big patch at the end that moves code
between .c files without making code changes.

Patch 1 updates documentation and adds a FPGA region API doc.

Patch 2 adds non-dt support for fpga-bridges

Patches 3 and 4 change the FPGA manager API.  The 3 functions
for programming an FPGA from a scatter-gather list, a contiguous
buffer, or a firmware file are replaced by a single function.
The parameters are placed in the fpga_image_info struct.
Functions to alloc/free a struct fpga_image_info are provided.
Getting a ref to a manager is separated from locking it.

Patches 5 to 16 separate FPGA region common code from FPGA region
support for Device Tree overlays.  This allows FPGA regions to be
created by PCIe or other schemes.  Region common code will
include two functions to register/unregister a region and one
function to program the region.

Changes from v1:
* split the final (large) patch into smaller patches for easier reviewability.
* documentation has been expanded
* reorder things, clean up
* dev_err instead of pr_err
* move functions that alloc/free image info from fpga-region.c to fpga-mgr.c
* move fpga-region.h to include/linux/fpga/
* add fpga_region_class_find to fpga-region.c
* move of_fpga_region_find to of-fpga-region.c



Alan Tull (16):
  doc: fpga: update documents for the FPGA API
  fpga: bridge: support getting bridge from device
  fpga: mgr: API change to replace fpga load functions with single
    function
  fpga: mgr: separate getting/locking FPGA manager
  fpga: region: use dev_err instead of pr_err
  fpga: region: remove unneeded of_node_get and put
  fpga: region: get mgr early on
  fpga: region: check for child regions before allocing image info
  fpga: region: fix slow warning with more than one overlay
  fpga: region: use image info as parameter for programming region
  fpga: region: separate out code that parses the overlay
  fpga: region: add fpga-region.h header
  fpga: region: rename some functions prior to moving
  fpga: region: add register/unregister functions
  fpga: region: add fpga_region_class_find
  fpga: region: move device tree support to of-fpga-region.c

 Documentation/fpga/fpga-mgr.txt    | 133 +++++-----
 Documentation/fpga/fpga-region.txt |  54 ++++
 Documentation/fpga/overview.txt    |  23 ++
 drivers/fpga/Kconfig               |  13 +-
 drivers/fpga/Makefile              |   1 +
 drivers/fpga/fpga-bridge.c         | 110 +++++++--
 drivers/fpga/fpga-mgr.c            | 111 +++++++--
 drivers/fpga/fpga-region.c         | 471 ++++-------------------------------
 drivers/fpga/of-fpga-region.c      | 493 +++++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-bridge.h   |   7 +-
 include/linux/fpga/fpga-mgr.h      |  29 ++-
 include/linux/fpga/fpga-region.h   |  41 +++
 12 files changed, 943 insertions(+), 543 deletions(-)
 create mode 100644 Documentation/fpga/fpga-region.txt
 create mode 100644 Documentation/fpga/overview.txt
 create mode 100644 drivers/fpga/of-fpga-region.c
 create mode 100644 include/linux/fpga/fpga-region.h

-- 
2.7.4

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

* [PATCH v2 01/16] doc: fpga: update documents for the FPGA API
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-04-20 14:09 ` [PATCH v2 02/16] fpga: bridge: support getting bridge from device Alan Tull
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

The FPGA manager has been simplified to have a single
fpga_mgr_load function which replaces the three
fpga_mgr_*load* functions.

The parameters presenting the FPGA image have been
added to struct fpga_image_info.

Additional functions have been added to alloc/free
fpga_image_info.

Getting a FPGA manager has been separated from
locking it.  So an unlocked reference can be saved and
only locked when we're about to program.

A document for fpga-region has been added as well.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v1: fixed some nits
v2: expanded to include other FPGA API changes besides locking
---
 Documentation/fpga/fpga-mgr.txt    | 133 +++++++++++++++++++------------------
 Documentation/fpga/fpga-region.txt |  54 +++++++++++++++
 Documentation/fpga/overview.txt    |  23 +++++++
 3 files changed, 147 insertions(+), 63 deletions(-)
 create mode 100644 Documentation/fpga/fpga-region.txt
 create mode 100644 Documentation/fpga/overview.txt

diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
index 78f197f..0fc41eb 100644
--- a/Documentation/fpga/fpga-mgr.txt
+++ b/Documentation/fpga/fpga-mgr.txt
@@ -11,61 +11,66 @@ hidden away in a low level driver which registers a set of ops with the core.
 The FPGA image data itself is very manufacturer specific, but for our purposes
 it's just binary data.  The FPGA manager core won't parse it.
 
+The FPGA image to be programmed can be in a scatter gather list, a single
+contiguous buffer, or a firmware file.  Because allocating contiguous kernel
+memory for the buffer should be avoided, users are encouraged to use a scatter
+gather list instead if possible.
+
+The particulars for programming the image are presented in a structure (struct
+fpga_image_info).  This struct contains parameters such as pointers to the
+FPGA image as well as image-specific particulars such as whether the image was
+built for full or partial reconfiguration.
 
 API Functions:
 ==============
 
-To program the FPGA from a file or from a buffer:
--------------------------------------------------
-
-	int fpga_mgr_buf_load(struct fpga_manager *mgr,
-			      struct fpga_image_info *info,
-		              const char *buf, size_t count);
-
-Load the FPGA from an image which exists as a contiguous buffer in
-memory. Allocating contiguous kernel memory for the buffer should be avoided,
-users are encouraged to use the _sg interface instead of this.
-
-        int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
-				 struct fpga_image_info *info,
-				 struct sg_table *sgt);
+To program the FPGA:
+--------------------
 
-Load the FPGA from an image from non-contiguous in memory. Callers can
-construct a sg_table using alloc_page backed memory.
+	int fpga_mgr_load(struct fpga_manager *mgr,
+			  struct fpga_image_info *info);
 
-	int fpga_mgr_firmware_load(struct fpga_manager *mgr,
-				   struct fpga_image_info *info,
-		                   const char *image_name);
-
-Load the FPGA from an image which exists as a file.  The image file must be on
-the firmware search path (see the firmware class documentation).  If successful,
+Load the FPGA from an image which is indicated in the info.  If successful,
 the FPGA ends up in operating mode.  Return 0 on success or a negative error
 code.
 
-A FPGA design contained in a FPGA image file will likely have particulars that
-affect how the image is programmed to the FPGA.  These are contained in struct
-fpga_image_info.  Currently the only such particular is a single flag bit
-indicating whether the image is for full or partial reconfiguration.
+To allocate or free a struct fpga_image_info:
+---------------------------------------------
+
+	struct fpga_image_info *fpga_image_info_alloc(struct device *dev);
+
+	void fpga_image_info_free(struct device *dev,
+				  struct fpga_image_info *info);
 
 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);
+	void fpga_mgr_put(struct fpga_manager *mgr);
 
-Given a DT node or device, get an exclusive reference to a FPGA manager.
+Given a DT node or device, get a reference to a FPGA manager.  This pointer
+can be saved until you are ready to program the FPGA.  fpga_mgr_put
+releases the reference.
 
-	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);
+	void fpga_mgr_unlock(struct fpga_magager *mgr);
+
+The user should call fpga_mgr_lock and verify that it returns 0 before
+attempting to program the FPGA.  Likeqise, the user should call
+fpga_mgr_unlock when done programming the FPGA.
 
 
 To register or unregister the low level FPGA-specific driver:
 -------------------------------------------------------------
 
 	int fpga_mgr_register(struct device *dev, const char *name,
-		              const struct fpga_manager_ops *mops,
-		              void *priv);
+			      const struct fpga_manager_ops *mops,
+			      void *priv);
 
 	void fpga_mgr_unregister(struct device *dev);
 
@@ -78,59 +83,61 @@ How to write an image buffer to a supported FPGA
 /* Include to get the API */
 #include <linux/fpga/fpga-mgr.h>
 
-/* device node that specifies the FPGA manager to use */
-struct device_node *mgr_node = ...
+struct fpga_manager *mgr;
+struct fpga_image_info *info;
+int ret;
 
-/* FPGA image is in this buffer.  count is size of the buffer. */
-char *buf = ...
-int count = ...
+/*
+ * Get a reference to FPGA manager.  This example uses the  device node of the
+ * manager.  You could use fpga_mgr_get() instead if you have the device instead
+ * of the device node.
+ */
+mgr = of_fpga_mgr_get(mgr_node);
 
 /* struct with information about the FPGA image to program. */
-struct fpga_image_info info;
+info = fpga_image_info_alloc(dev);
 
 /* flags indicates whether to do full or partial reconfiguration */
-info.flags = 0;
+info->flags = FPGA_MGR_PARTIAL_RECONFIG;
 
-int ret;
+/*
+ * At this point, indicate where the image is. This is pseudo-code; you're
+ * probably going to use one of these three.
+ */
+if (using scatter gather) {
 
-/* Get exclusive control of FPGA manager */
-struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
+	info->sgt = [your scatter gather table]
 
-/* Load the buffer to the FPGA */
-ret = fpga_mgr_buf_load(mgr, &info, buf, count);
-
-/* Release the FPGA manager */
-fpga_mgr_put(mgr);
+} else if (using a buffer) {
 
+	info->buf = [your image buffer]
+	info->count = [image buffer size]
 
-How to write an image file to a supported FPGA
-==============================================
-/* Include to get the API */
-#include <linux/fpga/fpga-mgr.h>
-
-/* device node that specifies the FPGA manager to use */
-struct device_node *mgr_node = ...
+} else if (using a firmware file) {
 
-/* FPGA image is in this file which is in the firmware search path */
-const char *path = "fpga-image-9.rbf"
+	info->firmware_name = devm_kstrdup(dev, firmware_name, GFP_KERNEL);
 
-/* struct with information about the FPGA image to program. */
-struct fpga_image_info info;
+} else {
 
-/* flags indicates whether to do full or partial reconfiguration */
-info.flags = 0;
+	not implemented!
 
-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);
+/* 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);
 
+/* Free your image storage in some appropriate way */
+...
+
+/* Deallocate the image info if you're done with it */
+fpga_image_info_free(dev, info);
 
 How to support a new FPGA device
 ================================
diff --git a/Documentation/fpga/fpga-region.txt b/Documentation/fpga/fpga-region.txt
new file mode 100644
index 0000000..9d53559
--- /dev/null
+++ b/Documentation/fpga/fpga-region.txt
@@ -0,0 +1,54 @@
+FPGA Regions
+
+Alan Tull 2017
+
+An fpga-region represents a reconfigurable region of a FPGA.  It could be the
+whole FPGA in the case of full reconfiguration or a partial reconfiguration area.
+
+An fpga-region will know which FPGA manager to use to do the programming as well
+as which bridges are involved.  The bridges exist to prevent junk data from
+going out onto processor busses or out onto FPGA pins during programming.  There
+are currently a lot of variations of how these can be implemented.  Some
+implementations will include the bridge in the manager.  The FPGA manager may be
+fixed hardware in some cases, while other implemenatations have managers as part
+of the static region of the FPGA to handle partial reconfiguration.  Some
+bridges are fixed hardware while other bridges exist in the FPGA fabric as a
+ring surrounding the FPGA reconfiguration areas.
+
+This document is meant to be an overview of API usage.  A more conceptual look
+at regions can be found in [1].
+
+===================
+The FPGA region API
+===================
+
+To register or unregister a region:
+-----------------------------------
+
+	int fpga_region_register(struct device *dev,
+				 struct fpga_region *region);
+	int fpga_region_unregister(struct fpga_region *region);
+
+An example of usage can be seen in the probe function of [2]
+
+To program an FPGA:
+-------------------
+
+	int fpga_region_program_fpga(struct fpga_region *region,
+				     struct fpga_image_info *image_info);
+
+image_info is described in [3]
+
+This function will attempt to:
+ * lock the region's mutex
+ * lock the region's FPGA manager
+ * build a list of FPGA bridges if a method has been specified to do so
+ * disable the bridges
+ * program the FPGA
+ * re-enable the bridges
+ * release the locks
+
+--
+[1] ../devicetree/bindings/fpga/fpga-region.txt
+[2] ../../drivers/fpga/of-fpga-region.c
+[3] ./fpga-mgr.txt
diff --git a/Documentation/fpga/overview.txt b/Documentation/fpga/overview.txt
new file mode 100644
index 0000000..149ac8a
--- /dev/null
+++ b/Documentation/fpga/overview.txt
@@ -0,0 +1,23 @@
+Linux kernel FPGA support
+
+Alan Tull 2017
+
+The main point of this project has been to separate the out the upper layers
+that know when to reprogram a FPGA from the lower layers that know how to
+reprogram a specific FPGA device.  The intention is to make this manufacturor
+agnostic, understanding that of course the FPGA images are very device specific
+themselves.
+
+At this point it time, the framework in the kernel includes:
+* low level FPGA manager drivers that know how to program a specific device
+* the fpga-mgr framework they are registered with
+* low level FPGA bridge drivers for hard/soft bridges which are intended to
+  be disable during FPGA programming
+* the fpga-bridge framework they are registered with
+* the fpga-region framework which associates and controls managers and bridges
+  as reconfigurable regions
+* the of-fpga-region support for reprogramming FPGAs when device tree overlays
+  are applied.
+
+I would encourage you the user to add code that creates fpga regions rather
+that trying to control managers and bridges separately.
-- 
2.7.4

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

* [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
  2017-04-20 14:09 ` [PATCH v2 01/16] doc: fpga: update documents for the FPGA API Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-05-03 11:58   ` Wu Hao
  2017-04-20 14:09 ` [PATCH v2 03/16] fpga: mgr: API change to replace fpga load functions with single function Alan Tull
                   ` (14 subsequent siblings)
  16 siblings, 1 reply; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

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

* fpga_bridge_get
  Get the bridge given the device.

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

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

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: use list_for_each_entry
    static the bridge_list_lock
    update copyright and author email
---
 drivers/fpga/fpga-bridge.c       | 110 +++++++++++++++++++++++++++++++--------
 drivers/fpga/fpga-region.c       |  11 ++--
 include/linux/fpga/fpga-bridge.h |   7 ++-
 3 files changed, 100 insertions(+), 28 deletions(-)

diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
index fcd2bd3..af6d97e 100644
--- a/drivers/fpga/fpga-bridge.c
+++ b/drivers/fpga/fpga-bridge.c
@@ -2,6 +2,7 @@
  * FPGA Bridge Framework Driver
  *
  *  Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved.
+ *  Copyright (C) 2017 Intel Corporation
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge)
 }
 EXPORT_SYMBOL_GPL(fpga_bridge_disable);
 
-/**
- * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
- *
- * @np: node pointer of a FPGA bridge
- * @info: fpga image specific information
- *
- * Return fpga_bridge struct if successful.
- * Return -EBUSY if someone already has a reference to the bridge.
- * Return -ENODEV if @np is not a FPGA Bridge.
- */
-struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
-				       struct fpga_image_info *info)
-
+struct fpga_bridge *__fpga_bridge_get(struct device *dev,
+				      struct fpga_image_info *info)
 {
-	struct device *dev;
 	struct fpga_bridge *bridge;
 	int ret = -ENODEV;
 
-	dev = class_find_device(fpga_bridge_class, NULL, np,
-				fpga_bridge_of_node_match);
-	if (!dev)
-		goto err_dev;
-
 	bridge = to_fpga_bridge(dev);
 	if (!bridge)
 		goto err_dev;
@@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
 	put_device(dev);
 	return ERR_PTR(ret);
 }
+
+/**
+ * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
+ *
+ * @np: node pointer of a FPGA bridge
+ * @info: fpga image specific information
+ *
+ * Return fpga_bridge struct if successful.
+ * Return -EBUSY if someone already has a reference to the bridge.
+ * Return -ENODEV if @np is not a FPGA Bridge.
+ */
+struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
+				       struct fpga_image_info *info)
+{
+	struct device *dev;
+
+	dev = class_find_device(fpga_bridge_class, NULL, np,
+				fpga_bridge_of_node_match);
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	return __fpga_bridge_get(dev, info);
+}
 EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
 
+static int fpga_bridge_dev_match(struct device *dev, const void *data)
+{
+	return dev->parent == data;
+}
+
+/**
+ * fpga_bridge_get - get an exclusive reference to a fpga bridge
+ * @dev:	parent device that fpga bridge was registered with
+ *
+ * Given a device, get an exclusive reference to a fpga bridge.
+ *
+ * Return: fpga manager struct or IS_ERR() condition containing error code.
+ */
+struct fpga_bridge *fpga_bridge_get(struct device *dev,
+				    struct fpga_image_info *info)
+{
+	struct device *bridge_dev;
+
+	bridge_dev = class_find_device(fpga_bridge_class, NULL, dev,
+				       fpga_bridge_dev_match);
+	if (!bridge_dev)
+		return ERR_PTR(-ENODEV);
+
+	return __fpga_bridge_get(bridge_dev, info);
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_get);
+
 /**
  * fpga_bridge_put - release a reference to a bridge
  *
@@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)
 EXPORT_SYMBOL_GPL(fpga_bridges_put);
 
 /**
- * fpga_bridges_get_to_list - get a bridge, add it to a list
+ * of_fpga_bridge_get_to_list - get a bridge, add it to a list
  *
  * @np: node pointer of a FPGA bridge
  * @info: fpga image specific information
@@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);
  *
  * Return 0 for success, error code from of_fpga_bridge_get() othewise.
  */
-int fpga_bridge_get_to_list(struct device_node *np,
+int of_fpga_bridge_get_to_list(struct device_node *np,
+			       struct fpga_image_info *info,
+			       struct list_head *bridge_list)
+{
+	struct fpga_bridge *bridge;
+	unsigned long flags;
+
+	bridge = of_fpga_bridge_get(np, info);
+	if (IS_ERR(bridge))
+		return PTR_ERR(bridge);
+
+	spin_lock_irqsave(&bridge_list_lock, flags);
+	list_add(&bridge->node, bridge_list);
+	spin_unlock_irqrestore(&bridge_list_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_fpga_bridge_get_to_list);
+
+/**
+ * fpga_bridge_get_to_list - given device, get a bridge, add it to a list
+ *
+ * @dev: FPGA bridge device
+ * @info: fpga image specific information
+ * @bridge_list: list of FPGA bridges
+ *
+ * Get an exclusive reference to the bridge and and it to the list.
+ *
+ * Return 0 for success, error code from fpga_bridge_get() othewise.
+ */
+int fpga_bridge_get_to_list(struct device *dev,
 			    struct fpga_image_info *info,
 			    struct list_head *bridge_list)
 {
 	struct fpga_bridge *bridge;
 	unsigned long flags;
 
-	bridge = of_fpga_bridge_get(np, info);
+	bridge = fpga_bridge_get(dev, info);
 	if (IS_ERR(bridge))
 		return PTR_ERR(bridge);
 
@@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void)
 }
 
 MODULE_DESCRIPTION("FPGA Bridge Driver");
-MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_AUTHOR("Alan Tull <atull@kernel.org>");
 MODULE_LICENSE("GPL v2");
 
 subsys_initcall(fpga_bridge_dev_init);
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 3b6b2f4..655940f 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,
 	int i, ret;
 
 	/* If parent is a bridge, add to list */
-	ret = fpga_bridge_get_to_list(region_np->parent, region->info,
-				      &region->bridge_list);
+	ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
+					 &region->bridge_list);
+
+	/* -EBUSY means parent is a bridge that is under use. Give up. */
 	if (ret == -EBUSY)
 		return ret;
 
+	/* Zero return code means parent was a bridge and was added to list. */
 	if (!ret)
 		parent_br = region_np->parent;
 
@@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
 			continue;
 
 		/* If node is a bridge, get it and add to list */
-		ret = fpga_bridge_get_to_list(br, region->info,
-					      &region->bridge_list);
+		ret = of_fpga_bridge_get_to_list(br, region->info,
+						 &region->bridge_list);
 
 		/* If any of the bridges are in use, give up */
 		if (ret == -EBUSY) {
diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
index dba6e3c..9f6696b 100644
--- a/include/linux/fpga/fpga-bridge.h
+++ b/include/linux/fpga/fpga-bridge.h
@@ -42,6 +42,8 @@ struct fpga_bridge {
 
 struct fpga_bridge *of_fpga_bridge_get(struct device_node *node,
 				       struct fpga_image_info *info);
+struct fpga_bridge *fpga_bridge_get(struct device *dev,
+				    struct fpga_image_info *info);
 void fpga_bridge_put(struct fpga_bridge *bridge);
 int fpga_bridge_enable(struct fpga_bridge *bridge);
 int fpga_bridge_disable(struct fpga_bridge *bridge);
@@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);
 int fpga_bridges_enable(struct list_head *bridge_list);
 int fpga_bridges_disable(struct list_head *bridge_list);
 void fpga_bridges_put(struct list_head *bridge_list);
-int fpga_bridge_get_to_list(struct device_node *np,
+int fpga_bridge_get_to_list(struct device *dev,
 			    struct fpga_image_info *info,
 			    struct list_head *bridge_list);
+int of_fpga_bridge_get_to_list(struct device_node *np,
+			       struct fpga_image_info *info,
+			       struct list_head *bridge_list);
 
 int fpga_bridge_register(struct device *dev, const char *name,
 			 const struct fpga_bridge_ops *br_ops, void *priv);
-- 
2.7.4

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

* [PATCH v2 03/16] fpga: mgr: API change to replace fpga load functions with single function
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
  2017-04-20 14:09 ` [PATCH v2 01/16] doc: fpga: update documents for the FPGA API Alan Tull
  2017-04-20 14:09 ` [PATCH v2 02/16] fpga: bridge: support getting bridge from device Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-04-21 20:08   ` Alan Tull
  2017-04-20 14:09 ` [PATCH v2 04/16] fpga: mgr: separate getting/locking FPGA manager Alan Tull
                   ` (13 subsequent siblings)
  16 siblings, 1 reply; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

fpga-mgr has three methods for programming FPGAs, depending on
whether the image is in a scatter gather list, a contiguous
buffer, or a firmware file. This makes it difficult to write
upper layers as the caller has to assume whether the FPGA image
is in a sg table, as a single buffer, or a firmware file.
This commit moves these parameters to struct fpga_image_info
and adds a single function for programming fpgas.

New functions:
* fpga_mgr_load - given fpga manager and struct fpga_image_info,
   program the fpga.

* fpga_image_info_alloc - alloc a struct fpga_image_info.

* fpga_image_info_free - free a struct fpga_image_info.

These three functions are unexported:
* fpga_mgr_buf_load_sg
* fpga_mgr_buf_load
* fpga_mgr_firmware_load

Also use devm_kstrdup to copy firmware_name so we aren't making
assumptions about where it comes from when allocing/freeing the
struct fpga_image_info.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: add fpga_image_info_alloc/free
    update copyright and author email
---
 drivers/fpga/fpga-mgr.c       | 59 +++++++++++++++++++++++++++++++++++--------
 drivers/fpga/fpga-region.c    | 42 +++++++++++++++++++-----------
 include/linux/fpga/fpga-mgr.h | 22 ++++++++++------
 3 files changed, 89 insertions(+), 34 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 188ffef..3478aab 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -2,6 +2,7 @@
  * FPGA Manager Core
  *
  *  Copyright (C) 2013-2015 Altera Corporation
+ *  Copyright (C) 2017 Intel Corporation
  *
  * With code from the mailing list:
  * Copyright (C) 2013 Xilinx, Inc.
@@ -31,6 +32,31 @@
 static DEFINE_IDA(fpga_mgr_ida);
 static struct class *fpga_mgr_class;
 
+struct fpga_image_info *fpga_image_info_alloc(struct device *dev)
+{
+	struct fpga_image_info *info;
+
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	return info;
+}
+EXPORT_SYMBOL_GPL(fpga_image_info_alloc);
+
+void fpga_image_info_free(struct device *dev,
+			  struct fpga_image_info *info)
+{
+	if (!info)
+		return;
+
+	if (info->firmware_name)
+		devm_kfree(dev, info->firmware_name);
+
+	devm_kfree(dev, info);
+}
+EXPORT_SYMBOL_GPL(fpga_image_info_free);
+
 /*
  * Call the low level driver's write_init function.  This will do the
  * device-specific things to get the FPGA into the state where it is ready to
@@ -137,8 +163,9 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr,
  *
  * Return: 0 on success, negative error code otherwise.
  */
-int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, struct fpga_image_info *info,
-			 struct sg_table *sgt)
+static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
+				struct fpga_image_info *info,
+				struct sg_table *sgt)
 {
 	int ret;
 
@@ -170,7 +197,6 @@ int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, struct fpga_image_info *info,
 
 	return fpga_mgr_write_complete(mgr, info);
 }
-EXPORT_SYMBOL_GPL(fpga_mgr_buf_load_sg);
 
 static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
 				    struct fpga_image_info *info,
@@ -210,8 +236,9 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
  *
  * Return: 0 on success, negative error code otherwise.
  */
-int fpga_mgr_buf_load(struct fpga_manager *mgr, struct fpga_image_info *info,
-		      const char *buf, size_t count)
+static int fpga_mgr_buf_load(struct fpga_manager *mgr,
+			     struct fpga_image_info *info,
+			     const char *buf, size_t count)
 {
 	struct page **pages;
 	struct sg_table sgt;
@@ -266,7 +293,6 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, struct fpga_image_info *info,
 
 	return rc;
 }
-EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
 
 /**
  * fpga_mgr_firmware_load - request firmware and load to fpga
@@ -282,9 +308,9 @@ EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
  *
  * Return: 0 on success, negative error code otherwise.
  */
-int fpga_mgr_firmware_load(struct fpga_manager *mgr,
-			   struct fpga_image_info *info,
-			   const char *image_name)
+static int fpga_mgr_firmware_load(struct fpga_manager *mgr,
+				  struct fpga_image_info *info,
+				  const char *image_name)
 {
 	struct device *dev = &mgr->dev;
 	const struct firmware *fw;
@@ -307,7 +333,18 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
+
+int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
+{
+	if (info->sgt)
+		return fpga_mgr_buf_load_sg(mgr, info, info->sgt);
+	if (info->buf && info->count)
+		return fpga_mgr_buf_load(mgr, info, info->buf, info->count);
+	if (info->firmware_name)
+		return fpga_mgr_firmware_load(mgr, info, info->firmware_name);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_load);
 
 static const char * const state_str[] = {
 	[FPGA_MGR_STATE_UNKNOWN] =		"unknown",
@@ -578,7 +615,7 @@ static void __exit fpga_mgr_class_exit(void)
 	ida_destroy(&fpga_mgr_ida);
 }
 
-MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_AUTHOR("Alan Tull <atull@kernel.org>");
 MODULE_DESCRIPTION("FPGA manager framework");
 MODULE_LICENSE("GPL v2");
 
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 655940f..93e4003 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -226,14 +226,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;
@@ -264,7 +261,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;
@@ -357,16 +354,14 @@ 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;
+	const char *firmware_name;
 	int ret;
 
-	info = devm_kzalloc(&region->dev, sizeof(*info), GFP_KERNEL);
+	info = fpga_image_info_alloc(&region->dev);
 	if (!info)
 		return -ENOMEM;
 
-	region->info = info;
-
 	/* Reject overlay if child FPGA Regions have firmware-name property */
 	ret = child_regions_with_firmware(nd->overlay);
 	if (ret)
@@ -382,7 +377,13 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 	if (of_property_read_bool(nd->overlay, "encrypted-fpga-config"))
 		info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
 
-	of_property_read_string(nd->overlay, "firmware-name", &firmware_name);
+	if (!of_property_read_string(nd->overlay, "firmware-name",
+				     &firmware_name)) {
+		info->firmware_name = devm_kstrdup(dev, firmware_name,
+						   GFP_KERNEL);
+		if (!info->firmware_name)
+			return -ENOMEM;
+	}
 
 	of_property_read_u32(nd->overlay, "region-unfreeze-timeout-us",
 			     &info->enable_timeout_us);
@@ -394,22 +395,33 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 			     &info->config_complete_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");
+		fpga_image_info_free(dev, info);
 		return -EINVAL;
 	}
 
 	/* FPGA is already configured externally.  We're done. */
-	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG)
+	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
+		fpga_image_info_free(&region->dev, info);
 		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");
+		fpga_image_info_free(dev, info);
 		return -EINVAL;
 	}
 
-	return fpga_region_program_fpga(region, firmware_name, nd->overlay);
+	region->info = info;
+	ret = fpga_region_program_fpga(region, nd->overlay);
+	if (ret) {
+		fpga_image_info_free(&region->dev, info);
+		region->info = NULL;
+	}
+
+	return ret;
 }
 
 /**
@@ -426,7 +438,7 @@ static void fpga_region_notify_post_remove(struct fpga_region *region,
 {
 	fpga_bridges_disable(&region->bridge_list);
 	fpga_bridges_put(&region->bridge_list);
-	devm_kfree(&region->dev, region->info);
+	fpga_image_info_free(&region->dev, region->info);
 	region->info = NULL;
 }
 
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index b4ac24c..1c53aa3 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -1,7 +1,8 @@
 /*
  * FPGA Framework
  *
- *  Copyright (C) 2013-2015 Altera Corporation
+ *  Copyright (C) 2013-2016 Altera Corporation
+ *  Copyright (C) 2017 Intel Corporation
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -79,12 +80,20 @@ enum fpga_mgr_states {
  * @disable_timeout_us: maximum time to disable traffic through bridge (uSec)
  * @config_complete_timeout_us: maximum time for FPGA to switch to operating
  *	   status in the write_complete op.
+ * @firmware_name: name of FPGA image firmware file
+ * @sgt: scatter/gather table containing FPGA image
+ * @buf: contiguous buffer containing FPGA image
+ * @count: size of buf
  */
 struct fpga_image_info {
 	u32 flags;
 	u32 enable_timeout_us;
 	u32 disable_timeout_us;
 	u32 config_complete_timeout_us;
+	char *firmware_name;
+	struct sg_table *sgt;
+	const char *buf;
+	size_t count;
 };
 
 /**
@@ -134,14 +143,11 @@ struct fpga_manager {
 
 #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
 
-int fpga_mgr_buf_load(struct fpga_manager *mgr, struct fpga_image_info *info,
-		      const char *buf, size_t count);
-int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, struct fpga_image_info *info,
-			 struct sg_table *sgt);
+struct fpga_image_info *fpga_image_info_alloc(struct device *dev);
 
-int fpga_mgr_firmware_load(struct fpga_manager *mgr,
-			   struct fpga_image_info *info,
-			   const char *image_name);
+void fpga_image_info_free(struct device *dev, struct fpga_image_info *info);
+
+int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
 
 struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
 
-- 
2.7.4

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

* [PATCH v2 04/16] fpga: mgr: separate getting/locking FPGA manager
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (2 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 03/16] fpga: mgr: API change to replace fpga load functions with single function Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-04-20 14:09 ` [PATCH v2 05/16] fpga: region: use dev_err instead of pr_err Alan Tull
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

This commit makes it straightforward to save a reference to an
FPGA manager and only lock it when programming the FPGA.

Add functions that get an FPGA manager's mutex for exclusive use:
* fpga_mgr_lock
* fpga_mgr_unlock

The following functions no longer lock an FPGA manager's mutex:
* of_fpga_mgr_get
* fpga_mgr_get
* fpga_mgr_put

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: add static for __fpga_mgr_get
    function documentation corrections
    use dev_err
---
 drivers/fpga/fpga-mgr.c       | 52 ++++++++++++++++++++++++++++++-------------
 drivers/fpga/fpga-region.c    | 13 +++++++++--
 include/linux/fpga/fpga-mgr.h |  3 +++
 3 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 3478aab..be13cce 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -401,28 +401,19 @@ ATTRIBUTE_GROUPS(fpga_mgr);
 static struct fpga_manager *__fpga_mgr_get(struct device *dev)
 {
 	struct fpga_manager *mgr;
-	int ret = -ENODEV;
 
 	mgr = to_fpga_manager(dev);
 	if (!mgr)
 		goto err_dev;
 
-	/* Get exclusive use of fpga manager */
-	if (!mutex_trylock(&mgr->ref_mutex)) {
-		ret = -EBUSY;
-		goto err_dev;
-	}
-
 	if (!try_module_get(dev->parent->driver->owner))
-		goto err_ll_mod;
+		goto err_dev;
 
 	return mgr;
 
-err_ll_mod:
-	mutex_unlock(&mgr->ref_mutex);
 err_dev:
 	put_device(dev);
-	return ERR_PTR(ret);
+	return ERR_PTR(-ENODEV);
 }
 
 static int fpga_mgr_dev_match(struct device *dev, const void *data)
@@ -431,10 +422,10 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data)
 }
 
 /**
- * fpga_mgr_get - get an exclusive reference to a fpga mgr
+ * fpga_mgr_get - get a reference to a fpga mgr
  * @dev:	parent device that fpga mgr was registered with
  *
- * Given a device, get an exclusive reference to a fpga mgr.
+ * Given a device, get a reference to a fpga mgr.
  *
  * Return: fpga manager struct or IS_ERR() condition containing error code.
  */
@@ -455,10 +446,10 @@ static int fpga_mgr_of_node_match(struct device *dev, const void *data)
 }
 
 /**
- * of_fpga_mgr_get - get an exclusive reference to a fpga mgr
+ * of_fpga_mgr_get - get a reference to a fpga mgr
  * @node:	device node
  *
- * Given a device node, get an exclusive reference to a fpga mgr.
+ * Given a device node, get a reference to a fpga mgr.
  *
  * Return: fpga manager struct or IS_ERR() condition containing error code.
  */
@@ -482,12 +473,41 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
 void fpga_mgr_put(struct fpga_manager *mgr)
 {
 	module_put(mgr->dev.parent->driver->owner);
-	mutex_unlock(&mgr->ref_mutex);
 	put_device(&mgr->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
 /**
+ * fpga_mgr_lock - Lock FPGA manager for exclusive use
+ * @mgr:	fpga manager
+ *
+ * Given a pointer to FPGA Manager (from fpga_mgr_get() or
+ * of_fpga_mgr_put()) attempt to get the mutex.
+ *
+ * Return: 0 for success or -EBUSY
+ */
+int fpga_mgr_lock(struct fpga_manager *mgr)
+{
+	if (!mutex_trylock(&mgr->ref_mutex)) {
+		dev_err(&mgr->dev, "FPGA manager is in use.\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_lock);
+
+/**
+ * fpga_mgr_unlock - Unlock FPGA manager
+ * @mgr:	fpga manager
+ */
+void fpga_mgr_unlock(struct fpga_manager *mgr)
+{
+	mutex_unlock(&mgr->ref_mutex);
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
+
+/**
  * fpga_mgr_register - register a low level fpga manager driver
  * @dev:	fpga manager device from pdev
  * @name:	fpga manager name
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 93e4003..5fa3c7d 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -125,7 +125,7 @@ static void fpga_region_put(struct fpga_region *region)
 }
 
 /**
- * fpga_region_get_manager - get exclusive reference for FPGA manager
+ * fpga_region_get_manager - get reference for FPGA manager
  * @region: FPGA region
  *
  * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
@@ -249,10 +249,16 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_region;
 	}
 
+	ret = fpga_mgr_lock(mgr);
+	if (ret) {
+		dev_err(dev, "FPGA manager is busy\n");
+		goto err_put_mgr;
+	}
+
 	ret = fpga_region_get_bridges(region, overlay);
 	if (ret) {
 		pr_err("failed to get fpga region bridges\n");
-		goto err_put_mgr;
+		goto err_unlock_mgr;
 	}
 
 	ret = fpga_bridges_disable(&region->bridge_list);
@@ -273,6 +279,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_br;
 	}
 
+	fpga_mgr_unlock(mgr);
 	fpga_mgr_put(mgr);
 	fpga_region_put(region);
 
@@ -280,6 +287,8 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 
 err_put_br:
 	fpga_bridges_put(&region->bridge_list);
+err_unlock_mgr:
+	fpga_mgr_unlock(mgr);
 err_put_mgr:
 	fpga_mgr_put(mgr);
 err_put_region:
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 1c53aa3..4e14bdb 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -149,6 +149,9 @@ void fpga_image_info_free(struct device *dev, struct fpga_image_info *info);
 
 int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
 
+int fpga_mgr_lock(struct fpga_manager *mgr);
+void fpga_mgr_unlock(struct fpga_manager *mgr);
+
 struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
 
 struct fpga_manager *fpga_mgr_get(struct device *dev);
-- 
2.7.4

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

* [PATCH v2 05/16] fpga: region: use dev_err instead of pr_err
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (3 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 04/16] fpga: mgr: separate getting/locking FPGA manager Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-04-20 14:09 ` [PATCH v2 06/16] fpga: region: remove unneeded of_node_get and put Alan Tull
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

Use dev_err messages instead of pr_err.

Also s/&region->dev/dev/ in two places where we already
have dev = &region->dev

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: new in this version of the patchset
---
 drivers/fpga/fpga-region.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 5fa3c7d..eaea9d4 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -102,7 +102,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)
 		return ERR_PTR(-ENODEV);
 	}
 
-	dev_dbg(&region->dev, "get\n");
+	dev_dbg(dev, "get\n");
 
 	return region;
 }
@@ -116,7 +116,7 @@ static void fpga_region_put(struct fpga_region *region)
 {
 	struct device *dev = &region->dev;
 
-	dev_dbg(&region->dev, "put\n");
+	dev_dbg(dev, "put\n");
 
 	module_put(dev->parent->driver->owner);
 	of_node_put(dev->of_node);
@@ -233,18 +233,19 @@ static int fpga_region_get_bridges(struct fpga_region *region,
 static int fpga_region_program_fpga(struct fpga_region *region,
 				    struct device_node *overlay)
 {
+	struct device *dev = &region->dev;
 	struct fpga_manager *mgr;
 	int ret;
 
 	region = fpga_region_get(region);
 	if (IS_ERR(region)) {
-		pr_err("failed to get fpga region\n");
+		dev_err(dev, "failed to get FPGA region\n");
 		return PTR_ERR(region);
 	}
 
 	mgr = fpga_region_get_manager(region);
 	if (IS_ERR(mgr)) {
-		pr_err("failed to get fpga region manager\n");
+		dev_err(dev, "failed to get FPGA manager\n");
 		ret = PTR_ERR(mgr);
 		goto err_put_region;
 	}
@@ -257,25 +258,25 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 
 	ret = fpga_region_get_bridges(region, overlay);
 	if (ret) {
-		pr_err("failed to get fpga region bridges\n");
+		dev_err(dev, "failed to get FPGA bridges\n");
 		goto err_unlock_mgr;
 	}
 
 	ret = fpga_bridges_disable(&region->bridge_list);
 	if (ret) {
-		pr_err("failed to disable region bridges\n");
+		dev_err(dev, "failed to disable bridges\n");
 		goto err_put_br;
 	}
 
 	ret = fpga_mgr_load(mgr, region->info);
 	if (ret) {
-		pr_err("failed to load fpga image\n");
+		dev_err(dev, "failed to load FPGA image\n");
 		goto err_put_br;
 	}
 
 	ret = fpga_bridges_enable(&region->bridge_list);
 	if (ret) {
-		pr_err("failed to enable region bridges\n");
+		dev_err(dev, "failed to enable region bridges\n");
 		goto err_put_br;
 	}
 
@@ -363,6 +364,7 @@ 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)
 {
+	struct device *dev = &region->dev;
 	struct fpga_image_info *info;
 	const char *firmware_name;
 	int ret;
@@ -405,7 +407,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 
 	/* 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");
+		dev_err(dev, "error: specified firmware and external-fpga-config");
 		fpga_image_info_free(dev, info);
 		return -EINVAL;
 	}
@@ -418,7 +420,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 
 	/* 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");
+		dev_err(dev, "should specify firmware-name or external-fpga-config\n");
 		fpga_image_info_free(dev, info);
 		return -EINVAL;
 	}
-- 
2.7.4

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

* [PATCH v2 06/16] fpga: region: remove unneeded of_node_get and put
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (4 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 05/16] fpga: region: use dev_err instead of pr_err Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-05-05 17:08   ` Moritz Fischer
  2017-04-20 14:09 ` [PATCH v2 07/16] fpga: region: get mgr early on Alan Tull
                   ` (10 subsequent siblings)
  16 siblings, 1 reply; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

Remove of_node_get/put in fpga_region_get/put.  Not
needed and will get in the way when I separate out
the common FPGA region code from Device Tree support
code.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
---
 drivers/fpga/fpga-region.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index eaea9d4..6784357 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -94,9 +94,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)
 	}
 
 	get_device(dev);
-	of_node_get(dev->of_node);
 	if (!try_module_get(dev->parent->driver->owner)) {
-		of_node_put(dev->of_node);
 		put_device(dev);
 		mutex_unlock(&region->mutex);
 		return ERR_PTR(-ENODEV);
@@ -119,7 +117,6 @@ static void fpga_region_put(struct fpga_region *region)
 	dev_dbg(dev, "put\n");
 
 	module_put(dev->parent->driver->owner);
-	of_node_put(dev->of_node);
 	put_device(dev);
 	mutex_unlock(&region->mutex);
 }
-- 
2.7.4

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

* [PATCH v2 07/16] fpga: region: get mgr early on
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (5 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 06/16] fpga: region: remove unneeded of_node_get and put Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-04-20 14:09 ` [PATCH v2 08/16] fpga: region: check for child regions before allocing image info Alan Tull
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

Get the FPGA manager during region creation.

This is a baby step in refactoring the FPGA region code to
separate out common FPGA region code from FPGA region
Device Tree overlay support.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
---
 drivers/fpga/fpga-region.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 6784357..7ff5426 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -31,12 +31,14 @@
  * @dev: FPGA Region device
  * @mutex: enforces exclusive reference to region
  * @bridge_list: list of FPGA bridges specified in region
+ * @mgr: FPGA manager
  * @info: fpga image specific information
  */
 struct fpga_region {
 	struct device dev;
 	struct mutex mutex; /* for exclusive reference to region */
 	struct list_head bridge_list;
+	struct fpga_manager *mgr;
 	struct fpga_image_info *info;
 };
 
@@ -123,7 +125,7 @@ static void fpga_region_put(struct fpga_region *region)
 
 /**
  * fpga_region_get_manager - get reference for FPGA manager
- * @region: FPGA region
+ * @np: device node of FPGA region
  *
  * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
  *
@@ -131,10 +133,8 @@ static void fpga_region_put(struct fpga_region *region)
  *
  * Return: fpga manager struct or IS_ERR() condition containing error code.
  */
-static struct fpga_manager *fpga_region_get_manager(struct fpga_region *region)
+static struct fpga_manager *fpga_region_get_manager(struct device_node *np)
 {
-	struct device *dev = &region->dev;
-	struct device_node *np = dev->of_node;
 	struct device_node  *mgr_node;
 	struct fpga_manager *mgr;
 
@@ -231,7 +231,6 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 				    struct device_node *overlay)
 {
 	struct device *dev = &region->dev;
-	struct fpga_manager *mgr;
 	int ret;
 
 	region = fpga_region_get(region);
@@ -240,17 +239,10 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		return PTR_ERR(region);
 	}
 
-	mgr = fpga_region_get_manager(region);
-	if (IS_ERR(mgr)) {
-		dev_err(dev, "failed to get FPGA manager\n");
-		ret = PTR_ERR(mgr);
-		goto err_put_region;
-	}
-
-	ret = fpga_mgr_lock(mgr);
+	ret = fpga_mgr_lock(region->mgr);
 	if (ret) {
 		dev_err(dev, "FPGA manager is busy\n");
-		goto err_put_mgr;
+		goto err_put_region;
 	}
 
 	ret = fpga_region_get_bridges(region, overlay);
@@ -265,7 +257,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(region->mgr, region->info);
 	if (ret) {
 		dev_err(dev, "failed to load FPGA image\n");
 		goto err_put_br;
@@ -277,8 +269,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_br;
 	}
 
-	fpga_mgr_unlock(mgr);
-	fpga_mgr_put(mgr);
+	fpga_mgr_unlock(region->mgr);
 	fpga_region_put(region);
 
 	return 0;
@@ -286,9 +277,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 err_put_br:
 	fpga_bridges_put(&region->bridge_list);
 err_unlock_mgr:
-	fpga_mgr_unlock(mgr);
-err_put_mgr:
-	fpga_mgr_put(mgr);
+	fpga_mgr_unlock(region->mgr);
 err_put_region:
 	fpga_region_put(region);
 
@@ -517,11 +506,20 @@ static int fpga_region_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct fpga_region *region;
+	struct fpga_manager *mgr;
 	int id, ret = 0;
 
+	mgr = fpga_region_get_manager(np);
+	if (IS_ERR(mgr))
+		return -EPROBE_DEFER;
+
 	region = kzalloc(sizeof(*region), GFP_KERNEL);
-	if (!region)
-		return -ENOMEM;
+	if (!region) {
+		ret = -ENOMEM;
+		goto err_put_mgr;
+	}
+
+	region->mgr = mgr;
 
 	id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL);
 	if (id < 0) {
@@ -557,6 +555,8 @@ static int fpga_region_probe(struct platform_device *pdev)
 	ida_simple_remove(&fpga_region_ida, id);
 err_kfree:
 	kfree(region);
+err_put_mgr:
+	fpga_mgr_put(mgr);
 
 	return ret;
 }
@@ -566,6 +566,7 @@ static int fpga_region_remove(struct platform_device *pdev)
 	struct fpga_region *region = platform_get_drvdata(pdev);
 
 	device_unregister(&region->dev);
+	fpga_mgr_put(region->mgr);
 
 	return 0;
 }
-- 
2.7.4

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

* [PATCH v2 08/16] fpga: region: check for child regions before allocing image info
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (6 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 07/16] fpga: region: get mgr early on Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-05-05 20:59   ` Moritz Fischer
  2017-04-20 14:09 ` [PATCH v2 09/16] fpga: region: fix slow warning with more than one overlay Alan Tull
                   ` (8 subsequent siblings)
  16 siblings, 1 reply; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

During a device tree overlay pre-apply notification, the check
for child FPGA regions can happen slightly earlier.  This saves
us from allocating the FPGA image info that just gets thrown
away.

This is a baby step in refactoring the FPGA region code to
separate out common FPGA region code from FPGA region
Device Tree overlay support.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
---
 drivers/fpga/fpga-region.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 7ff5426..c2730a8 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -355,15 +355,19 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 	const char *firmware_name;
 	int ret;
 
-	info = fpga_image_info_alloc(&region->dev);
-	if (!info)
-		return -ENOMEM;
-
-	/* Reject overlay if child FPGA Regions have firmware-name property */
+	/*
+	 * 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(nd->overlay);
 	if (ret)
 		return ret;
 
+	info = fpga_image_info_alloc(dev);
+	if (!info)
+		return -ENOMEM;
+
 	/* Read FPGA region properties from the overlay */
 	if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
 		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
-- 
2.7.4

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

* [PATCH v2 09/16] fpga: region: fix slow warning with more than one overlay
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (7 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 08/16] fpga: region: check for child regions before allocing image info Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-04-20 14:09 ` [PATCH v2 10/16] fpga: region: use image info as parameter for programming region Alan Tull
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

Prevent applying more than one overlay to a region.

This fixes a slow warning that happens when more than
one overlay is applied and then removed.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
---
 drivers/fpga/fpga-region.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index c2730a8..d6f2313 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -355,6 +355,11 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 	const char *firmware_name;
 	int ret;
 
+	if (region->info) {
+		dev_err(dev, "Region already has overlay applied.\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * Reject overlay if child FPGA Regions added in the overlay have
 	 * firmware-name property (would mean that an FPGA region that has
-- 
2.7.4

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

* [PATCH v2 10/16] fpga: region: use image info as parameter for programming region
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (8 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 09/16] fpga: region: fix slow warning with more than one overlay Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-05-09 15:25   ` Alan Tull
  2017-04-20 14:09 ` [PATCH v2 11/16] fpga: region: separate out code that parses the overlay Alan Tull
                   ` (6 subsequent siblings)
  16 siblings, 1 reply; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

Use FPGA image info as a parameter when region code is
programming the FPGA.

This is a baby step in refactoring the FPGA region code to
separate out common FPGA region code from FPGA region
Device Tree overlay support.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
---
 drivers/fpga/fpga-region.c    | 26 ++++++++++++++++----------
 include/linux/fpga/fpga-mgr.h |  4 ++++
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index d6f2313..1892126 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -223,16 +223,21 @@ 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.
+ * @info: FPGA image info
+ * Program an FPGA using fpga image info.
  * Return 0 for success or negative error code.
  */
 static int fpga_region_program_fpga(struct fpga_region *region,
-				    struct device_node *overlay)
+				    struct fpga_image_info *info)
 {
 	struct device *dev = &region->dev;
 	int ret;
 
+	if (region->info) {
+		dev_err(dev, "region already programmed\n");
+		return PTR_ERR(region);
+	}
+
 	region = fpga_region_get(region);
 	if (IS_ERR(region)) {
 		dev_err(dev, "failed to get FPGA region\n");
@@ -245,7 +250,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_region;
 	}
 
-	ret = fpga_region_get_bridges(region, overlay);
+	ret = fpga_region_get_bridges(region, info->overlay);
 	if (ret) {
 		dev_err(dev, "failed to get FPGA bridges\n");
 		goto err_unlock_mgr;
@@ -257,7 +262,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_br;
 	}
 
-	ret = fpga_mgr_load(region->mgr, region->info);
+	ret = fpga_mgr_load(region->mgr, info);
 	if (ret) {
 		dev_err(dev, "failed to load FPGA image\n");
 		goto err_put_br;
@@ -269,6 +274,8 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_br;
 	}
 
+	region->info = info;
+
 	fpga_mgr_unlock(region->mgr);
 	fpga_region_put(region);
 
@@ -373,6 +380,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 	if (!info)
 		return -ENOMEM;
 
+	info->overlay = nd->overlay;
+
 	/* Read FPGA region properties from the overlay */
 	if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
 		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
@@ -420,12 +429,9 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 		return -EINVAL;
 	}
 
-	region->info = info;
-	ret = fpga_region_program_fpga(region, nd->overlay);
-	if (ret) {
+	ret = fpga_region_program_fpga(region, info);
+	if (ret)
 		fpga_image_info_free(&region->dev, info);
-		region->info = NULL;
-	}
 
 	return ret;
 }
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 4e14bdb..66d0e32 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -84,6 +84,7 @@ enum fpga_mgr_states {
  * @sgt: scatter/gather table containing FPGA image
  * @buf: contiguous buffer containing FPGA image
  * @count: size of buf
+ * @overlay: Device Tree overlay
  */
 struct fpga_image_info {
 	u32 flags;
@@ -94,6 +95,9 @@ struct fpga_image_info {
 	struct sg_table *sgt;
 	const char *buf;
 	size_t count;
+#ifdef CONFIG_OF
+	struct device_node *overlay;
+#endif
 };
 
 /**
-- 
2.7.4

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

* [PATCH v2 11/16] fpga: region: separate out code that parses the overlay
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (9 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 10/16] fpga: region: use image info as parameter for programming region Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-04-20 14:09 ` [PATCH v2 12/16] fpga: region: add fpga-region.h header Alan Tull
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

New function of_fpga_region_parse_ov added, moving code
from fpga_region_notify_pre_apply.  This function
gets the FPGA image info from the overlay and is able
to simplify some of the logic involved.

This is a baby step in refactoring the FPGA region code to
separate out common code from Device Tree overlay support.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
---
 drivers/fpga/fpga-region.c | 129 +++++++++++++++++++++++++++------------------
 1 file changed, 77 insertions(+), 52 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 1892126..3fd4674a 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -329,33 +329,22 @@ 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.
+ * of_fpga_region_parse_ov - parse and check overlay applied to region
  *
- *   firmware-name         : program the FPGA
- *   external-fpga-config  : FPGA is already programmed
- *   encrypted-fpga-config : FPGA bitstream is encrypted
- *
- * 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 *of_fpga_region_parse_ov(
+						struct fpga_region *region,
+						struct device_node *overlay)
 {
 	struct device *dev = &region->dev;
 	struct fpga_image_info *info;
@@ -364,7 +353,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 
 	if (region->info) {
 		dev_err(dev, "Region already has overlay applied.\n");
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	/*
@@ -372,68 +361,104 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 	 * 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(nd->overlay);
+	ret = child_regions_with_firmware(overlay);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
 	info = fpga_image_info_alloc(dev);
 	if (!info)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
-	info->overlay = nd->overlay;
+	info->overlay = overlay;
 
 	/* 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;
 
-	if (of_property_read_bool(nd->overlay, "encrypted-fpga-config"))
+	if (of_property_read_bool(overlay, "encrypted-fpga-config"))
 		info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
 
-	if (!of_property_read_string(nd->overlay, "firmware-name",
+	if (!of_property_read_string(overlay, "firmware-name",
 				     &firmware_name)) {
 		info->firmware_name = devm_kstrdup(dev, firmware_name,
 						   GFP_KERNEL);
 		if (!info->firmware_name)
-			return -ENOMEM;
+			return ERR_PTR(-ENOMEM);
 	}
 
-	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);
 
-	of_property_read_u32(nd->overlay, "config-complete-timeout-us",
+	of_property_read_u32(overlay, "config-complete-timeout-us",
 			     &info->config_complete_timeout_us);
 
-	/* If FPGA was externally programmed, don't specify firmware */
-	if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && info->firmware_name) {
-		dev_err(dev, "error: specified firmware and external-fpga-config");
-		fpga_image_info_free(dev, info);
-		return -EINVAL;
+	/* If overlay is not programming the FPGA, don't need FPGA image info */
+	if (!info->firmware_name) {
+		ret = 0;
+		goto ret_no_info;
 	}
 
-	/* FPGA is already configured externally.  We're done. */
+	/*
+	 * If overlay informs us FPGA was externally programmed, specifying
+	 * firmware here would be ambiguous.
+	 */
 	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
-		fpga_image_info_free(&region->dev, info);
-		return 0;
+		dev_err(dev, "error: specified firmware and external-fpga-config");
+		ret = -EINVAL;
+		goto ret_no_info;
 	}
 
-	/* If we got this far, we should be programming the FPGA */
-	if (!info->firmware_name) {
-		dev_err(dev, "should specify firmware-name or external-fpga-config\n");
-		fpga_image_info_free(dev, info);
+	return info;
+ret_no_info:
+	fpga_image_info_free(dev, info);
+	return ERR_PTR(ret);
+}
+
+/**
+ * fpga_region_notify_pre_apply - pre-apply overlay notification
+ *
+ * @region: FPGA region that the overlay was 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 checks fail, overlay is rejected and does not get added to the
+ * live tree.
+ *
+ * Returns 0 for success or negative error code for failure.
+ */
+static int fpga_region_notify_pre_apply(struct fpga_region *region,
+					struct of_overlay_notify_data *nd)
+{
+	struct device *dev = &region->dev;
+	struct fpga_image_info *info;
+	int ret;
+
+	if (region->info) {
+		dev_err(dev, "Region already has overlay applied.\n");
 		return -EINVAL;
 	}
 
-	ret = fpga_region_program_fpga(region, info);
-	if (ret)
-		fpga_image_info_free(&region->dev, info);
+	info = of_fpga_region_parse_ov(region, nd->overlay);
+	if (IS_ERR(info))
+		return PTR_ERR(info);
 
-	return ret;
+	if (info) {
+		ret = fpga_region_program_fpga(region, info);
+		if (ret) {
+			fpga_image_info_free(dev, info);
+			return ret;
+		}
+	}
+
+	return 0;
 }
 
 /**
-- 
2.7.4

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

* [PATCH v2 12/16] fpga: region: add fpga-region.h header
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (10 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 11/16] fpga: region: separate out code that parses the overlay Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-04-20 14:09 ` [PATCH v2 13/16] fpga: region: rename some functions prior to moving Alan Tull
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

* Create fpga-region.h.
* Export fpga_region_program_fpga.
* Move struct fpga_region and other things to the header.

This is a step in separating FPGA region common code
from Device Tree support.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
    update author email
---
 drivers/fpga/fpga-region.c       | 26 +++++---------------------
 include/linux/fpga/fpga-region.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 21 deletions(-)
 create mode 100644 include/linux/fpga/fpga-region.h

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 3fd4674a..f1d1d36 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -18,6 +18,7 @@
 
 #include <linux/fpga/fpga-bridge.h>
 #include <linux/fpga/fpga-mgr.h>
+#include <linux/fpga/fpga-region.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -26,24 +27,6 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
-/**
- * struct fpga_region - FPGA Region structure
- * @dev: FPGA Region device
- * @mutex: enforces exclusive reference to region
- * @bridge_list: list of FPGA bridges specified in region
- * @mgr: FPGA manager
- * @info: fpga image specific information
- */
-struct fpga_region {
-	struct device dev;
-	struct mutex mutex; /* for exclusive reference to region */
-	struct list_head bridge_list;
-	struct fpga_manager *mgr;
-	struct fpga_image_info *info;
-};
-
-#define to_fpga_region(d) container_of(d, struct fpga_region, dev)
-
 static DEFINE_IDA(fpga_region_ida);
 static struct class *fpga_region_class;
 
@@ -227,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
  * Program an FPGA using fpga image info.
  * Return 0 for success or negative error code.
  */
-static int fpga_region_program_fpga(struct fpga_region *region,
-				    struct fpga_image_info *info)
+int fpga_region_program_fpga(struct fpga_region *region,
+			     struct fpga_image_info *info)
 {
 	struct device *dev = &region->dev;
 	int ret;
@@ -290,6 +273,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(fpga_region_program_fpga);
 
 /**
  * child_regions_with_firmware
@@ -672,5 +656,5 @@ subsys_initcall(fpga_region_init);
 module_exit(fpga_region_exit);
 
 MODULE_DESCRIPTION("FPGA Region");
-MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_AUTHOR("Alan Tull <atull@kernel.org>");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
new file mode 100644
index 0000000..1075f94
--- /dev/null
+++ b/include/linux/fpga/fpga-region.h
@@ -0,0 +1,29 @@
+#include <linux/device.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/fpga/fpga-bridge.h>
+
+#ifndef _FPGA_REGION_H
+#define _FPGA_REGION_H
+
+/**
+ * struct fpga_region - FPGA Region structure
+ * @dev: FPGA Region device
+ * @mutex: enforces exclusive reference to region
+ * @bridge_list: list of FPGA bridges specified in region
+ * @mgr: FPGA manager
+ * @info: FPGA image info
+ */
+struct fpga_region {
+	struct device dev;
+	struct mutex mutex; /* for exclusive reference to region */
+	struct list_head bridge_list;
+	struct fpga_manager *mgr;
+	struct fpga_image_info *info;
+};
+
+#define to_fpga_region(d) container_of(d, struct fpga_region, dev)
+
+int fpga_region_program_fpga(struct fpga_region *region,
+			     struct fpga_image_info *image_info);
+
+#endif /* _FPGA_REGION_H */
-- 
2.7.4

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

* [PATCH v2 13/16] fpga: region: rename some functions prior to moving
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (11 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 12/16] fpga: region: add fpga-region.h header Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-04-20 14:09 ` [PATCH v2 14/16] fpga: region: add register/unregister functions Alan Tull
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

Rename some functions that will be moved to
of-fpga-region.c.  Also change some parameters
and export a function to help with refactoring.

This is a step towards the larger goal of separating
device tree support from FPGA region common code.

* fpga_region_get_manager -> of_fpga_region_get_mgr

* add 'of_' prefix to the following:
  * fpga_region_find
  * fpga_region_get_bridges
  * fpga_region_notify_pre_apply
  * fpga_region_notify_post_remove),
  * fpga_region_probe/remove

Parameter changes:
* of_fpga_region_find
  change parameter to be the device node of the region.
* of_fpga_region_get_bridges
  change second parameter to FPGA image info.

Export of_fpga_region_find as well.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
---
 drivers/fpga/fpga-region.c | 60 ++++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index f1d1d36..e87e9a9 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -42,12 +42,14 @@ static int fpga_region_of_node_match(struct device *dev, const void *data)
 }
 
 /**
- * fpga_region_find - find FPGA region
+ * of_fpga_region_find - find FPGA region
  * @np: device node of FPGA Region
+ *
  * Caller will need to put_device(&region->dev) when done.
+ *
  * Returns FPGA Region struct or NULL
  */
-static struct fpga_region *fpga_region_find(struct device_node *np)
+static struct fpga_region *of_fpga_region_find(struct device_node *np)
 {
 	struct device *dev;
 
@@ -107,7 +109,7 @@ static void fpga_region_put(struct fpga_region *region)
 }
 
 /**
- * fpga_region_get_manager - get reference for FPGA manager
+ * of_fpga_region_get_mgr - get reference for FPGA manager
  * @np: device node of FPGA region
  *
  * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
@@ -116,7 +118,7 @@ static void fpga_region_put(struct fpga_region *region)
  *
  * Return: fpga manager struct or IS_ERR() condition containing error code.
  */
-static struct fpga_manager *fpga_region_get_manager(struct device_node *np)
+static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)
 {
 	struct device_node  *mgr_node;
 	struct fpga_manager *mgr;
@@ -139,9 +141,9 @@ static struct fpga_manager *fpga_region_get_manager(struct device_node *np)
 }
 
 /**
- * fpga_region_get_bridges - create a list of bridges
+ * of_fpga_region_get_bridges - create a list of bridges
  * @region: FPGA region
- * @overlay: device node of the overlay
+ * @info: FPGA image info
  *
  * Create a list of bridges including the parent bridge and the bridges
  * specified by "fpga-bridges" property.  Note that the
@@ -154,8 +156,8 @@ static struct fpga_manager *fpga_region_get_manager(struct device_node *np)
  * Return 0 for success (even if there are no bridges specified)
  * or -EBUSY if any of the bridges are in use.
  */
-static int fpga_region_get_bridges(struct fpga_region *region,
-				   struct device_node *overlay)
+static int of_fpga_region_get_bridges(struct fpga_region *region,
+				      struct fpga_image_info *info)
 {
 	struct device *dev = &region->dev;
 	struct device_node *region_np = dev->of_node;
@@ -163,7 +165,7 @@ static int fpga_region_get_bridges(struct fpga_region *region,
 	int i, ret;
 
 	/* If parent is a bridge, add to list */
-	ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
+	ret = of_fpga_bridge_get_to_list(region_np->parent, info,
 					 &region->bridge_list);
 
 	/* -EBUSY means parent is a bridge that is under use. Give up. */
@@ -175,8 +177,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(info->overlay, "fpga-bridges", 0))
+		np = info->overlay;
 	else
 		np = region_np;
 
@@ -233,7 +235,7 @@ int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_region;
 	}
 
-	ret = fpga_region_get_bridges(region, info->overlay);
+	ret = of_fpga_region_get_bridges(region, info);
 	if (ret) {
 		dev_err(dev, "failed to get FPGA bridges\n");
 		goto err_unlock_mgr;
@@ -405,7 +407,7 @@ static struct fpga_image_info *of_fpga_region_parse_ov(
 }
 
 /**
- * fpga_region_notify_pre_apply - pre-apply overlay notification
+ * of_fpga_region_notify_pre_apply - pre-apply overlay notification
  *
  * @region: FPGA region that the overlay was applied to
  * @nd: overlay notification data
@@ -418,8 +420,8 @@ static struct fpga_image_info *of_fpga_region_parse_ov(
  *
  * Returns 0 for success or negative error code for failure.
  */
-static int fpga_region_notify_pre_apply(struct fpga_region *region,
-					struct of_overlay_notify_data *nd)
+static int of_fpga_region_notify_pre_apply(struct fpga_region *region,
+					   struct of_overlay_notify_data *nd)
 {
 	struct device *dev = &region->dev;
 	struct fpga_image_info *info;
@@ -446,7 +448,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
 }
 
 /**
- * fpga_region_notify_post_remove - post-remove overlay notification
+ * of_fpga_region_notify_post_remove - post-remove overlay notification
  *
  * @region: FPGA region that was targeted by the overlay that was removed
  * @nd: overlay notification data
@@ -454,8 +456,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
  * Called after an overlay has been removed if the overlay's target was a
  * FPGA region.
  */
-static void fpga_region_notify_post_remove(struct fpga_region *region,
-					   struct of_overlay_notify_data *nd)
+static void of_fpga_region_notify_post_remove(struct fpga_region *region,
+					      struct of_overlay_notify_data *nd)
 {
 	fpga_bridges_disable(&region->bridge_list);
 	fpga_bridges_put(&region->bridge_list);
@@ -498,18 +500,18 @@ static int of_fpga_region_notify(struct notifier_block *nb,
 		return NOTIFY_OK;
 	}
 
-	region = fpga_region_find(nd->target);
+	region = of_fpga_region_find(nd->target);
 	if (!region)
 		return NOTIFY_OK;
 
 	ret = 0;
 	switch (action) {
 	case OF_OVERLAY_PRE_APPLY:
-		ret = fpga_region_notify_pre_apply(region, nd);
+		ret = of_fpga_region_notify_pre_apply(region, nd);
 		break;
 
 	case OF_OVERLAY_POST_REMOVE:
-		fpga_region_notify_post_remove(region, nd);
+		of_fpga_region_notify_post_remove(region, nd);
 		break;
 	}
 
@@ -525,7 +527,7 @@ static struct notifier_block fpga_region_of_nb = {
 	.notifier_call = of_fpga_region_notify,
 };
 
-static int fpga_region_probe(struct platform_device *pdev)
+static int of_fpga_region_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
@@ -533,7 +535,7 @@ static int fpga_region_probe(struct platform_device *pdev)
 	struct fpga_manager *mgr;
 	int id, ret = 0;
 
-	mgr = fpga_region_get_manager(np);
+	mgr = of_fpga_region_get_mgr(np);
 	if (IS_ERR(mgr))
 		return -EPROBE_DEFER;
 
@@ -585,7 +587,7 @@ static int fpga_region_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int fpga_region_remove(struct platform_device *pdev)
+static int of_fpga_region_remove(struct platform_device *pdev)
 {
 	struct fpga_region *region = platform_get_drvdata(pdev);
 
@@ -595,9 +597,9 @@ static int fpga_region_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct platform_driver fpga_region_driver = {
-	.probe = fpga_region_probe,
-	.remove = fpga_region_remove,
+static struct platform_driver of_fpga_region_driver = {
+	.probe = of_fpga_region_probe,
+	.remove = of_fpga_region_remove,
 	.driver = {
 		.name	= "fpga-region",
 		.of_match_table = of_match_ptr(fpga_region_of_match),
@@ -630,7 +632,7 @@ static int __init fpga_region_init(void)
 	if (ret)
 		goto err_class;
 
-	ret = platform_driver_register(&fpga_region_driver);
+	ret = platform_driver_register(&of_fpga_region_driver);
 	if (ret)
 		goto err_plat;
 
@@ -646,7 +648,7 @@ static int __init fpga_region_init(void)
 
 static void __exit fpga_region_exit(void)
 {
-	platform_driver_unregister(&fpga_region_driver);
+	platform_driver_unregister(&of_fpga_region_driver);
 	of_overlay_notifier_unregister(&fpga_region_of_nb);
 	class_destroy(fpga_region_class);
 	ida_destroy(&fpga_region_ida);
-- 
2.7.4

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

* [PATCH v2 14/16] fpga: region: add register/unregister functions
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (12 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 13/16] fpga: region: rename some functions prior to moving Alan Tull
@ 2017-04-20 14:09 ` Alan Tull
  2017-04-20 14:10 ` [PATCH v2 15/16] fpga: region: add fpga_region_class_find Alan Tull
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:09 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

Another step in separating common code from device tree specific
code for FPGA regions.

* add FPGA region register/unregister functions.
* add the register/unregister functions to the header
* use devm_kzalloc to alloc the region.
* add a method for getting bridges to the region struct
* add priv to the region struct

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
---
 drivers/fpga/fpga-region.c       | 99 +++++++++++++++++++++++++---------------
 include/linux/fpga/fpga-region.h |  8 ++++
 2 files changed, 70 insertions(+), 37 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index e87e9a9..e2a3fe6 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -235,10 +235,16 @@ int fpga_region_program_fpga(struct fpga_region *region,
 		goto err_put_region;
 	}
 
-	ret = of_fpga_region_get_bridges(region, info);
-	if (ret) {
-		dev_err(dev, "failed to get FPGA bridges\n");
-		goto err_unlock_mgr;
+	/*
+	 * In some cases, we already have a list of bridges in the
+	 * fpga region struct.  Or we don't have any bridges.
+	 */
+	if (region->get_bridges) {
+		ret = region->get_bridges(region, info);
+		if (ret) {
+			dev_err(dev, "failed to get fpga region bridges\n");
+			goto err_unlock_mgr;
+		}
 	}
 
 	ret = fpga_bridges_disable(&region->bridge_list);
@@ -267,7 +273,8 @@ int fpga_region_program_fpga(struct fpga_region *region,
 	return 0;
 
 err_put_br:
-	fpga_bridges_put(&region->bridge_list);
+	if (region->get_bridges)
+		fpga_bridges_put(&region->bridge_list);
 err_unlock_mgr:
 	fpga_mgr_unlock(region->mgr);
 err_put_region:
@@ -527,39 +534,20 @@ static struct notifier_block fpga_region_of_nb = {
 	.notifier_call = of_fpga_region_notify,
 };
 
-static int of_fpga_region_probe(struct platform_device *pdev)
+int fpga_region_register(struct device *dev, struct fpga_region *region)
 {
-	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
-	struct fpga_region *region;
-	struct fpga_manager *mgr;
 	int id, ret = 0;
 
-	mgr = of_fpga_region_get_mgr(np);
-	if (IS_ERR(mgr))
-		return -EPROBE_DEFER;
-
-	region = kzalloc(sizeof(*region), GFP_KERNEL);
-	if (!region) {
-		ret = -ENOMEM;
-		goto err_put_mgr;
-	}
-
-	region->mgr = mgr;
-
 	id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL);
-	if (id < 0) {
-		ret = id;
-		goto err_kfree;
-	}
+	if (id < 0)
+		return id;
 
 	mutex_init(&region->mutex);
 	INIT_LIST_HEAD(&region->bridge_list);
-
 	device_initialize(&region->dev);
 	region->dev.class = fpga_region_class;
 	region->dev.parent = dev;
-	region->dev.of_node = np;
+	region->dev.of_node = dev->of_node;
 	region->dev.id = id;
 	dev_set_drvdata(dev, region);
 
@@ -571,19 +559,58 @@ static int of_fpga_region_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_remove;
 
+	return 0;
+
+err_remove:
+	ida_simple_remove(&fpga_region_ida, id);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_region_register);
+
+int fpga_region_unregister(struct fpga_region *region)
+{
+	device_unregister(&region->dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fpga_region_unregister);
+
+static int of_fpga_region_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct fpga_region *region;
+	struct fpga_manager *mgr;
+	int ret;
+
+	/* Find the FPGA mgr specified by region or parent region. */
+	mgr = of_fpga_region_get_mgr(np);
+	if (IS_ERR(mgr))
+		return -EPROBE_DEFER;
+
+	region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL);
+	if (!region) {
+		ret = -ENOMEM;
+		goto eprobe_mgr_put;
+	}
+
+	region->mgr = mgr;
+
+	/* Specify how to get bridges for this type of region. */
+	region->get_bridges = of_fpga_region_get_bridges;
+
+	ret = fpga_region_register(dev, region);
+	if (ret)
+		goto eprobe_mgr_put;
+
 	of_platform_populate(np, fpga_region_of_match, NULL, &region->dev);
 
 	dev_info(dev, "FPGA Region probed\n");
 
 	return 0;
 
-err_remove:
-	ida_simple_remove(&fpga_region_ida, id);
-err_kfree:
-	kfree(region);
-err_put_mgr:
+eprobe_mgr_put:
 	fpga_mgr_put(mgr);
-
 	return ret;
 }
 
@@ -591,8 +618,7 @@ static int of_fpga_region_remove(struct platform_device *pdev)
 {
 	struct fpga_region *region = platform_get_drvdata(pdev);
 
-	device_unregister(&region->dev);
-	fpga_mgr_put(region->mgr);
+	fpga_region_unregister(region);
 
 	return 0;
 }
@@ -611,7 +637,6 @@ static void fpga_region_dev_release(struct device *dev)
 	struct fpga_region *region = to_fpga_region(dev);
 
 	ida_simple_remove(&fpga_region_ida, region->dev.id);
-	kfree(region);
 }
 
 /**
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index 1075f94..35e7e09 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -12,6 +12,8 @@
  * @bridge_list: list of FPGA bridges specified in region
  * @mgr: FPGA manager
  * @info: FPGA image info
+ * @priv: private data
+ * @get_bridges: optional function to get bridges to a list
  */
 struct fpga_region {
 	struct device dev;
@@ -19,6 +21,9 @@ struct fpga_region {
 	struct list_head bridge_list;
 	struct fpga_manager *mgr;
 	struct fpga_image_info *info;
+	void *priv;
+	int (*get_bridges)(struct fpga_region *region,
+			   struct fpga_image_info *image_info);
 };
 
 #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
@@ -26,4 +31,7 @@ struct fpga_region {
 int fpga_region_program_fpga(struct fpga_region *region,
 			     struct fpga_image_info *image_info);
 
+int fpga_region_register(struct device *dev, struct fpga_region *region);
+int fpga_region_unregister(struct fpga_region *region);
+
 #endif /* _FPGA_REGION_H */
-- 
2.7.4

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

* [PATCH v2 15/16] fpga: region: add fpga_region_class_find
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (13 preceding siblings ...)
  2017-04-20 14:09 ` [PATCH v2 14/16] fpga: region: add register/unregister functions Alan Tull
@ 2017-04-20 14:10 ` Alan Tull
  2017-05-03 15:44   ` Moritz Fischer
  2017-04-20 14:10 ` [PATCH v2 16/16] fpga: region: move device tree support to of-fpga-region.c Alan Tull
  2017-04-20 14:16 ` [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
  16 siblings, 1 reply; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:10 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

Add a function for searching the fpga-region class.  This
will be useful when device tree code is no longer in the
same file that declares the fpga-region class.  Another
step in separating common FPGA region code from device
tree support.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
---
 drivers/fpga/fpga-region.c       | 23 +++++++++++++++--------
 include/linux/fpga/fpga-region.h |  4 ++++
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index e2a3fe6..7ffb8c1 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -30,6 +30,20 @@
 static DEFINE_IDA(fpga_region_ida);
 static struct class *fpga_region_class;
 
+struct fpga_region *fpga_region_class_find(
+	struct device *start, const void *data,
+	int (*match)(struct device *, const void *))
+{
+	struct device *dev;
+
+	dev = class_find_device(fpga_region_class, start, data, match);
+	if (!dev)
+		return NULL;
+
+	return to_fpga_region(dev);
+}
+EXPORT_SYMBOL_GPL(fpga_region_class_find);
+
 static const struct of_device_id fpga_region_of_match[] = {
 	{ .compatible = "fpga-region", },
 	{},
@@ -51,14 +65,7 @@ static int fpga_region_of_node_match(struct device *dev, const void *data)
  */
 static struct fpga_region *of_fpga_region_find(struct device_node *np)
 {
-	struct device *dev;
-
-	dev = class_find_device(fpga_region_class, NULL, np,
-				fpga_region_of_node_match);
-	if (!dev)
-		return NULL;
-
-	return to_fpga_region(dev);
+	return fpga_region_class_find(NULL, np, fpga_region_of_node_match);
 }
 
 /**
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index 35e7e09..4966bb2 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -28,6 +28,10 @@ struct fpga_region {
 
 #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
 
+struct fpga_region *fpga_region_class_find(
+	struct device *start, const void *data,
+	int (*match)(struct device *, const void *));
+
 int fpga_region_program_fpga(struct fpga_region *region,
 			     struct fpga_image_info *image_info);
 
-- 
2.7.4

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

* [PATCH v2 16/16] fpga: region: move device tree support to of-fpga-region.c
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (14 preceding siblings ...)
  2017-04-20 14:10 ` [PATCH v2 15/16] fpga: region: add fpga_region_class_find Alan Tull
@ 2017-04-20 14:10 ` Alan Tull
  2017-04-28  6:38   ` Wu Hao
  2017-04-20 14:16 ` [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
  16 siblings, 1 reply; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:10 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

Create of-fpga-region.c

Move the following functions without modification from
fpga-region.c to of-fpga-region.c:

* of_fpga_region_find
* of_fpga_region_get_mgr
* of_fpga_region_get_bridges
* child_regions_with_firmware
* of_fpga_region_parse_ov
* of_fpga_region_notify_pre_apply
* of_fpga_region_notify_post_remove
* of_fpga_region_notify
* of_fpga_region_probe
* of_fpga_region_remove

Create two new function with some code from fpga_region_init/exit.

* of_fpga_region_init
* of_fpga_region_exit

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out code changes into other patches, only move code here
---
 drivers/fpga/Kconfig          |  13 +-
 drivers/fpga/Makefile         |   1 +
 drivers/fpga/fpga-region.c    | 449 +-------------------------------------
 drivers/fpga/of-fpga-region.c | 493 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 505 insertions(+), 451 deletions(-)
 create mode 100644 drivers/fpga/of-fpga-region.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 161ba9d..394c141 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -15,10 +15,17 @@ if FPGA
 
 config FPGA_REGION
 	tristate "FPGA Region"
-	depends on OF && FPGA_BRIDGE
+	depends on FPGA_BRIDGE
+	help
+	  FPGA Region common code.  A FPGA Region controls a FPGA Manager
+	  and the FPGA Bridges associated with either a reconfigurable
+	  region of an FPGA or a whole FPGA.
+
+config OF_FPGA_REGION
+	tristate "FPGA Region Device Tree Overlay Support"
+	depends on OF && FPGA_REGION
 	help
-	  FPGA Regions allow loading FPGA images under control of
-	  the Device Tree.
+	  Support for loading FPGA images under control of Device Tree.
 
 config FPGA_MGR_ICE40_SPI
 	tristate "Lattice iCE40 SPI"
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 2a4f021..85b45d0 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_XILINX_PR_DECOUPLER)	+= xilinx-pr-decoupler.o
 
 # High Level Interfaces
 obj-$(CONFIG_FPGA_REGION)		+= fpga-region.o
+obj-$(CONFIG_OF_FPGA_REGION)		+= of-fpga-region.o
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 7ffb8c1..09622d8 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -2,6 +2,7 @@
  * FPGA Region - Device Tree support for FPGA programming under Linux
  *
  *  Copyright (C) 2013-2016 Altera Corporation
+ *  Copyright (C) 2017 Intel Corporation
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -23,7 +24,6 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
-#include <linux/of_platform.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
@@ -44,30 +44,6 @@ struct fpga_region *fpga_region_class_find(
 }
 EXPORT_SYMBOL_GPL(fpga_region_class_find);
 
-static const struct of_device_id fpga_region_of_match[] = {
-	{ .compatible = "fpga-region", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, fpga_region_of_match);
-
-static int fpga_region_of_node_match(struct device *dev, const void *data)
-{
-	return dev->of_node == data;
-}
-
-/**
- * of_fpga_region_find - find FPGA region
- * @np: device node of FPGA Region
- *
- * Caller will need to put_device(&region->dev) when done.
- *
- * Returns FPGA Region struct or NULL
- */
-static struct fpga_region *of_fpga_region_find(struct device_node *np)
-{
-	return fpga_region_class_find(NULL, np, fpga_region_of_node_match);
-}
-
 /**
  * fpga_region_get - get an exclusive reference to a fpga region
  * @region: FPGA Region struct
@@ -116,103 +92,6 @@ static void fpga_region_put(struct fpga_region *region)
 }
 
 /**
- * of_fpga_region_get_mgr - get reference for FPGA manager
- * @np: device node of FPGA region
- *
- * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
- *
- * Caller should call fpga_mgr_put() when done with manager.
- *
- * Return: fpga manager struct or IS_ERR() condition containing error code.
- */
-static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)
-{
-	struct device_node  *mgr_node;
-	struct fpga_manager *mgr;
-
-	of_node_get(np);
-	while (np) {
-		if (of_device_is_compatible(np, "fpga-region")) {
-			mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
-			if (mgr_node) {
-				mgr = of_fpga_mgr_get(mgr_node);
-				of_node_put(np);
-				return mgr;
-			}
-		}
-		np = of_get_next_parent(np);
-	}
-	of_node_put(np);
-
-	return ERR_PTR(-EINVAL);
-}
-
-/**
- * of_fpga_region_get_bridges - create a list of bridges
- * @region: FPGA region
- * @info: FPGA image info
- *
- * Create a list of bridges including the parent bridge and the bridges
- * specified by "fpga-bridges" property.  Note that the
- * fpga_bridges_enable/disable/put functions are all fine with an empty list
- * if that happens.
- *
- * Caller should call fpga_bridges_put(&region->bridge_list) when
- * done with the bridges.
- *
- * Return 0 for success (even if there are no bridges specified)
- * or -EBUSY if any of the bridges are in use.
- */
-static int of_fpga_region_get_bridges(struct fpga_region *region,
-				      struct fpga_image_info *info)
-{
-	struct device *dev = &region->dev;
-	struct device_node *region_np = dev->of_node;
-	struct device_node *br, *np, *parent_br = NULL;
-	int i, ret;
-
-	/* If parent is a bridge, add to list */
-	ret = of_fpga_bridge_get_to_list(region_np->parent, info,
-					 &region->bridge_list);
-
-	/* -EBUSY means parent is a bridge that is under use. Give up. */
-	if (ret == -EBUSY)
-		return ret;
-
-	/* Zero return code means parent was a bridge and was added to list. */
-	if (!ret)
-		parent_br = region_np->parent;
-
-	/* If overlay has a list of bridges, use it. */
-	if (of_parse_phandle(info->overlay, "fpga-bridges", 0))
-		np = info->overlay;
-	else
-		np = region_np;
-
-	for (i = 0; ; i++) {
-		br = of_parse_phandle(np, "fpga-bridges", i);
-		if (!br)
-			break;
-
-		/* If parent bridge is in list, skip it. */
-		if (br == parent_br)
-			continue;
-
-		/* If node is a bridge, get it and add to list */
-		ret = of_fpga_bridge_get_to_list(br, region->info,
-						 &region->bridge_list);
-
-		/* If any of the bridges are in use, give up */
-		if (ret == -EBUSY) {
-			fpga_bridges_put(&region->bridge_list);
-			return -EBUSY;
-		}
-	}
-
-	return 0;
-}
-
-/**
  * fpga_region_program_fpga - program FPGA
  * @region: FPGA region
  * @info: FPGA image info
@@ -291,256 +170,6 @@ int fpga_region_program_fpga(struct fpga_region *region,
 }
 EXPORT_SYMBOL_GPL(fpga_region_program_fpga);
 
-/**
- * child_regions_with_firmware
- * @overlay: device node of the overlay
- *
- * If the overlay adds child FPGA regions, they are not allowed to have
- * firmware-name property.
- *
- * Return 0 for OK or -EINVAL if child FPGA region adds firmware-name.
- */
-static int child_regions_with_firmware(struct device_node *overlay)
-{
-	struct device_node *child_region;
-	const char *child_firmware_name;
-	int ret = 0;
-
-	of_node_get(overlay);
-
-	child_region = of_find_matching_node(overlay, fpga_region_of_match);
-	while (child_region) {
-		if (!of_property_read_string(child_region, "firmware-name",
-					     &child_firmware_name)) {
-			ret = -EINVAL;
-			break;
-		}
-		child_region = of_find_matching_node(child_region,
-						     fpga_region_of_match);
-	}
-
-	of_node_put(child_region);
-
-	if (ret)
-		pr_err("firmware-name not allowed in child FPGA region: %s",
-		       child_region->full_name);
-
-	return ret;
-}
-
-/**
- * of_fpga_region_parse_ov - parse and check overlay applied to region
- *
- * @region: FPGA region
- * @overlay: overlay applied to the FPGA region
- *
- * Given an overlay applied to a FPGA region, parse the FPGA image specific
- * info in the overlay and do some checking.
- *
- * Returns:
- *   NULL if overlay doesn't direct us to program the FPGA.
- *   fpga_image_info struct if there is an image to program.
- *   error code for invalid overlay.
- */
-static struct fpga_image_info *of_fpga_region_parse_ov(
-						struct fpga_region *region,
-						struct device_node *overlay)
-{
-	struct device *dev = &region->dev;
-	struct fpga_image_info *info;
-	const char *firmware_name;
-	int ret;
-
-	if (region->info) {
-		dev_err(dev, "Region already has overlay applied.\n");
-		return ERR_PTR(-EINVAL);
-	}
-
-	/*
-	 * Reject overlay if child FPGA Regions added in the overlay have
-	 * firmware-name property (would mean that an FPGA region that has
-	 * not been added to the live tree yet is doing FPGA programming).
-	 */
-	ret = child_regions_with_firmware(overlay);
-	if (ret)
-		return ERR_PTR(ret);
-
-	info = fpga_image_info_alloc(dev);
-	if (!info)
-		return ERR_PTR(-ENOMEM);
-
-	info->overlay = overlay;
-
-	/* Read FPGA region properties from the overlay */
-	if (of_property_read_bool(overlay, "partial-fpga-config"))
-		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
-
-	if (of_property_read_bool(overlay, "external-fpga-config"))
-		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
-
-	if (of_property_read_bool(overlay, "encrypted-fpga-config"))
-		info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
-
-	if (!of_property_read_string(overlay, "firmware-name",
-				     &firmware_name)) {
-		info->firmware_name = devm_kstrdup(dev, firmware_name,
-						   GFP_KERNEL);
-		if (!info->firmware_name)
-			return ERR_PTR(-ENOMEM);
-	}
-
-	of_property_read_u32(overlay, "region-unfreeze-timeout-us",
-			     &info->enable_timeout_us);
-
-	of_property_read_u32(overlay, "region-freeze-timeout-us",
-			     &info->disable_timeout_us);
-
-	of_property_read_u32(overlay, "config-complete-timeout-us",
-			     &info->config_complete_timeout_us);
-
-	/* If overlay is not programming the FPGA, don't need FPGA image info */
-	if (!info->firmware_name) {
-		ret = 0;
-		goto ret_no_info;
-	}
-
-	/*
-	 * If overlay informs us FPGA was externally programmed, specifying
-	 * firmware here would be ambiguous.
-	 */
-	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
-		dev_err(dev, "error: specified firmware and external-fpga-config");
-		ret = -EINVAL;
-		goto ret_no_info;
-	}
-
-	return info;
-ret_no_info:
-	fpga_image_info_free(dev, info);
-	return ERR_PTR(ret);
-}
-
-/**
- * of_fpga_region_notify_pre_apply - pre-apply overlay notification
- *
- * @region: FPGA region that the overlay was 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 checks fail, overlay is rejected and does not get added to the
- * live tree.
- *
- * Returns 0 for success or negative error code for failure.
- */
-static int of_fpga_region_notify_pre_apply(struct fpga_region *region,
-					   struct of_overlay_notify_data *nd)
-{
-	struct device *dev = &region->dev;
-	struct fpga_image_info *info;
-	int ret;
-
-	if (region->info) {
-		dev_err(dev, "Region already has overlay applied.\n");
-		return -EINVAL;
-	}
-
-	info = of_fpga_region_parse_ov(region, nd->overlay);
-	if (IS_ERR(info))
-		return PTR_ERR(info);
-
-	if (info) {
-		ret = fpga_region_program_fpga(region, info);
-		if (ret) {
-			fpga_image_info_free(dev, info);
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
-/**
- * of_fpga_region_notify_post_remove - post-remove overlay notification
- *
- * @region: FPGA region that was targeted by the overlay that was removed
- * @nd: overlay notification data
- *
- * Called after an overlay has been removed if the overlay's target was a
- * FPGA region.
- */
-static void of_fpga_region_notify_post_remove(struct fpga_region *region,
-					      struct of_overlay_notify_data *nd)
-{
-	fpga_bridges_disable(&region->bridge_list);
-	fpga_bridges_put(&region->bridge_list);
-	fpga_image_info_free(&region->dev, region->info);
-	region->info = NULL;
-}
-
-/**
- * of_fpga_region_notify - reconfig notifier for dynamic DT changes
- * @nb:		notifier block
- * @action:	notifier action
- * @arg:	reconfig data
- *
- * This notifier handles programming a FPGA when a "firmware-name" property is
- * added to a fpga-region.
- *
- * Returns NOTIFY_OK or error if FPGA programming fails.
- */
-static int of_fpga_region_notify(struct notifier_block *nb,
-				 unsigned long action, void *arg)
-{
-	struct of_overlay_notify_data *nd = arg;
-	struct fpga_region *region;
-	int ret;
-
-	switch (action) {
-	case OF_OVERLAY_PRE_APPLY:
-		pr_debug("%s OF_OVERLAY_PRE_APPLY\n", __func__);
-		break;
-	case OF_OVERLAY_POST_APPLY:
-		pr_debug("%s OF_OVERLAY_POST_APPLY\n", __func__);
-		return NOTIFY_OK;       /* not for us */
-	case OF_OVERLAY_PRE_REMOVE:
-		pr_debug("%s OF_OVERLAY_PRE_REMOVE\n", __func__);
-		return NOTIFY_OK;       /* not for us */
-	case OF_OVERLAY_POST_REMOVE:
-		pr_debug("%s OF_OVERLAY_POST_REMOVE\n", __func__);
-		break;
-	default:			/* should not happen */
-		return NOTIFY_OK;
-	}
-
-	region = of_fpga_region_find(nd->target);
-	if (!region)
-		return NOTIFY_OK;
-
-	ret = 0;
-	switch (action) {
-	case OF_OVERLAY_PRE_APPLY:
-		ret = of_fpga_region_notify_pre_apply(region, nd);
-		break;
-
-	case OF_OVERLAY_POST_REMOVE:
-		of_fpga_region_notify_post_remove(region, nd);
-		break;
-	}
-
-	put_device(&region->dev);
-
-	if (ret)
-		return notifier_from_errno(ret);
-
-	return NOTIFY_OK;
-}
-
-static struct notifier_block fpga_region_of_nb = {
-	.notifier_call = of_fpga_region_notify,
-};
-
 int fpga_region_register(struct device *dev, struct fpga_region *region)
 {
 	int id, ret = 0;
@@ -582,63 +211,6 @@ int fpga_region_unregister(struct fpga_region *region)
 }
 EXPORT_SYMBOL_GPL(fpga_region_unregister);
 
-static int of_fpga_region_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
-	struct fpga_region *region;
-	struct fpga_manager *mgr;
-	int ret;
-
-	/* Find the FPGA mgr specified by region or parent region. */
-	mgr = of_fpga_region_get_mgr(np);
-	if (IS_ERR(mgr))
-		return -EPROBE_DEFER;
-
-	region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL);
-	if (!region) {
-		ret = -ENOMEM;
-		goto eprobe_mgr_put;
-	}
-
-	region->mgr = mgr;
-
-	/* Specify how to get bridges for this type of region. */
-	region->get_bridges = of_fpga_region_get_bridges;
-
-	ret = fpga_region_register(dev, region);
-	if (ret)
-		goto eprobe_mgr_put;
-
-	of_platform_populate(np, fpga_region_of_match, NULL, &region->dev);
-
-	dev_info(dev, "FPGA Region probed\n");
-
-	return 0;
-
-eprobe_mgr_put:
-	fpga_mgr_put(mgr);
-	return ret;
-}
-
-static int of_fpga_region_remove(struct platform_device *pdev)
-{
-	struct fpga_region *region = platform_get_drvdata(pdev);
-
-	fpga_region_unregister(region);
-
-	return 0;
-}
-
-static struct platform_driver of_fpga_region_driver = {
-	.probe = of_fpga_region_probe,
-	.remove = of_fpga_region_remove,
-	.driver = {
-		.name	= "fpga-region",
-		.of_match_table = of_match_ptr(fpga_region_of_match),
-	},
-};
-
 static void fpga_region_dev_release(struct device *dev)
 {
 	struct fpga_region *region = to_fpga_region(dev);
@@ -652,36 +224,17 @@ static void fpga_region_dev_release(struct device *dev)
  */
 static int __init fpga_region_init(void)
 {
-	int ret;
-
 	fpga_region_class = class_create(THIS_MODULE, "fpga_region");
 	if (IS_ERR(fpga_region_class))
 		return PTR_ERR(fpga_region_class);
 
 	fpga_region_class->dev_release = fpga_region_dev_release;
 
-	ret = of_overlay_notifier_register(&fpga_region_of_nb);
-	if (ret)
-		goto err_class;
-
-	ret = platform_driver_register(&of_fpga_region_driver);
-	if (ret)
-		goto err_plat;
-
 	return 0;
-
-err_plat:
-	of_overlay_notifier_unregister(&fpga_region_of_nb);
-err_class:
-	class_destroy(fpga_region_class);
-	ida_destroy(&fpga_region_ida);
-	return ret;
 }
 
 static void __exit fpga_region_exit(void)
 {
-	platform_driver_unregister(&of_fpga_region_driver);
-	of_overlay_notifier_unregister(&fpga_region_of_nb);
 	class_destroy(fpga_region_class);
 	ida_destroy(&fpga_region_ida);
 }
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
new file mode 100644
index 0000000..080f20b
--- /dev/null
+++ b/drivers/fpga/of-fpga-region.c
@@ -0,0 +1,493 @@
+/*
+ * FPGA Region - Device Tree support for FPGA programming under Linux
+ *
+ *  Copyright (C) 2013-2016 Altera Corporation
+ *  Copyright (C) 2017 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/fpga/fpga-bridge.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/fpga/fpga-region.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+static const struct of_device_id fpga_region_of_match[] = {
+	{ .compatible = "fpga-region", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, fpga_region_of_match);
+
+static int fpga_region_of_node_match(struct device *dev, const void *data)
+{
+	return dev->of_node == data;
+}
+
+/**
+ * of_fpga_region_find - find FPGA region
+ * @np: device node of FPGA Region
+ *
+ * Caller will need to put_device(&region->dev) when done.
+ *
+ * Returns FPGA Region struct or NULL
+ */
+static struct fpga_region *of_fpga_region_find(struct device_node *np)
+{
+	return fpga_region_class_find(NULL, np, fpga_region_of_node_match);
+}
+
+/**
+ * of_fpga_region_get_mgr - get reference for FPGA manager
+ * @np: device node of FPGA region
+ *
+ * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
+ *
+ * Caller should call fpga_mgr_put() when done with manager.
+ *
+ * Return: fpga manager struct or IS_ERR() condition containing error code.
+ */
+static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)
+{
+	struct device_node  *mgr_node;
+	struct fpga_manager *mgr;
+
+	of_node_get(np);
+	while (np) {
+		if (of_device_is_compatible(np, "fpga-region")) {
+			mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
+			if (mgr_node) {
+				mgr = of_fpga_mgr_get(mgr_node);
+				of_node_put(np);
+				return mgr;
+			}
+		}
+		np = of_get_next_parent(np);
+	}
+	of_node_put(np);
+
+	return ERR_PTR(-EINVAL);
+}
+
+/**
+ * of_fpga_region_get_bridges - create a list of bridges
+ * @region: FPGA region
+ * @info: FPGA image info
+ *
+ * Create a list of bridges including the parent bridge and the bridges
+ * specified by "fpga-bridges" property.  Note that the
+ * fpga_bridges_enable/disable/put functions are all fine with an empty list
+ * if that happens.
+ *
+ * Caller should call fpga_bridges_put(&region->bridge_list) when
+ * done with the bridges.
+ *
+ * Return 0 for success (even if there are no bridges specified)
+ * or -EBUSY if any of the bridges are in use.
+ */
+static int of_fpga_region_get_bridges(struct fpga_region *region,
+				      struct fpga_image_info *info)
+{
+	struct device *dev = &region->dev;
+	struct device_node *region_np = dev->of_node;
+	struct device_node *br, *np, *parent_br = NULL;
+	int i, ret;
+
+	/* If parent is a bridge, add to list */
+	ret = of_fpga_bridge_get_to_list(region_np->parent, info,
+					 &region->bridge_list);
+
+	/* -EBUSY means parent is a bridge that is under use. Give up. */
+	if (ret == -EBUSY)
+		return ret;
+
+	/* Zero return code means parent was a bridge and was added to list. */
+	if (!ret)
+		parent_br = region_np->parent;
+
+	/* If overlay has a list of bridges, use it. */
+	if (of_parse_phandle(info->overlay, "fpga-bridges", 0))
+		np = info->overlay;
+	else
+		np = region_np;
+
+	for (i = 0; ; i++) {
+		br = of_parse_phandle(np, "fpga-bridges", i);
+		if (!br)
+			break;
+
+		/* If parent bridge is in list, skip it. */
+		if (br == parent_br)
+			continue;
+
+		/* If node is a bridge, get it and add to list */
+		ret = of_fpga_bridge_get_to_list(br, region->info,
+						 &region->bridge_list);
+
+		/* If any of the bridges are in use, give up */
+		if (ret == -EBUSY) {
+			fpga_bridges_put(&region->bridge_list);
+			return -EBUSY;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * child_regions_with_firmware
+ * @overlay: device node of the overlay
+ *
+ * If the overlay adds child FPGA regions, they are not allowed to have
+ * firmware-name property.
+ *
+ * Return 0 for OK or -EINVAL if child FPGA region adds firmware-name.
+ */
+static int child_regions_with_firmware(struct device_node *overlay)
+{
+	struct device_node *child_region;
+	const char *child_firmware_name;
+	int ret = 0;
+
+	of_node_get(overlay);
+
+	child_region = of_find_matching_node(overlay, fpga_region_of_match);
+	while (child_region) {
+		if (!of_property_read_string(child_region, "firmware-name",
+					     &child_firmware_name)) {
+			ret = -EINVAL;
+			break;
+		}
+		child_region = of_find_matching_node(child_region,
+						     fpga_region_of_match);
+	}
+
+	of_node_put(child_region);
+
+	if (ret)
+		pr_err("firmware-name not allowed in child FPGA region: %s",
+		       child_region->full_name);
+
+	return ret;
+}
+
+/**
+ * of_fpga_region_parse_ov - parse and check overlay applied to region
+ *
+ * @region: FPGA region
+ * @overlay: overlay applied to the FPGA region
+ *
+ * Given an overlay applied to a FPGA region, parse the FPGA image specific
+ * info in the overlay and do some checking.
+ *
+ * Returns:
+ *   NULL if overlay doesn't direct us to program the FPGA.
+ *   fpga_image_info struct if there is an image to program.
+ *   error code for invalid overlay.
+ */
+static struct fpga_image_info *of_fpga_region_parse_ov(
+						struct fpga_region *region,
+						struct device_node *overlay)
+{
+	struct device *dev = &region->dev;
+	struct fpga_image_info *info;
+	const char *firmware_name;
+	int ret;
+
+	if (region->info) {
+		dev_err(dev, "Region already has overlay applied.\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	/*
+	 * Reject overlay if child FPGA Regions added in the overlay have
+	 * firmware-name property (would mean that an FPGA region that has
+	 * not been added to the live tree yet is doing FPGA programming).
+	 */
+	ret = child_regions_with_firmware(overlay);
+	if (ret)
+		return ERR_PTR(ret);
+
+	info = fpga_image_info_alloc(dev);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	info->overlay = overlay;
+
+	/* Read FPGA region properties from the overlay */
+	if (of_property_read_bool(overlay, "partial-fpga-config"))
+		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
+
+	if (of_property_read_bool(overlay, "external-fpga-config"))
+		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
+
+	if (of_property_read_bool(overlay, "encrypted-fpga-config"))
+		info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
+
+	if (!of_property_read_string(overlay, "firmware-name",
+				     &firmware_name)) {
+		info->firmware_name = devm_kstrdup(dev, firmware_name,
+						   GFP_KERNEL);
+		if (!info->firmware_name)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	of_property_read_u32(overlay, "region-unfreeze-timeout-us",
+			     &info->enable_timeout_us);
+
+	of_property_read_u32(overlay, "region-freeze-timeout-us",
+			     &info->disable_timeout_us);
+
+	of_property_read_u32(overlay, "config-complete-timeout-us",
+			     &info->config_complete_timeout_us);
+
+	/* If overlay is not programming the FPGA, don't need FPGA image info */
+	if (!info->firmware_name) {
+		ret = 0;
+		goto ret_no_info;
+	}
+
+	/*
+	 * If overlay informs us FPGA was externally programmed, specifying
+	 * firmware here would be ambiguous.
+	 */
+	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
+		dev_err(dev, "error: specified firmware and external-fpga-config");
+		ret = -EINVAL;
+		goto ret_no_info;
+	}
+
+	return info;
+ret_no_info:
+	fpga_image_info_free(dev, info);
+	return ERR_PTR(ret);
+}
+
+/**
+ * of_fpga_region_notify_pre_apply - pre-apply overlay notification
+ *
+ * @region: FPGA region that the overlay was 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 checks fail, overlay is rejected and does not get added to the
+ * live tree.
+ *
+ * Returns 0 for success or negative error code for failure.
+ */
+static int of_fpga_region_notify_pre_apply(struct fpga_region *region,
+					   struct of_overlay_notify_data *nd)
+{
+	struct device *dev = &region->dev;
+	struct fpga_image_info *info;
+	int ret;
+
+	if (region->info) {
+		dev_err(dev, "Region already has overlay applied.\n");
+		return -EINVAL;
+	}
+
+	info = of_fpga_region_parse_ov(region, nd->overlay);
+	if (IS_ERR(info))
+		return PTR_ERR(info);
+
+	if (info) {
+		ret = fpga_region_program_fpga(region, info);
+		if (ret) {
+			fpga_image_info_free(dev, info);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * of_fpga_region_notify_post_remove - post-remove overlay notification
+ *
+ * @region: FPGA region that was targeted by the overlay that was removed
+ * @nd: overlay notification data
+ *
+ * Called after an overlay has been removed if the overlay's target was a
+ * FPGA region.
+ */
+static void of_fpga_region_notify_post_remove(struct fpga_region *region,
+					      struct of_overlay_notify_data *nd)
+{
+	fpga_bridges_disable(&region->bridge_list);
+	fpga_bridges_put(&region->bridge_list);
+	fpga_image_info_free(&region->dev, region->info);
+	region->info = NULL;
+}
+
+/**
+ * of_fpga_region_notify - reconfig notifier for dynamic DT changes
+ * @nb:		notifier block
+ * @action:	notifier action
+ * @arg:	reconfig data
+ *
+ * This notifier handles programming a FPGA when a "firmware-name" property is
+ * added to a fpga-region.
+ *
+ * Returns NOTIFY_OK or error if FPGA programming fails.
+ */
+static int of_fpga_region_notify(struct notifier_block *nb,
+				 unsigned long action, void *arg)
+{
+	struct of_overlay_notify_data *nd = arg;
+	struct fpga_region *region;
+	int ret;
+
+	switch (action) {
+	case OF_OVERLAY_PRE_APPLY:
+		pr_debug("%s OF_OVERLAY_PRE_APPLY\n", __func__);
+		break;
+	case OF_OVERLAY_POST_APPLY:
+		pr_debug("%s OF_OVERLAY_POST_APPLY\n", __func__);
+		return NOTIFY_OK;       /* not for us */
+	case OF_OVERLAY_PRE_REMOVE:
+		pr_debug("%s OF_OVERLAY_PRE_REMOVE\n", __func__);
+		return NOTIFY_OK;       /* not for us */
+	case OF_OVERLAY_POST_REMOVE:
+		pr_debug("%s OF_OVERLAY_POST_REMOVE\n", __func__);
+		break;
+	default:			/* should not happen */
+		return NOTIFY_OK;
+	}
+
+	region = of_fpga_region_find(nd->target);
+	if (!region)
+		return NOTIFY_OK;
+
+	ret = 0;
+	switch (action) {
+	case OF_OVERLAY_PRE_APPLY:
+		ret = of_fpga_region_notify_pre_apply(region, nd);
+		break;
+
+	case OF_OVERLAY_POST_REMOVE:
+		of_fpga_region_notify_post_remove(region, nd);
+		break;
+	}
+
+	put_device(&region->dev);
+
+	if (ret)
+		return notifier_from_errno(ret);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block fpga_region_of_nb = {
+	.notifier_call = of_fpga_region_notify,
+};
+
+static int of_fpga_region_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct fpga_region *region;
+	struct fpga_manager *mgr;
+	int ret;
+
+	/* Find the FPGA mgr specified by region or parent region. */
+	mgr = of_fpga_region_get_mgr(np);
+	if (IS_ERR(mgr))
+		return -EPROBE_DEFER;
+
+	region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL);
+	if (!region) {
+		ret = -ENOMEM;
+		goto eprobe_mgr_put;
+	}
+
+	region->mgr = mgr;
+
+	/* Specify how to get bridges for this type of region. */
+	region->get_bridges = of_fpga_region_get_bridges;
+
+	ret = fpga_region_register(dev, region);
+	if (ret)
+		goto eprobe_mgr_put;
+
+	of_platform_populate(np, fpga_region_of_match, NULL, &region->dev);
+
+	dev_info(dev, "FPGA Region probed\n");
+
+	return 0;
+
+eprobe_mgr_put:
+	fpga_mgr_put(mgr);
+	return ret;
+}
+
+static int of_fpga_region_remove(struct platform_device *pdev)
+{
+	struct fpga_region *region = platform_get_drvdata(pdev);
+
+	fpga_region_unregister(region);
+
+	return 0;
+}
+
+static struct platform_driver of_fpga_region_driver = {
+	.probe = of_fpga_region_probe,
+	.remove = of_fpga_region_remove,
+	.driver = {
+		.name	= "of-fpga-region",
+		.of_match_table = of_match_ptr(fpga_region_of_match),
+	},
+};
+
+/**
+ * fpga_region_init - init function for fpga_region class
+ * Creates the fpga_region class and registers a reconfig notifier.
+ */
+static int __init of_fpga_region_init(void)
+{
+	int ret;
+
+	ret = of_overlay_notifier_register(&fpga_region_of_nb);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&of_fpga_region_driver);
+	if (ret)
+		goto err_plat;
+
+	return 0;
+
+err_plat:
+	of_overlay_notifier_unregister(&fpga_region_of_nb);
+	return ret;
+}
+
+static void __exit of_fpga_region_exit(void)
+{
+	platform_driver_unregister(&of_fpga_region_driver);
+	of_overlay_notifier_unregister(&fpga_region_of_nb);
+}
+
+subsys_initcall(of_fpga_region_init);
+module_exit(of_fpga_region_exit);
+
+MODULE_DESCRIPTION("FPGA Region");
+MODULE_AUTHOR("Alan Tull <atull@kernel.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

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

* Re: [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree
  2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
                   ` (15 preceding siblings ...)
  2017-04-20 14:10 ` [PATCH v2 16/16] fpga: region: move device tree support to of-fpga-region.c Alan Tull
@ 2017-04-20 14:16 ` Alan Tull
  16 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-20 14:16 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

On Thu, Apr 20, 2017 at 9:09 AM, Alan Tull <atull@kernel.org> wrote:
> The current FPGA layer has two problems which this
> patchset addresses:
>
> * We now have 3 functions for programming a FPGA, depending
> on whether the image is in a sg list, a buffer, or firmware.
> So callers have to decide where the image will be or have
> to write extra code to maintain flexibilty.
>
> * users who aren't using device tree are left to write their
> own code that is essentially a rewrite of FPGA region.
>
> The major things this patchset accomplishes is:

I've pushed a branch to linux-fpga - review-next-20170420-atull-v2

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

* Re: [PATCH v2 03/16] fpga: mgr: API change to replace fpga load functions with single function
  2017-04-20 14:09 ` [PATCH v2 03/16] fpga: mgr: API change to replace fpga load functions with single function Alan Tull
@ 2017-04-21 20:08   ` Alan Tull
  0 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-21 20:08 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

On Thu, Apr 20, 2017 at 9:09 AM, Alan Tull <atull@kernel.org> wrote:

> @@ -382,7 +377,13 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
>         if (of_property_read_bool(nd->overlay, "encrypted-fpga-config"))
>                 info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
>
> -       of_property_read_string(nd->overlay, "firmware-name", &firmware_name);
> +       if (!of_property_read_string(nd->overlay, "firmware-name",
> +                                    &firmware_name)) {
> +               info->firmware_name = devm_kstrdup(dev, firmware_name,
> +                                                  GFP_KERNEL);

For those who weren't copied, the kbuild robot has kindly pointed out
that dev is
undeclared in this function.  It's fixed two patches later, but I'll
need to fix it for
bisectibility in v3.  I actually did bisectibility testing but got
tired and lazy near the
end.  I built & functionality tested individually starting at HEAD,
working back to
about HEAD~11, and then started :/ skipping patches.  Ugh.

Alan

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

* Re: [PATCH v2 16/16] fpga: region: move device tree support to of-fpga-region.c
  2017-04-20 14:10 ` [PATCH v2 16/16] fpga: region: move device tree support to of-fpga-region.c Alan Tull
@ 2017-04-28  6:38   ` Wu Hao
  2017-04-28 17:37     ` Alan Tull
  0 siblings, 1 reply; 39+ messages in thread
From: Wu Hao @ 2017-04-28  6:38 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, linux-kernel, linux-fpga

On Thu, Apr 20, 2017 at 09:10:01AM -0500, Alan Tull wrote:
> Create of-fpga-region.c
> 
> Move the following functions without modification from
> fpga-region.c to of-fpga-region.c:
> 
> * of_fpga_region_find
> * of_fpga_region_get_mgr
> * of_fpga_region_get_bridges
> * child_regions_with_firmware
> * of_fpga_region_parse_ov
> * of_fpga_region_notify_pre_apply
> * of_fpga_region_notify_post_remove
> * of_fpga_region_notify
> * of_fpga_region_probe
> * of_fpga_region_remove
> 
> Create two new function with some code from fpga_region_init/exit.
> 
> * of_fpga_region_init
> * of_fpga_region_exit
> 
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
> v2: split out code changes into other patches, only move code here
> ---
>  drivers/fpga/Kconfig          |  13 +-
>  drivers/fpga/Makefile         |   1 +
>  drivers/fpga/fpga-region.c    | 449 +-------------------------------------
>  drivers/fpga/of-fpga-region.c | 493 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 505 insertions(+), 451 deletions(-)
>  create mode 100644 drivers/fpga/of-fpga-region.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 161ba9d..394c141 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -15,10 +15,17 @@ if FPGA
>  
>  config FPGA_REGION
>  	tristate "FPGA Region"
> -	depends on OF && FPGA_BRIDGE
> +	depends on FPGA_BRIDGE
> +	help
> +	  FPGA Region common code.  A FPGA Region controls a FPGA Manager
> +	  and the FPGA Bridges associated with either a reconfigurable
> +	  region of an FPGA or a whole FPGA.
> +

Hi Alan

As FPGA_BRIDGE depends on OF, so FPGA_REGION still can't be selected
without OF. Should we remove the OF dependency for FPGA_BRIDGE as well?

Thanks
Hao

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

* Re: [PATCH v2 16/16] fpga: region: move device tree support to of-fpga-region.c
  2017-04-28  6:38   ` Wu Hao
@ 2017-04-28 17:37     ` Alan Tull
  0 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-04-28 17:37 UTC (permalink / raw)
  To: Wu Hao; +Cc: Moritz Fischer, linux-kernel, linux-fpga

On Fri, Apr 28, 2017 at 1:38 AM, Wu Hao <hao.wu@intel.com> wrote:
> On Thu, Apr 20, 2017 at 09:10:01AM -0500, Alan Tull wrote:
>> Create of-fpga-region.c
>>
>> Move the following functions without modification from
>> fpga-region.c to of-fpga-region.c:
>>
>> * of_fpga_region_find
>> * of_fpga_region_get_mgr
>> * of_fpga_region_get_bridges
>> * child_regions_with_firmware
>> * of_fpga_region_parse_ov
>> * of_fpga_region_notify_pre_apply
>> * of_fpga_region_notify_post_remove
>> * of_fpga_region_notify
>> * of_fpga_region_probe
>> * of_fpga_region_remove
>>
>> Create two new function with some code from fpga_region_init/exit.
>>
>> * of_fpga_region_init
>> * of_fpga_region_exit
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
>> ---
>> v2: split out code changes into other patches, only move code here
>> ---
>>  drivers/fpga/Kconfig          |  13 +-
>>  drivers/fpga/Makefile         |   1 +
>>  drivers/fpga/fpga-region.c    | 449 +-------------------------------------
>>  drivers/fpga/of-fpga-region.c | 493 ++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 505 insertions(+), 451 deletions(-)
>>  create mode 100644 drivers/fpga/of-fpga-region.c
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 161ba9d..394c141 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -15,10 +15,17 @@ if FPGA
>>
>>  config FPGA_REGION
>>       tristate "FPGA Region"
>> -     depends on OF && FPGA_BRIDGE
>> +     depends on FPGA_BRIDGE
>> +     help
>> +       FPGA Region common code.  A FPGA Region controls a FPGA Manager
>> +       and the FPGA Bridges associated with either a reconfigurable
>> +       region of an FPGA or a whole FPGA.
>> +
>
> Hi Alan
>
> As FPGA_BRIDGE depends on OF, so FPGA_REGION still can't be selected
> without OF. Should we remove the OF dependency for FPGA_BRIDGE as well?

Yes, I will do that in v3.

Thanks,
Alan

>
> Thanks
> Hao

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-04-20 14:09 ` [PATCH v2 02/16] fpga: bridge: support getting bridge from device Alan Tull
@ 2017-05-03 11:58   ` Wu Hao
  2017-05-03 20:07     ` Alan Tull
  0 siblings, 1 reply; 39+ messages in thread
From: Wu Hao @ 2017-05-03 11:58 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, linux-kernel, linux-fpga

On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
> Add two functions for getting the FPGA bridge from the device
> rather than device tree node.  This is to enable writing code
> that will support using FPGA bridges without device tree.
> Rename one old function to make it clear that it is device
> tree-ish.  This leaves us with 3 functions for getting a bridge:
> 
> * fpga_bridge_get
>   Get the bridge given the device.
> 
> * fpga_bridges_get_to_list
>   Given the device, get the bridge and add it to a list.
> 
> * of_fpga_bridges_get_to_list
>   Renamed from priviously existing fpga_bridges_get_to_list.
>   Given the device node, get the bridge and add it to a list.
> 

Hi Alan

Thanks a lot for providing this patch set for non device tree support. :)
Actually I am reworking the Intel FPGA device drivers based on this patch
set, and I find some problems with the existing APIs including fpga bridge
and manager. My idea is to create all fpga bridges/regions/manager under
the same platform device (FME), it allows FME driver to establish the
relationship for the bridges/regions/managers it creates in an easy way.
But I found current fpga class API doesn't support this very well.
e.g fpga_bridge_get/get_to_list only accept parent device as the input
parameter, but it doesn't work if we have multiple bridges (and
regions/manager) under the same platform device. fpga_mgr has similar
issue, but fpga_region APIs work better, as they accept fpga_region as
parameter not the shared parent device.

Do you think if having multiple fpga-* under one parent device is in the
right direction? If yes, shall we provide some more APIs which accept
fpga_bridge (and same for fpga-mgr) as parameter instead of the parent
device just like fpga-region?

Thanks
Hao

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

* Re: [PATCH v2 15/16] fpga: region: add fpga_region_class_find
  2017-04-20 14:10 ` [PATCH v2 15/16] fpga: region: add fpga_region_class_find Alan Tull
@ 2017-05-03 15:44   ` Moritz Fischer
  2017-05-03 20:08     ` Alan Tull
  0 siblings, 1 reply; 39+ messages in thread
From: Moritz Fischer @ 2017-05-03 15:44 UTC (permalink / raw)
  To: Alan Tull; +Cc: Linux Kernel Mailing List, linux-fpga

On Thu, Apr 20, 2017 at 7:10 AM, Alan Tull <atull@kernel.org> wrote:
> Add a function for searching the fpga-region class.  This
> will be useful when device tree code is no longer in the
> same file that declares the fpga-region class.  Another
> step in separating common FPGA region code from device
> tree support.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
Acked-by: Moritz Fischer <mdf@kernel.org>
> ---
> v2: split out from another patch
> ---
>  drivers/fpga/fpga-region.c       | 23 +++++++++++++++--------
>  include/linux/fpga/fpga-region.h |  4 ++++
>  2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index e2a3fe6..7ffb8c1 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -30,6 +30,20 @@
>  static DEFINE_IDA(fpga_region_ida);
>  static struct class *fpga_region_class;
>
> +struct fpga_region *fpga_region_class_find(
> +       struct device *start, const void *data,
> +       int (*match)(struct device *, const void *))
> +{
> +       struct device *dev;
> +
> +       dev = class_find_device(fpga_region_class, start, data, match);
> +       if (!dev)
> +               return NULL;
> +
> +       return to_fpga_region(dev);
> +}
> +EXPORT_SYMBOL_GPL(fpga_region_class_find);
> +
>  static const struct of_device_id fpga_region_of_match[] = {
>         { .compatible = "fpga-region", },
>         {},
> @@ -51,14 +65,7 @@ static int fpga_region_of_node_match(struct device *dev, const void *data)
>   */
>  static struct fpga_region *of_fpga_region_find(struct device_node *np)
>  {
> -       struct device *dev;
> -
> -       dev = class_find_device(fpga_region_class, NULL, np,
> -                               fpga_region_of_node_match);
> -       if (!dev)
> -               return NULL;
> -
> -       return to_fpga_region(dev);
> +       return fpga_region_class_find(NULL, np, fpga_region_of_node_match);
>  }
>
>  /**
> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> index 35e7e09..4966bb2 100644
> --- a/include/linux/fpga/fpga-region.h
> +++ b/include/linux/fpga/fpga-region.h
> @@ -28,6 +28,10 @@ struct fpga_region {
>
>  #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
>
> +struct fpga_region *fpga_region_class_find(
> +       struct device *start, const void *data,
> +       int (*match)(struct device *, const void *));
> +
>  int fpga_region_program_fpga(struct fpga_region *region,
>                              struct fpga_image_info *image_info);
>
> --
> 2.7.4
>

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-03 11:58   ` Wu Hao
@ 2017-05-03 20:07     ` Alan Tull
  2017-05-04  9:20       ` Wu Hao
  2017-05-04 21:31       ` Alan Tull
  0 siblings, 2 replies; 39+ messages in thread
From: Alan Tull @ 2017-05-03 20:07 UTC (permalink / raw)
  To: Wu Hao; +Cc: Moritz Fischer, linux-kernel, linux-fpga, matthew.gerlach

On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
>> Add two functions for getting the FPGA bridge from the device
>> rather than device tree node.  This is to enable writing code
>> that will support using FPGA bridges without device tree.
>> Rename one old function to make it clear that it is device
>> tree-ish.  This leaves us with 3 functions for getting a bridge:
>>
>> * fpga_bridge_get
>>   Get the bridge given the device.
>>
>> * fpga_bridges_get_to_list
>>   Given the device, get the bridge and add it to a list.
>>
>> * of_fpga_bridges_get_to_list
>>   Renamed from priviously existing fpga_bridges_get_to_list.
>>   Given the device node, get the bridge and add it to a list.
>>
>
> Hi Alan
>
> Thanks a lot for providing this patch set for non device tree support. :)
> Actually I am reworking the Intel FPGA device drivers based on this patch
> set, and I find some problems with the existing APIs including fpga bridge
> and manager. My idea is to create all fpga bridges/regions/manager under
> the same platform device (FME), it allows FME driver to establish the
> relationship for the bridges/regions/managers it creates in an easy way.
> But I found current fpga class API doesn't support this very well.
> e.g fpga_bridge_get/get_to_list only accept parent device as the input
> parameter, but it doesn't work if we have multiple bridges (and
> regions/manager) under the same platform device. fpga_mgr has similar
> issue, but fpga_region APIs work better, as they accept fpga_region as
> parameter not the shared parent device.

That's good feedback.  I can post a couple patches that apply on top
of that patchset to add the APIs you need.

Probably what I'll do is add

struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);

And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the following:

struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
                                struct fpga_image_info *info);

int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
                               struct fpga_image_info *info,
                               struct list_head *bridge_list);

Working on it now.

>
> Do you think if having multiple fpga-* under one parent device is in the
> right direction?

That should be fine as long as it's coded with an eye on making things
reusable and seeing beyond the current project.  Just thinking of the
future and of what can be of general usefulness for others.  And there
will be others interested in reusing this.

Alan

> If yes, shall we provide some more APIs which accept
> fpga_bridge (and same for fpga-mgr) as parameter instead of the parent
> device just like fpga-region?
>
> Thanks
> Hao
>

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

* Re: [PATCH v2 15/16] fpga: region: add fpga_region_class_find
  2017-05-03 15:44   ` Moritz Fischer
@ 2017-05-03 20:08     ` Alan Tull
  0 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-05-03 20:08 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Linux Kernel Mailing List, linux-fpga

On Wed, May 3, 2017 at 10:44 AM, Moritz Fischer <mdf@kernel.org> wrote:
> On Thu, Apr 20, 2017 at 7:10 AM, Alan Tull <atull@kernel.org> wrote:
>> Add a function for searching the fpga-region class.  This
>> will be useful when device tree code is no longer in the
>> same file that declares the fpga-region class.  Another
>> step in separating common FPGA region code from device
>> tree support.
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
> Acked-by: Moritz Fischer <mdf@kernel.org>

Hi Moritz,

Thanks!

Alan

>> ---
>> v2: split out from another patch
>> ---
>>  drivers/fpga/fpga-region.c       | 23 +++++++++++++++--------
>>  include/linux/fpga/fpga-region.h |  4 ++++
>>  2 files changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index e2a3fe6..7ffb8c1 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -30,6 +30,20 @@
>>  static DEFINE_IDA(fpga_region_ida);
>>  static struct class *fpga_region_class;
>>
>> +struct fpga_region *fpga_region_class_find(
>> +       struct device *start, const void *data,
>> +       int (*match)(struct device *, const void *))
>> +{
>> +       struct device *dev;
>> +
>> +       dev = class_find_device(fpga_region_class, start, data, match);
>> +       if (!dev)
>> +               return NULL;
>> +
>> +       return to_fpga_region(dev);
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_region_class_find);
>> +
>>  static const struct of_device_id fpga_region_of_match[] = {
>>         { .compatible = "fpga-region", },
>>         {},
>> @@ -51,14 +65,7 @@ static int fpga_region_of_node_match(struct device *dev, const void *data)
>>   */
>>  static struct fpga_region *of_fpga_region_find(struct device_node *np)
>>  {
>> -       struct device *dev;
>> -
>> -       dev = class_find_device(fpga_region_class, NULL, np,
>> -                               fpga_region_of_node_match);
>> -       if (!dev)
>> -               return NULL;
>> -
>> -       return to_fpga_region(dev);
>> +       return fpga_region_class_find(NULL, np, fpga_region_of_node_match);
>>  }
>>
>>  /**
>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
>> index 35e7e09..4966bb2 100644
>> --- a/include/linux/fpga/fpga-region.h
>> +++ b/include/linux/fpga/fpga-region.h
>> @@ -28,6 +28,10 @@ struct fpga_region {
>>
>>  #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
>>
>> +struct fpga_region *fpga_region_class_find(
>> +       struct device *start, const void *data,
>> +       int (*match)(struct device *, const void *));
>> +
>>  int fpga_region_program_fpga(struct fpga_region *region,
>>                              struct fpga_image_info *image_info);
>>
>> --
>> 2.7.4
>>

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-03 20:07     ` Alan Tull
@ 2017-05-04  9:20       ` Wu Hao
  2017-05-04 21:31       ` Alan Tull
  1 sibling, 0 replies; 39+ messages in thread
From: Wu Hao @ 2017-05-04  9:20 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, linux-kernel, linux-fpga, matthew.gerlach

On Wed, May 03, 2017 at 03:07:33PM -0500, Alan Tull wrote:
> On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
> > On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
> >> Add two functions for getting the FPGA bridge from the device
> >> rather than device tree node.  This is to enable writing code
> >> that will support using FPGA bridges without device tree.
> >> Rename one old function to make it clear that it is device
> >> tree-ish.  This leaves us with 3 functions for getting a bridge:
> >>
> >> * fpga_bridge_get
> >>   Get the bridge given the device.
> >>
> >> * fpga_bridges_get_to_list
> >>   Given the device, get the bridge and add it to a list.
> >>
> >> * of_fpga_bridges_get_to_list
> >>   Renamed from priviously existing fpga_bridges_get_to_list.
> >>   Given the device node, get the bridge and add it to a list.
> >>
> >
> > Hi Alan
> >
> > Thanks a lot for providing this patch set for non device tree support. :)
> > Actually I am reworking the Intel FPGA device drivers based on this patch
> > set, and I find some problems with the existing APIs including fpga bridge
> > and manager. My idea is to create all fpga bridges/regions/manager under
> > the same platform device (FME), it allows FME driver to establish the
> > relationship for the bridges/regions/managers it creates in an easy way.
> > But I found current fpga class API doesn't support this very well.
> > e.g fpga_bridge_get/get_to_list only accept parent device as the input
> > parameter, but it doesn't work if we have multiple bridges (and
> > regions/manager) under the same platform device. fpga_mgr has similar
> > issue, but fpga_region APIs work better, as they accept fpga_region as
> > parameter not the shared parent device.
> 
> That's good feedback.  I can post a couple patches that apply on top
> of that patchset to add the APIs you need.
> 
> Probably what I'll do is add
> 
> struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
> 
> And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the following:
> 
> struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>                                 struct fpga_image_info *info);
> 
> int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>                                struct fpga_image_info *info,
>                                struct list_head *bridge_list);
> 
> Working on it now.

Hi Alan

Thanks a lot! This sounds very good to me and I assume fpga_bridge_get_to_list
will accept struct fpga_bridge * as input parameter too. :)

> 
> >
> > Do you think if having multiple fpga-* under one parent device is in the
> > right direction?
> 
> That should be fine as long as it's coded with an eye on making things
> reusable and seeing beyond the current project.  Just thinking of the
> future and of what can be of general usefulness for others.  And there
> will be others interested in reusing this.
> 

Glad to hear that you agree with this. :)

I list some other APIs which have the similar issue, but may not related
to this patch directly.

void fpga_bridge_unregister(struct device *dev)
void fpga_mgr_unregister(struct device *dev)

They only accept the parent device, should we use struct fpga_bridge *and
struct fpga_manager * instead of the parent device in above 2 functions
too?

int fpga_bridge_register(struct device *dev, const char *name,
                         const struct fpga_bridge_ops *br_ops, void *priv)
int fpga_mgr_register(struct device *dev, const char *name,
		      const struct fpga_manager_ops *mops,
		      void *priv)

is it possible to return struct fpga_bridge/manager * in the register
functions? otherwise in driver we have to get the related pointer from
the drvdata of the parent device right after creation of each fpga-*.
The parent device only saves one fpga-* in drvdata at a time per current
API. If these APIs return the fpga-* pointer, then we don't need to care
about the what is saved in drvdata of the parent device.

Thanks
Hao

> Alan
> 
> > If yes, shall we provide some more APIs which accept
> > fpga_bridge (and same for fpga-mgr) as parameter instead of the parent
> > device just like fpga-region?
> >
> > Thanks
> > Hao
> >

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-03 20:07     ` Alan Tull
  2017-05-04  9:20       ` Wu Hao
@ 2017-05-04 21:31       ` Alan Tull
  2017-05-08  8:44         ` Wu, Hao
  1 sibling, 1 reply; 39+ messages in thread
From: Alan Tull @ 2017-05-04 21:31 UTC (permalink / raw)
  To: Wu Hao; +Cc: Moritz Fischer, linux-kernel, linux-fpga, matthew.gerlach

On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
> On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
>>> Add two functions for getting the FPGA bridge from the device
>>> rather than device tree node.  This is to enable writing code
>>> that will support using FPGA bridges without device tree.
>>> Rename one old function to make it clear that it is device
>>> tree-ish.  This leaves us with 3 functions for getting a bridge:
>>>
>>> * fpga_bridge_get
>>>   Get the bridge given the device.
>>>
>>> * fpga_bridges_get_to_list
>>>   Given the device, get the bridge and add it to a list.
>>>
>>> * of_fpga_bridges_get_to_list
>>>   Renamed from priviously existing fpga_bridges_get_to_list.
>>>   Given the device node, get the bridge and add it to a list.
>>>
>>
>> Hi Alan
>>
>> Thanks a lot for providing this patch set for non device tree support. :)
>> Actually I am reworking the Intel FPGA device drivers based on this patch
>> set, and I find some problems with the existing APIs including fpga bridge
>> and manager. My idea is to create all fpga bridges/regions/manager under
>> the same platform device (FME), it allows FME driver to establish the
>> relationship for the bridges/regions/managers it creates in an easy way.
>> But I found current fpga class API doesn't support this very well.
>> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>> parameter, but it doesn't work if we have multiple bridges (and
>> regions/manager) under the same platform device. fpga_mgr has similar
>> issue, but fpga_region APIs work better, as they accept fpga_region as
>> parameter not the shared parent device.
>
> That's good feedback.  I can post a couple patches that apply on top
> of that patchset to add the APIs you need.
>
> Probably what I'll do is add
>
> struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>
> And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the following:
>
> struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>                                 struct fpga_image_info *info);
>
> int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>                                struct fpga_image_info *info,
>                                struct list_head *bridge_list);
>
> Working on it now.
>
>>
>> Do you think if having multiple fpga-* under one parent device is in the
>> right direction?
>
> That should be fine as long as it's coded with an eye on making things
> reusable and seeing beyond the current project.  Just thinking of the
> future and of what can be of general usefulness for others.  And there
> will be others interested in reusing this.
>
> Alan

Actually,  I don't think you will need the additional APIs we were
just discussing after all.  What you have is a multifunction device
(single piece of hardware, multi functions such as in drivers/mfd).
It will have child devices for the mgr, bridges, and regions.  When
registering the mgr and bridges you will need to allocate child
devices and use them to create the mgr and bridges.

Alan

>
>> If yes, shall we provide some more APIs which accept
>> fpga_bridge (and same for fpga-mgr) as parameter instead of the parent
>> device just like fpga-region?
>>
>> Thanks
>> Hao
>>

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

* Re: [PATCH v2 06/16] fpga: region: remove unneeded of_node_get and put
  2017-04-20 14:09 ` [PATCH v2 06/16] fpga: region: remove unneeded of_node_get and put Alan Tull
@ 2017-05-05 17:08   ` Moritz Fischer
  0 siblings, 0 replies; 39+ messages in thread
From: Moritz Fischer @ 2017-05-05 17:08 UTC (permalink / raw)
  To: Alan Tull; +Cc: Linux Kernel Mailing List, linux-fpga

On Thu, Apr 20, 2017 at 7:09 AM, Alan Tull <atull@kernel.org> wrote:
> Remove of_node_get/put in fpga_region_get/put.  Not
> needed and will get in the way when I separate out
> the common FPGA region code from Device Tree support
> code.
>
> Signed-off-by: Alan Tull <atull@kernel.org>

Acked-by: Moritz Fischer <mdf@kernel.org>
> ---
> v2: split out from another patch
> ---
>  drivers/fpga/fpga-region.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index eaea9d4..6784357 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -94,9 +94,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)
>         }
>
>         get_device(dev);
> -       of_node_get(dev->of_node);
>         if (!try_module_get(dev->parent->driver->owner)) {
> -               of_node_put(dev->of_node);
>                 put_device(dev);
>                 mutex_unlock(&region->mutex);
>                 return ERR_PTR(-ENODEV);
> @@ -119,7 +117,6 @@ static void fpga_region_put(struct fpga_region *region)
>         dev_dbg(dev, "put\n");
>
>         module_put(dev->parent->driver->owner);
> -       of_node_put(dev->of_node);
>         put_device(dev);
>         mutex_unlock(&region->mutex);
>  }
> --
> 2.7.4
>

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

* Re: [PATCH v2 08/16] fpga: region: check for child regions before allocing image info
  2017-04-20 14:09 ` [PATCH v2 08/16] fpga: region: check for child regions before allocing image info Alan Tull
@ 2017-05-05 20:59   ` Moritz Fischer
  2017-05-08 21:03     ` Alan Tull
  0 siblings, 1 reply; 39+ messages in thread
From: Moritz Fischer @ 2017-05-05 20:59 UTC (permalink / raw)
  To: Alan Tull; +Cc: Linux Kernel Mailing List, linux-fpga

On Thu, Apr 20, 2017 at 7:09 AM, Alan Tull <atull@kernel.org> wrote:
> During a device tree overlay pre-apply notification, the check
> for child FPGA regions can happen slightly earlier.  This saves
> us from allocating the FPGA image info that just gets thrown
> away.
>
> This is a baby step in refactoring the FPGA region code to
> separate out common FPGA region code from FPGA region
> Device Tree overlay support.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
Acked-by: Moritz Fischer <mdf@kernel.org>
> ---
> v2: split out from another patch
> ---
>  drivers/fpga/fpga-region.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index 7ff5426..c2730a8 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -355,15 +355,19 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
>         const char *firmware_name;
>         int ret;
>
> -       info = fpga_image_info_alloc(&region->dev);
> -       if (!info)
> -               return -ENOMEM;
> -
> -       /* Reject overlay if child FPGA Regions have firmware-name property */
> +       /*
> +        * 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(nd->overlay);
>         if (ret)
>                 return ret;
>
> +       info = fpga_image_info_alloc(dev);
> +       if (!info)
> +               return -ENOMEM;
> +
>         /* Read FPGA region properties from the overlay */
>         if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
>                 info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
> --
> 2.7.4
>

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

* RE: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-04 21:31       ` Alan Tull
@ 2017-05-08  8:44         ` Wu, Hao
  2017-05-08 20:44           ` Alan Tull
  0 siblings, 1 reply; 39+ messages in thread
From: Wu, Hao @ 2017-05-08  8:44 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, linux-kernel, linux-fpga, matthew.gerlach

> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
> >> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
> >>> Add two functions for getting the FPGA bridge from the device
> >>> rather than device tree node.  This is to enable writing code
> >>> that will support using FPGA bridges without device tree.
> >>> Rename one old function to make it clear that it is device
> >>> tree-ish.  This leaves us with 3 functions for getting a bridge:
> >>>
> >>> * fpga_bridge_get
> >>>   Get the bridge given the device.
> >>>
> >>> * fpga_bridges_get_to_list
> >>>   Given the device, get the bridge and add it to a list.
> >>>
> >>> * of_fpga_bridges_get_to_list
> >>>   Renamed from priviously existing fpga_bridges_get_to_list.
> >>>   Given the device node, get the bridge and add it to a list.
> >>>
> >>
> >> Hi Alan
> >>
> >> Thanks a lot for providing this patch set for non device tree support. :)
> >> Actually I am reworking the Intel FPGA device drivers based on this patch
> >> set, and I find some problems with the existing APIs including fpga bridge
> >> and manager. My idea is to create all fpga bridges/regions/manager under
> >> the same platform device (FME), it allows FME driver to establish the
> >> relationship for the bridges/regions/managers it creates in an easy way.
> >> But I found current fpga class API doesn't support this very well.
> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
> >> parameter, but it doesn't work if we have multiple bridges (and
> >> regions/manager) under the same platform device. fpga_mgr has similar
> >> issue, but fpga_region APIs work better, as they accept fpga_region as
> >> parameter not the shared parent device.
> >
> > That's good feedback.  I can post a couple patches that apply on top
> > of that patchset to add the APIs you need.
> >
> > Probably what I'll do is add
> >
> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
> >
> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
> following:
> >
> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
> >                                 struct fpga_image_info *info);
> >
> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
> >                                struct fpga_image_info *info,
> >                                struct list_head *bridge_list);
> >
> > Working on it now.
> >
> >>
> >> Do you think if having multiple fpga-* under one parent device is in the
> >> right direction?
> >
> > That should be fine as long as it's coded with an eye on making things
> > reusable and seeing beyond the current project.  Just thinking of the
> > future and of what can be of general usefulness for others.  And there
> > will be others interested in reusing this.
> >
> > Alan
> 
> Actually,  I don't think you will need the additional APIs we were
> just discussing after all.  What you have is a multifunction device
> (single piece of hardware, multi functions such as in drivers/mfd).
> It will have child devices for the mgr, bridges, and regions.  When
> registering the mgr and bridges you will need to allocate child
> devices and use them to create the mgr and bridges.
> 
> Alan

Hi Alan

I tried to create child devices as the parent device for the mgr and
bridges in fme platform driver module. If only creates the device without
driver, it doesn't work as try_module_get(dev->parent->driver->owner)
always failed in mgr_get and bridge_get functions.

If it creates platform devices as child devices, and introduce new platform
device drivers for bridge and mgr, then it will be difficult to establish the
relationship for region/mgr/bridges (e.g when should region->mgr be
configured and cleared, as mgr is created/destroyed when mgr parent
device platform driver module is loaded/unload), and it maybe not really
necessary to introduce more different driver modules here.

But if it allows multiple fpga-* created under one device in one device
driver, it will be much easier to avoid above problems. So I asked if it
is possible to create multiple fpga-* under one parent device, I feel
this will not impact to current fpga drivers a lot, but provide more
flexibility for drivers to use fpga-region/bridge/manager to create
the topology in a device specific way, especially for non device
tree case. 

Thanks
Hao

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-08  8:44         ` Wu, Hao
@ 2017-05-08 20:44           ` Alan Tull
  2017-05-08 20:52             ` Moritz Fischer
  2017-05-09  7:16             ` Wu Hao
  0 siblings, 2 replies; 39+ messages in thread
From: Alan Tull @ 2017-05-08 20:44 UTC (permalink / raw)
  To: Wu, Hao; +Cc: Moritz Fischer, linux-kernel, linux-fpga, matthew.gerlach

On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>> >> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
>> >>> Add two functions for getting the FPGA bridge from the device
>> >>> rather than device tree node.  This is to enable writing code
>> >>> that will support using FPGA bridges without device tree.
>> >>> Rename one old function to make it clear that it is device
>> >>> tree-ish.  This leaves us with 3 functions for getting a bridge:
>> >>>
>> >>> * fpga_bridge_get
>> >>>   Get the bridge given the device.
>> >>>
>> >>> * fpga_bridges_get_to_list
>> >>>   Given the device, get the bridge and add it to a list.
>> >>>
>> >>> * of_fpga_bridges_get_to_list
>> >>>   Renamed from priviously existing fpga_bridges_get_to_list.
>> >>>   Given the device node, get the bridge and add it to a list.
>> >>>
>> >>
>> >> Hi Alan
>> >>
>> >> Thanks a lot for providing this patch set for non device tree support. :)
>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>> >> set, and I find some problems with the existing APIs including fpga bridge
>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>> >> the same platform device (FME), it allows FME driver to establish the
>> >> relationship for the bridges/regions/managers it creates in an easy way.
>> >> But I found current fpga class API doesn't support this very well.
>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>> >> parameter, but it doesn't work if we have multiple bridges (and
>> >> regions/manager) under the same platform device. fpga_mgr has similar
>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>> >> parameter not the shared parent device.
>> >
>> > That's good feedback.  I can post a couple patches that apply on top
>> > of that patchset to add the APIs you need.
>> >
>> > Probably what I'll do is add
>> >
>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>> >
>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>> following:
>> >
>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>> >                                 struct fpga_image_info *info);
>> >
>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>> >                                struct fpga_image_info *info,
>> >                                struct list_head *bridge_list);
>> >
>> > Working on it now.
>> >
>> >>
>> >> Do you think if having multiple fpga-* under one parent device is in the
>> >> right direction?
>> >
>> > That should be fine as long as it's coded with an eye on making things
>> > reusable and seeing beyond the current project.  Just thinking of the
>> > future and of what can be of general usefulness for others.  And there
>> > will be others interested in reusing this.
>> >
>> > Alan
>>
>> Actually,  I don't think you will need the additional APIs we were
>> just discussing after all.  What you have is a multifunction device
>> (single piece of hardware, multi functions such as in drivers/mfd).
>> It will have child devices for the mgr, bridges, and regions.  When
>> registering the mgr and bridges you will need to allocate child
>> devices and use them to create the mgr and bridges.
>>
>> Alan
>
> Hi Alan
>
> I tried to create child devices as the parent device for the mgr and
> bridges in fme platform driver module. If only creates the device without
> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
> always failed in mgr_get and bridge_get functions.

I tried it and it wasn't hard.

Each mgr or bridge driver should be a separate file which registers
its driver using 'module_platform_driver".  That way the drivers are
registered with the kernel in a normal fashion.  The thing we want
here is to not bypass the kernel driver model.

You'll need to keep the platform_device pointers in private data somewhere.

For each child platform device, do a platform_device_alloc and
platform_device_add.

Then to get the manager, you can do

mgr = fpga_mgr_get(&priv->mgr_pdev->dev);

If this is in your probe function, you can use -EPROBE_DEFER if
platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
whatever you've created and return -EPROBE_DEFER to wait for the
drivers you need to be registered and ready for devices to be added.

>
> If it creates platform devices as child devices, and introduce new platform
> device drivers for bridge and mgr, then it will be difficult to establish the
> relationship for region/mgr/bridges (e.g when should region->mgr be
> configured and cleared, as mgr is created/destroyed when mgr parent
> device platform driver module is loaded/unload), and it maybe not really
> necessary to introduce more different driver modules here.

It should be pretty easy to create/destroy child devices as shown
above.  The kernel does this all the time.

>
> But if it allows multiple fpga-* created under one device in one device
> driver, it will be much easier to avoid above problems. So I asked if it
> is possible to create multiple fpga-* under one parent device,

I think it's fine for your FME to create child platform devices.  It's
similar to a mfd, but the mfd framework hides the platform devices
from the module that creates them, unfortunately.

> I feel
> this will not impact to current fpga drivers a lot, but provide more
> flexibility for drivers to use fpga-region/bridge/manager to create
> the topology in a device specific way, especially for non device
> tree case.
>

I would like to see most of this code as FME enumeration code + a mgr
driver + a bridge driver + a region driver.  If the FME and the
enumeration code can be separate files, so much the better for general
usability.

The enumeration code can build a set of regions by doing something like this:
1. figure out what type of mgr and bridges your hardware FME has.
2. do platform_device_alloc and platform_device_add to create the mgr
device, save a pointer to its platform_device in your FME driver's
private data.
2. For each port, create a region and a bridge device.  Save the
region's platform device or struct in a list in your FME driver's
priv.
3. then you can create the sub function devices.

> Thanks
> Hao

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-08 20:44           ` Alan Tull
@ 2017-05-08 20:52             ` Moritz Fischer
  2017-05-08 21:02               ` Alan Tull
  2017-05-09  7:16             ` Wu Hao
  1 sibling, 1 reply; 39+ messages in thread
From: Moritz Fischer @ 2017-05-08 20:52 UTC (permalink / raw)
  To: Alan Tull; +Cc: Wu, Hao, linux-kernel, linux-fpga, matthew.gerlach

On Mon, May 8, 2017 at 1:44 PM, Alan Tull <atull@kernel.org> wrote:
> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>>> >> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
>>> >>> Add two functions for getting the FPGA bridge from the device
>>> >>> rather than device tree node.  This is to enable writing code
>>> >>> that will support using FPGA bridges without device tree.
>>> >>> Rename one old function to make it clear that it is device
>>> >>> tree-ish.  This leaves us with 3 functions for getting a bridge:
>>> >>>
>>> >>> * fpga_bridge_get
>>> >>>   Get the bridge given the device.
>>> >>>
>>> >>> * fpga_bridges_get_to_list
>>> >>>   Given the device, get the bridge and add it to a list.
>>> >>>
>>> >>> * of_fpga_bridges_get_to_list
>>> >>>   Renamed from priviously existing fpga_bridges_get_to_list.
>>> >>>   Given the device node, get the bridge and add it to a list.
>>> >>>
>>> >>
>>> >> Hi Alan
>>> >>
>>> >> Thanks a lot for providing this patch set for non device tree support. :)
>>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>>> >> set, and I find some problems with the existing APIs including fpga bridge
>>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>>> >> the same platform device (FME), it allows FME driver to establish the
>>> >> relationship for the bridges/regions/managers it creates in an easy way.
>>> >> But I found current fpga class API doesn't support this very well.
>>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>>> >> parameter, but it doesn't work if we have multiple bridges (and
>>> >> regions/manager) under the same platform device. fpga_mgr has similar
>>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>>> >> parameter not the shared parent device.
>>> >
>>> > That's good feedback.  I can post a couple patches that apply on top
>>> > of that patchset to add the APIs you need.
>>> >
>>> > Probably what I'll do is add
>>> >
>>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>>> >
>>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>>> following:
>>> >
>>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>>> >                                 struct fpga_image_info *info);
>>> >
>>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>>> >                                struct fpga_image_info *info,
>>> >                                struct list_head *bridge_list);
>>> >
>>> > Working on it now.
>>> >
>>> >>
>>> >> Do you think if having multiple fpga-* under one parent device is in the
>>> >> right direction?
>>> >
>>> > That should be fine as long as it's coded with an eye on making things
>>> > reusable and seeing beyond the current project.  Just thinking of the
>>> > future and of what can be of general usefulness for others.  And there
>>> > will be others interested in reusing this.
>>> >
>>> > Alan
>>>
>>> Actually,  I don't think you will need the additional APIs we were
>>> just discussing after all.  What you have is a multifunction device
>>> (single piece of hardware, multi functions such as in drivers/mfd).
>>> It will have child devices for the mgr, bridges, and regions.  When
>>> registering the mgr and bridges you will need to allocate child
>>> devices and use them to create the mgr and bridges.
>>>
>>> Alan
>>
>> Hi Alan
>>
>> I tried to create child devices as the parent device for the mgr and
>> bridges in fme platform driver module. If only creates the device without
>> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
>> always failed in mgr_get and bridge_get functions.
>
> I tried it and it wasn't hard.
>
> Each mgr or bridge driver should be a separate file which registers
> its driver using 'module_platform_driver".  That way the drivers are
> registered with the kernel in a normal fashion.  The thing we want
> here is to not bypass the kernel driver model.
>
> You'll need to keep the platform_device pointers in private data somewhere.
>
> For each child platform device, do a platform_device_alloc and
> platform_device_add.
>
> Then to get the manager, you can do
>
> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
>
> If this is in your probe function, you can use -EPROBE_DEFER if
> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
> whatever you've created and return -EPROBE_DEFER to wait for the
> drivers you need to be registered and ready for devices to be added.
>
>>
>> If it creates platform devices as child devices, and introduce new platform
>> device drivers for bridge and mgr, then it will be difficult to establish the
>> relationship for region/mgr/bridges (e.g when should region->mgr be
>> configured and cleared, as mgr is created/destroyed when mgr parent
>> device platform driver module is loaded/unload), and it maybe not really
>> necessary to introduce more different driver modules here.
>
> It should be pretty easy to create/destroy child devices as shown
> above.  The kernel does this all the time.
>
>>
>> But if it allows multiple fpga-* created under one device in one device
>> driver, it will be much easier to avoid above problems. So I asked if it
>> is possible to create multiple fpga-* under one parent device,
>
> I think it's fine for your FME to create child platform devices.  It's
> similar to a mfd, but the mfd framework hides the platform devices
> from the module that creates them, unfortunately.
>
>> I feel
>> this will not impact to current fpga drivers a lot, but provide more
>> flexibility for drivers to use fpga-region/bridge/manager to create
>> the topology in a device specific way, especially for non device
>> tree case.
>>
>
> I would like to see most of this code as FME enumeration code + a mgr
> driver + a bridge driver + a region driver.  If the FME and the
> enumeration code can be separate files, so much the better for general
> usability.
>
> The enumeration code can build a set of regions by doing something like this:
> 1. figure out what type of mgr and bridges your hardware FME has.
> 2. do platform_device_alloc and platform_device_add to create the mgr
> device, save a pointer to its platform_device in your FME driver's
> private data.
> 2. For each port, create a region and a bridge device.  Save the
> region's platform device or struct in a list in your FME driver's
> priv.
> 3. then you can create the sub function devices.

The above sounds like a poster-child application for MFD. If you do it
in a clever
way (i.e. write your platform drivers in a reusable way) you might be able to
just reuse them on your next generation.

Cheers,

Moritz

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-08 20:52             ` Moritz Fischer
@ 2017-05-08 21:02               ` Alan Tull
  2017-05-08 21:11                 ` Moritz Fischer
  0 siblings, 1 reply; 39+ messages in thread
From: Alan Tull @ 2017-05-08 21:02 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Wu, Hao, linux-kernel, linux-fpga, matthew.gerlach

On Mon, May 8, 2017 at 3:52 PM, Moritz Fischer <mdf@kernel.org> wrote:
> On Mon, May 8, 2017 at 1:44 PM, Alan Tull <atull@kernel.org> wrote:
>> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>>>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>>>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>>>> >> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
>>>> >>> Add two functions for getting the FPGA bridge from the device
>>>> >>> rather than device tree node.  This is to enable writing code
>>>> >>> that will support using FPGA bridges without device tree.
>>>> >>> Rename one old function to make it clear that it is device
>>>> >>> tree-ish.  This leaves us with 3 functions for getting a bridge:
>>>> >>>
>>>> >>> * fpga_bridge_get
>>>> >>>   Get the bridge given the device.
>>>> >>>
>>>> >>> * fpga_bridges_get_to_list
>>>> >>>   Given the device, get the bridge and add it to a list.
>>>> >>>
>>>> >>> * of_fpga_bridges_get_to_list
>>>> >>>   Renamed from priviously existing fpga_bridges_get_to_list.
>>>> >>>   Given the device node, get the bridge and add it to a list.
>>>> >>>
>>>> >>
>>>> >> Hi Alan
>>>> >>
>>>> >> Thanks a lot for providing this patch set for non device tree support. :)
>>>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>>>> >> set, and I find some problems with the existing APIs including fpga bridge
>>>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>>>> >> the same platform device (FME), it allows FME driver to establish the
>>>> >> relationship for the bridges/regions/managers it creates in an easy way.
>>>> >> But I found current fpga class API doesn't support this very well.
>>>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>>>> >> parameter, but it doesn't work if we have multiple bridges (and
>>>> >> regions/manager) under the same platform device. fpga_mgr has similar
>>>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>>>> >> parameter not the shared parent device.
>>>> >
>>>> > That's good feedback.  I can post a couple patches that apply on top
>>>> > of that patchset to add the APIs you need.
>>>> >
>>>> > Probably what I'll do is add
>>>> >
>>>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>>>> >
>>>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>>>> following:
>>>> >
>>>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>>>> >                                 struct fpga_image_info *info);
>>>> >
>>>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>>>> >                                struct fpga_image_info *info,
>>>> >                                struct list_head *bridge_list);
>>>> >
>>>> > Working on it now.
>>>> >
>>>> >>
>>>> >> Do you think if having multiple fpga-* under one parent device is in the
>>>> >> right direction?
>>>> >
>>>> > That should be fine as long as it's coded with an eye on making things
>>>> > reusable and seeing beyond the current project.  Just thinking of the
>>>> > future and of what can be of general usefulness for others.  And there
>>>> > will be others interested in reusing this.
>>>> >
>>>> > Alan
>>>>
>>>> Actually,  I don't think you will need the additional APIs we were
>>>> just discussing after all.  What you have is a multifunction device
>>>> (single piece of hardware, multi functions such as in drivers/mfd).
>>>> It will have child devices for the mgr, bridges, and regions.  When
>>>> registering the mgr and bridges you will need to allocate child
>>>> devices and use them to create the mgr and bridges.
>>>>
>>>> Alan
>>>
>>> Hi Alan
>>>
>>> I tried to create child devices as the parent device for the mgr and
>>> bridges in fme platform driver module. If only creates the device without
>>> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
>>> always failed in mgr_get and bridge_get functions.
>>
>> I tried it and it wasn't hard.
>>
>> Each mgr or bridge driver should be a separate file which registers
>> its driver using 'module_platform_driver".  That way the drivers are
>> registered with the kernel in a normal fashion.  The thing we want
>> here is to not bypass the kernel driver model.
>>
>> You'll need to keep the platform_device pointers in private data somewhere.
>>
>> For each child platform device, do a platform_device_alloc and
>> platform_device_add.
>>
>> Then to get the manager, you can do
>>
>> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
>>
>> If this is in your probe function, you can use -EPROBE_DEFER if
>> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
>> whatever you've created and return -EPROBE_DEFER to wait for the
>> drivers you need to be registered and ready for devices to be added.
>>
>>>
>>> If it creates platform devices as child devices, and introduce new platform
>>> device drivers for bridge and mgr, then it will be difficult to establish the
>>> relationship for region/mgr/bridges (e.g when should region->mgr be
>>> configured and cleared, as mgr is created/destroyed when mgr parent
>>> device platform driver module is loaded/unload), and it maybe not really
>>> necessary to introduce more different driver modules here.
>>
>> It should be pretty easy to create/destroy child devices as shown
>> above.  The kernel does this all the time.
>>
>>>
>>> But if it allows multiple fpga-* created under one device in one device
>>> driver, it will be much easier to avoid above problems. So I asked if it
>>> is possible to create multiple fpga-* under one parent device,
>>
>> I think it's fine for your FME to create child platform devices.  It's
>> similar to a mfd, but the mfd framework hides the platform devices
>> from the module that creates them, unfortunately.
>>
>>> I feel
>>> this will not impact to current fpga drivers a lot, but provide more
>>> flexibility for drivers to use fpga-region/bridge/manager to create
>>> the topology in a device specific way, especially for non device
>>> tree case.
>>>
>>
>> I would like to see most of this code as FME enumeration code + a mgr
>> driver + a bridge driver + a region driver.  If the FME and the
>> enumeration code can be separate files, so much the better for general
>> usability.
>>
>> The enumeration code can build a set of regions by doing something like this:
>> 1. figure out what type of mgr and bridges your hardware FME has.
>> 2. do platform_device_alloc and platform_device_add to create the mgr
>> device, save a pointer to its platform_device in your FME driver's
>> private data.
>> 2. For each port, create a region and a bridge device.  Save the
>> region's platform device or struct in a list in your FME driver's
>> priv.
>> 3. then you can create the sub function devices.
>
> The above sounds like a poster-child application for MFD. If you do it
> in a clever
> way (i.e. write your platform drivers in a reusable way) you might be able to
> just reuse them on your next generation.

Yes, I played with that with some test code.   I wrote some test code
that allocates dummy mgr and bridge devices using mfd_add_devices().
I didn't see any way of getting access to the devices after creating
them.  Maybe I'm missing something.  Neither the dev nor the
platform_device is saved in the cell struct.

Alan

>
> Cheers,
>
> Moritz

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

* Re: [PATCH v2 08/16] fpga: region: check for child regions before allocing image info
  2017-05-05 20:59   ` Moritz Fischer
@ 2017-05-08 21:03     ` Alan Tull
  0 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-05-08 21:03 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Linux Kernel Mailing List, linux-fpga

On Fri, May 5, 2017 at 3:59 PM, Moritz Fischer <mdf@kernel.org> wrote:
> On Thu, Apr 20, 2017 at 7:09 AM, Alan Tull <atull@kernel.org> wrote:
>> During a device tree overlay pre-apply notification, the check
>> for child FPGA regions can happen slightly earlier.  This saves
>> us from allocating the FPGA image info that just gets thrown
>> away.
>>
>> This is a baby step in refactoring the FPGA region code to
>> separate out common FPGA region code from FPGA region
>> Device Tree overlay support.
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
> Acked-by: Moritz Fischer <mdf@kernel.org>

Thanks!

Alan

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-08 21:02               ` Alan Tull
@ 2017-05-08 21:11                 ` Moritz Fischer
  2017-05-08 21:20                   ` Alan Tull
  0 siblings, 1 reply; 39+ messages in thread
From: Moritz Fischer @ 2017-05-08 21:11 UTC (permalink / raw)
  To: Alan Tull; +Cc: Wu, Hao, linux-kernel, linux-fpga, matthew.gerlach

Hi Alan,

On Mon, May 8, 2017 at 2:02 PM, Alan Tull <atull@kernel.org> wrote:
> On Mon, May 8, 2017 at 3:52 PM, Moritz Fischer <mdf@kernel.org> wrote:
>> On Mon, May 8, 2017 at 1:44 PM, Alan Tull <atull@kernel.org> wrote:
>>> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>>>>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>>>>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>>>>> >> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
>>>>> >>> Add two functions for getting the FPGA bridge from the device
>>>>> >>> rather than device tree node.  This is to enable writing code
>>>>> >>> that will support using FPGA bridges without device tree.
>>>>> >>> Rename one old function to make it clear that it is device
>>>>> >>> tree-ish.  This leaves us with 3 functions for getting a bridge:
>>>>> >>>
>>>>> >>> * fpga_bridge_get
>>>>> >>>   Get the bridge given the device.
>>>>> >>>
>>>>> >>> * fpga_bridges_get_to_list
>>>>> >>>   Given the device, get the bridge and add it to a list.
>>>>> >>>
>>>>> >>> * of_fpga_bridges_get_to_list
>>>>> >>>   Renamed from priviously existing fpga_bridges_get_to_list.
>>>>> >>>   Given the device node, get the bridge and add it to a list.
>>>>> >>>
>>>>> >>
>>>>> >> Hi Alan
>>>>> >>
>>>>> >> Thanks a lot for providing this patch set for non device tree support. :)
>>>>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>>>>> >> set, and I find some problems with the existing APIs including fpga bridge
>>>>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>>>>> >> the same platform device (FME), it allows FME driver to establish the
>>>>> >> relationship for the bridges/regions/managers it creates in an easy way.
>>>>> >> But I found current fpga class API doesn't support this very well.
>>>>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>>>>> >> parameter, but it doesn't work if we have multiple bridges (and
>>>>> >> regions/manager) under the same platform device. fpga_mgr has similar
>>>>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>>>>> >> parameter not the shared parent device.
>>>>> >
>>>>> > That's good feedback.  I can post a couple patches that apply on top
>>>>> > of that patchset to add the APIs you need.
>>>>> >
>>>>> > Probably what I'll do is add
>>>>> >
>>>>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>>>>> >
>>>>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>>>>> following:
>>>>> >
>>>>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>>>>> >                                 struct fpga_image_info *info);
>>>>> >
>>>>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>>>>> >                                struct fpga_image_info *info,
>>>>> >                                struct list_head *bridge_list);
>>>>> >
>>>>> > Working on it now.
>>>>> >
>>>>> >>
>>>>> >> Do you think if having multiple fpga-* under one parent device is in the
>>>>> >> right direction?
>>>>> >
>>>>> > That should be fine as long as it's coded with an eye on making things
>>>>> > reusable and seeing beyond the current project.  Just thinking of the
>>>>> > future and of what can be of general usefulness for others.  And there
>>>>> > will be others interested in reusing this.
>>>>> >
>>>>> > Alan
>>>>>
>>>>> Actually,  I don't think you will need the additional APIs we were
>>>>> just discussing after all.  What you have is a multifunction device
>>>>> (single piece of hardware, multi functions such as in drivers/mfd).
>>>>> It will have child devices for the mgr, bridges, and regions.  When
>>>>> registering the mgr and bridges you will need to allocate child
>>>>> devices and use them to create the mgr and bridges.
>>>>>
>>>>> Alan
>>>>
>>>> Hi Alan
>>>>
>>>> I tried to create child devices as the parent device for the mgr and
>>>> bridges in fme platform driver module. If only creates the device without
>>>> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
>>>> always failed in mgr_get and bridge_get functions.
>>>
>>> I tried it and it wasn't hard.
>>>
>>> Each mgr or bridge driver should be a separate file which registers
>>> its driver using 'module_platform_driver".  That way the drivers are
>>> registered with the kernel in a normal fashion.  The thing we want
>>> here is to not bypass the kernel driver model.
>>>
>>> You'll need to keep the platform_device pointers in private data somewhere.
>>>
>>> For each child platform device, do a platform_device_alloc and
>>> platform_device_add.
>>>
>>> Then to get the manager, you can do
>>>
>>> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
>>>
>>> If this is in your probe function, you can use -EPROBE_DEFER if
>>> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
>>> whatever you've created and return -EPROBE_DEFER to wait for the
>>> drivers you need to be registered and ready for devices to be added.
>>>
>>>>
>>>> If it creates platform devices as child devices, and introduce new platform
>>>> device drivers for bridge and mgr, then it will be difficult to establish the
>>>> relationship for region/mgr/bridges (e.g when should region->mgr be
>>>> configured and cleared, as mgr is created/destroyed when mgr parent
>>>> device platform driver module is loaded/unload), and it maybe not really
>>>> necessary to introduce more different driver modules here.
>>>
>>> It should be pretty easy to create/destroy child devices as shown
>>> above.  The kernel does this all the time.
>>>
>>>>
>>>> But if it allows multiple fpga-* created under one device in one device
>>>> driver, it will be much easier to avoid above problems. So I asked if it
>>>> is possible to create multiple fpga-* under one parent device,
>>>
>>> I think it's fine for your FME to create child platform devices.  It's
>>> similar to a mfd, but the mfd framework hides the platform devices
>>> from the module that creates them, unfortunately.
>>>
>>>> I feel
>>>> this will not impact to current fpga drivers a lot, but provide more
>>>> flexibility for drivers to use fpga-region/bridge/manager to create
>>>> the topology in a device specific way, especially for non device
>>>> tree case.
>>>>
>>>
>>> I would like to see most of this code as FME enumeration code + a mgr
>>> driver + a bridge driver + a region driver.  If the FME and the
>>> enumeration code can be separate files, so much the better for general
>>> usability.
>>>
>>> The enumeration code can build a set of regions by doing something like this:
>>> 1. figure out what type of mgr and bridges your hardware FME has.
>>> 2. do platform_device_alloc and platform_device_add to create the mgr
>>> device, save a pointer to its platform_device in your FME driver's
>>> private data.
>>> 2. For each port, create a region and a bridge device.  Save the
>>> region's platform device or struct in a list in your FME driver's
>>> priv.
>>> 3. then you can create the sub function devices.
>>
>> The above sounds like a poster-child application for MFD. If you do it
>> in a clever
>> way (i.e. write your platform drivers in a reusable way) you might be able to
>> just reuse them on your next generation.
>
> Yes, I played with that with some test code.   I wrote some test code
> that allocates dummy mgr and bridge devices using mfd_add_devices().
> I didn't see any way of getting access to the devices after creating
> them.  Maybe I'm missing something.  Neither the dev nor the
> platform_device is saved in the cell struct.

Currently working on some MFD stuff for an RTC, which device are you
trying to get to?

The parent from subdevice (fpga mgr?) you can get by something like:

foo_fpga_mgr_probe(struct platform_device *pdev)
{
    struct foo_parent *parent = dev_get_drvdata(dev->parent);
}

Or are you talking about the other way round (i.e. get access to children from
parent device) ? Which functionality would that achieve?

Cheers,

Moritz

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-08 21:11                 ` Moritz Fischer
@ 2017-05-08 21:20                   ` Alan Tull
  2017-05-08 21:55                     ` Moritz Fischer
  0 siblings, 1 reply; 39+ messages in thread
From: Alan Tull @ 2017-05-08 21:20 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Wu, Hao, linux-kernel, linux-fpga, matthew.gerlach

On Mon, May 8, 2017 at 4:11 PM, Moritz Fischer <mdf@kernel.org> wrote:
> Hi Alan,
>
> On Mon, May 8, 2017 at 2:02 PM, Alan Tull <atull@kernel.org> wrote:
>> On Mon, May 8, 2017 at 3:52 PM, Moritz Fischer <mdf@kernel.org> wrote:
>>> On Mon, May 8, 2017 at 1:44 PM, Alan Tull <atull@kernel.org> wrote:
>>>> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>>>>>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>>>>>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>>>>>> >> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
>>>>>> >>> Add two functions for getting the FPGA bridge from the device
>>>>>> >>> rather than device tree node.  This is to enable writing code
>>>>>> >>> that will support using FPGA bridges without device tree.
>>>>>> >>> Rename one old function to make it clear that it is device
>>>>>> >>> tree-ish.  This leaves us with 3 functions for getting a bridge:
>>>>>> >>>
>>>>>> >>> * fpga_bridge_get
>>>>>> >>>   Get the bridge given the device.
>>>>>> >>>
>>>>>> >>> * fpga_bridges_get_to_list
>>>>>> >>>   Given the device, get the bridge and add it to a list.
>>>>>> >>>
>>>>>> >>> * of_fpga_bridges_get_to_list
>>>>>> >>>   Renamed from priviously existing fpga_bridges_get_to_list.
>>>>>> >>>   Given the device node, get the bridge and add it to a list.
>>>>>> >>>
>>>>>> >>
>>>>>> >> Hi Alan
>>>>>> >>
>>>>>> >> Thanks a lot for providing this patch set for non device tree support. :)
>>>>>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>>>>>> >> set, and I find some problems with the existing APIs including fpga bridge
>>>>>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>>>>>> >> the same platform device (FME), it allows FME driver to establish the
>>>>>> >> relationship for the bridges/regions/managers it creates in an easy way.
>>>>>> >> But I found current fpga class API doesn't support this very well.
>>>>>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>>>>>> >> parameter, but it doesn't work if we have multiple bridges (and
>>>>>> >> regions/manager) under the same platform device. fpga_mgr has similar
>>>>>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>>>>>> >> parameter not the shared parent device.
>>>>>> >
>>>>>> > That's good feedback.  I can post a couple patches that apply on top
>>>>>> > of that patchset to add the APIs you need.
>>>>>> >
>>>>>> > Probably what I'll do is add
>>>>>> >
>>>>>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>>>>>> >
>>>>>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>>>>>> following:
>>>>>> >
>>>>>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>>>>>> >                                 struct fpga_image_info *info);
>>>>>> >
>>>>>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>>>>>> >                                struct fpga_image_info *info,
>>>>>> >                                struct list_head *bridge_list);
>>>>>> >
>>>>>> > Working on it now.
>>>>>> >
>>>>>> >>
>>>>>> >> Do you think if having multiple fpga-* under one parent device is in the
>>>>>> >> right direction?
>>>>>> >
>>>>>> > That should be fine as long as it's coded with an eye on making things
>>>>>> > reusable and seeing beyond the current project.  Just thinking of the
>>>>>> > future and of what can be of general usefulness for others.  And there
>>>>>> > will be others interested in reusing this.
>>>>>> >
>>>>>> > Alan
>>>>>>
>>>>>> Actually,  I don't think you will need the additional APIs we were
>>>>>> just discussing after all.  What you have is a multifunction device
>>>>>> (single piece of hardware, multi functions such as in drivers/mfd).
>>>>>> It will have child devices for the mgr, bridges, and regions.  When
>>>>>> registering the mgr and bridges you will need to allocate child
>>>>>> devices and use them to create the mgr and bridges.
>>>>>>
>>>>>> Alan
>>>>>
>>>>> Hi Alan
>>>>>
>>>>> I tried to create child devices as the parent device for the mgr and
>>>>> bridges in fme platform driver module. If only creates the device without
>>>>> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
>>>>> always failed in mgr_get and bridge_get functions.
>>>>
>>>> I tried it and it wasn't hard.
>>>>
>>>> Each mgr or bridge driver should be a separate file which registers
>>>> its driver using 'module_platform_driver".  That way the drivers are
>>>> registered with the kernel in a normal fashion.  The thing we want
>>>> here is to not bypass the kernel driver model.
>>>>
>>>> You'll need to keep the platform_device pointers in private data somewhere.
>>>>
>>>> For each child platform device, do a platform_device_alloc and
>>>> platform_device_add.
>>>>
>>>> Then to get the manager, you can do
>>>>
>>>> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
>>>>
>>>> If this is in your probe function, you can use -EPROBE_DEFER if
>>>> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
>>>> whatever you've created and return -EPROBE_DEFER to wait for the
>>>> drivers you need to be registered and ready for devices to be added.
>>>>
>>>>>
>>>>> If it creates platform devices as child devices, and introduce new platform
>>>>> device drivers for bridge and mgr, then it will be difficult to establish the
>>>>> relationship for region/mgr/bridges (e.g when should region->mgr be
>>>>> configured and cleared, as mgr is created/destroyed when mgr parent
>>>>> device platform driver module is loaded/unload), and it maybe not really
>>>>> necessary to introduce more different driver modules here.
>>>>
>>>> It should be pretty easy to create/destroy child devices as shown
>>>> above.  The kernel does this all the time.
>>>>
>>>>>
>>>>> But if it allows multiple fpga-* created under one device in one device
>>>>> driver, it will be much easier to avoid above problems. So I asked if it
>>>>> is possible to create multiple fpga-* under one parent device,
>>>>
>>>> I think it's fine for your FME to create child platform devices.  It's
>>>> similar to a mfd, but the mfd framework hides the platform devices
>>>> from the module that creates them, unfortunately.
>>>>
>>>>> I feel
>>>>> this will not impact to current fpga drivers a lot, but provide more
>>>>> flexibility for drivers to use fpga-region/bridge/manager to create
>>>>> the topology in a device specific way, especially for non device
>>>>> tree case.
>>>>>
>>>>
>>>> I would like to see most of this code as FME enumeration code + a mgr
>>>> driver + a bridge driver + a region driver.  If the FME and the
>>>> enumeration code can be separate files, so much the better for general
>>>> usability.
>>>>
>>>> The enumeration code can build a set of regions by doing something like this:
>>>> 1. figure out what type of mgr and bridges your hardware FME has.
>>>> 2. do platform_device_alloc and platform_device_add to create the mgr
>>>> device, save a pointer to its platform_device in your FME driver's
>>>> private data.
>>>> 2. For each port, create a region and a bridge device.  Save the
>>>> region's platform device or struct in a list in your FME driver's
>>>> priv.
>>>> 3. then you can create the sub function devices.
>>>
>>> The above sounds like a poster-child application for MFD. If you do it
>>> in a clever
>>> way (i.e. write your platform drivers in a reusable way) you might be able to
>>> just reuse them on your next generation.
>>
>> Yes, I played with that with some test code.   I wrote some test code
>> that allocates dummy mgr and bridge devices using mfd_add_devices().
>> I didn't see any way of getting access to the devices after creating
>> them.  Maybe I'm missing something.  Neither the dev nor the
>> platform_device is saved in the cell struct.
>
> Currently working on some MFD stuff for an RTC, which device are you
> trying to get to?
>
> The parent from subdevice (fpga mgr?) you can get by something like:
>
> foo_fpga_mgr_probe(struct platform_device *pdev)
> {
>     struct foo_parent *parent = dev_get_drvdata(dev->parent);
> }
>
> Or are you talking about the other way round (i.e. get access to children from
> parent device) ?

That's it.

> Which functionality would that achieve?

Suppose someone (non-DT case) is enumerating some hardware in the FPGA
that includes a mgr and some bridges.  So they create the mgr device.
Then for each bridge and they want to create a bridge device and a
region device and let the region know what mgr and bridge to use.  The
main enumerating device keeps track of all these regions so if stuff
gets unloaded, it can destroy it all properly.

Alan

>
> Cheers,
>
> Moritz

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-08 21:20                   ` Alan Tull
@ 2017-05-08 21:55                     ` Moritz Fischer
  0 siblings, 0 replies; 39+ messages in thread
From: Moritz Fischer @ 2017-05-08 21:55 UTC (permalink / raw)
  To: Alan Tull; +Cc: Wu, Hao, linux-kernel, linux-fpga, matthew.gerlach

Hi Alan,

On Mon, May 8, 2017 at 2:20 PM, Alan Tull <atull@kernel.org> wrote:
> On Mon, May 8, 2017 at 4:11 PM, Moritz Fischer <mdf@kernel.org> wrote:
>> Hi Alan,
>>
>> On Mon, May 8, 2017 at 2:02 PM, Alan Tull <atull@kernel.org> wrote:
>>> On Mon, May 8, 2017 at 3:52 PM, Moritz Fischer <mdf@kernel.org> wrote:
>>>> On Mon, May 8, 2017 at 1:44 PM, Alan Tull <atull@kernel.org> wrote:
>>>>> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>>>>>>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>>>>>>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>>>>>>> >> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
>>>>>>> >>> Add two functions for getting the FPGA bridge from the device
>>>>>>> >>> rather than device tree node.  This is to enable writing code
>>>>>>> >>> that will support using FPGA bridges without device tree.
>>>>>>> >>> Rename one old function to make it clear that it is device
>>>>>>> >>> tree-ish.  This leaves us with 3 functions for getting a bridge:
>>>>>>> >>>
>>>>>>> >>> * fpga_bridge_get
>>>>>>> >>>   Get the bridge given the device.
>>>>>>> >>>
>>>>>>> >>> * fpga_bridges_get_to_list
>>>>>>> >>>   Given the device, get the bridge and add it to a list.
>>>>>>> >>>
>>>>>>> >>> * of_fpga_bridges_get_to_list
>>>>>>> >>>   Renamed from priviously existing fpga_bridges_get_to_list.
>>>>>>> >>>   Given the device node, get the bridge and add it to a list.
>>>>>>> >>>
>>>>>>> >>
>>>>>>> >> Hi Alan
>>>>>>> >>
>>>>>>> >> Thanks a lot for providing this patch set for non device tree support. :)
>>>>>>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>>>>>>> >> set, and I find some problems with the existing APIs including fpga bridge
>>>>>>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>>>>>>> >> the same platform device (FME), it allows FME driver to establish the
>>>>>>> >> relationship for the bridges/regions/managers it creates in an easy way.
>>>>>>> >> But I found current fpga class API doesn't support this very well.
>>>>>>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>>>>>>> >> parameter, but it doesn't work if we have multiple bridges (and
>>>>>>> >> regions/manager) under the same platform device. fpga_mgr has similar
>>>>>>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>>>>>>> >> parameter not the shared parent device.
>>>>>>> >
>>>>>>> > That's good feedback.  I can post a couple patches that apply on top
>>>>>>> > of that patchset to add the APIs you need.
>>>>>>> >
>>>>>>> > Probably what I'll do is add
>>>>>>> >
>>>>>>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>>>>>>> >
>>>>>>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>>>>>>> following:
>>>>>>> >
>>>>>>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>>>>>>> >                                 struct fpga_image_info *info);
>>>>>>> >
>>>>>>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>>>>>>> >                                struct fpga_image_info *info,
>>>>>>> >                                struct list_head *bridge_list);
>>>>>>> >
>>>>>>> > Working on it now.
>>>>>>> >
>>>>>>> >>
>>>>>>> >> Do you think if having multiple fpga-* under one parent device is in the
>>>>>>> >> right direction?
>>>>>>> >
>>>>>>> > That should be fine as long as it's coded with an eye on making things
>>>>>>> > reusable and seeing beyond the current project.  Just thinking of the
>>>>>>> > future and of what can be of general usefulness for others.  And there
>>>>>>> > will be others interested in reusing this.
>>>>>>> >
>>>>>>> > Alan
>>>>>>>
>>>>>>> Actually,  I don't think you will need the additional APIs we were
>>>>>>> just discussing after all.  What you have is a multifunction device
>>>>>>> (single piece of hardware, multi functions such as in drivers/mfd).
>>>>>>> It will have child devices for the mgr, bridges, and regions.  When
>>>>>>> registering the mgr and bridges you will need to allocate child
>>>>>>> devices and use them to create the mgr and bridges.
>>>>>>>
>>>>>>> Alan
>>>>>>
>>>>>> Hi Alan
>>>>>>
>>>>>> I tried to create child devices as the parent device for the mgr and
>>>>>> bridges in fme platform driver module. If only creates the device without
>>>>>> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
>>>>>> always failed in mgr_get and bridge_get functions.
>>>>>
>>>>> I tried it and it wasn't hard.
>>>>>
>>>>> Each mgr or bridge driver should be a separate file which registers
>>>>> its driver using 'module_platform_driver".  That way the drivers are
>>>>> registered with the kernel in a normal fashion.  The thing we want
>>>>> here is to not bypass the kernel driver model.
>>>>>
>>>>> You'll need to keep the platform_device pointers in private data somewhere.
>>>>>
>>>>> For each child platform device, do a platform_device_alloc and
>>>>> platform_device_add.
>>>>>
>>>>> Then to get the manager, you can do
>>>>>
>>>>> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
>>>>>
>>>>> If this is in your probe function, you can use -EPROBE_DEFER if
>>>>> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
>>>>> whatever you've created and return -EPROBE_DEFER to wait for the
>>>>> drivers you need to be registered and ready for devices to be added.
>>>>>
>>>>>>
>>>>>> If it creates platform devices as child devices, and introduce new platform
>>>>>> device drivers for bridge and mgr, then it will be difficult to establish the
>>>>>> relationship for region/mgr/bridges (e.g when should region->mgr be
>>>>>> configured and cleared, as mgr is created/destroyed when mgr parent
>>>>>> device platform driver module is loaded/unload), and it maybe not really
>>>>>> necessary to introduce more different driver modules here.
>>>>>
>>>>> It should be pretty easy to create/destroy child devices as shown
>>>>> above.  The kernel does this all the time.
>>>>>
>>>>>>
>>>>>> But if it allows multiple fpga-* created under one device in one device
>>>>>> driver, it will be much easier to avoid above problems. So I asked if it
>>>>>> is possible to create multiple fpga-* under one parent device,
>>>>>
>>>>> I think it's fine for your FME to create child platform devices.  It's
>>>>> similar to a mfd, but the mfd framework hides the platform devices
>>>>> from the module that creates them, unfortunately.
>>>>>
>>>>>> I feel
>>>>>> this will not impact to current fpga drivers a lot, but provide more
>>>>>> flexibility for drivers to use fpga-region/bridge/manager to create
>>>>>> the topology in a device specific way, especially for non device
>>>>>> tree case.
>>>>>>
>>>>>
>>>>> I would like to see most of this code as FME enumeration code + a mgr
>>>>> driver + a bridge driver + a region driver.  If the FME and the
>>>>> enumeration code can be separate files, so much the better for general
>>>>> usability.
>>>>>
>>>>> The enumeration code can build a set of regions by doing something like this:
>>>>> 1. figure out what type of mgr and bridges your hardware FME has.
>>>>> 2. do platform_device_alloc and platform_device_add to create the mgr
>>>>> device, save a pointer to its platform_device in your FME driver's
>>>>> private data.
>>>>> 2. For each port, create a region and a bridge device.  Save the
>>>>> region's platform device or struct in a list in your FME driver's
>>>>> priv.
>>>>> 3. then you can create the sub function devices.
>>>>
>>>> The above sounds like a poster-child application for MFD. If you do it
>>>> in a clever
>>>> way (i.e. write your platform drivers in a reusable way) you might be able to
>>>> just reuse them on your next generation.
>>>
>>> Yes, I played with that with some test code.   I wrote some test code
>>> that allocates dummy mgr and bridge devices using mfd_add_devices().
>>> I didn't see any way of getting access to the devices after creating
>>> them.  Maybe I'm missing something.  Neither the dev nor the
>>> platform_device is saved in the cell struct.
>>
>> Currently working on some MFD stuff for an RTC, which device are you
>> trying to get to?
>>
>> The parent from subdevice (fpga mgr?) you can get by something like:
>>
>> foo_fpga_mgr_probe(struct platform_device *pdev)
>> {
>>     struct foo_parent *parent = dev_get_drvdata(dev->parent);
>> }
>>
>> Or are you talking about the other way round (i.e. get access to children from
>> parent device) ?
>
> That's it.
>
>> Which functionality would that achieve?
>
> Suppose someone (non-DT case) is enumerating some hardware in the FPGA
> that includes a mgr and some bridges.  So they create the mgr device.
> Then for each bridge and they want to create a bridge device and a
> region device and let the region know what mgr and bridge to use.  The
> main enumerating device keeps track of all these regions so if stuff
> gets unloaded, it can destroy it all properly.

Ok that might be tougher :( Don't have a solution for that up my sleeve ;-)

Cheers,
Moritz

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

* Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device
  2017-05-08 20:44           ` Alan Tull
  2017-05-08 20:52             ` Moritz Fischer
@ 2017-05-09  7:16             ` Wu Hao
  1 sibling, 0 replies; 39+ messages in thread
From: Wu Hao @ 2017-05-09  7:16 UTC (permalink / raw)
  To: Alan Tull; +Cc: Moritz Fischer, linux-kernel, linux-fpga, matthew.gerlach

On Mon, May 08, 2017 at 03:44:12PM -0500, Alan Tull wrote:
> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
> >> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
> >> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
> >> >> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote:
> >> >>> Add two functions for getting the FPGA bridge from the device
> >> >>> rather than device tree node.  This is to enable writing code
> >> >>> that will support using FPGA bridges without device tree.
> >> >>> Rename one old function to make it clear that it is device
> >> >>> tree-ish.  This leaves us with 3 functions for getting a bridge:
> >> >>>
> >> >>> * fpga_bridge_get
> >> >>>   Get the bridge given the device.
> >> >>>
> >> >>> * fpga_bridges_get_to_list
> >> >>>   Given the device, get the bridge and add it to a list.
> >> >>>
> >> >>> * of_fpga_bridges_get_to_list
> >> >>>   Renamed from priviously existing fpga_bridges_get_to_list.
> >> >>>   Given the device node, get the bridge and add it to a list.
> >> >>>
> >> >>
> >> >> Hi Alan
> >> >>
> >> >> Thanks a lot for providing this patch set for non device tree support. :)
> >> >> Actually I am reworking the Intel FPGA device drivers based on this patch
> >> >> set, and I find some problems with the existing APIs including fpga bridge
> >> >> and manager. My idea is to create all fpga bridges/regions/manager under
> >> >> the same platform device (FME), it allows FME driver to establish the
> >> >> relationship for the bridges/regions/managers it creates in an easy way.
> >> >> But I found current fpga class API doesn't support this very well.
> >> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
> >> >> parameter, but it doesn't work if we have multiple bridges (and
> >> >> regions/manager) under the same platform device. fpga_mgr has similar
> >> >> issue, but fpga_region APIs work better, as they accept fpga_region as
> >> >> parameter not the shared parent device.
> >> >
> >> > That's good feedback.  I can post a couple patches that apply on top
> >> > of that patchset to add the APIs you need.
> >> >
> >> > Probably what I'll do is add
> >> >
> >> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
> >> >
> >> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
> >> following:
> >> >
> >> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
> >> >                                 struct fpga_image_info *info);
> >> >
> >> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
> >> >                                struct fpga_image_info *info,
> >> >                                struct list_head *bridge_list);
> >> >
> >> > Working on it now.
> >> >
> >> >>
> >> >> Do you think if having multiple fpga-* under one parent device is in the
> >> >> right direction?
> >> >
> >> > That should be fine as long as it's coded with an eye on making things
> >> > reusable and seeing beyond the current project.  Just thinking of the
> >> > future and of what can be of general usefulness for others.  And there
> >> > will be others interested in reusing this.
> >> >
> >> > Alan
> >>
> >> Actually,  I don't think you will need the additional APIs we were
> >> just discussing after all.  What you have is a multifunction device
> >> (single piece of hardware, multi functions such as in drivers/mfd).
> >> It will have child devices for the mgr, bridges, and regions.  When
> >> registering the mgr and bridges you will need to allocate child
> >> devices and use them to create the mgr and bridges.
> >>
> >> Alan
> >
> > Hi Alan
> >
> > I tried to create child devices as the parent device for the mgr and
> > bridges in fme platform driver module. If only creates the device without
> > driver, it doesn't work as try_module_get(dev->parent->driver->owner)
> > always failed in mgr_get and bridge_get functions.
> 
> I tried it and it wasn't hard.
> 
> Each mgr or bridge driver should be a separate file which registers
> its driver using 'module_platform_driver".  That way the drivers are
> registered with the kernel in a normal fashion.  The thing we want
> here is to not bypass the kernel driver model.
> 
> You'll need to keep the platform_device pointers in private data somewhere.
> 
> For each child platform device, do a platform_device_alloc and
> platform_device_add.
> 
> Then to get the manager, you can do
> 
> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
> 
> If this is in your probe function, you can use -EPROBE_DEFER if
> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
> whatever you've created and return -EPROBE_DEFER to wait for the
> drivers you need to be registered and ready for devices to be added.
> 
> >
> > If it creates platform devices as child devices, and introduce new platform
> > device drivers for bridge and mgr, then it will be difficult to establish the
> > relationship for region/mgr/bridges (e.g when should region->mgr be
> > configured and cleared, as mgr is created/destroyed when mgr parent
> > device platform driver module is loaded/unload), and it maybe not really
> > necessary to introduce more different driver modules here.
> 
> It should be pretty easy to create/destroy child devices as shown
> above.  The kernel does this all the time.
> 
> >
> > But if it allows multiple fpga-* created under one device in one device
> > driver, it will be much easier to avoid above problems. So I asked if it
> > is possible to create multiple fpga-* under one parent device,
> 
> I think it's fine for your FME to create child platform devices.  It's
> similar to a mfd, but the mfd framework hides the platform devices
> from the module that creates them, unfortunately.
> 
> > I feel
> > this will not impact to current fpga drivers a lot, but provide more
> > flexibility for drivers to use fpga-region/bridge/manager to create
> > the topology in a device specific way, especially for non device
> > tree case.
> >
> 
> I would like to see most of this code as FME enumeration code + a mgr
> driver + a bridge driver + a region driver.  If the FME and the
> enumeration code can be separate files, so much the better for general
> usability.
> 
> The enumeration code can build a set of regions by doing something like this:
> 1. figure out what type of mgr and bridges your hardware FME has.
> 2. do platform_device_alloc and platform_device_add to create the mgr
> device, save a pointer to its platform_device in your FME driver's
> private data.
> 2. For each port, create a region and a bridge device.  Save the
> region's platform device or struct in a list in your FME driver's
> priv.
> 3. then you can create the sub function devices.

Thanks Alan for the suggestion.

I will follow this direction to rework the FME driver. In above step 2,
I will put fpga-mgr/bridge platform device info into the platform data
of the fpga-region platform device, then fpga-region knows which mgr
and bridge to use.

Thanks
Hao

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

* Re: [PATCH v2 10/16] fpga: region: use image info as parameter for programming region
  2017-04-20 14:09 ` [PATCH v2 10/16] fpga: region: use image info as parameter for programming region Alan Tull
@ 2017-05-09 15:25   ` Alan Tull
  0 siblings, 0 replies; 39+ messages in thread
From: Alan Tull @ 2017-05-09 15:25 UTC (permalink / raw)
  To: Moritz Fischer; +Cc: Alan Tull, linux-kernel, linux-fpga

On Thu, Apr 20, 2017 at 9:09 AM, Alan Tull <atull@kernel.org> wrote:
> Use FPGA image info as a parameter when region code is
> programming the FPGA.
>
> This is a baby step in refactoring the FPGA region code to
> separate out common FPGA region code from FPGA region
> Device Tree overlay support.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
> v2: split out from another patch
> ---
>  drivers/fpga/fpga-region.c    | 26 ++++++++++++++++----------
>  include/linux/fpga/fpga-mgr.h |  4 ++++
>  2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index d6f2313..1892126 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -223,16 +223,21 @@ 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.
> + * @info: FPGA image info
> + * Program an FPGA using fpga image info.
>   * Return 0 for success or negative error code.
>   */
>  static int fpga_region_program_fpga(struct fpga_region *region,
> -                                   struct device_node *overlay)
> +                                   struct fpga_image_info *info)
>  {
>         struct device *dev = &region->dev;
>         int ret;
>
> +       if (region->info) {
> +               dev_err(dev, "region already programmed\n");
> +               return PTR_ERR(region);
> +       }
> +

Currently if region has info, that is supposed to mean that the region
is programmed.  I can already see how that could be limiting.  I'm
think that in v3, I'm going to change this to add a region->state or
region->status that can indicate whether the region has been
programmed.  Having done that, the second parameter of this functon
can go away and region->info is used.

Alan

>         region = fpga_region_get(region);
>         if (IS_ERR(region)) {
>                 dev_err(dev, "failed to get FPGA region\n");
> @@ -245,7 +250,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>                 goto err_put_region;
>         }
>
> -       ret = fpga_region_get_bridges(region, overlay);
> +       ret = fpga_region_get_bridges(region, info->overlay);
>         if (ret) {
>                 dev_err(dev, "failed to get FPGA bridges\n");
>                 goto err_unlock_mgr;
> @@ -257,7 +262,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>                 goto err_put_br;
>         }
>
> -       ret = fpga_mgr_load(region->mgr, region->info);
> +       ret = fpga_mgr_load(region->mgr, info);
>         if (ret) {
>                 dev_err(dev, "failed to load FPGA image\n");
>                 goto err_put_br;
> @@ -269,6 +274,8 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>                 goto err_put_br;
>         }
>
> +       region->info = info;
> +
>         fpga_mgr_unlock(region->mgr);
>         fpga_region_put(region);
>
> @@ -373,6 +380,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
>         if (!info)
>                 return -ENOMEM;
>
> +       info->overlay = nd->overlay;
> +
>         /* Read FPGA region properties from the overlay */
>         if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
>                 info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
> @@ -420,12 +429,9 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
>                 return -EINVAL;
>         }
>
> -       region->info = info;
> -       ret = fpga_region_program_fpga(region, nd->overlay);
> -       if (ret) {
> +       ret = fpga_region_program_fpga(region, info);
> +       if (ret)
>                 fpga_image_info_free(&region->dev, info);
> -               region->info = NULL;
> -       }
>
>         return ret;
>  }
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 4e14bdb..66d0e32 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -84,6 +84,7 @@ enum fpga_mgr_states {
>   * @sgt: scatter/gather table containing FPGA image
>   * @buf: contiguous buffer containing FPGA image
>   * @count: size of buf
> + * @overlay: Device Tree overlay
>   */
>  struct fpga_image_info {
>         u32 flags;
> @@ -94,6 +95,9 @@ struct fpga_image_info {
>         struct sg_table *sgt;
>         const char *buf;
>         size_t count;
> +#ifdef CONFIG_OF
> +       struct device_node *overlay;
> +#endif
>  };
>
>  /**
> --
> 2.7.4
>

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

end of thread, other threads:[~2017-05-09 15:25 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-20 14:09 [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull
2017-04-20 14:09 ` [PATCH v2 01/16] doc: fpga: update documents for the FPGA API Alan Tull
2017-04-20 14:09 ` [PATCH v2 02/16] fpga: bridge: support getting bridge from device Alan Tull
2017-05-03 11:58   ` Wu Hao
2017-05-03 20:07     ` Alan Tull
2017-05-04  9:20       ` Wu Hao
2017-05-04 21:31       ` Alan Tull
2017-05-08  8:44         ` Wu, Hao
2017-05-08 20:44           ` Alan Tull
2017-05-08 20:52             ` Moritz Fischer
2017-05-08 21:02               ` Alan Tull
2017-05-08 21:11                 ` Moritz Fischer
2017-05-08 21:20                   ` Alan Tull
2017-05-08 21:55                     ` Moritz Fischer
2017-05-09  7:16             ` Wu Hao
2017-04-20 14:09 ` [PATCH v2 03/16] fpga: mgr: API change to replace fpga load functions with single function Alan Tull
2017-04-21 20:08   ` Alan Tull
2017-04-20 14:09 ` [PATCH v2 04/16] fpga: mgr: separate getting/locking FPGA manager Alan Tull
2017-04-20 14:09 ` [PATCH v2 05/16] fpga: region: use dev_err instead of pr_err Alan Tull
2017-04-20 14:09 ` [PATCH v2 06/16] fpga: region: remove unneeded of_node_get and put Alan Tull
2017-05-05 17:08   ` Moritz Fischer
2017-04-20 14:09 ` [PATCH v2 07/16] fpga: region: get mgr early on Alan Tull
2017-04-20 14:09 ` [PATCH v2 08/16] fpga: region: check for child regions before allocing image info Alan Tull
2017-05-05 20:59   ` Moritz Fischer
2017-05-08 21:03     ` Alan Tull
2017-04-20 14:09 ` [PATCH v2 09/16] fpga: region: fix slow warning with more than one overlay Alan Tull
2017-04-20 14:09 ` [PATCH v2 10/16] fpga: region: use image info as parameter for programming region Alan Tull
2017-05-09 15:25   ` Alan Tull
2017-04-20 14:09 ` [PATCH v2 11/16] fpga: region: separate out code that parses the overlay Alan Tull
2017-04-20 14:09 ` [PATCH v2 12/16] fpga: region: add fpga-region.h header Alan Tull
2017-04-20 14:09 ` [PATCH v2 13/16] fpga: region: rename some functions prior to moving Alan Tull
2017-04-20 14:09 ` [PATCH v2 14/16] fpga: region: add register/unregister functions Alan Tull
2017-04-20 14:10 ` [PATCH v2 15/16] fpga: region: add fpga_region_class_find Alan Tull
2017-05-03 15:44   ` Moritz Fischer
2017-05-03 20:08     ` Alan Tull
2017-04-20 14:10 ` [PATCH v2 16/16] fpga: region: move device tree support to of-fpga-region.c Alan Tull
2017-04-28  6:38   ` Wu Hao
2017-04-28 17:37     ` Alan Tull
2017-04-20 14:16 ` [PATCH v2 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).