[PATCH v6 0/5] Altera Cyclone Passive Serial SPI FPGA Manager

[PATCH v6 0/5] Altera Cyclone Passive Serial SPI FPGA Manager

[Joshua Clayton]

This series adds an FPGA manager for Altera cyclone FPGAs
that can program them using an spi port and a couple of gpios, using
Alteras passive serial protocol.

Need ACKs from ARCH maintainers for ARCH specific implementations of
__arch_bitrev8x4(), and I've added more ARCHes, so will need more ACKS, 
but the generic code will work without that patch, so if there is a holdup
the rest of the series can go in without patch 2

Changes from v5:
- Rebased on next-20161214xi
- Corrected for FPGA Mgr API change in  write_init() and write_complete()
- Better describe the device cyclone-ps-spi runs on in the file header.
- Split the bitrev8x4 patch into generic and arch specific patches...
- Added AARCH64 and MIPS implementations of bitrev8x4()... they all have to 
  have an implementation for it to compile cleanly across platforms
- Added the changes to imx6q-evi.dts to the patch set.

Changes from v4:
- Added the needed return statement to __arch_bitrev8x4()
- Added Rob Herrings ACK for and fix a typo in the commit log of patch 2

Changes from v3:
- Fixed up the state() function to return the state of the status pin
  reqested by Alan Tull
- Switched the pin to ACTIVE_LOW and coresponding logic level, and updated
  the corresponding documentation. Thanks Rob Herring for pointing out my
  mistake.
- Per Rob Herring, switched from "gpio" to "gpios" in dts

Changes from v2:
- Merged patch 3 and 4 as suggested in review by Moritz Fischer
- Changed FPGA_MIN_DELAY from 250 to 50 ms is the time advertized by
  Altera. This now works, as we don't assume it is done

Changes from v1:
- Changed the name from cyclone-spi-fpga-mgr to cyclone-ps-spi-fpga-mgr
  This name change was requested by Alan Tull, to be specific about which
  programming method is being employed on the fpga.
- Changed the name of the reset-gpio to config-gpio to closer match the
  way the pins are described in the Altera manual
- Moved MODULE_LICENCE, _AUTHOR, and _DESCRIPTION to the bottom

- Added a bitrev8x4() function to the bitrev headers and implemented ARM
 const, runtime, and ARM specific faster versions (This may end up
 needing to be a standalone patch)

- Moved the bitswapping into cyclonespi_write(), as requested.
  This falls short of my desired generic lsb first spi support, but is a step
  in that direction.

- Fixed whitespace problems introduced during refactoring

- Replaced magic number for initial delay with a descriptive macro
- Poll the fpga to see when it is ready rather than a fixed 1 ms sleep

Joshua Clayton (5):
  lib: add bitrev8x4()
  lib: implement __arch_bitrev8x4()
  doc: dt: add cyclone-ps-spi binding document
  fpga manager: Add cyclone-ps-spi driver for Altera FPGAs
  ARM: dts: imx6q-evi: support cyclone-ps-spi

 .../bindings/fpga/cyclone-ps-spi-fpga-mgr.txt      |  25 +++
 arch/arm/boot/dts/imx6q-evi.dts                    |  16 ++
 arch/arm/include/asm/bitrev.h                      |   6 +
 arch/arm64/include/asm/bitrev.h                    |   6 +
 arch/mips/include/asm/bitrev.h                     |   6 +
 drivers/fpga/Kconfig                               |   7 +
 drivers/fpga/Makefile                              |   1 +
 drivers/fpga/cyclone-ps-spi.c                      | 186 +++++++++++++++++++++
 include/linux/bitrev.h                             |  26 +++
 9 files changed, 279 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
 create mode 100644 drivers/fpga/cyclone-ps-spi.c

-- 
2.9.3

[PATCH v6 1/5] lib: add bitrev8x4()

[Joshua Clayton]

Add a function to reverse bytes within a 32 bit word.
Operate on a u32 rather than individual bytes.
ARCH specific versions require substantially fewer instructions than
working a byte at a time.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 include/linux/bitrev.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/linux/bitrev.h b/include/linux/bitrev.h
index fb790b8..868dcb6 100644
--- a/include/linux/bitrev.h
+++ b/include/linux/bitrev.h
@@ -27,6 +27,14 @@ static inline u32 __bitrev32(u32 x)
 	return (__bitrev16(x & 0xffff) << 16) | __bitrev16(x >> 16);
 }
 
+static inline u32 __bitrev8x4(u32 x)
+{
+	return(__bitrev8(x & 0xff) |
+	       (__bitrev8((x >> 8)  & 0xff) << 8) |
+	       (__bitrev8((x >> 16)  & 0xff) << 16) |
+	       (__bitrev8((x >> 24)  & 0xff) << 24));
+}
+
 #endif /* CONFIG_HAVE_ARCH_BITREVERSE */
 
 #define __constant_bitrev32(x)	\
@@ -50,6 +58,15 @@ static inline u32 __bitrev32(u32 x)
 	__x;								\
 })
 
+#define __constant_bitrev8x4(x) \
+({			\
+	u32 __x = x;	\
+	__x = ((__x & (u32)0xF0F0F0F0UL) >> 4) | ((__x & (u32)0x0F0F0F0FUL) << 4);	\
+	__x = ((__x & (u32)0xCCCCCCCCUL) >> 2) | ((__x & (u32)0x33333333UL) << 2);	\
+	__x = ((__x & (u32)0xAAAAAAAAUL) >> 1) | ((__x & (u32)0x55555555UL) << 1);	\
+	__x;								\
+})
+
 #define __constant_bitrev8(x)	\
 ({					\
 	u8 __x = x;			\
@@ -75,6 +92,14 @@ static inline u32 __bitrev32(u32 x)
 	__bitrev16(__x);				\
  })
 
