[PATCH v12 0/6] simple fpga bus and fpga bridge framework

[PATCH v12 0/6] simple fpga bus and fpga bridge framework

[atull]

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

The Simple FPGA Bus provides a manufacturer-agnostic interface
for reprogramming FPGAs using Device Tree Overlays.  It uses the
FPGA Bridge Framework and the upstreamed FPGA Manager Framework.

When a Device Tree Overlay is applied, the Simple FPGA Bus will
use information the overlay to:
 * Disable FPGA bridges
 * Reprogram the FPGA
 * Enable FPGA bridges
 * Populate the child devices

Simple FPGA Bus was originally submitted with the FPGA Manager
Framework which was accepted upstream.  This version uses the
FPGA Bridge Framework to provide methods to enable/bridges.

Alan Tull (6):
  fpga: add usage documentation for simple fpga bus
  fpga: add bindings document for simple fpga bus
  fpga: add simple-fpga-bus
  fpga: add fpga bridge framework
  ARM: socfpga: add bindings document for fpga bridge drivers
  ARM: socfpga: fpga bridge driver support

 .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   18 ++
 .../bindings/fpga/altera-hps2fpga-bridge.txt       |   36 +++
 .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 +++++
 Documentation/fpga/simple-fpga-bus.txt             |   58 ++++
 drivers/fpga/Kconfig                               |   21 ++
 drivers/fpga/Makefile                              |    7 +
 drivers/fpga/altera-fpga2sdram.c                   |  185 +++++++++++
 drivers/fpga/altera-hps2fpga.c                     |  234 ++++++++++++++
 drivers/fpga/fpga-bridge.c                         |  242 +++++++++++++++
 drivers/fpga/simple-fpga-bus.c                     |  327 ++++++++++++++++++++
 include/linux/fpga/fpga-bridge.h                   |   49 +++
 11 files changed, 1258 insertions(+)
 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/simple-fpga-bus.txt
 create mode 100644 Documentation/fpga/simple-fpga-bus.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/simple-fpga-bus.c
 create mode 100644 include/linux/fpga/fpga-bridge.h

-- 
1.7.9.5

[PATCH v12 1/6] fpga: add usage documentation for simple fpga bus

[atull]

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

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

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v9:  Initial version of this patch in patchset
v10: s/fpga/FPGA/g
     improve formatting
     some rewriting
     move to staging/simple-fpga-bus
v11: No change in this patch for v11 of the patch set
v12: Moved out of staging
     Small changes due to using FPGA bridge framework and not
     representing the bridges as resets.
---
 Documentation/fpga/simple-fpga-bus.txt |   58 ++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/fpga/simple-fpga-bus.txt

diff --git a/Documentation/fpga/simple-fpga-bus.txt b/Documentation/fpga/simple-fpga-bus.txt
new file mode 100644
index 0000000..bd43478
--- /dev/null
+++ b/Documentation/fpga/simple-fpga-bus.txt
@@ -0,0 +1,58 @@
+Simple FPGA Bus
+
+Alan Tull 2015
+
+Overview
+========
+
+The simple FPGA bus adds device tree overlay support for FPGA's.  Loading a
+DT overlay will result in the FPGA getting an image loaded, its bridges will
+be released, and the DT populated for nodes below the simple-fpga-bus.  This
+results in drivers getting probed for the hardware that just got added.  This
+is intended to support the FPGA usage where the FPGA has hardware that
+requires drivers.  Removing the overlay will result in the drivers getting
+removed and the bridges being disabled.
+
+The simple FPGA bus will need to disable and enable bridges that will only
+affect the child devices that are below the bus.  If partial reconfiguration
+is to be done, then bridges will need to be added within the FPGA design to
+protect the rest of the bus when one part of the FPGA design is being
+reconfigured.
+
+
+Sequence
+========
+
+Load the DT overlay.  One way to do that from user space is to use Pantelis'
+DT-Overlay configfs interface.
+
+This causes the simple FPGA bus go be probed and will do the following:
+ 1. Disable the FPGA bridges.
+ 2. Call the FPGA manager core to program the FPGA.
+ 3. Release the FPGA bridges.
+ 4. Call of_platform_populate resulting in device drivers getting probed.
+
+
+Requirements
+============
+
+ 1. An FPGA image that has a hardware block or blocks that use drivers that are
+    supported in the kernel.
+ 2. A device tree overlay (example is in the simple-fpga-bus bindings document).
+ 3. A FPGA manager driver supporting writing the FPGA.
+ 4. FPGA bridge drivers.
+
+The DT overlay includes bindings (documented in bindings/simple-fpga-bus.txt)
+that specify:
+ * Which FPGA manager to use.
+ * Which image file to load.
+ * Flags indicating whether this this image is for full reconfiguration or
+   partial.
+ * A list of FPGA bridges.
+ * Child nodes specifying the devices that will be added with appropriate
+   compatible strings, etc.
+
+Since this code uses the firmware interface to get the image and DT overlay,
+they currently have to be files on the file system.  It doesn't have to be that
+way forever as DT bindings could be added to point to other sources for the
+image.
-- 
1.7.9.5

Re: [PATCH v12 1/6] fpga: add usage documentation for simple fpga bus

[Moritz Fischer]

Hi Alan,

great docs! Couple of nits inline below

On Tue, Oct 27, 2015 at 3:09 PM,  <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
> From: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>
> Add a document spelling out usage of the simple fpga bus.
>
> Signed-off-by: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> ---
> v9:  Initial version of this patch in patchset
> v10: s/fpga/FPGA/g
>      improve formatting
>      some rewriting
>      move to staging/simple-fpga-bus
> v11: No change in this patch for v11 of the patch set
> v12: Moved out of staging
>      Small changes due to using FPGA bridge framework and not
>      representing the bridges as resets.
> ---
>  Documentation/fpga/simple-fpga-bus.txt |   58 ++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/fpga/simple-fpga-bus.txt
>
> diff --git a/Documentation/fpga/simple-fpga-bus.txt b/Documentation/fpga/simple-fpga-bus.txt
> new file mode 100644
> index 0000000..bd43478
> --- /dev/null
> +++ b/Documentation/fpga/simple-fpga-bus.txt
> @@ -0,0 +1,58 @@
> +Simple FPGA Bus
> +
> +Alan Tull 2015
> +
> +Overview
> +========
> +
> +The simple FPGA bus adds device tree overlay support for FPGA's.  Loading a

FPGAs
> +DT overlay will result in the FPGA getting an image loaded, its bridges will

I'd prefer 'will result in the FPGA being programmed with an image'

> +be released, and the DT populated for nodes below the simple-fpga-bus.  This
> +results in drivers getting probed for the hardware that just got added.  This
> +is intended to support the FPGA usage where the FPGA has hardware that
> +requires drivers.  Removing the overlay will result in the drivers getting
> +removed and the bridges being disabled.
> +
> +The simple FPGA bus will need to disable and enable bridges that will only
> +affect the child devices that are below the bus.  If partial reconfiguration
> +is to be done, then bridges will need to be added within the FPGA design to
> +protect the rest of the bus when one part of the FPGA design is being
> +reconfigured.
> +
> +
> +Sequence
> +========
> +
> +Load the DT overlay.  One way to do that from user space is to use Pantelis'
> +DT-Overlay configfs interface.
> +
> +This causes the simple FPGA bus go be probed and will do the following:

I think you mean *to* be probed
> + 1. Disable the FPGA bridges.
> + 2. Call the FPGA manager core to program the FPGA.
> + 3. Release the FPGA bridges.
> + 4. Call of_platform_populate resulting in device drivers getting probed.
> +
> +
> +Requirements
> +============
> +
> + 1. An FPGA image that has a hardware block or blocks that use drivers that are
> +    supported in the kernel.
> + 2. A device tree overlay (example is in the simple-fpga-bus bindings document).
> + 3. A FPGA manager driver supporting writing the FPGA.
> + 4. FPGA bridge drivers.
> +
> +The DT overlay includes bindings (documented in bindings/simple-fpga-bus.txt)
> +that specify:
> + * Which FPGA manager to use.
> + * Which image file to load.
> + * Flags indicating whether this this image is for full reconfiguration or
> +   partial.
> + * A list of FPGA bridges.
> + * Child nodes specifying the devices that will be added with appropriate
> +   compatible strings, etc.
> +
> +Since this code uses the firmware interface to get the image and DT overlay,
> +they currently have to be files on the file system.  It doesn't have to be that
> +way forever as DT bindings could be added to point to other sources for the
> +image.

Having this will be really useful, but I agree this can be a follow up patch.
> --
> 1.7.9.5
>

I'll take a closer look but feel free to add a:

Reviewed-by: Moritz Fischer <moritz.fischer-+aYTwkv1SeIAvxtiuMwx3w@public.gmane.org>

Cheers,

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

Re: [PATCH v12 1/6] fpga: add usage documentation for simple fpga bus

[atull]

On Wed, 28 Oct 2015, Moritz Fischer wrote:

Hi Moritz!

> Hi Alan,
> 
> great docs! Couple of nits inline below
> 
> On Tue, Oct 27, 2015 at 3:09 PM,  <atull@opensource.altera.com> wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> >
> > Add a document spelling out usage of the simple fpga bus.
> >
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > ---
> > v9:  Initial version of this patch in patchset
> > v10: s/fpga/FPGA/g
> >      improve formatting
> >      some rewriting
> >      move to staging/simple-fpga-bus
> > v11: No change in this patch for v11 of the patch set
> > v12: Moved out of staging
> >      Small changes due to using FPGA bridge framework and not
> >      representing the bridges as resets.
> > ---
> >  Documentation/fpga/simple-fpga-bus.txt |   58 ++++++++++++++++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >  create mode 100644 Documentation/fpga/simple-fpga-bus.txt
> >
> > diff --git a/Documentation/fpga/simple-fpga-bus.txt b/Documentation/fpga/simple-fpga-bus.txt
> > new file mode 100644
> > index 0000000..bd43478
> > --- /dev/null
> > +++ b/Documentation/fpga/simple-fpga-bus.txt
> > @@ -0,0 +1,58 @@
> > +Simple FPGA Bus
> > +
> > +Alan Tull 2015
> > +
> > +Overview
> > +========
> > +
> > +The simple FPGA bus adds device tree overlay support for FPGA's.  Loading a
> 
> FPGAs

D'oh!

> > +DT overlay will result in the FPGA getting an image loaded, its bridges will
> 
> I'd prefer 'will result in the FPGA being programmed with an image'

OK

> 
> > +be released, and the DT populated for nodes below the simple-fpga-bus.  This
> > +results in drivers getting probed for the hardware that just got added.  This
> > +is intended to support the FPGA usage where the FPGA has hardware that
> > +requires drivers.  Removing the overlay will result in the drivers getting
> > +removed and the bridges being disabled.
> > +
> > +The simple FPGA bus will need to disable and enable bridges that will only
> > +affect the child devices that are below the bus.  If partial reconfiguration
> > +is to be done, then bridges will need to be added within the FPGA design to
> > +protect the rest of the bus when one part of the FPGA design is being
> > +reconfigured.
> > +
> > +
> > +Sequence
> > +========
> > +
> > +Load the DT overlay.  One way to do that from user space is to use Pantelis'
> > +DT-Overlay configfs interface.
> > +
> > +This causes the simple FPGA bus go be probed and will do the following:
> 
> I think you mean *to* be probed

Yes

> > + 1. Disable the FPGA bridges.
> > + 2. Call the FPGA manager core to program the FPGA.
> > + 3. Release the FPGA bridges.
> > + 4. Call of_platform_populate resulting in device drivers getting probed.
> > +
> > +
> > +Requirements
> > +============
> > +
> > + 1. An FPGA image that has a hardware block or blocks that use drivers that are
> > +    supported in the kernel.
> > + 2. A device tree overlay (example is in the simple-fpga-bus bindings document).
> > + 3. A FPGA manager driver supporting writing the FPGA.
> > + 4. FPGA bridge drivers.
> > +
> > +The DT overlay includes bindings (documented in bindings/simple-fpga-bus.txt)
> > +that specify:
> > + * Which FPGA manager to use.
> > + * Which image file to load.
> > + * Flags indicating whether this this image is for full reconfiguration or
> > +   partial.
> > + * A list of FPGA bridges.
> > + * Child nodes specifying the devices that will be added with appropriate
> > +   compatible strings, etc.
> > +
> > +Since this code uses the firmware interface to get the image and DT overlay,
> > +they currently have to be files on the file system.  It doesn't have to be that
> > +way forever as DT bindings could be added to point to other sources for the
> > +image.
> 
> Having this will be really useful, but I agree this can be a follow up patch.
> > --
> > 1.7.9.5
> >
> 
> I'll take a closer look but feel free to add a:
> 
> Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>
> 
> Cheers,
> 
> Moritz
> 

Thanks for the review, I'll fix the nits.

Alan

[PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[atull]

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

New bindings document for simple fpga bus.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
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.
---
 .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt

diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
new file mode 100644
index 0000000..2e742f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
@@ -0,0 +1,81 @@
+Simple FPGA Bus
+===============
+
+A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
+before populating the devices below its node.  All this happens when a device
+tree overlay is added to the live tree.  This document describes that device
+tree overlay.
+
+Required properties:
+- compatible : should contain "simple-fpga-bus"
+- #address-cells, #size-cells, ranges: must be present to handle address space
+  mapping for children.
+
+Optional properties:
+- fpga-mgr : should contain a phandle to a FPGA manager.
+- fpga-firmware : should contain the name of a FPGA image file located on the
+  firmware search path.
+- partial-reconfig : boolean property should be defined if partial
+  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
+  is done.
+- fpga-bridges : should contain a list of bridges that the bus will disable
+  before   programming the FPGA and then enable after the FPGA has been
+
+Example:
+
+/dts-v1/;
+/plugin/;
+/ {
+	fragment@0 {
+		target-path="/soc";
+		__overlay__ {
+			#address-cells = <1>;
+	                #size-cells = <1>;
+
+			bridge@0xff200000 {
+				compatible = "simple-fpga-bus";
+				reg = <0xc0000000 0x20000000>,
+				      <0xff200000 0x00200000>;
+				reg-names = "axi_h2f", "axi_h2f_lw";
+
+				#address-cells = <0x2>;
+				#size-cells = <0x1>;
+
+				ranges = <0x00000000 0x00000000 0xc0000000 0x00010000>,
+					 <0x00000001 0x00020000 0xff220000 0x00000008>,
+					 <0x00000001 0x00010040 0xff210040 0x00000020>;
+
+				clocks = <0x2 0x2>;
+				clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
+
+				fpga-mgr = <&hps_0_fpgamgr>;
+				fpga-firmware = "soc_system.rbf";
+
+				fpga-bridges = <&hps_fpgabridge0>, <&hps_fpgabridge1>, <&hps_fpgabridge2>;
+
+				onchip_memory2_0: memory@0x000000000 {
+					device_type = "memory";
+					compatible = "ALTR,onchipmem-15.1";
+					reg = <0x00000000 0x00000000 0x00010000>;
+				};
+
+				jtag_uart: serial@0x100020000 {
+					compatible = "altr,juart-15.1", "altr,juart-1.0";
+					reg = <0x00000001 0x00020000 0x00000008>;
+					interrupt-parent = <&intc>;
+					interrupts = <0 42 4>;
+				};
+
+				led_pio: gpio@0x100010040 {
+					compatible = "altr,pio-15.1", "altr,pio-1.0";
+					reg = <0x00000001 0x00010040 0x00000020>;
+					altr,gpio-bank-width = <4>;
+					resetvalue = <0>;
+					#gpio-cells = <2>;
+					gpio-controller;
+				};
+			};
+		};
+	};
+};
+
-- 
1.7.9.5

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[Steffen Trumtrar]

Hi!

On Tue, Oct 27, 2015 at 05:09:11PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> New bindings document for simple fpga bus.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
> 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.
> ---
>  .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> new file mode 100644
> index 0000000..2e742f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> @@ -0,0 +1,81 @@
> +Simple FPGA Bus
> +===============
> +
> +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> +before populating the devices below its node.  All this happens when a device
> +tree overlay is added to the live tree.  This document describes that device
> +tree overlay.
> +

This is not really true, is it?
The driver should work without applying the overlay, e.g. the bootloader
might have already done it.

> +Required properties:
> +- compatible : should contain "simple-fpga-bus"
> +- #address-cells, #size-cells, ranges: must be present to handle address space
> +  mapping for children.
> +
> +Optional properties:
> +- fpga-mgr : should contain a phandle to a FPGA manager.
> +- fpga-firmware : should contain the name of a FPGA image file located on the
> +  firmware search path.
> +- partial-reconfig : boolean property should be defined if partial
> +  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
> +  is done.
> +- fpga-bridges : should contain a list of bridges that the bus will disable
> +  before   programming the FPGA and then enable after the FPGA has been
           ^^^                                                            ???

> +
> +Example:
> +
> +/dts-v1/;
> +/plugin/;
> +/ {
> +	fragment@0 {
> +		target-path="/soc";
> +		__overlay__ {
> +			#address-cells = <1>;
> +	                #size-cells = <1>;
> +
> +			bridge@0xff200000 {
                               ^^

> +				compatible = "simple-fpga-bus";
> +				reg = <0xc0000000 0x20000000>,
> +				      <0xff200000 0x00200000>;
> +				reg-names = "axi_h2f", "axi_h2f_lw";
> +
> +				#address-cells = <0x2>;
> +				#size-cells = <0x1>;
> +
> +				ranges = <0x00000000 0x00000000 0xc0000000 0x00010000>,
> +					 <0x00000001 0x00020000 0xff220000 0x00000008>,
> +					 <0x00000001 0x00010040 0xff210040 0x00000020>;
> +
> +				clocks = <0x2 0x2>;
> +				clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
> +
> +				fpga-mgr = <&hps_0_fpgamgr>;
> +				fpga-firmware = "soc_system.rbf";
> +
> +				fpga-bridges = <&hps_fpgabridge0>, <&hps_fpgabridge1>, <&hps_fpgabridge2>;
> +
> +				onchip_memory2_0: memory@0x000000000 {
                                                         ^^

> +					device_type = "memory";
> +					compatible = "ALTR,onchipmem-15.1";
> +					reg = <0x00000000 0x00000000 0x00010000>;
> +				};
> +
> +				jtag_uart: serial@0x100020000 {
                                                  ^^

> +					compatible = "altr,juart-15.1", "altr,juart-1.0";
> +					reg = <0x00000001 0x00020000 0x00000008>;
> +					interrupt-parent = <&intc>;
> +					interrupts = <0 42 4>;
> +				};
> +
> +				led_pio: gpio@0x100010040 {
                                              ^^

No 0x, please.

> +					compatible = "altr,pio-15.1", "altr,pio-1.0";
> +					reg = <0x00000001 0x00010040 0x00000020>;
> +					altr,gpio-bank-width = <4>;
> +					resetvalue = <0>;
> +					#gpio-cells = <2>;
> +					gpio-controller;
> +				};
> +			};
> +		};
> +	};
> +};
> +

Regards,
Steffen

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

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[atull]

On Wed, 28 Oct 2015, Steffen Trumtrar wrote:

> Hi!
> 
> On Tue, Oct 27, 2015 at 05:09:11PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > New bindings document for simple fpga bus.
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > ---
> > 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.
> > ---
> >  .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> > new file mode 100644
> > index 0000000..2e742f7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> > @@ -0,0 +1,81 @@
> > +Simple FPGA Bus
> > +===============
> > +
> > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> > +before populating the devices below its node.  All this happens when a device
> > +tree overlay is added to the live tree.  This document describes that device
> > +tree overlay.
> > +
> 
> This is not really true, is it?
> The driver should work without applying the overlay, e.g. the bootloader
> might have already done it.
> 

Yes it's true.  I'm not clear what you are saying.  If the bootloader has
programmed the FPGA, the overlay can leave out the optional properties
and the FPGA won't get reprogrammed; the child devices will still get
added and probed.  So this handles both the case where you want to reprogram
the FPGA under Linux and where the FPGA was programmed by a bootloader.

I'll fix the minor nits you are pointing out below.  Thanks for the feedback.

Alan

> > +Required properties:
> > +- compatible : should contain "simple-fpga-bus"
> > +- #address-cells, #size-cells, ranges: must be present to handle address space
> > +  mapping for children.
> > +
> > +Optional properties:
> > +- fpga-mgr : should contain a phandle to a FPGA manager.
> > +- fpga-firmware : should contain the name of a FPGA image file located on the
> > +  firmware search path.
> > +- partial-reconfig : boolean property should be defined if partial
> > +  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
> > +  is done.
> > +- fpga-bridges : should contain a list of bridges that the bus will disable
> > +  before   programming the FPGA and then enable after the FPGA has been
>            ^^^                                                            ???
> 
> > +
> > +Example:
> > +
> > +/dts-v1/;
> > +/plugin/;
> > +/ {
> > +	fragment@0 {
> > +		target-path="/soc";
> > +		__overlay__ {
> > +			#address-cells = <1>;
> > +	                #size-cells = <1>;
> > +
> > +			bridge@0xff200000 {
>                                ^^
> 
> > +				compatible = "simple-fpga-bus";
> > +				reg = <0xc0000000 0x20000000>,
> > +				      <0xff200000 0x00200000>;
> > +				reg-names = "axi_h2f", "axi_h2f_lw";
> > +
> > +				#address-cells = <0x2>;
> > +				#size-cells = <0x1>;
> > +
> > +				ranges = <0x00000000 0x00000000 0xc0000000 0x00010000>,
> > +					 <0x00000001 0x00020000 0xff220000 0x00000008>,
> > +					 <0x00000001 0x00010040 0xff210040 0x00000020>;
> > +
> > +				clocks = <0x2 0x2>;
> > +				clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
> > +
> > +				fpga-mgr = <&hps_0_fpgamgr>;
> > +				fpga-firmware = "soc_system.rbf";
> > +
> > +				fpga-bridges = <&hps_fpgabridge0>, <&hps_fpgabridge1>, <&hps_fpgabridge2>;
> > +
> > +				onchip_memory2_0: memory@0x000000000 {
>                                                          ^^
> 
> > +					device_type = "memory";
> > +					compatible = "ALTR,onchipmem-15.1";
> > +					reg = <0x00000000 0x00000000 0x00010000>;
> > +				};
> > +
> > +				jtag_uart: serial@0x100020000 {
>                                                   ^^
> 
> > +					compatible = "altr,juart-15.1", "altr,juart-1.0";
> > +					reg = <0x00000001 0x00020000 0x00000008>;
> > +					interrupt-parent = <&intc>;
> > +					interrupts = <0 42 4>;
> > +				};
> > +
> > +				led_pio: gpio@0x100010040 {
>                                               ^^
> 
> No 0x, please.
> 
> > +					compatible = "altr,pio-15.1", "altr,pio-1.0";
> > +					reg = <0x00000001 0x00010040 0x00000020>;
> > +					altr,gpio-bank-width = <4>;
> > +					resetvalue = <0>;
> > +					#gpio-cells = <2>;
> > +					gpio-controller;
> > +				};
> > +			};
> > +		};
> > +	};
> > +};
> > +
> 
> Regards,
> Steffen
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[Moritz Fischer]

On Wed, Oct 28, 2015 at 7:53 AM, atull <atull@opensource.altera.com> wrote:

>> > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
>> > +before populating the devices below its node.  All this happens when a device
>> > +tree overlay is added to the live tree.  This document describes that device
>> > +tree overlay.
>> > +
>>
>> This is not really true, is it?
>> The driver should work without applying the overlay, e.g. the bootloader
>> might have already done it.
>>
>
> Yes it's true.  I'm not clear what you are saying.  If the bootloader has
> programmed the FPGA, the overlay can leave out the optional properties
> and the FPGA won't get reprogrammed; the child devices will still get
> added and probed.  So this handles both the case where you want to reprogram
> the FPGA under Linux and where the FPGA was programmed by a bootloader.

I think what he means is that the document explicitly calls out the overlay,
when in theory it also works without an overlay. While being the most 'natural'
use-case, it is not the only one ;-)

Moritz

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[atull]

On Wed, 28 Oct 2015, Moritz Fischer wrote:

> On Wed, Oct 28, 2015 at 7:53 AM, atull <atull@opensource.altera.com> wrote:
> 
> >> > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> >> > +before populating the devices below its node.  All this happens when a device
> >> > +tree overlay is added to the live tree.  This document describes that device
> >> > +tree overlay.
> >> > +
> >>
> >> This is not really true, is it?
> >> The driver should work without applying the overlay, e.g. the bootloader
> >> might have already done it.
> >>
> >
> > Yes it's true.  I'm not clear what you are saying.  If the bootloader has
> > programmed the FPGA, the overlay can leave out the optional properties
> > and the FPGA won't get reprogrammed; the child devices will still get
> > added and probed.  So this handles both the case where you want to reprogram
> > the FPGA under Linux and where the FPGA was programmed by a bootloader.
> 
> I think what he means is that the document explicitly calls out the overlay,
> when in theory it also works without an overlay. While being the most 'natural'
> use-case, it is not the only one ;-)
> 
> Moritz
> 

Yes, I can just take out the two sentences that refer to overlays and just 
leave:

A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
before populating the devices below its node.

Alan

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[Steffen Trumtrar]

