[PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus

[PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus

[atull]

From: Alan Tull <atull@opensource.altera.com>

This patchset adds two chunks plus documentation:
 * fpga manager core: exports ABI functions that write an image to a FPGA
 * DT Overlay support: simple-fpga-bus to handle FPGA from a DT overlay

The core's ABI is minimal to start with: only 6 functions.  This gives a
common interface for programming various FPGA such that any higher level
interfaces such as the DT Overlays or anything else that is added can be
shared and not be manufacturor-specific.

The DT Overlays support exists for the usage where the FPGA will contain
some "hardware" that will need drivers.  Where that use model is not
appealing, the core ABI can be used to add a different use model such as
using an FPGA as acceleration as has been discussed.

This patchset gets rid of the sysfs controls that allowed direct
control of a FPGA from userspace.

This patchset is under drivers/staging as the interface could change.

The bindings for the socpfga fpga manager already are upstreamed as
1b4e119 Alan Tull : doc: add bindings document for altera fpga manager

The DT Support is dependent on Pantelis's dtc overlay patches from
https://github.com/pantoniou/dtc.git
and his DT overlays configfs interface patches and fixes from
https://github.com/pantoniou/linux-beagle-track-mainline

efb0c04 Pantelis Antoniou : gcl: Fix resource linking
85e785e Pantelis Antoniou : ARM: DT: Enable symbols when CONFIG_OF_OVERLAY is used
af0321f Pantelis Antoniou : OF: DT-Overlay configfs interface (v5)
4c1c675 Pantelis Antoniou : configfs: Implement binary attributes (v4)


Alan Tull (7):
  staging: usage documentation for FPGA manager core
  staging: usage documentation for simple fpga bus
  staging: add bindings document for simple fpga bus
  staging: fpga manager: add sysfs interface document
  staging: fpga manager core
  staging: add simple-fpga-bus
  staging: fpga manager: add driver for socfpga fpga manager

 drivers/staging/Kconfig                            |    2 +
 drivers/staging/Makefile                           |    1 +
 .../Documentation/ABI/sysfs-class-fpga-manager     |   26 +
 .../Documentation/bindings/simple-fpga-bus.txt     |   61 ++
 drivers/staging/fpga/Documentation/fpga-mgr.txt    |  117 ++++
 .../staging/fpga/Documentation/simple-fpga-bus.txt |   48 ++
 drivers/staging/fpga/Kconfig                       |   31 +
 drivers/staging/fpga/Makefile                      |   10 +
 drivers/staging/fpga/fpga-mgr.c                    |  373 ++++++++++++
 drivers/staging/fpga/simple-fpga-bus.c             |  323 ++++++++++
 drivers/staging/fpga/socfpga.c                     |  616 ++++++++++++++++++++
 include/linux/fpga/fpga-mgr.h                      |  127 ++++
 12 files changed, 1735 insertions(+)
 create mode 100644 drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
 create mode 100644 drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
 create mode 100644 drivers/staging/fpga/Documentation/fpga-mgr.txt
 create mode 100644 drivers/staging/fpga/Documentation/simple-fpga-bus.txt
 create mode 100644 drivers/staging/fpga/Kconfig
 create mode 100644 drivers/staging/fpga/Makefile
 create mode 100644 drivers/staging/fpga/fpga-mgr.c
 create mode 100644 drivers/staging/fpga/simple-fpga-bus.c
 create mode 100644 drivers/staging/fpga/socfpga.c
 create mode 100644 include/linux/fpga/fpga-mgr.h

-- 
1.7.9.5

[PATCH v9 1/7] staging: usage documentation for FPGA manager core

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add a document on the new FPGA manager core.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 drivers/staging/fpga/Documentation/fpga-mgr.txt |  117 +++++++++++++++++++++++
 1 file changed, 117 insertions(+)
 create mode 100644 drivers/staging/fpga/Documentation/fpga-mgr.txt

diff --git a/drivers/staging/fpga/Documentation/fpga-mgr.txt b/drivers/staging/fpga/Documentation/fpga-mgr.txt
new file mode 100644
index 0000000..b5b6ed4
--- /dev/null
+++ b/drivers/staging/fpga/Documentation/fpga-mgr.txt
@@ -0,0 +1,117 @@
+  FPGA Manager Core
+
+  Alan Tull 2015
+
+  Overview
+  --------
+The FPGA manager core exports a set of functions for programming an image to a
+FPGA.  All manufacturor specifics are hidden away in a low level driver.  The
+API is manufacturor agnostic.  Of course the FPGA image data itself is very
+manufacturor specific but for our purposes it's just data in a buffer or file
+or something.  The FPGA manager core won't parse it or know anything about it.
+
+
+  Files
+  -----
+drivers/staging/fpga/fpga-mgr.c
+include/linux/fpga/fpga-mgr.h
+
+
+  The API Functions
+  ----------------
+The API that is exported is currently 6 functions:
+
+   int fpga_mgr_buf_load(struct fpga_manager *mgr,
+                         u32 flags,
+                         const char *buf,
+                         size_t count);
+
+An FPGA image exists as a buffer in memory.  Load it into the FPGA.  The FPGA
+ends up in operating mode or return a negative error code.
+
+   int fpga_mgr_firmware_load(struct fpga_manager *mgr,
+                              u32 flags,
+                              const char *image_name);
+
+An FPGA image exists as a file that is on the firmware search path (see the
+firmware class documentation).  Load as above.
+
+   struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
+
+Given a DT node, get a reference to a fpga manager.
+
+   void fpga_mgr_put(struct fpga_manager *mgr);
+
+Release the reference to the fpga manager.
+
+   int fpga_mgr_register(struct device *dev,
+                         const char *name,
+                         const struct fpga_manager_ops *mops,
+                         void *priv);
+   void fpga_mgr_unregister(struct device *dev);
+
+Register/unregister the lower level device specific driver.
+
+
+  How To Write an Image Buffer to a supported FPGA
+  ------------------------------------------------
+/* device node that specifies the fpga manager to use */
+struct device_node *mgr_node;
+
+/* FPGA image is in this buffer.  count is size of buf. */
+char *buf;
+int count;
+int ret;
+
+struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
+ret = fpga_mgr_buf_load(mgr, flags, buf, count);
+fpga_mgr_put(mgr);
+
+
+  How To Write an Image File to a supported FPGA
+  ------------------------------------------------
+/* device node that specifies the fpga manager to use */
+struct device_node *mgr_node;
+
+/* FPGA image is in this buffer.  count is size of buf. */
+const char *path = "fpga-image-9.rbf"
+int ret;
+
+struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
+ret = fpga_mgr_firmware_load(mgr, flags, path);
+fpga_mgr_put(mgr);
+
+
+  How To Support a new FPGA device
+  --------------------------------
+To add another fpga manager, look at the bottom part of socfpga.c for an
+example, starting with the declaration of socfpga_fpga_ops.
+
+static const struct fpga_manager_ops socfpga_fpga_ops = {
+       .write_init = socfpga_fpga_ops_configure_init,
+       .write = socfpga_fpga_ops_configure_write,
+       .write_complete = socfpga_fpga_ops_configure_complete,
+       .state = socfpga_fpga_ops_state,
+};
+
+You will want to create a platform driver that has a set of ops like that
+and then register it with fpga_mgr_register in your probe function.  Your
+ops will implement whatever device specific register writes needed and
+will return negative error codes if things don't go well.
+
+The programming seqence is:
+ 1. .write_init
+ 2. .write (may be called once or multiple times)
+ 3. .write_complete
+
+The .write_init function will prepare the FPGA to receive the image data.
+
+The .write function receives an image buffer or a chunk of the image and
+writes it the FPGA.  The buffer may arrive as one chunk or a bunck of
+small chunks through this function being called multiple times.
+
+The .write_complete function is called after all the image has been written
+to put the FPGA into operating mode.
+
+The .state function will read your hardware and return a code of type
+"enum fpga_mgr_states".  It doesn't result in a change in hardware state.
-- 
1.7.9.5

Re: [PATCH v9 1/7] staging: usage documentation for FPGA manager core

[Pavel Machek]

On Fri 2015-07-17 10:51:11, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add a document on the new FPGA manager core.
> 

> --- /dev/null
> +++ b/drivers/staging/fpga/Documentation/fpga-mgr.txt
> @@ -0,0 +1,117 @@
> +  FPGA Manager Core
> +
> +  Alan Tull 2015
> +
> +  Overview
> +  --------

The formatting is quite funny here. Add a newline after ---'s? Or
better format it the way other documents are formatted?

> +The FPGA manager core exports a set of functions for programming an image to a

"into"?

> +FPGA.  All manufacturor specifics are hidden away in a low level driver.  The

manufacturer (more then one instance).

> +API is manufacturor agnostic.  Of course the FPGA image data itself is very
> +manufacturor specific but for our purposes it's just data in a buffer or file

, but

> +or something.  The FPGA manager core won't parse it or know anything about it.

kill "or know anything"?

> +  Files
> +  -----
> +drivers/staging/fpga/fpga-mgr.c
> +include/linux/fpga/fpga-mgr.h
> +

Kill this section, as it is going to change?

> +  The API Functions
> +  ----------------
> +The API that is exported is currently 6 functions:
> +
> +   int fpga_mgr_buf_load(struct fpga_manager *mgr,
> +                         u32 flags,
> +                         const char *buf,
> +                         size_t count);
> +
> +An FPGA image exists as a buffer in memory.  Load it into the FPGA.  The FPGA
> +ends up in operating mode or return a negative error code.

So 0 on success?

> +   int fpga_mgr_firmware_load(struct fpga_manager *mgr,
> +                              u32 flags,
> +                              const char *image_name);
> +
> +An FPGA image exists as a file that is on the firmware search path (see the

", that is in"

> +firmware class documentation).  Load as above.
> +
> +   struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
> +
> +Given a DT node, get a reference to a fpga manager.

fpga->FPGA, fix globally??

> +   void fpga_mgr_put(struct fpga_manager *mgr);
> +
> +Release the reference to the fpga manager.
> +
> +   int fpga_mgr_register(struct device *dev,
> +                         const char *name,
> +                         const struct fpga_manager_ops *mops,
> +                         void *priv);
> +   void fpga_mgr_unregister(struct device *dev);
> +
> +Register/unregister the lower level device specific driver.

"low level".. And "device specific" -> "FPGA-specific" ?


> +To add another fpga manager, look at the bottom part of socfpga.c for an
> +example, starting with the declaration of socfpga_fpga_ops.

Umm. You have good documentation below. Maybe you don't need to send
people to read sources...?

> +static const struct fpga_manager_ops socfpga_fpga_ops = {
> +       .write_init = socfpga_fpga_ops_configure_init,
> +       .write = socfpga_fpga_ops_configure_write,
> +       .write_complete = socfpga_fpga_ops_configure_complete,
> +       .state = socfpga_fpga_ops_state,
> +};
> +
> +You will want to create a platform driver that has a set of ops like that
> +and then register it with fpga_mgr_register in your probe function.  Your
> +ops will implement whatever device specific register writes needed and
> +will return negative error codes if things don't go well.
> +
> +The programming seqence is:

sequence.

> + 1. .write_init
> + 2. .write (may be called once or multiple times)
> + 3. .write_complete
> +
> +The .write_init function will prepare the FPGA to receive the image data.
> +
> +The .write function receives an image buffer or a chunk of the image and
> +writes it the FPGA.  The buffer may arrive as one chunk or a bunck
> of

bunch.

> +small chunks through this function being called multiple times.
> +
> +The .write_complete function is called after all the image has been written
> +to put the FPGA into operating mode.
> +
> +The .state function will read your hardware and return a code of type
> +"enum fpga_mgr_states".  It doesn't result in a change in hardware state.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH v9 2/7] staging: usage documentation for simple fpga bus

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add a document spelling out usage of the simple fpga bus.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 .../staging/fpga/Documentation/simple-fpga-bus.txt |   48 ++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 drivers/staging/fpga/Documentation/simple-fpga-bus.txt

diff --git a/drivers/staging/fpga/Documentation/simple-fpga-bus.txt b/drivers/staging/fpga/Documentation/simple-fpga-bus.txt
new file mode 100644
index 0000000..9df519f
--- /dev/null
+++ b/drivers/staging/fpga/Documentation/simple-fpga-bus.txt
@@ -0,0 +1,48 @@
+   Simple FPGA Bus
+
+   Alan Tull 2015
+
+   Overview
+   --------
+The simple FPGA bus adds device tree overlay support for FPGA's.  Loading a
+DT overlay will result in the FGPA getting an image loaded, its bridges will
+be released, and the DT populated for nodes below the simple-fpga-bus.  This
+results in drivers getting probed for the hardware that just got added.  This
+is intended to support the FPGA usage where the FPGA has hardware that
+requires drivers.  Removing the overlay will result in the drivers getting
+removed and the bridges being disabled.
+
+
+   Requirements
+   ------------
+ 1. An FPGA image that has a hardware block or blocks that use drivers that are
+    supported in the kernel.
+ 2. A device tree overlay.
+ 3. A FPGA manager driver supporting writing the FPGA.
+ 4. FPGA bridge resets supported as reset controllers.
+
+The DT overlay includes bindings (documented in bindings/simple-fpga-bus.txt)
+that specify:
+ * Which fpga manager to use
+ * Which image file to load
+ * Flags indicating whether this this image is for full reconfiguration or
+   partial.
+ * a list of resets that should be released.  These enable the FPGA bridges.
+ * child nodes specifying the devices that will be added with appropriate
+   compatible strings, etc.
+
+Since this code uses the firmware interface to get the image and DT overlay,
+they currently have to be files on the filesystem.  It doesn't have to be that
+way forever as DT bindings could be added to point to other sources for the
+image.
+
+
+   Sequence
+   --------
+ 1. Load the DT overlay.  One convenient way to do that is to use Pantelis'
+    handy configfs interface (more below).
+ 2. The simple FPGA bus gets probed and will do the following:
+    a. call the fpga manager core to program the FPGA
+    b. release the FPGA bridges
+    c. call of_platform_populate resulting in device drivers getting probed.
+
-- 
1.7.9.5

Re: [PATCH v9 2/7] staging: usage documentation for simple fpga bus

[Pavel Machek]

On Fri 2015-07-17 10:51:12, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add a document spelling out usage of the simple fpga bus.

> +The DT overlay includes bindings (documented in bindings/simple-fpga-bus.txt)
> +that specify:
> + * Which fpga manager to use

fpga->FPGA, globally.

> + * Which image file to load
> + * Flags indicating whether this this image is for full reconfiguration or
> +   partial.
> + * a list of resets that should be released.  These enable the FPGA bridges.
> + * child nodes specifying the devices that will be added with appropriate
> +   compatible strings, etc.

Either all entries in the list should start with big letter or none
should. Also . at end of line should be consistent.

> +   Sequence
> +   --------
> + 1. Load the DT overlay.  One convenient way to do that is to use Pantelis'
> +    handy configfs interface (more below).

Reader has no chance to know what Pantelis' configfs interface is, and
there's nothing below.

> + 2. The simple FPGA bus gets probed and will do the following:
> +    a. call the fpga manager core to program the FPGA
> +    b. release the FPGA bridges
> +    c. call of_platform_populate resulting in device drivers getting probed.
> +

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH v9 3/7] staging: add bindings document for simple fpga bus

[atull]

From: Alan Tull <atull@opensource.altera.com>

New bindings document for simple fpga bus.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 .../Documentation/bindings/simple-fpga-bus.txt     |   61 ++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt

diff --git a/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt b/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
new file mode 100644
index 0000000..221e781
--- /dev/null
+++ b/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
@@ -0,0 +1,61 @@
+Simple FPGA Bus
+===============
+
+A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
+before populating the devices below its node.
+
+Required properties:
+- compatible : should contain "simple-fpga-bus"
+- #address-cells, #size-cells, ranges: must be present to handle address space
+  mapping for children.
+
+Optional properties:
+- fpga-mgr : should contain a phandle to a fpga manager.
+- fpga-firmware : should contain the name of a fpga image file located on the
+  firmware search path.
+- partial-reconfig : boolean property should be defined if partial
+  reconfiguration is to be done.
+- resets : should contain a list of resets that should be released after the
+  fpga has been programmed i.e. fpga bridges.
+- reset-names : should contain a list of the names of the resets.
+
+Example:
+
+/dts-v1/;
+/plugin/;
+/ {
+	fragment@0 {
+		target-path="/soc";
+		__overlay__ {
+			#address-cells = <1>;
+	                #size-cells = <1>;
+
+			bridge@0xff200000 {
+				compatible = "simple-fpga-bus";
+				#address-cells = <0x2>;
+				#size-cells = <0x1>;
+				ranges = <0x1 0x10040 0xff210040 0x20>;
+
+				clocks = <0x2 0x2>;
+				clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
+
+				fpga-mgr = <&hps_0_fpgamgr>;
+				fpga-firmware = "soc_system.rbf";
+
+				resets = <&hps_fpgabridge0 0>, <&hps_fpgabridge1 0>, <&hps_fpgabridge2 0>;
+				reset-names = "hps2fpga", "lwhps2fpga", "fpga2hps";
+
+				gpio@0x100010040 {
+					compatible = "altr,pio-14.0", "altr,pio-1.0";
+					reg = <0x1 0x10040 0x20>;
+					clocks = <0x2>;
+					altr,gpio-bank-width = <0x4>;
+					resetvalue = <0x0>;
+					#gpio-cells = <0x2>;
+					gpio-controller;
+				};
+			};
+		};
+	};
+};
+
-- 
1.7.9.5

Re: [PATCH v9 3/7] staging: add bindings document for simple fpga bus

[Steffen Trumtrar]

Hi!

On Fri, Jul 17, 2015 at 10:51:13AM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> New bindings document for simple fpga bus.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
>  .../Documentation/bindings/simple-fpga-bus.txt     |   61 ++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
> 
> diff --git a/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt b/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
> new file mode 100644
> index 0000000..221e781
> --- /dev/null
> +++ b/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
> @@ -0,0 +1,61 @@
> +Simple FPGA Bus
> +===============
> +
> +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> +before populating the devices below its node.
> +
> +Required properties:
> +- compatible : should contain "simple-fpga-bus"
> +- #address-cells, #size-cells, ranges: must be present to handle address space
> +  mapping for children.
> +
> +Optional properties:
> +- fpga-mgr : should contain a phandle to a fpga manager.
> +- fpga-firmware : should contain the name of a fpga image file located on the
> +  firmware search path.
> +- partial-reconfig : boolean property should be defined if partial
> +  reconfiguration is to be done.
> +- resets : should contain a list of resets that should be released after the
> +  fpga has been programmed i.e. fpga bridges.
> +- reset-names : should contain a list of the names of the resets.
> +
> +Example:
> +
> +/dts-v1/;
> +/plugin/;
> +/ {
> +	fragment@0 {
> +		target-path="/soc";
> +		__overlay__ {
> +			#address-cells = <1>;
> +	                #size-cells = <1>;
> +
> +			bridge@0xff200000 {
> +				compatible = "simple-fpga-bus";
> +				#address-cells = <0x2>;
> +				#size-cells = <0x1>;
> +				ranges = <0x1 0x10040 0xff210040 0x20>;
> +
> +				clocks = <0x2 0x2>;
> +				clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
> +
> +				fpga-mgr = <&hps_0_fpgamgr>;
> +				fpga-firmware = "soc_system.rbf";
> +
> +				resets = <&hps_fpgabridge0 0>, <&hps_fpgabridge1 0>, <&hps_fpgabridge2 0>;
> +				reset-names = "hps2fpga", "lwhps2fpga", "fpga2hps";
> +
> +				gpio@0x100010040 {
> +					compatible = "altr,pio-14.0", "altr,pio-1.0";
> +					reg = <0x1 0x10040 0x20>;
> +					clocks = <0x2>;
> +					altr,gpio-bank-width = <0x4>;
> +					resetvalue = <0x0>;
> +					#gpio-cells = <0x2>;
> +					gpio-controller;
> +				};
> +			};
> +		};
> +	};
> +};
> +

Just some quick thougths for the Socfpga case:

What you are describing here is a virtual bus, that is not existing on
at least the Socfpga, right? I don't like this.
You are mixing different independent busses/devices into one and I don't
see why. I see that this is just an example, but why
  a) isn't the fpga-mgr the one knowing about the bitstream ?
     You can't have two of these busses with different bitstreams anyway
     And if you have multipe FPGAs you have multiple fpga-mgrs, no?
  b) shouldn't the h2f/lwh2f/f2h bridges be the "simple-bus devices" instead of
     just reset-controllers ? What about e.g. the bus width of the bridges?
     It can change depending on the bitstream. When I have an IP core that does
     DMA I might want my driver to be able to configure the bus width accordingly.
     There are other settings in the bridges that I can not set when they are just
     reset controllers.

I can understand that this is just an example, but again for the Socfpga case it
is IMHO wrong. I don't know about e.g. the Zynq without researching, though.

Regards,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v9 3/7] staging: add bindings document for simple fpga bus

[Jason Gunthorpe]

On Fri, Jul 17, 2015 at 09:49:13PM +0200, Steffen Trumtrar wrote:
> What you are describing here is a virtual bus, that is not existing on
> at least the Socfpga, right?

I wouldn't get too hung up on bus or not bus, the HW these days
doesn't even use busses.

For AXI systems, which is all the ARM designs, the the bridge between
the CPU and FPGA is always a physical AXI link hanging off one of the
CPU block's internal AXI switches.

I choose to model these ports in DT explicitly, because they represent
swappable attachment points, and often the switch ports have
programmable registers related to that port.

FPGA logic is always hanging off one of these physical ports.

Usually there are multiple independent links between the CPU and the
FPGA (ie Xilinx Virtex 4 has at least 4 CPU to FPGA bus links, Zynq
has something like 7)

Ie on Zynq, I do things like this:

/ {
        /* This is a simplification of the 5 AXI interconnect switches
           hardwired inside the CPU block */
        ps7_axi_interconnect_0: amba@0 {
	        // This is the register block that programs the FPGA
                ps7-dev-cfg@f8007000 {
                        clock-names = "ref_clk", "fclk0", "fclk1", "fclk2", "fclk3";
                        clocks = <&clkc 12>, <&clkc 15>, <&clkc 16>, <&clkc 17>, <&clkc 18>;
                        compatible = "xlnx,zynq-devcfg-1.0";
                        interrupt-parent = <&ps7_scugic_0>;
                        interrupts = <0 8 4>;
                        reg = <0xf8007000 0x100>;
                };

        // This node bundles the entire FPGA side
        programmable: fpga@pl {
	        // This is a physical port on one of the CPU core's AXI swithces
                gp0_axi: axi@40000000 {
                        k_gpio@80000 {
                        k_sysmon@81000 {
                        gpio3: klina_gpio@80010 {
                        i2c_qsfp1 {
		}

	        // This is another physical port on one of the CPU core's AXI swithces
                gp1_axi: axi@80000000 {
		}

                // The FPGA bitstream also hooks up to a 3rd AXI port for initiating DMA
                // hp0_axi ...
        }
}

Zynq has 5 internal AXI switches, but the typical Linux DT models them
all as single DT 'bus'

I've brought the FPGA exposed AXI ports out under the programmable
node, purely for convenience of coding the dynamic load/unload of all
the FPGA elements. We do full hot swap of the FPGA during system
operation.

The physical ports could be located someplace within the amba@0, but
since amba@0 is basically already a lie, I don't really mind this
slight divergence as it makes logical sense and life easier.

Usually what my FPGA designs do is take the AXI port(s) and then fan
out to something else, either more AXI ports through yet another AXI
switch, or convert to a low speed multi-drop bus and fan that out to a
number of devices. I don't usually model this, because it provide no
value to Linux to know these details.

Ultimately the gp0_axi/gp1_axi have a number of peripheral childern,
each with their own drivers, interrupts, etc.

In this particular design gp1_axi bridges to a FPGA soft-core which
provides a physical bus to another FPGA, which is represented as more
nested nodes, and another FPGA programmable node.

DMA for the brdige side flows through the hp0_axi port. (it consumes 3
AXI ports IIRC)

I think the simplified DT modeling is a reasonable compromise.

>   b) shouldn't the h2f/lwh2f/f2h bridges be the "simple-bus devices" instead of
>      just reset-controllers ? What about e.g. the bus width of the
>      bridges?

On Zynq there are a variety of resets and clocks going from the CPU
block to the FPGA, they all need to be configured properly to run
correctly, and they need a home. The control registers for these are
located someplace under amba@0, but I'd be happy to see the actual
values to program contained under the programming node. That fits much
better with the overlay scheme.

There is also some AXI port-specific registers that may need tuning as
well, but they can naturally fit under the axi port nodes..

>      It can change depending on the bitstream. When I have an IP core that does
>      DMA I might want my driver to be able to configure the bus width accordingly.
>      There are other settings in the bridges that I can not set when they are just
>      reset controllers.

bridge@0xff200000 should be the bus port and it can have configuration...

AXI negotiates things like link width dynamically, and for AXI, DMA
doesn't flow through the same AXI port as the control registers
anyhow.

DT is a very poor fit for a modeling a modern AXI interconnect system,
so there is often some irrelevant lossage..

Jason

Re: [PATCH v9 3/7] staging: add bindings document for simple fpga bus

[atull]

On Fri, 17 Jul 2015, Steffen Trumtrar wrote:

> Hi!
> 
> On Fri, Jul 17, 2015 at 10:51:13AM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > New bindings document for simple fpga bus.
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > ---
> >  .../Documentation/bindings/simple-fpga-bus.txt     |   61 ++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >  create mode 100644 drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
> > 
> > diff --git a/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt b/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
> > new file mode 100644
> > index 0000000..221e781
> > --- /dev/null
> > +++ b/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
> > @@ -0,0 +1,61 @@
> > +Simple FPGA Bus
> > +===============
> > +
> > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> > +before populating the devices below its node.
> > +
> > +Required properties:
> > +- compatible : should contain "simple-fpga-bus"
> > +- #address-cells, #size-cells, ranges: must be present to handle address space
> > +  mapping for children.
> > +
> > +Optional properties:
> > +- fpga-mgr : should contain a phandle to a fpga manager.
> > +- fpga-firmware : should contain the name of a fpga image file located on the
> > +  firmware search path.
> > +- partial-reconfig : boolean property should be defined if partial
> > +  reconfiguration is to be done.
> > +- resets : should contain a list of resets that should be released after the
> > +  fpga has been programmed i.e. fpga bridges.
> > +- reset-names : should contain a list of the names of the resets.
> > +
> > +Example:
> > +
> > +/dts-v1/;
> > +/plugin/;
> > +/ {
> > +	fragment@0 {
> > +		target-path="/soc";
> > +		__overlay__ {
> > +			#address-cells = <1>;
> > +	                #size-cells = <1>;
> > +
> > +			bridge@0xff200000 {
> > +				compatible = "simple-fpga-bus";
> > +				#address-cells = <0x2>;
> > +				#size-cells = <0x1>;
> > +				ranges = <0x1 0x10040 0xff210040 0x20>;
> > +
> > +				clocks = <0x2 0x2>;
> > +				clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
> > +
> > +				fpga-mgr = <&hps_0_fpgamgr>;
> > +				fpga-firmware = "soc_system.rbf";
> > +
> > +				resets = <&hps_fpgabridge0 0>, <&hps_fpgabridge1 0>, <&hps_fpgabridge2 0>;
> > +				reset-names = "hps2fpga", "lwhps2fpga", "fpga2hps";
> > +
> > +				gpio@0x100010040 {
> > +					compatible = "altr,pio-14.0", "altr,pio-1.0";
> > +					reg = <0x1 0x10040 0x20>;
> > +					clocks = <0x2>;
> > +					altr,gpio-bank-width = <0x4>;
> > +					resetvalue = <0x0>;
> > +					#gpio-cells = <0x2>;
> > +					gpio-controller;
> > +				};
> > +			};
> > +		};
> > +	};
> > +};
> > +
> 
> Just some quick thougths for the Socfpga case:
> 
> What you are describing here is a virtual bus, that is not existing on
> at least the Socfpga, right? I don't like this.
> You are mixing different independent busses/devices into one and I don't
> see why.

Hi Steffen,

It is a multi-interface bridge.  It is likely that a device will use more than
one at a time.  This is normal in the kernel and the device tree supports it.
My example is too simplistic.  It is common for a device to use the lwh2f
bridge for register access and the h2f bridge for data.  So you'd have

   reg = <0xc0000000 0x20000000>,
         <0xff200000 0x00200000>;

and ranges that map from those areas.

> I see that this is just an example, but why
>   a) isn't the fpga-mgr the one knowing about the bitstream ?

The fpga-mgr doesn't know much.  Someone has to tell it what to load.  I think
that is a good thing.

>      You can't have two of these busses with different bitstreams anyway
>      And if you have multipe FPGAs you have multiple fpga-mgrs, no?

You would have one fpga-mgr and a separate set of bridges per fpga.

>   b) shouldn't the h2f/lwh2f/f2h bridges be the "simple-bus devices" instead of
>      just reset-controllers ?

That's an artificial division of things.  These aren't really separate busses.
You would have a device that needs to be the child of two separate bridges then.

> What about e.g. the bus width of the bridges?
>      It can change depending on the bitstream. When I have an IP core that does
>      DMA I might want my driver to be able to configure the bus width accordingly.
>      There are other settings in the bridges that I can not set when they are just
>      reset controllers.

I was hoping to avoid adding another framework to the kernel.  Looks like a
bus framework will be necessary that can be controlled by the simple fpga bus.
I'd add simple fpga bus DT bindings like

    bridges = <&hps_fpgabridge0>, <&hps_fpgabridge1>;

The bindings for the bridges have already been proposed (you didn't like them)
and could be used in the overlay here to change bus width.

    https://lkml.org/lkml/2014/10/23/681

I'll revive those patches, but tear out the sysfs interface and just export
API's for enabling/disabling bridges that I could call from simple-fpga-bus.c

Alan

> 
> I can understand that this is just an example, but again for the Socfpga case it
> is IMHO wrong. I don't know about e.g. the Zynq without researching, though.
> 
> Regards,
> Steffen
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH v9 3/7] staging: add bindings document for simple fpga bus

[Steffen Trumtrar]

Hi!

On Fri, Jul 17, 2015 at 04:22:07PM -0500, atull wrote:
> On Fri, 17 Jul 2015, Steffen Trumtrar wrote:
> 
> > Hi!
> > 
> > On Fri, Jul 17, 2015 at 10:51:13AM -0500, atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
> > > From: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> > > 
> > > New bindings document for simple fpga bus.
> > > 
> > > Signed-off-by: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> > > ---
> > >  .../Documentation/bindings/simple-fpga-bus.txt     |   61 ++++++++++++++++++++
> > >  1 file changed, 61 insertions(+)
> > >  create mode 100644 drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
> > > 
> > > diff --git a/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt b/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
> > > new file mode 100644
> > > index 0000000..221e781
> > > --- /dev/null
> > > +++ b/drivers/staging/fpga/Documentation/bindings/simple-fpga-bus.txt
> > > @@ -0,0 +1,61 @@
> > > +Simple FPGA Bus
> > > +===============
> > > +
> > > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> > > +before populating the devices below its node.
> > > +
> > > +Required properties:
> > > +- compatible : should contain "simple-fpga-bus"
> > > +- #address-cells, #size-cells, ranges: must be present to handle address space
> > > +  mapping for children.
> > > +
> > > +Optional properties:
> > > +- fpga-mgr : should contain a phandle to a fpga manager.
> > > +- fpga-firmware : should contain the name of a fpga image file located on the
> > > +  firmware search path.
> > > +- partial-reconfig : boolean property should be defined if partial
> > > +  reconfiguration is to be done.
> > > +- resets : should contain a list of resets that should be released after the
> > > +  fpga has been programmed i.e. fpga bridges.
> > > +- reset-names : should contain a list of the names of the resets.
> > > +
> > > +Example:
> > > +
> > > +/dts-v1/;
> > > +/plugin/;
> > > +/ {
> > > +	fragment@0 {
> > > +		target-path="/soc";
> > > +		__overlay__ {
> > > +			#address-cells = <1>;
> > > +	                #size-cells = <1>;
> > > +
> > > +			bridge@0xff200000 {
> > > +				compatible = "simple-fpga-bus";
> > > +				#address-cells = <0x2>;
> > > +				#size-cells = <0x1>;
> > > +				ranges = <0x1 0x10040 0xff210040 0x20>;
> > > +
> > > +				clocks = <0x2 0x2>;
> > > +				clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
> > > +
> > > +				fpga-mgr = <&hps_0_fpgamgr>;
> > > +				fpga-firmware = "soc_system.rbf";
> > > +
> > > +				resets = <&hps_fpgabridge0 0>, <&hps_fpgabridge1 0>, <&hps_fpgabridge2 0>;
> > > +				reset-names = "hps2fpga", "lwhps2fpga", "fpga2hps";
> > > +
> > > +				gpio@0x100010040 {
> > > +					compatible = "altr,pio-14.0", "altr,pio-1.0";
> > > +					reg = <0x1 0x10040 0x20>;
> > > +					clocks = <0x2>;
> > > +					altr,gpio-bank-width = <0x4>;
> > > +					resetvalue = <0x0>;
> > > +					#gpio-cells = <0x2>;
> > > +					gpio-controller;
> > > +				};
> > > +			};
> > > +		};
> > > +	};
> > > +};
> > > +
> > 
> > Just some quick thougths for the Socfpga case:
> > 
> > What you are describing here is a virtual bus, that is not existing on
> > at least the Socfpga, right? I don't like this.
> > You are mixing different independent busses/devices into one and I don't
> > see why.
> 

Just to be clear: all in all I like this approach and I would prefer it to
go in the mainline kernel sooner than later. The thing is AFAIK we still
don't have a way to mark DT bindings as staging or in flux even if we do
have a staging area for drivers. That's why I want to be convinced that this
binding is "correct" from a binding perspective and not some linux driver
implementation. It's much easier to change a driver than breaking old dts files
with binding changes.

> It is a multi-interface bridge.  It is likely that a device will use more than
> one at a time.  This is normal in the kernel and the device tree supports it.
> My example is too simplistic.  It is common for a device to use the lwh2f
> bridge for register access and the h2f bridge for data.  So you'd have
> 
>    reg = <0xc0000000 0x20000000>,
>          <0xff200000 0x00200000>;
> 
> and ranges that map from those areas.
> 

Yeah, sure. I used that, too. Where is the problem with

	&hps2fpga {
		status = "okay";
		my-ip-core {
			config = <&lwhps2fpga>;
			...
		};
	};

	&lwhps2fpga {
		status = "okay";
	};

The typical "let's use syscon" case, no?!

> > I see that this is just an example, but why
> >   a) isn't the fpga-mgr the one knowing about the bitstream ?
> 
> The fpga-mgr doesn't know much.  Someone has to tell it what to load.  I think
> that is a good thing.
> 
> >      You can't have two of these busses with different bitstreams anyway
> >      And if you have multipe FPGAs you have multiple fpga-mgrs, no?
> 
> You would have one fpga-mgr and a separate set of bridges per fpga.
> 

Is that because of the framework or the hardware implementation?
What do I do, if I have a Socfpga+Bitstream and a Stratix that I program
via SPI/PCIe/whatever? Don't I have two different Managers now, hardware-wise?
System is done, dts is fixed. I would expect to provide two bitstreams in the
chosen-node and let the FPGA managers grep theirs.
But I can see, that it is not unreasonable to have this information in the bridge,
to have it describe what devices hang from it.

> >   b) shouldn't the h2f/lwh2f/f2h bridges be the "simple-bus devices" instead of
> >      just reset-controllers ?
> 
> That's an artificial division of things.  These aren't really separate busses.
> You would have a device that needs to be the child of two separate bridges then.
> 

phandle? Not an uncommon pattern, is it?!
The thing is: What level of hardware does a binding actually describe?
When I look into the TRM of the Socfpga, I see two seperate devices for HPS2FPGA
bridge and LWHPS2FPGA bridge. Both have their own address space, one port is connected
to the L3 main switch the other to the L3 Slave peripheral switch, both have different
AXI clocks and different bus width. This doesn't look like they are artificially
divided, but like they actually are divided. The TRM is the only thing I can use as
reference as a normal developer.

> > What about e.g. the bus width of the bridges?
> >      It can change depending on the bitstream. When I have an IP core that does
> >      DMA I might want my driver to be able to configure the bus width accordingly.
> >      There are other settings in the bridges that I can not set when they are just
> >      reset controllers.
> 
> I was hoping to avoid adding another framework to the kernel.  Looks like a
> bus framework will be necessary that can be controlled by the simple fpga bus.
> I'd add simple fpga bus DT bindings like
> 
>     bridges = <&hps_fpgabridge0>, <&hps_fpgabridge1>;
> 
> The bindings for the bridges have already been proposed (you didn't like them)
> and could be used in the overlay here to change bus width.
> 
>     https://lkml.org/lkml/2014/10/23/681
>

Still don't, sorry. That binding looks like it is backwards: it looks like it
starts from the use-case in linux and describes everything you need for that.
Instead of the other way around.

> I'll revive those patches, but tear out the sysfs interface and just export
> API's for enabling/disabling bridges that I could call from simple-fpga-bus.c
> 

I also posted an RFC for the bridges

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310181.html

and no, I'm not trying to pimp my driver ;-)

Regards,
Steffen

PS: Thanks for still working on this and trying to get a mainlineable solution
instead of just dumping something into some vendor tree.

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 3/7] staging: add bindings document for simple fpga bus

[Pavel Machek]

> +Optional properties:
> +- fpga-mgr : should contain a phandle to a fpga manager.

fpga->FPGA, globally.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH v9 4/7] staging: fpga manager: add sysfs interface document

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add documentation under drivers/staging for new fpga manager's
sysfs interface.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v5  : (actually second version, but keeping version numbers
      aligned with rest of patch series)
      Move document to drivers/staging/fpga/Documentation/ABI

v6  : No change in this patch for v6 of the patch set

v7  : No change in this patch for v7 of the patch set

v8  : No change in this patch for v8 of the patch set

v9  : Remove 'firmware' and 'reset' files
      Update state strings
---
 .../Documentation/ABI/sysfs-class-fpga-manager     |   26 ++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager

diff --git a/drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager b/drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
new file mode 100644
index 0000000..470905e
--- /dev/null
+++ b/drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
@@ -0,0 +1,26 @@
+What:		/sys/class/fpga_manager/<fpga>/name
+Date:		July 2015
+KernelVersion:	4.2
+Contact:	Alan Tull <atull@opensource.altera.com>
+Description:	Name of low level fpga manager driver.
+
+What:		/sys/class/fpga_manager/<fpga>/state
+Date:		July 2015
+KernelVersion:	4.2
+Contact:	Alan Tull <atull@opensource.altera.com>
+Description:	Read fpga manager state as a string.
+		Valid states may vary by manufacturer; superset is:
+		* unknown		= can't determine state
+		* power off		= FPGA power is off
+		* power up		= FPGA reports power is up
+		* reset			= FPGA held in reset state
+		* firmware request	= firmware class request in progress
+		* firmware request error = firmware request failed
+		* write init		= FPGA being prepared for programming
+		* write init error	= Error while preparing FPGA for
+					  programming
+		* write			= FPGA ready to receive image data
+		* write error		= Error while programming
+		* write complete	= Doing post programming steps
+		* write complete error	= Error while doing post programming
+		* operating		= FPGA is programmed and operating
-- 
1.7.9.5

Re: [PATCH v9 4/7] staging: fpga manager: add sysfs interface document

[Pavel Machek]

Hi!

> +What:		/sys/class/fpga_manager/<fpga>/state
> +Date:		July 2015
> +KernelVersion:	4.2
> +Contact:	Alan Tull <atull@opensource.altera.com>
> +Description:	Read fpga manager state as a string.

fpga->FPGA.

> +		Valid states may vary by manufacturer; superset is:
> +		* unknown		= can't determine state
> +		* power off		= FPGA power is off
> +		* power up		= FPGA reports power is up
> +		* reset			= FPGA held in reset state
> +		* firmware request	= firmware class request in progress
> +		* firmware request error = firmware request failed
> +		* write init		= FPGA being prepared for programming
> +		* write init error	= Error while preparing FPGA for
> +					  programming
> +		* write			= FPGA ready to receive image data
> +		* write error		= Error while programming
> +		* write complete	= Doing post programming steps
> +		* write complete error	= Error while doing post programming
> +		* operating		= FPGA is programmed and operating

This will need some more details. "firmware request" is hardly a
hardware state, does it belong here? Is power off or on while firmware
is being requested? How does the fpga get into power up phase?
Normally, you'd only power it on to do something more with it...?

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v9 4/7] staging: fpga manager: add sysfs interface document

[atull]

On Fri, 24 Jul 2015, Pavel Machek wrote:

Hi Pavel,

Thanks for your your feedback in cleaning up these docs.

> Hi!
> 
> > +What:		/sys/class/fpga_manager/<fpga>/state
> > +Date:		July 2015
> > +KernelVersion:	4.2
> > +Contact:	Alan Tull <atull@opensource.altera.com>
> > +Description:	Read fpga manager state as a string.
> 
> fpga->FPGA.

Yep

> 
> > +		Valid states may vary by manufacturer; superset is:
> > +		* unknown		= can't determine state
> > +		* power off		= FPGA power is off
> > +		* power up		= FPGA reports power is up
> > +		* reset			= FPGA held in reset state
> > +		* firmware request	= firmware class request in progress
> > +		* firmware request error = firmware request failed
> > +		* write init		= FPGA being prepared for programming
> > +		* write init error	= Error while preparing FPGA for
> > +					  programming
> > +		* write			= FPGA ready to receive image data
> > +		* write error		= Error while programming
> > +		* write complete	= Doing post programming steps
> > +		* write complete error	= Error while doing post programming
> > +		* operating		= FPGA is programmed and operating
> 

If I can make my intent clear, maybe we can figure out what will be most
useful here.  

The intent is to provide enough detail that if something goes wrong with
the FPGA programming (something that the driver can't take care of) then 
userspace can know that.  Such as if the firmware request fails, that 
could be due to not being able to find the firmware file.

> This will need some more details. "firmware request" is hardly a
> hardware state, does it belong here? 

This is a superset of FPGA states and fpga manager driver states as the
fpga manager driver is walking through the steps to get the FPGA into
a known operating state.  So it's a sequence, though some steps may get
skipped. If there is an error, then userspace can know what step failed.

Maybe this should be separated into fpga_state for hardware state and
fpga_mgr_status (to report what step of progress the fpga manager driver
is at during programming).  I want this to be useful and still not be 
device (FPGA) specific.

> Is power off or on while firmware
> is being requested? 

On.  It's a sequence.

> How does the fpga get into power up phase?
> Normally, you'd only power it on to do something more with it...?
> 
> 									Pavel



> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 

Re: [PATCH v9 4/7] staging: fpga manager: add sysfs interface document

[Pavel Machek]

On Fri 2015-07-24 07:39:15, atull wrote:
> On Fri, 24 Jul 2015, Pavel Machek wrote:
> 
> Hi Pavel,
> 
> Thanks for your your feedback in cleaning up these docs.
> 
> > Hi!
> > 
> > > +What:		/sys/class/fpga_manager/<fpga>/state
> > > +Date:		July 2015
> > > +KernelVersion:	4.2
> > > +Contact:	Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> > > +Description:	Read fpga manager state as a string.
> > 
> > fpga->FPGA.
> 
> Yep
> 
> > 
> > > +		Valid states may vary by manufacturer; superset is:
> > > +		* unknown		= can't determine state
> > > +		* power off		= FPGA power is off
> > > +		* power up		= FPGA reports power is up
> > > +		* reset			= FPGA held in reset state
> > > +		* firmware request	= firmware class request in progress
> > > +		* firmware request error = firmware request failed
> > > +		* write init		= FPGA being prepared for programming
> > > +		* write init error	= Error while preparing FPGA for
> > > +					  programming
> > > +		* write			= FPGA ready to receive image data
> > > +		* write error		= Error while programming
> > > +		* write complete	= Doing post programming steps
> > > +		* write complete error	= Error while doing post programming
> > > +		* operating		= FPGA is programmed and operating
> > 
> 
> If I can make my intent clear, maybe we can figure out what will be most
> useful here.  
> 
> The intent is to provide enough detail that if something goes wrong with
> the FPGA programming (something that the driver can't take care of) then 
> userspace can know that.  Such as if the firmware request fails, that 
> could be due to not being able to find the firmware file.
> 
> > This will need some more details. "firmware request" is hardly a
> > hardware state, does it belong here? 
> 
> This is a superset of FPGA states and fpga manager driver states as the
> fpga manager driver is walking through the steps to get the FPGA into
> a known operating state.  So it's a sequence, though some steps may get
> skipped. If there is an error, then userspace can know what step failed.
> 
> Maybe this should be separated into fpga_state for hardware state and
> fpga_mgr_status (to report what step of progress the fpga manager driver
> is at during programming).  I want this to be useful and still not be 
> device (FPGA) specific.
> 
> > Is power off or on while firmware
> > is being requested? 
> 
> On.  It's a sequence.

Aha. Ok, so maybe noting that states normally go in the sequence (with
exception of various errors) would be enough?

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 5/7] staging: fpga manager core

[atull]

From: Alan Tull <atull@opensource.altera.com>

API to support programming FPGA.

The following functions are exported as GPL:
* fpga_mgr_buf_load
   Load fpga from image in buffer

* fpga_mgr_firmware_load
   Request firmware and load it to the FPGA.

* fpga_mgr_register
* fpga_mgr_unregister
   FPGA device drivers can be added by calling
   fpga_mgr_register() to register a set of
   fpga_manager_ops to do device specific stuff.

* of_fpga_mgr_get
* fpga_mgr_put
   Get/put a reference to a fpga manager.

The following sysfs files are created:
* /sys/class/fpga_manager/<fpga>/name
  Name of low level driver.

* /sys/class/fpga_manager/<fpga>/state
  State of fpga manager

Signed-off-by: Alan Tull <atull@opensource.altera.com>
Acked-by: Michal Simek <michal.simek@xilinx.com>
---
v2: s/mangager/manager/
    check for invalid request_nr
    add fpga reset interface
    rework the state code
    remove FPGA_MGR_FAIL flag
    add _ERR states to fpga manager states enum
    low level state op now returns a state enum value
    initialize framework state from driver state op
    remove unused fpga read stuff
    merge sysfs.c into fpga-mgr.c
    move suspend/resume from bus.c to fpga-mgr.c

v3: Add struct device to fpga_manager (not as a pointer)
    Add to_fpga_manager
    Don't get irq in fpga-mgr.c (let low level driver do it)
    remove from struct fpga_manager: nr, np, parent
    get rid of fpga_mgr_get_new_minor()
    simplify fpga_manager_register:
      reorder parameters
      use dev instead of pdev
    get rid of code that used to make more sense when this
      was a char driver, don't alloc_chrdev_region
    use a mutex instead of flags

v4: Move to drivers/staging

v5: Make some things be static
    Kconfig: add 'if FPGA'
    Makefile: s/fpga-mgr-core.o/fpga-mgr.o/
    clean up includes
    use enum fpga_mgr_states instead of int
    static const char *state_str
    use DEVICE_ATTR_RO/RW/WO and ATTRIBUTE_GROUPS
    return -EINVAL instead of BUG_ON
    move ida_simple_get after kzalloc
    clean up fpga_mgr_remove
    fpga-mgr.h: remove '#if IS_ENABLED(CONFIG_FPGA)'
    add kernel-doc documentation
    Move header to new include/linux/fpga folder
    static const struct fpga_mgr_ops
    dev_info msg simplified

v6: no statically allocated string for image_name
    kernel doc fixes
    changes regarding enabling SYSFS for fpga mgr
    Makefile cleanup

v7: no change in this patch for v7 of the patchset

v8: no change in this patch for v8 of the patchset

v9: remove writable attributes 'firmware' and 'reset'
    remove suspend/resume support for now
    remove list of fpga managers; use class device iterators instead
    simplify locking by giving out only one ref exclusively
    add device tree support
    add flags
    par down API into fewer functions
    update copyright year
    rename some functions for clarity
    clean up unnecessary #includes
    add some error messages
    write_init may need to look at buffer header, so add to params
---
 drivers/staging/Kconfig         |    2 +
 drivers/staging/Makefile        |    1 +
 drivers/staging/fpga/Kconfig    |   14 ++
 drivers/staging/fpga/Makefile   |    8 +
 drivers/staging/fpga/fpga-mgr.c |  373 +++++++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-mgr.h   |  127 +++++++++++++
 6 files changed, 525 insertions(+)
 create mode 100644 drivers/staging/fpga/Kconfig
 create mode 100644 drivers/staging/fpga/Makefile
 create mode 100644 drivers/staging/fpga/fpga-mgr.c
 create mode 100644 include/linux/fpga/fpga-mgr.h

diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
index 7f6cae5..89c089c 100644
--- a/drivers/staging/Kconfig
+++ b/drivers/staging/Kconfig
@@ -112,4 +112,6 @@ source "drivers/staging/fsl-mc/Kconfig"
 
 source "drivers/staging/wilc1000/Kconfig"
 
+source "drivers/staging/fpga/Kconfig"
+
 endif # STAGING
diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index 347f647..e129940 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -48,3 +48,4 @@ obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD)	+= clocking-wizard/
 obj-$(CONFIG_FB_TFT)		+= fbtft/
 obj-$(CONFIG_FSL_MC_BUS)	+= fsl-mc/
 obj-$(CONFIG_WILC1000)		+= wilc1000/
