[1/3] fpga manager: Add cyclonespi driver for Altera fpgas

Commit Message

Joshua Clayton Oct. 6, 2016, 8:34 p.m. UTC

cyclonespi loads fpga firmware over spi, using the "passive serial"
interface on Altera Cyclone FPGAS.

one of the simpler ways to set up an fpga at runtime.
The signal interface is close to unidirectional spi with lsb first.

Signed-off-by: Joshua Clayton <[e-mail]>
---
 drivers/fpga/Kconfig      |   6 ++
 drivers/fpga/Makefile     |   1 +
 drivers/fpga/cyclonespi.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 180 insertions(+)
 create mode 100644 drivers/fpga/cyclonespi.c

Comments

Moritz Fischer Oct. 7, 2016, 2:48 a.m. UTC | #1

Hi Joshua,

couple of nits inline below:

On Thu, Oct 6, 2016 at 1:34 PM, Joshua Clayton <[e-mail]> wrote:

> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index cd84934..ccad5b1 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>
>  if FPGA
>
> +config FPGA_MGR_CYCLONE_SPI
> +       tristate "Altera Cyclone V SPI"
> +       depends on SPI
> +       help
> +         FPGA manager driver support for Altera Cyclone V over SPI
> +
>  config FPGA_MGR_SOCFPGA
>         tristate "Altera SOCFPGA FPGA Manager"
>         depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..c03f40de 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)                     += fpga-mgr.o
>
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_CYCLONE_SPI)     += cyclonespi.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)         += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)       += zynq-fpga.o
> diff --git a/drivers/fpga/cyclonespi.c b/drivers/fpga/cyclonespi.c
> new file mode 100644
> index 0000000..1ffa67c
> --- /dev/null
> +++ b/drivers/fpga/cyclonespi.c
> @@ -0,0 +1,173 @@
> +/**
> + * Copyright (c) 2015 United Western Technologies, Corporation
> + *
> + * Joshua Clayton <[e-mail]>
> + *
> + * Manage Altera fpga firmware that is loaded over spi.
> + * Firmware must be in binary "rbf" format.
> + * Works on Cyclone V. Should work on cyclone series.
> + * May work on other Altera fpgas.

I think at one point we decided it's gonna be 'FPGA' to be consistent.

> +
> +static const struct of_device_id of_ef_match[] = {
> +       { .compatible = "altr,cyclonespi-fpga-mgr", },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, of_ef_match);
> +
> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
> +{
> +       return mgr->state;
> +}
> +
> +static inline u32 revbit8x4(u32 n)
> +{
> +       n = ((n & 0xF0F0F0F0UL) >> 4) | ((n & 0x0F0F0F0FUL) << 4);
> +       n = ((n & 0xCCCCCCCCUL) >> 2) | ((n & 0x33333333UL) << 2);
> +       n = ((n & 0xAAAAAAAAUL) >> 1) | ((n & 0x55555555UL) << 1);
> +       return n;
> +}

During the Zynq FPGA manager reviews we decided that manipulating the bitstream
to be consumable by the driver is userland's job.

> +
> +static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
> +                               const char *buf, size_t count)
> +{
> +       struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> +       u32 *fw32 = (u32 *)buf;
> +       const u32 *fw_end = (u32 *)(buf + count);
> +
> +       if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> +               dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
> +               return -EINVAL;

-ENOTSUPP?
> +       }
> +
> +       gpiod_set_value(conf->reset, 0);
> +       udelay(50);

Should that value either be configurable, or a named constant?
> +       msleep(1);

See above.

> +       /* set buffer to lsb first */
> +       while (fw32 < fw_end) {
> +               *fw32 = revbit8x4(*fw32);
> +               fw32++;
> +       }