On Tue, Oct 27, 2015 at 05:09:11PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> New bindings document for simple fpga bus.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
> 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.
> ---
>  .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> new file mode 100644
> index 0000000..2e742f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> @@ -0,0 +1,81 @@
> +Simple FPGA Bus
> +===============
> +
> +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> +before populating the devices below its node.  All this happens when a device
> +tree overlay is added to the live tree.  This document describes that device
> +tree overlay.
> +
> +Required properties:
> +- compatible : should contain "simple-fpga-bus"
> +- #address-cells, #size-cells, ranges: must be present to handle address space
> +  mapping for children.
> +
> +Optional properties:
> +- fpga-mgr : should contain a phandle to a FPGA manager.
> +- fpga-firmware : should contain the name of a FPGA image file located on the
> +  firmware search path.
> +- partial-reconfig : boolean property should be defined if partial
> +  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
> +  is done.
> +- fpga-bridges : should contain a list of bridges that the bus will disable
> +  before   programming the FPGA and then enable after the FPGA has been
> +
> +Example:
> +
> +/dts-v1/;
> +/plugin/;
> +/ {
> +	fragment@0 {
> +		target-path="/soc";
> +		__overlay__ {
> +			#address-cells = <1>;
> +	                #size-cells = <1>;
> +
> +			bridge@0xff200000 {
> +				compatible = "simple-fpga-bus";
> +				reg = <0xc0000000 0x20000000>,
> +				      <0xff200000 0x00200000>;
> +				reg-names = "axi_h2f", "axi_h2f_lw";
> +
> +				#address-cells = <0x2>;
> +				#size-cells = <0x1>;
> +
> +				ranges = <0x00000000 0x00000000 0xc0000000 0x00010000>,
> +					 <0x00000001 0x00020000 0xff220000 0x00000008>,
> +					 <0x00000001 0x00010040 0xff210040 0x00000020>;
> +
> +				clocks = <0x2 0x2>;
> +				clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
> +
> +				fpga-mgr = <&hps_0_fpgamgr>;
> +				fpga-firmware = "soc_system.rbf";
> +
> +				fpga-bridges = <&hps_fpgabridge0>, <&hps_fpgabridge1>, <&hps_fpgabridge2>;

At least for Socfpga the HPS2FPGA and FPGA2HPS bridges can be configured to have
a bus width of 32, 64, 128 depending on the bitstream. How does this fit into here?
For DMA over this bus this is an interesting property to know, isn't it?
I guess something like "<&hps_fpgabridge0 64>" could be a possible solution, but
what is with the GPV registers of the bridges? Do they depend on the bitstream
loaded? Then this would get really messy I guess as it not a generic property.

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

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[atull]

On Wed, 28 Oct 2015, Steffen Trumtrar wrote:

> On Tue, Oct 27, 2015 at 05:09:11PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > New bindings document for simple fpga bus.
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > ---
> > 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.
> > ---
> >  .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> > new file mode 100644
> > index 0000000..2e742f7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> > @@ -0,0 +1,81 @@
> > +Simple FPGA Bus
> > +===============
> > +
> > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> > +before populating the devices below its node.  All this happens when a device
> > +tree overlay is added to the live tree.  This document describes that device
> > +tree overlay.
> > +
> > +Required properties:
> > +- compatible : should contain "simple-fpga-bus"
> > +- #address-cells, #size-cells, ranges: must be present to handle address space
> > +  mapping for children.
> > +
> > +Optional properties:
> > +- fpga-mgr : should contain a phandle to a FPGA manager.
> > +- fpga-firmware : should contain the name of a FPGA image file located on the
> > +  firmware search path.
> > +- partial-reconfig : boolean property should be defined if partial
> > +  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
> > +  is done.
> > +- fpga-bridges : should contain a list of bridges that the bus will disable
> > +  before   programming the FPGA and then enable after the FPGA has been
> > +
> > +Example:
> > +
> > +/dts-v1/;
> > +/plugin/;
> > +/ {
> > +	fragment@0 {
> > +		target-path="/soc";
> > +		__overlay__ {
> > +			#address-cells = <1>;
> > +	                #size-cells = <1>;
> > +
> > +			bridge@0xff200000 {
> > +				compatible = "simple-fpga-bus";
> > +				reg = <0xc0000000 0x20000000>,
> > +				      <0xff200000 0x00200000>;
> > +				reg-names = "axi_h2f", "axi_h2f_lw";
> > +
> > +				#address-cells = <0x2>;
> > +				#size-cells = <0x1>;
> > +
> > +				ranges = <0x00000000 0x00000000 0xc0000000 0x00010000>,
> > +					 <0x00000001 0x00020000 0xff220000 0x00000008>,
> > +					 <0x00000001 0x00010040 0xff210040 0x00000020>;
> > +
> > +				clocks = <0x2 0x2>;
> > +				clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
> > +
> > +				fpga-mgr = <&hps_0_fpgamgr>;
> > +				fpga-firmware = "soc_system.rbf";
> > +
> > +				fpga-bridges = <&hps_fpgabridge0>, <&hps_fpgabridge1>, <&hps_fpgabridge2>;
> 
> At least for Socfpga the HPS2FPGA and FPGA2HPS bridges can be configured to have
> a bus width of 32, 64, 128 depending on the bitstream. How does this fit into here?
> For DMA over this bus this is an interesting property to know, isn't it?

It's interesting if you want to change bridge width.

Most of the time I think people are not going to want or even be able to change
the width of their bridges dynamically.

> I guess something like "<&hps_fpgabridge0 64>" could be a possible solution,

Yes that does seem possible and easy to implement on the level of the simple
fpga bus and fpga bridge framework.  Each low level FPGA Bridge driver would
have #bridge-cells = <1> to support having a parameter.  Specifying
#bridge-cells = <0> would bring back to the current behaviour of no parameter.
A parameter could be added to struct fpga_bridge so that when the simple fpga
bus enables the bridge, it could pass the parameter back to the fpga bridge
driver.  All without changing any parameters of the functions.  Thanks for the
suggestion!

> but
> what is with the GPV registers of the bridges? Do they depend on the bitstream
> loaded? Then this would get really messy I guess as it not a generic property.

Not all low level drivers will be able to support this.  Those that can't support
this won't have to.  But it is a possibility without adding much code to the bridge
framework.

Alan

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

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[Rob Herring]

On Tue, Oct 27, 2015 at 5:09 PM,  <atull@opensource.altera.com> wrote:
> From: Alan Tull <atull@opensource.altera.com>
>
> New bindings document for simple fpga bus.
>
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
> 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.
> ---
>  .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
>
> diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> new file mode 100644
> index 0000000..2e742f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> @@ -0,0 +1,81 @@
> +Simple FPGA Bus
> +===============
> +
> +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> +before populating the devices below its node.  All this happens when a device
> +tree overlay is added to the live tree.  This document describes that device
> +tree overlay.
> +
> +Required properties:
> +- compatible : should contain "simple-fpga-bus"
> +- #address-cells, #size-cells, ranges: must be present to handle address space
> +  mapping for children.
> +
> +Optional properties:
> +- fpga-mgr : should contain a phandle to a FPGA manager.
> +- fpga-firmware : should contain the name of a FPGA image file located on the
> +  firmware search path.

Putting firmware filename in DT has come up in other cases recently[1]
and we concluded it should not be in the DT. Maybe the conclusion
would be different here, and if so we should have a common property
here.

> +- partial-reconfig : boolean property should be defined if partial
> +  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
> +  is done.
> +- fpga-bridges : should contain a list of bridges that the bus will disable
> +  before   programming the FPGA and then enable after the FPGA has been
> +
> +Example:
> +
> +/dts-v1/;
> +/plugin/;
> +/ {
> +       fragment@0 {
> +               target-path="/soc";
> +               __overlay__ {
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +
> +                       bridge@0xff200000 {
> +                               compatible = "simple-fpga-bus";
> +                               reg = <0xc0000000 0x20000000>,
> +                                     <0xff200000 0x00200000>;

You have registers for the bus, so therefore it is not simple. I think
the bus or bridge needs a specific compatible

Rob

[1] http://www.spinics.net/lists/devicetree/msg92462.html

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[atull]

On Wed, 28 Oct 2015, Rob Herring wrote:

> On Tue, Oct 27, 2015 at 5:09 PM,  <atull@opensource.altera.com> wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> >
> > New bindings document for simple fpga bus.
> >
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > ---
> > 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.
> > ---
> >  .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> >
> > diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> > new file mode 100644
> > index 0000000..2e742f7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> > @@ -0,0 +1,81 @@
> > +Simple FPGA Bus
> > +===============
> > +
> > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> > +before populating the devices below its node.  All this happens when a device
> > +tree overlay is added to the live tree.  This document describes that device
> > +tree overlay.
> > +
> > +Required properties:
> > +- compatible : should contain "simple-fpga-bus"
> > +- #address-cells, #size-cells, ranges: must be present to handle address space
> > +  mapping for children.
> > +
> > +Optional properties:
> > +- fpga-mgr : should contain a phandle to a FPGA manager.
> > +- fpga-firmware : should contain the name of a FPGA image file located on the
> > +  firmware search path.
> 
> Putting firmware filename in DT has come up in other cases recently[1]
> and we concluded it should not be in the DT. Maybe the conclusion
> would be different here, and if so we should have a common property
> here.

Interesting discussion.

One FPGA image will almost always have multiple hardware devices in it.
The device blocks will be reused in various combinations in different
FPGA images.  So hardwiring the image name in some specific driver won't
be possible.  Also many of the devices that could appear on a FPGA also
could appear in normal hardware.  Same driver for both cases, just
different address.

I won't have control of the name of the firmware file.  It will be something
that makes sense to a FPGA hardware guy so translating the node name to an
image name would be brittle.

Renaming this as a generic property "firmware-name" or "firmware" would be
an improvement on what I proposed.

If it is acceptible to have this in the DT, it means FPGA hardware development
workflow does not require a kernel rebuild.  The DT can collect together the
image name, the bridges to the FPGA (or to that area of the FPGA), and a list
of the devices/drivers in that image.

> 
> > +- partial-reconfig : boolean property should be defined if partial
> > +  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
> > +  is done.
> > +- fpga-bridges : should contain a list of bridges that the bus will disable
> > +  before   programming the FPGA and then enable after the FPGA has been
> > +
> > +Example:
> > +
> > +/dts-v1/;
> > +/plugin/;
> > +/ {
> > +       fragment@0 {
> > +               target-path="/soc";
> > +               __overlay__ {
> > +                       #address-cells = <1>;
> > +                       #size-cells = <1>;
> > +
> > +                       bridge@0xff200000 {
> > +                               compatible = "simple-fpga-bus";
> > +                               reg = <0xc0000000 0x20000000>,
> > +                                     <0xff200000 0x00200000>;
> 
> You have registers for the bus, so therefore it is not simple. I think
> the bus or bridge needs a specific compatible
> 

The reg here is cruft from device tree generation.  I don't use it and will
clean it out.  After I've done that, does that become simple again?

What I need in a bus is:
 - Handles 'ranges'
 - Controls enabling/disabling bridges and programs the FPGA
 - Populates the child devices (and there will probably be many)

This raises another issue: each area of the FPGA is likely to have multiple
bridges. That's why I had a list of phandles to bridges rather than
different bus for each type of bridge.  Is that acceptible-ish?

> Rob
> 
> [1] http://www.spinics.net/lists/devicetree/msg92462.html
> 

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[Rob Herring]

On Thu, Oct 29, 2015 at 11:02 AM, atull <atull@opensource.altera.com> wrote:
> On Wed, 28 Oct 2015, Rob Herring wrote:
>
>> On Tue, Oct 27, 2015 at 5:09 PM,  <atull@opensource.altera.com> wrote:
>> > From: Alan Tull <atull@opensource.altera.com>
>> >
>> > New bindings document for simple fpga bus.
>> >
>> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
>> > ---
>> > 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.
>> > ---
>> >  .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
>> >  1 file changed, 81 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
>> > new file mode 100644
>> > index 0000000..2e742f7
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
>> > @@ -0,0 +1,81 @@
>> > +Simple FPGA Bus
>> > +===============
>> > +
>> > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
>> > +before populating the devices below its node.  All this happens when a device
>> > +tree overlay is added to the live tree.  This document describes that device
>> > +tree overlay.
>> > +
>> > +Required properties:
>> > +- compatible : should contain "simple-fpga-bus"
>> > +- #address-cells, #size-cells, ranges: must be present to handle address space
>> > +  mapping for children.
>> > +
>> > +Optional properties:
>> > +- fpga-mgr : should contain a phandle to a FPGA manager.
>> > +- fpga-firmware : should contain the name of a FPGA image file located on the
>> > +  firmware search path.
>>
>> Putting firmware filename in DT has come up in other cases recently[1]
>> and we concluded it should not be in the DT. Maybe the conclusion
>> would be different here, and if so we should have a common property
>> here.
>
> Interesting discussion.
>
> One FPGA image will almost always have multiple hardware devices in it.
> The device blocks will be reused in various combinations in different
> FPGA images.  So hardwiring the image name in some specific driver won't
> be possible.  Also many of the devices that could appear on a FPGA also
> could appear in normal hardware.  Same driver for both cases, just
> different address.
>
> I won't have control of the name of the firmware file.  It will be something
> that makes sense to a FPGA hardware guy so translating the node name to an
> image name would be brittle.
>
> Renaming this as a generic property "firmware-name" or "firmware" would be
> an improvement on what I proposed.
>
> If it is acceptible to have this in the DT, it means FPGA hardware development
> workflow does not require a kernel rebuild.  The DT can collect together the
> image name, the bridges to the FPGA (or to that area of the FPGA), and a list
> of the devices/drivers in that image.

You can deal with this purely in userspace. The firmware userspace
helper can load any file you like. If the name is frequently changing,
then I agree it should not be in the kernel. But neither should it be
in the base DT. However, this would be in the overlay DT? In that use,
I think having the firmware name in DT is fine. The other option is
just put the firmware itself into the DT overlay (u-boot FIT images
are actually DTs with binary blobs). Either way please just create and
use a generic binding here.

>> > +- partial-reconfig : boolean property should be defined if partial
>> > +  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
>> > +  is done.
>> > +- fpga-bridges : should contain a list of bridges that the bus will disable
>> > +  before   programming the FPGA and then enable after the FPGA has been
>> > +
>> > +Example:
>> > +
>> > +/dts-v1/;
>> > +/plugin/;
>> > +/ {
>> > +       fragment@0 {
>> > +               target-path="/soc";
>> > +               __overlay__ {
>> > +                       #address-cells = <1>;
>> > +                       #size-cells = <1>;
>> > +
>> > +                       bridge@0xff200000 {
>> > +                               compatible = "simple-fpga-bus";
>> > +                               reg = <0xc0000000 0x20000000>,
>> > +                                     <0xff200000 0x00200000>;
>>
>> You have registers for the bus, so therefore it is not simple. I think
>> the bus or bridge needs a specific compatible
>>
>
> The reg here is cruft from device tree generation.  I don't use it and will
> clean it out.  After I've done that, does that become simple again?
>
> What I need in a bus is:
>  - Handles 'ranges'
>  - Controls enabling/disabling bridges and programs the FPGA

Where are these controls?

>  - Populates the child devices (and there will probably be many)

simple-bus handles at least the 1st and 3rd item. I suppose you don't
want the bus to probe before the bridge driver. If you want the bridge
driver to control that, you don't actually need a bus name.
of_platform_populate() will create all child devices. It is only if
you want to create the grandchildren that you need a bus match on the
child nodes.


> This raises another issue: each area of the FPGA is likely to have multiple
> bridges. That's why I had a list of phandles to bridges rather than
> different bus for each type of bridge.  Is that acceptible-ish?

Hard to say. A bridge tends to mean a parent bus to child bus
translator and the DT hierarchy generally follows the bus hierarchy,
so really it is better if DT follows the physical bus structure. Of
course, we really only do that with outbound bus and not the inbound
side.

I also worry that all this looks like it may be somewhat Altera specific.

Rob

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[atull]

On Fri, 30 Oct 2015, Rob Herring wrote:

> On Thu, Oct 29, 2015 at 11:02 AM, atull <atull@opensource.altera.com> wrote:
> > On Wed, 28 Oct 2015, Rob Herring wrote:
> >
> >> On Tue, Oct 27, 2015 at 5:09 PM,  <atull@opensource.altera.com> wrote:
> >> > From: Alan Tull <atull@opensource.altera.com>
> >> >
> >> > New bindings document for simple fpga bus.
> >> >
> >> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> >> > ---
> >> > 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.
> >> > ---
> >> >  .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
> >> >  1 file changed, 81 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> >> > new file mode 100644
> >> > index 0000000..2e742f7
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> >> > @@ -0,0 +1,81 @@
> >> > +Simple FPGA Bus
> >> > +===============
> >> > +
> >> > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> >> > +before populating the devices below its node.  All this happens when a device
> >> > +tree overlay is added to the live tree.  This document describes that device
> >> > +tree overlay.
> >> > +
> >> > +Required properties:
> >> > +- compatible : should contain "simple-fpga-bus"
> >> > +- #address-cells, #size-cells, ranges: must be present to handle address space
> >> > +  mapping for children.
> >> > +
> >> > +Optional properties:
> >> > +- fpga-mgr : should contain a phandle to a FPGA manager.
> >> > +- fpga-firmware : should contain the name of a FPGA image file located on the
> >> > +  firmware search path.
> >>
> >> Putting firmware filename in DT has come up in other cases recently[1]
> >> and we concluded it should not be in the DT. Maybe the conclusion
> >> would be different here, and if so we should have a common property
> >> here.
> >
> > Interesting discussion.
> >
> > One FPGA image will almost always have multiple hardware devices in it.
> > The device blocks will be reused in various combinations in different
> > FPGA images.  So hardwiring the image name in some specific driver won't
> > be possible.  Also many of the devices that could appear on a FPGA also
> > could appear in normal hardware.  Same driver for both cases, just
> > different address.
> >
> > I won't have control of the name of the firmware file.  It will be something
> > that makes sense to a FPGA hardware guy so translating the node name to an
> > image name would be brittle.
> >
> > Renaming this as a generic property "firmware-name" or "firmware" would be
> > an improvement on what I proposed.
> >
> > If it is acceptible to have this in the DT, it means FPGA hardware development
> > workflow does not require a kernel rebuild.  The DT can collect together the
> > image name, the bridges to the FPGA (or to that area of the FPGA), and a list
> > of the devices/drivers in that image.
> 
> You can deal with this purely in userspace.

We have.  It's ugly.  That means we have to also deal with bridges from
userspace since they have to be disable during FPGA programming.  And the
drivers have to be modules that we modprobe after FPGA programming.

> The firmware userspace
> helper can load any file you like. If the name is frequently changing,
> then I agree it should not be in the kernel. But neither should it be
> in the base DT. However, this would be in the overlay DT? In that use,
> I think having the firmware name in DT is fine. The other option is
> just put the firmware itself into the DT overlay (u-boot FIT images
> are actually DTs with binary blobs). Either way please just create and
> use a generic binding here.

Planning to use overlays.  I'll use "firmware-name."

> 
> >> > +- partial-reconfig : boolean property should be defined if partial
> >> > +  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
> >> > +  is done.
> >> > +- fpga-bridges : should contain a list of bridges that the bus will disable
> >> > +  before   programming the FPGA and then enable after the FPGA has been
> >> > +
> >> > +Example:
> >> > +
> >> > +/dts-v1/;
> >> > +/plugin/;
> >> > +/ {
> >> > +       fragment@0 {
> >> > +               target-path="/soc";
> >> > +               __overlay__ {
> >> > +                       #address-cells = <1>;
> >> > +                       #size-cells = <1>;
> >> > +
> >> > +                       bridge@0xff200000 {
> >> > +                               compatible = "simple-fpga-bus";
> >> > +                               reg = <0xc0000000 0x20000000>,
> >> > +                                     <0xff200000 0x00200000>;
> >>
> >> You have registers for the bus, so therefore it is not simple. I think
> >> the bus or bridge needs a specific compatible
> >>
> >
> > The reg here is cruft from device tree generation.  I don't use it and will
> > clean it out.  After I've done that, does that become simple again?
> >
> > What I need in a bus is:
> >  - Handles 'ranges'
> >  - Controls enabling/disabling bridges and programs the FPGA
> 
> Where are these controls?

The Simple FPGA Bus calls FPGA Bridge Framework functions to
enable/disable the bridges.

> 
> >  - Populates the child devices (and there will probably be many)
> 
> simple-bus handles at least the 1st and 3rd item. I suppose you don't
> want the bus to probe before the bridge driver. If you want the bridge
> driver to control that, you don't actually need a bus name.
> of_platform_populate() will create all child devices. It is only if
> you want to create the grandchildren that you need a bus match on the
> child nodes.
> 
> 
> > This raises another issue: each area of the FPGA is likely to have multiple
> > bridges. That's why I had a list of phandles to bridges rather than
> > different bus for each type of bridge.  Is that acceptible-ish?
> 
> Hard to say. A bridge tends to mean a parent bus to child bus
> translator and the DT hierarchy generally follows the bus hierarchy,
> so really it is better if DT follows the physical bus structure. Of
> course, we really only do that with outbound bus and not the inbound
> side.
> 
> I also worry that all this looks like it may be somewhat Altera specific.

I don't think it is Altera specific.  If you want to reprogram an
FPGA with devices that need drivers, you will likely need to:
 1. Disable some bridges to prevent spurious stuff on the bus
 2. Load the FPGA
 3. Enable the bridges
 4. Probe some drivers

In the case where the whole FPGA gets rewritten, it is possible to
combine the bridge control with the FPGA manager (then it *is* manufacturer
specific).  In the case where part of the FPGA gets rewritten (partial
reconfiguration), it is likely that some sort of bridge will exist on
the FPGA to protect the bus.  And there will be that kind of bridge for
each different area of the FPGA that needs to get rewritten.

The device tree overlay seems a very natural way of keeping all that
information together in one place.  Otherwise we have to invent
something in userspace.

Alan

> 
> Rob
> 

Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[Rob Herring]

On Tue, Nov 3, 2015 at 10:28 AM, atull <atull@opensource.altera.com> wrote:
> On Fri, 30 Oct 2015, Rob Herring wrote:
>
>> On Thu, Oct 29, 2015 at 11:02 AM, atull <atull@opensource.altera.com> wrote:
>> > On Wed, 28 Oct 2015, Rob Herring wrote:
>> >
>> >> On Tue, Oct 27, 2015 at 5:09 PM,  <atull@opensource.altera.com> wrote:
>> >> > From: Alan Tull <atull@opensource.altera.com>
>> >> >
>> >> > New bindings document for simple fpga bus.
>> >> >
>> >> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
>> >> > ---
>> >> > 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.
>> >> > ---
>> >> >  .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
>> >> >  1 file changed, 81 insertions(+)
>> >> >  create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
>> >> > new file mode 100644
>> >> > index 0000000..2e742f7
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
>> >> > @@ -0,0 +1,81 @@
>> >> > +Simple FPGA Bus
>> >> > +===============
>> >> > +
>> >> > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
>> >> > +before populating the devices below its node.  All this happens when a device
>> >> > +tree overlay is added to the live tree.  This document describes that device
>> >> > +tree overlay.
>> >> > +
>> >> > +Required properties:
>> >> > +- compatible : should contain "simple-fpga-bus"
>> >> > +- #address-cells, #size-cells, ranges: must be present to handle address space
>> >> > +  mapping for children.
>> >> > +
>> >> > +Optional properties:
>> >> > +- fpga-mgr : should contain a phandle to a FPGA manager.
>> >> > +- fpga-firmware : should contain the name of a FPGA image file located on the
>> >> > +  firmware search path.
>> >>
>> >> Putting firmware filename in DT has come up in other cases recently[1]
>> >> and we concluded it should not be in the DT. Maybe the conclusion
>> >> would be different here, and if so we should have a common property
>> >> here.
>> >
>> > Interesting discussion.
>> >
>> > One FPGA image will almost always have multiple hardware devices in it.
>> > The device blocks will be reused in various combinations in different
>> > FPGA images.  So hardwiring the image name in some specific driver won't
>> > be possible.  Also many of the devices that could appear on a FPGA also
>> > could appear in normal hardware.  Same driver for both cases, just
>> > different address.
>> >
>> > I won't have control of the name of the firmware file.  It will be something
>> > that makes sense to a FPGA hardware guy so translating the node name to an
>> > image name would be brittle.
>> >
>> > Renaming this as a generic property "firmware-name" or "firmware" would be
>> > an improvement on what I proposed.
>> >
>> > If it is acceptible to have this in the DT, it means FPGA hardware development
>> > workflow does not require a kernel rebuild.  The DT can collect together the
>> > image name, the bridges to the FPGA (or to that area of the FPGA), and a list
>> > of the devices/drivers in that image.
>>
>> You can deal with this purely in userspace.
>
> We have.  It's ugly.  That means we have to also deal with bridges from
> userspace since they have to be disable during FPGA programming.  And the
> drivers have to be modules that we modprobe after FPGA programming.

I didn't mean everything. From the FPGA mgr perspective, it just calls
request_firmware. From there, whether the kernel does it or a
userspace script does it should be transparent to the rest of the
flow. I fully agree the flow should be controlled from within the
kernel.

>> The firmware userspace
>> helper can load any file you like. If the name is frequently changing,
>> then I agree it should not be in the kernel. But neither should it be
>> in the base DT. However, this would be in the overlay DT? In that use,
>> I think having the firmware name in DT is fine. The other option is
>> just put the firmware itself into the DT overlay (u-boot FIT images
>> are actually DTs with binary blobs). Either way please just create and
>> use a generic binding here.
>
> Planning to use overlays.  I'll use "firmware-name."

Okay, but in v13 you didn't...

>
>>
>> >> > +- partial-reconfig : boolean property should be defined if partial
>> >> > +  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
>> >> > +  is done.
>> >> > +- fpga-bridges : should contain a list of bridges that the bus will disable
>> >> > +  before   programming the FPGA and then enable after the FPGA has been
>> >> > +
>> >> > +Example:
>> >> > +
>> >> > +/dts-v1/;
>> >> > +/plugin/;
>> >> > +/ {
>> >> > +       fragment@0 {
>> >> > +               target-path="/soc";
>> >> > +               __overlay__ {
>> >> > +                       #address-cells = <1>;
>> >> > +                       #size-cells = <1>;
>> >> > +
>> >> > +                       bridge@0xff200000 {
>> >> > +                               compatible = "simple-fpga-bus";
>> >> > +                               reg = <0xc0000000 0x20000000>,
>> >> > +                                     <0xff200000 0x00200000>;
>> >>
>> >> You have registers for the bus, so therefore it is not simple. I think
>> >> the bus or bridge needs a specific compatible
>> >>
>> >
>> > The reg here is cruft from device tree generation.  I don't use it and will
>> > clean it out.  After I've done that, does that become simple again?
>> >
>> > What I need in a bus is:
>> >  - Handles 'ranges'
>> >  - Controls enabling/disabling bridges and programs the FPGA
>>
>> Where are these controls?
>
> The Simple FPGA Bus calls FPGA Bridge Framework functions to
> enable/disable the bridges.

Can you describe this in terms of the h/w? I'd expect the s/w to be
the other way around. The bridge f/w instantiates the bus.

What I'm failing to understand is how the bridges and buses you are
defining relate to each other. I understand that a bridge controls the
bus behind it, but that relationship is not evident in this series.

>
>>
>> >  - Populates the child devices (and there will probably be many)
>>
>> simple-bus handles at least the 1st and 3rd item. I suppose you don't
>> want the bus to probe before the bridge driver. If you want the bridge
>> driver to control that, you don't actually need a bus name.
>> of_platform_populate() will create all child devices. It is only if
>> you want to create the grandchildren that you need a bus match on the
>> child nodes.
>>
>>
>> > This raises another issue: each area of the FPGA is likely to have multiple
>> > bridges. That's why I had a list of phandles to bridges rather than
>> > different bus for each type of bridge.  Is that acceptible-ish?
>>
>> Hard to say. A bridge tends to mean a parent bus to child bus
>> translator and the DT hierarchy generally follows the bus hierarchy,
>> so really it is better if DT follows the physical bus structure. Of
>> course, we really only do that with outbound bus and not the inbound
>> side.
>>
>> I also worry that all this looks like it may be somewhat Altera specific.
>
> I don't think it is Altera specific.  If you want to reprogram an
> FPGA with devices that need drivers, you will likely need to:
>  1. Disable some bridges to prevent spurious stuff on the bus
>  2. Load the FPGA
>  3. Enable the bridges
>  4. Probe some drivers
>
> In the case where the whole FPGA gets rewritten, it is possible to
> combine the bridge control with the FPGA manager (then it *is* manufacturer
> specific).  In the case where part of the FPGA gets rewritten (partial
> reconfiguration), it is likely that some sort of bridge will exist on
> the FPGA to protect the bus.  And there will be that kind of bridge for
> each different area of the FPGA that needs to get rewritten.
>
> The device tree overlay seems a very natural way of keeping all that
> information together in one place.  Otherwise we have to invent
> something in userspace.

I'm not saying it doesn't belong in DT. I just want to hear that this
all makes sense from someone else that is !Altera.

Rob

[PATCH v12 3/6] fpga: add simple-fpga-bus

[atull]

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

The Simple FPGA bus uses the FPGA Manager Framework and the
FPGA Bridge Framework to provide a manufactorer-agnostic
interface for reprogramming FPGAs that is Device Tree
Overlays-based.

When a Device Tree Overlay containing a Simple FPGA Bus is
applied, the Simple FPGA Bus will be probed and will:
 * Disable the FPGA bridges specified in the overlay
 * Reprogram a specified FPGA with a specified image file
 * Enable the specified FPGA bridges
 * Populate the child devices

Removing the Device Tree Overlay is also supported.

This supports fpga use where hardware blocks on a fpga will
need drivers.

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.
---
 drivers/fpga/Kconfig           |    7 +
 drivers/fpga/Makefile          |    3 +
 drivers/fpga/simple-fpga-bus.c |  327 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 337 insertions(+)
 create mode 100644 drivers/fpga/simple-fpga-bus.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 348e1dea..e8f5453 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -13,6 +13,13 @@ config FPGA
 
 if FPGA
 
+config SIMPLE_FPGA_BUS
+       bool "Simple FPGA Bus"
+       depends on OF
+       help
+         Simple FPGA Bus allows loading FPGA images under control of
+	 Device Tree.
+
 config FPGA_MGR_SOCFPGA
 	tristate "Altera SOCFPGA FPGA Manager"
 	depends on ARCH_SOCFPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index bf0c0d2..0385622 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_SOCFPGA_A10)	+= socfpga-a10.o
+
+# High Level Interfaces
+obj-$(CONFIG_SIMPLE_FPGA_BUS)		+= simple-fpga-bus.o
diff --git a/drivers/fpga/simple-fpga-bus.c b/drivers/fpga/simple-fpga-bus.c
new file mode 100644
index 0000000..6318cf1
--- /dev/null
+++ b/drivers/fpga/simple-fpga-bus.c
@@ -0,0 +1,327 @@
+/*
+ * Simple FPGA Bus
+ *
+ *  Copyright (C) 2013-2015 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/fpga/fpga-bridge.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+/**
+ * struct simple_fpga_bus - simple fpga bus private data
+ * @dev:	device from pdev
+ * @bridges:	FPGA bridges associated with this bus
+ * @num_bridges: size of the bridges array
+ */
+struct simple_fpga_bus {
+	struct device *dev;
+	struct fpga_bridge **bridges;
+	int num_bridges;
+};
+
+/**
+ * simple_fpga_bus_get_mgr - get associated fpga manager
+ * @priv: simple fpga bus private data
+ * pointer to fpga manager in priv->mgr on success
+ *
+ * Given a simple fpga bus, get a reference to its the fpga manager specified
+ * by its "fpga-mgr" device tree property.
+ *
+ * Return: 0 if the fpga manager is not specified.
+ *         pointer to struct fpga_manager if successful.
+ *         Negative error code otherwise.
+ */
+static struct fpga_manager *simple_fpga_bus_get_mgr(
+					struct simple_fpga_bus *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *mgr_node;
+
+	/*
+	 * Return 0 (not an error) if fpga manager is not specified.
+	 * This supports the case where fpga was already configured.
+	 */
+	mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
+	if (!mgr_node) {
+		dev_dbg(dev, "could not find fpga-mgr DT property\n");
+		return 0;
+	}
+
+	return of_fpga_mgr_get(mgr_node);
+}
+
+/**
+ * simple_fpga_bus_load_image - load an fpga image under device tree control
+ * @priv: simple fpga bus private data
+ *
+ * Given a simple fpga bus, load the fpga image specified in its device
+ * tree node.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int simple_fpga_bus_load_image(struct simple_fpga_bus *priv,
+				      struct fpga_manager *mgr)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	u32 flags = 0;
+	const char *path;
+
+	if (of_property_read_bool(np, "partial-reconfig"))
+		flags |= FPGA_MGR_PARTIAL_RECONFIG;
+
+	/* If firmware image is specified in the DT, load it */
+	if (!of_property_read_string(np, "fpga-firmware", &path))
+		return fpga_mgr_firmware_load(mgr, flags, path);
+
+	/*
+	 * Here we can add other methods of getting ahold of a fpga image
+	 * specified in the device tree and programming it.
+	 */
+
+	dev_dbg(dev, "No FPGA image to load.\n");
+
+	/* Status is that we have a fpga manager but no image specified. */
+	return -EINVAL;
+}
+
+/**
+ * simple_fpga_bus_bridge_enable - enable the fpga bridges
+ * @priv: simple fpga bus private data
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int simple_fpga_bus_bridge_enable(struct simple_fpga_bus *priv)
+{
+	int i, ret;
+
+	for (i = 0; i < priv->num_bridges; i++) {
+		ret = fpga_bridge_enable(priv->bridges[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * simple_fpga_bus_bridge_disable - disable the bridges
+ * @priv: simple fpga bus private data
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int simple_fpga_bus_bridge_disable(struct simple_fpga_bus *priv)
+{
+	int i, ret;
+
+	for (i = 0; i < priv->num_bridges; i++) {
+		ret = fpga_bridge_disable(priv->bridges[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * simple_fpga_bus_get_bridges - get references for fpga bridges
+ * @priv: simple fpga bus private data
+ *
+ * Given a simple fpga bus, get references for its associated fpga bridges.
+ * These are pointed to by "fpga-bridges" device tree property.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int simple_fpga_bus_get_bridges(struct simple_fpga_bus *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	struct of_phandle_args out_args;
+	struct fpga_bridge *bridge, **bridges;
+	int i, num_bridges, ret;
+
+	num_bridges = of_count_phandle_with_args(np, "fpga-bridges", NULL);
+	if (!num_bridges) {
+		dev_info(dev, "No fpga bridges found\n");
+		return -EINVAL;
+	}
+
+	bridges = kcalloc(num_bridges, sizeof(*bridge), GFP_KERNEL);
+	if (!bridges)
+		return -ENOMEM;
+
+	for (i = 0; i < num_bridges; i++) {
+		ret = of_parse_phandle_with_args(np, "fpga-bridges", NULL, i,
+						 &out_args);
+		if (ret)
+			goto err_free_bridges;
+
+		bridge = of_fpga_bridge_get(out_args.np);
+		if (!bridge) {
+			dev_err(dev, "No fpga bridge found for phandle\n");
+			goto err_free_bridges;
+		}
+
+		dev_info(dev, "Found bridge: %s\n", dev_name(&bridge->dev));
+
+		bridges[i] = bridge;
+	}
+
+	priv->bridges = bridges;
+	priv->num_bridges = num_bridges;
+
+	return 0;
+
+err_free_bridges:
+	while (i)
+		fpga_bridge_put(bridges[--i]);
+
+	kfree(bridges);
+	return -EINVAL;
+}
+
+/**
+ * simple_fpga_bus_put_bridges - release references for the fpga bridges
+ * @priv: simple fpga bus private data
+ */
+static void simple_fpga_bus_put_bridges(struct simple_fpga_bus *priv)
+{
+	int i;
+
+	for (i = 0; i < priv->num_bridges; i++)
+		fpga_bridge_put(priv->bridges[i]);
+
+	kfree(priv->bridges);
+	priv->num_bridges = 0;
+}
+
+/**
+ * simple_fpga_bus_probe - Probe function for simple fpga bus.
+ * @pdev: platform device
+ *
+ * Do the necessary steps to program the FPGA and enable associated bridges.
+ * Then populate the device tree below this bus to get drivers probed for the
+ * hardware that is on the FPGA.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static int simple_fpga_bus_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct simple_fpga_bus *priv;
+	struct fpga_manager *mgr;
+	int ret;
+
+	pr_err("%s line %d\n", __func__, __LINE__);
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	/*
+	 * If simple_fpga_bus_get_mgr returns a negative error, the fpga
+	 * manager may not have probed yet.
+	 */
+	mgr = simple_fpga_bus_get_mgr(priv);
+	if (IS_ERR(mgr))
+		return -EPROBE_DEFER;
+
+	if (mgr) {
+		ret = simple_fpga_bus_get_bridges(priv);
+		if (ret) {
+			ret = -EPROBE_DEFER;
+			goto err_release_mgr;
+		}
+
+		ret = simple_fpga_bus_bridge_disable(priv);
+		if (ret)
+			goto err_release_br;
+
+		ret = simple_fpga_bus_load_image(priv, mgr);
+		if (ret)
+			goto err_release_br;
+
+		ret = simple_fpga_bus_bridge_enable(priv);
+		if (ret)
+			goto err_release_br;
+
+		fpga_mgr_put(mgr);
+	}
+
+	of_platform_populate(np, of_default_bus_match_table, NULL, dev);
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+
+err_release_br:
+	simple_fpga_bus_put_bridges(priv);
+err_release_mgr:
+	fpga_mgr_put(mgr);
+
+	return ret;
+}
+
+static int simple_fpga_bus_remove(struct platform_device *pdev)
+{
+	struct simple_fpga_bus *priv = platform_get_drvdata(pdev);
+
+	simple_fpga_bus_bridge_disable(priv);
+	simple_fpga_bus_put_bridges(priv);
+
+	return 0;
+}
+
+static const struct of_device_id simple_fpga_bus_of_match[] = {
+	{ .compatible = "simple-fpga-bus", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, simple_fpga_bus_of_match);
+
+static struct platform_driver simple_fpga_bus_driver = {
+	.probe = simple_fpga_bus_probe,
+	.remove = simple_fpga_bus_remove,
+	.driver = {
+		.name	= "simple-fpga-bus",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(simple_fpga_bus_of_match),
+	},
+};
+
+static int __init simple_fpga_bus_init(void)
+{
+	return platform_driver_register(&simple_fpga_bus_driver);
+}
+
+static void __exit simple_fpga_bus_exit(void)
+{
+	platform_driver_unregister(&simple_fpga_bus_driver);
+}
+
+module_init(simple_fpga_bus_init);
+module_exit(simple_fpga_bus_exit);
+
+MODULE_DESCRIPTION("Simple FPGA Bus");
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Steffen Trumtrar]

On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> The Simple FPGA bus uses the FPGA Manager Framework and the
> FPGA Bridge Framework to provide a manufactorer-agnostic
> interface for reprogramming FPGAs that is Device Tree
> Overlays-based.
> 
> When a Device Tree Overlay containing a Simple FPGA Bus is
> applied, the Simple FPGA Bus will be probed and will:
>  * Disable the FPGA bridges specified in the overlay
>  * Reprogram a specified FPGA with a specified image file
>  * Enable the specified FPGA bridges
>  * Populate the child devices
> 
> Removing the Device Tree Overlay is also supported.
> 
> This supports fpga use where hardware blocks on a fpga will
> need drivers.
> 
> 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.
> ---
>  drivers/fpga/Kconfig           |    7 +
>  drivers/fpga/Makefile          |    3 +
>  drivers/fpga/simple-fpga-bus.c |  327 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 337 insertions(+)
>  create mode 100644 drivers/fpga/simple-fpga-bus.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 348e1dea..e8f5453 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,13 @@ config FPGA
>  
>  if FPGA
>  
> +config SIMPLE_FPGA_BUS
> +       bool "Simple FPGA Bus"
> +       depends on OF
> +       help
> +         Simple FPGA Bus allows loading FPGA images under control of
> +	 Device Tree.
> +
>  config FPGA_MGR_SOCFPGA
>  	tristate "Altera SOCFPGA FPGA Manager"
>  	depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index bf0c0d2..0385622 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_SOCFPGA_A10)	+= socfpga-a10.o
> +
> +# High Level Interfaces
> +obj-$(CONFIG_SIMPLE_FPGA_BUS)		+= simple-fpga-bus.o
> diff --git a/drivers/fpga/simple-fpga-bus.c b/drivers/fpga/simple-fpga-bus.c
> new file mode 100644
> index 0000000..6318cf1
> --- /dev/null
> +++ b/drivers/fpga/simple-fpga-bus.c
> @@ -0,0 +1,327 @@
> +/*
> + * Simple FPGA Bus
> + *
> + *  Copyright (C) 2013-2015 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/fpga/fpga-bridge.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +/**
> + * struct simple_fpga_bus - simple fpga bus private data
> + * @dev:	device from pdev
> + * @bridges:	FPGA bridges associated with this bus
> + * @num_bridges: size of the bridges array
> + */
> +struct simple_fpga_bus {
> +	struct device *dev;
> +	struct fpga_bridge **bridges;
> +	int num_bridges;
> +};
> +
> +/**
> + * simple_fpga_bus_get_mgr - get associated fpga manager
> + * @priv: simple fpga bus private data
> + * pointer to fpga manager in priv->mgr on success
> + *
> + * Given a simple fpga bus, get a reference to its the fpga manager specified
> + * by its "fpga-mgr" device tree property.
> + *
> + * Return: 0 if the fpga manager is not specified.
> + *         pointer to struct fpga_manager if successful.
> + *         Negative error code otherwise.
> + */
> +static struct fpga_manager *simple_fpga_bus_get_mgr(
> +					struct simple_fpga_bus *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *mgr_node;
> +
> +	/*
> +	 * Return 0 (not an error) if fpga manager is not specified.
> +	 * This supports the case where fpga was already configured.
> +	 */
> +	mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
> +	if (!mgr_node) {
> +		dev_dbg(dev, "could not find fpga-mgr DT property\n");
> +		return 0;
> +	}
> +
> +	return of_fpga_mgr_get(mgr_node);
> +}
> +
> +/**
> + * simple_fpga_bus_load_image - load an fpga image under device tree control
> + * @priv: simple fpga bus private data
> + *
> + * Given a simple fpga bus, load the fpga image specified in its device
> + * tree node.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int simple_fpga_bus_load_image(struct simple_fpga_bus *priv,
> +				      struct fpga_manager *mgr)
> +{
> +	struct device *dev = priv->dev;
> +	struct device_node *np = dev->of_node;
> +	u32 flags = 0;
> +	const char *path;
> +
> +	if (of_property_read_bool(np, "partial-reconfig"))
> +		flags |= FPGA_MGR_PARTIAL_RECONFIG;
> +
> +	/* If firmware image is specified in the DT, load it */
> +	if (!of_property_read_string(np, "fpga-firmware", &path))
> +		return fpga_mgr_firmware_load(mgr, flags, path);
> +
> +	/*
> +	 * Here we can add other methods of getting ahold of a fpga image
> +	 * specified in the device tree and programming it.
> +	 */
> +
> +	dev_dbg(dev, "No FPGA image to load.\n");
> +
> +	/* Status is that we have a fpga manager but no image specified. */
> +	return -EINVAL;
> +}
> +
> +/**
> + * simple_fpga_bus_bridge_enable - enable the fpga bridges
> + * @priv: simple fpga bus private data
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int simple_fpga_bus_bridge_enable(struct simple_fpga_bus *priv)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < priv->num_bridges; i++) {
> +		ret = fpga_bridge_enable(priv->bridges[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * simple_fpga_bus_bridge_disable - disable the bridges
> + * @priv: simple fpga bus private data
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int simple_fpga_bus_bridge_disable(struct simple_fpga_bus *priv)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < priv->num_bridges; i++) {
> +		ret = fpga_bridge_disable(priv->bridges[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * simple_fpga_bus_get_bridges - get references for fpga bridges
> + * @priv: simple fpga bus private data
> + *
> + * Given a simple fpga bus, get references for its associated fpga bridges.
> + * These are pointed to by "fpga-bridges" device tree property.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int simple_fpga_bus_get_bridges(struct simple_fpga_bus *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct device_node *np = dev->of_node;
> +	struct of_phandle_args out_args;
> +	struct fpga_bridge *bridge, **bridges;
> +	int i, num_bridges, ret;
> +
> +	num_bridges = of_count_phandle_with_args(np, "fpga-bridges", NULL);
> +	if (!num_bridges) {
> +		dev_info(dev, "No fpga bridges found\n");
> +		return -EINVAL;
> +	}
> +
> +	bridges = kcalloc(num_bridges, sizeof(*bridge), GFP_KERNEL);
> +	if (!bridges)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_bridges; i++) {
> +		ret = of_parse_phandle_with_args(np, "fpga-bridges", NULL, i,
> +						 &out_args);
> +		if (ret)
> +			goto err_free_bridges;
> +
> +		bridge = of_fpga_bridge_get(out_args.np);
> +		if (!bridge) {
> +			dev_err(dev, "No fpga bridge found for phandle\n");
> +			goto err_free_bridges;
> +		}
> +
> +		dev_info(dev, "Found bridge: %s\n", dev_name(&bridge->dev));
> +
> +		bridges[i] = bridge;
> +	}
> +
> +	priv->bridges = bridges;
> +	priv->num_bridges = num_bridges;
> +
> +	return 0;
> +
> +err_free_bridges:
> +	while (i)
> +		fpga_bridge_put(bridges[--i]);
> +
> +	kfree(bridges);
> +	return -EINVAL;
> +}
> +
> +/**
> + * simple_fpga_bus_put_bridges - release references for the fpga bridges
> + * @priv: simple fpga bus private data
> + */
> +static void simple_fpga_bus_put_bridges(struct simple_fpga_bus *priv)
> +{
> +	int i;
> +
> +	for (i = 0; i < priv->num_bridges; i++)
> +		fpga_bridge_put(priv->bridges[i]);
> +
> +	kfree(priv->bridges);
> +	priv->num_bridges = 0;
> +}
> +
> +/**
> + * simple_fpga_bus_probe - Probe function for simple fpga bus.
> + * @pdev: platform device
> + *
> + * Do the necessary steps to program the FPGA and enable associated bridges.
> + * Then populate the device tree below this bus to get drivers probed for the
> + * hardware that is on the FPGA.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int simple_fpga_bus_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct simple_fpga_bus *priv;
> +	struct fpga_manager *mgr;
> +	int ret;
> +
> +	pr_err("%s line %d\n", __func__, __LINE__);

Debugging residue, I guess, please remove.

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +
> +	/*
> +	 * If simple_fpga_bus_get_mgr returns a negative error, the fpga
> +	 * manager may not have probed yet.
> +	 */
> +	mgr = simple_fpga_bus_get_mgr(priv);
> +	if (IS_ERR(mgr))
> +		return -EPROBE_DEFER;
> +
> +	if (mgr) {
> +		ret = simple_fpga_bus_get_bridges(priv);
> +		if (ret) {
> +			ret = -EPROBE_DEFER;
> +			goto err_release_mgr;
> +		}
> +
> +		ret = simple_fpga_bus_bridge_disable(priv);
> +		if (ret)
> +			goto err_release_br;
> +
> +		ret = simple_fpga_bus_load_image(priv, mgr);
> +		if (ret)
> +			goto err_release_br;
> +
> +		ret = simple_fpga_bus_bridge_enable(priv);
> +		if (ret)
> +			goto err_release_br;
> +
> +		fpga_mgr_put(mgr);
> +	}
> +
> +	of_platform_populate(np, of_default_bus_match_table, NULL, dev);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +
> +err_release_br:
> +	simple_fpga_bus_put_bridges(priv);
> +err_release_mgr:
> +	fpga_mgr_put(mgr);
> +
> +	return ret;
> +}
> +
> +static int simple_fpga_bus_remove(struct platform_device *pdev)
> +{
> +	struct simple_fpga_bus *priv = platform_get_drvdata(pdev);
> +
> +	simple_fpga_bus_bridge_disable(priv);
> +	simple_fpga_bus_put_bridges(priv);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id simple_fpga_bus_of_match[] = {
> +	{ .compatible = "simple-fpga-bus", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, simple_fpga_bus_of_match);
> +
> +static struct platform_driver simple_fpga_bus_driver = {
> +	.probe = simple_fpga_bus_probe,
> +	.remove = simple_fpga_bus_remove,
> +	.driver = {
> +		.name	= "simple-fpga-bus",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(simple_fpga_bus_of_match),
> +	},
> +};
> +
> +static int __init simple_fpga_bus_init(void)
> +{
> +	return platform_driver_register(&simple_fpga_bus_driver);
> +}
> +
> +static void __exit simple_fpga_bus_exit(void)
> +{
> +	platform_driver_unregister(&simple_fpga_bus_driver);
> +}
> +
> +module_init(simple_fpga_bus_init);
> +module_exit(simple_fpga_bus_exit);
> +
> +MODULE_DESCRIPTION("Simple FPGA Bus");
> +MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> +MODULE_LICENSE("GPL v2");

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

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[atull]

On Wed, 28 Oct 2015, Steffen Trumtrar wrote:

> > +static int simple_fpga_bus_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct simple_fpga_bus *priv;
> > +	struct fpga_manager *mgr;
> > +	int ret;
> > +
> > +	pr_err("%s line %d\n", __func__, __LINE__);
> 
> Debugging residue, I guess, please remove.

OK

Thanks,

Alan

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> The Simple FPGA bus uses the FPGA Manager Framework and the
> FPGA Bridge Framework to provide a manufactorer-agnostic
> interface for reprogramming FPGAs that is Device Tree
> Overlays-based.

Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
all, from what I can tell.

Therefore, I would have expected the FPGA Bridge drivers to sit under
the fpga-socfpga driver, and not be a first class feature of the
kernels' FPGA manager subsystem.

  Josh

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[atull]

On Wed, 28 Oct 2015, Josh Cartwright wrote:

> On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > The Simple FPGA bus uses the FPGA Manager Framework and the
> > FPGA Bridge Framework to provide a manufactorer-agnostic
> > interface for reprogramming FPGAs that is Device Tree
> > Overlays-based.
> 
> Do you intend the "simple-fpga-bus" to be used on Zynq as well?

Yes

>  The
> whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> all, from what I can tell.

I want to make this useful for all of us.  I'll make some minor changes so
the simple fpga bus will not exit when it encounters an overlay that doesn't
specify any bridges.

Zynq doesn't have bridges to disable during reconfiguration?  I guess
that's handled automatically in hardware somehow?

> 
> Therefore, I would have expected the FPGA Bridge drivers to sit under
> the fpga-socfpga driver, and not be a first class feature of the
> kernels' FPGA manager subsystem.

I need some way for simple FPGA bus to be able to configure bridges during
the reconfiguration cycle.  That will have to hook into the simple fpga bus,
even if not everybody uses it.  I'll submit minor changes that make it
convenient to either have bridges if you need them or leave them out if
you don't.

Alan

> 
>   Josh
> 

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Moritz Fischer]

On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <joshc@ni.com> wrote:
> On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> The Simple FPGA bus uses the FPGA Manager Framework and the
>> FPGA Bridge Framework to provide a manufactorer-agnostic
>> interface for reprogramming FPGAs that is Device Tree
>> Overlays-based.
>
> Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> all, from what I can tell.

For Zynq the zynq-fpga driver takes care of the level shifters on full
reconfiguration,
and doesn't for partial reconfiguration. Now depending on which parts
of the fabric
are partial reconfigured (say AXI masters), one might run into issues
with a setup like that.

My first plan was to counter that by using zynq-reset to hold the
reset high during
reconfiguration of that part of the FPGA.

I'm happy to rethink that part and maybe redo the level shifters and
resets together in a bridge
driver under devicetree control gives finer grained control.

Cheers,

Moritz

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <joshc@ni.com> wrote:
> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
> >> From: Alan Tull <atull@opensource.altera.com>
> >>
> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> >> interface for reprogramming FPGAs that is Device Tree
> >> Overlays-based.
> >
> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> > all, from what I can tell.
> 
> For Zynq the zynq-fpga driver takes care of the level shifters on full
> reconfiguration,
> and doesn't for partial reconfiguration. Now depending on which parts
> of the fabric
> are partial reconfigured (say AXI masters), one might run into issues
> with a setup like that.
> 
> My first plan was to counter that by using zynq-reset to hold the
> reset high during
> reconfiguration of that part of the FPGA.
> 
> I'm happy to rethink that part and maybe redo the level shifters and
> resets together in a bridge
> driver under devicetree control gives finer grained control.

There is already a framework which is used to describe and manipulate
level shifting/other IO properties, and that is pinctrl, and if we
wanted to use an appropriate abstraction, I think pinctrl would be the
best bet.

Implementing the FPGA Bridge interface in the Zynq driver because it's
what the core expects is just backwards.  It's an abstraction inversion.

  Josh

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Moritz Fischer]

On Wed, Oct 28, 2015 at 9:18 AM, Josh Cartwright <joshc-acOepvfBmUk@public.gmane.org> wrote:
> On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
>> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <joshc-acOepvfBmUk@public.gmane.org> wrote:
>> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
>> >> From: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>> >>
>> >> The Simple FPGA bus uses the FPGA Manager Framework and the
>> >> FPGA Bridge Framework to provide a manufactorer-agnostic
>> >> interface for reprogramming FPGAs that is Device Tree
>> >> Overlays-based.
>> >
>> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
>> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
>> > all, from what I can tell.
>>
>> For Zynq the zynq-fpga driver takes care of the level shifters on full
>> reconfiguration,
>> and doesn't for partial reconfiguration. Now depending on which parts
>> of the fabric
>> are partial reconfigured (say AXI masters), one might run into issues
>> with a setup like that.
>>
>> My first plan was to counter that by using zynq-reset to hold the
>> reset high during
>> reconfiguration of that part of the FPGA.
>>
>> I'm happy to rethink that part and maybe redo the level shifters and
>> resets together in a bridge
>> driver under devicetree control gives finer grained control.
>
> There is already a framework which is used to describe and manipulate
> level shifting/other IO properties, and that is pinctrl, and if we
> wanted to use an appropriate abstraction, I think pinctrl would be the
> best bet.

Alright, I'll investigate that. Again, for the non-partial reconfig
case I'm happy
with the behavior as implemented, for the partial reconfig I just
haven't run into
issues with not dealing with the level shifters.

> Implementing the FPGA Bridge interface in the Zynq driver because it's
> what the core expects is just backwards.  It's an abstraction inversion.

Yeah, you're probably right.

Thanks,

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

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[atull]

On Wed, 28 Oct 2015, Moritz Fischer wrote:

> On Wed, Oct 28, 2015 at 9:18 AM, Josh Cartwright <joshc@ni.com> wrote:
> > On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> >> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <joshc@ni.com> wrote:
> >> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
> >> >> From: Alan Tull <atull@opensource.altera.com>
> >> >>
> >> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> >> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> >> >> interface for reprogramming FPGAs that is Device Tree
> >> >> Overlays-based.
> >> >
> >> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> >> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> >> > all, from what I can tell.
> >>
> >> For Zynq the zynq-fpga driver takes care of the level shifters on full
> >> reconfiguration,
> >> and doesn't for partial reconfiguration. Now depending on which parts
> >> of the fabric
> >> are partial reconfigured (say AXI masters), one might run into issues
> >> with a setup like that.
> >>
> >> My first plan was to counter that by using zynq-reset to hold the
> >> reset high during
> >> reconfiguration of that part of the FPGA.
> >>
> >> I'm happy to rethink that part and maybe redo the level shifters and
> >> resets together in a bridge
> >> driver under devicetree control gives finer grained control.
> >
> > There is already a framework which is used to describe and manipulate
> > level shifting/other IO properties, and that is pinctrl, and if we
> > wanted to use an appropriate abstraction, I think pinctrl would be the
> > best bet.
> 
> Alright, I'll investigate that. Again, for the non-partial reconfig
> case I'm happy
> with the behavior as implemented, for the partial reconfig I just
> haven't run into
> issues with not dealing with the level shifters.

Are you suggesting pinctrl instead of introducing a FPGA Bridge Framework?
If it fits, that's great.  Steffen is urging us to include reconfiguring
width of the bridge so I'm trying to figure how and if that all fits in
here.

> 
> > Implementing the FPGA Bridge interface in the Zynq driver because it's
> > what the core expects is just backwards.  It's an abstraction inversion.
> 
> Yeah, you're probably right.

Sorry I wasn't clear earlier, but I am going to change simple FPGA bus
to be OK with the "fpga-bridges" property being left out.  The change is
to do that should just be for simple_fpga_bus_get_bridges() to return 0
if it doesn't find any bridges.  The FPGA can still get programmed and
the child devices get populated.  All the other FPGA bridge-related
function calls (simple_fpga_bus_bridge_enable/disable and
simple_fpga_bus_put_bridges) won't have any effect when
simple_fpga_bus_get_bridges leaves num_bridges==0 and bridges==NULL.

So if you don't need bridges, you can leave that out (once I've fixed
that).

Actually if you want to use simple fpga bus with FPGA bridges compiled out,
I could even have the fpga bridge APIs become empty functions.

Alan

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

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[atull]

On Wed, 28 Oct 2015, atull wrote:

> On Wed, 28 Oct 2015, Moritz Fischer wrote:
> 
> > On Wed, Oct 28, 2015 at 9:18 AM, Josh Cartwright <joshc@ni.com> wrote:
> > > On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> > >> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <joshc@ni.com> wrote:
> > >> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
> > >> >> From: Alan Tull <atull@opensource.altera.com>
> > >> >>
> > >> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> > >> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> > >> >> interface for reprogramming FPGAs that is Device Tree
> > >> >> Overlays-based.
> > >> >
> > >> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > >> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> > >> > all, from what I can tell.
> > >>
> > >> For Zynq the zynq-fpga driver takes care of the level shifters on full
> > >> reconfiguration,
> > >> and doesn't for partial reconfiguration. Now depending on which parts
> > >> of the fabric
> > >> are partial reconfigured (say AXI masters), one might run into issues
> > >> with a setup like that.
> > >>
> > >> My first plan was to counter that by using zynq-reset to hold the
> > >> reset high during
> > >> reconfiguration of that part of the FPGA.
> > >>
> > >> I'm happy to rethink that part and maybe redo the level shifters and
> > >> resets together in a bridge
> > >> driver under devicetree control gives finer grained control.
> > >
> > > There is already a framework which is used to describe and manipulate
> > > level shifting/other IO properties, and that is pinctrl, and if we
> > > wanted to use an appropriate abstraction, I think pinctrl would be the
> > > best bet.
> > 
> > Alright, I'll investigate that. Again, for the non-partial reconfig
> > case I'm happy
> > with the behavior as implemented, for the partial reconfig I just
> > haven't run into
> > issues with not dealing with the level shifters.
> 
> Are you suggesting pinctrl instead of introducing a FPGA Bridge Framework?
> If it fits, that's great.  Steffen is urging us to include reconfiguring
> width of the bridge so I'm trying to figure how and if that all fits in
> here.
> 

Hi Josh,

I don't think pinctrl is a good match here or even could be made to do
what we need here.  Simple FPGA Bus needs, at minimum, an API to call
to enable/disable data through a bridge to protect the processor bus
from spurious data while the FPGA is being programmed.

Alan

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Wed, Oct 28, 2015 at 12:03:41PM -0500, atull wrote:
> On Wed, 28 Oct 2015, Moritz Fischer wrote:
> 
> > On Wed, Oct 28, 2015 at 9:18 AM, Josh Cartwright <joshc@ni.com> wrote:
> > > On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> > >> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <joshc@ni.com> wrote:
> > >> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
> > >> >> From: Alan Tull <atull@opensource.altera.com>
> > >> >>
> > >> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> > >> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> > >> >> interface for reprogramming FPGAs that is Device Tree
> > >> >> Overlays-based.
> > >> >
> > >> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > >> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> > >> > all, from what I can tell.
> > >>
> > >> For Zynq the zynq-fpga driver takes care of the level shifters on full
> > >> reconfiguration,
> > >> and doesn't for partial reconfiguration. Now depending on which parts
> > >> of the fabric
> > >> are partial reconfigured (say AXI masters), one might run into issues
> > >> with a setup like that.
> > >>
> > >> My first plan was to counter that by using zynq-reset to hold the
> > >> reset high during
> > >> reconfiguration of that part of the FPGA.
> > >>
> > >> I'm happy to rethink that part and maybe redo the level shifters and
> > >> resets together in a bridge
> > >> driver under devicetree control gives finer grained control.
> > >
> > > There is already a framework which is used to describe and manipulate
> > > level shifting/other IO properties, and that is pinctrl, and if we
> > > wanted to use an appropriate abstraction, I think pinctrl would be the
> > > best bet.
> > 
> > Alright, I'll investigate that. Again, for the non-partial reconfig
> > case I'm happy
> > with the behavior as implemented, for the partial reconfig I just
> > haven't run into
> > issues with not dealing with the level shifters.
> 
> Are you suggesting pinctrl instead of introducing a FPGA Bridge Framework?

I'm suggesting that for the set of operations/configuration states that
need to be managed _for the Zynq[1]_ during reprogramming, I think
pinctrl might be a good fit.

But the pinctrl state activation would happen in the context of the zynq
fpga_mgr_ops write

I do not think it's a good fit for the socfpga, or for the lower level
fpga drivers _in general_.  Nor do I think that the FPGA Bridge
framework, as written, is a good fit for fpgas in general.

  Josh

[1]: Speaking only of the Zynq 7000-series, I don't know anything about
     the fancy new Zynq MPSoC :)

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Wed, Oct 28, 2015 at 12:59:16PM -0500, Josh Cartwright wrote:
> On Wed, Oct 28, 2015 at 12:03:41PM -0500, atull wrote:
> > On Wed, 28 Oct 2015, Moritz Fischer wrote:
> > 
> > > On Wed, Oct 28, 2015 at 9:18 AM, Josh Cartwright <joshc@ni.com> wrote:
> > > > On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> > > >> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <joshc@ni.com> wrote:
> > > >> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
> > > >> >> From: Alan Tull <atull@opensource.altera.com>
> > > >> >>
> > > >> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> > > >> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> > > >> >> interface for reprogramming FPGAs that is Device Tree
> > > >> >> Overlays-based.
> > > >> >
> > > >> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > > >> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> > > >> > all, from what I can tell.
> > > >>
> > > >> For Zynq the zynq-fpga driver takes care of the level shifters on full
> > > >> reconfiguration,
> > > >> and doesn't for partial reconfiguration. Now depending on which parts
> > > >> of the fabric
> > > >> are partial reconfigured (say AXI masters), one might run into issues
> > > >> with a setup like that.
> > > >>
> > > >> My first plan was to counter that by using zynq-reset to hold the
> > > >> reset high during
> > > >> reconfiguration of that part of the FPGA.
> > > >>
> > > >> I'm happy to rethink that part and maybe redo the level shifters and
> > > >> resets together in a bridge
> > > >> driver under devicetree control gives finer grained control.
> > > >
> > > > There is already a framework which is used to describe and manipulate
> > > > level shifting/other IO properties, and that is pinctrl, and if we
> > > > wanted to use an appropriate abstraction, I think pinctrl would be the
> > > > best bet.
> > > 
> > > Alright, I'll investigate that. Again, for the non-partial reconfig
> > > case I'm happy
> > > with the behavior as implemented, for the partial reconfig I just
> > > haven't run into
> > > issues with not dealing with the level shifters.
> > 
> > Are you suggesting pinctrl instead of introducing a FPGA Bridge Framework?
> 
> I'm suggesting that for the set of operations/configuration states that
> need to be managed _for the Zynq[1]_ during reprogramming, I think
> pinctrl might be a good fit.
> 
> But the pinctrl state activation would happen in the context of the zynq
> fpga_mgr_ops write

*grr*... in the context of the zynq's write_init(), and write_complete()
callbacks.

  Josh

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[atull]

On Wed, 28 Oct 2015, Josh Cartwright wrote:

> On Wed, Oct 28, 2015 at 12:03:41PM -0500, atull wrote:
> > On Wed, 28 Oct 2015, Moritz Fischer wrote:
> > 
> > > On Wed, Oct 28, 2015 at 9:18 AM, Josh Cartwright <joshc@ni.com> wrote:
> > > > On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> > > >> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <joshc@ni.com> wrote:
> > > >> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
> > > >> >> From: Alan Tull <atull@opensource.altera.com>
> > > >> >>
> > > >> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> > > >> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> > > >> >> interface for reprogramming FPGAs that is Device Tree
> > > >> >> Overlays-based.
> > > >> >
> > > >> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > > >> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> > > >> > all, from what I can tell.
> > > >>
> > > >> For Zynq the zynq-fpga driver takes care of the level shifters on full
> > > >> reconfiguration,
> > > >> and doesn't for partial reconfiguration. Now depending on which parts
> > > >> of the fabric
> > > >> are partial reconfigured (say AXI masters), one might run into issues
> > > >> with a setup like that.
> > > >>
> > > >> My first plan was to counter that by using zynq-reset to hold the
> > > >> reset high during
> > > >> reconfiguration of that part of the FPGA.
> > > >>
> > > >> I'm happy to rethink that part and maybe redo the level shifters and
> > > >> resets together in a bridge
> > > >> driver under devicetree control gives finer grained control.
> > > >
> > > > There is already a framework which is used to describe and manipulate
> > > > level shifting/other IO properties, and that is pinctrl, and if we
> > > > wanted to use an appropriate abstraction, I think pinctrl would be the
> > > > best bet.
> > > 
> > > Alright, I'll investigate that. Again, for the non-partial reconfig
> > > case I'm happy
> > > with the behavior as implemented, for the partial reconfig I just
> > > haven't run into
> > > issues with not dealing with the level shifters.
> > 
> > Are you suggesting pinctrl instead of introducing a FPGA Bridge Framework?
> 
> I'm suggesting that for the set of operations/configuration states that
> need to be managed _for the Zynq[1]_ during reprogramming, I think
> pinctrl might be a good fit.

Hi Josh,

OK, thanks for the clarification.  Communication is hard :)

> 
> But the pinctrl state activation would happen in the context of the zynq
> fpga_mgr_ops write
> 
> I do not think it's a good fit for the socfpga, or for the lower level
> fpga drivers _in general_.  Nor do I think that the FPGA Bridge
> framework, as written, is a good fit for fpgas in general.

OK so about that last sentence...

For full reconfiguration, we could control the hardware bridges from the
FPGA Manager and that would be fine, because the area affected by the bridge
is the whole area, which is what the FPGA Manager will reprogram.

For partial reconfiguration, we can have one FPGA Manager programming
several PR areas.  Each PR area will need its own bridge within the
FPGA.  It no longer makes sense to control bridges from the manager.
So Xilinx may need this yet, for PR.

And again, if you don't need it, you won't have to deal with it.  I'll make
it so that the simple fpga bus works fine with the fpga bridges compiled out
even.

Alan

> 
>   Josh
> 
> [1]: Speaking only of the Zynq 7000-series, I don't know anything about
>      the fancy new Zynq MPSoC :)
> 

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Moritz Fischer]

[..]
> I do not think it's a good fit for the socfpga, or for the lower level
> fpga drivers _in general_.  Nor do I think that the FPGA Bridge
> framework, as written, is a good fit for fpgas in general.

Could you elaborate a bit more on why you feel that way? For all
configurations that I could
create in my mind (even hierarchical) I could come up with a way to
model that using the
proposed architecture in a very natural way. I'm of course aware that
my imagination is limited ;-)

Cheers,

Moritz

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Rob Herring]

On Wed, Oct 28, 2015 at 11:18 AM, Josh Cartwright <joshc@ni.com> wrote:
> On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
>> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <joshc@ni.com> wrote:
>> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, atull@opensource.altera.com wrote:
>> >> From: Alan Tull <atull@opensource.altera.com>
>> >>
>> >> The Simple FPGA bus uses the FPGA Manager Framework and the
>> >> FPGA Bridge Framework to provide a manufactorer-agnostic
>> >> interface for reprogramming FPGAs that is Device Tree
>> >> Overlays-based.
>> >
>> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
>> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
>> > all, from what I can tell.
>>
>> For Zynq the zynq-fpga driver takes care of the level shifters on full
>> reconfiguration,
>> and doesn't for partial reconfiguration. Now depending on which parts
>> of the fabric
>> are partial reconfigured (say AXI masters), one might run into issues
>> with a setup like that.
>>
>> My first plan was to counter that by using zynq-reset to hold the
>> reset high during
>> reconfiguration of that part of the FPGA.
>>
>> I'm happy to rethink that part and maybe redo the level shifters and
>> resets together in a bridge
>> driver under devicetree control gives finer grained control.
>
> There is already a framework which is used to describe and manipulate
> level shifting/other IO properties, and that is pinctrl, and if we
> wanted to use an appropriate abstraction, I think pinctrl would be the
> best bet.

I'm not all that familiar with FPGAs, but using pinctrl seems like
overkill to me. We're talking about isolating things while doing
reconfiguration, right? That seems more similar to power domain
controls than I/O pin control to me.

> Implementing the FPGA Bridge interface in the Zynq driver because it's
> what the core expects is just backwards.  It's an abstraction inversion.

+1

Rob

[PATCH v12 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 FPGAs.

The functions are:
* of_fpga_bridge_get
* fpga_bridge_put
   Get/put a 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.

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
---
 drivers/fpga/Kconfig             |    7 ++
 drivers/fpga/Makefile            |    3 +
 drivers/fpga/fpga-bridge.c       |  242 ++++++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-bridge.h |   49 ++++++++
 4 files changed, 301 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 e8f5453..143072b 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -32,6 +32,13 @@ config FPGA_MGR_SOCFPGA_A10
 	help
 	  FPGA manager driver support for Altera Arria10 SoCFPGA.
 
+config FPGA_BRIDGE
+       bool "FPGA Bridge Drivers"
+       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 0385622..9302662 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -9,5 +9,8 @@ obj-$(CONFIG_FPGA)			+= fpga-mgr.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
 
+# FPGA Bridge Drivers
+obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
+
 # High Level Interfaces
 obj-$(CONFIG_SIMPLE_FPGA_BUS)		+= simple-fpga-bus.o
diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
new file mode 100644
index 0000000..c135988
--- /dev/null
+++ b/drivers/fpga/fpga-bridge.c
@@ -0,0 +1,242 @@
+/*
+ * fpga bridge 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>
+
+static DEFINE_IDA(fpga_bridge_ida);
+static struct class *fpga_bridge_class;
+
+/**
+ * fpga_bridge_enable
+ * @bridge: fpga bridge
+ *
+ * Enable transactions on the bridge
+ *
+ * Return: 0 for success, error code otherwise.
+ */
+int fpga_bridge_enable(struct fpga_bridge *bridge)
+{
+	pr_err("%s %s\n", __func__, dev_name(&bridge->dev));
+
+	return bridge->br_ops->enable_set(bridge, 1);
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_enable);
+
+/**
+ * fpga_bridge_disable
+ * @bridge: fpga bridge
+ *
+ * Disable transactions on the bridge
+ *
+ * Return: 0 for success, error code otherwise.
+ */
+int fpga_bridge_disable(struct fpga_bridge *bridge)
+{
+	pr_err("%s %s\n", __func__, dev_name(&bridge->dev));
+
+	return bridge->br_ops->enable_set(bridge, 0);
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_disable);
+
+static int fpga_bridge_of_node_match(struct device *dev, const void *data)
+{
+	return dev->of_node == data;
+}
+
+/**
+ * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
+ * @node: device node
+ *
+ * Given a device node, get an exclusive reference to a fpga bridge.
+ *
+ * Returns a fpga manager struct or IS_ERR() condition containing errno.
+ */
+struct fpga_bridge *of_fpga_bridge_get(struct device_node *node)
+{
+	struct device *dev;
+	struct fpga_bridge *bridge;
+	int ret = -ENODEV;
+
+	dev = class_find_device(fpga_bridge_class, NULL, node,
+				fpga_bridge_of_node_match);
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	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;
+
+	return bridge;
+
+err_ll_mod:
+	mutex_unlock(&bridge->mutex);
+err_dev:
+	put_device(dev);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
+
+/**
+ * fpga_bridge_put - release a reference to a fpga bridge
+ * @bridge: fpga bridge
+ */
+void fpga_bridge_put(struct fpga_bridge *bridge)
+{
+	module_put(bridge->dev.parent->driver->owner);
+	mutex_unlock(&bridge->mutex);
+	put_device(&bridge->dev);
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_put);
+
+/**
+ * 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 on success, negative error code otherwise.
+ */
+int fpga_bridge_register(struct device *dev, const char *name,
+			 struct fpga_bridge_ops *br_ops, void *priv)
+{
+	struct fpga_bridge *bridge;
+	const char *dt_label;
+	int id, ret = 0;
+
+	if (!br_ops || !br_ops->enable_set || !br_ops->enable_show) {
+		dev_err(dev, "Attempt to register without fpga_bridge_ops\n");
+		return -EINVAL;
+	}
+
+	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);
+
+	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);
+
+	dt_label = of_get_property(bridge->dev.of_node, "label", NULL);
+	if (dt_label)
+		ret = dev_set_name(&bridge->dev, "%s", dt_label);
+	else
+		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);
+
+void fpga_bridge_unregister(struct device *dev)
+{
+	struct fpga_bridge *bridge = dev_get_drvdata(dev);
+
+	dev_info(&bridge->dev, "%s : %s\n", __func__, bridge->name);
+
+	/*
+	 * If the low level driver provides a method for putting bridge into
+	 * a desired state upon unregister, do it.
+	 */
+	if (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)
+{
+	pr_info("FPGA bridge framework driver\n");
+
+	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_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..da6791e
--- /dev/null
+++ b/include/linux/fpga/fpga-bridge.h
@@ -0,0 +1,49 @@
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/platform_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
+ * @br_ops: pointer to struct of fpga bridge ops
+ * @priv: low level driver private date
+ */
+struct fpga_bridge {
+	const char *name;
+	struct device dev;
+	struct mutex mutex;
+	struct fpga_bridge_ops *br_ops;
+	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_bridge_register(struct device *dev, const char *name,
+			 struct fpga_bridge_ops *br_ops, void *priv);
+void fpga_bridge_unregister(struct device *dev);
+
+#endif /* _LINUX_FPGA_BRIDGE_H */
-- 
1.7.9.5

Re: [PATCH v12 4/6] fpga: add fpga bridge framework

[Steffen Trumtrar]

On Tue, Oct 27, 2015 at 05:09:13PM -0500, atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
> From: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> 
> 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 FPGAs.
> 
> The functions are:
> * of_fpga_bridge_get
> * fpga_bridge_put
>    Get/put a 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.
> 
> Signed-off-by: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> ---
> 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
> ---
>  drivers/fpga/Kconfig             |    7 ++
>  drivers/fpga/Makefile            |    3 +
>  drivers/fpga/fpga-bridge.c       |  242 ++++++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-bridge.h |   49 ++++++++
>  4 files changed, 301 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 e8f5453..143072b 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -32,6 +32,13 @@ config FPGA_MGR_SOCFPGA_A10
>  	help
>  	  FPGA manager driver support for Altera Arria10 SoCFPGA.
>  
> +config FPGA_BRIDGE
> +       bool "FPGA Bridge Drivers"
> +       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 0385622..9302662 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -9,5 +9,8 @@ obj-$(CONFIG_FPGA)			+= fpga-mgr.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
>  
> +# FPGA Bridge Drivers
> +obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
> +
>  # High Level Interfaces
>  obj-$(CONFIG_SIMPLE_FPGA_BUS)		+= simple-fpga-bus.o
> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> new file mode 100644
> index 0000000..c135988
> --- /dev/null
> +++ b/drivers/fpga/fpga-bridge.c
> @@ -0,0 +1,242 @@
> +/*
> + * fpga bridge 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>
> +
> +static DEFINE_IDA(fpga_bridge_ida);
> +static struct class *fpga_bridge_class;
> +
> +/**
> + * fpga_bridge_enable
> + * @bridge: fpga bridge
> + *
> + * Enable transactions on the bridge
> + *
> + * Return: 0 for success, error code otherwise.
> + */
> +int fpga_bridge_enable(struct fpga_bridge *bridge)
> +{
> +	pr_err("%s %s\n", __func__, dev_name(&bridge->dev));

Please clean this...

> +
> +	return bridge->br_ops->enable_set(bridge, 1);
> +}
> +EXPORT_SYMBOL_GPL(fpga_bridge_enable);
> +
> +/**
> + * fpga_bridge_disable
> + * @bridge: fpga bridge
> + *
> + * Disable transactions on the bridge
> + *
> + * Return: 0 for success, error code otherwise.
> + */
> +int fpga_bridge_disable(struct fpga_bridge *bridge)
> +{
> +	pr_err("%s %s\n", __func__, dev_name(&bridge->dev));

and this up.

> +
> +	return bridge->br_ops->enable_set(bridge, 0);
> +}
> +EXPORT_SYMBOL_GPL(fpga_bridge_disable);
> +
> +static int fpga_bridge_of_node_match(struct device *dev, const void *data)
> +{
> +	return dev->of_node == data;
> +}
> +
> +/**
> + * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
> + * @node: device node
> + *
> + * Given a device node, get an exclusive reference to a fpga bridge.
> + *
> + * Returns a fpga manager struct or IS_ERR() condition containing errno.
> + */
> +struct fpga_bridge *of_fpga_bridge_get(struct device_node *node)
> +{
> +	struct device *dev;
> +	struct fpga_bridge *bridge;
> +	int ret = -ENODEV;
> +
> +	dev = class_find_device(fpga_bridge_class, NULL, node,
> +				fpga_bridge_of_node_match);
> +	if (!dev)
> +		return ERR_PTR(-ENODEV);
> +
> +	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;
> +
> +	return bridge;
> +
> +err_ll_mod:
> +	mutex_unlock(&bridge->mutex);
> +err_dev:
> +	put_device(dev);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
> +
> +/**
> + * fpga_bridge_put - release a reference to a fpga bridge
> + * @bridge: fpga bridge
> + */
> +void fpga_bridge_put(struct fpga_bridge *bridge)
> +{
> +	module_put(bridge->dev.parent->driver->owner);
> +	mutex_unlock(&bridge->mutex);
> +	put_device(&bridge->dev);
> +}
> +EXPORT_SYMBOL_GPL(fpga_bridge_put);
> +
> +/**
> + * 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 on success, negative error code otherwise.
> + */
> +int fpga_bridge_register(struct device *dev, const char *name,
> +			 struct fpga_bridge_ops *br_ops, void *priv)
> +{
> +	struct fpga_bridge *bridge;
> +	const char *dt_label;
> +	int id, ret = 0;
> +
> +	if (!br_ops || !br_ops->enable_set || !br_ops->enable_show) {
> +		dev_err(dev, "Attempt to register without fpga_bridge_ops\n");
> +		return -EINVAL;
> +	}
> +
> +	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);
> +
> +	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);
> +
> +	dt_label = of_get_property(bridge->dev.of_node, "label", NULL);
> +	if (dt_label)
> +		ret = dev_set_name(&bridge->dev, "%s", dt_label);
> +	else
> +		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);
> +
> +void fpga_bridge_unregister(struct device *dev)
> +{
> +	struct fpga_bridge *bridge = dev_get_drvdata(dev);
> +
> +	dev_info(&bridge->dev, "%s : %s\n", __func__, bridge->name);

Is this necessary information?

> +
> +	/*
> +	 * If the low level driver provides a method for putting bridge into
> +	 * a desired state upon unregister, do it.
> +	 */
> +	if (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)
> +{
> +	pr_info("FPGA bridge framework driver\n");

Dito.
IMHO unnecessary log spam. Maybe change this to dbg?

> +
> +	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_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-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>");
> +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..da6791e
> --- /dev/null
> +++ b/include/linux/fpga/fpga-bridge.h
> @@ -0,0 +1,49 @@
> +#include <linux/cdev.h>

You don't seem to use this.

> +#include <linux/device.h>
> +#include <linux/platform_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
> + * @br_ops: pointer to struct of fpga bridge ops
> + * @priv: low level driver private date
> + */
> +struct fpga_bridge {
> +	const char *name;
> +	struct device dev;
> +	struct mutex mutex;
> +	struct fpga_bridge_ops *br_ops;
> +	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_bridge_register(struct device *dev, const char *name,
> +			 struct fpga_bridge_ops *br_ops, void *priv);
> +void fpga_bridge_unregister(struct device *dev);
> +
> +#endif /* _LINUX_FPGA_BRIDGE_H */

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

Re: [PATCH v12 4/6] fpga: add fpga bridge framework

[atull]

On Wed, 28 Oct 2015, Steffen Trumtrar wrote:

> > +int fpga_bridge_enable(struct fpga_bridge *bridge)
> > +{
> > +	pr_err("%s %s\n", __func__, dev_name(&bridge->dev));
> 
> Please clean this...
> 
> > +
> > +	return bridge->br_ops->enable_set(bridge, 1);
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_bridge_enable);
> > +
> > +/**
> > + * fpga_bridge_disable
> > + * @bridge: fpga bridge
> > + *
> > + * Disable transactions on the bridge
> > + *
> > + * Return: 0 for success, error code otherwise.
> > + */
> > +int fpga_bridge_disable(struct fpga_bridge *bridge)
> > +{
> > +	pr_err("%s %s\n", __func__, dev_name(&bridge->dev));
> 
> and this up.
> 

OK

> > +void fpga_bridge_unregister(struct device *dev)
> > +{
> > +	struct fpga_bridge *bridge = dev_get_drvdata(dev);
> > +
> > +	dev_info(&bridge->dev, "%s : %s\n", __func__, bridge->name);
> 
> Is this necessary information?

I can remove it.

> > +static int __init fpga_bridge_dev_init(void)
> > +{
> > +	pr_info("FPGA bridge framework driver\n");
> 
> Dito.
> IMHO unnecessary log spam. Maybe change this to dbg?

Sure.

> > --- /dev/null
> > +++ b/include/linux/fpga/fpga-bridge.h
> > @@ -0,0 +1,49 @@
> > +#include <linux/cdev.h>
> 
> You don't seem to use this.

Correct.  I'll take it out.

Thanks for the review!

Alan

[PATCH v12 5/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 <dinguyen@opensource.altera.com>
Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
Signed-off-by: Matthew Gerlach <mgerlach@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
---
 .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   18 ++++++++++
 .../bindings/fpga/altera-hps2fpga-bridge.txt       |   36 ++++++++++++++++++++
 2 files changed, 54 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..11eb5b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
@@ -0,0 +1,18 @@
+Altera FPGA To SDRAM Bridge Driver
+
+Required properties:
+- compatible		: Should contain "altr,socfpga-fpga2sdram-bridge"
+
+Optional properties:
+- label			: User-readable name for this bridge.
+			  Default is br<device#>
+- init-val		: 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: fpgabridge@3 {
+		compatible = "altr,socfpga-fpga2sdram-bridge";
+		label = "fpga2sdram";
+		init-val = <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..eb52f3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
@@ -0,0 +1,36 @@
+Altera FPGA/HPS Bridge Driver
+
+Required properties:
+- compatible	: Should contain one of:
+		  "altr,socfpga-hps2fpga-bridge",
+		  "altr,socfpga-lwhps2fpga-bridge", or
+		  "altr,socfpga-fpga2hps-bridge"
+- clocks	: Clocks used by this module.
+
+Optional properties:
+- label		: User-readable name for this bridge.
+		  Default is br<device#>
+- init-val	: 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@0 {
+		compatible = "altr,socfpga-hps2fpga-bridge";
+		label = "hps2fpga";
+		clocks = <&l4_main_clk>;
+		init-val = <1>;
+	};
+
+	hps_fpgabridge1: fpgabridge@1 {
+		compatible = "altr,socfpga-lwhps2fpga-bridge";
+		label = "lwhps2fpga";
+		clocks = <&l4_main_clk>;
+		init-val = <0>;
+	};
+
+	hps_fpgabridge2: fpgabridge@2 {
+		compatible = "altr,socfpga-fpga2hps-bridge";
+		label = "fpga2hps";
+		clocks = <&l4_main_clk>;
+	};
-- 
1.7.9.5

Re: [PATCH v12 5/6] ARM: socfpga: add bindings document for fpga bridge drivers

[Steffen Trumtrar]

Hi!

On Tue, Oct 27, 2015 at 05:09:14PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add bindings documentation for Altera SOCFPGA bridges:
>  * fpga2sdram
>  * fpga2hps
>  * hps2fpga
>  * lwhps2fpga
> 
> Signed-off-by: Alan Tull <dinguyen@opensource.altera.com>
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Signed-off-by: Matthew Gerlach <mgerlach@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
> ---
>  .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   18 ++++++++++
>  .../bindings/fpga/altera-hps2fpga-bridge.txt       |   36 ++++++++++++++++++++
>  2 files changed, 54 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..11eb5b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> @@ -0,0 +1,18 @@
> +Altera FPGA To SDRAM Bridge Driver
> +
> +Required properties:
> +- compatible		: Should contain "altr,socfpga-fpga2sdram-bridge"
> +
> +Optional properties:
> +- label			: User-readable name for this bridge.
> +			  Default is br<device#>
> +- init-val		: 0 if driver should disable bridge at startup
> +			  1 if driver should enable bridge at startup
> +			  Default is to leave bridge in current state.

I know, that there are bindings, that configure via devicetree, but AFAIK
this is not okay.

> +
> +Example:
> +	fpga2sdram_br: fpgabridge@3 {
> +		compatible = "altr,socfpga-fpga2sdram-bridge";
> +		label = "fpga2sdram";
> +		init-val = <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..eb52f3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> @@ -0,0 +1,36 @@
> +Altera FPGA/HPS Bridge Driver
> +
> +Required properties:
> +- compatible	: Should contain one of:
> +		  "altr,socfpga-hps2fpga-bridge",
> +		  "altr,socfpga-lwhps2fpga-bridge", or
> +		  "altr,socfpga-fpga2hps-bridge"
> +- clocks	: Clocks used by this module.
> +
> +Optional properties:
> +- label		: User-readable name for this bridge.
> +		  Default is br<device#>
> +- init-val	: 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@0 {
> +		compatible = "altr,socfpga-hps2fpga-bridge";
> +		label = "hps2fpga";
> +		clocks = <&l4_main_clk>;
> +		init-val = <1>;
> +	};
> +
> +	hps_fpgabridge1: fpgabridge@1 {
> +		compatible = "altr,socfpga-lwhps2fpga-bridge";
> +		label = "lwhps2fpga";
> +		clocks = <&l4_main_clk>;
> +		init-val = <0>;
> +	};
> +
> +	hps_fpgabridge2: fpgabridge@2 {
> +		compatible = "altr,socfpga-fpga2hps-bridge";
> +		label = "fpga2hps";
> +		clocks = <&l4_main_clk>;
> +	};

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

Re: [PATCH v12 5/6] ARM: socfpga: add bindings document for fpga bridge drivers

[atull]

On Wed, 28 Oct 2015, Steffen Trumtrar wrote:

> Hi!
> 
> On Tue, Oct 27, 2015 at 05:09:14PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Add bindings documentation for Altera SOCFPGA bridges:
> >  * fpga2sdram
> >  * fpga2hps
> >  * hps2fpga
> >  * lwhps2fpga
> > 
> > Signed-off-by: Alan Tull <dinguyen@opensource.altera.com>
> > Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Signed-off-by: Matthew Gerlach <mgerlach@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
> > ---
> >  .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   18 ++++++++++
> >  .../bindings/fpga/altera-hps2fpga-bridge.txt       |   36 ++++++++++++++++++++
> >  2 files changed, 54 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..11eb5b7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> > @@ -0,0 +1,18 @@
> > +Altera FPGA To SDRAM Bridge Driver
> > +
> > +Required properties:
> > +- compatible		: Should contain "altr,socfpga-fpga2sdram-bridge"
> > +
> > +Optional properties:
> > +- label			: User-readable name for this bridge.
> > +			  Default is br<device#>
> > +- init-val		: 0 if driver should disable bridge at startup
> > +			  1 if driver should enable bridge at startup
> > +			  Default is to leave bridge in current state.
> 
> I know, that there are bindings, that configure via devicetree, but AFAIK
> this is not okay.

Yes there is configuration in the DT. I attended a presentation at ELC in 
Dublin 3 weeks ago - Frank Rowand on "Solving Device Tree Issues."  He said in 
addition to non-probable hardware information, it was a "dirty secret" that DT
includes configuration information and some policy.

Alan

Re: [PATCH v12 5/6] ARM: socfpga: add bindings document for fpga bridge drivers

[Rob Herring]

On Tue, Oct 27, 2015 at 5:09 PM,  <atull@opensource.altera.com> wrote:
> From: Alan Tull <atull@opensource.altera.com>
>
> Add bindings documentation for Altera SOCFPGA bridges:
>  * fpga2sdram
>  * fpga2hps
>  * hps2fpga
>  * lwhps2fpga
>
> Signed-off-by: Alan Tull <dinguyen@opensource.altera.com>

Oops...

> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Signed-off-by: Matthew Gerlach <mgerlach@altera.com>

These should be roughly in order of who did modifications. I'd expect
you to be last.

> ---
> 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
> ---
>  .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   18 ++++++++++
>  .../bindings/fpga/altera-hps2fpga-bridge.txt       |   36 ++++++++++++++++++++
>  2 files changed, 54 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..11eb5b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> @@ -0,0 +1,18 @@
> +Altera FPGA To SDRAM Bridge Driver
> +
> +Required properties:
> +- compatible           : Should contain "altr,socfpga-fpga2sdram-bridge"
> +
> +Optional properties:
> +- label                        : User-readable name for this bridge.
> +                         Default is br<device#>

Why does the user need label? We generally use label to match physical
labels like "Rear USB port" or "disk LED" or something.

> +- init-val             : 0 if driver should disable bridge at startup
> +                         1 if driver should enable bridge at startup

Perhaps "bridge-enable" would be a more descriptive name.

And to comment on other replies, I have no problem with this type of
property in the DT. But yes, configuration type properties will get
more scrutiny.

> +                         Default is to leave bridge in current state.
> +
> +Example:
> +       fpga2sdram_br: fpgabridge@3 {
> +               compatible = "altr,socfpga-fpga2sdram-bridge";
> +               label = "fpga2sdram";
> +               init-val = <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..eb52f3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> @@ -0,0 +1,36 @@
> +Altera FPGA/HPS Bridge Driver
> +
> +Required properties:
> +- compatible   : Should contain one of:
> +                 "altr,socfpga-hps2fpga-bridge",
> +                 "altr,socfpga-lwhps2fpga-bridge", or
> +                 "altr,socfpga-fpga2hps-bridge"
> +- clocks       : Clocks used by this module.
> +
> +Optional properties:
> +- label                : User-readable name for this bridge.
> +                 Default is br<device#>
> +- init-val     : 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@0 {
> +               compatible = "altr,socfpga-hps2fpga-bridge";
> +               label = "hps2fpga";
> +               clocks = <&l4_main_clk>;
> +               init-val = <1>;
> +       };
> +
> +       hps_fpgabridge1: fpgabridge@1 {
> +               compatible = "altr,socfpga-lwhps2fpga-bridge";
> +               label = "lwhps2fpga";
> +               clocks = <&l4_main_clk>;
> +               init-val = <0>;
> +       };
> +
> +       hps_fpgabridge2: fpgabridge@2 {
> +               compatible = "altr,socfpga-fpga2hps-bridge";
> +               label = "fpga2hps";
> +               clocks = <&l4_main_clk>;
> +       };
> --
> 1.7.9.5
>

Re: [PATCH v12 5/6] ARM: socfpga: add bindings document for fpga bridge drivers

[atull]

On Wed, 28 Oct 2015, Rob Herring wrote:

> On Tue, Oct 27, 2015 at 5:09 PM,  <atull@opensource.altera.com> wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> >
> > Add bindings documentation for Altera SOCFPGA bridges:
> >  * fpga2sdram
> >  * fpga2hps
> >  * hps2fpga
> >  * lwhps2fpga
> >
> > Signed-off-by: Alan Tull <dinguyen@opensource.altera.com>
> 
> Oops...

Wow. Ugh.

> 
> > Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Signed-off-by: Matthew Gerlach <mgerlach@altera.com>
> 
> These should be roughly in order of who did modifications. I'd expect
> you to be last.

I wrote the driver, Matthew, Dinh, and I all made mods.

> 
> > ---
> > 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
> > ---
> >  .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   18 ++++++++++
> >  .../bindings/fpga/altera-hps2fpga-bridge.txt       |   36 ++++++++++++++++++++
> >  2 files changed, 54 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..11eb5b7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> > @@ -0,0 +1,18 @@
> > +Altera FPGA To SDRAM Bridge Driver
> > +
> > +Required properties:
> > +- compatible           : Should contain "altr,socfpga-fpga2sdram-bridge"
> > +
> > +Optional properties:
> > +- label                        : User-readable name for this bridge.
> > +                         Default is br<device#>
> 
> Why does the user need label? We generally use label to match physical
> labels like "Rear USB port" or "disk LED" or something.
> 

I'll remove it.

> > +- init-val             : 0 if driver should disable bridge at startup
> > +                         1 if driver should enable bridge at startup
> 
> Perhaps "bridge-enable" would be a more descriptive name.
> 

I could change it to bridge-enable.  Alternatively I could have two
booleans that are bridge-enable and bridge-disable as:

  bridge-enable  : boolean property should be defined if driver should
                   enable bridge at startup
  bridge-disable : boolean property should be defined if driver should
                   disable bridge at startup
  Default if neither bridge-enable or bridge-disable are specified is
    driver will leave bridge in its current state.

> And to comment on other replies, I have no problem with this type of
> property in the DT. But yes, configuration type properties will get
> more scrutiny.
> 
> > +                         Default is to leave bridge in current state.
> > +
> > +Example:
> > +       fpga2sdram_br: fpgabridge@3 {
> > +               compatible = "altr,socfpga-fpga2sdram-bridge";
> > +               label = "fpga2sdram";
> > +               init-val = <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..eb52f3b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > @@ -0,0 +1,36 @@
> > +Altera FPGA/HPS Bridge Driver
> > +
> > +Required properties:
> > +- compatible   : Should contain one of:
> > +                 "altr,socfpga-hps2fpga-bridge",
> > +                 "altr,socfpga-lwhps2fpga-bridge", or
> > +                 "altr,socfpga-fpga2hps-bridge"
> > +- clocks       : Clocks used by this module.
> > +
> > +Optional properties:
> > +- label                : User-readable name for this bridge.
> > +                 Default is br<device#>
> > +- init-val     : 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@0 {
> > +               compatible = "altr,socfpga-hps2fpga-bridge";
> > +               label = "hps2fpga";
> > +               clocks = <&l4_main_clk>;
> > +               init-val = <1>;
> > +       };
> > +
> > +       hps_fpgabridge1: fpgabridge@1 {
> > +               compatible = "altr,socfpga-lwhps2fpga-bridge";
> > +               label = "lwhps2fpga";
> > +               clocks = <&l4_main_clk>;
> > +               init-val = <0>;
> > +       };
> > +
> > +       hps_fpgabridge2: fpgabridge@2 {
> > +               compatible = "altr,socfpga-fpga2hps-bridge";
> > +               label = "fpga2hps";
> > +               clocks = <&l4_main_clk>;
> > +       };
> > --
> > 1.7.9.5
> >
> 

[PATCH v12 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 <dinguyen@opensource.altera.com>
Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
Signed-off-by: Matthew Gerlach <mgerlach@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
---
 drivers/fpga/Kconfig             |    7 ++
 drivers/fpga/Makefile            |    1 +
 drivers/fpga/altera-fpga2sdram.c |  185 ++++++++++++++++++++++++++++++
 drivers/fpga/altera-hps2fpga.c   |  234 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 427 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 143072b..004b83a 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -39,6 +39,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 9302662..cc01db3 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.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_SIMPLE_FPGA_BUS)		+= simple-fpga-bus.o
diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
new file mode 100644
index 0000000..ee56903
--- /dev/null
+++ b/drivers/fpga/altera-fpga2sdram.c
@@ -0,0 +1,185 @@
+/*
+ * 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;
+	uint32_t *prop_value;
+	uint32_t prop_max;
+};
+
+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;
+	uint32_t init_val;
+	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, "init-val", &init_val)) {
+		if (init_val > 1) {
+			dev_warn(dev, "invalid init-val %u > 1\n", init_val);
+		} else {
+			dev_info(dev, "%s bridge\n",
+				(init_val ? "enabling" : "disabling"));
+			ret = _alt_fpga2sdram_enable_set(priv, init_val);
+			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 = {
+	.remove = alt_fpga_bridge_remove,
+	.driver = {
+		.name	= "altera_fpga2sdram_bridge",
+		.of_match_table = of_match_ptr(altera_fpga_of_match),
+	},
+};
+
+static int __init alt_fpga_bridge_init(void)
+{
+	return platform_driver_probe(&altera_fpga_driver,
+				     alt_fpga_bridge_probe);
+}
+
+static void __exit alt_fpga_bridge_exit(void)
+{
+	platform_driver_unregister(&altera_fpga_driver);
+}
+
+module_init(alt_fpga_bridge_init);
+module_exit(alt_fpga_bridge_exit);
+
+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..3ec6394
--- /dev/null
+++ b/drivers/fpga/altera-hps2fpga.c
@@ -0,0 +1,234 @@
+/*
+ * 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 SOCFPGA_RSTMGR_BRGMODRST		0x1c
+#define ALT_RSTMGR_BRGMODRST_H2F_MSK		0x00000001
+#define ALT_RSTMGR_BRGMODRST_LWH2F_MSK		0x00000002
+#define ALT_RSTMGR_BRGMODRST_F2H_MSK		0x00000004
+
+#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 reset_mask;
+	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);
+}
+
+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,
+	.reset_mask = ALT_RSTMGR_BRGMODRST_H2F_MSK,
+	.remap_mask = ALT_L3_REMAP_H2F_MSK,
+};
+
+static struct altera_hps2fpga_data lwhps2fpga_data  = {
+	.name = LWHPS2FPGA_BRIDGE_NAME,
+	.reset_mask = ALT_RSTMGR_BRGMODRST_LWH2F_MSK,
+	.remap_mask = ALT_L3_REMAP_LWH2F_MSK,
+};
+
+static struct altera_hps2fpga_data fpga2hps_data  = {
+	.name = FPGA2HPS_BRIDGE_NAME,
+	.reset_mask = ALT_RSTMGR_BRGMODRST_F2H_MSK,
+};
+
+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;
+	uint32_t init_val;
+	int ret;
+
+	of_id = of_match_device(altera_fpga_of_match, dev);
+	priv = (struct altera_hps2fpga_data *)of_id->data;
+	WARN_ON(!priv);
+
+	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, "init-val", &init_val)) {
+		if (init_val > 1) {
+			dev_warn(dev, "invalid init-val %u > 1\n", init_val);
+		} else {
+			dev_info(dev, "%s bridge\n",
+				(init_val ? "enabling" : "disabling"));
+
+			ret = _alt_hps2fpga_enable_set(priv, init_val);
+			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 altera_fpga_driver = {
+	.remove = alt_fpga_bridge_remove,
+	.driver = {
+		.name	= "altera_hps2fpga_bridge",
+		.of_match_table = of_match_ptr(altera_fpga_of_match),
+	},
+};
+
+static int __init alt_fpga_bridge_init(void)
+{
+	return platform_driver_probe(&altera_fpga_driver,
+				     alt_fpga_bridge_probe);
+}
+
+static void __exit alt_fpga_bridge_exit(void)
+{
+	platform_driver_unregister(&altera_fpga_driver);
+}
+
+module_init(alt_fpga_bridge_init);
+module_exit(alt_fpga_bridge_exit);
+
+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 v12 6/6] ARM: socfpga: fpga bridge driver support

[Steffen Trumtrar]

On Tue, Oct 27, 2015 at 05:09:15PM -0500, atull@opensource.altera.com wrote:
> 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 <dinguyen@opensource.altera.com>
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Signed-off-by: Matthew Gerlach <mgerlach@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
> ---
>  drivers/fpga/Kconfig             |    7 ++
>  drivers/fpga/Makefile            |    1 +
>  drivers/fpga/altera-fpga2sdram.c |  185 ++++++++++++++++++++++++++++++
>  drivers/fpga/altera-hps2fpga.c   |  234 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 427 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 143072b..004b83a 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -39,6 +39,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 9302662..cc01db3 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.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_SIMPLE_FPGA_BUS)		+= simple-fpga-bus.o
> diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
> new file mode 100644
> index 0000000..ee56903
> --- /dev/null
> +++ b/drivers/fpga/altera-fpga2sdram.c
> @@ -0,0 +1,185 @@
> +/*
> + * 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;
> +	uint32_t *prop_value;
> +	uint32_t prop_max;
> +};
> +
> +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;
> +	uint32_t init_val;
> +	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);

Hm, what does this do? All I can get from the documentation is, that this
is a general purpose register and has nothing to do with this particular bridge.

> +
> +	if (!of_property_read_u32(dev->of_node, "init-val", &init_val)) {
> +		if (init_val > 1) {
> +			dev_warn(dev, "invalid init-val %u > 1\n", init_val);
> +		} else {
> +			dev_info(dev, "%s bridge\n",
> +				(init_val ? "enabling" : "disabling"));
> +			ret = _alt_fpga2sdram_enable_set(priv, init_val);
> +			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 = {
> +	.remove = alt_fpga_bridge_remove,
> +	.driver = {
> +		.name	= "altera_fpga2sdram_bridge",
> +		.of_match_table = of_match_ptr(altera_fpga_of_match),
> +	},
> +};
> +
> +static int __init alt_fpga_bridge_init(void)
> +{
> +	return platform_driver_probe(&altera_fpga_driver,
> +				     alt_fpga_bridge_probe);
> +}
> +
> +static void __exit alt_fpga_bridge_exit(void)
> +{
> +	platform_driver_unregister(&altera_fpga_driver);
> +}
> +
> +module_init(alt_fpga_bridge_init);
> +module_exit(alt_fpga_bridge_exit);
> +
> +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..3ec6394
> --- /dev/null
> +++ b/drivers/fpga/altera-hps2fpga.c
> @@ -0,0 +1,234 @@
> +/*
> + * 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 SOCFPGA_RSTMGR_BRGMODRST		0x1c
> +#define ALT_RSTMGR_BRGMODRST_H2F_MSK		0x00000001
> +#define ALT_RSTMGR_BRGMODRST_LWH2F_MSK		0x00000002
> +#define ALT_RSTMGR_BRGMODRST_F2H_MSK		0x00000004
> +
> +#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 {
This name and the alt_hps2fpga_xxx functions are confusing, as you have
one bridge of this name but use it for all the bridges.

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

Why do you remap the On-chip RAM here. This doesn't belong in this
driver.

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

regmap_update_bits? Otherwise you will overwrite the other bridges
settings.

> +	}
> +
> +	return ret;
> +}
> +
> +static int alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
> +{
> +	return _alt_hps2fpga_enable_set(bridge->priv, enable);
> +}
> +
> +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,
> +	.reset_mask = ALT_RSTMGR_BRGMODRST_H2F_MSK,
> +	.remap_mask = ALT_L3_REMAP_H2F_MSK,
> +};
> +
> +static struct altera_hps2fpga_data lwhps2fpga_data  = {
> +	.name = LWHPS2FPGA_BRIDGE_NAME,
> +	.reset_mask = ALT_RSTMGR_BRGMODRST_LWH2F_MSK,
> +	.remap_mask = ALT_L3_REMAP_LWH2F_MSK,
> +};
> +
> +static struct altera_hps2fpga_data fpga2hps_data  = {
> +	.name = FPGA2HPS_BRIDGE_NAME,
> +	.reset_mask = ALT_RSTMGR_BRGMODRST_F2H_MSK,
> +};
> +
> +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;
> +	uint32_t init_val;
> +	int ret;
> +
> +	of_id = of_match_device(altera_fpga_of_match, dev);
> +	priv = (struct altera_hps2fpga_data *)of_id->data;
> +	WARN_ON(!priv);

Why don't you just return here if you have a NULL pointer?

> +
> +	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, "init-val", &init_val)) {
> +		if (init_val > 1) {
> +			dev_warn(dev, "invalid init-val %u > 1\n", init_val);
> +		} else {
> +			dev_info(dev, "%s bridge\n",
> +				(init_val ? "enabling" : "disabling"));
> +
> +			ret = _alt_hps2fpga_enable_set(priv, init_val);
> +			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 altera_fpga_driver = {
> +	.remove = alt_fpga_bridge_remove,
> +	.driver = {
> +		.name	= "altera_hps2fpga_bridge",
> +		.of_match_table = of_match_ptr(altera_fpga_of_match),
> +	},
> +};
> +
> +static int __init alt_fpga_bridge_init(void)
> +{
> +	return platform_driver_probe(&altera_fpga_driver,
> +				     alt_fpga_bridge_probe);
> +}
> +
> +static void __exit alt_fpga_bridge_exit(void)
> +{
> +	platform_driver_unregister(&altera_fpga_driver);
> +}
> +
> +module_init(alt_fpga_bridge_init);
> +module_exit(alt_fpga_bridge_exit);
> +
> +MODULE_DESCRIPTION("Altera SoCFPGA HPS to FPGA Bridge");
> +MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> +MODULE_LICENSE("GPL v2");

Regards,
Steffen

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

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

[atull]

On Wed, 28 Oct 2015, Steffen Trumtrar wrote:

> On Tue, Oct 27, 2015 at 05:09:15PM -0500, atull@opensource.altera.com wrote:
> > 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 <dinguyen@opensource.altera.com>
> > Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Signed-off-by: Matthew Gerlach <mgerlach@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
> > ---
> >  drivers/fpga/Kconfig             |    7 ++
> >  drivers/fpga/Makefile            |    1 +
> >  drivers/fpga/altera-fpga2sdram.c |  185 ++++++++++++++++++++++++++++++
> >  drivers/fpga/altera-hps2fpga.c   |  234 ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 427 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 143072b..004b83a 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -39,6 +39,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 9302662..cc01db3 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.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_SIMPLE_FPGA_BUS)		+= simple-fpga-bus.o
> > diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
> > new file mode 100644
> > index 0000000..ee56903
> > --- /dev/null
> > +++ b/drivers/fpga/altera-fpga2sdram.c
> > @@ -0,0 +1,185 @@
> > +/*
> > + * 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;
> > +	uint32_t *prop_value;
> > +	uint32_t prop_max;
> > +};
> > +
> > +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;
> > +	uint32_t init_val;
> > +	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);
> 
> Hm, what does this do? All I can get from the documentation is, that this
> is a general purpose register and has nothing to do with this particular bridge.
> 

It's described at the top of the file.  This bridge has to be configured before
the kernel boots, and this is a way of passing that configuration.

> > +
> > +	if (!of_property_read_u32(dev->of_node, "init-val", &init_val)) {
> > +		if (init_val > 1) {
> > +			dev_warn(dev, "invalid init-val %u > 1\n", init_val);
> > +		} else {
> > +			dev_info(dev, "%s bridge\n",
> > +				(init_val ? "enabling" : "disabling"));
> > +			ret = _alt_fpga2sdram_enable_set(priv, init_val);
> > +			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 = {
> > +	.remove = alt_fpga_bridge_remove,
> > +	.driver = {
> > +		.name	= "altera_fpga2sdram_bridge",
> > +		.of_match_table = of_match_ptr(altera_fpga_of_match),
> > +	},
> > +};
> > +
> > +static int __init alt_fpga_bridge_init(void)
> > +{
> > +	return platform_driver_probe(&altera_fpga_driver,
> > +				     alt_fpga_bridge_probe);
> > +}
> > +
> > +static void __exit alt_fpga_bridge_exit(void)
> > +{
> > +	platform_driver_unregister(&altera_fpga_driver);
> > +}
> > +
> > +module_init(alt_fpga_bridge_init);
> > +module_exit(alt_fpga_bridge_exit);
> > +
> > +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..3ec6394
> > --- /dev/null
> > +++ b/drivers/fpga/altera-hps2fpga.c
> > @@ -0,0 +1,234 @@
> > +/*
> > + * 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 SOCFPGA_RSTMGR_BRGMODRST		0x1c
> > +#define ALT_RSTMGR_BRGMODRST_H2F_MSK		0x00000001
> > +#define ALT_RSTMGR_BRGMODRST_LWH2F_MSK		0x00000002
> > +#define ALT_RSTMGR_BRGMODRST_F2H_MSK		0x00000004
> > +
> > +#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 {
> This name and the alt_hps2fpga_xxx functions are confusing, as you have
> one bridge of this name but use it for all the bridges.

I think it's OK.

> 
> > +	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 reset_mask;
> > +	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;
> 
> Why do you remap the On-chip RAM here. This doesn't belong in this
> driver.

We have to when we enable or disable.

> 
> > +
> > +		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);
> 
> regmap_update_bits? Otherwise you will overwrite the other bridges
> settings.

This works.  This is a write-only register.

> 
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
> > +{
> > +	return _alt_hps2fpga_enable_set(bridge->priv, enable);
> > +}
> > +
> > +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,
> > +	.reset_mask = ALT_RSTMGR_BRGMODRST_H2F_MSK,
> > +	.remap_mask = ALT_L3_REMAP_H2F_MSK,
> > +};
> > +
> > +static struct altera_hps2fpga_data lwhps2fpga_data  = {
> > +	.name = LWHPS2FPGA_BRIDGE_NAME,
> > +	.reset_mask = ALT_RSTMGR_BRGMODRST_LWH2F_MSK,
> > +	.remap_mask = ALT_L3_REMAP_LWH2F_MSK,
> > +};
> > +
> > +static struct altera_hps2fpga_data fpga2hps_data  = {
> > +	.name = FPGA2HPS_BRIDGE_NAME,
> > +	.reset_mask = ALT_RSTMGR_BRGMODRST_F2H_MSK,
> > +};
> > +
> > +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;
> > +	uint32_t init_val;
> > +	int ret;
> > +
> > +	of_id = of_match_device(altera_fpga_of_match, dev);
> > +	priv = (struct altera_hps2fpga_data *)of_id->data;
> > +	WARN_ON(!priv);
> 
> Why don't you just return here if you have a NULL pointer?

Actually we don't need that WARN_ON.  I can take it out.

> 
> > +
> > +	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, "init-val", &init_val)) {
> > +		if (init_val > 1) {
> > +			dev_warn(dev, "invalid init-val %u > 1\n", init_val);
> > +		} else {
> > +			dev_info(dev, "%s bridge\n",
> > +				(init_val ? "enabling" : "disabling"));
> > +
> > +			ret = _alt_hps2fpga_enable_set(priv, init_val);
> > +			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 altera_fpga_driver = {
> > +	.remove = alt_fpga_bridge_remove,
> > +	.driver = {
> > +		.name	= "altera_hps2fpga_bridge",
> > +		.of_match_table = of_match_ptr(altera_fpga_of_match),
> > +	},
> > +};
> > +
> > +static int __init alt_fpga_bridge_init(void)
> > +{
> > +	return platform_driver_probe(&altera_fpga_driver,
> > +				     alt_fpga_bridge_probe);
> > +}
> > +
> > +static void __exit alt_fpga_bridge_exit(void)
> > +{
> > +	platform_driver_unregister(&altera_fpga_driver);
> > +}
> > +
> > +module_init(alt_fpga_bridge_init);
> > +module_exit(alt_fpga_bridge_exit);
> > +
> > +MODULE_DESCRIPTION("Altera SoCFPGA HPS to FPGA Bridge");
> > +MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> > +MODULE_LICENSE("GPL v2");
> 
> Regards,
> Steffen
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>