[PATCH v16 0/6] Device Tree support for FPGA programming

[PATCH v16 0/6] Device Tree support for FPGA programming

[atull]

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

v16 Refactors the FPGA Area and FPGA Bus into single thing called an
FPGA Region and eliminates using simple-bus.  I'm using the word
"region" as it's a term is used in the literature of both the major
FPGA manufacturors.

Changes for v16:
* Refactor the FPGA Area and FPGA Bus into a FPGA Region.
* Don't use simple-bus.
* FPGA Managers and FPGA Bridges are now specified by phandle using the 
  "fpga-mgr" and "fpga-bridges" properties.  fpga-bridges can specify
  more than one bridge.
* Device Tree overlays should be targeted to a FPGA Region.
* The overlays need only contain firmware-name and the child nodes.
* To model a system containing >1 partial reconfiguration region,
  an overlay could add FPGA Regions to the base FPGA Regions.
* Child FPGA Regions inherit the parent FGPA Manager, but specify
  their own set of bridges if needes as partial reconfig regions
  will likely need their own bridges.
* All this is discussed in bindings/fpga/fpga-region.txt

One other highlight:
The little engine that runs this thing is a reconfig notifier
in fpga-region.c.  This notifier that will program an FPGA if a
"firmware-name" property gets added to a fpga-region.  Then
it will call of_platform_populate().  The current behavior in Linux
when a DT overlay is applied is that the reconfig notifications
go out in heirarchical order: first notifications are for the
properties, then notifications for the child nodes.  So an overlay
that adds a 'firmware-name' property and some child nodes to a
fpga-region will cause FPGA programming and child node
populating in the right order.

One issue with the dynamic DT stuff:
I've tried returning and error from the notifier if FPGA programming
fails; the error is noted on the console, but the child nodes
get probed anyway.


Alan Tull (6):
  fpga: add bindings document for fpga region
  add sysfs document for fpga bridge class
  ARM: socfpga: add bindings document for fpga bridge drivers
  fpga: add fpga bridge framework
  fpga: fpga-region: device tree control for FPGA
  ARM: socfpga: fpga bridge driver support

 Documentation/ABI/testing/sysfs-class-fpga-bridge  |   11 +
 .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   15 +
 .../bindings/fpga/altera-hps2fpga-bridge.txt       |   47 ++
 .../devicetree/bindings/fpga/fpga-region.txt       |  348 +++++++++++++++
 drivers/fpga/Kconfig                               |   21 +
 drivers/fpga/Makefile                              |    7 +
 drivers/fpga/altera-fpga2sdram.c                   |  174 ++++++++
 drivers/fpga/altera-hps2fpga.c                     |  213 +++++++++
 drivers/fpga/fpga-bridge.c                         |  388 +++++++++++++++++
 drivers/fpga/fpga-region.c                         |  460 ++++++++++++++++++++
 include/linux/fpga/fpga-bridge.h                   |   55 +++
 11 files changed, 1739 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-bridge
 create mode 100644 Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
 create mode 100644 Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
 create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
 create mode 100644 drivers/fpga/altera-fpga2sdram.c
 create mode 100644 drivers/fpga/altera-hps2fpga.c
 create mode 100644 drivers/fpga/fpga-bridge.c
 create mode 100644 drivers/fpga/fpga-region.c
 create mode 100644 include/linux/fpga/fpga-bridge.h

-- 
1.7.9.5

[PATCH v16 1/6] fpga: add bindings document for fpga region

[atull]

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

New bindings document for FPGA Region to support programming
FPGA's under Device Tree control

Signed-off-by: Alan Tull <atull@opensource.altera.com>
Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
v9:  initial version added to this patchset
v10: s/fpga/FPGA/g
     replace DT overlay example with slightly more complicated example
     move to staging/simple-fpga-bus
v11: No change in this patch for v11 of the patch set
v12: Moved out of staging.
     Changed to use FPGA bridges framework instead of resets
     for bridges.
v13: bridge@0xff20000 -> bridge@ff200000, etc
     Leave out directly talking about overlays
     Remove regs and clocks directly under simple-fpga-bus in example
     Use common "firmware-name" binding instead of "fpga-firmware"
v14: Use firmware-name in bindings description
     Call it FPGA Area
     Remove bindings that specify FPGA Manager and FPGA Bridges
v15: Cleanup as per Rob's comments
     Combine usage doc with bindings document
     Document as being Altera specific
     Additions and changes to add FPGA Bus
v16: Reworked to document FPGA Regions
     rename altera-fpga-bus-fpga-area.txt -> fpga-region.txt
     Remove references that made it sound exclusive to Altera
     Remove altr, prefix from fpga-bus and fpga-area compatible strings
     Added Moritz' usage example with Xilinx
     Cleaned up unit addresses
---
 .../devicetree/bindings/fpga/fpga-region.txt       |  348 ++++++++++++++++++++
 1 file changed, 348 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt

diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
new file mode 100644
index 0000000..ccd7127
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
@@ -0,0 +1,348 @@
+FPGA Region Device Tree Binding
+
+Alan Tull 2016
+
+ CONTENTS
+ - Introduction
+ - Terminology
+ - Overview
+ - Constraints
+ - FPGA Region
+ - Supported Use Models
+ - Sequence
+ - Device Tree Examples
+
+
+Introduction
+============
+
+FPGA Regions are introduced as a way to solve the problem of how to program an
+FPGA under an operating system and have the new hardware show up in the device
+tree.  By adding these bindings to the Device Tree, a system can have the
+information needed to program the FPGA and add the desired hardware, and also
+the information about the devices to be added to the Device Tree once the
+programming has succeeded.
+
+
+Terminology
+===========
+
+Full Reconfiguration
+ * The entire FPGA is programmed.
+
+Partial Reconfiguration (PR)
+Partial Reconfiguration Region (PRR)
+ * The FPGA is divided into regions.  Each of these regions can be programmed
+   while the rest of the FPGA is not affected.  Not all FPGA's support this.
+ * PRR's may vary in size and in the connections at their edge.  The image
+   that is loaded into a PRR must fit and must use a subset of the region's
+   connections.
+
+Base image
+ * An FPGA image that is designed to do full reconfiguration of the FPGA.
+ * A base image may set up a set of regions to allow for partial
+   reconfiguration.
+
+Persona
+ * An FPGA image that is designed to be loaded into a PRR.  There may be
+   any number of personas designed to fit into a PRR, but only one at at time
+   may be loaded.
+ * A persona may create more regions.
+
+FPGA Manager & FPGA Manager Framework
+ * An FPGA Manager is a hardware block that programs an FPGA under the control
+   of a host processor.
+ * The FPGA Manager Framework provides drivers and functions to program an
+   FPGA.
+
+FPGA Bridge Framework
+ * Provides drivers and functions to control bridges that enable/disable
+   data to the FPGA.
+ * FPGA Bridges should be disabled while the FPGA is being programmed to
+   prevent spurious data on the bus.
+ * FPGA Bridges may not be needed in implementations where the FPGA Manager
+   handles this.
+
+Freeze Blocks
+ * Freeze Blocks function as FPGA Bridges within the FPGA fabric.  In the case
+   of PR, the buses from the processor are split within the FPGA.  Each PR
+   region gets its own split of the buses, protected by an independently
+   controlled Freeze Block.  Several busses may be connected to a single
+   PR region; a Freeze Block controls the traffic of all these busses
+   together.
+
+
+Overview
+========
+
+This binding introduces the FPGA Region.
+
+An FPGA Region references the devices needed to be able to program an FPGA
+device.  The base FPGA Region in the device tree is required to include a
+property with a phandle to an FPGA Manager.  This region may also contain a
+property that has a list of FPGA Bridge phandles, if needed.  Child FPGA Regions
+inherit the parent's FPGA Manager but specify their own bridges.
+
+The base FPGA Region supports full reconfiguration of the FPGA device.  If the
+FPGA image loaded contains the logic that creates a set of Partial
+Reconfiguration Regions, then the DT that programs the FPGA should also add a
+set of FPGA Regions as children of the original FPGA Region.
+The child FPGA Regions do not require specifying an FPGA Manager as they will
+use the ancestor region's FPGA Manager.
+
+The FPGA Manager is the hardware block that handles programming the FPGA.  FPGA
+Bridges function to prevent spurious data from the FPGA going on the processor
+busses during FPGA programming.  In the case of partial reconfiguration,
+additional bridges (Freeze Blocks) for each reconfiguration region are required.
+
+The intended use is that device tree overlays can be used to add hardware to an
+FPGA while an operating system is running.  In that case, the live device tree
+will contain an FPGA Manager, (possibly) FPGA Bridges, and the base FPGA Region.
+The device tree overlays contain the name of the FPGA image file to be
+programmed and the child devices that will be contained in the FPGA after
+programming.  Applying such an overlay will cause the FPGA to be programmed and
+the child devices to be populated.  Removing an overlay will cause the devices
+to be removed from the device tree and free up those FPGA resources.
+
+
+Constraints
+===========
+
+It is beyond the scope of this document to fully describe all the FPGA design
+constraints required to make partial reconfiguration work[1] [2], but a few
+deserve quick mention.  A partial reconfiguration FPGA image must have
+boundary connections that line up with those the current programming of the
+FPGA.  Also, those during programming, those connections must be frozen.  This
+can be achieved by "Freeze Blocks" which are FPGA Bridges that exist on the FPGA
+fabric prior to the partial reconfiguration.
+
+
+FPGA Region
+===========
+
+An FPGA Region specifies the devices needed to reconfigure a FPGA device.
+
+Required properties:
+- compatible   : should contain "fpga-region"
+- fpga-mgr     : should contain a phandle to an FPGA Manager.  Child FPGA
+		 Regions inherit this property from the parent, so it
+		 should be left out for any child FPGA Regions.
+- fpga-bridges : should contain a list of phandles to FPGA Bridges.  This
+		 property is optional if the FPGA Manager controls the
+		 bridges during reprogramming.
+- #address-cells, #size-cells, ranges: must be present to handle address space
+  mapping for children.
+
+Properties added in an overlay:
+- firmware-name : should contain the name of an FPGA image file located on the
+  firmware search path.
+- partial-reconfig : boolean property should be defined if partial
+  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
+  is done.
+- child nodes : devices in the FPGA after programming.
+
+In the example below, fpgamgr@ff706000 is used to program the FPGA.  The two
+bridges specified would be disabled during the programming and re-enabled
+afterwards if the programming succeeds.  The FPGA would be programmed with the
+image contained in the "soc_system.rbf" specified in the overlay.  Assuming
+programming succeeds, the child devices would be populated afterwords.
+
+Example:
+Base tree contains:
+
+	fpgamgr0: fpgamgr@ff706000 {
+		compatible = "altr,socfpga-fpga-mgr";
+		reg = <0xff706000 0x1000
+		       0xffb90000 0x1000>;
+		interrupts = <0 175 4>;
+	};
+
+	bridge0: bridge@ff400000 {
+		compatible = "altr,socfpga-lwhps2fpga-bridge";
+		reg = <0xff400000 0x100000>;
+		resets = <&rst LWHPS2FPGA_RESET>;
+		reset-names = "lwhps2fpga";
+		clocks = <&l4_main_clk>;
+	};
+
+	bridge1: bridge@ff500000 {
+		compatible = "altr,socfpga-hps2fpga-bridge";
+		reg = <0xff500000 0x10000>;
+		resets = <&rst HPS2FPGA_RESET>;
+		reset-names = "hps2fpga";
+		clocks = <&l4_main_clk>;
+	};
+
+	base_fpga_region {
+		compatible = "fpga-region";
+		fpga-mgr = <&fpgamgr0>;
+		fpga-bridges = <&bridge0>, <&bridge1>;
+
+		#address-cells = <0x1>;
+		#size-cells = <0x1>;
+		ranges;
+	};
+
+Overlay has:
+
+/dts-v1/;
+/plugin/;
+/ {
+	fragment@0 {
+		target-path = "/soc/base_fpga_region";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		__overlay__ {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0x00020000 0xff220000 0x00000008>,
+				 <0x00010040 0xff210040 0x00000020>;
+
+			firmware-name = "soc_system.rbf";
+
+			jtag_uart: serial@20000 {
+				compatible = "altr,juart-1.0";
+				reg = <0x00020000 0x00000008>;
+				interrupt-parent = <&intc>;
+				interrupts = <0 42 4>;
+			};
+
+			led_pio: gpio@10040 {
+				compatible = "altr,pio-1.0";
+				reg = <0x00010040 0x00000020>;
+				altr,gpio-bank-width = <4>;
+				#gpio-cells = <2>;
+				gpio-controller;
+			};
+		};
+	};
+};
+
+Supported Use Models
+====================
+
+Here's a list of supported use models.  We may need to add more.  Some uses are
+specific to one FPGA device or another.
+
+In all cases the live DT must specify the FPGA Manager, FPGA Bridges (if any),
+and a FPGA Region.  The target of the Device Tree Overlay is the FPGA Region.
+
+ * No FPGA Bridges
+   In this case, the FPGA Manager which programs the FPGA also handles the
+   bridges.  No FPGA Bridge devices are needed for full reconfiguration.
+
+ * Full reconfiguration with bridges
+   In this case, there are several bridges between the processor and FPGA that
+   need to be disabled during full reconfiguration.  The live DT before the
+   overlay is applied will include the FPGA Manager, Bridges, and Region.
+   The FPGA Region will include the phandles of the FPGA Manager and Bridges.
+
+ * Partial reconfiguration with bridges in the FPGA
+   In this case, the FPGA will have more than one PRR that will be programmed
+   separately.  While one PRR is being programmed, other PRR's may be active
+   on the bus.  To manage this, Freeze blocks need to exist on the FPGA
+   that can freeze all the buses going to one FPGA region while the buses are
+   enabled for other sections.
+
+Sequence
+========
+
+When a DT overlay is loaded, the FPGA Region will be notified and will do the
+following:
+ 1. Disable the FPGA Bridges.
+ 2. Use the the FPGA manager core to program the FPGA.
+ 3. Enable the FPGA Bridges.
+ 4. Call of_platform_populate resulting in device drivers getting probed.
+
+When the overlay is removed, the FPGA Region will be notified and will disable
+the bridges and the child nodes will be removed.
+
+Device Tree Examples
+====================
+
+The intention of this section is to give some simple examples, focusing on
+the placement of the elements detailed above, especially:
+ * FPGA Manager
+ * FPGA Bridges
+ * FPGA Region
+ * ranges
+ * target-path or target
+
+For the purposes of this section, I'm dividing the Device Tree into two parts,
+each with its own requirements.  The two parts are:
+ * The live DT prior to the overlay being added
+ * The DT overlay
+
+The live Device Tree must contain an FPGA Region, an FPGA Manager, and any FPGA
+Bridges.  The FPGA Region's "fpga-mgr" property specifies the manager by phandle
+to handle programming the FPGA.  If the FPGA Region is the child of another FPGA
+Region, the parent's FPGA Manager is used.  If FPGA Bridges need to be involved,
+they are specified in the FPGA Region by the "fpga-bridges" property.  During
+FPGA programming, the FPGA Region will disable the bridges that are in its
+"fpga-bridges" list and will re-enable them after FPGA programming has
+succeeded.
+
+The Device Tree Overlay will contain:
+ * "target-path" or "target"
+   The insertion point where the the contents of the overlay will go into the
+   live tree.  target-path is a full path, while target is a phandle.
+ * "ranges"
+ * "firmware-name"
+   Specifies the name of the FPGA image file on the firmware search
+   path.  The search path is described in the firmware class documentation.
+ * "partial-reconfig"
+   This binding is a boolean and should be present if partial reconfiguration
+   is to be done.
+ * child nodes corresponding to hardware that will be loaded in this region of
+   the FPGA.
+
+Device Tree Example: Full Reconfiguration without Bridges
+=========================================================
+
+Live Device Tree contains:
+	fpga_mgr0: fpgamgr@f8007000 {
+		compatible = "xlnx,zynq-devcfg-1.0";
+		reg = <0xf8007000 0x100>;
+		interrupt-parent = <&intc>;
+		interrupts = <0 8 4>;
+		clocks = <&clkc 12>;
+		clock-names = "ref_clk";
+		syscon = <&slcr>;
+	};
+
+	base_fpga_region {
+		compatible = "fpga-region";
+		fpga-mgr = <&fpga_mgr0>;
+		#address-cells = <0x1>;
+		#size-cells = <0x1>;
+		ranges;
+	};
+
+DT Overlay contains:
+/dts-v1/;
+/plugin/;
+/ {
+fragment@0 {
+	target = <&base_fpga_region>;
+	#address-cells = <1>;
+	#size-cells = <1>;
+	__overlay__ {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		firmware-name = "zynq-gpio.bin";
+
+		gpio1: gpio@40000000 {
+			compatible = "xlnx,xps-gpio-1.00.a";
+			reg = <0x40000000 0x10000>;
+			gpio-controller;
+			#gpio-cells = <0x2>;
+			xlnx,gpio-width= <0x6>;
+		};
+	};
+};
+
+--
+[1] www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/ug/ug_partrecon.pdf
+[2] tspace.library.utoronto.ca/bitstream/1807/67932/1/Byma_Stuart_A_201411_MAS_thesis.pdf
-- 
1.7.9.5

Re: [PATCH v16 1/6] fpga: add bindings document for fpga region

[Josh Cartwright]

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

Hey Alan-

First off, thanks for all of your (and others') work on this.

On Fri, Feb 05, 2016 at 03:29:58PM -0600, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> New bindings document for FPGA Region to support programming
> FPGA's under Device Tree control
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
[..]
> ---
>  .../devicetree/bindings/fpga/fpga-region.txt       |  348 ++++++++++++++++++++
>  1 file changed, 348 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> new file mode 100644
> index 0000000..ccd7127
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
[..]
> +FPGA Manager & FPGA Manager Framework
> + * An FPGA Manager is a hardware block that programs an FPGA under the control
> +   of a host processor.
> + * The FPGA Manager Framework provides drivers and functions to program an
> +   FPGA.
> +
> +FPGA Bridge Framework
> + * Provides drivers and functions to control bridges that enable/disable
> +   data to the FPGA.
> + * FPGA Bridges should be disabled while the FPGA is being programmed to
> +   prevent spurious data on the bus.
> + * FPGA Bridges may not be needed in implementations where the FPGA Manager
> +   handles this.

It still seems strange for me architecturally for the FPGA Bridge to be
a first-class top-level concept in your architecture, as they are a
reflection of the SoC FPGA manager design.  That is, I would expect the
bridges not to be associated with the FPGA Region, but with the FPGA
manager.

Although, I will concede that you you've made it possible to not use
FPGA Bridges (like on Zynq where they aren't necessary), so maybe it
doesn't matter, just smells strangely.

> +Freeze Blocks
> + * Freeze Blocks function as FPGA Bridges within the FPGA fabric.  In the case
> +   of PR, the buses from the processor are split within the FPGA.  Each PR
> +   region gets its own split of the buses, protected by an independently
> +   controlled Freeze Block.  Several busses may be connected to a single
> +   PR region; a Freeze Block controls the traffic of all these busses
> +   together.
> +
> +
[..]
> +Device Tree Examples
> +====================
> +
> +The intention of this section is to give some simple examples, focusing on
> +the placement of the elements detailed above, especially:
> + * FPGA Manager
> + * FPGA Bridges
> + * FPGA Region
> + * ranges
> + * target-path or target
> +
> +For the purposes of this section, I'm dividing the Device Tree into two parts,
> +each with its own requirements.  The two parts are:
> + * The live DT prior to the overlay being added
> + * The DT overlay
> +
> +The live Device Tree must contain an FPGA Region, an FPGA Manager, and any FPGA
> +Bridges.  The FPGA Region's "fpga-mgr" property specifies the manager by phandle
> +to handle programming the FPGA.  If the FPGA Region is the child of another FPGA
> +Region, the parent's FPGA Manager is used.  If FPGA Bridges need to be involved,
> +they are specified in the FPGA Region by the "fpga-bridges" property.  During
> +FPGA programming, the FPGA Region will disable the bridges that are in its
> +"fpga-bridges" list and will re-enable them after FPGA programming has
> +succeeded.
> +
> +The Device Tree Overlay will contain:
> + * "target-path" or "target"
> +   The insertion point where the the contents of the overlay will go into the
> +   live tree.  target-path is a full path, while target is a phandle.
> + * "ranges"
> + * "firmware-name"
> +   Specifies the name of the FPGA image file on the firmware search
> +   path.  The search path is described in the firmware class documentation.
> + * "partial-reconfig"
> +   This binding is a boolean and should be present if partial reconfiguration
> +   is to be done.

Another architectural smell: there are categorically two different types
of properties here.  The first kind is those properties which describe
_what_ IP exists within the FPGA image (all of the nodes under the regions, etc).
The second kind of properties are those which describe _how_ the image
should be written (partial-reconfig, firmware-name).

It seems weird, but maybe it doesn't matter much.

Thanks,
  Josh

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

Re: [PATCH v16 1/6] fpga: add bindings document for fpga region

[atull]

On Fri, 5 Feb 2016, Josh Cartwright wrote:

> Hey Alan-
> 
> First off, thanks for all of your (and others') work on this.
> 
> On Fri, Feb 05, 2016 at 03:29:58PM -0600, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > New bindings document for FPGA Region to support programming
> > FPGA's under Device Tree control
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> [..]
> > ---
> >  .../devicetree/bindings/fpga/fpga-region.txt       |  348 ++++++++++++++++++++
> >  1 file changed, 348 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > new file mode 100644
> > index 0000000..ccd7127
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> [..]
> > +FPGA Manager & FPGA Manager Framework
> > + * An FPGA Manager is a hardware block that programs an FPGA under the control
> > +   of a host processor.
> > + * The FPGA Manager Framework provides drivers and functions to program an
> > +   FPGA.
> > +
> > +FPGA Bridge Framework
> > + * Provides drivers and functions to control bridges that enable/disable
> > +   data to the FPGA.
> > + * FPGA Bridges should be disabled while the FPGA is being programmed to
> > +   prevent spurious data on the bus.
> > + * FPGA Bridges may not be needed in implementations where the FPGA Manager
> > +   handles this.
> 
> It still seems strange for me architecturally for the FPGA Bridge to be
> a first-class top-level concept in your architecture, as they are a
> reflection of the SoC FPGA manager design.

It's not so strange if you keep in mind the intent: I want to support
both partial and full reconfiguration.  Also I want this to work not
just for Xilinx but also for Altera :) That makes it a top-level
concept.

>  That is, I would expect the
> bridges not to be associated with the FPGA Region, but with the FPGA
> manager.

Maybe for the one use case of full reconfiguation on Zynq.  More
generally, FPGA Bridges may be hardened hardware devices or they may
be soft hardware in the FPGA that allow some regions of the FGPA to be
isolated from the cpu busses while other FPGA regions are active. The
latter case is to support partial reconfiguration for devices that can
support partial reconfiguration.  The partial reconfiguration region
that is being programmed will need to be isolated from the bus during
programming while the rest of the FPGA remains active.  That requires
that the bridges for each partial reconfiguration region be specified
per region.  Supporting both full and partial reconfiguration is a
priority for me.  I'll have to review this document and see how I can
make this more clear.

> 
> Although, I will concede that you you've made it possible to not use
> FPGA Bridges (like on Zynq where they aren't necessary), so maybe it
> doesn't matter, just smells strangely.

Hmmm my patches are extra fragrent today...  

> 
> > +Freeze Blocks
> > + * Freeze Blocks function as FPGA Bridges within the FPGA fabric.  In the case
> > +   of PR, the buses from the processor are split within the FPGA.  Each PR
> > +   region gets its own split of the buses, protected by an independently
> > +   controlled Freeze Block.  Several busses may be connected to a single
> > +   PR region; a Freeze Block controls the traffic of all these busses
> > +   together.
> > +
> > +
> [..]
> > +Device Tree Examples
> > +====================
> > +
> > +The intention of this section is to give some simple examples, focusing on
> > +the placement of the elements detailed above, especially:
> > + * FPGA Manager
> > + * FPGA Bridges
> > + * FPGA Region
> > + * ranges
> > + * target-path or target
> > +
> > +For the purposes of this section, I'm dividing the Device Tree into two parts,
> > +each with its own requirements.  The two parts are:
> > + * The live DT prior to the overlay being added
> > + * The DT overlay
> > +
> > +The live Device Tree must contain an FPGA Region, an FPGA Manager, and any FPGA
> > +Bridges.  The FPGA Region's "fpga-mgr" property specifies the manager by phandle
> > +to handle programming the FPGA.  If the FPGA Region is the child of another FPGA
> > +Region, the parent's FPGA Manager is used.  If FPGA Bridges need to be involved,
> > +they are specified in the FPGA Region by the "fpga-bridges" property.  During
> > +FPGA programming, the FPGA Region will disable the bridges that are in its
> > +"fpga-bridges" list and will re-enable them after FPGA programming has
> > +succeeded.
> > +
> > +The Device Tree Overlay will contain:
> > + * "target-path" or "target"
> > +   The insertion point where the the contents of the overlay will go into the
> > +   live tree.  target-path is a full path, while target is a phandle.
> > + * "ranges"
> > + * "firmware-name"
> > +   Specifies the name of the FPGA image file on the firmware search
> > +   path.  The search path is described in the firmware class documentation.
> > + * "partial-reconfig"
> > +   This binding is a boolean and should be present if partial reconfiguration
> > +   is to be done.
> 
> Another architectural smell: there are categorically two different types
> of properties here.  The first kind is those properties which describe
> _what_ IP exists within the FPGA image (all of the nodes under the regions, etc).
> The second kind of properties are those which describe _how_ the image
> should be written (partial-reconfig, firmware-name).
> 
> It seems weird, but maybe it doesn't matter much.

Reprogramming an FPGA will always involve 3 types of information:
static information of the hardened hardware (FPGA Manager and hardware
bridges), configuration information (FPGA image and whether to do
partial or full reconfiguration), and post-configuration information
(devices that will exist after programming).  That's going to be true
whether we use the device tree or not.  My goal is to have a
convenient way to reprogram the FPGA and populate the children.  So if
the smell can be tolerated, it can perhaps be very sweet to have the
convenience of having all this information together in a manageable
fashion.  Let's see what the DT folks say.

Anyway thanks for reviewing my stinky patches!  

Alan

> 
> Thanks,
>   Josh
> 

Re: [PATCH v16 1/6] fpga: add bindings document for fpga region

[Rob Herring]

On Fri, Feb 05, 2016 at 04:44:46PM -0600, Josh Cartwright wrote:
> Hey Alan-
> 
> First off, thanks for all of your (and others') work on this.
> 
> On Fri, Feb 05, 2016 at 03:29:58PM -0600, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > New bindings document for FPGA Region to support programming
> > FPGA's under Device Tree control
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> [..]
> > ---
> >  .../devicetree/bindings/fpga/fpga-region.txt       |  348 ++++++++++++++++++++
> >  1 file changed, 348 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > new file mode 100644
> > index 0000000..ccd7127
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> [..]
> > +FPGA Manager & FPGA Manager Framework
> > + * An FPGA Manager is a hardware block that programs an FPGA under the control
> > +   of a host processor.
> > + * The FPGA Manager Framework provides drivers and functions to program an
> > +   FPGA.
> > +
> > +FPGA Bridge Framework
> > + * Provides drivers and functions to control bridges that enable/disable
> > +   data to the FPGA.
> > + * FPGA Bridges should be disabled while the FPGA is being programmed to
> > +   prevent spurious data on the bus.
> > + * FPGA Bridges may not be needed in implementations where the FPGA Manager
> > +   handles this.
> 
> It still seems strange for me architecturally for the FPGA Bridge to be
> a first-class top-level concept in your architecture, as they are a
> reflection of the SoC FPGA manager design.  That is, I would expect the
> bridges not to be associated with the FPGA Region, but with the FPGA
> manager.
> 
> Although, I will concede that you you've made it possible to not use
> FPGA Bridges (like on Zynq where they aren't necessary), so maybe it
> doesn't matter, just smells strangely.

In general, DT models buses in the node hierarchy. To go from one bus to 
another, you need a bridge. Going from an onchip bus to an FPGA bus 
has to have some sort of bridge logic in between for isolation 
minimally. Zynq has to have something similar. Perhaps the bridge 
control is not part of the bridges themselves?

Rob

[PATCH v16 2/6] add sysfs document for fpga bridge class

[atull]

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

Add documentation for new FPGA bridge class's sysfs interface.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
--
v15: Document added in v15 of patch set
v16: No change to this patch in v16 of patch set
---
 Documentation/ABI/testing/sysfs-class-fpga-bridge |   11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-bridge

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-bridge b/Documentation/ABI/testing/sysfs-class-fpga-bridge
new file mode 100644
index 0000000..312ae2c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-fpga-bridge
@@ -0,0 +1,11 @@
+What:		/sys/class/fpga_bridge/<bridge>/name
+Date:		January 2016
+KernelVersion:	4.5
+Contact:	Alan Tull <atull@opensource.altera.com>
+Description:	Name of low level FPGA bridge driver.
+
+What:		/sys/class/fpga_bridge/<bridge>/state
+Date:		January 2016
+KernelVersion:	4.5
+Contact:	Alan Tull <atull@opensource.altera.com>
+Description:	Show bridge state as "enabled" or "disabled"
-- 
1.7.9.5

[PATCH v16 3/6] ARM: socfpga: add bindings document for fpga bridge drivers

[atull]

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

Add bindings documentation for Altera SOCFPGA bridges:
 * fpga2sdram
 * fpga2hps
 * hps2fpga
 * lwhps2fpga

Signed-off-by: Alan Tull <atull@opensource.altera.com>
Signed-off-by: Matthew Gerlach <mgerlach@altera.com>
Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
v2:  separate into 2 documents for the 2 drivers
v12: bump version to line up with simple-fpga-bus version
     remove Linux specific notes such as references to sysfs
     move non-DT specific documentation elsewhere
     remove bindings that would have been used to pass configuration
     clean up formatting
v13: Remove 'label' property
     Change property from init-val to bridge-enable
     Fix email address
v14: Add resets
     Change order of bridges to put lw bridge (controlling bridge) first
v15: No change in this patch for v15 of this patch set
v16: Added regs property, cleaned up unit addresses
---
 .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   15 +++++++
 .../bindings/fpga/altera-hps2fpga-bridge.txt       |   47 ++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
 create mode 100644 Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt

diff --git a/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
new file mode 100644
index 0000000..4479a79
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
@@ -0,0 +1,15 @@
+Altera FPGA To SDRAM Bridge Driver
+
+Required properties:
+- compatible		: Should contain "altr,socfpga-fpga2sdram-bridge"
+
+Optional properties:
+- bridge-enable		: 0 if driver should disable bridge at startup
+			  1 if driver should enable bridge at startup
+			  Default is to leave bridge in current state.
+
+Example:
+	fpga2sdram_br {
+		compatible = "altr,socfpga-fpga2sdram-bridge";
+		bridge-enable = <0>;
+	};
diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
new file mode 100644
index 0000000..e6b7474
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
@@ -0,0 +1,47 @@
+Altera FPGA/HPS Bridge Driver
+
+Required properties:
+- regs		: base address and size for AXI bridge module
+- compatible	: Should contain one of:
+		  "altr,socfpga-lwhps2fpga-bridge",
+		  "altr,socfpga-hps2fpga-bridge", or
+		  "altr,socfpga-fpga2hps-bridge"
+- reset-names	: Should contain one of:
+		  "lwhps2fpga",
+		  "hps2fpga", or
+		  "fpga2hps"
+- resets	: Phandle and reset specifier for the reset listed in
+		  reset-names
+- clocks	: Clocks used by this module.
+
+Optional properties:
+- bridge-enable	: 0 if driver should disable bridge at startup.
+		  1 if driver should enable bridge at startup.
+		  Default is to leave bridge in its current state.
+
+Example:
+	hps_fpgabridge0: fpgabridge@ff400000 {
+		compatible = "altr,socfpga-lwhps2fpga-bridge";
+		reg = <0xff400000 0x100000>;
+		resets = <&rst LWHPS2FPGA_RESET>;
+		reset-names = "lwhps2fpga";
+		clocks = <&l4_main_clk>;
+		bridge-enable = <0>;
+	};
+
+	hps_fpgabridge1: fpgabridge@ff500000 {
+		compatible = "altr,socfpga-hps2fpga-bridge";
+		reg = <0xff500000 0x10000>;
+		resets = <&rst HPS2FPGA_RESET>;
+		reset-names = "hps2fpga";
+		clocks = <&l4_main_clk>;
+		bridge-enable = <1>;
+	};
+
+	hps_fpgabridge2: fpgabridge@ff600000 {
+		compatible = "altr,socfpga-fpga2hps-bridge";
+		reg = <0xff600000 0x100000>;
+		resets = <&rst FPGA2HPS_RESET>;
+		reset-names = "fpga2hps";
+		clocks = <&l4_main_clk>;
+	};
-- 
1.7.9.5

[PATCH v16 4/6] fpga: add fpga bridge framework

[atull]

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

This framework adds API functions for enabling/
disabling FPGA bridges under kernel control.

This allows the Linux kernel to disable FPGA bridges
during FPGA reprogramming and to enable FPGA bridges
when FPGA reprogramming is done.  This framework is
be manufacturer-agnostic, allowing it to be used in
interfaces that use the FPGA Manager Framework to
reprogram FPGA's.

The functions are:
* of_fpga_bridge_get
* fpga_bridge_put
   Get/put an exclusive reference to a FPGA bridge.

* fpga_bridge_enable
* fpga_bridge_disable
   Enable/Disable traffic through a bridge.

* fpga_bridge_register
* fpga_bridge_unregister
   Register/unregister a device-specific low level FPGA
   Bridge driver.

Get an exclusive reference to a bridge and add it to a list:
* fpga_bridge_get_to_list

To enable/disable/put a set of bridges that are on a list:
* fpga_bridges_enable
* fpga_bridges_disable
* fpga_bridges_put

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v2:  Minor cleanup
v12: Bump version to line up with simple fpga bus
     Remove sysfs
     Improve get/put functions, get the low level driver too.
     Clean up class implementation
     Add kernel doc documentation
     Rename (un)register_fpga_bridge -> fpga_bridge_(un)register
v13: Add inlined empty functions for if not CONFIG_FPGA_BRIDGE
     Clean up debugging
     Remove unneeded #include in .h
     Remove unnecessary prints
     Remove 'label' DT binding.
     Document the mutex
v14: Allow bridges with no ops
     *const* struct fpga_bridge_ops
     Add functions to git/put/enable/disable list of bridges
     Add list node to struct fpga_bridge
     Do of_node_get/put in of_fpga_bridge_get()
     Add r/o attributes: name and state
v15: No change in this patch for v15 of this patch set
v16: Remove of_get_fpga_bus function
---
 drivers/fpga/Kconfig             |    7 +
 drivers/fpga/Makefile            |    3 +
 drivers/fpga/fpga-bridge.c       |  388 ++++++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-bridge.h |   55 ++++++
 4 files changed, 453 insertions(+)
 create mode 100644 drivers/fpga/fpga-bridge.c
 create mode 100644 include/linux/fpga/fpga-bridge.h

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index c9b9fdf..b6cfd89 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -24,6 +24,13 @@ config FPGA_MGR_ZYNQ_FPGA
 	help
 	  FPGA manager driver support for Xilinx Zynq FPGAs.
 
+config FPGA_BRIDGE
+       bool "FPGA Bridge Framework"
+       depends on OF
+       help
+         Say Y here if you want to support bridges connected between host
+	 processors and FPGAs or between FPGAs.
+
 endif # FPGA
 
 endmenu
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 8d83fc6..4baef00 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -8,3 +8,6 @@ obj-$(CONFIG_FPGA)			+= fpga-mgr.o
 # FPGA Manager Drivers
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
+
+# FPGA Bridge Drivers
+obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
new file mode 100644
index 0000000..5119d8e
--- /dev/null
+++ b/drivers/fpga/fpga-bridge.c
@@ -0,0 +1,388 @@
+/*
+ * FPGA Bridge Framework Driver
+ *
+ *  Copyright (C) 2013-2015 Altera Corporation, All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/fpga/fpga-bridge.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+static DEFINE_IDA(fpga_bridge_ida);
+static struct class *fpga_bridge_class;
+
+/* Lock for adding/removing bridges to linked lists*/
+spinlock_t bridge_list_lock;
+
+static int fpga_bridge_of_node_match(struct device *dev, const void *data)
+{
+	return dev->of_node == data;
+}
+
+/**
+ * fpga_bridge_enable - Enable transactions on the bridge
+ *
+ * @bridge: FPGA bridge
+ *
+ * Return: 0 for success, error code otherwise.
+ */
+int fpga_bridge_enable(struct fpga_bridge *bridge)
+{
+	dev_dbg(&bridge->dev, "enable\n");
+
+	if (bridge->br_ops && bridge->br_ops->enable_set)
+		return bridge->br_ops->enable_set(bridge, 1);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_enable);
+
+/**
+ * fpga_bridge_disable - Disable transactions on the bridge
+ *
+ * @bridge: FPGA bridge
+ *
+ * Return: 0 for success, error code otherwise.
+ */
+int fpga_bridge_disable(struct fpga_bridge *bridge)
+{
+	dev_dbg(&bridge->dev, "disable\n");
+
+	if (bridge->br_ops && bridge->br_ops->enable_set)
+		return bridge->br_ops->enable_set(bridge, 0);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_disable);
+
+/**
+ * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
+ *
+ * @np: node pointer of a FPGA bridge
+ *
+ * Return fpga_bridge struct if successful.
+ * Return -EBUSY if someone already has a reference to the bridge.
+ * Return -ENODEV if @np is not a FPGA Bridge.
+ */
+struct fpga_bridge *of_fpga_bridge_get(struct device_node *np)
+{
+	struct device *dev;
+	struct fpga_bridge *bridge;
+	int ret = -ENODEV;
+
+	of_node_get(np);
+
+	dev = class_find_device(fpga_bridge_class, NULL, np,
+				fpga_bridge_of_node_match);
+	if (!dev)
+		goto err_dev;
+
+	bridge = to_fpga_bridge(dev);
+	if (!bridge)
+		goto err_dev;
+
+	if (!mutex_trylock(&bridge->mutex)) {
+		ret = -EBUSY;
+		goto err_dev;
+	}
+
+	if (!try_module_get(dev->parent->driver->owner))
+		goto err_ll_mod;
+
+	dev_dbg(&bridge->dev, "get\n");
+
+	return bridge;
+
+err_ll_mod:
+	mutex_unlock(&bridge->mutex);
+err_dev:
+	put_device(dev);
+	of_node_put(np);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
+
+/**
+ * fpga_bridge_put - release a reference to a bridge
+ *
+ * @bridge: FPGA bridge
+ */
+void fpga_bridge_put(struct fpga_bridge *bridge)
+{
+	dev_dbg(&bridge->dev, "put\n");
+
+	module_put(bridge->dev.parent->driver->owner);
+	mutex_unlock(&bridge->mutex);
+	put_device(&bridge->dev);
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_put);
+
+/**
+ * fpga_bridges_enable - enable bridges in a list
+ * @bridge_list: list of FPGA bridges
+ *
+ * Enable each bridge in the list.  If list is empty, do nothing.
+ *
+ * Return 0 for success or empty bridge list; return error code otherwise.
+ */
+int fpga_bridges_enable(struct list_head *bridge_list)
+{
+	struct fpga_bridge *bridge;
+	struct list_head *node;
+	int ret;
+
+	list_for_each(node, bridge_list) {
+		bridge = list_entry(node, struct fpga_bridge, node);
+		ret = fpga_bridge_enable(bridge);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fpga_bridges_enable);
+
+/**
+ * fpga_bridges_disable - disable bridges in a list
+ *
+ * @bridge_list: list of FPGA bridges
+ *
+ * Disable each bridge in the list.  If list is empty, do nothing.
+ *
+ * Return 0 for success or empty bridge list; return error code otherwise.
+ */
+int fpga_bridges_disable(struct list_head *bridge_list)
+{
+	struct fpga_bridge *bridge;
+	struct list_head *node;
+	int ret;
+
+	list_for_each(node, bridge_list) {
+		bridge = list_entry(node, struct fpga_bridge, node);
+		ret = fpga_bridge_disable(bridge);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fpga_bridges_disable);
+
+/**
+ * fpga_bridges_put - put bridges
+ *
+ * @bridge_list: list of FPGA bridges
+ *
+ * For each bridge in the list, put the bridge and remove it from the list.
+ * If list is empty, do nothing.
+ */
+void fpga_bridges_put(struct list_head *bridge_list)
+{
+	struct fpga_bridge *bridge;
+	struct list_head *node, *next;
+	unsigned long flags;
+
+	list_for_each_safe(node, next, bridge_list) {
+		bridge = list_entry(node, struct fpga_bridge, node);
+
+		fpga_bridge_put(bridge);
+
+		spin_lock_irqsave(&bridge_list_lock, flags);
+		list_del(&bridge->node);
+		spin_unlock_irqrestore(&bridge_list_lock, flags);
+	}
+}
+EXPORT_SYMBOL_GPL(fpga_bridges_put);
+
+/**
+ * fpga_bridges_get_to_list - get a bridge, add it to a list
+ *
+ * @np: node pointer of a FPGA bridge
+ * @bridge_list: list of FPGA bridges
+ *
+ * Get an exclusive reference to the bridge and and it to the list.
+ *
+ * Return 0 for success, error code from of_fpga_bridge_get() othewise.
+ */
+int fpga_bridge_get_to_list(struct device_node *np,
+			    struct list_head *bridge_list)
+{
+	struct fpga_bridge *bridge;
+	unsigned long flags;
+
+	bridge = of_fpga_bridge_get(np);
+	if (IS_ERR(bridge))
+		return PTR_ERR(bridge);
+
+	spin_lock_irqsave(&bridge_list_lock, flags);
+	list_add(&bridge->node, bridge_list);
+	spin_unlock_irqrestore(&bridge_list_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_get_to_list);
+
+static ssize_t name_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct fpga_bridge *bridge = to_fpga_bridge(dev);
+
+	return sprintf(buf, "%s\n", bridge->name);
+}
+
+static ssize_t state_show(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	struct fpga_bridge *bridge = to_fpga_bridge(dev);
+	int enable = 1;
+
+	if (bridge->br_ops && bridge->br_ops->enable_show)
+		enable = bridge->br_ops->enable_show(bridge);
+
+	return sprintf(buf, "%s\n", enable ? "enabled" : "disabled");
+}
+
+static DEVICE_ATTR_RO(name);
+static DEVICE_ATTR_RO(state);
+
+static struct attribute *fpga_bridge_attrs[] = {
+	&dev_attr_name.attr,
+	&dev_attr_state.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(fpga_bridge);
+
+/**
+ * fpga_bridge_register - register a fpga bridge driver
+ * @dev:	FPGA bridge device from pdev
+ * @name:	FPGA bridge name
+ * @br_ops:	pointer to structure of fpga bridge ops
+ * @priv:	FPGA bridge private data
+ *
+ * Return: 0 for success, error code otherwise.
+ */
+int fpga_bridge_register(struct device *dev, const char *name,
+			 const struct fpga_bridge_ops *br_ops, void *priv)
+{
+	struct fpga_bridge *bridge;
+	int id, ret = 0;
+
+	if (!name || !strlen(name)) {
+		dev_err(dev, "Attempt to register with no name!\n");
+		return -EINVAL;
+	}
+
+	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
+	if (!bridge)
+		return -ENOMEM;
+
+	id = ida_simple_get(&fpga_bridge_ida, 0, 0, GFP_KERNEL);
+	if (id < 0) {
+		ret = id;
+		goto error_kfree;
+	}
+
+	mutex_init(&bridge->mutex);
+	INIT_LIST_HEAD(&bridge->node);
+
+	bridge->name = name;
+	bridge->br_ops = br_ops;
+	bridge->priv = priv;
+
+	device_initialize(&bridge->dev);
+	bridge->dev.class = fpga_bridge_class;
+	bridge->dev.parent = dev;
+	bridge->dev.of_node = dev->of_node;
+	bridge->dev.id = id;
+	dev_set_drvdata(dev, bridge);
+
+	ret = dev_set_name(&bridge->dev, "br%d", id);
+	if (ret)
+		goto error_device;
+
+	ret = device_add(&bridge->dev);
+	if (ret)
+		goto error_device;
+
+	dev_info(bridge->dev.parent, "fpga bridge [%s] registered\n",
+		 bridge->name);
+
+	return 0;
+
+error_device:
+	ida_simple_remove(&fpga_bridge_ida, id);
+error_kfree:
+	kfree(bridge);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_register);
+
+/**
+ * fpga_bridge_unregister - unregister a fpga bridge driver
+ * @dev: FPGA bridge device from pdev
+ */
+void fpga_bridge_unregister(struct device *dev)
+{
+	struct fpga_bridge *bridge = dev_get_drvdata(dev);
+
+	/*
+	 * If the low level driver provides a method for putting bridge into
+	 * a desired state upon unregister, do it.
+	 */
+	if (bridge->br_ops && bridge->br_ops->fpga_bridge_remove)
+		bridge->br_ops->fpga_bridge_remove(bridge);
+
+	device_unregister(&bridge->dev);
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_unregister);
+
+static void fpga_bridge_dev_release(struct device *dev)
+{
+	struct fpga_bridge *bridge = to_fpga_bridge(dev);
+
+	ida_simple_remove(&fpga_bridge_ida, bridge->dev.id);
+	kfree(bridge);
+}
+
+static int __init fpga_bridge_dev_init(void)
+{
+	spin_lock_init(&bridge_list_lock);
+
+	fpga_bridge_class = class_create(THIS_MODULE, "fpga_bridge");
+	if (IS_ERR(fpga_bridge_class))
+		return PTR_ERR(fpga_bridge_class);
+
+	fpga_bridge_class->dev_groups = fpga_bridge_groups;
+	fpga_bridge_class->dev_release = fpga_bridge_dev_release;
+
+	return 0;
+}
+
+static void __exit fpga_bridge_dev_exit(void)
+{
+	class_destroy(fpga_bridge_class);
+	ida_destroy(&fpga_bridge_ida);
+}
+
+MODULE_DESCRIPTION("FPGA Bridge Driver");
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_LICENSE("GPL v2");
+
+subsys_initcall(fpga_bridge_dev_init);
+module_exit(fpga_bridge_dev_exit);
diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
new file mode 100644
index 0000000..1a96934
--- /dev/null
+++ b/include/linux/fpga/fpga-bridge.h
@@ -0,0 +1,55 @@
+#include <linux/device.h>
+
+#ifndef _LINUX_FPGA_BRIDGE_H
+#define _LINUX_FPGA_BRIDGE_H
+
+struct fpga_bridge;
+
+/**
+ * struct fpga_bridge_ops - ops for low level FPGA bridge drivers
+ * @enable_show: returns the FPGA bridge's status
+ * @enable_set: set a FPGA bridge as enabled or disabled
+ * @fpga_bridge_remove: set FPGA into a specific state during driver remove
+ */
+struct fpga_bridge_ops {
+	int (*enable_show)(struct fpga_bridge *bridge);
+	int (*enable_set)(struct fpga_bridge *bridge, bool enable);
+	void (*fpga_bridge_remove)(struct fpga_bridge *bridge);
+};
+
+/**
+ * struct fpga_bridge - FPGA bridge structure
+ * @name: name of low level FPGA bridge
+ * @dev: FPGA bridge device
+ * @mutex: enforces exclusive reference to bridge
+ * @br_ops: pointer to struct of FPGA bridge ops
+ * @node: FPGA bridge list node
+ * @priv: low level driver private date
+ */
+struct fpga_bridge {
+	const char *name;
+	struct device dev;
+	struct mutex mutex; /* for exclusive reference to bridge */
+	const struct fpga_bridge_ops *br_ops;
+	struct list_head node;
+	void *priv;
+};
+
+#define to_fpga_bridge(d) container_of(d, struct fpga_bridge, dev)
+
+struct fpga_bridge *of_fpga_bridge_get(struct device_node *node);
+void fpga_bridge_put(struct fpga_bridge *bridge);
+int fpga_bridge_enable(struct fpga_bridge *bridge);
+int fpga_bridge_disable(struct fpga_bridge *bridge);
+
+int fpga_bridges_enable(struct list_head *bridge_list);
+int fpga_bridges_disable(struct list_head *bridge_list);
+void fpga_bridges_put(struct list_head *bridge_list);
+int fpga_bridge_get_to_list(struct device_node *np,
+			    struct list_head *bridge_list);
+
+int fpga_bridge_register(struct device *dev, const char *name,
+			 const struct fpga_bridge_ops *br_ops, void *priv);
+void fpga_bridge_unregister(struct device *dev);
+
+#endif /* _LINUX_FPGA_BRIDGE_H */
-- 
1.7.9.5

[PATCH v16 5/6] fpga: fpga-region: device tree control for FPGA

[atull]

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

FPGA Regions support programming FPGA under control of the Device
Tree.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v9:  initial version (this patch added during rest of patchset's v9)
v10: request deferral if fpga mgr or bridges not available yet
     cleanup as fpga manager core goes into the real kernel
     Don't assume bridges are disabled before programming FPGA
     Don't hang onto reference for fpga manager
     Move to staging/simple-fpga-bus
v11: No change in this patch for v11 of the patch set
v12: Moved out of staging.
     Use fpga bridges framework.
v13: If no bridges are specified, assume we don't need any.
     Clean up debug messages
     Some dev_info -> dev_dbg
     Remove unneeded #include
     Fix size of array of pointers
     Don't need to specify .owner
     Use common binding: firmware-name
v14: OK it's not a simple bus.  Call it "FPGA Area"
     Remove bindings that specify FPGA manager and FPGA bridges
     Use parent FPGA bridge and bridges that are its peers
     Use ancestor FPGA Manager
v15: Add altr,fpga-bus implementation
     Change compatible string "fpga-area" -> "altr,fpga-area"
v16: Much changes as FPGA Areas and Busses become FPGA Regions
     Add reconfig notifier, don't rely on simple-bus
---
 drivers/fpga/Kconfig       |    7 +
 drivers/fpga/Makefile      |    3 +
 drivers/fpga/fpga-region.c |  460 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 470 insertions(+)
 create mode 100644 drivers/fpga/fpga-region.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index b6cfd89..c419d4b 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -13,6 +13,13 @@ config FPGA
 
 if FPGA
 
+config FPGA_REGION
+       bool "FPGA Region"
+       depends on OF
+       help
+	 FPGA Regions allow loading FPGA images under control of
+	 the Device Tree.
+
 config FPGA_MGR_SOCFPGA
 	tristate "Altera SOCFPGA FPGA Manager"
 	depends on ARCH_SOCFPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 4baef00..8d746c3 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -11,3 +11,6 @@ obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
 
 # FPGA Bridge Drivers
 obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
+
+# High Level Interfaces
+obj-$(CONFIG_FPGA_REGION)		+= fpga-region.o
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
new file mode 100644
index 0000000..eb4d3a6
--- /dev/null
+++ b/drivers/fpga/fpga-region.c
@@ -0,0 +1,460 @@
+/*
+ * FPGA Region - Device Tree support for FPGA programming under Linux
+ *
+ *  Copyright (C) 2013-2016 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/idr.h>
+#include <linux/fpga/fpga-bridge.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+/**
+ * struct fpga_region - FPGA Region structure
+ * @dev: FPGA Region device
+ * @mutex: enforces exclusive reference to region
+ * @bridge_list: list of FPGA bridges specified in region
+ */
+struct fpga_region {
+	struct device dev;
+	struct mutex mutex; /* for exclusive reference to region */
+	struct list_head bridge_list;
+};
+
+#define to_fpga_region(d) container_of(d, struct fpga_region, dev)
+
+static DEFINE_IDA(fpga_region_ida);
+static struct class *fpga_region_class;
+
+static const struct of_device_id fpga_region_of_match[] = {
+	{ .compatible = "fpga-region", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, fpga_region_of_match);
+
+static int fpga_region_of_node_match(struct device *dev, const void *data)
+{
+	return dev->of_node == data;
+}
+
+/**
+ * fpga_region_find - find FPGA region
+ * @np: device node of FPGA Region
+ * Caller will need to put_device(&region->dev) when done.
+ * Returns FPGA Region struct or NULL
+ */
+static struct fpga_region *fpga_region_find(struct device_node *np)
+{
+	struct device *dev;
+
+	dev = class_find_device(fpga_region_class, NULL, np,
+				fpga_region_of_node_match);
+	if (!dev) {
+		pr_err("%s did not find FPGA Region in class: %s\n", __func__,
+		       np->full_name);
+		return NULL;
+	}
+
+	return to_fpga_region(dev);
+}
+
+/**
+ * fpga_region_get - get an exclusive reference to a fpga region
+ * @region: FPGA Region struct
+ *
+ * Caller should call fpga_region_put() when done with region.
+ *
+ * Return fpga_region struct if successful.
+ * Return -EBUSY if someone already has a reference to the region.
+ * Return -ENODEV if @np is not a FPGA Region.
+ */
+static struct fpga_region *fpga_region_get(struct fpga_region *region)
+{
+	struct device *dev = &region->dev;
+
+	if (!mutex_trylock(&region->mutex)) {
+		dev_dbg(dev, "%s: FPGA Region already in use\n", __func__);
+		return ERR_PTR(-EBUSY);
+	}
+
+	get_device(dev);
+	of_node_get(dev->of_node);
+	if (!try_module_get(dev->parent->driver->owner)) {
+		of_node_put(dev->of_node);
+		put_device(dev);
+		mutex_unlock(&region->mutex);
+		return ERR_PTR(-ENODEV);
+	}
+
+	dev_dbg(&region->dev, "get\n");
+
+	return region;
+}
+
+/**
+ * fpga_region_put - release a reference to a region
+ *
+ * @region: FPGA region
+ */
+static void fpga_region_put(struct fpga_region *region)
+{
+	struct device *dev = &region->dev;
+
+	dev_dbg(&region->dev, "put\n");
+
+	module_put(dev->parent->driver->owner);
+	of_node_put(dev->of_node);
+	put_device(dev);
+	mutex_unlock(&region->mutex);
+}
+
+/**
+ * fpga_region_get_manager - get exclusive reference for FPGA manager
+ * @region: FPGA region
+ *
+ * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
+ *
+ * Caller should call fpga_mgr_put() when done with manager.
+ *
+ * Return: fpga manager struct or IS_ERR() condition containing error code.
+ */
+static struct fpga_manager *fpga_region_get_manager(struct fpga_region *region)
+{
+	struct device *dev = &region->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node  *mgr_node;
+	struct fpga_manager *mgr;
+
+	of_node_get(np);
+	while (np) {
+		if (of_device_is_compatible(np, "fpga-region")) {
+			mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
+			if (mgr_node) {
+				mgr = of_fpga_mgr_get(mgr_node);
+				of_node_put(np);
+				return mgr;
+			}
+		}
+		np = of_get_next_parent(np);
+	}
+	of_node_put(np);
+
+	return ERR_PTR(-EINVAL);
+}
+
+/**
+ * fpga_region_get_bridges - create a list of bridges
+ * @region: FPGA region
+ *
+ * Create a list of bridges specified by "fpga-bridges" property.
+ * If no bridges are specified, list will be empty.  Note that the
+ * fpga_bridges_enable/disable/put functions are all fine with an empty list.
+ *
+ * Caller should call fpga_bridges_put(&region->bridge_list) when
+ * done with the bridges.
+ *
+ * Return 0 for success (even if there are no bridges specified)
+ * or -EBUSY if any of the bridges are in use.
+ */
+static int fpga_region_get_bridges(struct fpga_region *region)
+{
+	struct device *dev = &region->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *br_node;
+	int i, ret;
+
+	for (i = 0; ; i++) {
+		br_node = of_parse_phandle(np, "fpga-bridges", i);
+		if (!br_node)
+			break;
+
+		/* If node is a bridge, get it and add to list */
+		ret = fpga_bridge_get_to_list(br_node, &region->bridge_list);
+
+		/* If any of the bridges are in use, give up */
+		if (ret == -EBUSY) {
+			fpga_bridges_put(&region->bridge_list);
+			return -EBUSY;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * fpga_region_program_fpga - program FPGA
+ * @region: FPGA region
+ * Program an FPGA using information in the device tree.
+ * Function assumes that there is a firmware-name property.
+ * Return 0 for success or negative error code.
+ */
+static int fpga_region_program_fpga(struct fpga_region *region)
+{
+	struct device *dev = &region->dev;
+	struct device_node *np = dev->of_node;
+	struct fpga_manager *mgr;
+	const char *path;
+	u32 flags = 0;
+	int ret;
+
+	region = fpga_region_get(region);
+	if (IS_ERR(region))
+		return PTR_ERR(region);
+
+	mgr = fpga_region_get_manager(region);
+	if (IS_ERR(mgr))
+		return PTR_ERR(mgr);
+
+	of_property_read_string(np, "firmware-name", &path);
+
+	if (of_property_read_bool(np, "partial-reconfig"))
+		flags |= FPGA_MGR_PARTIAL_RECONFIG;
+
+	ret = fpga_region_get_bridges(region);
+	if (ret)
+		goto err_put_mgr;
+
+	ret = fpga_bridges_disable(&region->bridge_list);
+	if (ret)
+		goto err_put_br;
+
+	ret = fpga_mgr_firmware_load(mgr, flags, path);
+	if (ret)
+		goto err_put_br;
+
+	ret = fpga_bridges_enable(&region->bridge_list);
+	if (ret)
+		goto err_put_br;
+
+	fpga_mgr_put(mgr);
+	fpga_region_put(region);
+
+	return 0;
+
+err_put_br:
+	fpga_bridges_put(&region->bridge_list);
+err_put_mgr:
+	fpga_mgr_put(mgr);
+	fpga_region_put(region);
+
+	return ret;
+}
+
+/**
+ * fpga_region_notify_remove_property - handle firmware-name property remove
+ * @region: FPGA Region structure
+ *
+ * This function handles the case where a firmware-name property has been
+ * removed from a FPGA Region.  The FPGA bridges for that region will be
+ * disabled and released.
+ */
+static void fpga_region_notify_remove_property(struct fpga_region *region)
+{
+	region = fpga_region_get(region);
+	if (IS_ERR(region))
+		return;
+
+	fpga_bridges_disable(&region->bridge_list);
+	fpga_bridges_put(&region->bridge_list);
+
+	fpga_region_put(region);
+}
+
+/**
+ * of_fpga_region_notify - reconfig notifier for dynamic DT changes
+ * @nb:		notifier block
+ * @action:	notifier action
+ * @arg:	reconfig data
+ *
+ * This notifier handles programming a FPGA when a "firmware-name" property is
+ * added to a fpga-region.
+ *
+ * Returns NOTIFY_OK or error if FPGA programming fails.
+ */
+static int of_fpga_region_notify(struct notifier_block *nb,
+				 unsigned long action, void *arg)
+{
+	struct of_reconfig_data *rd = arg;
+	struct device_node *np;
+	struct fpga_region *region;
+	int ret;
+
+	switch (action) {
+	case OF_RECONFIG_ADD_PROPERTY:
+	case OF_RECONFIG_REMOVE_PROPERTY:
+		/* Add/Remove a region's 'firmware-name' property */
+		np = rd->dn;
+		if (!of_device_is_compatible(np, "fpga-region") ||
+		    strcmp(rd->prop->name, "firmware-name"))
+			return NOTIFY_OK;       /* not for us */
+
+		region = fpga_region_find(np);
+		if (!region)
+			return NOTIFY_OK;
+
+		if (action == OF_RECONFIG_ADD_PROPERTY) {
+			ret = fpga_region_program_fpga(region);
+			if (ret) {
+				put_device(&region->dev);
+				return notifier_from_errno(ret);
+			}
+			of_platform_populate(np, fpga_region_of_match,
+					     NULL, &region->dev);
+		} else { /* OF_RECONFIG_REMOVE_PROPERTY */
+			fpga_region_notify_remove_property(region);
+		}
+
+		put_device(&region->dev);
+		break;
+
+	default:
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block fpga_region_of_nb = {
+	.notifier_call = of_fpga_region_notify,
+};
+
+static int fpga_region_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct fpga_region *region;
+	const char *path;
+	int id, ret = 0;
+
+	region = kzalloc(sizeof(*region), GFP_KERNEL);
+	if (!region)
+		return -ENOMEM;
+
+	id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL);
+	if (id < 0) {
+		ret = id;
+		goto err_kfree;
+	}
+
+	mutex_init(&region->mutex);
+	INIT_LIST_HEAD(&region->bridge_list);
+
+	device_initialize(&region->dev);
+	region->dev.class = fpga_region_class;
+	region->dev.parent = dev;
+	region->dev.of_node = np;
+	region->dev.id = id;
+	dev_set_drvdata(dev, region);
+
+	ret = dev_set_name(&region->dev, "region%d", id);
+	if (ret)
+		goto err_remove;
+
+	ret = device_add(&region->dev);
+	if (ret)
+		goto err_remove;
+
+	if (!of_property_read_string(np, "firmware-name", &path)) {
+		ret = fpga_region_program_fpga(region);
+		if (ret)
+			goto err_unreg;
+	}
+
+	of_platform_populate(np, fpga_region_of_match, NULL, &region->dev);
+
+	dev_info(dev, "FPGA Region probed\n");
+
+	return 0;
+
+err_unreg:
+	device_unregister(dev);
+err_remove:
+	ida_simple_remove(&fpga_region_ida, id);
+err_kfree:
+	kfree(region);
+
+	return ret;
+}
+
+static int fpga_region_remove(struct platform_device *pdev)
+{
+	struct fpga_region *region = platform_get_drvdata(pdev);
+
+	/* If fpga_region_probe programmed the FPGA, disable/put bridges */
+	fpga_bridges_disable(&region->bridge_list);
+	fpga_bridges_put(&region->bridge_list);
+
+	device_unregister(&region->dev);
+
+	return 0;
+}
+
+static struct platform_driver fpga_region_driver = {
+	.probe = fpga_region_probe,
+	.remove = fpga_region_remove,
+	.driver = {
+		.name	= "fpga-region",
+		.of_match_table = of_match_ptr(fpga_region_of_match),
+	},
+};
+
+static void fpga_region_dev_release(struct device *dev)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+
+	ida_simple_remove(&fpga_region_ida, region->dev.id);
+	kfree(region);
+}
+
+/**
+ * fpga_region_init - init function for fpga_region class
+ * Creates the fpga_region class and registers a reconfig notifier.
+ */
+static int __init fpga_region_init(void)
+{
+	int ret;
+
+	fpga_region_class = class_create(THIS_MODULE, "fpga_region");
+	if (IS_ERR(fpga_region_class))
+		return PTR_ERR(fpga_region_class);
+
+	fpga_region_class->dev_release = fpga_region_dev_release;
+
+	ret = of_reconfig_notifier_register(&fpga_region_of_nb);
+	if (ret)
+		return ret;
+
+	return platform_driver_register(&fpga_region_driver);
+}
+
+static void __exit fpga_region_exit(void)
+{
+	platform_driver_unregister(&fpga_region_driver);
+	of_reconfig_notifier_unregister(&fpga_region_of_nb);
+
+	class_destroy(fpga_region_class);
+	ida_destroy(&fpga_region_ida);
+}
+
+module_init(fpga_region_init);
+module_exit(fpga_region_exit);
+
+MODULE_DESCRIPTION("FPGA Region");
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[atull]

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

Supports Altera SOCFPGA bridges:
 * fpga2sdram
 * fpga2hps
 * hps2fpga
 * lwhps2fpga

Allows enabling/disabling the bridges through the FPGA
Bridge Framework API functions.

The fpga2sdram driver only supports enabling and disabling
of the ports that been configured early on.  This is due to
a hardware limitation where the read, write, and command
ports on the fpga2sdram bridge can only be reconfigured
while there are no transactions to the sdram, i.e. when
running out of OCRAM before the kernel boots.

Device tree property 'init-val' configures the driver to
enable or disable the bridge during probe.  If the property
does not exist, the driver will leave the bridge in its
current state.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
Signed-off-by: Matthew Gerlach <mgerlach@altera.com>
Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
v2:  Use resets instead of directly writing reset registers
v12: Bump version to align with simple-fpga-bus version
     Get rid of the sysfs interface
     fpga2sdram: get configuration stored in handoff register
v13: Remove unneeded WARN_ON
     Change property from init-val to bridge-enable
     Checkpatch cleanup
     Fix email address
v14: use module_platform_driver
     remove unused struct field and some #defines
     don't really need exclamation points on error msgs
     *const* struct fpga_bridge_ops
v15: No change in this patch for v15 of this patch set
v16: No change in this patch for v16 of this patch set
---
 drivers/fpga/Kconfig             |    7 ++
 drivers/fpga/Makefile            |    1 +
 drivers/fpga/altera-fpga2sdram.c |  174 +++++++++++++++++++++++++++++++
 drivers/fpga/altera-hps2fpga.c   |  213 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 395 insertions(+)
 create mode 100644 drivers/fpga/altera-fpga2sdram.c
 create mode 100644 drivers/fpga/altera-hps2fpga.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index c419d4b..45fd823 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -38,6 +38,13 @@ config FPGA_BRIDGE
          Say Y here if you want to support bridges connected between host
 	 processors and FPGAs or between FPGAs.
 
+config SOCFPGA_FPGA_BRIDGE
+	bool "Altera SoCFPGA FPGA Bridges"
+	depends on ARCH_SOCFPGA && FPGA_BRIDGE
+	help
+	  Say Y to enable drivers for FPGA bridges for Altera SOCFPGA
+	  devices.
+
 endif # FPGA
 
 endmenu
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 8d746c3..e658436 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
 
 # FPGA Bridge Drivers
 obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
+obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
 
 # High Level Interfaces
 obj-$(CONFIG_FPGA_REGION)		+= fpga-region.o
diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
new file mode 100644
index 0000000..91f4a40
--- /dev/null
+++ b/drivers/fpga/altera-fpga2sdram.c
@@ -0,0 +1,174 @@
+/*
+ * FPGA to SDRAM Bridge Driver for Altera SoCFPGA Devices
+ *
+ *  Copyright (C) 2013-2015 Altera Corporation, All Rights Reserved.
+ *
+ * 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/>.
+ */
+
+/*
+ * This driver manages a bridge between an FPGA and the SDRAM used by the ARM
+ * host processor system (HPS).
+ *
+ * The bridge contains 4 read ports, 4 write ports, and 6 command ports.
+ * Reconfiguring these ports requires that no SDRAM transactions occur during
+ * reconfiguration.  The code reconfiguring the ports cannot run out of SDRAM
+ * nor can the FPGA access the SDRAM during reconfiguration.  This driver does
+ * not support reconfiguring the ports.  The ports are configured by code
+ * running out of on chip ram before Linux is started and the configuration
+ * is passed in a handoff register in the system manager.
+ *
+ * This driver supports enabling and disabling of the configured ports, which
+ * allows for safe reprogramming of the FPGA, assuming that the new FPGA image
+ * uses the same port configuration.  Bridges must be disabled before
+ * reprogramming the FPGA and re-enabled after the FPGA has been programmed.
+ */
+
+#include <linux/fpga/fpga-bridge.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+
+#define ALT_SDR_CTL_FPGAPORTRST_OFST		0x80
+#define ALT_SDR_CTL_FPGAPORTRST_PORTRSTN_MSK	0x00003fff
+#define ALT_SDR_CTL_FPGAPORTRST_RD_SHIFT	0
+#define ALT_SDR_CTL_FPGAPORTRST_WR_SHIFT	4
+#define ALT_SDR_CTL_FPGAPORTRST_CTRL_SHIFT	8
+
+#define SYSMGR_ISWGRP_HANDOFF3          (0x8C)
+#define ISWGRP_HANDOFF_FPGA2SDR         SYSMGR_ISWGRP_HANDOFF3
+
+#define F2S_BRIDGE_NAME "fpga2sdram"
+
+struct alt_fpga2sdram_data {
+	struct device *dev;
+	struct regmap *sdrctl;
+	int mask;
+};
+
+static int alt_fpga2sdram_enable_show(struct fpga_bridge *bridge)
+{
+	struct alt_fpga2sdram_data *priv = bridge->priv;
+	int value;
+
+	regmap_read(priv->sdrctl, ALT_SDR_CTL_FPGAPORTRST_OFST, &value);
+
+	return (value & priv->mask) == priv->mask;
+}
+
+static inline int _alt_fpga2sdram_enable_set(struct alt_fpga2sdram_data *priv,
+					     bool enable)
+{
+	return regmap_update_bits(priv->sdrctl, ALT_SDR_CTL_FPGAPORTRST_OFST,
+				  priv->mask, enable ? priv->mask : 0);
+}
+
+static int alt_fpga2sdram_enable_set(struct fpga_bridge *bridge, bool enable)
+{
+	return _alt_fpga2sdram_enable_set(bridge->priv, enable);
+}
+
+struct prop_map {
+	char *prop_name;
+	u32 *prop_value;
+	u32 prop_max;
+};
+
+static const struct fpga_bridge_ops altera_fpga2sdram_br_ops = {
+	.enable_set = alt_fpga2sdram_enable_set,
+	.enable_show = alt_fpga2sdram_enable_show,
+};
+
+static const struct of_device_id altera_fpga_of_match[] = {
+	{ .compatible = "altr,socfpga-fpga2sdram-bridge" },
+	{},
+};
+
+static int alt_fpga_bridge_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct alt_fpga2sdram_data *priv;
+	u32 enable;
+	struct regmap *sysmgr;
+	int ret = 0;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	priv->sdrctl = syscon_regmap_lookup_by_compatible("altr,sdr-ctl");
+	if (IS_ERR(priv->sdrctl)) {
+		dev_err(dev, "regmap for altr,sdr-ctl lookup failed.\n");
+		return PTR_ERR(priv->sdrctl);
+	}
+
+	sysmgr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
+	if (IS_ERR(priv->sdrctl)) {
+		dev_err(dev, "regmap for altr,sys-mgr lookup failed.\n");
+		return PTR_ERR(sysmgr);
+	}
+
+	/* Get f2s bridge configuration saved in handoff register */
+	regmap_read(sysmgr, SYSMGR_ISWGRP_HANDOFF3, &priv->mask);
+
+	ret = fpga_bridge_register(dev, F2S_BRIDGE_NAME,
+				   &altera_fpga2sdram_br_ops, priv);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "driver initialized with handoff %08x\n", priv->mask);
+
+	if (!of_property_read_u32(dev->of_node, "bridge-enable", &enable)) {
+		if (enable > 1) {
+			dev_warn(dev, "invalid bridge-enable %u > 1\n", enable);
+		} else {
+			dev_info(dev, "%s bridge\n",
+				 (enable ? "enabling" : "disabling"));
+			ret = _alt_fpga2sdram_enable_set(priv, enable);
+			if (ret) {
+				fpga_bridge_unregister(&pdev->dev);
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
+static int alt_fpga_bridge_remove(struct platform_device *pdev)
+{
+	fpga_bridge_unregister(&pdev->dev);
+
+	return 0;
+}
+
+MODULE_DEVICE_TABLE(of, altera_fpga_of_match);
+
+static struct platform_driver altera_fpga_driver = {
+	.probe = alt_fpga_bridge_probe,
+	.remove = alt_fpga_bridge_remove,
+	.driver = {
+		.name	= "altera_fpga2sdram_bridge",
+		.of_match_table = of_match_ptr(altera_fpga_of_match),
+	},
+};
+
+module_platform_driver(altera_fpga_driver);
+
+MODULE_DESCRIPTION("Altera SoCFPGA FPGA to SDRAM Bridge");
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/fpga/altera-hps2fpga.c b/drivers/fpga/altera-hps2fpga.c
new file mode 100644
index 0000000..c15df47
--- /dev/null
+++ b/drivers/fpga/altera-hps2fpga.c
@@ -0,0 +1,213 @@
+/*
+ * FPGA to/from HPS Bridge Driver for Altera SoCFPGA Devices
+ *
+ *  Copyright (C) 2013-2015 Altera Corporation, All Rights Reserved.
+ *
+ * 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/>.
+ */
+
+/*
+ * This driver manages bridges on a Altera SOCFPGA between the ARM host
+ * processor system (HPS) and the embedded FPGA.
+ *
+ * This driver supports enabling and disabling of the configured ports, which
+ * allows for safe reprogramming of the FPGA, assuming that the new FPGA image
+ * uses the same port configuration.  Bridges must be disabled before
+ * reprogramming the FPGA and re-enabled after the FPGA has been programmed.
+ */
+
+#include <linux/clk.h>
+#include <linux/fpga/fpga-bridge.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#define ALT_L3_REMAP_OFST			0x0
+#define ALT_L3_REMAP_MPUZERO_MSK		0x00000001
+#define ALT_L3_REMAP_H2F_MSK			0x00000008
+#define ALT_L3_REMAP_LWH2F_MSK			0x00000010
+
+#define HPS2FPGA_BRIDGE_NAME			"hps2fpga"
+#define LWHPS2FPGA_BRIDGE_NAME			"lwhps2fpga"
+#define FPGA2HPS_BRIDGE_NAME			"fpga2hps"
+
+struct altera_hps2fpga_data {
+	const char *name;
+	struct reset_control *bridge_reset;
+	struct regmap *l3reg;
+	/* The L3 REMAP register is write only, so keep a cached value. */
+	unsigned int l3_remap_value;
+	unsigned int remap_mask;
+	struct clk *clk;
+};
+
+static int alt_hps2fpga_enable_show(struct fpga_bridge *bridge)
+{
+	struct altera_hps2fpga_data *priv = bridge->priv;
+
+	return reset_control_status(priv->bridge_reset);
+}
+
+static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
+				    bool enable)
+{
+	int ret;
+
+	/* bring bridge out of reset */
+	if (enable)
+		ret = reset_control_deassert(priv->bridge_reset);
+	else
+		ret = reset_control_assert(priv->bridge_reset);
+	if (ret)
+		return ret;
+
+	/* Allow bridge to be visible to L3 masters or not */
+	if (priv->remap_mask) {
+		priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
+
+		if (enable)
+			priv->l3_remap_value |= priv->remap_mask;
+		else
+			priv->l3_remap_value &= ~priv->remap_mask;
+
+		ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
+				   priv->l3_remap_value);
+	}
+
+	return ret;
+}
+
+static int alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
+{
+	return _alt_hps2fpga_enable_set(bridge->priv, enable);
+}
+
+static const struct fpga_bridge_ops altera_hps2fpga_br_ops = {
+	.enable_set = alt_hps2fpga_enable_set,
+	.enable_show = alt_hps2fpga_enable_show,
+};
+
+static struct altera_hps2fpga_data hps2fpga_data  = {
+	.name = HPS2FPGA_BRIDGE_NAME,
+	.remap_mask = ALT_L3_REMAP_H2F_MSK,
+};
+
+static struct altera_hps2fpga_data lwhps2fpga_data  = {
+	.name = LWHPS2FPGA_BRIDGE_NAME,
+	.remap_mask = ALT_L3_REMAP_LWH2F_MSK,
+};
+
+static struct altera_hps2fpga_data fpga2hps_data  = {
+	.name = FPGA2HPS_BRIDGE_NAME,
+};
+
+static const struct of_device_id altera_fpga_of_match[] = {
+	{ .compatible = "altr,socfpga-hps2fpga-bridge",
+	  .data = &hps2fpga_data },
+	{ .compatible = "altr,socfpga-lwhps2fpga-bridge",
+	  .data = &lwhps2fpga_data },
+	{ .compatible = "altr,socfpga-fpga2hps-bridge",
+	  .data = &fpga2hps_data },
+	{},
+};
+
+static int alt_fpga_bridge_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct altera_hps2fpga_data *priv;
+	const struct of_device_id *of_id;
+	u32 enable;
+	int ret;
+
+	of_id = of_match_device(altera_fpga_of_match, dev);
+	priv = (struct altera_hps2fpga_data *)of_id->data;
+
+	priv->bridge_reset = devm_reset_control_get(dev, priv->name);
+	if (IS_ERR(priv->bridge_reset)) {
+		dev_err(dev, "Could not get %s reset control\n", priv->name);
+		return PTR_ERR(priv->bridge_reset);
+	}
+
+	priv->l3reg = syscon_regmap_lookup_by_compatible("altr,l3regs");
+	if (IS_ERR(priv->l3reg)) {
+		dev_err(dev, "regmap for altr,l3regs lookup failed\n");
+		return PTR_ERR(priv->l3reg);
+	}
+
+	priv->clk = of_clk_get(dev->of_node, 0);
+	if (IS_ERR(priv->clk)) {
+		dev_err(dev, "no clock specified\n");
+		return PTR_ERR(priv->clk);
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		dev_err(dev, "could not enable clock\n");
+		return -EBUSY;
+	}
+
+	ret = fpga_bridge_register(dev, priv->name, &altera_hps2fpga_br_ops,
+				   priv);
+	if (ret)
+		return ret;
+
+	if (!of_property_read_u32(dev->of_node, "bridge-enable", &enable)) {
+		if (enable > 1) {
+			dev_warn(dev, "invalid bridge-enable %u > 1\n", enable);
+		} else {
+			dev_info(dev, "%s bridge\n",
+				 (enable ? "enabling" : "disabling"));
+
+			ret = _alt_hps2fpga_enable_set(priv, enable);
+			if (ret) {
+				fpga_bridge_unregister(&pdev->dev);
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
+static int alt_fpga_bridge_remove(struct platform_device *pdev)
+{
+	struct fpga_bridge *bridge = platform_get_drvdata(pdev);
+	struct altera_hps2fpga_data *priv = bridge->priv;
+
+	fpga_bridge_unregister(&pdev->dev);
+
+	clk_disable_unprepare(priv->clk);
+	clk_put(priv->clk);
+
+	return 0;
+}
+
+MODULE_DEVICE_TABLE(of, altera_fpga_of_match);
+
+static struct platform_driver alt_fpga_bridge_driver = {
+	.probe = alt_fpga_bridge_probe,
+	.remove = alt_fpga_bridge_remove,
+	.driver = {
+		.name	= "altera_hps2fpga_bridge",
+		.of_match_table = of_match_ptr(altera_fpga_of_match),
+	},
+};
+
+module_platform_driver(alt_fpga_bridge_driver);
+
+MODULE_DESCRIPTION("Altera SoCFPGA HPS to FPGA Bridge");
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[Trent Piepho]

On Fri, 2016-02-05 at 15:30 -0600, atull@opensource.altera.com wrote:
> Supports Altera SOCFPGA bridges:
>  * fpga2sdram
>  * fpga2hps
>  * hps2fpga
>  * lwhps2fpga
> 
> Allows enabling/disabling the bridges through the FPGA
> Bridge Framework API functions.

I'm replying to v16 because it exists on gmane, while v17 appears not
to.  lkml.org's forward feature appears to be broken so I can't reply to
that message (no way to get message-id).  But v17 of this patch should
be the same.  If a v18 was posted, I've not been able to find it.

> diff --git a/drivers/fpga/altera-hps2fpga.c b/drivers/fpga/altera-hps2fpga.c
> new file mode 100644
> index 0000000..c15df47
> --- /dev/null
> +++ b/drivers/fpga/altera-hps2fpga.c
> @@ -0,0 +1,213 @@
> +/*
> + * FPGA to/from HPS Bridge Driver for Altera SoCFPGA Devices
> + *
> + *  Copyright (C) 2013-2015 Altera Corporation, All Rights Reserved.
> + *
> + * 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/>.
> + */
> +
> +/*
> + * This driver manages bridges on a Altera SOCFPGA between the ARM host
> + * processor system (HPS) and the embedded FPGA.
> + *
> + * This driver supports enabling and disabling of the configured ports, which
> + * allows for safe reprogramming of the FPGA, assuming that the new FPGA image
> + * uses the same port configuration.  Bridges must be disabled before
> + * reprogramming the FPGA and re-enabled after the FPGA has been programmed.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/fpga/fpga-bridge.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +#define ALT_L3_REMAP_OFST			0x0
> +#define ALT_L3_REMAP_MPUZERO_MSK		0x00000001
> +#define ALT_L3_REMAP_H2F_MSK			0x00000008
> +#define ALT_L3_REMAP_LWH2F_MSK			0x00000010
> +
> +#define HPS2FPGA_BRIDGE_NAME			"hps2fpga"
> +#define LWHPS2FPGA_BRIDGE_NAME			"lwhps2fpga"
> +#define FPGA2HPS_BRIDGE_NAME			"fpga2hps"
> +
> +struct altera_hps2fpga_data {
> +	const char *name;
> +	struct reset_control *bridge_reset;
> +	struct regmap *l3reg;
> +	/* The L3 REMAP register is write only, so keep a cached value. */
> +	unsigned int l3_remap_value;
> +	unsigned int remap_mask;
> +	struct clk *clk;
> +};
> +
> +static int alt_hps2fpga_enable_show(struct fpga_bridge *bridge)
> +{
> +	struct altera_hps2fpga_data *priv = bridge->priv;
> +
> +	return reset_control_status(priv->bridge_reset);
> +}
> +
> +static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
> +				    bool enable)
> +{
> +	int ret;
> +
> +	/* bring bridge out of reset */
> +	if (enable)
> +		ret = reset_control_deassert(priv->bridge_reset);
> +	else
> +		ret = reset_control_assert(priv->bridge_reset);
> +	if (ret)
> +		return ret;
> +
> +	/* Allow bridge to be visible to L3 masters or not */
> +	if (priv->remap_mask) {
> +		priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;

Doesn't seem like this belongs here.  I realize the write-only register
is a problem.  Maybe the syscon driver should be initializing this
value?

> +
> +		if (enable)
> +			priv->l3_remap_value |= priv->remap_mask;
> +		else
> +			priv->l3_remap_value &= ~priv->remap_mask;
> +
> +		ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
> +				   priv->l3_remap_value);

This isn't going work if more than one bridge is used.  Each bridge has
its own priv and thus priv->l3_remap_value.  Each bridge's priv will
have just the bit for it's own remap set.  The 2nd bridge to be enabled
will turn off the 1st bridge when it re-write the l3 register.

If all the bridges shared a static global to cache the reg, then this
problem would be a replaced by a race, since nothing would be managing
concurrent access to that global from the independent bridge devices.

How about using the already existing regmap cache ability take care of
this?  Use regmap_update_bits() to update just the desired bit and let
remap take care of keeping track caching the register and protecting
access from multiple users.  It should support that and it should
support write-only registers, with the creator of the regmap (the syscon
driver in this case) supplying the initial value of the write-only reg.
Which is where ALT_L3_REMAP_MPUZERO_MSK could go in.


> +	}
> +
> +	return ret;
> +}
> +
> +static int alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
> +{
> +	return _alt_hps2fpga_enable_set(bridge->priv, enable);
> +}
> +
> +static const struct fpga_bridge_ops altera_hps2fpga_br_ops = {
> +	.enable_set = alt_hps2fpga_enable_set,
> +	.enable_show = alt_hps2fpga_enable_show,
> +};
> +
> +static struct altera_hps2fpga_data hps2fpga_data  = {
> +	.name = HPS2FPGA_BRIDGE_NAME,
> +	.remap_mask = ALT_L3_REMAP_H2F_MSK,
> +};

Each of these data structs also includes space for all the private data
field of the drivers' state.  Seems a bit inefficient if only two of
them are configuration data.  It also means only one device of each type
can exists.  If one creates two bridges of the same type they'll
(silently) share a priv data struct and randomly break.  And the config
data structs can't be const.

What if these structs were a different altera_hps_config struct, which
the private data struct could then copy or point to?

struct altera_hpsbridge_config {
	const char *name;
	uint32_t remap_mask;
};

struct altera_hpsbridge_data {
	const struct altera_hpsbridge_config *config;
	...;
	struct clk *clk;
};


> +
> +static struct altera_hps2fpga_data lwhps2fpga_data  = {
> +	.name = LWHPS2FPGA_BRIDGE_NAME,
> +	.remap_mask = ALT_L3_REMAP_LWH2F_MSK,
> +};
> +
> +static struct altera_hps2fpga_data fpga2hps_data  = {
> +	.name = FPGA2HPS_BRIDGE_NAME,
> +};
> +
> +static const struct of_device_id altera_fpga_of_match[] = {
> +	{ .compatible = "altr,socfpga-hps2fpga-bridge",
> +	  .data = &hps2fpga_data },
> +	{ .compatible = "altr,socfpga-lwhps2fpga-bridge",
> +	  .data = &lwhps2fpga_data },
> +	{ .compatible = "altr,socfpga-fpga2hps-bridge",
> +	  .data = &fpga2hps_data },
> +	{},
> +};
> +
> +static int alt_fpga_bridge_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct altera_hps2fpga_data *priv;
> +	const struct of_device_id *of_id;
> +	u32 enable;
> +	int ret;
> +
> +	of_id = of_match_device(altera_fpga_of_match, dev);
> +	priv = (struct altera_hps2fpga_data *)of_id->data;
> +
> +	priv->bridge_reset = devm_reset_control_get(dev, priv->name);
> +	if (IS_ERR(priv->bridge_reset)) {
> +		dev_err(dev, "Could not get %s reset control\n", priv->name);
> +		return PTR_ERR(priv->bridge_reset);
> +	}
> +


> +	priv->l3reg = syscon_regmap_lookup_by_compatible("altr,l3regs");
> +	if (IS_ERR(priv->l3reg)) {
> +		dev_err(dev, "regmap for altr,l3regs lookup failed\n");
> +		return PTR_ERR(priv->l3reg);
> +	}

Perhaps this could be wrapped in if(priv->remap_mask) { }.  The fpga2hps
bridge has no bits in the l3 remap register, so why should it need a
phandle to the l3 syscon?  This also prevents this driver from working
on Arria10, since it has no l3remap register at all, for any of the
bridges, so there's nothing for the phandle to point to.

> +
> +	priv->clk = of_clk_get(dev->of_node, 0);
> +	if (IS_ERR(priv->clk)) {
> +		dev_err(dev, "no clock specified\n");
> +		return PTR_ERR(priv->clk);
> +	}

devm_clk_get(dev, NULL); should get the 1st clock in the OF node, but
use the dev resource manager, so it doesn't need to be put.

> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret) {
> +		dev_err(dev, "could not enable clock\n");
> +		return -EBUSY;

clk_put() on clk missing here and also the other error returns.

> +	}
> +
> +	ret = fpga_bridge_register(dev, priv->name, &altera_hps2fpga_br_ops,
> +				   priv);
> +	if (ret)
> +		return ret;
> +
> +	if (!of_property_read_u32(dev->of_node, "bridge-enable", &enable)) {
> +		if (enable > 1) {
> +			dev_warn(dev, "invalid bridge-enable %u > 1\n", enable);
> +		} else {
> +			dev_info(dev, "%s bridge\n",
> +				 (enable ? "enabling" : "disabling"));
> +
> +			ret = _alt_hps2fpga_enable_set(priv, enable);

Should this go through the bridge api, e.g. fpga_bridge_enable()?  Since
the bridge has already been registered.  Or is the bridge framework
supposed to be able to support bridges that might be enabled or disabled
behind its back?  If so, then isn't there a race here with
_alt_hps2fpga_enable_set() possible being called at the same time as
other operations on this bridge triggered by the code in fpga-bridge.c?

Alternatively, could the bridge be enabled or disabled before being
registered?

> +			if (ret) {
> +				fpga_bridge_unregister(&pdev->dev);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int alt_fpga_bridge_remove(struct platform_device *pdev)
> +{
> +	struct fpga_bridge *bridge = platform_get_drvdata(pdev);
> +	struct altera_hps2fpga_data *priv = bridge->priv;
> +
> +	fpga_bridge_unregister(&pdev->dev);
> +
> +	clk_disable_unprepare(priv->clk);
> +	clk_put(priv->clk);
> +
> +	return 0;
> +}
> +
> +MODULE_DEVICE_TABLE(of, altera_fpga_of_match);
> +
> +static struct platform_driver alt_fpga_bridge_driver = {
> +	.probe = alt_fpga_bridge_probe,
> +	.remove = alt_fpga_bridge_remove,
> +	.driver = {
> +		.name	= "altera_hps2fpga_bridge",
> +		.of_match_table = of_match_ptr(altera_fpga_of_match),
> +	},
> +};
> +
> +module_platform_driver(alt_fpga_bridge_driver);
> +
> +MODULE_DESCRIPTION("Altera SoCFPGA HPS to FPGA Bridge");
> +MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Fri, 10 Jun 2016, Trent Piepho wrote:

> On Fri, 2016-02-05 at 15:30 -0600, atull@opensource.altera.com wrote:
> > Supports Altera SOCFPGA bridges:
> >  * fpga2sdram
> >  * fpga2hps
> >  * hps2fpga
> >  * lwhps2fpga
> > 
> > Allows enabling/disabling the bridges through the FPGA
> > Bridge Framework API functions.
> 
> I'm replying to v16 because it exists on gmane, while v17 appears not
> to.  lkml.org's forward feature appears to be broken so I can't reply to
> that message (no way to get message-id).  But v17 of this patch should
> be the same.  If a v18 was posted, I've not been able to find it.

Hi Trent,

Yes, we're up to v17. V18 will be soon, but v16 is good enough for
the purposes of this review.

> > +
> > +#define ALT_L3_REMAP_OFST			0x0
> > +#define ALT_L3_REMAP_MPUZERO_MSK		0x00000001
> > +#define ALT_L3_REMAP_H2F_MSK			0x00000008
> > +#define ALT_L3_REMAP_LWH2F_MSK			0x00000010
> > +
> > +#define HPS2FPGA_BRIDGE_NAME			"hps2fpga"
> > +#define LWHPS2FPGA_BRIDGE_NAME			"lwhps2fpga"
> > +#define FPGA2HPS_BRIDGE_NAME			"fpga2hps"
> > +
> > +struct altera_hps2fpga_data {
> > +	const char *name;
> > +	struct reset_control *bridge_reset;
> > +	struct regmap *l3reg;
> > +	/* The L3 REMAP register is write only, so keep a cached value. */
> > +	unsigned int l3_remap_value;
> > +	unsigned int remap_mask;
> > +	struct clk *clk;
> > +};
> > +
> > +static int alt_hps2fpga_enable_show(struct fpga_bridge *bridge)
> > +{
> > +	struct altera_hps2fpga_data *priv = bridge->priv;
> > +
> > +	return reset_control_status(priv->bridge_reset);
> > +}
> > +
> > +static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
> > +				    bool enable)
> > +{
> > +	int ret;
> > +
> > +	/* bring bridge out of reset */
> > +	if (enable)
> > +		ret = reset_control_deassert(priv->bridge_reset);
> > +	else
> > +		ret = reset_control_assert(priv->bridge_reset);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Allow bridge to be visible to L3 masters or not */
> > +	if (priv->remap_mask) {
> > +		priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
> 
> Doesn't seem like this belongs here.  I realize the write-only register
> is a problem.  Maybe the syscon driver should be initializing this
> value?
> 
> > +
> > +		if (enable)
> > +			priv->l3_remap_value |= priv->remap_mask;
> > +		else
> > +			priv->l3_remap_value &= ~priv->remap_mask;
> > +
> > +		ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
> > +				   priv->l3_remap_value);
> 
> This isn't going work if more than one bridge is used.  Each bridge has
> its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> have just the bit for it's own remap set.  The 2nd bridge to be enabled
> will turn off the 1st bridge when it re-write the l3 register.
> 
> If all the bridges shared a static global to cache the reg, then this
> problem would be a replaced by a race, since nothing would be managing
> concurrent access to that global from the independent bridge devices.
> 
> How about using the already existing regmap cache ability take care of
> this?  Use regmap_update_bits() to update just the desired bit and let
> remap take care of keeping track caching the register and protecting
> access from multiple users.  It should support that and it should
> support write-only registers, with the creator of the regmap (the syscon
> driver in this case) supplying the initial value of the write-only reg.
> Which is where ALT_L3_REMAP_MPUZERO_MSK could go in.

Please correct me if I'm wrong, but I think that regmap supports
the features you are talking about, but not syscon.

One simple solution would be to take l3_remap_value out of the priv
and let it be shared by all h2f bridges.  That involves the least
amount of change.

> 
> 
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
> > +{
> > +	return _alt_hps2fpga_enable_set(bridge->priv, enable);
> > +}
> > +
> > +static const struct fpga_bridge_ops altera_hps2fpga_br_ops = {
> > +	.enable_set = alt_hps2fpga_enable_set,
> > +	.enable_show = alt_hps2fpga_enable_show,
> > +};
> > +
> > +static struct altera_hps2fpga_data hps2fpga_data  = {
> > +	.name = HPS2FPGA_BRIDGE_NAME,
> > +	.remap_mask = ALT_L3_REMAP_H2F_MSK,
> > +};
> 
> Each of these data structs also includes space for all the private data
> field of the drivers' state.  Seems a bit inefficient if only two of
> them are configuration data.  It also means only one device of each type
> can exists.  If one creates two bridges of the same type they'll
> (silently) share a priv data struct and randomly break.  And the config
> data structs can't be const.

Our hardware doesn't contain two devices of any of these three types.

> 
> What if these structs were a different altera_hps_config struct, which
> the private data struct could then copy or point to?
> 
> struct altera_hpsbridge_config {
> 	const char *name;
> 	uint32_t remap_mask;
> };
> 
> struct altera_hpsbridge_data {
> 	const struct altera_hpsbridge_config *config;
> 	...;
> 	struct clk *clk;
> };

Yes, that sounds good and sensible.  I'll do that in v18.

> 
> 
> > +
> > +static struct altera_hps2fpga_data lwhps2fpga_data  = {
> > +	.name = LWHPS2FPGA_BRIDGE_NAME,
> > +	.remap_mask = ALT_L3_REMAP_LWH2F_MSK,
> > +};
> > +
> > +static struct altera_hps2fpga_data fpga2hps_data  = {
> > +	.name = FPGA2HPS_BRIDGE_NAME,
> > +};
> > +
> > +static const struct of_device_id altera_fpga_of_match[] = {
> > +	{ .compatible = "altr,socfpga-hps2fpga-bridge",
> > +	  .data = &hps2fpga_data },
> > +	{ .compatible = "altr,socfpga-lwhps2fpga-bridge",
> > +	  .data = &lwhps2fpga_data },
> > +	{ .compatible = "altr,socfpga-fpga2hps-bridge",
> > +	  .data = &fpga2hps_data },
> > +	{},
> > +};
> > +
> > +static int alt_fpga_bridge_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct altera_hps2fpga_data *priv;
> > +	const struct of_device_id *of_id;
> > +	u32 enable;
> > +	int ret;
> > +
> > +	of_id = of_match_device(altera_fpga_of_match, dev);
> > +	priv = (struct altera_hps2fpga_data *)of_id->data;
> > +
> > +	priv->bridge_reset = devm_reset_control_get(dev, priv->name);
> > +	if (IS_ERR(priv->bridge_reset)) {
> > +		dev_err(dev, "Could not get %s reset control\n", priv->name);
> > +		return PTR_ERR(priv->bridge_reset);
> > +	}
> > +
> 
> 
> > +	priv->l3reg = syscon_regmap_lookup_by_compatible("altr,l3regs");
> > +	if (IS_ERR(priv->l3reg)) {
> > +		dev_err(dev, "regmap for altr,l3regs lookup failed\n");
> > +		return PTR_ERR(priv->l3reg);
> > +	}
> 
> Perhaps this could be wrapped in if(priv->remap_mask) { }.  The fpga2hps
> bridge has no bits in the l3 remap register, so why should it need a
> phandle to the l3 syscon?  This also prevents this driver from working
> on Arria10, since it has no l3remap register at all, for any of the
> bridges, so there's nothing for the phandle to point to.

Agreed.

> 
> > +
> > +	priv->clk = of_clk_get(dev->of_node, 0);
> > +	if (IS_ERR(priv->clk)) {
> > +		dev_err(dev, "no clock specified\n");
> > +		return PTR_ERR(priv->clk);
> > +	}
> 
> devm_clk_get(dev, NULL); should get the 1st clock in the OF node, but
> use the dev resource manager, so it doesn't need to be put.

Yes

> 
> > +
> > +	ret = clk_prepare_enable(priv->clk);
> > +	if (ret) {
> > +		dev_err(dev, "could not enable clock\n");
> > +		return -EBUSY;
> 
> clk_put() on clk missing here and also the other error returns.

I'll use devm_clk_get() so that won't be needed.

> 
> > +	}
> > +
> > +	ret = fpga_bridge_register(dev, priv->name, &altera_hps2fpga_br_ops,
> > +				   priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!of_property_read_u32(dev->of_node, "bridge-enable", &enable)) {
> > +		if (enable > 1) {
> > +			dev_warn(dev, "invalid bridge-enable %u > 1\n", enable);
> > +		} else {
> > +			dev_info(dev, "%s bridge\n",
> > +				 (enable ? "enabling" : "disabling"));
> > +
> > +			ret = _alt_hps2fpga_enable_set(priv, enable);
> 
> Should this go through the bridge api, e.g. fpga_bridge_enable()?  Since
> the bridge has already been registered.  Or is the bridge framework
> supposed to be able to support bridges that might be enabled or disabled
> behind its back?  If so, then isn't there a race here with
> _alt_hps2fpga_enable_set() possible being called at the same time as
> other operations on this bridge triggered by the code in fpga-bridge.c?
> 
> Alternatively, could the bridge be enabled or disabled before being
> registered?

I'll do the enabling/disabling before registering the driver.

Thanks for your code review!

Alan

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[Trent Piepho]

On Mon, 2016-06-13 at 14:35 -0500, atull wrote:
> > > +
> > > +	/* Allow bridge to be visible to L3 masters or not */
> > > +	if (priv->remap_mask) {
> > > +		priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
> > 
> > Doesn't seem like this belongs here.  I realize the write-only register
> > is a problem.  Maybe the syscon driver should be initializing this
> > value?
> > 
> > > +
> > > +		if (enable)
> > > +			priv->l3_remap_value |= priv->remap_mask;
> > > +		else
> > > +			priv->l3_remap_value &= ~priv->remap_mask;
> > > +
> > > +		ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
> > > +				   priv->l3_remap_value);
> > 
> > This isn't going work if more than one bridge is used.  Each bridge has
> > its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> > have just the bit for it's own remap set.  The 2nd bridge to be enabled
> > will turn off the 1st bridge when it re-write the l3 register.
> > 
> > If all the bridges shared a static global to cache the reg, then this
> > problem would be a replaced by a race, since nothing would be managing
> > concurrent access to that global from the independent bridge devices.
> > 
> > How about using the already existing regmap cache ability take care of
> > this?  Use regmap_update_bits() to update just the desired bit and let
> > remap take care of keeping track caching the register and protecting
> > access from multiple users.  It should support that and it should
> > support write-only registers, with the creator of the regmap (the syscon
> > driver in this case) supplying the initial value of the write-only reg.
> > Which is where ALT_L3_REMAP_MPUZERO_MSK could go in.
> 
> Please correct me if I'm wrong, but I think that regmap supports
> the features you are talking about, but not syscon.

>From my testing, it will work ok if the syscon driver were to set
syscon_config.cache_type one of the caches.  Since the l3 regs read back
as 0, rather than not being readable at all, making them write-only and
giving a default isn't strictly necessary.

It wouldn't be hard to add a write-only property to syscon.

> One simple solution would be to take l3_remap_value out of the priv
> and let it be shared by all h2f bridges.  That involves the least
> amount of change.

You'll need a spin-lock to protect against concurrent access.


> > > +
> > > +static int alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
> > > +{
> > > +	return _alt_hps2fpga_enable_set(bridge->priv, enable);
> > > +}
> > > +
> > > +static const struct fpga_bridge_ops altera_hps2fpga_br_ops = {
> > > +	.enable_set = alt_hps2fpga_enable_set,
> > > +	.enable_show = alt_hps2fpga_enable_show,
> > > +};
> > > +
> > > +static struct altera_hps2fpga_data hps2fpga_data  = {
> > > +	.name = HPS2FPGA_BRIDGE_NAME,
> > > +	.remap_mask = ALT_L3_REMAP_H2F_MSK,
> > > +};
> > 
> > Each of these data structs also includes space for all the private data
> > field of the drivers' state.  Seems a bit inefficient if only two of
> > them are configuration data.  It also means only one device of each type
> > can exists.  If one creates two bridges of the same type they'll
> > (silently) share a priv data struct and randomly break.  And the config
> > data structs can't be const.
> 
> Our hardware doesn't contain two devices of any of these three types.

Not yet, but doesn't Arria10 have three FPGA2SDRAM bridges?  But really,
it's just that I think having each device allocated state data for just
that device is more in line with the Linux device model than shared
static state pre-allocated for all devices of the same type to share.
 
> > What if these structs were a different altera_hps_config struct, which
> > the private data struct could then copy or point to?
> > 
> > struct altera_hpsbridge_config {
> > 	const char *name;
> > 	uint32_t remap_mask;
> > };
> > 
> > struct altera_hpsbridge_data {
> > 	const struct altera_hpsbridge_config *config;
> > 	...;
> > 	struct clk *clk;
> > };
> 
> Yes, that sounds good and sensible.  I'll do that in v18.

Another thought, if the "altr,l3regs" binding included not just the
phandle to the l3regs device, but also the bit associated with the
bridge, then there wouldn't need to be bridge specific configuration for
the driver.  The same way the reset and clock properties point not just
to the reset and clock controller, but to the bit in the controller.

> > > +
> > > +static struct altera_hps2fpga_data lwhps2fpga_data  = {
> > > +	.name = LWHPS2FPGA_BRIDGE_NAME,
> > > +	.remap_mask = ALT_L3_REMAP_LWH2F_MSK,
> > > +};
> > > +
> > > +static struct altera_hps2fpga_data fpga2hps_data  = {
> > > +	.name = FPGA2HPS_BRIDGE_NAME,
> > > +};
> > > +
> > > +static const struct of_device_id altera_fpga_of_match[] = {
> > > +	{ .compatible = "altr,socfpga-hps2fpga-bridge",
> > > +	  .data = &hps2fpga_data },
> > > +	{ .compatible = "altr,socfpga-lwhps2fpga-bridge",
> > > +	  .data = &lwhps2fpga_data },
> > > +	{ .compatible = "altr,socfpga-fpga2hps-bridge",
> > > +	  .data = &fpga2hps_data },
> > > +	{},
> > > +};
> > > +
> > > +static int alt_fpga_bridge_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct altera_hps2fpga_data *priv;
> > > +	const struct of_device_id *of_id;
> > > +	u32 enable;
> > > +	int ret;
> > > +
> > > +	of_id = of_match_device(altera_fpga_of_match, dev);
> > > +	priv = (struct altera_hps2fpga_data *)of_id->data;
> > > +
> > > +	priv->bridge_reset = devm_reset_control_get(dev, priv->name);
> > > +	if (IS_ERR(priv->bridge_reset)) {
> > > +		dev_err(dev, "Could not get %s reset control\n", priv->name);
> > > +		return PTR_ERR(priv->bridge_reset);
> > > +	}
> > > +
> > 
> > 
> > > +	priv->l3reg = syscon_regmap_lookup_by_compatible("altr,l3regs");
> > > +	if (IS_ERR(priv->l3reg)) {
> > > +		dev_err(dev, "regmap for altr,l3regs lookup failed\n");
> > > +		return PTR_ERR(priv->l3reg);
> > > +	}
> > 
> > Perhaps this could be wrapped in if(priv->remap_mask) { }.  The fpga2hps
> > bridge has no bits in the l3 remap register, so why should it need a
> > phandle to the l3 syscon?  This also prevents this driver from working
> > on Arria10, since it has no l3remap register at all, for any of the
> > bridges, so there's nothing for the phandle to point to.
> 
> Agreed.
> 
> > 
> > > +
> > > +	priv->clk = of_clk_get(dev->of_node, 0);
> > > +	if (IS_ERR(priv->clk)) {
> > > +		dev_err(dev, "no clock specified\n");
> > > +		return PTR_ERR(priv->clk);
> > > +	}
> > 
> > devm_clk_get(dev, NULL); should get the 1st clock in the OF node, but
> > use the dev resource manager, so it doesn't need to be put.
> 
> Yes
> 
> > 
> > > +
> > > +	ret = clk_prepare_enable(priv->clk);
> > > +	if (ret) {
> > > +		dev_err(dev, "could not enable clock\n");
> > > +		return -EBUSY;
> > 
> > clk_put() on clk missing here and also the other error returns.
> 
> I'll use devm_clk_get() so that won't be needed.
> 
> > 
> > > +	}
> > > +
> > > +	ret = fpga_bridge_register(dev, priv->name, &altera_hps2fpga_br_ops,
> > > +				   priv);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (!of_property_read_u32(dev->of_node, "bridge-enable", &enable)) {
> > > +		if (enable > 1) {
> > > +			dev_warn(dev, "invalid bridge-enable %u > 1\n", enable);
> > > +		} else {
> > > +			dev_info(dev, "%s bridge\n",
> > > +				 (enable ? "enabling" : "disabling"));
> > > +
> > > +			ret = _alt_hps2fpga_enable_set(priv, enable);
> > 
> > Should this go through the bridge api, e.g. fpga_bridge_enable()?  Since
> > the bridge has already been registered.  Or is the bridge framework
> > supposed to be able to support bridges that might be enabled or disabled
> > behind its back?  If so, then isn't there a race here with
> > _alt_hps2fpga_enable_set() possible being called at the same time as
> > other operations on this bridge triggered by the code in fpga-bridge.c?
> > 
> > Alternatively, could the bridge be enabled or disabled before being
> > registered?
> 
> I'll do the enabling/disabling before registering the driver.
> 
> Thanks for your code review!
> 
> Alan

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[Andrea Galbusera]

On Fri, Jun 10, 2016 at 4:18 AM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> On Fri, 2016-02-05 at 15:30 -0600, atull@opensource.altera.com wrote:
>> Supports Altera SOCFPGA bridges:
>>  * fpga2sdram
>>  * fpga2hps
>>  * hps2fpga
>>  * lwhps2fpga
>>
>> Allows enabling/disabling the bridges through the FPGA
>> Bridge Framework API functions.
>
> I'm replying to v16 because it exists on gmane, while v17 appears not
> to.  lkml.org's forward feature appears to be broken so I can't reply to
> that message (no way to get message-id).  But v17 of this patch should
> be the same.  If a v18 was posted, I've not been able to find it.
>
>> diff --git a/drivers/fpga/altera-hps2fpga.c b/drivers/fpga/altera-hps2fpga.c
>> new file mode 100644
>> index 0000000..c15df47
>> --- /dev/null
>> +++ b/drivers/fpga/altera-hps2fpga.c
>> @@ -0,0 +1,213 @@
>> +/*
>> + * FPGA to/from HPS Bridge Driver for Altera SoCFPGA Devices
>> + *
>> + *  Copyright (C) 2013-2015 Altera Corporation, All Rights Reserved.
>> + *
>> + * 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/>.
>> + */
>> +
>> +/*
>> + * This driver manages bridges on a Altera SOCFPGA between the ARM host
>> + * processor system (HPS) and the embedded FPGA.
>> + *
>> + * This driver supports enabling and disabling of the configured ports, which
>> + * allows for safe reprogramming of the FPGA, assuming that the new FPGA image
>> + * uses the same port configuration.  Bridges must be disabled before
>> + * reprogramming the FPGA and re-enabled after the FPGA has been programmed.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/fpga/fpga-bridge.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +
>> +#define ALT_L3_REMAP_OFST                    0x0
>> +#define ALT_L3_REMAP_MPUZERO_MSK             0x00000001
>> +#define ALT_L3_REMAP_H2F_MSK                 0x00000008
>> +#define ALT_L3_REMAP_LWH2F_MSK                       0x00000010
>> +
>> +#define HPS2FPGA_BRIDGE_NAME                 "hps2fpga"
>> +#define LWHPS2FPGA_BRIDGE_NAME                       "lwhps2fpga"
>> +#define FPGA2HPS_BRIDGE_NAME                 "fpga2hps"
>> +
>> +struct altera_hps2fpga_data {
>> +     const char *name;
>> +     struct reset_control *bridge_reset;
>> +     struct regmap *l3reg;
>> +     /* The L3 REMAP register is write only, so keep a cached value. */
>> +     unsigned int l3_remap_value;
>> +     unsigned int remap_mask;
>> +     struct clk *clk;
>> +};
>> +
>> +static int alt_hps2fpga_enable_show(struct fpga_bridge *bridge)
>> +{
>> +     struct altera_hps2fpga_data *priv = bridge->priv;
>> +
>> +     return reset_control_status(priv->bridge_reset);
>> +}
>> +
>> +static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
>> +                                 bool enable)
>> +{
>> +     int ret;
>> +
>> +     /* bring bridge out of reset */
>> +     if (enable)
>> +             ret = reset_control_deassert(priv->bridge_reset);
>> +     else
>> +             ret = reset_control_assert(priv->bridge_reset);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* Allow bridge to be visible to L3 masters or not */
>> +     if (priv->remap_mask) {
>> +             priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
>
> Doesn't seem like this belongs here.  I realize the write-only register
> is a problem.  Maybe the syscon driver should be initializing this
> value?
>
>> +
>> +             if (enable)
>> +                     priv->l3_remap_value |= priv->remap_mask;
>> +             else
>> +                     priv->l3_remap_value &= ~priv->remap_mask;
>> +
>> +             ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
>> +                                priv->l3_remap_value);
>
> This isn't going work if more than one bridge is used.  Each bridge has
> its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> have just the bit for it's own remap set.  The 2nd bridge to be enabled
> will turn off the 1st bridge when it re-write the l3 register.

I can confirm this is exactly what happens with tag
"rel_socfpga-4.1.22-ltsi_16.06.02_pr" of socfpga-4.1.22-ltsi branch
from altera-opensource/linux-socfpga which includes more or less the
code in this patch. If you have 2 bridges (lw-hps2fpga and hps2fpga)
you end up with only one of them being visible. Easily spot by logging
l3_remap_value being passed to regmap_write()...

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Thu, 28 Jul 2016, Andrea Galbusera wrote:

> On Fri, Jun 10, 2016 at 4:18 AM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> > On Fri, 2016-02-05 at 15:30 -0600, atull@opensource.altera.com wrote:
> >> Supports Altera SOCFPGA bridges:
> >>  * fpga2sdram
> >>  * fpga2hps
> >>  * hps2fpga
> >>  * lwhps2fpga
> >>
> >> Allows enabling/disabling the bridges through the FPGA
> >> Bridge Framework API functions.
> >
> > I'm replying to v16 because it exists on gmane, while v17 appears not
> > to.  lkml.org's forward feature appears to be broken so I can't reply to
> > that message (no way to get message-id).  But v17 of this patch should
> > be the same.  If a v18 was posted, I've not been able to find it.
> >
> >> diff --git a/drivers/fpga/altera-hps2fpga.c b/drivers/fpga/altera-hps2fpga.c
> >> new file mode 100644
> >> index 0000000..c15df47
> >> --- /dev/null
> >> +++ b/drivers/fpga/altera-hps2fpga.c
> >> @@ -0,0 +1,213 @@
> >> +/*
> >> + * FPGA to/from HPS Bridge Driver for Altera SoCFPGA Devices
> >> + *
> >> + *  Copyright (C) 2013-2015 Altera Corporation, All Rights Reserved.
> >> + *
> >> + * 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/>.
> >> + */
> >> +
> >> +/*
> >> + * This driver manages bridges on a Altera SOCFPGA between the ARM host
> >> + * processor system (HPS) and the embedded FPGA.
> >> + *
> >> + * This driver supports enabling and disabling of the configured ports, which
> >> + * allows for safe reprogramming of the FPGA, assuming that the new FPGA image
> >> + * uses the same port configuration.  Bridges must be disabled before
> >> + * reprogramming the FPGA and re-enabled after the FPGA has been programmed.
> >> + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/fpga/fpga-bridge.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/mfd/syscon.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/reset.h>
> >> +
> >> +#define ALT_L3_REMAP_OFST                    0x0
> >> +#define ALT_L3_REMAP_MPUZERO_MSK             0x00000001
> >> +#define ALT_L3_REMAP_H2F_MSK                 0x00000008
> >> +#define ALT_L3_REMAP_LWH2F_MSK                       0x00000010
> >> +
> >> +#define HPS2FPGA_BRIDGE_NAME                 "hps2fpga"
> >> +#define LWHPS2FPGA_BRIDGE_NAME                       "lwhps2fpga"
> >> +#define FPGA2HPS_BRIDGE_NAME                 "fpga2hps"
> >> +
> >> +struct altera_hps2fpga_data {
> >> +     const char *name;
> >> +     struct reset_control *bridge_reset;
> >> +     struct regmap *l3reg;
> >> +     /* The L3 REMAP register is write only, so keep a cached value. */
> >> +     unsigned int l3_remap_value;
> >> +     unsigned int remap_mask;
> >> +     struct clk *clk;
> >> +};
> >> +
> >> +static int alt_hps2fpga_enable_show(struct fpga_bridge *bridge)
> >> +{
> >> +     struct altera_hps2fpga_data *priv = bridge->priv;
> >> +
> >> +     return reset_control_status(priv->bridge_reset);
> >> +}
> >> +
> >> +static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
> >> +                                 bool enable)
> >> +{
> >> +     int ret;
> >> +
> >> +     /* bring bridge out of reset */
> >> +     if (enable)
> >> +             ret = reset_control_deassert(priv->bridge_reset);
> >> +     else
> >> +             ret = reset_control_assert(priv->bridge_reset);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >> +     /* Allow bridge to be visible to L3 masters or not */
> >> +     if (priv->remap_mask) {
> >> +             priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
> >
> > Doesn't seem like this belongs here.  I realize the write-only register
> > is a problem.  Maybe the syscon driver should be initializing this
> > value?
> >
> >> +
> >> +             if (enable)
> >> +                     priv->l3_remap_value |= priv->remap_mask;
> >> +             else
> >> +                     priv->l3_remap_value &= ~priv->remap_mask;
> >> +
> >> +             ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
> >> +                                priv->l3_remap_value);
> >
> > This isn't going work if more than one bridge is used.  Each bridge has
> > its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> > have just the bit for it's own remap set.  The 2nd bridge to be enabled
> > will turn off the 1st bridge when it re-write the l3 register.
> 
> I can confirm this is exactly what happens with tag
> "rel_socfpga-4.1.22-ltsi_16.06.02_pr" of socfpga-4.1.22-ltsi branch
> from altera-opensource/linux-socfpga which includes more or less the
> code in this patch. If you have 2 bridges (lw-hps2fpga and hps2fpga)
> you end up with only one of them being visible. Easily spot by logging
> l3_remap_value being passed to regmap_write()...
> 

Anatolij kindly provided a patch for this issue.  I'll push it
to my github repo when I can.

Alan
---
>From e408cc03dfcdf5769133d069dda5914372b7aa54 Mon Sep 17 00:00:00 2001
From: Anatolij Gustschin <agust@denx.de>
Date: Fri, 22 Jul 2016 17:59:58 +0200
Subject: [PATCH 1/2] fpga: altera-hps2fpga: fix HPS2FPGA bridge visibility to L3 masters

While FPGA reconfiguration over device tree overlays the HPS2FPGA
bridge visibility to L3 masters is disabled. This results in abort
errors when accessing the address range of the FPGA devices behind
the HPS2FPGA bridge, i.e.:
  ...
  Unhandled fault: imprecise external abort (0x406) at 0xf0400000
  pgd = eef48000
  [f0400000] *pgd=2e362811, *pte=00000000, *ppte=00000000
  Internal error: : 406 [#1] SMP ARM
  ...

This visibility configuration error happens in bridge enable
function because the per-bridge 'priv->l3_remap_value' variable
doesn't cache all already written bits to write-only 'remap'
register. After FPGA reconfiguration the HPS2FPGA and LWHPS2FPGA
bridges must be enabled (bits 3 and 4 of the 'remap' register set).
So enable_set function is called for HPS2FPGA and then for LWHPS2FPGA
bridge. In the first call the value 0x9 is written to the 'remap'
register, in the second call the value 0x11 is written, resulting
in disabled HPS2FPGA visibility.

Use remap shadow register common for all bridges to fix the
external abort issue.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
 drivers/fpga/altera-hps2fpga.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/fpga/altera-hps2fpga.c b/drivers/fpga/altera-hps2fpga.c
index a2f0bd6..476e548 100644
--- a/drivers/fpga/altera-hps2fpga.c
+++ b/drivers/fpga/altera-hps2fpga.c
@@ -34,6 +34,7 @@
 #include <linux/of_platform.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
+#include <linux/spinlock.h>
 
 #define ALT_L3_REMAP_OFST			0x0
 #define ALT_L3_REMAP_MPUZERO_MSK		0x00000001
@@ -48,8 +49,6 @@ struct altera_hps2fpga_data {
 	const char *name;
 	struct reset_control *bridge_reset;
 	struct regmap *l3reg;
-	/* The L3 REMAP register is write only, so keep a cached value. */
-	unsigned int l3_remap_value;
 	unsigned int remap_mask;
 	struct clk *clk;
 };
@@ -61,9 +60,14 @@ static int alt_hps2fpga_enable_show(struct fpga_bridge *bridge)
 	return reset_control_status(priv->bridge_reset);
 }
 
+/* The L3 REMAP register is write only, so keep a cached value. */
+static unsigned int l3_remap_shadow;
+static spinlock_t l3_remap_lock;
+
 static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
 				    bool enable)
 {
+	unsigned long flags;
 	int ret;
 
 	/* bring bridge out of reset */
@@ -76,15 +80,17 @@ static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
 
 	/* Allow bridge to be visible to L3 masters or not */
 	if (priv->remap_mask) {
-		priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
+		spin_lock_irqsave(&l3_remap_lock, flags);
+		l3_remap_shadow |= ALT_L3_REMAP_MPUZERO_MSK;
 
 		if (enable)
-			priv->l3_remap_value |= priv->remap_mask;
+			l3_remap_shadow |= priv->remap_mask;
 		else
-			priv->l3_remap_value &= ~priv->remap_mask;
+			l3_remap_shadow &= ~priv->remap_mask;
 
 		ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
-				   priv->l3_remap_value);
+				   l3_remap_shadow);
+		spin_unlock_irqrestore(&l3_remap_lock, flags);
 	}
 
 	return ret;
@@ -159,6 +165,8 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 
+	spin_lock_init(&l3_remap_lock);
+
 	ret = fpga_bridge_register(dev, priv->name, &altera_hps2fpga_br_ops,
 				   priv);
 	if (ret)
-- 
1.7.9.5

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[Trent Piepho]

On Thu, 2016-07-28 at 10:21 -0500, atull wrote:
> > >
> > > This isn't going work if more than one bridge is used.  Each bridge has
> > > its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> > > have just the bit for it's own remap set.  The 2nd bridge to be enabled
> > > will turn off the 1st bridge when it re-write the l3 register.
> > 
> > I can confirm this is exactly what happens with tag
> > "rel_socfpga-4.1.22-ltsi_16.06.02_pr" of socfpga-4.1.22-ltsi branch
> > from altera-opensource/linux-socfpga which includes more or less the
> > code in this patch. If you have 2 bridges (lw-hps2fpga and hps2fpga)
> > you end up with only one of them being visible. Easily spot by logging
> > l3_remap_value being passed to regmap_write()...
> > 
> 
> Anatolij kindly provided a patch for this issue.  I'll push it
> to my github repo when I can.

I still think a better solution would be to allow the syscon driver
manage shared access.  The purpose of syscon is to manage access to a
shared resource from multiple devices.  And regmap already has the
ability to cache a write-only register and allow thread safe access to
modify bits in said register.  The problem is just the pain of trying to
do anything to syscon DT bindings.  Something like "write-only" in the
syscon binding that sets a couple values in the regmap_config is all
that's necessary.

Might as well not use syscon at all and have the bridge driver map the
l3regs itself, since it doesn't really use syscon for anything.

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Thu, 28 Jul 2016, Trent Piepho wrote:

> On Thu, 2016-07-28 at 10:21 -0500, atull wrote:
> > > >
> > > > This isn't going work if more than one bridge is used.  Each bridge has
> > > > its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> > > > have just the bit for it's own remap set.  The 2nd bridge to be enabled
> > > > will turn off the 1st bridge when it re-write the l3 register.
> > > 
> > > I can confirm this is exactly what happens with tag
> > > "rel_socfpga-4.1.22-ltsi_16.06.02_pr" of socfpga-4.1.22-ltsi branch
> > > from altera-opensource/linux-socfpga which includes more or less the
> > > code in this patch. If you have 2 bridges (lw-hps2fpga and hps2fpga)
> > > you end up with only one of them being visible. Easily spot by logging
> > > l3_remap_value being passed to regmap_write()...
> > > 
> > 
> > Anatolij kindly provided a patch for this issue.  I'll push it
> > to my github repo when I can.
> 
> I still think a better solution would be to allow the syscon driver
> manage shared access.  The purpose of syscon is to manage access to a
> shared resource from multiple devices.  And regmap already has the
> ability to cache a write-only register and allow thread safe access to
> modify bits in said register.  The problem is just the pain of trying to
> do anything to syscon DT bindings.  Something like "write-only" in the
> syscon binding that sets a couple values in the regmap_config is all
> that's necessary.
> 
> Might as well not use syscon at all and have the bridge driver map the
> l3regs itself, since it doesn't really use syscon for anything.
> 

I agree.  Just need time to do it.

Alan

Re: [PATCH v16 0/6] Device Tree support for FPGA programming

[atull]

On Fri, 5 Feb 2016, atull@opensource.altera.com wrote:

> From: Alan Tull <atull@opensource.altera.com>
> 
> v16 Refactors the FPGA Area and FPGA Bus into single thing called an
> FPGA Region and eliminates using simple-bus.  I'm using the word
> "region" as it's a term is used in the literature of both the major
> FPGA manufacturors.
> 
> Changes for v16:
> * Refactor the FPGA Area and FPGA Bus into a FPGA Region.
> * Don't use simple-bus.
> * FPGA Managers and FPGA Bridges are now specified by phandle using the 
>   "fpga-mgr" and "fpga-bridges" properties.  fpga-bridges can specify
>   more than one bridge.
> * Device Tree overlays should be targeted to a FPGA Region.
> * The overlays need only contain firmware-name and the child nodes.
> * To model a system containing >1 partial reconfiguration region,
>   an overlay could add FPGA Regions to the base FPGA Regions.
> * Child FPGA Regions inherit the parent FGPA Manager, but specify
>   their own set of bridges if needes as partial reconfig regions
>   will likely need their own bridges.
> * All this is discussed in bindings/fpga/fpga-region.txt
> 
> One other highlight:
> The little engine that runs this thing is a reconfig notifier
> in fpga-region.c.  This notifier that will program an FPGA if a
> "firmware-name" property gets added to a fpga-region.  Then
> it will call of_platform_populate().  The current behavior in Linux
> when a DT overlay is applied is that the reconfig notifications
> go out in heirarchical order: first notifications are for the
> properties, then notifications for the child nodes.  So an overlay
> that adds a 'firmware-name' property and some child nodes to a
> fpga-region will cause FPGA programming and child node
> populating in the right order.

I figured out how to get rid of the reconfig notifier.

> 
> One issue with the dynamic DT stuff:
> I've tried returning and error from the notifier if FPGA programming
> fails; the error is noted on the console, but the child nodes
> get probed anyway.

I looked into it further and now I've got a solution for this issue
that I can post soon.  I can stop using the DT overlay configfs
interface and add a sysfs file for applying an overlay to an FPGA
region.  The FPGA region implementation will see the overlay before it
becomes part of the live tree.  Then it can do the FPGA programming
and see that succeed before the child nodes become part of the live
tree.  If FPGA programming fails, the overlay will be rejected before
it becomes part of the live tree.  By the time 'firmware-name' and the
child nodes show up in the live tree, they will be post-configuration
information.

Each fpga_region appears in sysfs and will add a file for loading the
overlay targeted to that region.  Such as:

echo fpga-dt-overlay.dtb.o > \
  /sys/class/fpga_region/region0/overlay_name

fpga-region.c's overlay_name attribute code will load the overlay
file, unflatten and resolve it.  Then it will check to make sure it
passes a few rules.  Such as: the overlay must contain a fragment that
is targeted to the region whose sysfs it was applied.

If the overlay passes the rules check, the FPGA region will program
the FPGA.  If that succeeds, then it calls of_overlay_create() and the
overlay is added into the live tree and the children get populated.

So fpga-region.c will ensure that the FPGA is already programmed
before the child nodes are added to the DT.

This solution also means that fpga-region.c no longer needs a reconfig
notifier.

I'll have to look at the documentation to see how that changes how I
talk about the bindings.  The documentation will have to become two
files again since part of this is Linux specific and part is DT
bindings.

Alan

> 
> 
> Alan Tull (6):
>   fpga: add bindings document for fpga region
>   add sysfs document for fpga bridge class
>   ARM: socfpga: add bindings document for fpga bridge drivers
>   fpga: add fpga bridge framework
>   fpga: fpga-region: device tree control for FPGA
>   ARM: socfpga: fpga bridge driver support
> 
>  Documentation/ABI/testing/sysfs-class-fpga-bridge  |   11 +
>  .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   15 +
>  .../bindings/fpga/altera-hps2fpga-bridge.txt       |   47 ++
>  .../devicetree/bindings/fpga/fpga-region.txt       |  348 +++++++++++++++
>  drivers/fpga/Kconfig                               |   21 +
>  drivers/fpga/Makefile                              |    7 +
>  drivers/fpga/altera-fpga2sdram.c                   |  174 ++++++++
>  drivers/fpga/altera-hps2fpga.c                     |  213 +++++++++
>  drivers/fpga/fpga-bridge.c                         |  388 +++++++++++++++++
>  drivers/fpga/fpga-region.c                         |  460 ++++++++++++++++++++
>  include/linux/fpga/fpga-bridge.h                   |   55 +++
>  11 files changed, 1739 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-bridge
>  create mode 100644 Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
>  create mode 100644 Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
>  create mode 100644 drivers/fpga/altera-fpga2sdram.c
>  create mode 100644 drivers/fpga/altera-hps2fpga.c
>  create mode 100644 drivers/fpga/fpga-bridge.c
>  create mode 100644 drivers/fpga/fpga-region.c
>  create mode 100644 include/linux/fpga/fpga-bridge.h
> 
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH v16 0/6] Device Tree support for FPGA programming

[Rob Herring]

On Thu, Feb 11, 2016 at 2:49 PM, atull <atull@opensource.altera.com> wrote:
> On Fri, 5 Feb 2016, atull@opensource.altera.com wrote:
>
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> v16 Refactors the FPGA Area and FPGA Bus into single thing called an
>> FPGA Region and eliminates using simple-bus.  I'm using the word
>> "region" as it's a term is used in the literature of both the major
>> FPGA manufacturors.
>>
>> Changes for v16:
>> * Refactor the FPGA Area and FPGA Bus into a FPGA Region.
>> * Don't use simple-bus.
>> * FPGA Managers and FPGA Bridges are now specified by phandle using the
>>   "fpga-mgr" and "fpga-bridges" properties.  fpga-bridges can specify
>>   more than one bridge.
>> * Device Tree overlays should be targeted to a FPGA Region.
>> * The overlays need only contain firmware-name and the child nodes.
>> * To model a system containing >1 partial reconfiguration region,
>>   an overlay could add FPGA Regions to the base FPGA Regions.
>> * Child FPGA Regions inherit the parent FGPA Manager, but specify
>>   their own set of bridges if needes as partial reconfig regions
>>   will likely need their own bridges.
>> * All this is discussed in bindings/fpga/fpga-region.txt
>>
>> One other highlight:
>> The little engine that runs this thing is a reconfig notifier
>> in fpga-region.c.  This notifier that will program an FPGA if a
>> "firmware-name" property gets added to a fpga-region.  Then
>> it will call of_platform_populate().  The current behavior in Linux
>> when a DT overlay is applied is that the reconfig notifications
>> go out in heirarchical order: first notifications are for the
>> properties, then notifications for the child nodes.  So an overlay
>> that adds a 'firmware-name' property and some child nodes to a
>> fpga-region will cause FPGA programming and child node
>> populating in the right order.
>
> I figured out how to get rid of the reconfig notifier.
>
>>
>> One issue with the dynamic DT stuff:
>> I've tried returning and error from the notifier if FPGA programming
>> fails; the error is noted on the console, but the child nodes
>> get probed anyway.
>
> I looked into it further and now I've got a solution for this issue
> that I can post soon.  I can stop using the DT overlay configfs
> interface and add a sysfs file for applying an overlay to an FPGA
> region.  The FPGA region implementation will see the overlay before it
> becomes part of the live tree.  Then it can do the FPGA programming
> and see that succeed before the child nodes become part of the live
> tree.  If FPGA programming fails, the overlay will be rejected before
> it becomes part of the live tree.  By the time 'firmware-name' and the
> child nodes show up in the live tree, they will be post-configuration
> information.

Um, no. We don't need 2 interfaces for loading overlays from
userspace. I could see this being a common problem and it needs to be
solved. But given the configfs interface is not upstream yet, perhaps
you should worry about that after the current series is in.

Perhaps we need a pre-add notifier and the core will only load the
overlay if nothing handles it. Really, a solution without notifiers
would be preferred. Maybe register handlers with the DT core for
certain paths.

Rob

Re: [PATCH v16 0/6] Device Tree support for FPGA programming

[atull]

On Thu, 11 Feb 2016, Rob Herring wrote:

> On Thu, Feb 11, 2016 at 2:49 PM, atull <atull@opensource.altera.com> wrote:
> > On Fri, 5 Feb 2016, atull@opensource.altera.com wrote:
> >
> >> From: Alan Tull <atull@opensource.altera.com>
> >>
> >> v16 Refactors the FPGA Area and FPGA Bus into single thing called an
> >> FPGA Region and eliminates using simple-bus.  I'm using the word
> >> "region" as it's a term is used in the literature of both the major
> >> FPGA manufacturors.
> >>
> >> Changes for v16:
> >> * Refactor the FPGA Area and FPGA Bus into a FPGA Region.
> >> * Don't use simple-bus.
> >> * FPGA Managers and FPGA Bridges are now specified by phandle using the
> >>   "fpga-mgr" and "fpga-bridges" properties.  fpga-bridges can specify
> >>   more than one bridge.
> >> * Device Tree overlays should be targeted to a FPGA Region.
> >> * The overlays need only contain firmware-name and the child nodes.
> >> * To model a system containing >1 partial reconfiguration region,
> >>   an overlay could add FPGA Regions to the base FPGA Regions.
> >> * Child FPGA Regions inherit the parent FGPA Manager, but specify
> >>   their own set of bridges if needes as partial reconfig regions
> >>   will likely need their own bridges.
> >> * All this is discussed in bindings/fpga/fpga-region.txt
> >>
> >> One other highlight:
> >> The little engine that runs this thing is a reconfig notifier
> >> in fpga-region.c.  This notifier that will program an FPGA if a
> >> "firmware-name" property gets added to a fpga-region.  Then
> >> it will call of_platform_populate().  The current behavior in Linux
> >> when a DT overlay is applied is that the reconfig notifications
> >> go out in heirarchical order: first notifications are for the
> >> properties, then notifications for the child nodes.  So an overlay
> >> that adds a 'firmware-name' property and some child nodes to a
> >> fpga-region will cause FPGA programming and child node
> >> populating in the right order.
> >
> > I figured out how to get rid of the reconfig notifier.
> >
> >>
> >> One issue with the dynamic DT stuff:
> >> I've tried returning and error from the notifier if FPGA programming
> >> fails; the error is noted on the console, but the child nodes
> >> get probed anyway.
> >
> > I looked into it further and now I've got a solution for this issue
> > that I can post soon.  I can stop using the DT overlay configfs
> > interface and add a sysfs file for applying an overlay to an FPGA
> > region.  The FPGA region implementation will see the overlay before it
> > becomes part of the live tree.  Then it can do the FPGA programming
> > and see that succeed before the child nodes become part of the live
> > tree.  If FPGA programming fails, the overlay will be rejected before
> > it becomes part of the live tree.  By the time 'firmware-name' and the
> > child nodes show up in the live tree, they will be post-configuration
> > information.
> 
> Um, no. We don't need 2 interfaces for loading overlays from
> userspace. I could see this being a common problem and it needs to be
> solved. But given the configfs interface is not upstream yet, perhaps
> you should worry about that after the current series is in.
> 
> Perhaps we need a pre-add notifier and the core will only load the
> overlay if nothing handles it. Really, a solution without notifiers
> would be preferred. Maybe register handlers with the DT core for
> certain paths.
> 
> Rob
> 

Yes.  If any handler returns error, the overlay doesn't go into the
main tree.  Handler type to be registed could be:

  int pre_add_handler(struct device_node *overlay,
                      struct device_node *target)

That gives us the overlay after it's been unflattened and phandles
resolved and the node that it was targeted to.  I was going to
need find_target_node() to be exported, but this avoids that.

Registration could by compatible string, of match, or path.  Path
would be too rigid in my case, I'd want to register for compatible
"fpga-region"

Alan

Re: [PATCH v16 0/6] Device Tree support for FPGA programming

[atull]

On Thu, 11 Feb 2016, atull wrote:

> On Thu, 11 Feb 2016, Rob Herring wrote:
> 
> > On Thu, Feb 11, 2016 at 2:49 PM, atull <atull@opensource.altera.com> wrote:
> > > On Fri, 5 Feb 2016, atull@opensource.altera.com wrote:
> > >
> > >> From: Alan Tull <atull@opensource.altera.com>
> > >>
> > >> v16 Refactors the FPGA Area and FPGA Bus into single thing called an
> > >> FPGA Region and eliminates using simple-bus.  I'm using the word
> > >> "region" as it's a term is used in the literature of both the major
> > >> FPGA manufacturors.
> > >>
> > >> Changes for v16:
> > >> * Refactor the FPGA Area and FPGA Bus into a FPGA Region.
> > >> * Don't use simple-bus.
> > >> * FPGA Managers and FPGA Bridges are now specified by phandle using the
> > >>   "fpga-mgr" and "fpga-bridges" properties.  fpga-bridges can specify
> > >>   more than one bridge.
> > >> * Device Tree overlays should be targeted to a FPGA Region.
> > >> * The overlays need only contain firmware-name and the child nodes.
> > >> * To model a system containing >1 partial reconfiguration region,
> > >>   an overlay could add FPGA Regions to the base FPGA Regions.
> > >> * Child FPGA Regions inherit the parent FGPA Manager, but specify
> > >>   their own set of bridges if needes as partial reconfig regions
> > >>   will likely need their own bridges.
> > >> * All this is discussed in bindings/fpga/fpga-region.txt
> > >>
> > >> One other highlight:
> > >> The little engine that runs this thing is a reconfig notifier
> > >> in fpga-region.c.  This notifier that will program an FPGA if a
> > >> "firmware-name" property gets added to a fpga-region.  Then
> > >> it will call of_platform_populate().  The current behavior in Linux
> > >> when a DT overlay is applied is that the reconfig notifications
> > >> go out in heirarchical order: first notifications are for the
> > >> properties, then notifications for the child nodes.  So an overlay
> > >> that adds a 'firmware-name' property and some child nodes to a
> > >> fpga-region will cause FPGA programming and child node
> > >> populating in the right order.
> > >
> > > I figured out how to get rid of the reconfig notifier.
> > >
> > >>
> > >> One issue with the dynamic DT stuff:
> > >> I've tried returning and error from the notifier if FPGA programming
> > >> fails; the error is noted on the console, but the child nodes
> > >> get probed anyway.
> > >
> > > I looked into it further and now I've got a solution for this issue
> > > that I can post soon.  I can stop using the DT overlay configfs
> > > interface and add a sysfs file for applying an overlay to an FPGA
> > > region.  The FPGA region implementation will see the overlay before it
> > > becomes part of the live tree.  Then it can do the FPGA programming
> > > and see that succeed before the child nodes become part of the live
> > > tree.  If FPGA programming fails, the overlay will be rejected before
> > > it becomes part of the live tree.  By the time 'firmware-name' and the
> > > child nodes show up in the live tree, they will be post-configuration
> > > information.
> > 
> > Um, no. We don't need 2 interfaces for loading overlays from
> > userspace. I could see this being a common problem and it needs to be
> > solved. But given the configfs interface is not upstream yet, perhaps
> > you should worry about that after the current series is in.
> > 
> > Perhaps we need a pre-add notifier and the core will only load the
> > overlay if nothing handles it. Really, a solution without notifiers
> > would be preferred. Maybe register handlers with the DT core for
> > certain paths.
> > 
> > Rob
> > 
> 
> Yes.  If any handler returns error, the overlay doesn't go into the
> main tree.  Handler type to be registed could be:
> 
>   int pre_add_handler(struct device_node *overlay,
>                       struct device_node *target)

And a third parameter of some flags to indicate whether the
overlay is being added or removed.

> 
> That gives us the overlay after it's been unflattened and phandles
> resolved and the node that it was targeted to.  I was going to
> need find_target_node() to be exported, but this avoids that.
> 
> Registration could by compatible string, of match, or path.  Path
> would be too rigid in my case, I'd want to register for compatible
> "fpga-region"
> 
> Alan
> 

Re: [PATCH v16 0/6] Device Tree support for FPGA programming

[Moritz Fischer]

Hi Alan,

On Thu, Feb 11, 2016 at 2:17 PM, atull <atull@opensource.altera.com> wrote:

>> > > I looked into it further and now I've got a solution for this issue
>> > > that I can post soon.  I can stop using the DT overlay configfs
>> > > interface and add a sysfs file for applying an overlay to an FPGA
>> > > region.  The FPGA region implementation will see the overlay before it
>> > > becomes part of the live tree.  Then it can do the FPGA programming
>> > > and see that succeed before the child nodes become part of the live
>> > > tree.  If FPGA programming fails, the overlay will be rejected before
>> > > it becomes part of the live tree.  By the time 'firmware-name' and the
>> > > child nodes show up in the live tree, they will be post-configuration
>> > > information.

I agree this would be a very nice interface, but Rob is right, having
two different
interfaces to load device-trees is probably bad in general.

>> Yes.  If any handler returns error, the overlay doesn't go into the
>> main tree.  Handler type to be registed could be:
>>
>>   int pre_add_handler(struct device_node *overlay,
>>                       struct device_node *target)
>
> And a third parameter of some flags to indicate whether the
> overlay is being added or removed.

Looks good to me.

>> That gives us the overlay after it's been unflattened and phandles
>> resolved and the node that it was targeted to.  I was going to
>> need find_target_node() to be exported, but this avoids that.
>>
>> Registration could by compatible string, of match, or path.  Path
>> would be too rigid in my case, I'd want to register for compatible
>> "fpga-region"

Looks good to me. I think we're getting pretty close :)

Moritz