+#define bitrev8x4(x) \
+({			\
+	u32 __x = x;	\
+	__builtin_constant_p(__x) ?	\
+	__constant_bitrev8x4(__x) :			\
+	__bitrev8x4(__x);				\
+})
+
 #define bitrev8(x) \
 ({			\
 	u8 __x = x;	\
-- 
2.9.3

[PATCH v6 2/5] lib: implement __arch_bitrev8x4()

[Joshua Clayton]

Implement faster bitrev8x4() for arm, arm64 and mips, all the platforms
with CONFIG_HAVE_ARCH_BITREVERSE.
ARM platforms just need a byteswap added to the existing __arch_bitrev32()
Amusingly, the mips implementation is exactly the opposite, requiring
removal of the byteswap from its __arch_bitrev32()

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 arch/arm/include/asm/bitrev.h   | 6 ++++++
 arch/arm64/include/asm/bitrev.h | 6 ++++++
 arch/mips/include/asm/bitrev.h  | 6 ++++++
 include/linux/bitrev.h          | 1 +
 4 files changed, 19 insertions(+)

diff --git a/arch/arm/include/asm/bitrev.h b/arch/arm/include/asm/bitrev.h
index ec291c3..9482f78 100644
--- a/arch/arm/include/asm/bitrev.h
+++ b/arch/arm/include/asm/bitrev.h
@@ -17,4 +17,10 @@ static __always_inline __attribute_const__ u8 __arch_bitrev8(u8 x)
 	return __arch_bitrev32((u32)x) >> 24;
 }
 
+static __always_inline __attribute_const__ u32 __arch_bitrev8x4(u32 x)
+{
+	__asm__ ("rbit %0, %1; rev %0, %0" : "=r" (x) : "r" (x));
+	return x;
+}
+
 #endif
diff --git a/arch/arm64/include/asm/bitrev.h b/arch/arm64/include/asm/bitrev.h
index a5a0c36..1801078 100644
--- a/arch/arm64/include/asm/bitrev.h
+++ b/arch/arm64/include/asm/bitrev.h
@@ -16,4 +16,10 @@ static __always_inline __attribute_const__ u8 __arch_bitrev8(u8 x)
 	return __arch_bitrev32((u32)x) >> 24;
 }
 
+static __always_inline __attribute_const__ u32 __arch_bitrev8x4(u32 x)
+{
+	__asm__ ("rbit %0, %1; rev %0, %0" : "=r" (x) : "r" (x));
+	return x;
+}
+
 #endif
diff --git a/arch/mips/include/asm/bitrev.h b/arch/mips/include/asm/bitrev.h
index bc739a4..9ac6439 100644
--- a/arch/mips/include/asm/bitrev.h
+++ b/arch/mips/include/asm/bitrev.h
@@ -27,4 +27,10 @@ static __always_inline __attribute_const__ u8 __arch_bitrev8(u8 x)
 	return ret;
 }
 
+static __always_inline __attribute_const__ u32 __arch_bitrev8x4(u32 x)
+{
+	u32 ret;
+	asm("bitswap    %0, %1" : "=r"(ret) : "r"(x));
+	return ret;
+}
 #endif /* __MIPS_ASM_BITREV_H__ */
diff --git a/include/linux/bitrev.h b/include/linux/bitrev.h
index 868dcb6..b1cfa1a 100644
--- a/include/linux/bitrev.h
+++ b/include/linux/bitrev.h
@@ -9,6 +9,7 @@
 #define __bitrev32 __arch_bitrev32
 #define __bitrev16 __arch_bitrev16
 #define __bitrev8 __arch_bitrev8
+#define __bitrev8x4 __arch_bitrev8x4
 
 #else
 extern u8 const byte_rev_table[256];
-- 
2.9.3

Re: [PATCH v6 2/5] lib: implement __arch_bitrev8x4()

[Will Deacon]