+obj-$(CONFIG_FPGA)		+= fpga/
diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
new file mode 100644
index 0000000..8254ca0
--- /dev/null
+++ b/drivers/staging/fpga/Kconfig
@@ -0,0 +1,14 @@
+#
+# FPGA framework configuration
+#
+
+menu "FPGA Configuration Support"
+
+config FPGA
+	bool "FPGA Configuration Framework"
+	help
+	  Say Y here if you want support for configuring FPGAs from the
+	  kernel.  The FPGA framework adds a FPGA manager class and FPGA
+	  manager drivers.
+
+endmenu
diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
new file mode 100644
index 0000000..3313c52
--- /dev/null
+++ b/drivers/staging/fpga/Makefile
@@ -0,0 +1,8 @@
+#
+# Makefile for the fpga framework and fpga manager drivers.
+#
+
+# Core FPGA Manager Framework
+obj-$(CONFIG_FPGA)			+= fpga-mgr.o
+
+# FPGA Manager Drivers
diff --git a/drivers/staging/fpga/fpga-mgr.c b/drivers/staging/fpga/fpga-mgr.c
new file mode 100644
index 0000000..1363f8f
--- /dev/null
+++ b/drivers/staging/fpga/fpga-mgr.c
@@ -0,0 +1,373 @@
+/*
+ * FPGA Manager Core
+ *
+ *  Copyright (C) 2013-2015 Altera Corporation
+ *
+ * With code from the mailing list:
+ * Copyright (C) 2013 Xilinx, Inc.
+ *
+ * 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/firmware.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+
+static DEFINE_IDA(fpga_mgr_ida);
+static struct class *fpga_mgr_class;
+
+/**
+ * fpga_mgr_buf_load - load fpga from image in buffer
+ * @mgr:	fpga manager
+ * @flags:	flags setting fpga confuration modes
+ * @buf:	buffer contain fpga image
+ * @count:	byte count of buf
+ *
+ * Step the low level fpga manager through the device-specific steps of getting
+ * an FPGA ready to be configured, writing the image to it, then doing whatever
+ * post-configuration steps necessary.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
+		      size_t count)
+{
+	struct device *dev = &mgr->dev;
+	int ret;
+
+	if (!mgr)
+		return -ENODEV;
+
+	/*
+	 * 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 receive an FPGA image.
+	 */
+	mgr->state = FPGA_MGR_STATE_WRITE_INIT;
+	ret = mgr->mops->write_init(mgr, flags, buf, count);
+	if (ret) {
+		dev_err(dev, "Error preparing FPGA for writing\n");
+		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
+		return ret;
+	}
+
+	/*
+	 * Write the FPGA image to the FPGA.
+	 */
+	mgr->state = FPGA_MGR_STATE_WRITE;
+	ret = mgr->mops->write(mgr, buf, count);
+	if (ret) {
+		dev_err(dev, "Error while writing image data to FPGA\n");
+		mgr->state = FPGA_MGR_STATE_WRITE_ERR;
+		return ret;
+	}
+
+	/*
+	 * After all the FPGA image has been written, do the device specific
+	 * steps to finish and set the FPGA into operating mode.
+	 */
+	mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
+	ret = mgr->mops->write_complete(mgr);
+	if (ret) {
+		dev_err(dev, "Error after writing image data to FPGA\n");
+		mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
+
+/**
+ * fpga_mgr_firmware_load - request firmware and load to fpga
+ * @mgr:	fpga manager
+ * @flags:	flags setting fpga confuration modes
+ * @image_name:	name of image file on the firmware search path
+ *
+ * Request an FPGA image using the firmware class, then write out to the FPGA.
+ * Update the state before each step to provide info on what step failed if
+ * there is a failure.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
+			   const char *image_name)
+{
+	struct device *dev = &mgr->dev;
+	const struct firmware *fw;
+	int ret;
+
+	if (!mgr)
+		return -ENODEV;
+
+	dev_info(dev, "writing %s to %s\n", image_name, mgr->name);
+
+	mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
+
+	ret = request_firmware(&fw, image_name, dev);
+	if (ret) {
+		mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
+		dev_err(dev, "Error requesting firmware %s\n", image_name);
+		return ret;
+	}
+
+	ret = fpga_mgr_buf_load(mgr, flags, fw->data, fw->size);
+	if (ret)
+		return ret;
+
+	release_firmware(fw);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
+
+static const char * const state_str[] = {
+	[FPGA_MGR_STATE_UNKNOWN] =		"unknown",
+	[FPGA_MGR_STATE_POWER_OFF] =		"power off",
+	[FPGA_MGR_STATE_POWER_UP] =		"power up",
+	[FPGA_MGR_STATE_RESET] =		"reset",
+
+	/* requesting FPGA image from firmware */
+	[FPGA_MGR_STATE_FIRMWARE_REQ] =		"firmware request",
+	[FPGA_MGR_STATE_FIRMWARE_REQ_ERR] =	"firmware request error",
+
+	/* Preparing FPGA to receive image */
+	[FPGA_MGR_STATE_WRITE_INIT] =		"write init",
+	[FPGA_MGR_STATE_WRITE_INIT_ERR] =	"write init error",
+
+	/* Writing image to FPGA */
+	[FPGA_MGR_STATE_WRITE] =		"write",
+	[FPGA_MGR_STATE_WRITE_ERR] =		"write error",
+
+	/* Finishing configuration after image has been written */
+	[FPGA_MGR_STATE_WRITE_COMPLETE] =	"write complete",
+	[FPGA_MGR_STATE_WRITE_COMPLETE_ERR] =	"write complete error",
+
+	/* FPGA reports to be in normal operating mode */
+	[FPGA_MGR_STATE_OPERATING] =		"operating",
+};
+
+static ssize_t name_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	return sprintf(buf, "%s\n", mgr->name);
+}
+
+static ssize_t state_show(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	return sprintf(buf, "%s\n", state_str[mgr->state]);
+}
+
+static DEVICE_ATTR_RO(name);
+static DEVICE_ATTR_RO(state);
+
+static struct attribute *fpga_mgr_attrs[] = {
+	&dev_attr_name.attr,
+	&dev_attr_state.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(fpga_mgr);
+
+static int fpga_mgr_of_node_match(struct device *dev, const void *data)
+{
+	return dev->of_node == data;
+}
+
+/**
+ * of_fpga_mgr_get - get an exclusive reference to a fpga mgr
+ * @node:	device node
+ *
+ * Given a device node, get an exclusive reference to a fpga mgr.
+ *
+ * Return: fpga manager struct or IS_ERR() condition containing error code.
+ */
+struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
+{
+	struct fpga_manager *mgr;
+	struct device *dev;
+
+	if (!node)
+		return ERR_PTR(-EINVAL);
+
+	dev = class_find_device(fpga_mgr_class, NULL, node,
+				fpga_mgr_of_node_match);
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	mgr = to_fpga_manager(dev);
+	put_device(dev);
+	if (!mgr)
+		return ERR_PTR(-ENODEV);
+
+	if (!mutex_trylock(&mgr->ref_mutex))
+		return ERR_PTR(-EBUSY);
+
+	return mgr;
+}
+EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
+
+/**
+ * fpga_mgr_put - release a reference to a fpga manager
+ * @mgr:	fpga manager structure
+ */
+void fpga_mgr_put(struct fpga_manager *mgr)
+{
+	if (mgr)
+		mutex_unlock(&mgr->ref_mutex);
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_put);
+
+/**
+ * fpga_mgr_register - register a low level fpga manager driver
+ * @dev:	fpga manager device from pdev
+ * @name:	fpga manager name
+ * @mops:	pointer to structure of fpga manager ops
+ * @priv:	fpga manager private data
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int fpga_mgr_register(struct device *dev, const char *name,
+		      const struct fpga_manager_ops *mops,
+		      void *priv)
+{
+	struct fpga_manager *mgr;
+	const char *dt_label;
+	int id, ret;
+
+	if (!mops || !mops->write_init || !mops->write ||
+	    !mops->write_complete || !mops->state) {
+		dev_err(dev, "Attempt to register without fpga_manager_ops\n");
+		return -EINVAL;
+	}
+
+	if (!name || !strlen(name)) {
+		dev_err(dev, "Attempt to register with no name!\n");
+		return -EINVAL;
+	}
+
+	mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
+	id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
+	if (id < 0) {
+		ret = id;
+		goto error_kfree;
+	}
+
+	mutex_init(&mgr->ref_mutex);
+
+	mgr->name = name;
+	mgr->mops = mops;
+	mgr->priv = priv;
+
+	/*
+	 * Initialize framework state by requesting low level driver read state
+	 * from device.  FPGA may be in reset mode or may have been programmed
+	 * by bootloader or EEPROM.
+	 */
+	mgr->state = mgr->mops->state(mgr);
+
+	device_initialize(&mgr->dev);
+	mgr->dev.class = fpga_mgr_class;
+	mgr->dev.parent = dev;
+	mgr->dev.of_node = dev->of_node;
+	mgr->dev.id = id;
+	dev_set_drvdata(dev, mgr);
+
+	dt_label = of_get_property(mgr->dev.of_node, "label", NULL);
+	if (dt_label)
+		ret = dev_set_name(&mgr->dev, "%s", dt_label);
+	else
+		ret = dev_set_name(&mgr->dev, "fpga%d", id);
+
+	ret = device_add(&mgr->dev);
+	if (ret)
+		goto error_device;
+
+	dev_info(&mgr->dev, "%s registered\n", mgr->name);
+
+	return 0;
+
+error_device:
+	ida_simple_remove(&fpga_mgr_ida, id);
+error_kfree:
+	kfree(mgr);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_register);
+
+/**
+ * fpga_mgr_unregister - unregister a low level fpga manager driver
+ * @dev:	fpga manager device from pdev
+ */
+void fpga_mgr_unregister(struct device *dev)
+{
+	struct fpga_manager *mgr = dev_get_drvdata(dev);
+
+	dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
+
+	/*
+	 * If the low level driver provides a method for putting fpga into
+	 * a desired state upon unregister, do it.
+	 */
+	if (mgr->mops->fpga_remove)
+		mgr->mops->fpga_remove(mgr);
+
+	device_unregister(&mgr->dev);
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
+
+static void fpga_mgr_dev_release(struct device *dev)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
+	kfree(mgr);
+}
+
+static int __init fpga_mgr_class_init(void)
+{
+	pr_info("FPGA manager framework\n");
+
+	fpga_mgr_class = class_create(THIS_MODULE, "fpga_manager");
+	if (IS_ERR(fpga_mgr_class))
+		return PTR_ERR(fpga_mgr_class);
+
+	fpga_mgr_class->dev_groups = fpga_mgr_groups;
+	fpga_mgr_class->dev_release = fpga_mgr_dev_release;
+
+	return 0;
+}
+
+static void __exit fpga_mgr_class_exit(void)
+{
+	class_destroy(fpga_mgr_class);
+	ida_destroy(&fpga_mgr_ida);
+}
+
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_DESCRIPTION("FPGA manager framework");
+MODULE_LICENSE("GPL v2");
+
+subsys_initcall(fpga_mgr_class_init);
+module_exit(fpga_mgr_class_exit);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
new file mode 100644
index 0000000..14a2ca6
--- /dev/null
+++ b/include/linux/fpga/fpga-mgr.h
@@ -0,0 +1,127 @@
+/*
+ * FPGA Framework
+ *
+ *  Copyright (C) 2013-2015 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+
+#ifndef _LINUX_FPGA_MGR_H
+#define _LINUX_FPGA_MGR_H
+
+struct fpga_manager;
+
+/**
+ * enum fpga_mgr_states - fpga framework states
+ * @FPGA_MGR_STATE_UNKNOWN: can't determine state
+ * @FPGA_MGR_STATE_POWER_OFF: FPGA power is off
+ * @FPGA_MGR_STATE_POWER_UP: FPGA reports power is up
+ * @FPGA_MGR_STATE_RESET: FPGA in reset state
+ * @FPGA_MGR_STATE_FIRMWARE_REQ: firmware request in progress
+ * @FPGA_MGR_STATE_FIRMWARE_REQ_ERR: firmware request failed
+ * @FPGA_MGR_STATE_WRITE_INIT: preparing FPGA for programming
+ * @FPGA_MGR_STATE_WRITE_INIT_ERR: Error during WRITE_INIT stage
+ * @FPGA_MGR_STATE_WRITE: writing image to FPGA
+ * @FPGA_MGR_STATE_WRITE_ERR: Error while writing FPGA
+ * @FPGA_MGR_STATE_WRITE_COMPLETE: Doing post programming steps
+ * @FPGA_MGR_STATE_WRITE_COMPLETE_ERR: Error during WRITE_COMPLETE
+ * @FPGA_MGR_STATE_OPERATING: FPGA is programmed and operating
+ */
+enum fpga_mgr_states {
+	/* default FPGA states */
+	FPGA_MGR_STATE_UNKNOWN,
+	FPGA_MGR_STATE_POWER_OFF,
+	FPGA_MGR_STATE_POWER_UP,
+	FPGA_MGR_STATE_RESET,
+
+	/* getting an image for loading */
+	FPGA_MGR_STATE_FIRMWARE_REQ,
+	FPGA_MGR_STATE_FIRMWARE_REQ_ERR,
+
+	/* write sequence: init, write, complete */
+	FPGA_MGR_STATE_WRITE_INIT,
+	FPGA_MGR_STATE_WRITE_INIT_ERR,
+	FPGA_MGR_STATE_WRITE,
+	FPGA_MGR_STATE_WRITE_ERR,
+	FPGA_MGR_STATE_WRITE_COMPLETE,
+	FPGA_MGR_STATE_WRITE_COMPLETE_ERR,
+
+	/* fpga is programmed and operating */
+	FPGA_MGR_STATE_OPERATING,
+};
+
+/*
+ * FPGA Manager flags
+ * FPGA_MGR_PARTIAL_RECONFIG: do partial reconfiguration if supported
+ */
+#define FPGA_MGR_PARTIAL_RECONFIG (1)
+
+/**
+ * struct fpga_manager_ops - ops for low level fpga manager drivers
+ * @state: returns an enum value of the FPGA's state
+ * @write_init: prepare the FPGA to receive confuration data
+ * @write: write count bytes of configuration data to the FPGA
+ * @write_complete: set FPGA to operating state after writing is done
+ * @fpga_remove: optional: Set FPGA into a specific state during driver remove
+ *
+ * fpga_manager_ops are the low level functions implemented by a specific
+ * fpga manager driver.  The optional ones are tested for NULL before being
+ * called, so leaving them out is fine.
+ */
+struct fpga_manager_ops {
+	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
+	int (*write_init)(struct fpga_manager *mgr, u32 flags,
+			  const char *buf, size_t count);
+	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
+	int (*write_complete)(struct fpga_manager *mgr);
+	void (*fpga_remove)(struct fpga_manager *mgr);
+};
+
+/**
+ * struct fpga_manager - fpga manager structure
+ * @name: name of low level fpga manager
+ * @dev: fpga manager device
+ * @ref_mutex: only allows one reference to fpga manager
+ * @state: state of fpga manager
+ * @mops: pointer to struct of fpga manager ops
+ * @priv: low level driver private date
+ */
+struct fpga_manager {
+	const char *name;
+	struct device dev;
+	struct mutex ref_mutex;
+	enum fpga_mgr_states state;
+	const struct fpga_manager_ops *mops;
+	void *priv;
+};
+
+#define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
+
+int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags,
+		      const char *buf, size_t count);
+
+int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
+			   const char *image_name);
+
+struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
+
+void fpga_mgr_put(struct fpga_manager *mgr);
+
+int fpga_mgr_register(struct device *dev, const char *name,
+		      const struct fpga_manager_ops *mops, void *priv);
+
+void fpga_mgr_unregister(struct device *dev);
+
+#endif /*_LINUX_FPGA_MGR_H */
-- 
1.7.9.5

Re: [PATCH v9 5/7] staging: fpga manager core

[Randy Dunlap]

On 07/17/15 08:51, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> ---
>  drivers/staging/Kconfig         |    2 +
>  drivers/staging/Makefile        |    1 +
>  drivers/staging/fpga/Kconfig    |   14 ++
>  drivers/staging/fpga/Makefile   |    8 +
>  drivers/staging/fpga/fpga-mgr.c |  373 +++++++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h   |  127 +++++++++++++
>  6 files changed, 525 insertions(+)
>  create mode 100644 drivers/staging/fpga/Kconfig
>  create mode 100644 drivers/staging/fpga/Makefile
>  create mode 100644 drivers/staging/fpga/fpga-mgr.c
>  create mode 100644 include/linux/fpga/fpga-mgr.h

> diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
> new file mode 100644
> index 0000000..8254ca0
> --- /dev/null
> +++ b/drivers/staging/fpga/Kconfig
> @@ -0,0 +1,14 @@
> +#
> +# FPGA framework configuration
> +#
> +
> +menu "FPGA Configuration Support"
> +
> +config FPGA
> +	bool "FPGA Configuration Framework"
> +	help
> +	  Say Y here if you want support for configuring FPGAs from the
> +	  kernel.  The FPGA framework adds a FPGA manager class and FPGA
> +	  manager drivers.
> +
> +endmenu

Is there some good reason why this is 'bool' instead of 'tristate'?
I.e., why can't it be built as a loadable module?

Thanks.

-- 
~Randy

Re: [PATCH v9 5/7] staging: fpga manager core

[atull]

On Fri, 17 Jul 2015, Randy Dunlap wrote:

> On 07/17/15 08:51, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > ---
> >  drivers/staging/Kconfig         |    2 +
> >  drivers/staging/Makefile        |    1 +
> >  drivers/staging/fpga/Kconfig    |   14 ++
> >  drivers/staging/fpga/Makefile   |    8 +
> >  drivers/staging/fpga/fpga-mgr.c |  373 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/fpga/fpga-mgr.h   |  127 +++++++++++++
> >  6 files changed, 525 insertions(+)
> >  create mode 100644 drivers/staging/fpga/Kconfig
> >  create mode 100644 drivers/staging/fpga/Makefile
> >  create mode 100644 drivers/staging/fpga/fpga-mgr.c
> >  create mode 100644 include/linux/fpga/fpga-mgr.h
> 
> > diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
> > new file mode 100644
> > index 0000000..8254ca0
> > --- /dev/null
> > +++ b/drivers/staging/fpga/Kconfig
> > @@ -0,0 +1,14 @@
> > +#
> > +# FPGA framework configuration
> > +#
> > +
> > +menu "FPGA Configuration Support"
> > +
> > +config FPGA
> > +	bool "FPGA Configuration Framework"
> > +	help
> > +	  Say Y here if you want support for configuring FPGAs from the
> > +	  kernel.  The FPGA framework adds a FPGA manager class and FPGA
> > +	  manager drivers.
> > +
> > +endmenu
> 
> Is there some good reason why this is 'bool' instead of 'tristate'?
> I.e., why can't it be built as a loadable module?

Hi Randy,

The simple fpga bus probe function will need probe deferral added for it to
work with that if it gets probed before the fpga manager is loaded.  But I 
think it needed that anyway.  I'll work on it.

Thanks,
Alan


> 
> Thanks.
> 
> -- 
> ~Randy
> 

Re: [PATCH v9 5/7] staging: fpga manager core

[Moritz Fischer]

Hi Alan,

a couple of small things I found while reworking the Zynq version to
match the v9 patchset:

On Fri, Jul 17, 2015 at 8:51 AM,  <atull@opensource.altera.com> wrote:
> From: Alan Tull <atull@opensource.altera.com>
>
> API to support programming FPGA.
>
> The following functions are exported as GPL:
> * fpga_mgr_buf_load
>    Load fpga from image in buffer
>
> * fpga_mgr_firmware_load
>    Request firmware and load it to the FPGA.
>
> * fpga_mgr_register
> * fpga_mgr_unregister
>    FPGA device drivers can be added by calling
>    fpga_mgr_register() to register a set of
>    fpga_manager_ops to do device specific stuff.
>
> * of_fpga_mgr_get
> * fpga_mgr_put
>    Get/put a reference to a fpga manager.
>
> The following sysfs files are created:
> * /sys/class/fpga_manager/<fpga>/name
>   Name of low level driver.
>
> * /sys/class/fpga_manager/<fpga>/state
>   State of fpga manager
>
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> Acked-by: Michal Simek <michal.simek@xilinx.com>
> ---
> v2: s/mangager/manager/
>     check for invalid request_nr
>     add fpga reset interface
>     rework the state code
>     remove FPGA_MGR_FAIL flag
>     add _ERR states to fpga manager states enum
>     low level state op now returns a state enum value
>     initialize framework state from driver state op
>     remove unused fpga read stuff
>     merge sysfs.c into fpga-mgr.c
>     move suspend/resume from bus.c to fpga-mgr.c
>
> v3: Add struct device to fpga_manager (not as a pointer)
>     Add to_fpga_manager
>     Don't get irq in fpga-mgr.c (let low level driver do it)
>     remove from struct fpga_manager: nr, np, parent
>     get rid of fpga_mgr_get_new_minor()
>     simplify fpga_manager_register:
>       reorder parameters
>       use dev instead of pdev
>     get rid of code that used to make more sense when this
>       was a char driver, don't alloc_chrdev_region
>     use a mutex instead of flags
>
> v4: Move to drivers/staging
>
> v5: Make some things be static
>     Kconfig: add 'if FPGA'
>     Makefile: s/fpga-mgr-core.o/fpga-mgr.o/
>     clean up includes
>     use enum fpga_mgr_states instead of int
>     static const char *state_str
>     use DEVICE_ATTR_RO/RW/WO and ATTRIBUTE_GROUPS
>     return -EINVAL instead of BUG_ON
>     move ida_simple_get after kzalloc
>     clean up fpga_mgr_remove
>     fpga-mgr.h: remove '#if IS_ENABLED(CONFIG_FPGA)'
>     add kernel-doc documentation
>     Move header to new include/linux/fpga folder
>     static const struct fpga_mgr_ops
>     dev_info msg simplified
>
> v6: no statically allocated string for image_name
>     kernel doc fixes
>     changes regarding enabling SYSFS for fpga mgr
>     Makefile cleanup
>
> v7: no change in this patch for v7 of the patchset
>
> v8: no change in this patch for v8 of the patchset
>
> v9: remove writable attributes 'firmware' and 'reset'
>     remove suspend/resume support for now
>     remove list of fpga managers; use class device iterators instead
>     simplify locking by giving out only one ref exclusively
>     add device tree support
>     add flags
>     par down API into fewer functions
>     update copyright year
>     rename some functions for clarity
>     clean up unnecessary #includes
>     add some error messages
>     write_init may need to look at buffer header, so add to params
> ---
>  drivers/staging/Kconfig         |    2 +
>  drivers/staging/Makefile        |    1 +
>  drivers/staging/fpga/Kconfig    |   14 ++
>  drivers/staging/fpga/Makefile   |    8 +
>  drivers/staging/fpga/fpga-mgr.c |  373 +++++++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h   |  127 +++++++++++++
>  6 files changed, 525 insertions(+)
>  create mode 100644 drivers/staging/fpga/Kconfig
>  create mode 100644 drivers/staging/fpga/Makefile
>  create mode 100644 drivers/staging/fpga/fpga-mgr.c
>  create mode 100644 include/linux/fpga/fpga-mgr.h
>
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index 7f6cae5..89c089c 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -112,4 +112,6 @@ source "drivers/staging/fsl-mc/Kconfig"
>
>  source "drivers/staging/wilc1000/Kconfig"
>
> +source "drivers/staging/fpga/Kconfig"
> +
>  endif # STAGING
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 347f647..e129940 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -48,3 +48,4 @@ obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD) += clocking-wizard/
>  obj-$(CONFIG_FB_TFT)           += fbtft/
>  obj-$(CONFIG_FSL_MC_BUS)       += fsl-mc/
>  obj-$(CONFIG_WILC1000)         += wilc1000/
> +obj-$(CONFIG_FPGA)             += fpga/
> diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
> new file mode 100644
> index 0000000..8254ca0
> --- /dev/null
> +++ b/drivers/staging/fpga/Kconfig
> @@ -0,0 +1,14 @@
> +#
> +# FPGA framework configuration
> +#
> +
> +menu "FPGA Configuration Support"
> +
> +config FPGA
> +       bool "FPGA Configuration Framework"
> +       help
> +         Say Y here if you want support for configuring FPGAs from the
> +         kernel.  The FPGA framework adds a FPGA manager class and FPGA
> +         manager drivers.
> +
> +endmenu
> diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
> new file mode 100644
> index 0000000..3313c52
> --- /dev/null
> +++ b/drivers/staging/fpga/Makefile
> @@ -0,0 +1,8 @@
> +#
> +# Makefile for the fpga framework and fpga manager drivers.
> +#
> +
> +# Core FPGA Manager Framework
> +obj-$(CONFIG_FPGA)                     += fpga-mgr.o
> +
> +# FPGA Manager Drivers
> diff --git a/drivers/staging/fpga/fpga-mgr.c b/drivers/staging/fpga/fpga-mgr.c
> new file mode 100644
> index 0000000..1363f8f
> --- /dev/null
> +++ b/drivers/staging/fpga/fpga-mgr.c
> @@ -0,0 +1,373 @@
> +/*
> + * FPGA Manager Core
> + *
> + *  Copyright (C) 2013-2015 Altera Corporation
> + *
> + * With code from the mailing list:
> + * Copyright (C) 2013 Xilinx, Inc.
> + *
> + * 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/firmware.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/idr.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +static DEFINE_IDA(fpga_mgr_ida);
> +static struct class *fpga_mgr_class;
> +
> +/**
> + * fpga_mgr_buf_load - load fpga from image in buffer
> + * @mgr:       fpga manager
> + * @flags:     flags setting fpga confuration modes
> + * @buf:       buffer contain fpga image
> + * @count:     byte count of buf
> + *
> + * Step the low level fpga manager through the device-specific steps of getting
> + * an FPGA ready to be configured, writing the image to it, then doing whatever
> + * post-configuration steps necessary.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
> +                     size_t count)
> +{
> +       struct device *dev = &mgr->dev;
> +       int ret;
> +
> +       if (!mgr)
> +               return -ENODEV;
> +
> +       /*
> +        * 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 receive an FPGA image.
> +        */
> +       mgr->state = FPGA_MGR_STATE_WRITE_INIT;
> +       ret = mgr->mops->write_init(mgr, flags, buf, count);
> +       if (ret) {
> +               dev_err(dev, "Error preparing FPGA for writing\n");
> +               mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> +               return ret;
> +       }
> +
> +       /*
> +        * Write the FPGA image to the FPGA.
> +        */
> +       mgr->state = FPGA_MGR_STATE_WRITE;
> +       ret = mgr->mops->write(mgr, buf, count);
> +       if (ret) {
> +               dev_err(dev, "Error while writing image data to FPGA\n");
> +               mgr->state = FPGA_MGR_STATE_WRITE_ERR;
> +               return ret;
> +       }
> +
> +       /*
> +        * After all the FPGA image has been written, do the device specific
> +        * steps to finish and set the FPGA into operating mode.
> +        */
> +       mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
> +       ret = mgr->mops->write_complete(mgr);

Could we pass in flags here? This way the driver wouldn't have to keep
track of the flags. For my case it would simplify the partial reconfig
case.
> +       if (ret) {
> +               dev_err(dev, "Error after writing image data to FPGA\n");
> +               mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
> +
> +/**
> + * fpga_mgr_firmware_load - request firmware and load to fpga
> + * @mgr:       fpga manager
> + * @flags:     flags setting fpga confuration modes
> + * @image_name:        name of image file on the firmware search path
> + *
> + * Request an FPGA image using the firmware class, then write out to the FPGA.
> + * Update the state before each step to provide info on what step failed if
> + * there is a failure.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
> +                          const char *image_name)
> +{
> +       struct device *dev = &mgr->dev;
> +       const struct firmware *fw;
> +       int ret;
> +
> +       if (!mgr)
> +               return -ENODEV;
> +
> +       dev_info(dev, "writing %s to %s\n", image_name, mgr->name);
> +
> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
> +
> +       ret = request_firmware(&fw, image_name, dev);
> +       if (ret) {
> +               mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
> +               dev_err(dev, "Error requesting firmware %s\n", image_name);
> +               return ret;
> +       }
> +
> +       ret = fpga_mgr_buf_load(mgr, flags, fw->data, fw->size);
> +       if (ret)
> +               return ret;
> +
> +       release_firmware(fw);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
> +
> +static const char * const state_str[] = {
> +       [FPGA_MGR_STATE_UNKNOWN] =              "unknown",
> +       [FPGA_MGR_STATE_POWER_OFF] =            "power off",
> +       [FPGA_MGR_STATE_POWER_UP] =             "power up",
> +       [FPGA_MGR_STATE_RESET] =                "reset",
> +
> +       /* requesting FPGA image from firmware */
> +       [FPGA_MGR_STATE_FIRMWARE_REQ] =         "firmware request",
> +       [FPGA_MGR_STATE_FIRMWARE_REQ_ERR] =     "firmware request error",
> +
> +       /* Preparing FPGA to receive image */
> +       [FPGA_MGR_STATE_WRITE_INIT] =           "write init",
> +       [FPGA_MGR_STATE_WRITE_INIT_ERR] =       "write init error",
> +
> +       /* Writing image to FPGA */
> +       [FPGA_MGR_STATE_WRITE] =                "write",
> +       [FPGA_MGR_STATE_WRITE_ERR] =            "write error",
> +
> +       /* Finishing configuration after image has been written */
> +       [FPGA_MGR_STATE_WRITE_COMPLETE] =       "write complete",
> +       [FPGA_MGR_STATE_WRITE_COMPLETE_ERR] =   "write complete error",
> +
> +       /* FPGA reports to be in normal operating mode */
> +       [FPGA_MGR_STATE_OPERATING] =            "operating",
> +};
> +
> +static ssize_t name_show(struct device *dev,
> +                        struct device_attribute *attr, char *buf)
> +{
> +       struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> +       return sprintf(buf, "%s\n", mgr->name);
> +}
> +
> +static ssize_t state_show(struct device *dev,
> +                         struct device_attribute *attr, char *buf)
> +{
> +       struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> +       return sprintf(buf, "%s\n", state_str[mgr->state]);
> +}
> +
> +static DEVICE_ATTR_RO(name);
> +static DEVICE_ATTR_RO(state);
> +
> +static struct attribute *fpga_mgr_attrs[] = {
> +       &dev_attr_name.attr,
> +       &dev_attr_state.attr,
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(fpga_mgr);
> +
> +static int fpga_mgr_of_node_match(struct device *dev, const void *data)
> +{
> +       return dev->of_node == data;
> +}
> +
> +/**
> + * of_fpga_mgr_get - get an exclusive reference to a fpga mgr
> + * @node:      device node
> + *
> + * Given a device node, get an exclusive reference to a fpga mgr.
> + *
> + * Return: fpga manager struct or IS_ERR() condition containing error code.
> + */
> +struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
> +{
> +       struct fpga_manager *mgr;
> +       struct device *dev;
> +
> +       if (!node)
> +               return ERR_PTR(-EINVAL);
> +
> +       dev = class_find_device(fpga_mgr_class, NULL, node,
> +                               fpga_mgr_of_node_match);
> +       if (!dev)
> +               return ERR_PTR(-ENODEV);
> +
> +       mgr = to_fpga_manager(dev);
> +       put_device(dev);
> +       if (!mgr)
> +               return ERR_PTR(-ENODEV);
> +
> +       if (!mutex_trylock(&mgr->ref_mutex))
> +               return ERR_PTR(-EBUSY);
> +
> +       return mgr;
> +}
> +EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
> +
> +/**
> + * fpga_mgr_put - release a reference to a fpga manager
> + * @mgr:       fpga manager structure
> + */
> +void fpga_mgr_put(struct fpga_manager *mgr)
> +{
> +       if (mgr)
> +               mutex_unlock(&mgr->ref_mutex);
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_put);
> +
> +/**
> + * fpga_mgr_register - register a low level fpga manager driver
> + * @dev:       fpga manager device from pdev
> + * @name:      fpga manager name
> + * @mops:      pointer to structure of fpga manager ops
> + * @priv:      fpga manager private data
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int fpga_mgr_register(struct device *dev, const char *name,
> +                     const struct fpga_manager_ops *mops,
> +                     void *priv)
> +{
> +       struct fpga_manager *mgr;
> +       const char *dt_label;
> +       int id, ret;
> +
> +       if (!mops || !mops->write_init || !mops->write ||
> +           !mops->write_complete || !mops->state) {
> +               dev_err(dev, "Attempt to register without fpga_manager_ops\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!name || !strlen(name)) {
> +               dev_err(dev, "Attempt to register with no name!\n");
> +               return -EINVAL;
> +       }
> +
> +       mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> +       if (!mgr)
> +               return -ENOMEM;
> +
> +       id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
> +       if (id < 0) {
> +               ret = id;
> +               goto error_kfree;
> +       }
> +
> +       mutex_init(&mgr->ref_mutex);
> +
> +       mgr->name = name;
> +       mgr->mops = mops;
> +       mgr->priv = priv;
> +
> +       /*
> +        * Initialize framework state by requesting low level driver read state
> +        * from device.  FPGA may be in reset mode or may have been programmed
> +        * by bootloader or EEPROM.
> +        */
> +       mgr->state = mgr->mops->state(mgr);
> +
> +       device_initialize(&mgr->dev);
> +       mgr->dev.class = fpga_mgr_class;
> +       mgr->dev.parent = dev;
> +       mgr->dev.of_node = dev->of_node;
> +       mgr->dev.id = id;
> +       dev_set_drvdata(dev, mgr);
> +
> +       dt_label = of_get_property(mgr->dev.of_node, "label", NULL);
> +       if (dt_label)
> +               ret = dev_set_name(&mgr->dev, "%s", dt_label);
> +       else
> +               ret = dev_set_name(&mgr->dev, "fpga%d", id);
> +
> +       ret = device_add(&mgr->dev);
> +       if (ret)
> +               goto error_device;
> +
> +       dev_info(&mgr->dev, "%s registered\n", mgr->name);
> +
> +       return 0;
> +
> +error_device:
> +       ida_simple_remove(&fpga_mgr_ida, id);
> +error_kfree:
> +       kfree(mgr);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_register);
> +
> +/**
> + * fpga_mgr_unregister - unregister a low level fpga manager driver
> + * @dev:       fpga manager device from pdev
> + */
> +void fpga_mgr_unregister(struct device *dev)
> +{
> +       struct fpga_manager *mgr = dev_get_drvdata(dev);
> +
> +       dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
> +
> +       /*
> +        * If the low level driver provides a method for putting fpga into
> +        * a desired state upon unregister, do it.
> +        */
> +       if (mgr->mops->fpga_remove)
> +               mgr->mops->fpga_remove(mgr);
> +
> +       device_unregister(&mgr->dev);
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
> +
> +static void fpga_mgr_dev_release(struct device *dev)
> +{
> +       struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> +       ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
> +       kfree(mgr);
> +}
> +
> +static int __init fpga_mgr_class_init(void)
> +{
> +       pr_info("FPGA manager framework\n");
> +
> +       fpga_mgr_class = class_create(THIS_MODULE, "fpga_manager");
> +       if (IS_ERR(fpga_mgr_class))
> +               return PTR_ERR(fpga_mgr_class);
> +
> +       fpga_mgr_class->dev_groups = fpga_mgr_groups;
> +       fpga_mgr_class->dev_release = fpga_mgr_dev_release;
> +
> +       return 0;
> +}
> +
> +static void __exit fpga_mgr_class_exit(void)
> +{
> +       class_destroy(fpga_mgr_class);
> +       ida_destroy(&fpga_mgr_ida);
> +}
> +
> +MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> +MODULE_DESCRIPTION("FPGA manager framework");
> +MODULE_LICENSE("GPL v2");
> +
> +subsys_initcall(fpga_mgr_class_init);
> +module_exit(fpga_mgr_class_exit);
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> new file mode 100644
> index 0000000..14a2ca6
> --- /dev/null
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -0,0 +1,127 @@
> +/*
> + * FPGA Framework
> + *
> + *  Copyright (C) 2013-2015 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +
> +#ifndef _LINUX_FPGA_MGR_H
> +#define _LINUX_FPGA_MGR_H
> +
> +struct fpga_manager;
> +
> +/**
> + * enum fpga_mgr_states - fpga framework states
> + * @FPGA_MGR_STATE_UNKNOWN: can't determine state
> + * @FPGA_MGR_STATE_POWER_OFF: FPGA power is off
> + * @FPGA_MGR_STATE_POWER_UP: FPGA reports power is up
> + * @FPGA_MGR_STATE_RESET: FPGA in reset state
> + * @FPGA_MGR_STATE_FIRMWARE_REQ: firmware request in progress
> + * @FPGA_MGR_STATE_FIRMWARE_REQ_ERR: firmware request failed
> + * @FPGA_MGR_STATE_WRITE_INIT: preparing FPGA for programming
> + * @FPGA_MGR_STATE_WRITE_INIT_ERR: Error during WRITE_INIT stage
> + * @FPGA_MGR_STATE_WRITE: writing image to FPGA
> + * @FPGA_MGR_STATE_WRITE_ERR: Error while writing FPGA
> + * @FPGA_MGR_STATE_WRITE_COMPLETE: Doing post programming steps
> + * @FPGA_MGR_STATE_WRITE_COMPLETE_ERR: Error during WRITE_COMPLETE
> + * @FPGA_MGR_STATE_OPERATING: FPGA is programmed and operating
> + */
> +enum fpga_mgr_states {
> +       /* default FPGA states */
> +       FPGA_MGR_STATE_UNKNOWN,
> +       FPGA_MGR_STATE_POWER_OFF,
> +       FPGA_MGR_STATE_POWER_UP,
> +       FPGA_MGR_STATE_RESET,
> +
> +       /* getting an image for loading */
> +       FPGA_MGR_STATE_FIRMWARE_REQ,
> +       FPGA_MGR_STATE_FIRMWARE_REQ_ERR,
> +
> +       /* write sequence: init, write, complete */
> +       FPGA_MGR_STATE_WRITE_INIT,
> +       FPGA_MGR_STATE_WRITE_INIT_ERR,
> +       FPGA_MGR_STATE_WRITE,
> +       FPGA_MGR_STATE_WRITE_ERR,
> +       FPGA_MGR_STATE_WRITE_COMPLETE,
> +       FPGA_MGR_STATE_WRITE_COMPLETE_ERR,
> +
> +       /* fpga is programmed and operating */
> +       FPGA_MGR_STATE_OPERATING,
> +};
> +
> +/*
> + * FPGA Manager flags
> + * FPGA_MGR_PARTIAL_RECONFIG: do partial reconfiguration if supported
> + */
> +#define FPGA_MGR_PARTIAL_RECONFIG (1)
Could this be BIT(0) instead?
> +
> +/**
> + * struct fpga_manager_ops - ops for low level fpga manager drivers
> + * @state: returns an enum value of the FPGA's state
> + * @write_init: prepare the FPGA to receive confuration data
> + * @write: write count bytes of configuration data to the FPGA
> + * @write_complete: set FPGA to operating state after writing is done
> + * @fpga_remove: optional: Set FPGA into a specific state during driver remove
> + *
> + * fpga_manager_ops are the low level functions implemented by a specific
> + * fpga manager driver.  The optional ones are tested for NULL before being
> + * called, so leaving them out is fine.
> + */
> +struct fpga_manager_ops {
> +       enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
> +       int (*write_init)(struct fpga_manager *mgr, u32 flags,
> +                         const char *buf, size_t count);
> +       int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
> +       int (*write_complete)(struct fpga_manager *mgr);
See comment above, having the flags here would be very convenient for
my usecase.
> +       void (*fpga_remove)(struct fpga_manager *mgr);
> +};
> +
> +/**
> + * struct fpga_manager - fpga manager structure
> + * @name: name of low level fpga manager
> + * @dev: fpga manager device
> + * @ref_mutex: only allows one reference to fpga manager
> + * @state: state of fpga manager
> + * @mops: pointer to struct of fpga manager ops
> + * @priv: low level driver private date
> + */
> +struct fpga_manager {
> +       const char *name;
> +       struct device dev;
> +       struct mutex ref_mutex;
> +       enum fpga_mgr_states state;
> +       const struct fpga_manager_ops *mops;
> +       void *priv;
> +};
> +
> +#define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> +
> +int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags,
> +                     const char *buf, size_t count);
> +
> +int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
> +                          const char *image_name);
> +
> +struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
> +
> +void fpga_mgr_put(struct fpga_manager *mgr);
> +
> +int fpga_mgr_register(struct device *dev, const char *name,
> +                     const struct fpga_manager_ops *mops, void *priv);
> +
> +void fpga_mgr_unregister(struct device *dev);
> +
> +#endif /*_LINUX_FPGA_MGR_H */
> --
> 1.7.9.5
>
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Overall looks pretty good. I still need to look at the bridge part,
currently I have the resets and level shifters in the zynq-fpga
driver,
but maybe breaking them out makes sense.

Cheers,

Moritz

Re: [PATCH v9 5/7] staging: fpga manager core

[atull]

On Wed, 22 Jul 2015, Moritz Fischer wrote:

Hi Miritz,

> Hi Alan,
> 
> a couple of small things I found while reworking the Zynq version to
> match the v9 patchset:
> 
> On Fri, Jul 17, 2015 at 8:51 AM,  <atull@opensource.altera.com> wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> >
...
> > +       ret = mgr->mops->write_complete(mgr);
> 
> Could we pass in flags here? This way the driver wouldn't have to keep
> track of the flags. For my case it would simplify the partial reconfig
> case.

Yes, that change will be very simple and makes sense to me.

> > +/*
> > + * FPGA Manager flags
> > + * FPGA_MGR_PARTIAL_RECONFIG: do partial reconfiguration if supported
> > + */
> > +#define FPGA_MGR_PARTIAL_RECONFIG (1)
> Could this be BIT(0) instead?

Yes, ha.  That's what it should have been.

> > +
> > +/**
> > + * struct fpga_manager_ops - ops for low level fpga manager drivers
> > + * @state: returns an enum value of the FPGA's state
> > + * @write_init: prepare the FPGA to receive confuration data
> > + * @write: write count bytes of configuration data to the FPGA
> > + * @write_complete: set FPGA to operating state after writing is done
> > + * @fpga_remove: optional: Set FPGA into a specific state during driver remove
> > + *
> > + * fpga_manager_ops are the low level functions implemented by a specific
> > + * fpga manager driver.  The optional ones are tested for NULL before being
> > + * called, so leaving them out is fine.
> > + */
> > +struct fpga_manager_ops {
> > +       enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
> > +       int (*write_init)(struct fpga_manager *mgr, u32 flags,
> > +                         const char *buf, size_t count);
> > +       int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
> > +       int (*write_complete)(struct fpga_manager *mgr);
> See comment above, having the flags here would be very convenient for
> my usecase.

Yes

> > +       void (*fpga_remove)(struct fpga_manager *mgr);
> > +};
> > +
...
> 
> Overall looks pretty good. I still need to look at the bridge part,
> currently I have the resets and level shifters in the zynq-fpga
> driver,
> but maybe breaking them out makes sense.
> 
> Cheers,
> 
> Moritz
> 

