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

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

[Joshua Clayton]

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 <stillcompiling@gmail.com>
---
 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
 
 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 <stillcompiling@gmail.com>
+ *
+ * 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 <stillcompiling@gmail.com>");
+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)
-- 
2.7.4

[PATCH 2/3] doc: dt: add cyclone-spi binding document

[Joshua Clayton]

Describe a cyclonespi devicetree entry, required features

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 .../bindings/fpga/cyclone-spi-fpga-mgr.txt         | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt

diff --git a/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
new file mode 100644
index 0000000..8de34db
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
@@ -0,0 +1,23 @@
+Altera SOCFPGA FPGA Manager
+
+Altera cyclone FPGAs support a method of loading the bitstream over what is
+referred to as "passive serial".
+The passive serial link is not technically spi, and might require extra
+circuits in order to play nicely with other spi slaves on the same bus.
+
+See https://www.altera.com/literature/hb/cyc/cyc_c51013.pdf
+
+Required properties:
+- compatible  : should contain "altr,cyclonespi-fpga-mgr"
+- reg         : spi slave id of the fpga
+- reset-gpio  : reset pin (referred to as nCONFIG in the cyclone manual)
+- status-gpio : status pin (referred to as nSTATUS in the cyclone manual)
+
+Example:
+	fpga_spi: evi-fpga-spi at 0 {
+		compatible = "altr,cyclonespi-fpga-mgr";
+		spi-max-frequency = <20000000>;
+		reg = <0>;
+		reset-gpio = <&gpio4 9 GPIO_ACTIVE_HIGH>;
+		status-gpio = <&gpio4 11 GPIO_ACTIVE_HIGH>;
+	};
-- 
2.7.4

[PATCH 2/3] doc: dt: add cyclone-spi binding document

[Moritz Fischer]

Hi Joshua,

On Thu, Oct 6, 2016 at 1:34 PM, Joshua Clayton <stillcompiling@gmail.com> wrote:
> Describe a cyclonespi devicetree entry, required features
>
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> ---
>  .../bindings/fpga/cyclone-spi-fpga-mgr.txt         | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
>
> diff --git a/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
> new file mode 100644
> index 0000000..8de34db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
> @@ -0,0 +1,23 @@
> +Altera SOCFPGA FPGA Manager

Copy & Paste? :)

> +Altera cyclone FPGAs support a method of loading the bitstream over what is

cyclone->Cyclone

> +referred to as "passive serial".
> +The passive serial link is not technically spi, and might require extra
> +circuits in order to play nicely with other spi slaves on the same bus.
> +
> +See https://www.altera.com/literature/hb/cyc/cyc_c51013.pdf
> +
> +Required properties:
> +- compatible  : should contain "altr,cyclonespi-fpga-mgr"

Alan, do you guys have any input on the compat string?

I think generally the bindings should go before the actual usage in
your patch series. Meaning you wanna document the binding
before you use it. I think this patch should be [1/3].

Cheers,

Moritz

[PATCH 2/3] doc: dt: add cyclone-spi binding document

[Joshua Clayton]

Moritz,

thank you very much for the review.
 