On Fri, Dec 16, 2016 at 03:17:51PM -0800, Joshua Clayton wrote:
> Implement faster bitrev8x4() for arm, arm64 and mips, all the platforms
> with CONFIG_HAVE_ARCH_BITREVERSE.
> ARM platforms just need a byteswap added to the existing __arch_bitrev32()
> Amusingly, the mips implementation is exactly the opposite, requiring
> removal of the byteswap from its __arch_bitrev32()
> 
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> ---
>  arch/arm/include/asm/bitrev.h   | 6 ++++++
>  arch/arm64/include/asm/bitrev.h | 6 ++++++
>  arch/mips/include/asm/bitrev.h  | 6 ++++++
>  include/linux/bitrev.h          | 1 +
>  4 files changed, 19 insertions(+)
> 
> diff --git a/arch/arm/include/asm/bitrev.h b/arch/arm/include/asm/bitrev.h
> index ec291c3..9482f78 100644
> --- a/arch/arm/include/asm/bitrev.h
> +++ b/arch/arm/include/asm/bitrev.h
> @@ -17,4 +17,10 @@ static __always_inline __attribute_const__ u8 __arch_bitrev8(u8 x)
>  	return __arch_bitrev32((u32)x) >> 24;
>  }
>  
> +static __always_inline __attribute_const__ u32 __arch_bitrev8x4(u32 x)
> +{
> +	__asm__ ("rbit %0, %1; rev %0, %0" : "=r" (x) : "r" (x));
> +	return x;
> +}
> +
>  #endif
> diff --git a/arch/arm64/include/asm/bitrev.h b/arch/arm64/include/asm/bitrev.h
> index a5a0c36..1801078 100644
> --- a/arch/arm64/include/asm/bitrev.h
> +++ b/arch/arm64/include/asm/bitrev.h
> @@ -16,4 +16,10 @@ static __always_inline __attribute_const__ u8 __arch_bitrev8(u8 x)
>  	return __arch_bitrev32((u32)x) >> 24;
>  }
>  
> +static __always_inline __attribute_const__ u32 __arch_bitrev8x4(u32 x)
> +{
> +	__asm__ ("rbit %0, %1; rev %0, %0" : "=r" (x) : "r" (x));

This is broken -- you're operating on 64-bit registers. I only noticed
because if you write:

  swab32(bitrev32(x))

then GCC generates:

  rbit	w0, w0
  rev	w0, w0

so perhaps we should just implement the asm-generic version like that
and not bother with the arch-specific stuff?

Will

Re: [PATCH v6 2/5] lib: implement __arch_bitrev8x4()

[Joshua Clayton]

Will,


On 12/19/2016 02:06 AM, Will Deacon wrote:
> On Fri, Dec 16, 2016 at 03:17:51PM -0800, Joshua Clayton wrote:
>> Implement faster bitrev8x4() for arm, arm64 and mips, all the platforms
>> with CONFIG_HAVE_ARCH_BITREVERSE.
>> ARM platforms just need a byteswap added to the existing __arch_bitrev32()
>> Amusingly, the mips implementation is exactly the opposite, requiring
>> removal of the byteswap from its __arch_bitrev32()
>>
>> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
>> ---
>>  arch/arm/include/asm/bitrev.h   | 6 ++++++
>>  arch/arm64/include/asm/bitrev.h | 6 ++++++
>>  arch/mips/include/asm/bitrev.h  | 6 ++++++
>>  include/linux/bitrev.h          | 1 +
>>  4 files changed, 19 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/bitrev.h b/arch/arm/include/asm/bitrev.h
>> index ec291c3..9482f78 100644
>> --- a/arch/arm/include/asm/bitrev.h
>> +++ b/arch/arm/include/asm/bitrev.h
>> @@ -17,4 +17,10 @@ static __always_inline __attribute_const__ u8 __arch_bitrev8(u8 x)
>>  	return __arch_bitrev32((u32)x) >> 24;
>>  }
>>  
>> +static __always_inline __attribute_const__ u32 __arch_bitrev8x4(u32 x)
>> +{
>> +	__asm__ ("rbit %0, %1; rev %0, %0" : "=r" (x) : "r" (x));
>> +	return x;
>> +}
>> +
>>  #endif
>> diff --git a/arch/arm64/include/asm/bitrev.h b/arch/arm64/include/asm/bitrev.h
>> index a5a0c36..1801078 100644
>> --- a/arch/arm64/include/asm/bitrev.h
>> +++ b/arch/arm64/include/asm/bitrev.h
>> @@ -16,4 +16,10 @@ static __always_inline __attribute_const__ u8 __arch_bitrev8(u8 x)
>>  	return __arch_bitrev32((u32)x) >> 24;
>>  }
>>  
>> +static __always_inline __attribute_const__ u32 __arch_bitrev8x4(u32 x)
>> +{
>> +	__asm__ ("rbit %0, %1; rev %0, %0" : "=r" (x) : "r" (x));
> This is broken -- you're operating on 64-bit registers. I only noticed
> because if you write:
Ugh. mea culpa. I squinted at the AARCH64 asm and erroneously
believed it to be the same as arm.
>   swab32(bitrev32(x))
>
> then GCC generates:
>
>   rbit	w0, w0
>   rev	w0, w0
>
> so perhaps we should just implement the asm-generic version like that
> and not bother with the arch-specific stuff?
>
> Will
You are so right.
That is exactly what I will do.

Thanks,

Joshua

[PATCH v6 3/5] doc: dt: add cyclone-ps-spi binding document

[Joshua Clayton]

Describe a cyclone-ps-spi devicetree entry, required features

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/fpga/cyclone-ps-spi-fpga-mgr.txt      | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt

diff --git a/Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
new file mode 100644
index 0000000..3f515c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
@@ -0,0 +1,25 @@
+Altera Cyclone Passive Serial SPI 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,cyclone-ps-spi-fpga-mgr"
+- reg         : spi slave id of the fpga
+- config-gpios : config pin (referred to as nCONFIG in the cyclone manual)
+- status-gpios : status pin (referred to as nSTATUS in the cyclone manual)
+
+both gpios pins are normally active low open drain.
+
+Example:
+	fpga_spi: evi-fpga-spi@0 {
+		compatible = "altr,cyclone-ps-spi-fpga-mgr";
+		spi-max-frequency = <20000000>;
+		reg = <0>;
+		config-gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
+		status-gpios = <&gpio4 11 GPIO_ACTIVE_LOW>;
+	};
-- 
2.9.3

Re: [PATCH v6 3/5] doc: dt: add cyclone-ps-spi binding document

[Alan Tull]

On Fri, 16 Dec 2016, Joshua Clayton wrote:

> Describe a cyclone-ps-spi devicetree entry, required features
> 
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> Acked-by: Rob Herring <robh@kernel.org>

Acked-by: Alan Tull <atull@opensource.altera.com>

> ---
>  .../bindings/fpga/cyclone-ps-spi-fpga-mgr.txt      | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
> new file mode 100644
> index 0000000..3f515c7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
> @@ -0,0 +1,25 @@
> +Altera Cyclone Passive Serial SPI 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,cyclone-ps-spi-fpga-mgr"
> +- reg         : spi slave id of the fpga
> +- config-gpios : config pin (referred to as nCONFIG in the cyclone manual)
> +- status-gpios : status pin (referred to as nSTATUS in the cyclone manual)
> +
> +both gpios pins are normally active low open drain.
> +
> +Example:
> +	fpga_spi: evi-fpga-spi@0 {
> +		compatible = "altr,cyclone-ps-spi-fpga-mgr";
> +		spi-max-frequency = <20000000>;
> +		reg = <0>;
> +		config-gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
> +		status-gpios = <&gpio4 11 GPIO_ACTIVE_LOW>;
> +	};
> -- 
> 2.9.3
> 
> 

[PATCH v6 4/5] fpga manager: Add cyclone-ps-spi driver for Altera FPGAs

[Joshua Clayton]

cyclone-ps-spi loads FPGA firmware over spi, using the "passive serial"
interface on Altera Cyclone FPGAS.

This is 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          |   7 ++
 drivers/fpga/Makefile         |   1 +
 drivers/fpga/cyclone-ps-spi.c | 186 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+)
 create mode 100644 drivers/fpga/cyclone-ps-spi.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index ce861a2..e6c032d 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -20,6 +20,13 @@ config FPGA_REGION
 	  FPGA Regions allow loading FPGA images under control of
 	  the Device Tree.
 