Thanks for the review.  I'll probably send out a v10 next week including your
recommendations.

Alan

[PATCH v9 6/7] staging: add simple-fpga-bus

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add simple fpga bus.  This is a bus that configures an fpga and its
bridges before populating the devices below it.  This is intended
for use with device tree overlays.

Note that FPGA bridges are seen as reset controllers so no special
framework for FPGA bridges will need to be added.

This supports fpga use where hardware blocks on a fpga will need
drivers (as opposed to fpga used as an acceleration without drivers.)

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 drivers/staging/fpga/Kconfig           |   11 ++
 drivers/staging/fpga/Makefile          |    1 +
 drivers/staging/fpga/simple-fpga-bus.c |  323 ++++++++++++++++++++++++++++++++
 3 files changed, 335 insertions(+)
 create mode 100644 drivers/staging/fpga/simple-fpga-bus.c

diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
index 8254ca0..8d003e3 100644
--- a/drivers/staging/fpga/Kconfig
+++ b/drivers/staging/fpga/Kconfig
@@ -11,4 +11,15 @@ config FPGA
 	  kernel.  The FPGA framework adds a FPGA manager class and FPGA
 	  manager drivers.
 
+if FPGA
+
+config SIMPLE_FPGA_BUS
+       bool "Simple FPGA Bus"
+       depends on OF
+       help
+         Simple FPGA Bus allows loading FPGA images under control of
+	 Device Tree.
+
+endif # FPGA
+
 endmenu
diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
index 3313c52..6115213 100644
--- a/drivers/staging/fpga/Makefile
+++ b/drivers/staging/fpga/Makefile
@@ -4,5 +4,6 @@
 
 # Core FPGA Manager Framework
 obj-$(CONFIG_FPGA)			+= fpga-mgr.o
+obj-$(CONFIG_SIMPLE_FPGA_BUS)		+= simple-fpga-bus.o
 
 # FPGA Manager Drivers