See above.
> +
> +       return 0;
> +}
> +
> +static int cyclonespi_write(struct fpga_manager *mgr, const char *buf,
> +                          size_t count)
> +{
> +       struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> +       const char *fw_data = buf;
> +       const char *fw_data_end = fw_data + count;
> +
> +       while (fw_data < fw_data_end) {
> +               int ret;
> +               int stride = fw_data_end - fw_data;
> +
> +               if (stride > SZ_4K)
> +                       stride = SZ_4K;
> +
> +               ret = spi_write(conf->spi, fw_data, stride);
> +               if (ret) {
> +                       dev_err(&mgr->dev, "spi error in firmware write: %d\n",
> +                                       ret);
> +                       return ret;
> +               }
> +               fw_data += stride;
> +       }
> +
> +       return 0;
> +}
> +
> +static int cyclonespi_write_complete(struct fpga_manager *mgr, u32 flags)
> +{
> +       struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> +
> +       if (gpiod_get_value(conf->status) == 0) {
> +               dev_err(&mgr->dev, "Error during configuration.\n");
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct fpga_manager_ops cyclonespi_ops = {
> +       .state = cyclonespi_state,
> +       .write_init = cyclonespi_write_init,
> +       .write = cyclonespi_write,
> +       .write_complete = cyclonespi_write_complete,
> +};
> +
> +static int cyclonespi_probe(struct spi_device *spi)
> +{
> +       struct cyclonespi_conf *conf = devm_kzalloc(&spi->dev, sizeof(*conf),
> +                                               GFP_KERNEL);
> +
> +       if (!conf)
> +               return -ENOMEM;
> +
> +       conf->spi = spi;
> +       conf->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> +       if (IS_ERR(conf->reset)) {
> +               dev_err(&spi->dev, "Failed to get reset gpio: %ld\n",
> +                       PTR_ERR(conf->reset));
> +               return PTR_ERR(conf->reset);
> +       }
> +
> +       conf->status = devm_gpiod_get(&spi->dev, "status", GPIOD_IN);
> +       if (IS_ERR(conf->status)) {
> +               dev_err(&spi->dev, "Failed to get status gpio: %ld\n",
> +                               PTR_ERR(conf->status));
> +               return PTR_ERR(conf->status);
> +       }
> +
> +       return fpga_mgr_register(&spi->dev, "Altera SPI FPGA Manager",
> +                                &cyclonespi_ops, conf);

Nit: Altera >Cyclone< SPI FPGA Manager

Thanks ... reminds me I wanted to submit my patch for the icoboard ;-)

Moritz

Alan Tull Oct. 7, 2016, 6:21 p.m. UTC | #2

On Fri, 7 Oct 2016, Moritz Fischer wrote:

> > +static inline u32 revbit8x4(u32 n)
> > +{
> > +       n = ((n & 0xF0F0F0F0UL) >> 4) | ((n & 0x0F0F0F0FUL) << 4);
> > +       n = ((n & 0xCCCCCCCCUL) >> 2) | ((n & 0x33333333UL) << 2);
> > +       n = ((n & 0xAAAAAAAAUL) >> 1) | ((n & 0x55555555UL) << 1);
> > +       return n;
> > +}
> 
> During the Zynq FPGA manager reviews we decided that manipulating the bitstream
> to be consumable by the driver is userland's job.

Moritz, Can you remind me what that issue was there (or point me to
that email, I can't find it)?  I don't think I had a problem with that
in your case.  In general I think if these drivers can take the
bitstream that comes from the manufacturer's tools and stuff it into
the FPGA, then we are accomplishing what we want.  So I am OK with
this here.  The intent of the driver is to load a standard rbf, same
as the other Altera FPGA drivers.

There is a problem here though it will be easy to fix.  This call to
revbit8x4 should happen in cyclonespi_write(), not in
cyclonespi_write_init(). The reason for that is that write_init() may
just get the first chunk of the image (the header) and that write()
will be called multiple times for the remaining chunks.  The current
FPGA manager API won't show this problem since you have to give
fpga_mgr_buf_load the whole image buffer at once.  But it is easy to
imagine that some time in the future we may want to expand the FPGA
manager API to support streaming where we don't have the whole buffer.

Thanks for submitting, Joshua.  Will be looking at this over the
next several days.

Alan

Moritz Fischer Oct. 7, 2016, 6:42 p.m. UTC | #3

Hi Alan,

On Fri, Oct 7, 2016 at 11:21 AM, atull <[e-mail]> wrote:

> Moritz, Can you remind me what that issue was there (or point me to
> that email, I can't find it)?  I don't think I had a problem with that
> in your case.  In general I think if these drivers can take the
> bitstream that comes from the manufacturer's tools and stuff it into
> the FPGA, then we are accomplishing what we want.  So I am OK with
> this here.  The intent of the driver is to load a standard rbf, same
> as the other Altera FPGA drivers.

My first version of the zynq fpga manager had byte swapping in there
and detection of which format was used. Greg even (accidentially)
merged my initial code
which I then cleaned up in 4d10eaff5bfc69997a769f9c83b749f0a8c542fa
("fpga: zynq-fpga: Change fw format to handle bin instead of bit.") to
address the review
comments.

I do see your point about useability, and if this is something that
keeps coming up
we could pull that into the framework with a flag to SWAP or as part
of the image_information
struct.

Thoughts?

Cheers,

Moritz

[1] https://mail-archive.com/[e-mail]/msg995245.html

Joshua Clayton Oct. 7, 2016, 8:54 p.m. UTC | #4

On 10/07/2016 11:21 AM, atull wrote:
> On Fri, 7 Oct 2016, Moritz Fischer wrote:
>
>>> +static inline u32 revbit8x4(u32 n)
>>> +{
>>> +       n = ((n & 0xF0F0F0F0UL) >> 4) | ((n & 0x0F0F0F0FUL) << 4);
>>> +       n = ((n & 0xCCCCCCCCUL) >> 2) | ((n & 0x33333333UL) << 2);
>>> +       n = ((n & 0xAAAAAAAAUL) >> 1) | ((n & 0x55555555UL) << 1);
>>> +       return n;
>>> +}
The real issue is that The FPGA wants lsb first, and my SPI driver
doesn't support it.
What I really wanted to do here was to get generic support for lsb-first
SPI into the SPI subsystem.
>> During the Zynq FPGA manager reviews we decided that manipulating the bitstream
>> to be consumable by the driver is userland's job.
> Moritz, Can you remind me what that issue was there (or point me to
> that email, I can't find it)?  I don't think I had a problem with that
> in your case.  In general I think if these drivers can take the
> bitstream that comes from the manufacturer's tools and stuff it into
> the FPGA, then we are accomplishing what we want.  So I am OK with
> this here.  The intent of the driver is to load a standard rbf, same
> as the other Altera FPGA drivers.
> There is a problem here though it will be easy to fix.  This call to
> revbit8x4 should happen in cyclonespi_write(), not in
> cyclonespi_write_init(). The reason for that is that write_init() may
> just get the first chunk of the image (the header) and that write()
> will be called multiple times for the remaining chunks.  The current
> FPGA manager API won't show this problem since you have to give
> fpga_mgr_buf_load the whole image buffer at once.  But it is easy to
> imagine that some time in the future we may want to expand the FPGA
> manager API to support streaming where we don't have the whole buffer.
OK.
If generic lsb first support for SPI is too high a bar (which it may be),
I will move the bit reversing code into the write function.
> Thanks for submitting, Joshua.  Will be looking at this over the
> next several days.
>
> Alan
Thanks for the quick response!
I'll be looking forward to your review,

Joshua Clayton

Alan Tull Oct. 10, 2016, 6:29 p.m. UTC | #5

On Thu, 6 Oct 2016, Joshua Clayton wrote:

> cyclonespi loads fpga firmware over spi, using the "passive serial"
> interface on Altera Cyclone FPGAS.
> 
> one of the simpler ways to set up an fpga at runtime.
> The signal interface is close to unidirectional spi with lsb first.
> 
> Signed-off-by: Joshua Clayton <[e-mail]>
> ---
>  drivers/fpga/Kconfig      |   6 ++
>  drivers/fpga/Makefile     |   1 +
>  drivers/fpga/cyclonespi.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 180 insertions(+)
>  create mode 100644 drivers/fpga/cyclonespi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index cd84934..ccad5b1 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>  

Hi Joshua,

This looks really good.  Mostly minor comments.

>  if FPGA
>  
> +config FPGA_MGR_CYCLONE_SPI

Suggestions here and below to keep PS in the name of things.
Such as FPGA_MGR_CYCLONE_PS_SPI.

> +	tristate "Altera Cyclone V SPI"

"Altera Cyclone V Passive Serial over SPI" ?

> +	depends on SPI
> +	help
> +	  FPGA manager driver support for Altera Cyclone V over SPI
> +
>  config FPGA_MGR_SOCFPGA
>  	tristate "Altera SOCFPGA FPGA Manager"
>  	depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..c03f40de 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)			+= fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_CYCLONE_SPI)	+= cyclonespi.o

cyclone-ps-spi.o and change name of c file.

>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
> diff --git a/drivers/fpga/cyclonespi.c b/drivers/fpga/cyclonespi.c
> new file mode 100644
> index 0000000..1ffa67c
> --- /dev/null
> +++ b/drivers/fpga/cyclonespi.c
> @@ -0,0 +1,173 @@
> +/**
> + * Copyright (c) 2015 United Western Technologies, Corporation
> + *
> + * Joshua Clayton <[e-mail]>
> + *
> + * Manage Altera fpga firmware that is loaded over spi.
> + * Firmware must be in binary "rbf" format.
> + * Works on Cyclone V. Should work on cyclone series.
> + * May work on other Altera fpgas.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>
> +#include <linux/spi/spi.h>
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Joshua Clayton <[e-mail]>");
> +MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi");

Please move these 3 MODULE_* lines to the bottom of this file.

> +
> +struct cyclonespi_conf {
> +	struct gpio_desc *reset;
> +	struct gpio_desc *status;
> +	struct spi_device *spi;
> +};
> +
> +static const struct of_device_id of_ef_match[] = {
> +	{ .compatible = "altr,cyclonespi-fpga-mgr", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, of_ef_match);
> +
> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
> +{
> +	return mgr->state;

fpga-mgr.c's fpga_mgr_register() reads the initial state
from this function.  Please return one of the values of enum
fpga_mgr_states here.  If state isn't known, then use
FPGA_MGR_STATE_UNKNOWN.

> +}
> +
> +static inline u32 revbit8x4(u32 n)
> +{
> +	n = ((n & 0xF0F0F0F0UL) >> 4) | ((n & 0x0F0F0F0FUL) << 4);
> +	n = ((n & 0xCCCCCCCCUL) >> 2) | ((n & 0x33333333UL) << 2);
> +	n = ((n & 0xAAAAAAAAUL) >> 1) | ((n & 0x55555555UL) << 1);
> +	return n;
> +}
> +
> +static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
> +				const char *buf, size_t count)
> +{
> +	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> +	u32 *fw32 = (u32 *)buf;
> +	const u32 *fw_end = (u32 *)(buf + count);
> +
> +	if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> +		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
> +		return -EINVAL;
> +	}
> +
> +	gpiod_set_value(conf->reset, 0);
> +	udelay(50);

Moritz brought up whether the delay/sleep values should be
configurable.  It looks like these are set values and have
probably been increased for good measure.  These could be
macros named to contain symbol names in the datasheet's PS
timing waveform, if that is where these are coming from.

The timing diagram has the Tcf2st0 as 800nSec max and
Tcf2st1 (below) as 40uSec.  I guess either we are looking at
different specs or you just found that you got better
results if you padded the values out?

I would normally suggest a convention of macro names
that maps neatly to the value names in the datasheet
such as CYCLONE_PS_TCF2ST0_USEC.  The problem is
the units.


> +	if (gpiod_get_value(conf->status) == 1) {
> +		dev_err(&mgr->dev, "Status pin should be low.\n");
> +		return -EIO;
> +	}
> +
> +	gpiod_set_value(conf->reset, 1);
> +	msleep(1);

CYCLONE_PS_TCF2ST1_USEC?  I'm not sure what to suggest
because the units in the timing diagram I'm looking at are
nanoseconds but this ended up being a mSec.

> +	if (gpiod_get_value(conf->status) == 0) {
> +		dev_err(&mgr->dev, "Status pin not ready.\n");
> +		return -EIO;
> +	}
> +
> +	/* set buffer to lsb first */
> +	while (fw32 < fw_end) {
> +		*fw32 = revbit8x4(*fw32);
> +		fw32++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cyclonespi_write(struct fpga_manager *mgr, const char *buf,
> +			   size_t count)

Please fix checkpatch.pl warnings.  Here and a few other places it
says "CHECK: Alignment should match open parenthesis"

> +{
> +	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> +	const char *fw_data = buf;
> +	const char *fw_data_end = fw_data + count;
> +
> +	while (fw_data < fw_data_end) {
> +		int ret;
> +		int stride = fw_data_end - fw_data;
> +
> +		if (stride > SZ_4K)
> +			stride = SZ_4K;
> +
> +		ret = spi_write(conf->spi, fw_data, stride);
> +		if (ret) {
> +			dev_err(&mgr->dev, "spi error in firmware write: %d\n",
> +					ret);
> +			return ret;
> +		}
> +		fw_data += stride;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cyclonespi_write_complete(struct fpga_manager *mgr, u32 flags)
> +{
> +	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> +
> +	if (gpiod_get_value(conf->status) == 0) {
> +		dev_err(&mgr->dev, "Error during configuration.\n");
> +		return -EIO;
> +	}

Does your hardware give you access to the CONF_DONE pin?
If so, can you get better status info from that?

> +
> +	return 0;
> +}
> +
> +static const struct fpga_manager_ops cyclonespi_ops = {
> +	.state = cyclonespi_state,
> +	.write_init = cyclonespi_write_init,
> +	.write = cyclonespi_write,
> +	.write_complete = cyclonespi_write_complete,
> +};
> +
> +static int cyclonespi_probe(struct spi_device *spi)
> +{
> +	struct cyclonespi_conf *conf = devm_kzalloc(&spi->dev, sizeof(*conf),
> +						GFP_KERNEL);
> +
> +	if (!conf)
> +		return -ENOMEM;
> +
> +	conf->spi = spi;
> +	conf->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(conf->reset)) {
> +		dev_err(&spi->dev, "Failed to get reset gpio: %ld\n",
> +			PTR_ERR(conf->reset));
> +		return PTR_ERR(conf->reset);
> +	}
> +
> +	conf->status = devm_gpiod_get(&spi->dev, "status", GPIOD_IN);
> +	if (IS_ERR(conf->status)) {
> +		dev_err(&spi->dev, "Failed to get status gpio: %ld\n",
> +				PTR_ERR(conf->status));
> +		return PTR_ERR(conf->status);
> +	}
> +
> +	return fpga_mgr_register(&spi->dev, "Altera SPI FPGA Manager",
> +				 &cyclonespi_ops, conf);
> +}
> +
> +static int cyclonespi_remove(struct spi_device *spi)
> +{
> +	fpga_mgr_unregister(&spi->dev);
> +
> +	return 0;
> +}
> +
> +static struct spi_driver cyclonespi_driver = {
> +	.driver = {
> +		.name   = "cyclonespi",
> +		.owner  = THIS_MODULE,
> +		.of_match_table = of_match_ptr(of_ef_match),
> +	},
> +	.probe  = cyclonespi_probe,
> +	.remove = cyclonespi_remove,
> +};
> +
> +module_spi_driver(cyclonespi_driver)
> -- 
> 2.7.4
> 
>

Alan Tull Oct. 10, 2016, 6:38 p.m. UTC | #6

On Fri, 7 Oct 2016, Moritz Fischer wrote:

> Hi Alan,
> 
> On Fri, Oct 7, 2016 at 11:21 AM, atull <[e-mail]> wrote:
> 
> > Moritz, Can you remind me what that issue was there (or point me to
> > that email, I can't find it)?  I don't think I had a problem with that
> > in your case.  In general I think if these drivers can take the
> > bitstream that comes from the manufacturer's tools and stuff it into
> > the FPGA, then we are accomplishing what we want.  So I am OK with
> > this here.  The intent of the driver is to load a standard rbf, same
> > as the other Altera FPGA drivers.
> 
> My first version of the zynq fpga manager had byte swapping in there
> and detection of which format was used. Greg even (accidentially)
> merged my initial code
> which I then cleaned up in 4d10eaff5bfc69997a769f9c83b749f0a8c542fa
> ("fpga: zynq-fpga: Change fw format to handle bin instead of bit.") to
> address the review
> comments.
> 
> I do see your point about useability, and if this is something that
> keeps coming up
> we could pull that into the framework with a flag to SWAP or as part
> of the image_information
> struct.
> 
> Thoughts?

I agree.  Some day we may want to see it useful to extend
the flow to do that.  Currently it's not needed so we can
wait until we see clearly what is needed.

Alan

> 
> Cheers,
> 
> Moritz
> 
> [1] https://mail-archive.com/[e-mail]/msg995245.html
>

Patch

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index cd84934..ccad5b1 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -13,6 +13,12 @@  config FPGA
 
 if FPGA
 
+config FPGA_MGR_CYCLONE_SPI
+	tristate "Altera Cyclone V SPI"
+	depends on SPI
+	help
+	  FPGA manager driver support for Altera Cyclone V over SPI
+
 config FPGA_MGR_SOCFPGA
 	tristate "Altera SOCFPGA FPGA Manager"
 	depends on ARCH_SOCFPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 8d83fc6..c03f40de 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -6,5 +6,6 @@ 
 obj-$(CONFIG_FPGA)			+= fpga-mgr.o
 
 # FPGA Manager Drivers
+obj-$(CONFIG_FPGA_MGR_CYCLONE_SPI)	+= cyclonespi.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
diff --git a/drivers/fpga/cyclonespi.c b/drivers/fpga/cyclonespi.c
new file mode 100644
index 0000000..1ffa67c
--- /dev/null
+++ b/drivers/fpga/cyclonespi.c
@@ -0,0 +1,173 @@ 
+/**
+ * Copyright (c) 2015 United Western Technologies, Corporation
+ *
+ * Joshua Clayton <[e-mail]>
+ *
+ * Manage Altera fpga firmware that is loaded over spi.
+ * Firmware must be in binary "rbf" format.
+ * Works on Cyclone V. Should work on cyclone series.
+ * May work on other Altera fpgas.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/spi/spi.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joshua Clayton <[e-mail]>");
+MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi");
+
+struct cyclonespi_conf {
+	struct gpio_desc *reset;
+	struct gpio_desc *status;
+	struct spi_device *spi;
+};
+
+static const struct of_device_id of_ef_match[] = {
+	{ .compatible = "altr,cyclonespi-fpga-mgr", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_ef_match);
+
+static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
+{
+	return mgr->state;
+}
+
+static inline u32 revbit8x4(u32 n)
+{
+	n = ((n & 0xF0F0F0F0UL) >> 4) | ((n & 0x0F0F0F0FUL) << 4);
+	n = ((n & 0xCCCCCCCCUL) >> 2) | ((n & 0x33333333UL) << 2);
+	n = ((n & 0xAAAAAAAAUL) >> 1) | ((n & 0x55555555UL) << 1);
+	return n;
+}
+
+static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
+				const char *buf, size_t count)
+{
+	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+	u32 *fw32 = (u32 *)buf;
+	const u32 *fw_end = (u32 *)(buf + count);
+
+	if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
+		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
+		return -EINVAL;
+	}
+
+	gpiod_set_value(conf->reset, 0);
+	udelay(50);
+	if (gpiod_get_value(conf->status) == 1) {
+		dev_err(&mgr->dev, "Status pin should be low.\n");
+		return -EIO;
+	}
+
+	gpiod_set_value(conf->reset, 1);
+	msleep(1);
+	if (gpiod_get_value(conf->status) == 0) {
+		dev_err(&mgr->dev, "Status pin not ready.\n");
+		return -EIO;
+	}
+
+	/* set buffer to lsb first */
+	while (fw32 < fw_end) {
+		*fw32 = revbit8x4(*fw32);
+		fw32++;
+	}
+
+	return 0;
+}
+
+static int cyclonespi_write(struct fpga_manager *mgr, const char *buf,
+			   size_t count)
+{
+	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+	const char *fw_data = buf;
+	const char *fw_data_end = fw_data + count;
+
+	while (fw_data < fw_data_end) {
+		int ret;
+		int stride = fw_data_end - fw_data;
+
+		if (stride > SZ_4K)
+			stride = SZ_4K;
+
+		ret = spi_write(conf->spi, fw_data, stride);
+		if (ret) {
+			dev_err(&mgr->dev, "spi error in firmware write: %d\n",
+					ret);
+			return ret;
+		}
+		fw_data += stride;
+	}
+
+	return 0;
+}
+
+static int cyclonespi_write_complete(struct fpga_manager *mgr, u32 flags)
+{
+	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+
+	if (gpiod_get_value(conf->status) == 0) {
+		dev_err(&mgr->dev, "Error during configuration.\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static const struct fpga_manager_ops cyclonespi_ops = {
+	.state = cyclonespi_state,
+	.write_init = cyclonespi_write_init,
+	.write = cyclonespi_write,
+	.write_complete = cyclonespi_write_complete,
+};
+
+static int cyclonespi_probe(struct spi_device *spi)
+{
+	struct cyclonespi_conf *conf = devm_kzalloc(&spi->dev, sizeof(*conf),
+						GFP_KERNEL);
+
+	if (!conf)
+		return -ENOMEM;
+
+	conf->spi = spi;
+	conf->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(conf->reset)) {
+		dev_err(&spi->dev, "Failed to get reset gpio: %ld\n",
+			PTR_ERR(conf->reset));
+		return PTR_ERR(conf->reset);
+	}
+
+	conf->status = devm_gpiod_get(&spi->dev, "status", GPIOD_IN);
+	if (IS_ERR(conf->status)) {
+		dev_err(&spi->dev, "Failed to get status gpio: %ld\n",
+				PTR_ERR(conf->status));
+		return PTR_ERR(conf->status);
+	}
+
+	return fpga_mgr_register(&spi->dev, "Altera SPI FPGA Manager",
+				 &cyclonespi_ops, conf);
+}
+
+static int cyclonespi_remove(struct spi_device *spi)
+{
+	fpga_mgr_unregister(&spi->dev);
+
+	return 0;
+}
+
+static struct spi_driver cyclonespi_driver = {
+	.driver = {
+		.name   = "cyclonespi",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_match_ptr(of_ef_match),
+	},
+	.probe  = cyclonespi_probe,
+	.remove = cyclonespi_remove,
+};
+
+module_spi_driver(cyclonespi_driver)