+config FPGA_MGR_CYCLONE_PS_SPI
+	tristate "Altera Cyclone FPGA Passive Serial over SPI"
+	depends on SPI
+	help
+	  FPGA manager driver support for Altera Cyclone using the
+	  passive serial interface over SPI
+
 config FPGA_MGR_SOCFPGA
 	tristate "Altera SOCFPGA FPGA Manager"
 	depends on ARCH_SOCFPGA || COMPILE_TEST
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 8df07bc..a112bef 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -6,6 +6,7 @@
 obj-$(CONFIG_FPGA)			+= fpga-mgr.o
 
 # FPGA Manager Drivers
+obj-$(CONFIG_FPGA_MGR_CYCLONE_PS_SPI)	+= cyclone-ps-spi.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
 obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c
new file mode 100644
index 0000000..f9126f9
--- /dev/null
+++ b/drivers/fpga/cyclone-ps-spi.c
@@ -0,0 +1,186 @@
+/**
+ * Altera Cyclone Passive Serial SPI Driver
+ *
+ *  Copyright (c) 2017 United Western Technologies, Corporation
+ *
+ *  Joshua Clayton <stillcompiling@gmail.com>
+ *
+ * Manage Altera FPGA firmware that is loaded over spi using the passive
+ * serial configuration method.
+ * Firmware must be in binary "rbf" format.
+ * Works on Cyclone V. Should work on cyclone series.
+ * May work on other Altera FPGAs.
+ *
+ */
+
+#include <linux/bitrev.h>
+#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>
+#include <linux/sizes.h>
+
+#define FPGA_RESET_TIME		50   /* time in usecs to trigger FPGA config */
+#define FPGA_MIN_DELAY		50   /* min usecs to wait for config status */
+#define FPGA_MAX_DELAY		1000 /* max usecs to wait for config status */
+
+struct cyclonespi_conf {
+	struct gpio_desc *config;
+	struct gpio_desc *status;
+	struct spi_device *spi;
+};
+
+static const struct of_device_id of_ef_match[] = {
+	{ .compatible = "altr,cyclone-ps-spi-fpga-mgr", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_ef_match);
+
+static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
+{
+	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+
+	if (gpiod_get_value(conf->status))
+		return FPGA_MGR_STATE_RESET;
+
+	return FPGA_MGR_STATE_UNKNOWN;
+}
+
+static int cyclonespi_write_init(struct fpga_manager *mgr,
+				 struct fpga_image_info *info,
+				 const char *buf, size_t count)
+{
+	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+	int i;
+
+	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
+		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
+		return -EINVAL;
+	}
+
+	gpiod_set_value(conf->config, 1);
+	usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20);
+	if (!gpiod_get_value(conf->status)) {
+		dev_err(&mgr->dev, "Status pin should be low.\n");
+		return -EIO;
+	}
+
+	gpiod_set_value(conf->config, 0);
+	for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) {
+		usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20);
+		if (!gpiod_get_value(conf->status))
+			return 0;
+	}
+
+	dev_err(&mgr->dev, "Status pin not ready.\n");
+	return -EIO;
+}
+
+static void rev_buf(void *buf, size_t len)
+{
+	u32 *fw32 = (u32 *)buf;
+	const u32 *fw_end = (u32 *)(buf + len);
+
+	/* set buffer to lsb first */
+	while (fw32 < fw_end) {
+		*fw32 = bitrev8x4(*fw32);
+		fw32++;
+	}
+}
+
+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;
+		size_t stride = min(fw_data_end - fw_data, SZ_4K);
+
+		rev_buf((void *)fw_data, stride);
+		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,
+				     struct fpga_image_info *info)
+{
+	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+
+	if (gpiod_get_value(conf->status)) {
+		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->config = devm_gpiod_get(&spi->dev, "config", GPIOD_OUT_HIGH);
+	if (IS_ERR(conf->config)) {
+		dev_err(&spi->dev, "Failed to get config gpio: %ld\n",
+			PTR_ERR(conf->config));
+		return PTR_ERR(conf->config);
+	}
+
+	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 Cyclone PS 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   = "cyclone-ps-spi",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_match_ptr(of_ef_match),
+	},
+	.probe  = cyclonespi_probe,
+	.remove = cyclonespi_remove,
+};
+
+module_spi_driver(cyclonespi_driver)
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joshua Clayton <stillcompiling@gmail.com>");
+MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi");
-- 
2.9.3

Re: [PATCH v6 4/5] fpga manager: Add cyclone-ps-spi driver for Altera FPGAs

[Uwe Kleine-König]