diff --git a/drivers/staging/fpga/simple-fpga-bus.c b/drivers/staging/fpga/simple-fpga-bus.c
new file mode 100644
index 0000000..bf178d8
--- /dev/null
+++ b/drivers/staging/fpga/simple-fpga-bus.c
@@ -0,0 +1,323 @@
+/*
+ * Simple FPGA Bus
+ *
+ *  Copyright (C) 2013-2015 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+/**
+ * struct simple_fpga_bus - simple fpga bus private data
+ * @dev:	device from pdev
+ * @mgr:	the fpga manager associated with this bus
+ * @bridges:	an array of reset controls for controlling FPGA bridges
+ *		associated with this bus
+ * @num_bridges: size of the bridges array
+ */
+struct simple_fpga_bus {
+	struct device *dev;
+	struct fpga_manager *mgr;
+	struct reset_control **bridges;
+	int num_bridges;
+};
+
+/**
+ * simple_fpga_bus_get_mgr - get associated fpga manager
+ * @priv: simple fpga bus private data
+ * pointer to fpga manager in priv->mgr on success
+ *
+ * Given a simple fpga bus, get a reference to its the fpga manager specified
+ * by its "fpga-mgr" device tree property.
+ *
+ * Return: 0 if success or if the fpga manager is not specified.
+ *         Negative error code otherwise.
+ */
+static int simple_fpga_bus_get_mgr(struct simple_fpga_bus *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	struct fpga_manager *mgr;
+	struct device_node *mgr_node;
+
+	/*
+	 * Return 0 (not an error) if fpga manager is not specified.
+	 * This supports the case where fpga was already configured.
+	 */
+	mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
+	if (!mgr_node) {
+		dev_dbg(dev, "could not find fpga-mgr DT property\n");
+		return 0;
+	}
+
+	mgr = of_fpga_mgr_get(mgr_node);
+	if (IS_ERR(mgr))
+		return PTR_ERR(mgr);
+
+	priv->mgr = mgr;
+
+	return 0;
+}
+
+/**
+ * simple_fpga_bus_load_image - load an fpga image under device tree control
+ * @priv: simple fpga bus private data
+ *
+ * Given a simple fpga bus, load the fpga image specified in its device
+ * tree node.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int simple_fpga_bus_load_image(struct simple_fpga_bus *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	struct fpga_manager *mgr = priv->mgr;
+	u32 flags = 0;
+	const char *path;
+
+	if (of_property_read_bool(np, "partial-reconfig"))
+		flags |= FPGA_MGR_PARTIAL_RECONFIG;
+
+	/* If firmware image is specified in the DT, load it */
+	if (!of_property_read_string(np, "fpga-firmware", &path))
+		return fpga_mgr_firmware_load(mgr, flags, path);
+
+	/*
+	 * Here we can add other methods of getting ahold of a fpga image
+	 * specified in the device tree and programming it.
+	 */
+
+	dev_info(dev, "No FPGA image to load.\n");
+
+	/* Status is that we have a fpga manager but no image specified. */
+	return -EINVAL;
+}
+
+/**
+ * simple_fpga_bus_bridge_enable - enable the fpga bridges
+ * @priv: simple fpga bus private data
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int simple_fpga_bus_bridge_enable(struct simple_fpga_bus *priv)
+{
+	int i, ret;
+
+	for (i = 0; i < priv->num_bridges; i++) {
+		ret = reset_control_deassert(priv->bridges[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * simple_fpga_bus_bridge_disable - disable the bridges
+ * @priv: simple fpga bus private data
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int simple_fpga_bus_bridge_disable(struct simple_fpga_bus *priv)
+{
+	int i, ret;
+
+	for (i = 0; i < priv->num_bridges; i++) {
+		ret = reset_control_assert(priv->bridges[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * simple_fpga_bus_get_bridges - get references for fpga bridges
+ * @priv: simple fpga bus private data
+ *
+ * Given a simple fpga bus, get references for its associated fpga bridges so
+ * that it can enable/disable the bridges.  These are specified by "resets"
+ * and "reset-names" device tree properties.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int simple_fpga_bus_get_bridges(struct simple_fpga_bus *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	const char *reset_name;
+	struct reset_control **bridges;
+	int i, num_resets, num_names, ret;
+
+	num_resets = of_count_phandle_with_args(np, "resets", "#reset-cells");
+	num_names = of_property_count_strings(np, "reset-names");
+	if (num_resets <= 0 || num_names <= 0) {
+		dev_info(dev, "No fpga bridge resets found\n");
+		return -EINVAL;
+	}
+	if (num_resets != num_names) {
+		dev_dbg(dev, "Number of resets and reset-names differ.");
+		return -EINVAL;
+	}
+
+	bridges = kcalloc(num_resets, sizeof(struct reset_control *),
+			  GFP_KERNEL);
+	if (!bridges)
+		return -ENOMEM;
+
+	for (i = 0; i < num_resets; i++) {
+		ret = of_property_read_string_index(np, "reset-names", i,
+						    &reset_name);
+		if (ret)
+			return ret;
+
+		bridges[i] = of_reset_control_get(np, reset_name);
+		if (IS_ERR(bridges[i])) {
+			ret = PTR_ERR(bridges[i]);
+			goto err_free_bridges;
+		}
+	}
+
+	priv->bridges = bridges;
+	priv->num_bridges = num_resets;
+
+	return 0;
+
+err_free_bridges:
+	for (i = 0; i < num_resets; i++)
+		reset_control_put(priv->bridges[i]);
+
+	kfree(bridges);
+	return ret;
+}
+
+/**
+ * simple_fpga_bus_put_bridges - release references for the fpga bridges
+ * @priv: simple fpga bus private data
+ */
+static void simple_fpga_bus_put_bridges(struct simple_fpga_bus *priv)
+{
+	int i;
+
+	for (i = 0; i < priv->num_bridges; i++)
+		reset_control_put(priv->bridges[i]);
+
+	kfree(priv->bridges);
+	priv->num_bridges = 0;
+}
+
+/**
+ * simple_fpga_bus_probe - Probe function for simple fpga bus.
+ * @pdev: platform device
+ *
+ * Do the necessary steps to program the FPGA and enable associated bridges.
+ * Then populate the device tree below this bus to get drivers probed for the
+ * hardware that is on the FPGA.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int simple_fpga_bus_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct simple_fpga_bus *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	ret = simple_fpga_bus_get_mgr(priv);
+	if (ret)
+		return ret;
+
+	if (priv->mgr) {
+		ret = simple_fpga_bus_get_bridges(priv);
+		if (ret)
+			goto err_release_mgr;
+
+		ret = simple_fpga_bus_load_image(priv);
+		if (ret)
+			goto err_release_br;
+
+		ret = simple_fpga_bus_bridge_enable(priv);
+		if (ret)
+			goto err_release_br;
+	}
+
+	of_platform_populate(np, of_default_bus_match_table, NULL, dev);
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+
+err_release_br:
+	simple_fpga_bus_put_bridges(priv);
+err_release_mgr:
+	fpga_mgr_put(priv->mgr);
+
+	return ret;
+}
+
+static int simple_fpga_bus_remove(struct platform_device *pdev)
+{
+	struct simple_fpga_bus *priv = platform_get_drvdata(pdev);
+
+	simple_fpga_bus_bridge_disable(priv);
+	simple_fpga_bus_put_bridges(priv);
+	fpga_mgr_put(priv->mgr);
+
+	return 0;
+}
+
+static const struct of_device_id simple_fpga_bus_of_match[] = {
+	{ .compatible = "simple-fpga-bus", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, simple_fpga_bus_of_match);
+
+static struct platform_driver simple_fpga_bus_driver = {
+	.probe = simple_fpga_bus_probe,
+	.remove = simple_fpga_bus_remove,
+	.driver = {
+		.name	= "simple-fpga-bus",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(simple_fpga_bus_of_match),
+	},
+};
+
+static int __init simple_fpga_bus_init(void)
+{
+	return platform_driver_register(&simple_fpga_bus_driver);
+}
+
+static void __exit simple_fpga_bus_exit(void)
+{
+	platform_driver_unregister(&simple_fpga_bus_driver);
+}
+
+module_init(simple_fpga_bus_init);
+module_exit(simple_fpga_bus_exit);
+
+MODULE_DESCRIPTION("Simple FPGA Bus");
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v9 6/7] staging: add simple-fpga-bus

[Moritz Fischer]

Hi Alan,

I saw that your socfpga driver doesn't support the partial reconfig
use case (not a big deal).
What I currently do for Zynq is if I'm doing a non-partial reconfig is
that I disable input
level shifters and assert *all* resets while reprogramming in my FPGA
manager .write_init() and .write_complete().
For a partial reconfig use case that obviously doesn't work, since I
don't want to
bring down the entire interconnect.
In a partial reconfiguration situation, would I have separate simple-fpga buses
for each of the parts that I swap out, each with it's own reset and
bitfile attached?

On Fri, Jul 17, 2015 at 8:51 AM,  <atull@opensource.altera.com> wrote:
> From: Alan Tull <atull@opensource.altera.com>
>
> Add simple fpga bus.  This is a bus that configures an fpga and its
> bridges before populating the devices below it.  This is intended
> for use with device tree overlays.
>
> Note that FPGA bridges are seen as reset controllers so no special
> framework for FPGA bridges will need to be added.
>
> This supports fpga use where hardware blocks on a fpga will need
> drivers (as opposed to fpga used as an acceleration without drivers.)
>
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
>  drivers/staging/fpga/Kconfig           |   11 ++
>  drivers/staging/fpga/Makefile          |    1 +
>  drivers/staging/fpga/simple-fpga-bus.c |  323 ++++++++++++++++++++++++++++++++
>  3 files changed, 335 insertions(+)
>  create mode 100644 drivers/staging/fpga/simple-fpga-bus.c
>
> diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
> index 8254ca0..8d003e3 100644
> --- a/drivers/staging/fpga/Kconfig
> +++ b/drivers/staging/fpga/Kconfig
> @@ -11,4 +11,15 @@ config FPGA
>           kernel.  The FPGA framework adds a FPGA manager class and FPGA
>           manager drivers.
>
> +if FPGA
> +
> +config SIMPLE_FPGA_BUS
> +       bool "Simple FPGA Bus"
> +       depends on OF
> +       help
> +         Simple FPGA Bus allows loading FPGA images under control of
> +        Device Tree.
> +
> +endif # FPGA
> +
>  endmenu
> diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
> index 3313c52..6115213 100644
> --- a/drivers/staging/fpga/Makefile
> +++ b/drivers/staging/fpga/Makefile
> @@ -4,5 +4,6 @@
>
>  # Core FPGA Manager Framework
>  obj-$(CONFIG_FPGA)                     += fpga-mgr.o
> +obj-$(CONFIG_SIMPLE_FPGA_BUS)          += simple-fpga-bus.o
>
>  # FPGA Manager Drivers
> diff --git a/drivers/staging/fpga/simple-fpga-bus.c b/drivers/staging/fpga/simple-fpga-bus.c
> new file mode 100644
> index 0000000..bf178d8
> --- /dev/null
> +++ b/drivers/staging/fpga/simple-fpga-bus.c
> @@ -0,0 +1,323 @@
> +/*
> + * Simple FPGA Bus
> + *
> + *  Copyright (C) 2013-2015 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +/**
> + * struct simple_fpga_bus - simple fpga bus private data
> + * @dev:       device from pdev
> + * @mgr:       the fpga manager associated with this bus
> + * @bridges:   an array of reset controls for controlling FPGA bridges
> + *             associated with this bus
> + * @num_bridges: size of the bridges array
> + */
> +struct simple_fpga_bus {
> +       struct device *dev;
> +       struct fpga_manager *mgr;
> +       struct reset_control **bridges;
> +       int num_bridges;
> +};
> +
> +/**
> + * simple_fpga_bus_get_mgr - get associated fpga manager
> + * @priv: simple fpga bus private data
> + * pointer to fpga manager in priv->mgr on success
> + *
> + * Given a simple fpga bus, get a reference to its the fpga manager specified
> + * by its "fpga-mgr" device tree property.
> + *
> + * Return: 0 if success or if the fpga manager is not specified.
> + *         Negative error code otherwise.
> + */
> +static int simple_fpga_bus_get_mgr(struct simple_fpga_bus *priv)
> +{
> +       struct device *dev = priv->dev;
> +       struct device_node *np = dev->of_node;
> +       struct fpga_manager *mgr;
> +       struct device_node *mgr_node;
> +
> +       /*
> +        * Return 0 (not an error) if fpga manager is not specified.
> +        * This supports the case where fpga was already configured.
> +        */
> +       mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
> +       if (!mgr_node) {
> +               dev_dbg(dev, "could not find fpga-mgr DT property\n");
> +               return 0;
> +       }
> +
> +       mgr = of_fpga_mgr_get(mgr_node);
> +       if (IS_ERR(mgr))
> +               return PTR_ERR(mgr);
> +
> +       priv->mgr = mgr;
> +
> +       return 0;
> +}
> +
> +/**
> + * simple_fpga_bus_load_image - load an fpga image under device tree control
> + * @priv: simple fpga bus private data
> + *
> + * Given a simple fpga bus, load the fpga image specified in its device
> + * tree node.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int simple_fpga_bus_load_image(struct simple_fpga_bus *priv)
> +{
> +       struct device *dev = priv->dev;
> +       struct device_node *np = dev->of_node;
> +       struct fpga_manager *mgr = priv->mgr;
> +       u32 flags = 0;
> +       const char *path;
> +
> +       if (of_property_read_bool(np, "partial-reconfig"))
> +               flags |= FPGA_MGR_PARTIAL_RECONFIG;
> +
> +       /* If firmware image is specified in the DT, load it */
> +       if (!of_property_read_string(np, "fpga-firmware", &path))
> +               return fpga_mgr_firmware_load(mgr, flags, path);
> +
> +       /*
> +        * Here we can add other methods of getting ahold of a fpga image
> +        * specified in the device tree and programming it.
> +        */
> +
> +       dev_info(dev, "No FPGA image to load.\n");
> +
> +       /* Status is that we have a fpga manager but no image specified. */
> +       return -EINVAL;
> +}
> +
> +/**
> + * simple_fpga_bus_bridge_enable - enable the fpga bridges
> + * @priv: simple fpga bus private data
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int simple_fpga_bus_bridge_enable(struct simple_fpga_bus *priv)
> +{
> +       int i, ret;
> +
> +       for (i = 0; i < priv->num_bridges; i++) {
> +               ret = reset_control_deassert(priv->bridges[i]);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * simple_fpga_bus_bridge_disable - disable the bridges
> + * @priv: simple fpga bus private data
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int simple_fpga_bus_bridge_disable(struct simple_fpga_bus *priv)
> +{
> +       int i, ret;
> +
> +       for (i = 0; i < priv->num_bridges; i++) {
> +               ret = reset_control_assert(priv->bridges[i]);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * simple_fpga_bus_get_bridges - get references for fpga bridges
> + * @priv: simple fpga bus private data
> + *
> + * Given a simple fpga bus, get references for its associated fpga bridges so
> + * that it can enable/disable the bridges.  These are specified by "resets"
> + * and "reset-names" device tree properties.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int simple_fpga_bus_get_bridges(struct simple_fpga_bus *priv)
> +{
> +       struct device *dev = priv->dev;
> +       struct device_node *np = dev->of_node;
> +       const char *reset_name;
> +       struct reset_control **bridges;
> +       int i, num_resets, num_names, ret;
> +
> +       num_resets = of_count_phandle_with_args(np, "resets", "#reset-cells");
> +       num_names = of_property_count_strings(np, "reset-names");
> +       if (num_resets <= 0 || num_names <= 0) {
> +               dev_info(dev, "No fpga bridge resets found\n");
> +               return -EINVAL;
> +       }
> +       if (num_resets != num_names) {
> +               dev_dbg(dev, "Number of resets and reset-names differ.");
> +               return -EINVAL;
> +       }
> +
> +       bridges = kcalloc(num_resets, sizeof(struct reset_control *),
> +                         GFP_KERNEL);
> +       if (!bridges)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < num_resets; i++) {
> +               ret = of_property_read_string_index(np, "reset-names", i,
> +                                                   &reset_name);
> +               if (ret)
> +                       return ret;
> +
> +               bridges[i] = of_reset_control_get(np, reset_name);
> +               if (IS_ERR(bridges[i])) {
> +                       ret = PTR_ERR(bridges[i]);
> +                       goto err_free_bridges;
> +               }
> +       }
> +
> +       priv->bridges = bridges;
> +       priv->num_bridges = num_resets;
> +
> +       return 0;
> +
> +err_free_bridges:
> +       for (i = 0; i < num_resets; i++)
> +               reset_control_put(priv->bridges[i]);
> +
> +       kfree(bridges);
> +       return ret;
> +}
> +
> +/**
> + * simple_fpga_bus_put_bridges - release references for the fpga bridges
> + * @priv: simple fpga bus private data
> + */
> +static void simple_fpga_bus_put_bridges(struct simple_fpga_bus *priv)
> +{
> +       int i;
> +
> +       for (i = 0; i < priv->num_bridges; i++)
> +               reset_control_put(priv->bridges[i]);
> +
> +       kfree(priv->bridges);
> +       priv->num_bridges = 0;
> +}
> +
> +/**
> + * simple_fpga_bus_probe - Probe function for simple fpga bus.
> + * @pdev: platform device
> + *
> + * Do the necessary steps to program the FPGA and enable associated bridges.
> + * Then populate the device tree below this bus to get drivers probed for the
> + * hardware that is on the FPGA.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int simple_fpga_bus_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct simple_fpga_bus *priv;
> +       int ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->dev = dev;
> +
> +       ret = simple_fpga_bus_get_mgr(priv);
> +       if (ret)
> +               return ret;
> +
> +       if (priv->mgr) {
> +               ret = simple_fpga_bus_get_bridges(priv);
See below.
> +               if (ret)
> +                       goto err_release_mgr;
> +
> +               ret = simple_fpga_bus_load_image(priv);
> +               if (ret)
> +                       goto err_release_br;
> +
> +               ret = simple_fpga_bus_bridge_enable(priv);
In my tests I never saw an actual disable happen. This might be a bug
in my reset controller driver,
if that's the case please ignore my nit ;-) But wouldn't you want to
assert while reconfiguring?
> +               if (ret)
> +                       goto err_release_br;
> +       }
> +
> +       of_platform_populate(np, of_default_bus_match_table, NULL, dev);
> +
> +       platform_set_drvdata(pdev, priv);
> +
> +       return 0;
> +
> +err_release_br:
> +       simple_fpga_bus_put_bridges(priv);
> +err_release_mgr:
> +       fpga_mgr_put(priv->mgr);
> +
> +       return ret;
> +}
> +
> +static int simple_fpga_bus_remove(struct platform_device *pdev)
> +{
> +       struct simple_fpga_bus *priv = platform_get_drvdata(pdev);
> +
> +       simple_fpga_bus_bridge_disable(priv);
> +       simple_fpga_bus_put_bridges(priv);
> +       fpga_mgr_put(priv->mgr);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id simple_fpga_bus_of_match[] = {
> +       { .compatible = "simple-fpga-bus", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, simple_fpga_bus_of_match);
> +
> +static struct platform_driver simple_fpga_bus_driver = {
> +       .probe = simple_fpga_bus_probe,
> +       .remove = simple_fpga_bus_remove,
> +       .driver = {
> +               .name   = "simple-fpga-bus",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = of_match_ptr(simple_fpga_bus_of_match),
> +       },
> +};
> +
> +static int __init simple_fpga_bus_init(void)
> +{
> +       return platform_driver_register(&simple_fpga_bus_driver);
> +}
> +
> +static void __exit simple_fpga_bus_exit(void)
> +{
> +       platform_driver_unregister(&simple_fpga_bus_driver);
> +}
> +
> +module_init(simple_fpga_bus_init);
> +module_exit(simple_fpga_bus_exit);
> +
> +MODULE_DESCRIPTION("Simple FPGA Bus");
> +MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 1.7.9.5
>
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Cheers,

Moritz

Re: [PATCH v9 6/7] staging: add simple-fpga-bus

[Jason Gunthorpe]

On Thu, Jul 23, 2015 at 02:55:52PM -0700, Moritz Fischer wrote:
> Hi Alan,
> 
> I saw that your socfpga driver doesn't support the partial reconfig
> use case (not a big deal).
> What I currently do for Zynq is if I'm doing a non-partial reconfig is
> that I disable input
> level shifters and assert *all* resets while reprogramming in my FPGA
> manager .write_init() and .write_complete().

I do this as well, but it is a bit more complex.. FPGA specific code
has to run around and ensure all DMA is shut off, then we need to make
sure no CPU issued AXI transactions can happen, then we can tear down
the FPGA side.

If the FPGA is torn down while an AXI op is inprogress things go
sideways, we have to work to prevent that :)

This happens almost for free, I use DT and the device model to
disconnect the drivers. The drivers are careful to synchronously fence
off in-progress DMA. Then drop the DT nodes associated with the
FPGA, finally the actual FPGA cells can be reset.

> In a partial reconfiguration situation, would I have separate
> simple-fpga buses for each of the parts that I swap out, each with
> it's own reset and bitfile attached?

I'd think of partial reconfiguration as another nested FPGA. The
resets and so forth could be attached to soft controllers in the
unswappable part of the FPGA.

DT nodes have to surround it in some way...

Jason

Re: [PATCH v9 6/7] staging: add simple-fpga-bus

[atull]

On Thu, 23 Jul 2015, Jason Gunthorpe wrote:

> On Thu, Jul 23, 2015 at 02:55:52PM -0700, Moritz Fischer wrote:
> > Hi Alan,
> > 
> > I saw that your socfpga driver doesn't support the partial reconfig
> > use case (not a big deal).
> > What I currently do for Zynq is if I'm doing a non-partial reconfig is
> > that I disable input
> > level shifters and assert *all* resets while reprogramming in my FPGA
> > manager .write_init() and .write_complete().
> 
> I do this as well, but it is a bit more complex.. FPGA specific code
> has to run around and ensure all DMA is shut off, then we need to make
> sure no CPU issued AXI transactions can happen, then we can tear down
> the FPGA side.
> 
> If the FPGA is torn down while an AXI op is inprogress things go
> sideways, we have to work to prevent that :)
> 
> This happens almost for free, I use DT and the device model to
> disconnect the drivers. The drivers are careful to synchronously fence
> off in-progress DMA. Then drop the DT nodes associated with the
> FPGA, finally the actual FPGA cells can be reset.

Yes, the kernel gives us this almost for free.  That's what I like
about using DT overlays to control FPGA programming.

> 
> > In a partial reconfiguration situation, would I have separate
> > simple-fpga buses for each of the parts that I swap out, each with
> > it's own reset and bitfile attached?
> 
> I'd think of partial reconfiguration as another nested FPGA. The
> resets and so forth could be attached to soft controllers in the
> unswappable part of the FPGA.
> 
> DT nodes have to surround it in some way...
> 
> Jason
> 

Yes, in this way each PR chunk will need its own reset so it
won't wiggle busses and affect the rest of the system during PR.

I noticed that currently simple-fpga-bus.c holds an exclusive
ref of the fpga manager.  This would keep a 2nd pr from being
able to access the same fpga manager, so I'll have to change
it so that simple-fpga-bus.c will  put the ref before exiting
probe.

Alan

[PATCH v9 7/7] staging: fpga manager: add driver for socfpga fpga manager

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add driver to fpga manager framework to allow configuration
of FPGA in Altera SoCFPGA parts.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
Acked-by: Michal Simek <michal.simek@xilinx.com>
---
v2: fpga_manager struct now contains struct device
    fpga_manager_register parameters now take device

v3: skip a version to align versions

v4: move to drivers/staging

v5: fix array_size.cocci warnings
    fix platform_no_drv_owner.cocci warnings
    Remove .owner = THIS_MODULE
    include asm/irq.h
    clean up list of includes
    use altera_fpga_reset for ops
    use enum fpga_mgr_states or u32 as needed
    use devm_request_irq
    check irq <= 0 instead of == NO_IRQ
    Use ARRAY_SIZE
    rename altera -> socfpga
    static const socfpga_fpga_ops
    header moved to linux/fpga/ folder
    remove ifdef'ed code
    use platform_get_resource and platform_get_irq
    move .probe and .remove lines adjacent
    use module_platform_driver
    use __maybe_unused
    only need to 'if (IS_ENABLED(CONFIG_REGULATOR))' in one fn
    fix "unsigned 'mode' is never < 0"

v6: return error for (unused) default of case statement

v7: PTR_ERR should access the value just tested by IS_ERR

v8: change compatible string to be more chip specific

v9: update copyright year
    remove suspend/resume support and regulators
    use updated fn names for register/unregister
    use updated params for write_init
    check for partial reconfiguration request
---
 drivers/staging/fpga/Kconfig   |    6 +
 drivers/staging/fpga/Makefile  |    1 +
 drivers/staging/fpga/socfpga.c |  616 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 623 insertions(+)
 create mode 100644 drivers/staging/fpga/socfpga.c

diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
index 8d003e3..36dd7b6 100644
--- a/drivers/staging/fpga/Kconfig
+++ b/drivers/staging/fpga/Kconfig
@@ -13,6 +13,12 @@ config FPGA
 
 if FPGA
 
+config FPGA_MGR_SOCFPGA
+	bool "Altera SOCFPGA FPGA Manager"
+	depends on ARCH_SOCFPGA
+	help
+	  FPGA manager driver support for Altera SOCFPGA.
+
 config SIMPLE_FPGA_BUS
        bool "Simple FPGA Bus"
        depends on OF
diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
index 6115213..d1f9406 100644
--- a/drivers/staging/fpga/Makefile
+++ b/drivers/staging/fpga/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_FPGA)			+= fpga-mgr.o
 obj-$(CONFIG_SIMPLE_FPGA_BUS)		+= simple-fpga-bus.o
 
 # FPGA Manager Drivers
+obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
diff --git a/drivers/staging/fpga/socfpga.c b/drivers/staging/fpga/socfpga.c
new file mode 100644
index 0000000..38276f9
--- /dev/null
+++ b/drivers/staging/fpga/socfpga.c
@@ -0,0 +1,616 @@
+/*
+ * FPGA Manager Driver for Altera SOCFPGA
+ *
+ *  Copyright (C) 2013-2015 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/pm.h>
+#include <linux/string.h>
+
+/* Register offsets */
+#define SOCFPGA_FPGMGR_STAT_OFST				0x0
+#define SOCFPGA_FPGMGR_CTL_OFST					0x4
+#define SOCFPGA_FPGMGR_DCLKCNT_OFST				0x8
+#define SOCFPGA_FPGMGR_DCLKSTAT_OFST				0xc
+#define SOCFPGA_FPGMGR_GPIO_INTEN_OFST				0x830
+#define SOCFPGA_FPGMGR_GPIO_INTMSK_OFST				0x834
+#define SOCFPGA_FPGMGR_GPIO_INTTYPE_LEVEL_OFST			0x838
+#define SOCFPGA_FPGMGR_GPIO_INT_POL_OFST			0x83c
+#define SOCFPGA_FPGMGR_GPIO_INTSTAT_OFST			0x840
+#define SOCFPGA_FPGMGR_GPIO_RAW_INTSTAT_OFST			0x844
+#define SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST			0x84c
+#define SOCFPGA_FPGMGR_GPIO_EXT_PORTA_OFST			0x850
+
+/* Register bit defines */
+/* SOCFPGA_FPGMGR_STAT register mode field values */
+#define SOCFPGA_FPGMGR_STAT_POWER_UP				0x0 /*ramping*/
+#define SOCFPGA_FPGMGR_STAT_RESET				0x1
+#define SOCFPGA_FPGMGR_STAT_CFG					0x2
+#define SOCFPGA_FPGMGR_STAT_INIT				0x3
+#define SOCFPGA_FPGMGR_STAT_USER_MODE				0x4
+#define SOCFPGA_FPGMGR_STAT_UNKNOWN				0x5
+#define SOCFPGA_FPGMGR_STAT_STATE_MASK				0x7
+/* This is a flag value that doesn't really happen in this register field */
+#define SOCFPGA_FPGMGR_STAT_POWER_OFF				0x0
+
+#define MSEL_PP16_FAST_NOAES_NODC				0x0
+#define MSEL_PP16_FAST_AES_NODC					0x1
+#define MSEL_PP16_FAST_AESOPT_DC				0x2
+#define MSEL_PP16_SLOW_NOAES_NODC				0x4
+#define MSEL_PP16_SLOW_AES_NODC					0x5
+#define MSEL_PP16_SLOW_AESOPT_DC				0x6
+#define MSEL_PP32_FAST_NOAES_NODC				0x8
+#define MSEL_PP32_FAST_AES_NODC					0x9
+#define MSEL_PP32_FAST_AESOPT_DC				0xa
+#define MSEL_PP32_SLOW_NOAES_NODC				0xc
+#define MSEL_PP32_SLOW_AES_NODC					0xd
+#define MSEL_PP32_SLOW_AESOPT_DC				0xe
+#define SOCFPGA_FPGMGR_STAT_MSEL_MASK				0x000000f8
+#define SOCFPGA_FPGMGR_STAT_MSEL_SHIFT				3
+
+/* SOCFPGA_FPGMGR_CTL register */
+#define SOCFPGA_FPGMGR_CTL_EN					0x00000001
+#define SOCFPGA_FPGMGR_CTL_NCE					0x00000002
+#define SOCFPGA_FPGMGR_CTL_NCFGPULL				0x00000004
+
+#define CDRATIO_X1						0x00000000
+#define CDRATIO_X2						0x00000040
+#define CDRATIO_X4						0x00000080
+#define CDRATIO_X8						0x000000c0
+#define SOCFPGA_FPGMGR_CTL_CDRATIO_MASK				0x000000c0
+
+#define SOCFPGA_FPGMGR_CTL_AXICFGEN				0x00000100
+
+#define CFGWDTH_16						0x00000000
+#define CFGWDTH_32						0x00000200
+#define SOCFPGA_FPGMGR_CTL_CFGWDTH_MASK				0x00000200
+
+/* SOCFPGA_FPGMGR_DCLKSTAT register */
+#define SOCFPGA_FPGMGR_DCLKSTAT_DCNTDONE_E_DONE			0x1
+
+/* SOCFPGA_FPGMGR_GPIO_* registers share the same bit positions */
+#define SOCFPGA_FPGMGR_MON_NSTATUS				0x0001
+#define SOCFPGA_FPGMGR_MON_CONF_DONE				0x0002
+#define SOCFPGA_FPGMGR_MON_INIT_DONE				0x0004
+#define SOCFPGA_FPGMGR_MON_CRC_ERROR				0x0008
+#define SOCFPGA_FPGMGR_MON_CVP_CONF_DONE			0x0010
+#define SOCFPGA_FPGMGR_MON_PR_READY				0x0020
+#define SOCFPGA_FPGMGR_MON_PR_ERROR				0x0040
+#define SOCFPGA_FPGMGR_MON_PR_DONE				0x0080
+#define SOCFPGA_FPGMGR_MON_NCONFIG_PIN				0x0100
+#define SOCFPGA_FPGMGR_MON_NSTATUS_PIN				0x0200
+#define SOCFPGA_FPGMGR_MON_CONF_DONE_PIN			0x0400
+#define SOCFPGA_FPGMGR_MON_FPGA_POWER_ON			0x0800
+#define SOCFPGA_FPGMGR_MON_STATUS_MASK				0x0fff
+
+#define SOCFPGA_FPGMGR_NUM_SUPPLIES 3
+#define SOCFPGA_RESUME_TIMEOUT 3
+
+/* In power-up order. Reverse for power-down. */
+static const char *supply_names[SOCFPGA_FPGMGR_NUM_SUPPLIES] __maybe_unused = {
+	"FPGA-1.5V",
+	"FPGA-1.1V",
+	"FPGA-2.5V",
+};
+
+struct socfpga_fpga_priv {
+	void __iomem *fpga_base_addr;
+	void __iomem *fpga_data_addr;
+	struct completion status_complete;
+	int irq;
+};
+
+struct cfgmgr_mode {
+	/* Values to set in the CTRL register */
+	u32 ctrl;
+
+	/* flag that this table entry is a valid mode */
+	bool valid;
+};
+
+/* For SOCFPGA_FPGMGR_STAT_MSEL field */
+static struct cfgmgr_mode cfgmgr_modes[] = {
+	[MSEL_PP16_FAST_NOAES_NODC] = { CFGWDTH_16 | CDRATIO_X1, 1 },
+	[MSEL_PP16_FAST_AES_NODC] =   { CFGWDTH_16 | CDRATIO_X2, 1 },
+	[MSEL_PP16_FAST_AESOPT_DC] =  { CFGWDTH_16 | CDRATIO_X4, 1 },
+	[MSEL_PP16_SLOW_NOAES_NODC] = { CFGWDTH_16 | CDRATIO_X1, 1 },
+	[MSEL_PP16_SLOW_AES_NODC] =   { CFGWDTH_16 | CDRATIO_X2, 1 },
+	[MSEL_PP16_SLOW_AESOPT_DC] =  { CFGWDTH_16 | CDRATIO_X4, 1 },
+	[MSEL_PP32_FAST_NOAES_NODC] = { CFGWDTH_32 | CDRATIO_X1, 1 },
+	[MSEL_PP32_FAST_AES_NODC] =   { CFGWDTH_32 | CDRATIO_X4, 1 },
+	[MSEL_PP32_FAST_AESOPT_DC] =  { CFGWDTH_32 | CDRATIO_X8, 1 },
+	[MSEL_PP32_SLOW_NOAES_NODC] = { CFGWDTH_32 | CDRATIO_X1, 1 },
+	[MSEL_PP32_SLOW_AES_NODC] =   { CFGWDTH_32 | CDRATIO_X4, 1 },
+	[MSEL_PP32_SLOW_AESOPT_DC] =  { CFGWDTH_32 | CDRATIO_X8, 1 },
+};
+
+static u32 socfpga_fpga_readl(struct socfpga_fpga_priv *priv, u32 reg_offset)
+{
+	return readl(priv->fpga_base_addr + reg_offset);
+}
+
+static void socfpga_fpga_writel(struct socfpga_fpga_priv *priv, u32 reg_offset,
+				u32 value)
+{
+	writel(value, priv->fpga_base_addr + reg_offset);
+}
+
+static u32 socfpga_fpga_raw_readl(struct socfpga_fpga_priv *priv,
+				  u32 reg_offset)
+{
+	return __raw_readl(priv->fpga_base_addr + reg_offset);
+}
+
+static void socfpga_fpga_raw_writel(struct socfpga_fpga_priv *priv,
+				    u32 reg_offset, u32 value)
+{
+	__raw_writel(value, priv->fpga_base_addr + reg_offset);
+}
+
+static void socfpga_fpga_data_writel(struct socfpga_fpga_priv *priv, u32 value)
+{
+	writel(value, priv->fpga_data_addr);
+}
+
+static inline void socfpga_fpga_set_bitsl(struct socfpga_fpga_priv *priv,
+					  u32 offset, u32 bits)
+{
+	u32 val;
+
+	val = socfpga_fpga_readl(priv, offset);
+	val |= bits;
+	socfpga_fpga_writel(priv, offset, val);
+}
+
+static inline void socfpga_fpga_clr_bitsl(struct socfpga_fpga_priv *priv,
+					  u32 offset, u32 bits)
+{
+	u32 val;
+
+	val = socfpga_fpga_readl(priv, offset);
+	val &= ~bits;
+	socfpga_fpga_writel(priv, offset, val);
+}
+
+static u32 socfpga_fpga_mon_status_get(struct socfpga_fpga_priv *priv)
+{
+	return socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_GPIO_EXT_PORTA_OFST) &
+		SOCFPGA_FPGMGR_MON_STATUS_MASK;
+}
+
+static u32 socfpga_fpga_state_get(struct socfpga_fpga_priv *priv)
+{
+	u32 status = socfpga_fpga_mon_status_get(priv);
+
+	if ((status & SOCFPGA_FPGMGR_MON_FPGA_POWER_ON) == 0)
+		return SOCFPGA_FPGMGR_STAT_POWER_OFF;
+
+	return socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_STAT_OFST) &
+		SOCFPGA_FPGMGR_STAT_STATE_MASK;
+}
+
+static void socfpga_fpga_clear_done_status(struct socfpga_fpga_priv *priv)
+{
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_DCLKSTAT_OFST,
+			    SOCFPGA_FPGMGR_DCLKSTAT_DCNTDONE_E_DONE);
+}
+
+/*
+ * Set the DCLKCNT, wait for DCLKSTAT to report the count completed, and clear
+ * the complete status.
+ */
+static int socfpga_fpga_dclk_set_and_wait_clear(struct socfpga_fpga_priv *priv,
+						u32 count)
+{
+	int timeout = 2;
+	u32 done;
+
+	/* Clear any existing DONE status. */
+	if (socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_DCLKSTAT_OFST))
+		socfpga_fpga_clear_done_status(priv);
+
+	/* Issue the DCLK count. */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_DCLKCNT_OFST, count);
+
+	/* Poll DCLKSTAT to see if it completed in the timeout period. */
+	do {
+		done = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_DCLKSTAT_OFST);
+		if (done == SOCFPGA_FPGMGR_DCLKSTAT_DCNTDONE_E_DONE) {
+			socfpga_fpga_clear_done_status(priv);
+			return 0;
+		}
+		udelay(1);
+	} while (timeout--);
+
+	return -ETIMEDOUT;
+}
+
+static int socfpga_fpga_wait_for_state(struct socfpga_fpga_priv *priv,
+				       u32 state)
+{
+	int timeout = 2;
+
+	/*
+	 * HW doesn't support an interrupt for changes in state, so poll to see
+	 * if it matches the requested state within the timeout period.
+	 */
+	do {
+		if ((socfpga_fpga_state_get(priv) & state) != 0)
+			return 0;
+		msleep(20);
+	} while (timeout--);
+
+	return -ETIMEDOUT;
+}
+
+static void socfpga_fpga_enable_irqs(struct socfpga_fpga_priv *priv, u32 irqs)
+{
+	/* set irqs to level sensitive */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTTYPE_LEVEL_OFST, 0);
+
+	/* set interrupt polarity */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INT_POL_OFST, irqs);
+
+	/* clear irqs */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST, irqs);
+
+	/* unmask interrupts */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTMSK_OFST, 0);
+
+	/* enable interrupts */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTEN_OFST, irqs);
+}
+
+static void socfpga_fpga_disable_irqs(struct socfpga_fpga_priv *priv)
+{
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTEN_OFST, 0);
+}
+
+static irqreturn_t socfpga_fpga_isr(int irq, void *dev_id)
+{
+	struct socfpga_fpga_priv *priv = dev_id;
+	u32 irqs, st;
+	bool conf_done, nstatus;
+
+	/* clear irqs */
+	irqs = socfpga_fpga_raw_readl(priv, SOCFPGA_FPGMGR_GPIO_INTSTAT_OFST);
+
+	socfpga_fpga_raw_writel(priv, SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST, irqs);
+
+	st = socfpga_fpga_raw_readl(priv, SOCFPGA_FPGMGR_GPIO_EXT_PORTA_OFST);
+	conf_done = (st & SOCFPGA_FPGMGR_MON_CONF_DONE) != 0;
+	nstatus = (st & SOCFPGA_FPGMGR_MON_NSTATUS) != 0;
+
+	/* success */
+	if (conf_done && nstatus) {
+		/* disable irqs */
+		socfpga_fpga_raw_writel(priv,
+					SOCFPGA_FPGMGR_GPIO_INTEN_OFST, 0);
+		complete(&priv->status_complete);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int socfpga_fpga_wait_for_config_done(struct socfpga_fpga_priv *priv)
+{
+	int timeout, ret = 0;
+
+	socfpga_fpga_disable_irqs(priv);
+	init_completion(&priv->status_complete);
+	socfpga_fpga_enable_irqs(priv, SOCFPGA_FPGMGR_MON_CONF_DONE);
+
+	timeout = wait_for_completion_interruptible_timeout(
+						&priv->status_complete,
+						msecs_to_jiffies(10));
+	if (timeout == 0)
+		ret = -ETIMEDOUT;
+
+	socfpga_fpga_disable_irqs(priv);
+	return ret;
+}
+
+static int socfpga_fpga_cfg_mode_get(struct socfpga_fpga_priv *priv)
+{
+	u32 msel;
+
+	msel = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_STAT_OFST);
+	msel &= SOCFPGA_FPGMGR_STAT_MSEL_MASK;
+	msel >>= SOCFPGA_FPGMGR_STAT_MSEL_SHIFT;
+
+	/* Check that this MSEL setting is supported */
+	if ((msel >= ARRAY_SIZE(cfgmgr_modes)) || !cfgmgr_modes[msel].valid)
+		return -EINVAL;
+
+	return msel;
+}
+
+static int socfpga_fpga_cfg_mode_set(struct socfpga_fpga_priv *priv)
+{
+	u32 ctrl_reg;
+	int mode;
+
+	/* get value from MSEL pins */
+	mode = socfpga_fpga_cfg_mode_get(priv);
+	if (mode < 0)
+		return mode;
+
+	/* Adjust CTRL for the CDRATIO */
+	ctrl_reg = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_CTL_OFST);
+	ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_CDRATIO_MASK;
+	ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_CFGWDTH_MASK;
+	ctrl_reg |= cfgmgr_modes[mode].ctrl;
+
+	/* Set NCE to 0. */
+	ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_NCE;
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_CTL_OFST, ctrl_reg);
+
+	return 0;
+}
+
+static int socfpga_fpga_reset(struct fpga_manager *mgr)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	u32 ctrl_reg, status;
+	int ret;
+
+	/*
+	 * Step 1:
+	 *  - Set CTRL.CFGWDTH, CTRL.CDRATIO to match cfg mode
+	 *  - Set CTRL.NCE to 0
+	 */
+	ret = socfpga_fpga_cfg_mode_set(priv);
+	if (ret)
+		return ret;
+
+	/* Step 2: Set CTRL.EN to 1 */
+	socfpga_fpga_set_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
+			       SOCFPGA_FPGMGR_CTL_EN);
+
+	/* Step 3: Set CTRL.NCONFIGPULL to 1 to put FPGA in reset */
+	ctrl_reg = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_CTL_OFST);
+	ctrl_reg |= SOCFPGA_FPGMGR_CTL_NCFGPULL;
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_CTL_OFST, ctrl_reg);
+
+	/* Step 4: Wait for STATUS.MODE to report FPGA is in reset phase */
+	status = socfpga_fpga_wait_for_state(priv, SOCFPGA_FPGMGR_STAT_RESET);
+
+	/* Step 5: Set CONTROL.NCONFIGPULL to 0 to release FPGA from reset */
+	ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_NCFGPULL;
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_CTL_OFST, ctrl_reg);
+
+	/* Timeout waiting for reset */
+	if (status)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+/*
+ * Prepare the FPGA to receive the configuration data.
+ */
+static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
+					   const char *buf, size_t count)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	int ret;
+
+	if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
+		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
+		return -EINVAL;
+	}
+	/* Steps 1 - 5: Reset the FPGA */
+	ret = socfpga_fpga_reset(mgr);
+	if (ret)
+		return ret;
+
+	/* Step 6: Wait for FPGA to enter configuration phase */
+	if (socfpga_fpga_wait_for_state(priv, SOCFPGA_FPGMGR_STAT_CFG))
+		return -ETIMEDOUT;
+
+	/* Step 7: Clear nSTATUS interrupt */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST,
+			    SOCFPGA_FPGMGR_MON_NSTATUS);
+
+	/* Step 8: Set CTRL.AXICFGEN to 1 to enable transfer of config data */
+	socfpga_fpga_set_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
+			       SOCFPGA_FPGMGR_CTL_AXICFGEN);
+
+	return 0;
+}
+
+/*
+ * Step 9: write data to the FPGA data register
+ */
+static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr,
+					    const char *buf, size_t count)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	u32 *buffer_32 = (u32 *)buf;
+	size_t i = 0;
+
+	if (count <= 0)
+		return -EINVAL;
+
+	/* Write out the complete 32-bit chunks. */
+	while (count >= sizeof(u32)) {
+		socfpga_fpga_data_writel(priv, buffer_32[i++]);
+		count -= sizeof(u32);
+	}
+
+	/* Write out remaining non 32-bit chunks. */
+	switch (count) {
+	case 3:
+		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff);
+		break;
+	case 2:
+		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff);
+		break;
+	case 1:
+		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff);
+		break;
+	case 0:
+		break;
+	default:
+		/* This will never happen. */
+		return -EFAULT;
+	}
+
+	return 0;
+}
+
+static int socfpga_fpga_ops_configure_complete(struct fpga_manager *mgr)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	u32 status;
+
+	/*
+	 * Step 10:
+	 *  - Observe CONF_DONE and nSTATUS (active low)
+	 *  - if CONF_DONE = 1 and nSTATUS = 1, configuration was successful
+	 *  - if CONF_DONE = 0 and nSTATUS = 0, configuration failed
+	 */
+	status = socfpga_fpga_wait_for_config_done(priv);
+	if (status)
+		return status;
+
+	/* Step 11: Clear CTRL.AXICFGEN to disable transfer of config data */
+	socfpga_fpga_clr_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
+			       SOCFPGA_FPGMGR_CTL_AXICFGEN);
+
+	/*
+	 * Step 12:
+	 *  - Write 4 to DCLKCNT
+	 *  - Wait for STATUS.DCNTDONE = 1
+	 *  - Clear W1C bit in STATUS.DCNTDONE
+	 */
+	if (socfpga_fpga_dclk_set_and_wait_clear(priv, 4))
+		return -ETIMEDOUT;
+
+	/* Step 13: Wait for STATUS.MODE to report USER MODE */
+	if (socfpga_fpga_wait_for_state(priv, SOCFPGA_FPGMGR_STAT_USER_MODE))
+		return -ETIMEDOUT;
+
+	/* Step 14: Set CTRL.EN to 0 */
+	socfpga_fpga_clr_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
+			       SOCFPGA_FPGMGR_CTL_EN);
+
+	return 0;
+}
+
+/* Translate state register values to FPGA framework state */
+static const enum fpga_mgr_states socfpga_state_to_framework_state[] = {
+	[SOCFPGA_FPGMGR_STAT_POWER_OFF] = FPGA_MGR_STATE_POWER_OFF,
+	[SOCFPGA_FPGMGR_STAT_RESET] = FPGA_MGR_STATE_RESET,
+	[SOCFPGA_FPGMGR_STAT_CFG] = FPGA_MGR_STATE_WRITE_INIT,
+	[SOCFPGA_FPGMGR_STAT_INIT] = FPGA_MGR_STATE_WRITE_INIT,
+	[SOCFPGA_FPGMGR_STAT_USER_MODE] = FPGA_MGR_STATE_OPERATING,
+	[SOCFPGA_FPGMGR_STAT_UNKNOWN] = FPGA_MGR_STATE_UNKNOWN,
+};
+
+static enum fpga_mgr_states socfpga_fpga_ops_state(struct fpga_manager *mgr)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	enum fpga_mgr_states ret;
+	u32 state;
+
+	state = socfpga_fpga_state_get(priv);
+
+	if (state < ARRAY_SIZE(socfpga_state_to_framework_state))
+		ret = socfpga_state_to_framework_state[state];
+	else
+		ret = FPGA_MGR_STATE_UNKNOWN;
+
+	return ret;
+}
+
+static const struct fpga_manager_ops socfpga_fpga_ops = {
+	.state = socfpga_fpga_ops_state,
+	.write_init = socfpga_fpga_ops_configure_init,
+	.write = socfpga_fpga_ops_configure_write,
+	.write_complete = socfpga_fpga_ops_configure_complete,
+};
+
+static int socfpga_fpga_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct socfpga_fpga_priv *priv;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->fpga_base_addr = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->fpga_base_addr))
+		return PTR_ERR(priv->fpga_base_addr);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	priv->fpga_data_addr = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->fpga_data_addr))
+		return PTR_ERR(priv->fpga_data_addr);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0)
+		return priv->irq;
+
+	ret = devm_request_irq(dev, priv->irq, socfpga_fpga_isr, 0,
+			       dev_name(dev), priv);
+	if (IS_ERR_VALUE(ret))
+		return ret;
+
+	return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
+				 &socfpga_fpga_ops, priv);
+}
+
+static int socfpga_fpga_remove(struct platform_device *pdev)
+{
+	fpga_mgr_unregister(&pdev->dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id socfpga_fpga_of_match[] = {
+	{ .compatible = "altr,socfpga-fpga-mgr", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, socfpga_fpga_of_match);
+#endif
+
+static struct platform_driver socfpga_fpga_driver = {
+	.probe = socfpga_fpga_probe,
+	.remove = socfpga_fpga_remove,
+	.driver = {
+		.name	= "socfpga_fpga_manager",
+		.of_match_table = of_match_ptr(socfpga_fpga_of_match),
+	},
+};
+
+module_platform_driver(socfpga_fpga_driver);
+
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_DESCRIPTION("Altera SOCFPGA FPGA Manager");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v9 7/7] staging: fpga manager: add driver for socfpga fpga manager

[Moritz Fischer]

Alan,

it looks pretty good so far. I have worked with Michal and developed a
Zynq equivalent against your last
patchset which can be found in the Xilinx tree now.

I just briefly glanced the changes below just two nits that caught my eye.
I'll take a closer look while trying to update the zynq-fpga driver to
work with your changes.

On Fri, Jul 17, 2015 at 8:51 AM,  <atull@opensource.altera.com> wrote:
> From: Alan Tull <atull@opensource.altera.com>
>
> Add driver to fpga manager framework to allow configuration
> of FPGA in Altera SoCFPGA parts.
>
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> Acked-by: Michal Simek <michal.simek@xilinx.com>
> ---
> v2: fpga_manager struct now contains struct device
>     fpga_manager_register parameters now take device
>
> v3: skip a version to align versions
>
> v4: move to drivers/staging
>
> v5: fix array_size.cocci warnings
>     fix platform_no_drv_owner.cocci warnings
>     Remove .owner = THIS_MODULE
>     include asm/irq.h
>     clean up list of includes
>     use altera_fpga_reset for ops
>     use enum fpga_mgr_states or u32 as needed
>     use devm_request_irq
>     check irq <= 0 instead of == NO_IRQ
>     Use ARRAY_SIZE
>     rename altera -> socfpga
>     static const socfpga_fpga_ops
>     header moved to linux/fpga/ folder
>     remove ifdef'ed code
>     use platform_get_resource and platform_get_irq
>     move .probe and .remove lines adjacent
>     use module_platform_driver
>     use __maybe_unused
>     only need to 'if (IS_ENABLED(CONFIG_REGULATOR))' in one fn
>     fix "unsigned 'mode' is never < 0"
>
> v6: return error for (unused) default of case statement
>
> v7: PTR_ERR should access the value just tested by IS_ERR
>
> v8: change compatible string to be more chip specific
>
> v9: update copyright year
>     remove suspend/resume support and regulators
>     use updated fn names for register/unregister
>     use updated params for write_init
>     check for partial reconfiguration request
> ---
>  drivers/staging/fpga/Kconfig   |    6 +
>  drivers/staging/fpga/Makefile  |    1 +
>  drivers/staging/fpga/socfpga.c |  616 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 623 insertions(+)
>  create mode 100644 drivers/staging/fpga/socfpga.c
>
> diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
> index 8d003e3..36dd7b6 100644
> --- a/drivers/staging/fpga/Kconfig
> +++ b/drivers/staging/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>
>  if FPGA
>
> +config FPGA_MGR_SOCFPGA
> +       bool "Altera SOCFPGA FPGA Manager"
> +       depends on ARCH_SOCFPGA
> +       help
> +         FPGA manager driver support for Altera SOCFPGA.
> +
>  config SIMPLE_FPGA_BUS
>         bool "Simple FPGA Bus"
>         depends on OF
> diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
> index 6115213..d1f9406 100644
> --- a/drivers/staging/fpga/Makefile
> +++ b/drivers/staging/fpga/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_FPGA)                      += fpga-mgr.o
>  obj-$(CONFIG_SIMPLE_FPGA_BUS)          += simple-fpga-bus.o
>
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_SOCFPGA)         += socfpga.o
> diff --git a/drivers/staging/fpga/socfpga.c b/drivers/staging/fpga/socfpga.c
> new file mode 100644
> index 0000000..38276f9
> --- /dev/null
> +++ b/drivers/staging/fpga/socfpga.c
> @@ -0,0 +1,616 @@
> +/*
> + * FPGA Manager Driver for Altera SOCFPGA
> + *
> + *  Copyright (C) 2013-2015 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/pm.h>
As you removed the suspend / resume part, do you still need this?
> +#include <linux/string.h>
> +
> +/* Register offsets */
> +#define SOCFPGA_FPGMGR_STAT_OFST                               0x0
> +#define SOCFPGA_FPGMGR_CTL_OFST                                        0x4
> +#define SOCFPGA_FPGMGR_DCLKCNT_OFST                            0x8
> +#define SOCFPGA_FPGMGR_DCLKSTAT_OFST                           0xc
> +#define SOCFPGA_FPGMGR_GPIO_INTEN_OFST                         0x830
> +#define SOCFPGA_FPGMGR_GPIO_INTMSK_OFST                                0x834
> +#define SOCFPGA_FPGMGR_GPIO_INTTYPE_LEVEL_OFST                 0x838
> +#define SOCFPGA_FPGMGR_GPIO_INT_POL_OFST                       0x83c
> +#define SOCFPGA_FPGMGR_GPIO_INTSTAT_OFST                       0x840
> +#define SOCFPGA_FPGMGR_GPIO_RAW_INTSTAT_OFST                   0x844
> +#define SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST                     0x84c
> +#define SOCFPGA_FPGMGR_GPIO_EXT_PORTA_OFST                     0x850
> +
> +/* Register bit defines */
> +/* SOCFPGA_FPGMGR_STAT register mode field values */
> +#define SOCFPGA_FPGMGR_STAT_POWER_UP                           0x0 /*ramping*/
> +#define SOCFPGA_FPGMGR_STAT_RESET                              0x1
> +#define SOCFPGA_FPGMGR_STAT_CFG                                        0x2
> +#define SOCFPGA_FPGMGR_STAT_INIT                               0x3
> +#define SOCFPGA_FPGMGR_STAT_USER_MODE                          0x4
> +#define SOCFPGA_FPGMGR_STAT_UNKNOWN                            0x5
> +#define SOCFPGA_FPGMGR_STAT_STATE_MASK                         0x7
> +/* This is a flag value that doesn't really happen in this register field */
> +#define SOCFPGA_FPGMGR_STAT_POWER_OFF                          0x0
> +
> +#define MSEL_PP16_FAST_NOAES_NODC                              0x0
> +#define MSEL_PP16_FAST_AES_NODC                                        0x1
> +#define MSEL_PP16_FAST_AESOPT_DC                               0x2
> +#define MSEL_PP16_SLOW_NOAES_NODC                              0x4
> +#define MSEL_PP16_SLOW_AES_NODC                                        0x5
> +#define MSEL_PP16_SLOW_AESOPT_DC                               0x6
> +#define MSEL_PP32_FAST_NOAES_NODC                              0x8
> +#define MSEL_PP32_FAST_AES_NODC                                        0x9
> +#define MSEL_PP32_FAST_AESOPT_DC                               0xa
> +#define MSEL_PP32_SLOW_NOAES_NODC                              0xc
> +#define MSEL_PP32_SLOW_AES_NODC                                        0xd
> +#define MSEL_PP32_SLOW_AESOPT_DC                               0xe
> +#define SOCFPGA_FPGMGR_STAT_MSEL_MASK                          0x000000f8
> +#define SOCFPGA_FPGMGR_STAT_MSEL_SHIFT                         3
> +
> +/* SOCFPGA_FPGMGR_CTL register */
> +#define SOCFPGA_FPGMGR_CTL_EN                                  0x00000001
> +#define SOCFPGA_FPGMGR_CTL_NCE                                 0x00000002
> +#define SOCFPGA_FPGMGR_CTL_NCFGPULL                            0x00000004
> +
> +#define CDRATIO_X1                                             0x00000000
> +#define CDRATIO_X2                                             0x00000040
> +#define CDRATIO_X4                                             0x00000080
> +#define CDRATIO_X8                                             0x000000c0
> +#define SOCFPGA_FPGMGR_CTL_CDRATIO_MASK                                0x000000c0
> +
> +#define SOCFPGA_FPGMGR_CTL_AXICFGEN                            0x00000100
> +
> +#define CFGWDTH_16                                             0x00000000
> +#define CFGWDTH_32                                             0x00000200
> +#define SOCFPGA_FPGMGR_CTL_CFGWDTH_MASK                                0x00000200
> +
> +/* SOCFPGA_FPGMGR_DCLKSTAT register */
> +#define SOCFPGA_FPGMGR_DCLKSTAT_DCNTDONE_E_DONE                        0x1
> +
> +/* SOCFPGA_FPGMGR_GPIO_* registers share the same bit positions */
> +#define SOCFPGA_FPGMGR_MON_NSTATUS                             0x0001
> +#define SOCFPGA_FPGMGR_MON_CONF_DONE                           0x0002
> +#define SOCFPGA_FPGMGR_MON_INIT_DONE                           0x0004
> +#define SOCFPGA_FPGMGR_MON_CRC_ERROR                           0x0008
> +#define SOCFPGA_FPGMGR_MON_CVP_CONF_DONE                       0x0010
> +#define SOCFPGA_FPGMGR_MON_PR_READY                            0x0020
> +#define SOCFPGA_FPGMGR_MON_PR_ERROR                            0x0040
> +#define SOCFPGA_FPGMGR_MON_PR_DONE                             0x0080
> +#define SOCFPGA_FPGMGR_MON_NCONFIG_PIN                         0x0100
> +#define SOCFPGA_FPGMGR_MON_NSTATUS_PIN                         0x0200
> +#define SOCFPGA_FPGMGR_MON_CONF_DONE_PIN                       0x0400
> +#define SOCFPGA_FPGMGR_MON_FPGA_POWER_ON                       0x0800
> +#define SOCFPGA_FPGMGR_MON_STATUS_MASK                         0x0fff
> +
> +#define SOCFPGA_FPGMGR_NUM_SUPPLIES 3
> +#define SOCFPGA_RESUME_TIMEOUT 3
> +
> +/* In power-up order. Reverse for power-down. */
> +static const char *supply_names[SOCFPGA_FPGMGR_NUM_SUPPLIES] __maybe_unused = {
> +       "FPGA-1.5V",
> +       "FPGA-1.1V",
> +       "FPGA-2.5V",
> +};
> +
> +struct socfpga_fpga_priv {
> +       void __iomem *fpga_base_addr;
> +       void __iomem *fpga_data_addr;
> +       struct completion status_complete;
> +       int irq;
> +};
> +
> +struct cfgmgr_mode {
> +       /* Values to set in the CTRL register */
> +       u32 ctrl;
> +
> +       /* flag that this table entry is a valid mode */
> +       bool valid;
> +};
> +
> +/* For SOCFPGA_FPGMGR_STAT_MSEL field */
> +static struct cfgmgr_mode cfgmgr_modes[] = {
> +       [MSEL_PP16_FAST_NOAES_NODC] = { CFGWDTH_16 | CDRATIO_X1, 1 },
> +       [MSEL_PP16_FAST_AES_NODC] =   { CFGWDTH_16 | CDRATIO_X2, 1 },
> +       [MSEL_PP16_FAST_AESOPT_DC] =  { CFGWDTH_16 | CDRATIO_X4, 1 },
> +       [MSEL_PP16_SLOW_NOAES_NODC] = { CFGWDTH_16 | CDRATIO_X1, 1 },
> +       [MSEL_PP16_SLOW_AES_NODC] =   { CFGWDTH_16 | CDRATIO_X2, 1 },
> +       [MSEL_PP16_SLOW_AESOPT_DC] =  { CFGWDTH_16 | CDRATIO_X4, 1 },
> +       [MSEL_PP32_FAST_NOAES_NODC] = { CFGWDTH_32 | CDRATIO_X1, 1 },
> +       [MSEL_PP32_FAST_AES_NODC] =   { CFGWDTH_32 | CDRATIO_X4, 1 },
> +       [MSEL_PP32_FAST_AESOPT_DC] =  { CFGWDTH_32 | CDRATIO_X8, 1 },
> +       [MSEL_PP32_SLOW_NOAES_NODC] = { CFGWDTH_32 | CDRATIO_X1, 1 },
> +       [MSEL_PP32_SLOW_AES_NODC] =   { CFGWDTH_32 | CDRATIO_X4, 1 },
> +       [MSEL_PP32_SLOW_AESOPT_DC] =  { CFGWDTH_32 | CDRATIO_X8, 1 },
> +};
> +
> +static u32 socfpga_fpga_readl(struct socfpga_fpga_priv *priv, u32 reg_offset)
> +{
> +       return readl(priv->fpga_base_addr + reg_offset);
> +}
> +
> +static void socfpga_fpga_writel(struct socfpga_fpga_priv *priv, u32 reg_offset,
> +                               u32 value)
> +{
> +       writel(value, priv->fpga_base_addr + reg_offset);
> +}
> +
> +static u32 socfpga_fpga_raw_readl(struct socfpga_fpga_priv *priv,
> +                                 u32 reg_offset)
> +{
> +       return __raw_readl(priv->fpga_base_addr + reg_offset);
> +}
> +
> +static void socfpga_fpga_raw_writel(struct socfpga_fpga_priv *priv,
> +                                   u32 reg_offset, u32 value)
> +{
> +       __raw_writel(value, priv->fpga_base_addr + reg_offset);
> +}
> +
> +static void socfpga_fpga_data_writel(struct socfpga_fpga_priv *priv, u32 value)
> +{
> +       writel(value, priv->fpga_data_addr);
> +}
> +
> +static inline void socfpga_fpga_set_bitsl(struct socfpga_fpga_priv *priv,
> +                                         u32 offset, u32 bits)
> +{
> +       u32 val;
> +
> +       val = socfpga_fpga_readl(priv, offset);
> +       val |= bits;
> +       socfpga_fpga_writel(priv, offset, val);
> +}
> +
> +static inline void socfpga_fpga_clr_bitsl(struct socfpga_fpga_priv *priv,
> +                                         u32 offset, u32 bits)
> +{
> +       u32 val;
> +
> +       val = socfpga_fpga_readl(priv, offset);
> +       val &= ~bits;
> +       socfpga_fpga_writel(priv, offset, val);
> +}
> +
> +static u32 socfpga_fpga_mon_status_get(struct socfpga_fpga_priv *priv)
> +{
> +       return socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_GPIO_EXT_PORTA_OFST) &
> +               SOCFPGA_FPGMGR_MON_STATUS_MASK;
> +}
> +
> +static u32 socfpga_fpga_state_get(struct socfpga_fpga_priv *priv)
> +{
> +       u32 status = socfpga_fpga_mon_status_get(priv);
> +
> +       if ((status & SOCFPGA_FPGMGR_MON_FPGA_POWER_ON) == 0)
> +               return SOCFPGA_FPGMGR_STAT_POWER_OFF;
> +
> +       return socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_STAT_OFST) &
> +               SOCFPGA_FPGMGR_STAT_STATE_MASK;
> +}
> +
> +static void socfpga_fpga_clear_done_status(struct socfpga_fpga_priv *priv)
> +{
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_DCLKSTAT_OFST,
> +                           SOCFPGA_FPGMGR_DCLKSTAT_DCNTDONE_E_DONE);
> +}
> +
> +/*
> + * Set the DCLKCNT, wait for DCLKSTAT to report the count completed, and clear
> + * the complete status.
> + */
> +static int socfpga_fpga_dclk_set_and_wait_clear(struct socfpga_fpga_priv *priv,
> +                                               u32 count)
> +{
> +       int timeout = 2;
> +       u32 done;
> +
> +       /* Clear any existing DONE status. */
> +       if (socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_DCLKSTAT_OFST))
> +               socfpga_fpga_clear_done_status(priv);
> +
> +       /* Issue the DCLK count. */
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_DCLKCNT_OFST, count);
> +
> +       /* Poll DCLKSTAT to see if it completed in the timeout period. */
> +       do {
> +               done = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_DCLKSTAT_OFST);
> +               if (done == SOCFPGA_FPGMGR_DCLKSTAT_DCNTDONE_E_DONE) {
> +                       socfpga_fpga_clear_done_status(priv);
> +                       return 0;
> +               }
> +               udelay(1);
> +       } while (timeout--);
> +
> +       return -ETIMEDOUT;
> +}
> +
> +static int socfpga_fpga_wait_for_state(struct socfpga_fpga_priv *priv,
> +                                      u32 state)
> +{
> +       int timeout = 2;
> +
> +       /*
> +        * HW doesn't support an interrupt for changes in state, so poll to see
> +        * if it matches the requested state within the timeout period.
> +        */
> +       do {
> +               if ((socfpga_fpga_state_get(priv) & state) != 0)
> +                       return 0;
> +               msleep(20);
> +       } while (timeout--);
> +
> +       return -ETIMEDOUT;
> +}
> +
> +static void socfpga_fpga_enable_irqs(struct socfpga_fpga_priv *priv, u32 irqs)
> +{
> +       /* set irqs to level sensitive */
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTTYPE_LEVEL_OFST, 0);
> +
> +       /* set interrupt polarity */
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INT_POL_OFST, irqs);
> +
> +       /* clear irqs */
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST, irqs);
> +
> +       /* unmask interrupts */
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTMSK_OFST, 0);
> +
> +       /* enable interrupts */
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTEN_OFST, irqs);
> +}
> +
> +static void socfpga_fpga_disable_irqs(struct socfpga_fpga_priv *priv)
> +{
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTEN_OFST, 0);
> +}
> +
> +static irqreturn_t socfpga_fpga_isr(int irq, void *dev_id)
> +{
> +       struct socfpga_fpga_priv *priv = dev_id;
> +       u32 irqs, st;
> +       bool conf_done, nstatus;
> +
> +       /* clear irqs */
> +       irqs = socfpga_fpga_raw_readl(priv, SOCFPGA_FPGMGR_GPIO_INTSTAT_OFST);
> +
> +       socfpga_fpga_raw_writel(priv, SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST, irqs);
> +
> +       st = socfpga_fpga_raw_readl(priv, SOCFPGA_FPGMGR_GPIO_EXT_PORTA_OFST);
> +       conf_done = (st & SOCFPGA_FPGMGR_MON_CONF_DONE) != 0;
> +       nstatus = (st & SOCFPGA_FPGMGR_MON_NSTATUS) != 0;
> +
> +       /* success */
> +       if (conf_done && nstatus) {
> +               /* disable irqs */
> +               socfpga_fpga_raw_writel(priv,
> +                                       SOCFPGA_FPGMGR_GPIO_INTEN_OFST, 0);
> +               complete(&priv->status_complete);
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int socfpga_fpga_wait_for_config_done(struct socfpga_fpga_priv *priv)
> +{
> +       int timeout, ret = 0;
> +
> +       socfpga_fpga_disable_irqs(priv);
> +       init_completion(&priv->status_complete);
> +       socfpga_fpga_enable_irqs(priv, SOCFPGA_FPGMGR_MON_CONF_DONE);
> +
> +       timeout = wait_for_completion_interruptible_timeout(
> +                                               &priv->status_complete,
> +                                               msecs_to_jiffies(10));
> +       if (timeout == 0)
> +               ret = -ETIMEDOUT;
> +
> +       socfpga_fpga_disable_irqs(priv);
> +       return ret;
> +}
> +
> +static int socfpga_fpga_cfg_mode_get(struct socfpga_fpga_priv *priv)
> +{
> +       u32 msel;
> +
> +       msel = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_STAT_OFST);
> +       msel &= SOCFPGA_FPGMGR_STAT_MSEL_MASK;
> +       msel >>= SOCFPGA_FPGMGR_STAT_MSEL_SHIFT;
> +
> +       /* Check that this MSEL setting is supported */
> +       if ((msel >= ARRAY_SIZE(cfgmgr_modes)) || !cfgmgr_modes[msel].valid)
> +               return -EINVAL;
> +
> +       return msel;
> +}
> +
> +static int socfpga_fpga_cfg_mode_set(struct socfpga_fpga_priv *priv)
> +{
> +       u32 ctrl_reg;
> +       int mode;
> +
> +       /* get value from MSEL pins */
> +       mode = socfpga_fpga_cfg_mode_get(priv);
> +       if (mode < 0)
> +               return mode;
> +
> +       /* Adjust CTRL for the CDRATIO */
> +       ctrl_reg = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_CTL_OFST);
> +       ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_CDRATIO_MASK;
> +       ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_CFGWDTH_MASK;
> +       ctrl_reg |= cfgmgr_modes[mode].ctrl;
> +
> +       /* Set NCE to 0. */
> +       ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_NCE;
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_CTL_OFST, ctrl_reg);
> +
> +       return 0;
> +}
> +
> +static int socfpga_fpga_reset(struct fpga_manager *mgr)
> +{
> +       struct socfpga_fpga_priv *priv = mgr->priv;
> +       u32 ctrl_reg, status;
> +       int ret;
> +
> +       /*
> +        * Step 1:
> +        *  - Set CTRL.CFGWDTH, CTRL.CDRATIO to match cfg mode
> +        *  - Set CTRL.NCE to 0
> +        */
> +       ret = socfpga_fpga_cfg_mode_set(priv);
> +       if (ret)
> +               return ret;
> +
> +       /* Step 2: Set CTRL.EN to 1 */
> +       socfpga_fpga_set_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
> +                              SOCFPGA_FPGMGR_CTL_EN);
> +
> +       /* Step 3: Set CTRL.NCONFIGPULL to 1 to put FPGA in reset */
> +       ctrl_reg = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_CTL_OFST);
> +       ctrl_reg |= SOCFPGA_FPGMGR_CTL_NCFGPULL;
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_CTL_OFST, ctrl_reg);
> +
> +       /* Step 4: Wait for STATUS.MODE to report FPGA is in reset phase */
> +       status = socfpga_fpga_wait_for_state(priv, SOCFPGA_FPGMGR_STAT_RESET);
> +
> +       /* Step 5: Set CONTROL.NCONFIGPULL to 0 to release FPGA from reset */
> +       ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_NCFGPULL;
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_CTL_OFST, ctrl_reg);
> +
> +       /* Timeout waiting for reset */
> +       if (status)
> +               return -ETIMEDOUT;
> +
> +       return 0;
> +}
> +
> +/*
> + * Prepare the FPGA to receive the configuration data.
> + */
> +static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
> +                                          const char *buf, size_t count)
Is there a reason buf and count need to be passed here?
> +{
> +       struct socfpga_fpga_priv *priv = mgr->priv;
> +       int ret;
> +
> +       if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> +               dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
> +               return -EINVAL;
> +       }
> +       /* Steps 1 - 5: Reset the FPGA */
> +       ret = socfpga_fpga_reset(mgr);
> +       if (ret)
> +               return ret;
> +
> +       /* Step 6: Wait for FPGA to enter configuration phase */
> +       if (socfpga_fpga_wait_for_state(priv, SOCFPGA_FPGMGR_STAT_CFG))
> +               return -ETIMEDOUT;
> +
> +       /* Step 7: Clear nSTATUS interrupt */
> +       socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST,
> +                           SOCFPGA_FPGMGR_MON_NSTATUS);
> +
> +       /* Step 8: Set CTRL.AXICFGEN to 1 to enable transfer of config data */
> +       socfpga_fpga_set_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
> +                              SOCFPGA_FPGMGR_CTL_AXICFGEN);
> +
> +       return 0;
> +}
> +
> +/*
> + * Step 9: write data to the FPGA data register
> + */
> +static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr,
> +                                           const char *buf, size_t count)
> +{
> +       struct socfpga_fpga_priv *priv = mgr->priv;
> +       u32 *buffer_32 = (u32 *)buf;
> +       size_t i = 0;
> +
> +       if (count <= 0)
> +               return -EINVAL;
> +
> +       /* Write out the complete 32-bit chunks. */
> +       while (count >= sizeof(u32)) {
> +               socfpga_fpga_data_writel(priv, buffer_32[i++]);
> +               count -= sizeof(u32);
> +       }
> +
> +       /* Write out remaining non 32-bit chunks. */
> +       switch (count) {
> +       case 3:
> +               socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff);
> +               break;
> +       case 2:
> +               socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff);
> +               break;
> +       case 1:
> +               socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff);
> +               break;
> +       case 0:
> +               break;
> +       default:
> +               /* This will never happen. */
> +               return -EFAULT;
> +       }
> +
> +       return 0;
> +}
> +
> +static int socfpga_fpga_ops_configure_complete(struct fpga_manager *mgr)
> +{
> +       struct socfpga_fpga_priv *priv = mgr->priv;
> +       u32 status;
> +
> +       /*
> +        * Step 10:
> +        *  - Observe CONF_DONE and nSTATUS (active low)
> +        *  - if CONF_DONE = 1 and nSTATUS = 1, configuration was successful
> +        *  - if CONF_DONE = 0 and nSTATUS = 0, configuration failed
> +        */
> +       status = socfpga_fpga_wait_for_config_done(priv);
> +       if (status)
> +               return status;
> +
> +       /* Step 11: Clear CTRL.AXICFGEN to disable transfer of config data */
> +       socfpga_fpga_clr_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
> +                              SOCFPGA_FPGMGR_CTL_AXICFGEN);
> +
> +       /*
> +        * Step 12:
> +        *  - Write 4 to DCLKCNT
> +        *  - Wait for STATUS.DCNTDONE = 1
> +        *  - Clear W1C bit in STATUS.DCNTDONE
> +        */
> +       if (socfpga_fpga_dclk_set_and_wait_clear(priv, 4))
> +               return -ETIMEDOUT;
> +
> +       /* Step 13: Wait for STATUS.MODE to report USER MODE */
> +       if (socfpga_fpga_wait_for_state(priv, SOCFPGA_FPGMGR_STAT_USER_MODE))
> +               return -ETIMEDOUT;
> +
> +       /* Step 14: Set CTRL.EN to 0 */
> +       socfpga_fpga_clr_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
> +                              SOCFPGA_FPGMGR_CTL_EN);
> +
> +       return 0;
> +}
> +
> +/* Translate state register values to FPGA framework state */
> +static const enum fpga_mgr_states socfpga_state_to_framework_state[] = {
> +       [SOCFPGA_FPGMGR_STAT_POWER_OFF] = FPGA_MGR_STATE_POWER_OFF,
> +       [SOCFPGA_FPGMGR_STAT_RESET] = FPGA_MGR_STATE_RESET,
> +       [SOCFPGA_FPGMGR_STAT_CFG] = FPGA_MGR_STATE_WRITE_INIT,
> +       [SOCFPGA_FPGMGR_STAT_INIT] = FPGA_MGR_STATE_WRITE_INIT,
> +       [SOCFPGA_FPGMGR_STAT_USER_MODE] = FPGA_MGR_STATE_OPERATING,
> +       [SOCFPGA_FPGMGR_STAT_UNKNOWN] = FPGA_MGR_STATE_UNKNOWN,
> +};
> +
> +static enum fpga_mgr_states socfpga_fpga_ops_state(struct fpga_manager *mgr)
> +{
> +       struct socfpga_fpga_priv *priv = mgr->priv;
> +       enum fpga_mgr_states ret;
> +       u32 state;
> +
> +       state = socfpga_fpga_state_get(priv);
> +
> +       if (state < ARRAY_SIZE(socfpga_state_to_framework_state))
> +               ret = socfpga_state_to_framework_state[state];
> +       else
> +               ret = FPGA_MGR_STATE_UNKNOWN;
> +
> +       return ret;
> +}
> +
> +static const struct fpga_manager_ops socfpga_fpga_ops = {
> +       .state = socfpga_fpga_ops_state,
> +       .write_init = socfpga_fpga_ops_configure_init,
> +       .write = socfpga_fpga_ops_configure_write,
> +       .write_complete = socfpga_fpga_ops_configure_complete,
> +};
> +
> +static int socfpga_fpga_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct socfpga_fpga_priv *priv;
> +       struct resource *res;
> +       int ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       priv->fpga_base_addr = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(priv->fpga_base_addr))
> +               return PTR_ERR(priv->fpga_base_addr);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       priv->fpga_data_addr = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(priv->fpga_data_addr))
> +               return PTR_ERR(priv->fpga_data_addr);
> +
> +       priv->irq = platform_get_irq(pdev, 0);
> +       if (priv->irq < 0)
> +               return priv->irq;
> +
> +       ret = devm_request_irq(dev, priv->irq, socfpga_fpga_isr, 0,
> +                              dev_name(dev), priv);
> +       if (IS_ERR_VALUE(ret))
> +               return ret;
> +
> +       return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
> +                                &socfpga_fpga_ops, priv);
> +}
> +
> +static int socfpga_fpga_remove(struct platform_device *pdev)
> +{
> +       fpga_mgr_unregister(&pdev->dev);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id socfpga_fpga_of_match[] = {
> +       { .compatible = "altr,socfpga-fpga-mgr", },
> +       {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, socfpga_fpga_of_match);
> +#endif
> +
> +static struct platform_driver socfpga_fpga_driver = {
> +       .probe = socfpga_fpga_probe,
> +       .remove = socfpga_fpga_remove,
> +       .driver = {
> +               .name   = "socfpga_fpga_manager",
> +               .of_match_table = of_match_ptr(socfpga_fpga_of_match),
> +       },
> +};
> +
> +module_platform_driver(socfpga_fpga_driver);
> +
> +MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> +MODULE_DESCRIPTION("Altera SOCFPGA FPGA Manager");
> +MODULE_LICENSE("GPL v2");
> --
> 1.7.9.5
>
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Overall good job, and thanks for pushing this!

Cheers,

Moritz

Re: [PATCH v9 7/7] staging: fpga manager: add driver for socfpga fpga manager

[atull]

On Fri, 17 Jul 2015, Moritz Fischer wrote:

Hi Moritz,

> Alan,
> 
> it looks pretty good so far. I have worked with Michal and developed a
> Zynq equivalent against your last
> patchset which can be found in the Xilinx tree now.
> 
> I just briefly glanced the changes below just two nits that caught my eye.
> I'll take a closer look while trying to update the zynq-fpga driver to
> work with your changes.
> 
...
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/pm.h>
> As you removed the suspend / resume part, do you still need this?
> > +#include <linux/string.h>

Yep, I can take out this include.

> > +
> > +/*
> > + * Prepare the FPGA to receive the configuration data.
> > + */
> > +static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
> > +                                          const char *buf, size_t count)
> Is there a reason buf and count need to be passed here?
> > +{
> > +       struct socfpga_fpga_priv *priv = mgr->priv;
> > +       int ret;

Its to allow .write_init to look at the image header if it needs to.
Not every FPGA manager is going to need buf and count.  This one doesn't
(cyclone5).  Your .write_init can ignore them if you don't need them.

But Arria10 does (that's a separate driver that I didn't include in this
patchset). In that case I need to parse the image header to know whether
the bitstream is compressed, etc. to know how to configure the FPGA
manager registers before the FPGA can receive image data.

Thanks for reviewing!

Alan

> >
> > _______________________________________________
> > devel mailing list
> > devel@linuxdriverproject.org
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
> 
> Overall good job, and thanks for pushing this!
> 
> Cheers,
> 
> Moritz
> 

Re: [PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus

[Jason Gunthorpe]

On Fri, Jul 17, 2015 at 10:51:10AM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> This patchset adds two chunks plus documentation:
>  * fpga manager core: exports ABI functions that write an image to a FPGA
>  * DT Overlay support: simple-fpga-bus to handle FPGA from a DT overlay

I didn't read super closely, but overall it makes sense to me..

Providing an in-kernel API will let someone else figure out how to
expose that to user space. The DT based scheme seems pretty nice.

Can you use this without DT overlay? Ie if I provide the FGPA
description as part of my boot time DT will it just work?

Jason

Re: [PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus

[atull]

On Fri, 17 Jul 2015, Jason Gunthorpe wrote:

> On Fri, Jul 17, 2015 at 10:51:10AM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > This patchset adds two chunks plus documentation:
> >  * fpga manager core: exports ABI functions that write an image to a FPGA
> >  * DT Overlay support: simple-fpga-bus to handle FPGA from a DT overlay
> 
> I didn't read super closely, but overall it makes sense to me..
> 
> Providing an in-kernel API will let someone else figure out how to
> expose that to user space. The DT based scheme seems pretty nice.
> 

Thanks!

> Can you use this without DT overlay? Ie if I provide the FGPA
> description as part of my boot time DT will it just work?

The simple fpga bus would need to defer probing until after the fpga 
manager driver and bridge drivers are probed (that's easy).  Since it is 
using firmware, it will also have to defer until the filesystem is 
available so it can get the fpga image to load.  I'll work on it.

Alan

> 
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus

[atull]

On Fri, 17 Jul 2015, atull wrote:

> On Fri, 17 Jul 2015, Jason Gunthorpe wrote:
> 
> > On Fri, Jul 17, 2015 at 10:51:10AM -0500, atull@opensource.altera.com wrote:
> > > From: Alan Tull <atull@opensource.altera.com>
> > > 
> > > This patchset adds two chunks plus documentation:
> > >  * fpga manager core: exports ABI functions that write an image to a FPGA
> > >  * DT Overlay support: simple-fpga-bus to handle FPGA from a DT overlay
> > 
> > I didn't read super closely, but overall it makes sense to me..
> > 
> > Providing an in-kernel API will let someone else figure out how to
> > expose that to user space. The DT based scheme seems pretty nice.
> > 
> 
> Thanks!
> 
> > Can you use this without DT overlay? Ie if I provide the FGPA
> > description as part of my boot time DT will it just work?
> 
> The simple fpga bus would need to defer probing until after the fpga 
> manager driver and bridge drivers are probed (that's easy).  Since it is 
> using firmware, it will also have to defer until the filesystem is 
> available so it can get the fpga image to load.  I'll work on it.
> 
> Alan

I looked some more; I don't see a simple way of deferring probing until 
after the filesystem is loaded (so that the image file would be 
available), late_initcall is still not late enough.

Alan


> 
> > 
> > Jason
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus

[Jason Gunthorpe]

On Wed, Jul 22, 2015 at 03:32:32PM -0500, atull wrote:

> I looked some more; I don't see a simple way of deferring probing until 
> after the filesystem is loaded (so that the image file would be 
> available), late_initcall is still not late enough.

That seems weird, are you saying you can't use request firmware at all
from a compiled in driver? I don't know much about that flow, sorry.

Having that work would mean the system can have a reasonable in-tree
use case without requiring the out of tree dt overlay stuff.

Jason

Re: [PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus

[atull]

On Wed, 22 Jul 2015, Jason Gunthorpe wrote:

> On Wed, Jul 22, 2015 at 03:32:32PM -0500, atull wrote:
> 
> > I looked some more; I don't see a simple way of deferring probing until 
> > after the filesystem is loaded (so that the image file would be 
> > available), late_initcall is still not late enough.
> 
> That seems weird, are you saying you can't use request firmware at all
> from a compiled in driver? I don't know much about that flow, sorry.
> 
> Having that work would mean the system can have a reasonable in-tree
> use case without requiring the out of tree dt overlay stuff.
> 

It would work if the firmware is built into the kernel image.  It's
the same function call.

Alan

> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus

[Greg KH]

On Fri, Jul 17, 2015 at 10:51:10AM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> This patchset adds two chunks plus documentation:
>  * fpga manager core: exports ABI functions that write an image to a FPGA
>  * DT Overlay support: simple-fpga-bus to handle FPGA from a DT overlay
> 
> The core's ABI is minimal to start with: only 6 functions.  This gives a
> common interface for programming various FPGA such that any higher level
> interfaces such as the DT Overlays or anything else that is added can be
> shared and not be manufacturor-specific.
> 
> The DT Overlays support exists for the usage where the FPGA will contain
> some "hardware" that will need drivers.  Where that use model is not
> appealing, the core ABI can be used to add a different use model such as
> using an FPGA as acceleration as has been discussed.
> 
> This patchset gets rid of the sysfs controls that allowed direct
> control of a FPGA from userspace.
> 
> This patchset is under drivers/staging as the interface could change.
> 
> The bindings for the socpfga fpga manager already are upstreamed as
> 1b4e119 Alan Tull : doc: add bindings document for altera fpga manager
> 
> The DT Support is dependent on Pantelis's dtc overlay patches from
> https://github.com/pantoniou/dtc.git
> and his DT overlays configfs interface patches and fixes from
> https://github.com/pantoniou/linux-beagle-track-mainline
> 
> efb0c04 Pantelis Antoniou : gcl: Fix resource linking
> 85e785e Pantelis Antoniou : ARM: DT: Enable symbols when CONFIG_OF_OVERLAY is used
> af0321f Pantelis Antoniou : OF: DT-Overlay configfs interface (v5)
> 4c1c675 Pantelis Antoniou : configfs: Implement binary attributes (v4)
> 
> 
> Alan Tull (7):
>   staging: usage documentation for FPGA manager core
>   staging: usage documentation for simple fpga bus
>   staging: add bindings document for simple fpga bus
>   staging: fpga manager: add sysfs interface document
>   staging: fpga manager core
>   staging: add simple-fpga-bus
>   staging: fpga manager: add driver for socfpga fpga manager
> 
>  drivers/staging/Kconfig                            |    2 +
>  drivers/staging/Makefile                           |    1 +
>  .../Documentation/ABI/sysfs-class-fpga-manager     |   26 +
>  .../Documentation/bindings/simple-fpga-bus.txt     |   61 ++
>  drivers/staging/fpga/Documentation/fpga-mgr.txt    |  117 ++++
>  .../staging/fpga/Documentation/simple-fpga-bus.txt |   48 ++
>  drivers/staging/fpga/Kconfig                       |   31 +
>  drivers/staging/fpga/Makefile                      |   10 +
>  drivers/staging/fpga/fpga-mgr.c                    |  373 ++++++++++++
>  drivers/staging/fpga/simple-fpga-bus.c             |  323 ++++++++++
>  drivers/staging/fpga/socfpga.c                     |  616 ++++++++++++++++++++

All drivers/staging/*/ directories need a TODO file that lists what
needs to be done to it in order to get the code out of staging.  Please
redo the series and add that.

>  include/linux/fpga/fpga-mgr.h                      |  127 ++++

This should be within drivers/staging/ all staging code should be
self-contained.

Why isn't this going into the "real" part of the kernel?  Why staging?

thanks,

greg k-h

Re: [PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus

[atull]

On Thu, 23 Jul 2015, Greg KH wrote:

> On Fri, Jul 17, 2015 at 10:51:10AM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > This patchset adds two chunks plus documentation:
> >  * fpga manager core: exports ABI functions that write an image to a FPGA
> >  * DT Overlay support: simple-fpga-bus to handle FPGA from a DT overlay
> > 
> > The core's ABI is minimal to start with: only 6 functions.  This gives a
> > common interface for programming various FPGA such that any higher level
> > interfaces such as the DT Overlays or anything else that is added can be
> > shared and not be manufacturor-specific.
> > 
> > The DT Overlays support exists for the usage where the FPGA will contain
> > some "hardware" that will need drivers.  Where that use model is not
> > appealing, the core ABI can be used to add a different use model such as
> > using an FPGA as acceleration as has been discussed.
> > 
> > This patchset gets rid of the sysfs controls that allowed direct
> > control of a FPGA from userspace.
> > 
> > This patchset is under drivers/staging as the interface could change.
> > 
> > The bindings for the socpfga fpga manager already are upstreamed as
> > 1b4e119 Alan Tull : doc: add bindings document for altera fpga manager
> > 
> > The DT Support is dependent on Pantelis's dtc overlay patches from
> > https://github.com/pantoniou/dtc.git
> > and his DT overlays configfs interface patches and fixes from
> > https://github.com/pantoniou/linux-beagle-track-mainline
> > 
> > efb0c04 Pantelis Antoniou : gcl: Fix resource linking
> > 85e785e Pantelis Antoniou : ARM: DT: Enable symbols when CONFIG_OF_OVERLAY is used
> > af0321f Pantelis Antoniou : OF: DT-Overlay configfs interface (v5)
> > 4c1c675 Pantelis Antoniou : configfs: Implement binary attributes (v4)
> > 
> > 
> > Alan Tull (7):
> >   staging: usage documentation for FPGA manager core
> >   staging: usage documentation for simple fpga bus
> >   staging: add bindings document for simple fpga bus
> >   staging: fpga manager: add sysfs interface document
> >   staging: fpga manager core
> >   staging: add simple-fpga-bus
> >   staging: fpga manager: add driver for socfpga fpga manager
> > 
> >  drivers/staging/Kconfig                            |    2 +
> >  drivers/staging/Makefile                           |    1 +
> >  .../Documentation/ABI/sysfs-class-fpga-manager     |   26 +
> >  .../Documentation/bindings/simple-fpga-bus.txt     |   61 ++
> >  drivers/staging/fpga/Documentation/fpga-mgr.txt    |  117 ++++
> >  .../staging/fpga/Documentation/simple-fpga-bus.txt |   48 ++
> >  drivers/staging/fpga/Kconfig                       |   31 +
> >  drivers/staging/fpga/Makefile                      |   10 +
> >  drivers/staging/fpga/fpga-mgr.c                    |  373 ++++++++++++
> >  drivers/staging/fpga/simple-fpga-bus.c             |  323 ++++++++++
> >  drivers/staging/fpga/socfpga.c                     |  616 ++++++++++++++++++++
> 
> All drivers/staging/*/ directories need a TODO file that lists what
> needs to be done to it in order to get the code out of staging.  Please
> redo the series and add that.
> 
> >  include/linux/fpga/fpga-mgr.h                      |  127 ++++
> 
> This should be within drivers/staging/ all staging code should be
> self-contained.
> 
> Why isn't this going into the "real" part of the kernel?  Why staging?
> 

Hi Greg,

For v10 next week, I will likely break this into two patchsets, one for the
real kernel (drivers/fpga) and one for staging.

fpga-mgr.c can go into drivers/fpga since both Xilinx and Altera have
already been using this code.  It's not likely to change much.

The part that should go into staging is whatever interface is
controversial, that may change.  That's simple-fpga-bus.c and any
other interfaces that get added that use the functions exported by
fpga-mgr.c.  Maybe this 2nd patch set should be a RFC since it is still
dependent on some of Pantelis' stuff that's not in yet.

Alan Tull

> thanks,
> 
> greg k-h
>