Skip to contentSkip to conversation

fpga manager: Add cyclonespi driver for Altera fpgas

PATCH In review Joshua Clayton 3 participants 12 messages Oct 6, 2016 – Oct 10, 2016 1 message not in the archive

Subsystems (3)
Device tree bindings Arm i.MX and Layerscape FPGA manager framework
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Joshua Clayton

author
#1 lore raw

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

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 <[email protected]>
---
 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
drivers/fpga/Kconfig +6 -0
index cd84934..ccad5b1 100644
@@ -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
drivers/fpga/Makefile +1 -0
index 8d83fc6..c03f40de 100644
@@ -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
drivers/fpga/cyclonespi.c new file +173 -0

173 lines not on this page · raw message

Signature · 1 line
2.7.4
author
#2 lore raw

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

Describe a cyclonespi devicetree entry, required features

Signed-off-by Joshua Clayton <[email protected]>
---
 .../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
Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt new file +23 -0
new file mode 100644
index 0000000..8de34db
@@ -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>;
+	};
Signature · 1 line
2.7.4
#3 lore raw

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

Hi Joshua,

Quoted 14 lines · Joshua Clayton

Describe a cyclonespi devicetree entry, required features

Signed-off-by Joshua Clayton <[email protected]>
---
 .../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
Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt new file +0 -0
new file mode 100644
index 0000000..8de34db

Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt line 1

Quoted 1 line
Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt new file +1 -0
@@ -0,0 +1,23 @@
+Altera SOCFPGA FPGA Manager

Copy & Paste? :)

Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt line 3

Quoted 1 line
Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt new file +1 -0
+Altera cyclone FPGAs support a method of loading the bitstream over what is

cyclone->Cyclone

Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt line 11

Quoted 8 lines
Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt new file +8 -0
+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

author
#4 lore raw

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

Moritz,

thank you very much for the review.

Quoted 11 lines · Moritz Fischer

Hi Joshua,

Quoted 8 lines · Joshua Clayton

Describe a cyclonespi devicetree entry, required features

Signed-off-by Joshua Clayton <[email protected]>
---
 .../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
Quoted 8 lines

Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt line 1

Quoted 7 lines
Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt new file +1 -0
new file mode 100644
index 0000000..8de34db
@@ -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.

Quoted 3 lines

Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt line 3

Quoted 2 lines
Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt new file +1 -0
+Altera cyclone FPGAs support a method of loading the bitstream over what is

cyclone->Cyclone

OK.

Quoted 9 lines

Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt line 11

Quoted 8 lines
Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt new file +8 -0
+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.

Quoted 3 lines

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.

Quoted 3 lines

Cheers,

Moritz

I'll give Alan a chance to review and then spin a V2

Joshua Clayton

#5 lore raw

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

In reply to Moritz Fischer
Quoted 10 lines · Moritz Fischer

Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt line 11

Quoted 8 lines
Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt new file +8 -0
+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 ?

Quoted 4 lines

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

Quoted 5 lines

Cheers,

Moritz

author
#6 lore raw

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

In reply to Joshua Clayton

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

Signed-off-by Joshua Clayton <[email protected]>
---
 arch/arm/boot/dts/imx6q-evi.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
arch/arm/boot/dts/imx6q-evi.dts +16 -0
index 6de21ff..7e5d3cf 100644
@@ -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
Signature · 1 line
2.7.4
#7 lore raw
In reply to Joshua Clayton

Hi Joshua,

couple of nits inline below:

Quoted 34 lines · Joshua Clayton
drivers/fpga/Kconfig +6 -0
index cd84934..ccad5b1 100644
@@ -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
drivers/fpga/Makefile +1 -0
index 8d83fc6..c03f40de 100644
@@ -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
drivers/fpga/cyclonespi.c new file +0 -0
new file mode 100644
index 0000000..1ffa67c

drivers/fpga/cyclonespi.c line 9

Quoted 9 lines
drivers/fpga/cyclonespi.c new file +9 -0
@@ -0,0 +1,173 @@
+/**
+ * Copyright (c) 2015 United Western Technologies, Corporation
+ *
+ * Joshua Clayton <[email protected]>
+ *
+ * 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.

Quoted 9 lines
drivers/fpga/cyclonespi.c new file +9 -0
+
+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)
+{

drivers/fpga/cyclonespi.c line 47

Quoted 10 lines
drivers/fpga/cyclonespi.c new file +10 -0
+       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.

drivers/fpga/cyclonespi.c line 58

Quoted 11 lines
drivers/fpga/cyclonespi.c new file +11 -0
+
+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?

drivers/fpga/cyclonespi.c line 62

Quoted 4 lines
drivers/fpga/cyclonespi.c new file +4 -0
+       }
+
+       gpiod_set_value(conf->reset, 0);
+       udelay(50);

Should that value either be configurable, or a named constant?

drivers/fpga/cyclonespi.c line 69

Quoted 1 line
drivers/fpga/cyclonespi.c new file +1 -0
+       msleep(1);

See above.

drivers/fpga/cyclonespi.c line 79

Quoted 5 lines
drivers/fpga/cyclonespi.c new file +5 -0
+       /* set buffer to lsb first */
+       while (fw32 < fw_end) {
+               *fw32 = revbit8x4(*fw32);
+               fw32++;
+       }