On Fri, Dec 16, 2016 at 03:17:53PM -0800, Joshua Clayton wrote:
> cyclone-ps-spi loads FPGA firmware over spi, using the "passive serial"
> interface on Altera Cyclone FPGAS.
> 
> This is 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          |   7 ++
>  drivers/fpga/Makefile         |   1 +
>  drivers/fpga/cyclone-ps-spi.c | 186 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 194 insertions(+)
>  create mode 100644 drivers/fpga/cyclone-ps-spi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index ce861a2..e6c032d 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -20,6 +20,13 @@ config FPGA_REGION
>  	  FPGA Regions allow loading FPGA images under control of
>  	  the Device Tree.
>  
> +config FPGA_MGR_CYCLONE_PS_SPI
> +	tristate "Altera Cyclone FPGA Passive Serial over SPI"
> +	depends on SPI
> +	help
> +	  FPGA manager driver support for Altera Cyclone using the
> +	  passive serial interface over SPI
> +
>  config FPGA_MGR_SOCFPGA
>  	tristate "Altera SOCFPGA FPGA Manager"
>  	depends on ARCH_SOCFPGA || COMPILE_TEST
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8df07bc..a112bef 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,6 +6,7 @@
>  obj-$(CONFIG_FPGA)			+= fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_CYCLONE_PS_SPI)	+= cyclone-ps-spi.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
> diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c
> new file mode 100644
> index 0000000..f9126f9
> --- /dev/null
> +++ b/drivers/fpga/cyclone-ps-spi.c
> @@ -0,0 +1,186 @@
> +/**
> + * Altera Cyclone Passive Serial SPI Driver
> + *
> + *  Copyright (c) 2017 United Western Technologies, Corporation

In which timezone it's already 2017? s/  / /

> + *
> + *  Joshua Clayton <stillcompiling@gmail.com>
> + *
> + * Manage Altera FPGA firmware that is loaded over spi using the passive
> + * serial configuration method.
> + * Firmware must be in binary "rbf" format.
> + * Works on Cyclone V. Should work on cyclone series.
> + * May work on other Altera FPGAs.

I can test this later on an Arria 10. I'm not sure what the connection
between "Cyclone" and "Arria" is, but the protocol looks similar.

> + *
> + */
> +
> +#include <linux/bitrev.h>
> +#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>
> +#include <linux/sizes.h>
> +
> +#define FPGA_RESET_TIME		50   /* time in usecs to trigger FPGA config */
> +#define FPGA_MIN_DELAY		50   /* min usecs to wait for config status */
> +#define FPGA_MAX_DELAY		1000 /* max usecs to wait for config status */
> +
> +struct cyclonespi_conf {
> +	struct gpio_desc *config;
> +	struct gpio_desc *status;
> +	struct spi_device *spi;
> +};
> +
> +static const struct of_device_id of_ef_match[] = {
> +	{ .compatible = "altr,cyclone-ps-spi-fpga-mgr", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, of_ef_match);

barebox already has such a driver, the binding is available at

	https://git.pengutronix.de/cgit/barebox/tree/Documentation/devicetree/bindings/firmware/altr,passive-serial.txt

(This isn't completely accurate because nstat is optional in the driver.)

> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
> +{
> +	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> +
> +	if (gpiod_get_value(conf->status))
> +		return FPGA_MGR_STATE_RESET;
> +
> +	return FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int cyclonespi_write_init(struct fpga_manager *mgr,
> +				 struct fpga_image_info *info,
> +				 const char *buf, size_t count)
> +{
> +	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> +	int i;
> +
> +	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> +		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
> +		return -EINVAL;
> +	}
> +
> +	gpiod_set_value(conf->config, 1);
> +	usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20);
> +	if (!gpiod_get_value(conf->status)) {
> +		dev_err(&mgr->dev, "Status pin should be low.\n");

You write this when get_value returns 0. There is something fishy.

> +		return -EIO;
> +	}
> +
> +	gpiod_set_value(conf->config, 0);
> +	for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) {
> +		usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20);
> +		if (!gpiod_get_value(conf->status))
> +			return 0;
> +	}
> +
> +	dev_err(&mgr->dev, "Status pin not ready.\n");
> +	return -EIO;

For Arria 10 the documentation has:

	To ensure a successful configuration, send the entire
	configuration data to the device. CONF_DONE is released high
	when the device receives all the configuration data
	successfully. After CONF_DONE goes high, send two additional
	falling edges on DCLK to begin initialization and enter user
	mode.

ISTR this is necessary for Arria V, too.

> +}
> +
> +static void rev_buf(void *buf, size_t len)
> +{
> +	u32 *fw32 = (u32 *)buf;
> +	const u32 *fw_end = (u32 *)(buf + len);
> +
> +	/* set buffer to lsb first */
> +	while (fw32 < fw_end) {
> +		*fw32 = bitrev8x4(*fw32);
> +		fw32++;
> +	}

Is the size of the firmware always a multiple of 32 bit? If len isn't a
multiple of 4 you access data after the end of buf.

> +}
> +
> +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;
> +		size_t stride = min(fw_data_end - fw_data, SZ_4K);
> +
> +		rev_buf((void *)fw_data, stride);

This isn't necessary if the spi controller supports SPI_LSB_FIRST. At
least the mvebu spi core can do this for you. (The driver doesn't
support it yet, though.)

> +		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_probe(struct spi_device *spi)
> +{
> +	struct cyclonespi_conf *conf = devm_kzalloc(&spi->dev, sizeof(*conf),
> +						GFP_KERNEL);

please indent to the opening (.

> +
> +	if (!conf)
> +		return -ENOMEM;
> +
> +	conf->spi = spi;
> +	conf->config = devm_gpiod_get(&spi->dev, "config", GPIOD_OUT_HIGH);
> +	if (IS_ERR(conf->config)) {
> +		dev_err(&spi->dev, "Failed to get config gpio: %ld\n",
> +			PTR_ERR(conf->config));
> +		return PTR_ERR(conf->config);
> +	}
> +
> +	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 Cyclone PS 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   = "cyclone-ps-spi",
> +		.owner  = THIS_MODULE,
> +		.of_match_table = of_match_ptr(of_ef_match),
> +	},
> +	.probe  = cyclonespi_probe,
> +	.remove = cyclonespi_remove,
> +};

I'm not a big fan of aligning the assignment operators. This tends to
get out of sync or results in bigger than necessary changes in follow up
patches. Note that it's out of sync already now, so I suggest to use a
single space before =.

> +
> +module_spi_driver(cyclonespi_driver)
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Joshua Clayton <stillcompiling@gmail.com>");
> +MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi");
> -- 

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v6 4/5] fpga manager: Add cyclone-ps-spi driver for Altera FPGAs

[Joshua Clayton]

Uwe,

Thanks so much for your review.