On 10/06/2016 07:53 PM, Moritz Fischer wrote:
> Hi Joshua,
>
> On Thu, Oct 6, 2016 at 1:34 PM, Joshua Clayton <stillcompiling@gmail.com> wrote:
>> Describe a cyclonespi devicetree entry, required features
>>
>> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
>> ---
>>  .../bindings/fpga/cyclone-spi-fpga-mgr.txt         | 23 ++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
>> new file mode 100644
>> index 0000000..8de34db
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
>> @@ -0,0 +1,23 @@
>> +Altera SOCFPGA FPGA Manager
> Copy & Paste? :)
Oops :(
As you might image, documentation was the last item done with the least attention.
Will fix.
>
>> +Altera cyclone FPGAs support a method of loading the bitstream over what is
> cyclone->Cyclone
OK.
>> +referred to as "passive serial".
>> +The passive serial link is not technically spi, and might require extra
>> +circuits in order to play nicely with other spi slaves on the same bus.
>> +
>> +See https://www.altera.com/literature/hb/cyc/cyc_c51013.pdf
>> +
>> +Required properties:
>> +- compatible  : should contain "altr,cyclonespi-fpga-mgr"
> Alan, do you guys have any input on the compat string?
I am open to change if it makes sense. I tried to keep the format similar.
> I think generally the bindings should go before the actual usage in
> your patch series. Meaning you wanna document the binding
> before you use it. I think this patch should be [1/3].
Ah, In my mind I had it backwards.
> Cheers,
>
> Moritz
I'll give Alan a chance to review and then spin a V2

Joshua Clayton

[PATCH 2/3] doc: dt: add cyclone-spi binding document

[atull]

On Fri, 7 Oct 2016, Moritz Fischer wrote:

> > +referred to as "passive serial".
> > +The passive serial link is not technically spi, and might require extra
> > +circuits in order to play nicely with other spi slaves on the same bus.
> > +
> > +See https://www.altera.com/literature/hb/cyc/cyc_c51013.pdf
> > +
> > +Required properties:
> > +- compatible  : should contain "altr,cyclonespi-fpga-mgr"
> 
> Alan, do you guys have any input on the compat string?

We want to make it clear that this is for fpga configuration
using a specific programming method (passive serial).

How about altr,cyclone-ps-spi ?

> 
> I think generally the bindings should go before the actual usage in
> your patch series. Meaning you wanna document the binding
> before you use it. I think this patch should be [1/3].

I agree.

Thanks,
Alan

> 
> Cheers,
> 
> Moritz
> 

[PATCH 3/3] ARM: dts: imx6q-evi: support cyclonespi

[Joshua Clayton]

Add support for Altera cyclone V FPGA connected to an spi port

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 arch/arm/boot/dts/imx6q-evi.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-evi.dts b/arch/arm/boot/dts/imx6q-evi.dts
index 6de21ff..7e5d3cf 100644
--- a/arch/arm/boot/dts/imx6q-evi.dts
+++ b/arch/arm/boot/dts/imx6q-evi.dts
@@ -95,6 +95,15 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_ecspi1 &pinctrl_ecspi1cs>;
 	status = "okay";
+
+	fpga_spi: cyclonespi at 0 {
+		compatible = "altr,cyclonespi-fpga-mgr";
+		spi-max-frequency = <20000000>;
+		reg = <0>;
+		pinctrl-0 = <&pinctrl_fpgaspi>;
+		reset-gpio = <&gpio4 9 GPIO_ACTIVE_HIGH>;
+		status-gpio = <&gpio4 11 GPIO_ACTIVE_HIGH>;
+	};
 };
 
 &ecspi3 {
@@ -325,6 +334,13 @@
 		>;
 	};
 
+	pinctrl_fpgaspi: fpgaspigrp {
+		fsl,pins = <
+			MX6QDL_PAD_KEY_ROW1__GPIO4_IO09 0x1b0b0
+			MX6QDL_PAD_KEY_ROW2__GPIO4_IO11 0x1b0b0
+		>;
+	};
+
 	pinctrl_gpminand: gpminandgrp {
 		fsl,pins = <
 			MX6QDL_PAD_NANDF_CLE__NAND_CLE 0xb0b1
-- 
2.7.4

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

[Moritz Fischer]

Hi Joshua,

couple of nits inline below:

On Thu, Oct 6, 2016 at 1:34 PM, Joshua Clayton <stillcompiling@gmail.com> 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 <stillcompiling@gmail.com>
> + *
> + * 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

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

[atull]

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

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

[Moritz Fischer]

Hi Alan,

On Fri, Oct 7, 2016 at 11:21 AM, atull <atull@opensource.altera.com> 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/linux-kernel at vger.kernel.org/msg995245.html

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

[atull]

On Fri, 7 Oct 2016, Moritz Fischer wrote:

> Hi Alan,
> 
> On Fri, Oct 7, 2016 at 11:21 AM, atull <atull@opensource.altera.com> 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/linux-kernel at vger.kernel.org/msg995245.html
> 

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

[Joshua Clayton]

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

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

[atull]

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 <stillcompiling@gmail.com>
> ---
>  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 <stillcompiling@gmail.com>
> + *
> + * 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 <stillcompiling@gmail.com>");
> +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
> 
>