See above.

Quoted 64 lines
drivers/fpga/cyclonespi.c new file +64 -0
+
+       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);
+       }

drivers/fpga/cyclonespi.c line 153

Quoted 10 lines
drivers/fpga/cyclonespi.c new file +10 -0
+
+       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

#8 lore raw
Quoted 10 lines · Moritz Fischer

drivers/fpga/cyclonespi.c line 47

Quoted 7 lines
drivers/fpga/cyclonespi.c new file +7 -0
+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

#9 lore raw

Hi Alan,

Quoted 7 lines · atull

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

#10 lore raw
Quoted 20 lines · Moritz Fischer

Hi Alan,

Quoted 7 lines · atull

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.

Quoted 7 lines

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

Quoted 7 lines

Cheers,

Moritz

[1] https://mail-archive.com/linux-kernel at vger.kernel.org/msg995245.html

author
#11 lore raw
In reply to atull
Quoted 9 lines · atull
Quoted 7 lines · Moritz Fischer

drivers/fpga/cyclonespi.c line 47

Quoted 7 lines
drivers/fpga/cyclonespi.c new file +7 -0
+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.

Quoted 18 lines
Quoted 2 lines

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.

Quoted 4 lines

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

#12 lore raw
In reply to Joshua Clayton
Quoted 19 lines · 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 <[email protected]>
---
 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
drivers/fpga/Kconfig +0 -0
index cd84934..ccad5b1 100644

drivers/fpga/Kconfig config FPGA · line 13

Quoted 1 line
drivers/fpga/Kconfig +0 -0
@@ -13,6 +13,12 @@ config FPGA

Hi Joshua,

This looks really good. Mostly minor comments.

drivers/fpga/Kconfig config FPGA · line 16

Quoted 3 lines
drivers/fpga/Kconfig +1 -0
 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.

drivers/fpga/Kconfig config FPGA · line 17

Quoted 1 line
drivers/fpga/Kconfig +1 -0
+	tristate "Altera Cyclone V SPI"

"Altera Cyclone V Passive Serial over SPI" ?

Quoted 12 lines
drivers/fpga/Kconfig +4 -0
+	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
drivers/fpga/Makefile +0 -0
index 8d83fc6..c03f40de 100644

drivers/fpga/Makefile line 9

Quoted 4 lines
drivers/fpga/Makefile +1 -0
@@ -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.

Quoted 20 lines
drivers/fpga/Makefile +0 -0
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
drivers/fpga/cyclonespi.c new file +12 -0
new file mode 100644
index 0000000..1ffa67c
@@ -0,0 +1,173 @@
+/**
+ * Copyright (c) 2015 United Western Technologies, Corporation
+ *
+ * Joshua Clayton <[email protected]>
+ *
+ * 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.
+ *
+ */
+

drivers/fpga/cyclonespi.c line 22

Quoted 10 lines
drivers/fpga/cyclonespi.c new file +10 -0
+#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 <[email protected]>");
+MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi");

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

Quoted 6 lines
drivers/fpga/cyclonespi.c new file +6 -0
+
+struct cyclonespi_conf {
+	struct gpio_desc *reset;
+	struct gpio_desc *status;
+	struct spi_device *spi;
+};

drivers/fpga/cyclonespi.c line 38

Quoted 10 lines
drivers/fpga/cyclonespi.c new file +10 -0
+
+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.

Quoted 14 lines
drivers/fpga/cyclonespi.c new file +14 -0
+}
+
+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;

drivers/fpga/cyclonespi.c line 62

Quoted 10 lines
drivers/fpga/cyclonespi.c new file +10 -0
+	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.

drivers/fpga/cyclonespi.c line 69

Quoted 7 lines
drivers/fpga/cyclonespi.c new file +7 -0
+	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.

Quoted 6 lines
drivers/fpga/cyclonespi.c new file +6 -0
+	if (gpiod_get_value(conf->status) == 0) {
+		dev_err(&mgr->dev, "Status pin not ready.\n");
+		return -EIO;
+	}
+
+	/* set buffer to lsb first */

drivers/fpga/cyclonespi.c line 85

Quoted 10 lines
drivers/fpga/cyclonespi.c new file +10 -0
+	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"

Quoted 22 lines
drivers/fpga/cyclonespi.c new file +22 -0
+{
+	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;

drivers/fpga/cyclonespi.c line 117

Quoted 10 lines
drivers/fpga/cyclonespi.c new file +10 -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?

Quoted 60 lines
drivers/fpga/cyclonespi.c new file +56 -0
+
+	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)
Signature · 1 line
2.7.4