On 12/18/2016 11:23 PM, Uwe Kleine-König wrote:
> On Fri, Dec 16, 2016 at 03:17:53PM -0800, Joshua Clayton wrote:
>> cyclone-ps-spi loads FPGA firmware over spi, using the "passive serial"
>> interface on Altera Cyclone FPGAS.
>>
>> This is 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          |   7 ++
>>  drivers/fpga/Makefile         |   1 +
>>  drivers/fpga/cyclone-ps-spi.c | 186 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 194 insertions(+)
>>  create mode 100644 drivers/fpga/cyclone-ps-spi.c
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index ce861a2..e6c032d 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -20,6 +20,13 @@ config FPGA_REGION
>>  	  FPGA Regions allow loading FPGA images under control of
>>  	  the Device Tree.
>>  
>> +config FPGA_MGR_CYCLONE_PS_SPI
>> +	tristate "Altera Cyclone FPGA Passive Serial over SPI"
>> +	depends on SPI
>> +	help
>> +	  FPGA manager driver support for Altera Cyclone using the
>> +	  passive serial interface over SPI
>> +
>>  config FPGA_MGR_SOCFPGA
>>  	tristate "Altera SOCFPGA FPGA Manager"
>>  	depends on ARCH_SOCFPGA || COMPILE_TEST
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index 8df07bc..a112bef 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -6,6 +6,7 @@
>>  obj-$(CONFIG_FPGA)			+= fpga-mgr.o
>>  
>>  # FPGA Manager Drivers
>> +obj-$(CONFIG_FPGA_MGR_CYCLONE_PS_SPI)	+= cyclone-ps-spi.o
>>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
>>  obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
>>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
>> diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c
>> new file mode 100644
>> index 0000000..f9126f9
>> --- /dev/null
>> +++ b/drivers/fpga/cyclone-ps-spi.c
>> @@ -0,0 +1,186 @@
>> +/**
>> + * Altera Cyclone Passive Serial SPI Driver
>> + *
>> + *  Copyright (c) 2017 United Western Technologies, Corporation
> In which timezone it's already 2017? s/  / /
>
LOL. It will be 2017 long before 4.11 was my thinking.
I guess I've never spent much time on time stamp etiquette for copyright.
It said "2015" in v5, despite still being revised.
>> + *
>> + *  Joshua Clayton <stillcompiling@gmail.com>
>> + *
>> + * Manage Altera FPGA firmware that is loaded over spi using the passive
>> + * serial configuration method.
>> + * Firmware must be in binary "rbf" format.
>> + * Works on Cyclone V. Should work on cyclone series.
>> + * May work on other Altera FPGAs.
> I can test this later on an Arria 10. I'm not sure what the connection
> between "Cyclone" and "Arria" is, but the protocol looks similar.
My guess was it would be.
Would be wonderful to have someone to test.
>> + *
>> + */
>> +
>> +#include <linux/bitrev.h>
>> +#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>
>> +#include <linux/sizes.h>
>> +
>> +#define FPGA_RESET_TIME		50   /* time in usecs to trigger FPGA config */
>> +#define FPGA_MIN_DELAY		50   /* min usecs to wait for config status */
>> +#define FPGA_MAX_DELAY		1000 /* max usecs to wait for config status */
>> +
>> +struct cyclonespi_conf {
>> +	struct gpio_desc *config;
>> +	struct gpio_desc *status;
>> +	struct spi_device *spi;
>> +};
>> +
>> +static const struct of_device_id of_ef_match[] = {
>> +	{ .compatible = "altr,cyclone-ps-spi-fpga-mgr", },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, of_ef_match);
> barebox already has such a driver, the binding is available at
>
> 	https://git.pengutronix.de/cgit/barebox/tree/Documentation/devicetree/bindings/firmware/altr,passive-serial.txt
>
> (This isn't completely accurate because nstat is optional in the driver.)
Interesting.
The binding looks ... like we should synchronize those bindings.
In the case of my hardware, I don't have access to the confd, but do
have access to nstat. I was thinking that using confd to signal done
would be a nice but optional... Ideally you'd have both.
>> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
>> +{
>> +	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
>> +
>> +	if (gpiod_get_value(conf->status))
>> +		return FPGA_MGR_STATE_RESET;
>> +
>> +	return FPGA_MGR_STATE_UNKNOWN;
>> +}
>> +
>> +static int cyclonespi_write_init(struct fpga_manager *mgr,
>> +				 struct fpga_image_info *info,
>> +				 const char *buf, size_t count)
>> +{
>> +	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
>> +	int i;
>> +
>> +	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
>> +		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	gpiod_set_value(conf->config, 1);
>> +	usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20);
>> +	if (!gpiod_get_value(conf->status)) {
>> +		dev_err(&mgr->dev, "Status pin should be low.\n");
> You write this when get_value returns 0. There is something fishy.
I'll take a look. These gpios are "active low", so a logical 1 is a
physical low, IIRC. Maybe I should change the wording:
Something like dev_err(&mgr->dev, "Status pin should be in reset.\n");

Perhaps?


>> +		return -EIO;
>> +	}
>> +
>> +	gpiod_set_value(conf->config, 0);
>> +	for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) {
>> +		usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20);
>> +		if (!gpiod_get_value(conf->status))
>> +			return 0;
>> +	}
>> +
>> +	dev_err(&mgr->dev, "Status pin not ready.\n");
>> +	return -EIO;
> For Arria 10 the documentation has:
>
> 	To ensure a successful configuration, send the entire
> 	configuration data to the device. CONF_DONE is released high
> 	when the device receives all the configuration data
> 	successfully. After CONF_DONE goes high, send two additional
> 	falling edges on DCLK to begin initialization and enter user
> 	mode.
>
> ISTR this is necessary for Arria V, too.
DCLK is the spi clock, yes?
Would sending an extra byte after CONF_DONE is released suffice?
>> +}
>> +
>> +static void rev_buf(void *buf, size_t len)
>> +{
>> +	u32 *fw32 = (u32 *)buf;
>> +	const u32 *fw_end = (u32 *)(buf + len);
>> +
>> +	/* set buffer to lsb first */
>> +	while (fw32 < fw_end) {
>> +		*fw32 = bitrev8x4(*fw32);
>> +		fw32++;
>> +	}
> Is the size of the firmware always a multiple of 32 bit? If len isn't a
> multiple of 4 you access data after the end of buf.
The rbf cyclone V bitstream is padded out with extra bytes of FFFFFFFF
and always a multiple of 4 bytes.
I could not find anywhere this is documented.
I guess we should not assume this will always be the case.
I'll add something to handle the tail.
>> +}
>> +
>> +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;
>> +		size_t stride = min(fw_data_end - fw_data, SZ_4K);
>> +
>> +		rev_buf((void *)fw_data, stride);
> This isn't necessary if the spi controller supports SPI_LSB_FIRST. At
> least the mvebu spi core can do this for you. (The driver doesn't
> support it yet, though.)
This is true, but many of them do not.

Moritz Fischer had  proposal to add things like this with a flag.
It could then be part of the library, rather than part of the driver

Speaking of which,
I made an unsuccessful attempt to hack generic lsb first SPI
with an extra bounce buffer.
Sending was fine, but I ran into trouble with LSB first rx
(I think) because of dma.

>> +		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_probe(struct spi_device *spi)
>> +{
>> +	struct cyclonespi_conf *conf = devm_kzalloc(&spi->dev, sizeof(*conf),
>> +						GFP_KERNEL);
> please indent to the opening (.
Will fix.
>> +
>> +	if (!conf)
>> +		return -ENOMEM;
>> +
>> +	conf->spi = spi;
>> +	conf->config = devm_gpiod_get(&spi->dev, "config", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(conf->config)) {
>> +		dev_err(&spi->dev, "Failed to get config gpio: %ld\n",
>> +			PTR_ERR(conf->config));
>> +		return PTR_ERR(conf->config);
>> +	}
>> +
>> +	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 Cyclone PS 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   = "cyclone-ps-spi",
>> +		.owner  = THIS_MODULE,
>> +		.of_match_table = of_match_ptr(of_ef_match),
>> +	},
>> +	.probe  = cyclonespi_probe,
>> +	.remove = cyclonespi_remove,
>> +};
> I'm not a big fan of aligning the assignment operators. This tends to
> get out of sync or results in bigger than necessary changes in follow up
> patches. Note that it's out of sync already now, so I suggest to use a
> single space before =.
Yes, I can see it your way. Will change.
>> +
>> +module_spi_driver(cyclonespi_driver)
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Joshua Clayton <stillcompiling@gmail.com>");
>> +MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi");
>> -- 
> Best regards
> Uwe
>
Even bester regards,

Joshua

Re: [PATCH v6 4/5] fpga manager: Add cyclone-ps-spi driver for Altera FPGAs

[Uwe Kleine-König]

Hello Joshua,

On Tue, Dec 20, 2016 at 11:47:37AM -0800, Joshua Clayton wrote:
> >> + *  Copyright (c) 2017 United Western Technologies, Corporation
> > In which timezone it's already 2017? s/  / /
> >
> LOL. It will be 2017 long before 4.11 was my thinking.
> I guess I've never spent much time on time stamp etiquette for copyright.
> It said "2015" in v5, despite still being revised.

Well, you have to put the date when you worked on the driver.

> >> + *
> >> + *  Joshua Clayton <stillcompiling@gmail.com>
> >> + *
> >> + * Manage Altera FPGA firmware that is loaded over spi using the passive
> >> + * serial configuration method.
> >> + * Firmware must be in binary "rbf" format.
> >> + * Works on Cyclone V. Should work on cyclone series.
> >> + * May work on other Altera FPGAs.
> > I can test this later on an Arria 10. I'm not sure what the connection
> > between "Cyclone" and "Arria" is, but the protocol looks similar.
> My guess was it would be.
> Would be wonderful to have someone to test.

I didn't come around yet.

> >> + *
> >> + */
> >> +
> >> +#include <linux/bitrev.h>
> >> +#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>
> >> +#include <linux/sizes.h>
> >> +
> >> +#define FPGA_RESET_TIME		50   /* time in usecs to trigger FPGA config */
> >> +#define FPGA_MIN_DELAY		50   /* min usecs to wait for config status */
> >> +#define FPGA_MAX_DELAY		1000 /* max usecs to wait for config status */
> >> +
> >> +struct cyclonespi_conf {
> >> +	struct gpio_desc *config;
> >> +	struct gpio_desc *status;
> >> +	struct spi_device *spi;
> >> +};
> >> +
> >> +static const struct of_device_id of_ef_match[] = {
> >> +	{ .compatible = "altr,cyclone-ps-spi-fpga-mgr", },
> >> +	{}
> >> +};
> >> +MODULE_DEVICE_TABLE(of, of_ef_match);
> > barebox already has such a driver, the binding is available at
> >
> > 	https://git.pengutronix.de/cgit/barebox/tree/Documentation/devicetree/bindings/firmware/altr,passive-serial.txt
> >
> > (This isn't completely accurate because nstat is optional in the driver.)
> Interesting.
> The binding looks ... like we should synchronize those bindings.

ack.

> >> +	gpiod_set_value(conf->config, 1);
> >> +	usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20);
> >> +	if (!gpiod_get_value(conf->status)) {
> >> +		dev_err(&mgr->dev, "Status pin should be low.\n");
> > You write this when get_value returns 0. There is something fishy.
> I'll take a look. These gpios are "active low", so a logical 1 is a
> physical low, IIRC. Maybe I should change the wording:
> Something like dev_err(&mgr->dev, "Status pin should be in reset.\n");

maybe "inactive"? Then then you should better use nconfig as dts name
(as done in the barebox binding) and put the active low information into
the flag.

> >> +		return -EIO;
> >> +	}
> >> +
> >> +	gpiod_set_value(conf->config, 0);
> >> +	for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) {
> >> +		usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20);
> >> +		if (!gpiod_get_value(conf->status))
> >> +			return 0;
> >> +	}
> >> +
> >> +	dev_err(&mgr->dev, "Status pin not ready.\n");
> >> +	return -EIO;
> > For Arria 10 the documentation has:
> >
> > 	To ensure a successful configuration, send the entire
> > 	configuration data to the device. CONF_DONE is released high
> > 	when the device receives all the configuration data
> > 	successfully. After CONF_DONE goes high, send two additional
> > 	falling edges on DCLK to begin initialization and enter user
> > 	mode.
> >
> > ISTR this is necessary for Arria V, too.
> DCLK is the spi clock, yes?
> Would sending an extra byte after CONF_DONE is released suffice?

The barebox driver sends two bytes.

> >> +}
> >> +
> >> +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;
> >> +		size_t stride = min(fw_data_end - fw_data, SZ_4K);
> >> +
> >> +		rev_buf((void *)fw_data, stride);
> > This isn't necessary if the spi controller supports SPI_LSB_FIRST. At
> > least the mvebu spi core can do this for you. (The driver doesn't
> > support it yet, though.)
> This is true, but many of them do not.

And I think it's hard to detect if the adapter driver supports this or
simply ignores it.
 
> Moritz Fischer had  proposal to add things like this with a flag.
> It could then be part of the library, rather than part of the driver
> 
> Speaking of which,
> I made an unsuccessful attempt to hack generic lsb first SPI
> with an extra bounce buffer.
> Sending was fine, but I ran into trouble with LSB first rx
> (I think) because of dma.

Link?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v6 5/5] ARM: dts: imx6q-evi: support cyclone-ps-spi

[Joshua Clayton]

Add support for Altera cyclone V FPGA connected to an spi port
to the evi devicetree file

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 7c7c1a8..ec4d365 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@0 {
+		compatible = "altr,cyclone-ps-spi-fpga-mgr";
+		spi-max-frequency = <20000000>;
+		reg = <0>;
+		pinctrl-0 = <&pinctrl_fpgaspi>;
+		config-gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
+		status-gpios = <&gpio4 11 GPIO_ACTIVE_LOW>;
+	};
 };
 
 &ecspi3 {
@@ -322,6 +331,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.9.3

Re: [PATCH v6 5/5] ARM: dts: imx6q-evi: support cyclone-ps-spi

[Alan Tull]

On Fri, 16 Dec 2016, Joshua Clayton wrote:

> Add support for Altera cyclone V FPGA connected to an spi port
> to the evi devicetree file
> 
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>

Acked-by: Alan Tull <atull@opensource.altera.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 7c7c1a8..ec4d365 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@0 {
> +		compatible = "altr,cyclone-ps-spi-fpga-mgr";
> +		spi-max-frequency = <20000000>;
> +		reg = <0>;
> +		pinctrl-0 = <&pinctrl_fpgaspi>;
> +		config-gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
> +		status-gpios = <&gpio4 11 GPIO_ACTIVE_LOW>;
> +	};
>  };
>  
>  &ecspi3 {
> @@ -322,6 +331,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.9.3
> 
> 

Re: [PATCH v6 0/5] Altera Cyclone Passive Serial SPI FPGA Manager

[Alan Tull]

On Fri, 16 Dec 2016, Joshua Clayton wrote:

> This series adds an FPGA manager for Altera cyclone FPGAs
> that can program them using an spi port and a couple of gpios, using
> Alteras passive serial protocol.
> 
> Need ACKs from ARCH maintainers for ARCH specific implementations of
> __arch_bitrev8x4(), and I've added more ARCHes, so will need more ACKS, 
> but the generic code will work without that patch, so if there is a holdup
> the rest of the series can go in without patch 2

Hi Joshua,

We still need Russell to ACK or take patch 1.  It's the merge window
so he may be busy.

Alan

> 
> Changes from v5:
> - Rebased on next-20161214xi
> - Corrected for FPGA Mgr API change in  write_init() and write_complete()
> - Better describe the device cyclone-ps-spi runs on in the file header.
> - Split the bitrev8x4 patch into generic and arch specific patches...
> - Added AARCH64 and MIPS implementations of bitrev8x4()... they all have to 
>   have an implementation for it to compile cleanly across platforms
> - Added the changes to imx6q-evi.dts to the patch set.
> 
> Changes from v4:
> - Added the needed return statement to __arch_bitrev8x4()
> - Added Rob Herrings ACK for and fix a typo in the commit log of patch 2
> 
> Changes from v3:
> - Fixed up the state() function to return the state of the status pin
>   reqested by Alan Tull
> - Switched the pin to ACTIVE_LOW and coresponding logic level, and updated
>   the corresponding documentation. Thanks Rob Herring for pointing out my
>   mistake.
> - Per Rob Herring, switched from "gpio" to "gpios" in dts
> 
> Changes from v2:
> - Merged patch 3 and 4 as suggested in review by Moritz Fischer
> - Changed FPGA_MIN_DELAY from 250 to 50 ms is the time advertized by
>   Altera. This now works, as we don't assume it is done
> 
> Changes from v1:
> - Changed the name from cyclone-spi-fpga-mgr to cyclone-ps-spi-fpga-mgr
>   This name change was requested by Alan Tull, to be specific about which
>   programming method is being employed on the fpga.
> - Changed the name of the reset-gpio to config-gpio to closer match the
>   way the pins are described in the Altera manual
> - Moved MODULE_LICENCE, _AUTHOR, and _DESCRIPTION to the bottom
> 
> - Added a bitrev8x4() function to the bitrev headers and implemented ARM
>  const, runtime, and ARM specific faster versions (This may end up
>  needing to be a standalone patch)
> 
> - Moved the bitswapping into cyclonespi_write(), as requested.
>   This falls short of my desired generic lsb first spi support, but is a step
>   in that direction.
> 
> - Fixed whitespace problems introduced during refactoring
> 
> - Replaced magic number for initial delay with a descriptive macro
> - Poll the fpga to see when it is ready rather than a fixed 1 ms sleep
> 
> Joshua Clayton (5):
>   lib: add bitrev8x4()
>   lib: implement __arch_bitrev8x4()
>   doc: dt: add cyclone-ps-spi binding document
>   fpga manager: Add cyclone-ps-spi driver for Altera FPGAs
>   ARM: dts: imx6q-evi: support cyclone-ps-spi
> 
>  .../bindings/fpga/cyclone-ps-spi-fpga-mgr.txt      |  25 +++
>  arch/arm/boot/dts/imx6q-evi.dts                    |  16 ++
>  arch/arm/include/asm/bitrev.h                      |   6 +
>  arch/arm64/include/asm/bitrev.h                    |   6 +
>  arch/mips/include/asm/bitrev.h                     |   6 +
>  drivers/fpga/Kconfig                               |   7 +
>  drivers/fpga/Makefile                              |   1 +
>  drivers/fpga/cyclone-ps-spi.c                      | 186 +++++++++++++++++++++
>  include/linux/bitrev.h                             |  26 +++
>  9 files changed, 279 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
>  create mode 100644 drivers/fpga/cyclone-ps-spi.c
> 
> -- 
> 2.9.3
> 
> --
> 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
>