[PATCH v8 1/3] of: Add vendor prefix for Lattice Semiconductor

[PATCH v8 1/3] of: Add vendor prefix for Lattice Semiconductor

[Joel Holdsworth]

Lattice Semiconductor Corporation is a manufacturer of integrated
circuits and IP products, including low-power FPGAs, video connectivity
devices and millimeter wave wireless products.

Website: http://latticesemi.com

Signed-off-by: Joel Holdsworth <joel@airwebreathe.org.uk>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 1992aa9..d64a835 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -146,6 +146,7 @@ kosagi	Sutajio Ko-Usagi PTE Ltd.
 kyo	Kyocera Corporation
 lacie	LaCie
 lantiq	Lantiq Semiconductor
+lattice	Lattice Semiconductor
 lenovo	Lenovo Group Ltd.
 lg	LG Corporation
 linux	Linux-specific binding
-- 
2.7.4

[PATCH v8 2/3] Documentation: Add binding document for Lattice iCE40 FPGA manager

[Joel Holdsworth]

This adds documentation of the device tree bindings of the Lattice iCE40
FPGA driver for the FPGA manager framework.

Signed-off-by: Joel Holdsworth <joel@airwebreathe.org.uk>
---
 .../bindings/fpga/lattice-ice40-fpga-mgr.txt        | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt

diff --git a/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
new file mode 100644
index 0000000..7e7a78b
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
@@ -0,0 +1,21 @@
+Lattice iCE40 FPGA Manager
+
+Required properties:
+- compatible:		Should contain "lattice,ice40-fpga-mgr"
+- reg:			SPI chip select
+- spi-max-frequency:	Maximum SPI frequency (>=1000000, <=25000000)
+- cdone-gpios:		GPIO input connected to CDONE pin
+- reset-gpios:		Active-low GPIO output connected to CRESET_B pin. Note
+			that unless the GPIO is held low during startup, the
+			FPGA will enter Master SPI mode and drive SCK with a
+			clock signal potentially jamming other devices on the
+			bus until the firmware is loaded.
+
+Example:
+	ice40: ice40@0 {
+		compatible = "lattice,ice40-fpga-mgr";
+		reg = <0>;
+		spi-max-frequency = <1000000>;
+		cdone-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio 22 GPIO_ACTIVE_LOW>;
+	};
-- 
2.7.4

Re: [PATCH v8 2/3] Documentation: Add binding document for Lattice iCE40 FPGA manager

[Marek Vasut]

On 11/07/2016 03:49 AM, Joel Holdsworth wrote:
> This adds documentation of the device tree bindings of the Lattice iCE40
> FPGA driver for the FPGA manager framework.
> 
> Signed-off-by: Joel Holdsworth <joel@airwebreathe.org.uk>
> ---
>  .../bindings/fpga/lattice-ice40-fpga-mgr.txt        | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
> new file mode 100644
> index 0000000..7e7a78b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
> @@ -0,0 +1,21 @@
> +Lattice iCE40 FPGA Manager
> +
> +Required properties:
> +- compatible:		Should contain "lattice,ice40-fpga-mgr"
> +- reg:			SPI chip select
> +- spi-max-frequency:	Maximum SPI frequency (>=1000000, <=25000000)
> +- cdone-gpios:		GPIO input connected to CDONE pin
> +- reset-gpios:		Active-low GPIO output connected to CRESET_B pin. Note
> +			that unless the GPIO is held low during startup, the
> +			FPGA will enter Master SPI mode and drive SCK with a
> +			clock signal potentially jamming other devices on the
> +			bus until the firmware is loaded.
> +
> +Example:
> +	ice40: ice40@0 {
> +		compatible = "lattice,ice40-fpga-mgr";
> +		reg = <0>;
> +		spi-max-frequency = <1000000>;
> +		cdone-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio 22 GPIO_ACTIVE_LOW>;

Wouldn't it make more sense to have "gpios" and "gpio-names" ? That
scales better imo, although in this case we cannot really talk about
scaling.

> +	};
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH v8 2/3] Documentation: Add binding document for Lattice iCE40 FPGA manager

[Joel Holdsworth]

>> +		cdone-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
>> +		reset-gpios = <&gpio 22 GPIO_ACTIVE_LOW>;
>
> Wouldn't it make more sense to have "gpios" and "gpio-names" ? That
> scales better imo, although in this case we cannot really talk about
> scaling.
>

I don't believe that would be conventional. '-gpios' seems to be the 
standard over '-gpio' even for a single GPIO.

Having multiple GPIOs in that field is only relevant when it could be an 
array e.g. multiple chip-select GPIO lines. Not for multiple GPIOs with 
differing functions.

Also it doesn't fit with the way devm_gpiod_get works where you select 
GPIO(s) from the device-tree, and specify whether it's an input or an 
output. In this case one is an output, one is an input.

Joel

Re: [PATCH v8 2/3] Documentation: Add binding document for Lattice iCE40 FPGA manager

[Rob Herring]

On Sun, Nov 06, 2016 at 07:49:21PM -0700, Joel Holdsworth wrote:
> This adds documentation of the device tree bindings of the Lattice iCE40
> FPGA driver for the FPGA manager framework.
> 
> Signed-off-by: Joel Holdsworth <joel@airwebreathe.org.uk>
> ---
>  .../bindings/fpga/lattice-ice40-fpga-mgr.txt        | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v8 2/3] Documentation: Add binding document for Lattice iCE40 FPGA manager

[atull]

On Mon, 14 Nov 2016, Rob Herring wrote:

> On Sun, Nov 06, 2016 at 07:49:21PM -0700, Joel Holdsworth wrote:
> > This adds documentation of the device tree bindings of the Lattice iCE40
> > FPGA driver for the FPGA manager framework.
> > 
> > Signed-off-by: Joel Holdsworth <joel@airwebreathe.org.uk>
> > ---
> >  .../bindings/fpga/lattice-ice40-fpga-mgr.txt        | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 

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

Re: [PATCH v8 2/3] Documentation: Add binding document for Lattice iCE40 FPGA manager

[Moritz Fischer]

On Fri, Nov 18, 2016 at 10:56 AM, atull <atull@opensource.altera.com> wrote:
> On Mon, 14 Nov 2016, Rob Herring wrote:
>
>> On Sun, Nov 06, 2016 at 07:49:21PM -0700, Joel Holdsworth wrote:
>> > This adds documentation of the device tree bindings of the Lattice iCE40
>> > FPGA driver for the FPGA manager framework.
>> >
>> > Signed-off-by: Joel Holdsworth <joel@airwebreathe.org.uk>
>> > ---
>> >  .../bindings/fpga/lattice-ice40-fpga-mgr.txt        | 21 +++++++++++++++++++++
>> >  1 file changed, 21 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
>>
>> Acked-by: Rob Herring <robh@kernel.org>
>>
>
> Acked-by: Alan Tull <atull@opensource.altera.com>
Acked-by: Moritz Fischer <moritz.fischer@ettus.com>

Re: [PATCH v8 2/3] Documentation: Add binding document for Lattice iCE40 FPGA manager

[Marek Vasut]

On 11/18/2016 08:17 PM, Moritz Fischer wrote:
> On Fri, Nov 18, 2016 at 10:56 AM, atull <atull@opensource.altera.com> wrote:
>> On Mon, 14 Nov 2016, Rob Herring wrote:
>>
>>> On Sun, Nov 06, 2016 at 07:49:21PM -0700, Joel Holdsworth wrote:
>>>> This adds documentation of the device tree bindings of the Lattice iCE40
>>>> FPGA driver for the FPGA manager framework.
>>>>
>>>> Signed-off-by: Joel Holdsworth <joel@airwebreathe.org.uk>
>>>> ---
>>>>  .../bindings/fpga/lattice-ice40-fpga-mgr.txt        | 21 +++++++++++++++++++++
>>>>  1 file changed, 21 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
>>>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>>
>>
>> Acked-by: Alan Tull <atull@opensource.altera.com>
> Acked-by: Moritz Fischer <moritz.fischer@ettus.com>
Acked-by: Marek Vasut <marex@denx.de>

-- 
Best regards,
Marek Vasut

[PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Joel Holdsworth]

The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
and very regular structure, designed for low-cost, high-volume consumer
and system applications.

This patch adds support to the FPGA manager for configuring the SRAM of
iCE40LM, iCE40LP, iCE40HX, iCE40 Ultra, iCE40 UltraLite and iCE40
UltraPlus devices, through slave SPI.

The iCE40 family is notable because it is the first FPGA family to have
complete reverse engineered bit-stream documentation for the iCE40LP and
iCE40HX devices. Furthermore, there is now a Free Software Verilog
synthesis tool-chain: the "IceStorm" tool-chain.

This project is the work of Clifford Wolf, who is the maintainer of
Yosys Verilog RTL synthesis framework, and Mathias Lasser, with notable
contributions from "Cotton Seed", the main author of "arachne-pnr"; a
place-and-route tool for iCE40 FPGAs.

Having a Free Software synthesis tool-chain offers interesting
opportunities for embedded devices that are able reconfigure themselves
with open firmware that is generated on the device itself. For example
a mobile device might have an application processor with an iCE40 FPGA
attached, which implements slave devices, or through which the processor
communicates with other devices through the FPGA fabric.

A kernel driver for the iCE40 is useful, because in some cases, the FPGA
may need to be configured before other devices can be accessed.

An example of such a device is the icoBoard; a RaspberryPI HAT which
features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
Digilent-compatible PMOD modules. A PMOD module may contain a device
with which the kernel communicates, via the FPGA.

Signed-off-by: Joel Holdsworth <joel@airwebreathe.org.uk>
---
 drivers/fpga/Kconfig     |   6 ++
 drivers/fpga/Makefile    |   1 +
 drivers/fpga/ice40-spi.c | 217 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/fpga/ice40-spi.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index d614102..5b0f137 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -13,6 +13,12 @@ config FPGA
 
 if FPGA
 
+config FPGA_MGR_ICE40_SPI
+	tristate "Lattice iCE40 SPI"
+	depends on OF && SPI
+	help
+	  FPGA manager driver support for Lattice iCE40 FPGAs 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..adb5811 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_ICE40_SPI)	+= ice40-spi.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c
new file mode 100644
index 0000000..7d7595b
--- /dev/null
+++ b/drivers/fpga/ice40-spi.c
@@ -0,0 +1,217 @@
+/*
+ * FPGA Manager Driver for Lattice iCE40.
+ *
+ *  Copyright (c) 2016 Joel Holdsworth
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This driver adds support to the FPGA manager for configuring the SRAM of
+ * Lattice iCE40 FPGAs through slave SPI.
+ */
+
+#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>
+
+#define ICE40_SPI_FPGAMGR_RESET_DELAY 1 /* us (>200ns) */
+#define ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY 1200 /* us */
+
+#define ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES DIV_ROUND_UP(49, 8)
+
+struct ice40_fpga_priv {
+	struct spi_device *dev;
+	struct gpio_desc *reset;
+	struct gpio_desc *cdone;
+};
+
+static enum fpga_mgr_states ice40_fpga_ops_state(struct fpga_manager *mgr)
+{
+	struct ice40_fpga_priv *priv = mgr->priv;
+
+	return gpiod_get_value(priv->cdone) ? FPGA_MGR_STATE_OPERATING :
+		FPGA_MGR_STATE_UNKNOWN;
+}
+
+static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
+				     const char *buf, size_t count)
+{
+	struct ice40_fpga_priv *priv = mgr->priv;
+	struct spi_device *dev = priv->dev;
+	struct spi_message message;
+	struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1,
+		.delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY};
+	struct spi_transfer housekeeping_delay_then_release_cs = {
+		.delay_usecs = ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY};
+	int ret;
+
+	if ((flags & FPGA_MGR_PARTIAL_RECONFIG)) {
+		dev_err(&dev->dev,
+			"Partial reconfiguration is not supported\n");
+		return -ENOTSUPP;
+	}
+
+	/* Lock the bus, assert CRESET_B and SS_B and delay >200ns */
+	spi_bus_lock(dev->master);
+
+	gpiod_set_value(priv->reset, 1);
+
+	spi_message_init(&message);
+	spi_message_add_tail(&assert_cs_then_reset_delay, &message);
+	ret = spi_sync_locked(dev, &message);
+
+	/* Come out of reset */
+	gpiod_set_value(priv->reset, 0);
+
+	/* Abort if the chip-select failed */
+	if (ret)
+		goto fail;
+
+	/* Check CDONE is de-asserted i.e. the FPGA is reset */
+	if (gpiod_get_value(priv->cdone)) {
+		dev_err(&dev->dev, "Device reset failed, CDONE is asserted\n");
+		ret = -EIO;
+		goto fail;
+	}
+
+	/* Wait for the housekeeping to complete, and release SS_B */
+	spi_message_init(&message);
+	spi_message_add_tail(&housekeeping_delay_then_release_cs, &message);
+	ret = spi_sync_locked(dev, &message);
+
+fail:
+	spi_bus_unlock(dev->master);
+
+	return ret;
+}
+
+static int ice40_fpga_ops_write(struct fpga_manager *mgr,
+				const char *buf, size_t count)
+{
+	struct ice40_fpga_priv *priv = mgr->priv;
+
+	return spi_write(priv->dev, buf, count);
+}
+
+static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr, u32 flags)
+{
+	struct ice40_fpga_priv *priv = mgr->priv;
+	struct spi_device *dev = priv->dev;
+	const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,};
+
+	/* Check CDONE is asserted */
+	if (!gpiod_get_value(priv->cdone)) {
+		dev_err(&dev->dev,
+			"CDONE was not asserted after firmware transfer\n");
+		return -EIO;
+	}
+
+	/* Send of zero-padding to activate the firmware */
+	return spi_write(dev, padding, sizeof(padding));
+}
+
+static void ice40_fpga_ops_fpga_remove(struct fpga_manager *mgr)
+{
+	struct ice40_fpga_priv *priv = mgr->priv;
+
+	/* Enter reset */
+	gpiod_set_value(priv->reset, 1);
+}
+
+static const struct fpga_manager_ops ice40_fpga_ops = {
+	.state = ice40_fpga_ops_state,
+	.write_init = ice40_fpga_ops_write_init,
+	.write = ice40_fpga_ops_write,
+	.write_complete = ice40_fpga_ops_write_complete,
+	.fpga_remove = ice40_fpga_ops_fpga_remove
+};
+
+static int ice40_fpga_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct device_node *np = spi->dev.of_node;
+	struct ice40_fpga_priv *priv;
+	int ret;
+
+	if (!np) {
+		dev_err(dev, "No Device Tree entry\n");
+		return -EINVAL;
+	}
+
+	priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = spi;
+
+	/* Check board setup data. */
+	if (spi->max_speed_hz > 25000000) {
+		dev_err(dev, "Speed is too high\n");
+		return -EINVAL;
+	}
+
+	if (spi->max_speed_hz < 1000000) {
+		dev_err(dev, "Speed is too low\n");
+		return -EINVAL;
+	}
+
+	if (spi->mode & SPI_CPHA) {
+		dev_err(dev, "Bad mode\n");
+		return -EINVAL;
+	}
+
+	/* Set up the GPIOs */
+	priv->cdone = devm_gpiod_get(dev, "cdone", GPIOD_IN);
+	if (IS_ERR(priv->cdone)) {
+		dev_err(dev, "Failed to get CDONE GPIO: %ld\n",
+			PTR_ERR(priv->cdone));
+		return -EINVAL;
+	}
+
+	priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->reset)) {
+		dev_err(dev, "Failed to get CRESET_B GPIO: %ld\n",
+			PTR_ERR(priv->reset));
+		return -EINVAL;
+	}
+
+	/* Register with the FPGA manager */
+	ret = fpga_mgr_register(dev, "Lattice iCE40 FPGA Manager",
+				&ice40_fpga_ops, priv);
+	if (ret) {
+		dev_err(dev, "Unable to register FPGA manager");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ice40_fpga_remove(struct spi_device *spi)
+{
+	fpga_mgr_unregister(&spi->dev);
+	return 0;
+}
+
+static const struct of_device_id ice40_fpga_of_match[] = {
+	{ .compatible = "lattice,ice40-fpga-mgr", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ice40_fpga_of_match);
+
+static struct spi_driver ice40_fpga_driver = {
+	.probe = ice40_fpga_probe,
+	.remove = ice40_fpga_remove,
+	.driver = {
+		.name = "ice40spi",
+		.of_match_table = of_match_ptr(ice40_fpga_of_match),
+	},
+};
+
+module_spi_driver(ice40_fpga_driver);
+
+MODULE_AUTHOR("Joel Holdsworth <joel@airwebreathe.org.uk>");
+MODULE_DESCRIPTION("Lattice iCE40 FPGA Manager");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Marek Vasut]

On 11/07/2016 03:49 AM, Joel Holdsworth wrote:
> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
> and very regular structure, designed for low-cost, high-volume consumer
> and system applications.

[...]

> +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> +				     const char *buf, size_t count)
> +{
> +	struct ice40_fpga_priv *priv = mgr->priv;
> +	struct spi_device *dev = priv->dev;
> +	struct spi_message message;
> +	struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1,
> +		.delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY};

Should be this way for the sake of readability, fix globally:

	struct spi_transfer assert_cs_then_reset_delay = {
		.cs_change	= 1,
		.delay_usecs	= ICE40_SPI_FPGAMGR_RESET_DELAY
	};

Also I believe this could be const.

> +	struct spi_transfer housekeeping_delay_then_release_cs = {
> +		.delay_usecs = ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY};

Don't we have some less hacky way of toggling the nCS ? Is this even nCS
or is this some control pin of the FPGA ? Maybe it should be treated
like a regular GPIO instead ?

[...]

> +static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr, u32 flags)
> +{
> +	struct ice40_fpga_priv *priv = mgr->priv;
> +	struct spi_device *dev = priv->dev;
> +	const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,};

The comma is not needed.

> +
> +	/* Check CDONE is asserted */
> +	if (!gpiod_get_value(priv->cdone)) {
> +		dev_err(&dev->dev,
> +			"CDONE was not asserted after firmware transfer\n");
> +		return -EIO;
> +	}
> +
> +	/* Send of zero-padding to activate the firmware */
> +	return spi_write(dev, padding, sizeof(padding));
> +}

[...]

> +	/* Check board setup data. */
> +	if (spi->max_speed_hz > 25000000) {
> +		dev_err(dev, "Speed is too high\n");
> +		return -EINVAL;
> +	}
> +
> +	if (spi->max_speed_hz < 1000000) {
> +		dev_err(dev, "Speed is too low\n");
> +		return -EINVAL;
> +	}

Do you have some explanation for this limitation ?

[...]

-- 
Best regards,
Marek Vasut

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Joel Holdsworth]

Hi Marek,

Thanks again for your comments.


On 07/11/16 11:01, Marek Vasut wrote:
> On 11/07/2016 03:49 AM, Joel Holdsworth wrote:
>> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
>> and very regular structure, designed for low-cost, high-volume consumer
>> and system applications.
>
> [...]
>
>> +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>> +				     const char *buf, size_t count)
>> +{
>> +	struct ice40_fpga_priv *priv = mgr->priv;
>> +	struct spi_device *dev = priv->dev;
>> +	struct spi_message message;
>> +	struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1,
>> +		.delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY};
>
> Should be this way for the sake of readability, fix globally:
>
> 	struct spi_transfer assert_cs_then_reset_delay = {
> 		.cs_change	= 1,
> 		.delay_usecs	= ICE40_SPI_FPGAMGR_RESET_DELAY
> 	};

Sure ok. Personally, I prefer it to be concise, but I'm happy to accept 
the norms.

>
> Also I believe this could be const.

It doesn't work const - I tried it. spi_message_add_tail() expects it to 
be non-const. Looking at 'struct spi_transfer' it appears there is 
various bits of state used to perform the transfer, as well as the 
pointer to the next item in the single-linked list.


>
>> +	struct spi_transfer housekeeping_delay_then_release_cs = {
>> +		.delay_usecs = ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY};
>
> Don't we have some less hacky way of toggling the nCS ? Is this even nCS
> or is this some control pin of the FPGA ? Maybe it should be treated
> like a regular GPIO instead ?

I've been round this loop before also. drivers/spi/spi.c has a static 
function 'static void spi_set_cs(struct spi_device *dev, bool enable)'. 
It manipulates the CS line of devices where CS is built into the SPI 
master, and manipulates the GPIO on other devices.

I don't know why it's non-public - I tried to get an answer from the SPI 
folks, but didn't get one. I guess they don't want to encourage drivers 
to manually manipulate the CS line - because SPI transfers are usually 
meant to be interruptible by higher priority transfers to other devices 
at any time. The only reason it's legit for me to manually manipulate CS 
is because I first lock the bus.

Previously I had a copy of spi_set_cs copy-pasted into my driver, but in 
the end I decided to replace that with the zero-length transfers because 
there's a danger that if the original spi_set_cs() gets rewritten some 
time, my copy-paste code would leave around some nasty legacy.

On the whole, I don't think the zero-length transfers are too 
egregiously bad, and all the alternatives seem worse to me.



>> +	const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,};
>
> The comma is not needed.

True. I'll make that change.


>> +	/* Check board setup data. */
>> +	if (spi->max_speed_hz > 25000000) {
>> +		dev_err(dev, "Speed is too high\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (spi->max_speed_hz < 1000000) {
>> +		dev_err(dev, "Speed is too low\n");
>> +		return -EINVAL;
>> +	}
>
> Do you have some explanation for this limitation ?
>

Not really no.

The 'DS1040 - iCE40 LP/HX Family Data Sheet' page 3-18 claims f_max for 
Slave SPI Writing is >=1MHz && <=25MHz.

The exact reason I don't know.

Are they running a PLL off the clock? What if the clock is really 
jittery - it seems to work fine when I've tested it with bit-banged SPI 
on the RPi; just as well as with hardware SPI.

Or is it something to do with some pre-commit on-chip firmware storage? 
e.g. to check the CRC. Does the storage buffer have some time limitation 
before it gets committed to the FPGA core?

I'm not sure, so I decided to just reflect the datasheet instructions 
back to the user.


Thanks
Joel

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Marek Vasut]

On 11/07/2016 07:49 PM, Joel Holdsworth wrote:
> Hi Marek,

Hi,

> Thanks again for your comments.
> 
> 
> On 07/11/16 11:01, Marek Vasut wrote:
>> On 11/07/2016 03:49 AM, Joel Holdsworth wrote:
>>> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
>>> and very regular structure, designed for low-cost, high-volume consumer
>>> and system applications.
>>
>> [...]
>>
>>> +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32
>>> flags,
>>> +                     const char *buf, size_t count)
>>> +{
>>> +    struct ice40_fpga_priv *priv = mgr->priv;
>>> +    struct spi_device *dev = priv->dev;
>>> +    struct spi_message message;
>>> +    struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1,
>>> +        .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY};
>>
>> Should be this way for the sake of readability, fix globally:
>>
>>     struct spi_transfer assert_cs_then_reset_delay = {
>>         .cs_change    = 1,
>>         .delay_usecs    = ICE40_SPI_FPGAMGR_RESET_DELAY
>>     };
> 
> Sure ok. Personally, I prefer it to be concise, but I'm happy to accept
> the norms.

I prefer it to be readable :)

>> Also I believe this could be const.
> 
> It doesn't work const - I tried it. spi_message_add_tail() expects it to
> be non-const. Looking at 'struct spi_transfer' it appears there is
> various bits of state used to perform the transfer, as well as the
> pointer to the next item in the single-linked list.

Ah, right.

>>
>>> +    struct spi_transfer housekeeping_delay_then_release_cs = {
>>> +        .delay_usecs = ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY};
>>
>> Don't we have some less hacky way of toggling the nCS ? Is this even nCS
>> or is this some control pin of the FPGA ? Maybe it should be treated
>> like a regular GPIO instead ?
> 
> I've been round this loop before also. drivers/spi/spi.c has a static
> function 'static void spi_set_cs(struct spi_device *dev, bool enable)'.
> It manipulates the CS line of devices where CS is built into the SPI
> master, and manipulates the GPIO on other devices.
> 
> I don't know why it's non-public - I tried to get an answer from the SPI
> folks, but didn't get one. I guess they don't want to encourage drivers
> to manually manipulate the CS line - because SPI transfers are usually
> meant to be interruptible by higher priority transfers to other devices
> at any time. The only reason it's legit for me to manually manipulate CS
> is because I first lock the bus.
> 
> Previously I had a copy of spi_set_cs copy-pasted into my driver, but in
> the end I decided to replace that with the zero-length transfers because
> there's a danger that if the original spi_set_cs() gets rewritten some
> time, my copy-paste code would leave around some nasty legacy.
> 
> On the whole, I don't think the zero-length transfers are too
> egregiously bad, and all the alternatives seem worse to me.

So why not turn the CS line into GPIO and just toggle the GPIO?

>>> +    const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,};
>>
>> The comma is not needed.
> 
> True. I'll make that change.
> 
> 
>>> +    /* Check board setup data. */
>>> +    if (spi->max_speed_hz > 25000000) {
>>> +        dev_err(dev, "Speed is too high\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (spi->max_speed_hz < 1000000) {
>>> +        dev_err(dev, "Speed is too low\n");
>>> +        return -EINVAL;
>>> +    }
>>
>> Do you have some explanation for this limitation ?
>>
> 
> Not really no.
> 
> The 'DS1040 - iCE40 LP/HX Family Data Sheet' page 3-18 claims f_max for
> Slave SPI Writing is >=1MHz && <=25MHz.
> 
> The exact reason I don't know.

OK

> Are they running a PLL off the clock? What if the clock is really
> jittery - it seems to work fine when I've tested it with bit-banged SPI
> on the RPi; just as well as with hardware SPI.
> 
> Or is it something to do with some pre-commit on-chip firmware storage?
> e.g. to check the CRC. Does the storage buffer have some time limitation
> before it gets committed to the FPGA core?
> 
> I'm not sure, so I decided to just reflect the datasheet instructions
> back to the user.

Sounds good, thanks.

-- 
Best regards,
Marek Vasut

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Moritz Fischer]

Marek,

On Mon, Nov 7, 2016 at 10:53 AM, Marek Vasut <marex@denx.de> wrote:

>> On the whole, I don't think the zero-length transfers are too
>> egregiously bad, and all the alternatives seem worse to me.
>
> So why not turn the CS line into GPIO and just toggle the GPIO?

Does that work with *all* SPI controllers?

Cheers,

Moritz

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Joel Holdsworth]

>>> On the whole, I don't think the zero-length transfers are too
>>> egregiously bad, and all the alternatives seem worse to me.
>>
>> So why not turn the CS line into GPIO and just toggle the GPIO?
>
> Does that work with *all* SPI controllers?
>

It does not - no. See my other email.

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Marek Vasut]

On 11/08/2016 06:30 PM, Joel Holdsworth wrote:
>>>> On the whole, I don't think the zero-length transfers are too
>>>> egregiously bad, and all the alternatives seem worse to me.
>>>
>>> So why not turn the CS line into GPIO and just toggle the GPIO?
>>
>> Does that work with *all* SPI controllers?
>>
> 
> It does not - no. See my other email.

And is that line an actual CS of that lattice chip or a generic input
which almost works like CS?

-- 
Best regards,
Marek Vasut

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Joel Holdsworth]

On 09/11/16 05:01, Marek Vasut wrote:
> On 11/08/2016 06:30 PM, Joel Holdsworth wrote:
>>>>> On the whole, I don't think the zero-length transfers are too
>>>>> egregiously bad, and all the alternatives seem worse to me.
>>>>
>>>> So why not turn the CS line into GPIO and just toggle the GPIO?
>>>
>>> Does that work with *all* SPI controllers?
>>>
>>
>> It does not - no. See my other email.
>
> And is that line an actual CS of that lattice chip or a generic input
> which almost works like CS?
>

I mean a generic output vs. a special CS output built into the SPI 
master of the application processor. Take a look at how spi_set_cs(..) 
works:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c?id=refs/tags/v4.9-rc4#n695

static void spi_set_cs(struct spi_device *spi, bool enable)
{
	if (spi->mode & SPI_CS_HIGH)
		enable = !enable;

	if (gpio_is_valid(spi->cs_gpio))
		gpio_set_value(spi->cs_gpio, !enable);
	else if (spi->master->set_cs)
		spi->master->set_cs(spi, !enable);
}

So on some SPI masters, spi->master->set_cs is handled separately from 
normal GPIOs. Hence why I want to use this machinery, rather than doing 
it with a GPIO.

Joel

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Marek Vasut]

On 11/09/2016 07:37 PM, Joel Holdsworth wrote:
> On 09/11/16 05:01, Marek Vasut wrote:
>> On 11/08/2016 06:30 PM, Joel Holdsworth wrote:
>>>>>> On the whole, I don't think the zero-length transfers are too
>>>>>> egregiously bad, and all the alternatives seem worse to me.
>>>>>
>>>>> So why not turn the CS line into GPIO and just toggle the GPIO?
>>>>
>>>> Does that work with *all* SPI controllers?
>>>>
>>>
>>> It does not - no. See my other email.
>>
>> And is that line an actual CS of that lattice chip or a generic input
>> which almost works like CS?
>>
> 
> I mean a generic output vs. a special CS output built into the SPI
> master of the application processor. Take a look at how spi_set_cs(..)
> works:

No. I am asking whether the signal which is INPUT on the iCE40 side is
really a chipselect signal for the SPI bus OR something which mostly
behaves/looks like a chipselect but is not really a chipselect.

> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c?id=refs/tags/v4.9-rc4#n695
> 
> 
> static void spi_set_cs(struct spi_device *spi, bool enable)
> {
>     if (spi->mode & SPI_CS_HIGH)
>         enable = !enable;
> 
>     if (gpio_is_valid(spi->cs_gpio))
>         gpio_set_value(spi->cs_gpio, !enable);
>     else if (spi->master->set_cs)
>         spi->master->set_cs(spi, !enable);
> }
> 
> So on some SPI masters, spi->master->set_cs is handled separately from
> normal GPIOs. Hence why I want to use this machinery, rather than doing
> it with a GPIO.

This is not relevant. FYI: using separate GPIO as a SPI chip select has
it's own problems.

-- 
Best regards,
Marek Vasut

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Joel Holdsworth]

On 09/11/16 11:39, Marek Vasut wrote:
> On 11/09/2016 07:37 PM, Joel Holdsworth wrote:
>> On 09/11/16 05:01, Marek Vasut wrote:
>>> On 11/08/2016 06:30 PM, Joel Holdsworth wrote:
>>>>>>> On the whole, I don't think the zero-length transfers are too
>>>>>>> egregiously bad, and all the alternatives seem worse to me.
>>>>>>
>>>>>> So why not turn the CS line into GPIO and just toggle the GPIO?
>>>>>
>>>>> Does that work with *all* SPI controllers?
>>>>>
>>>>
>>>> It does not - no. See my other email.
>>>
>>> And is that line an actual CS of that lattice chip or a generic input
>>> which almost works like CS?
>>>
>>
>> I mean a generic output vs. a special CS output built into the SPI
>> master of the application processor. Take a look at how spi_set_cs(..)
>> works:
>
> No. I am asking whether the signal which is INPUT on the iCE40 side is
> really a chipselect signal for the SPI bus OR something which mostly
> behaves/looks like a chipselect but is not really a chipselect.

Oh I see. The SS_B line is the SPI SlaveSelect for the configuration port.

This is the text from the datasheet:

"SPI Slave Select. Active Low. Includes an internal weak pull-up 
resistor to VCC_SPI during configuration. During configuration, the 
logic level sampled on this pin deter-mines the configuration mode used 
by the iCE40 device. An input when sampled at the start of 
configuration. An input when in SPI Peripheral configuration mode 
(SPI_SS_B = Low). An output when in Master SPI Flash configuration mode."

So yes - it is a "real" SPI chip-select line.

Joel

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Marek Vasut]

On 11/09/2016 07:54 PM, Joel Holdsworth wrote:
> On 09/11/16 11:39, Marek Vasut wrote:
>> On 11/09/2016 07:37 PM, Joel Holdsworth wrote:
>>> On 09/11/16 05:01, Marek Vasut wrote:
>>>> On 11/08/2016 06:30 PM, Joel Holdsworth wrote:
>>>>>>>> On the whole, I don't think the zero-length transfers are too
>>>>>>>> egregiously bad, and all the alternatives seem worse to me.
>>>>>>>
>>>>>>> So why not turn the CS line into GPIO and just toggle the GPIO?
>>>>>>
>>>>>> Does that work with *all* SPI controllers?
>>>>>>
>>>>>
>>>>> It does not - no. See my other email.
>>>>
>>>> And is that line an actual CS of that lattice chip or a generic input
>>>> which almost works like CS?
>>>>
>>>
>>> I mean a generic output vs. a special CS output built into the SPI
>>> master of the application processor. Take a look at how spi_set_cs(..)
>>> works:
>>
>> No. I am asking whether the signal which is INPUT on the iCE40 side is
>> really a chipselect signal for the SPI bus OR something which mostly
>> behaves/looks like a chipselect but is not really a chipselect.
> 
> Oh I see. The SS_B line is the SPI SlaveSelect for the configuration port.
> 
> This is the text from the datasheet:
> 
> "SPI Slave Select. Active Low. Includes an internal weak pull-up
> resistor to VCC_SPI during configuration. During configuration, the
> logic level sampled on this pin deter-mines the configuration mode used
> by the iCE40 device. An input when sampled at the start of
> configuration. An input when in SPI Peripheral configuration mode
> (SPI_SS_B = Low). An output when in Master SPI Flash configuration mode."
> 
> So yes - it is a "real" SPI chip-select line.

OK, thanks for checking.

-- 
Best regards,
Marek Vasut

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Joel Holdsworth]

Hi Marek,

>>> Should be this way for the sake of readability, fix globally:
>>>
>>>     struct spi_transfer assert_cs_then_reset_delay = {
>>>         .cs_change    = 1,
>>>         .delay_usecs    = ICE40_SPI_FPGAMGR_RESET_DELAY
>>>     };
>>
>> Sure ok. Personally, I prefer it to be concise, but I'm happy to accept
>> the norms.
>
> I prefer it to be readable :)
>

I'll conform to what you're saying.

But I just want to point out that this...


spi_message_add_tail(&(struct spi_transfer){.cs_change = 1,
	.delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY}, &message);


...is clearly more readable than this...


struct spi_transfer assert_cs_then_reset_delay = {
     .cs_change    = 1,
     .delay_usecs  = ICE40_SPI_FPGAMGR_RESET_DELAY
};

....
<30 lines of unrelated code>
....

spi_message_add_tail(&assert_cs_then_reset_delay, &message);



...in my opinion anyway ;)



>> Previously I had a copy of spi_set_cs copy-pasted into my driver, but in
>> the end I decided to replace that with the zero-length transfers because
>> there's a danger that if the original spi_set_cs() gets rewritten some
>> time, my copy-paste code would leave around some nasty legacy.
>>
>> On the whole, I don't think the zero-length transfers are too
>> egregiously bad, and all the alternatives seem worse to me.
>
> So why not turn the CS line into GPIO and just toggle the GPIO?
>

Two reasons.

1. On some devices the CS line is built into the SPI master, rather than 
being a normal GPIO.

2. The SPI driver stack addresses SPI devices in terms of which CS line 
they are attached to. You can't have an spi_device in the kernel where 
the SPI driver machinery doesn't have a CS line to control. Moreover it 
needs to be possible for another SPI device to interrupt a running 
transfer to the FPGA. Supporting this involves the SPI framework 
managing the CS line.



Thanks
Joel

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Moritz Fischer]

Hi Joel,

couple of nits inline below:

On Sun, Nov 6, 2016 at 6:49 PM, Joel Holdsworth
<joel@airwebreathe.org.uk> wrote:
> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
> and very regular structure, designed for low-cost, high-volume consumer
> and system applications.
>
> This patch adds support to the FPGA manager for configuring the SRAM of
> iCE40LM, iCE40LP, iCE40HX, iCE40 Ultra, iCE40 UltraLite and iCE40
> UltraPlus devices, through slave SPI.
>
> The iCE40 family is notable because it is the first FPGA family to have
> complete reverse engineered bit-stream documentation for the iCE40LP and
> iCE40HX devices. Furthermore, there is now a Free Software Verilog
> synthesis tool-chain: the "IceStorm" tool-chain.
>
> This project is the work of Clifford Wolf, who is the maintainer of
> Yosys Verilog RTL synthesis framework, and Mathias Lasser, with notable
> contributions from "Cotton Seed", the main author of "arachne-pnr"; a
> place-and-route tool for iCE40 FPGAs.
>
> Having a Free Software synthesis tool-chain offers interesting
> opportunities for embedded devices that are able reconfigure themselves
> with open firmware that is generated on the device itself. For example
> a mobile device might have an application processor with an iCE40 FPGA
> attached, which implements slave devices, or through which the processor
> communicates with other devices through the FPGA fabric.
>
> A kernel driver for the iCE40 is useful, because in some cases, the FPGA
> may need to be configured before other devices can be accessed.
>
> An example of such a device is the icoBoard; a RaspberryPI HAT which
> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
> Digilent-compatible PMOD modules. A PMOD module may contain a device
> with which the kernel communicates, via the FPGA.

Personally I find this a bit verbose, but that's just me.

>
> Signed-off-by: Joel Holdsworth <joel@airwebreathe.org.uk>
> ---
>  drivers/fpga/Kconfig     |   6 ++
>  drivers/fpga/Makefile    |   1 +
>  drivers/fpga/ice40-spi.c | 217 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 224 insertions(+)
>  create mode 100644 drivers/fpga/ice40-spi.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index d614102..5b0f137 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>
>  if FPGA
>
> +config FPGA_MGR_ICE40_SPI
> +       tristate "Lattice iCE40 SPI"
> +       depends on OF && SPI
> +       help
> +         FPGA manager driver support for Lattice iCE40 FPGAs 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..adb5811 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_ICE40_SPI)       += ice40-spi.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)         += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)       += zynq-fpga.o
> diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c
> new file mode 100644
> index 0000000..7d7595b
> --- /dev/null
> +++ b/drivers/fpga/ice40-spi.c
> @@ -0,0 +1,217 @@
> +/*
> + * FPGA Manager Driver for Lattice iCE40.
> + *
> + *  Copyright (c) 2016 Joel Holdsworth
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This driver adds support to the FPGA manager for configuring the SRAM of
> + * Lattice iCE40 FPGAs through slave SPI.
> + */
> +
> +#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>
> +
> +#define ICE40_SPI_FPGAMGR_RESET_DELAY 1 /* us (>200ns) */
> +#define ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY 1200 /* us */
> +
> +#define ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES DIV_ROUND_UP(49, 8)
> +
> +struct ice40_fpga_priv {
> +       struct spi_device *dev;
> +       struct gpio_desc *reset;
> +       struct gpio_desc *cdone;
> +};
> +
> +static enum fpga_mgr_states ice40_fpga_ops_state(struct fpga_manager *mgr)
> +{
> +       struct ice40_fpga_priv *priv = mgr->priv;
> +
> +       return gpiod_get_value(priv->cdone) ? FPGA_MGR_STATE_OPERATING :
> +               FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> +                                    const char *buf, size_t count)
> +{
> +       struct ice40_fpga_priv *priv = mgr->priv;
> +       struct spi_device *dev = priv->dev;
> +       struct spi_message message;
> +       struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1,
> +               .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY};

Formatting looks odd, can you move the .cs_change to the next line?

> +       struct spi_transfer housekeeping_delay_then_release_cs = {
> +               .delay_usecs = ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY};
> +       int ret;
> +
> +       if ((flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> +               dev_err(&dev->dev,
> +                       "Partial reconfiguration is not supported\n");
> +               return -ENOTSUPP;
> +       }
> +
> +       /* Lock the bus, assert CRESET_B and SS_B and delay >200ns */
> +       spi_bus_lock(dev->master);
> +
> +       gpiod_set_value(priv->reset, 1);
> +
> +       spi_message_init(&message);
> +       spi_message_add_tail(&assert_cs_then_reset_delay, &message);
> +       ret = spi_sync_locked(dev, &message);
> +
> +       /* Come out of reset */
> +       gpiod_set_value(priv->reset, 0);
> +
> +       /* Abort if the chip-select failed */
> +       if (ret)
> +               goto fail;
> +
> +       /* Check CDONE is de-asserted i.e. the FPGA is reset */
> +       if (gpiod_get_value(priv->cdone)) {
> +               dev_err(&dev->dev, "Device reset failed, CDONE is asserted\n");
> +               ret = -EIO;
> +               goto fail;
> +       }
> +
> +       /* Wait for the housekeeping to complete, and release SS_B */
> +       spi_message_init(&message);
> +       spi_message_add_tail(&housekeeping_delay_then_release_cs, &message);
> +       ret = spi_sync_locked(dev, &message);
> +
> +fail:
> +       spi_bus_unlock(dev->master);
> +
> +       return ret;
> +}
> +
> +static int ice40_fpga_ops_write(struct fpga_manager *mgr,
> +                               const char *buf, size_t count)
> +{
> +       struct ice40_fpga_priv *priv = mgr->priv;
> +
> +       return spi_write(priv->dev, buf, count);
> +}
> +
> +static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr, u32 flags)
> +{
> +       struct ice40_fpga_priv *priv = mgr->priv;
> +       struct spi_device *dev = priv->dev;
> +       const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,};
> +
> +       /* Check CDONE is asserted */
> +       if (!gpiod_get_value(priv->cdone)) {
> +               dev_err(&dev->dev,
> +                       "CDONE was not asserted after firmware transfer\n");
> +               return -EIO;
> +       }
> +
> +       /* Send of zero-padding to activate the firmware */
> +       return spi_write(dev, padding, sizeof(padding));

I'd move all of these into the write() callback.

> +}
> +
> +static void ice40_fpga_ops_fpga_remove(struct fpga_manager *mgr)
> +{
> +       struct ice40_fpga_priv *priv = mgr->priv;
> +
> +       /* Enter reset */
> +       gpiod_set_value(priv->reset, 1);

I know Marek had suggested this, none of the other drivers behave like that.
I'm not sure this is expected behavior for most people.

> +}
> +
> +static const struct fpga_manager_ops ice40_fpga_ops = {
> +       .state = ice40_fpga_ops_state,
> +       .write_init = ice40_fpga_ops_write_init,
> +       .write = ice40_fpga_ops_write,
> +       .write_complete = ice40_fpga_ops_write_complete,
> +       .fpga_remove = ice40_fpga_ops_fpga_remove
> +};
> +
> +static int ice40_fpga_probe(struct spi_device *spi)
> +{
> +       struct device *dev = &spi->dev;
> +       struct device_node *np = spi->dev.of_node;
> +       struct ice40_fpga_priv *priv;
> +       int ret;
> +
> +       if (!np) {
> +               dev_err(dev, "No Device Tree entry\n");
> +               return -EINVAL;
> +       }
> +
> +       priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->dev = spi;
> +
> +       /* Check board setup data. */
> +       if (spi->max_speed_hz > 25000000) {
> +               dev_err(dev, "Speed is too high\n");
> +               return -EINVAL;
> +       }
> +
> +       if (spi->max_speed_hz < 1000000) {
> +               dev_err(dev, "Speed is too low\n");
> +               return -EINVAL;
> +       }
> +
> +       if (spi->mode & SPI_CPHA) {
> +               dev_err(dev, "Bad mode\n");
> +               return -EINVAL;
> +       }
> +
> +       /* Set up the GPIOs */
> +       priv->cdone = devm_gpiod_get(dev, "cdone", GPIOD_IN);
> +       if (IS_ERR(priv->cdone)) {
> +               dev_err(dev, "Failed to get CDONE GPIO: %ld\n",
> +                       PTR_ERR(priv->cdone));
> +               return -EINVAL;
> +       }
> +
> +       priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(priv->reset)) {
> +               dev_err(dev, "Failed to get CRESET_B GPIO: %ld\n",
> +                       PTR_ERR(priv->reset));
> +               return -EINVAL;
> +       }
> +
> +       /* Register with the FPGA manager */
> +       ret = fpga_mgr_register(dev, "Lattice iCE40 FPGA Manager",
> +                               &ice40_fpga_ops, priv);
> +       if (ret) {
> +               dev_err(dev, "Unable to register FPGA manager");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ice40_fpga_remove(struct spi_device *spi)
> +{
> +       fpga_mgr_unregister(&spi->dev);
> +       return 0;
> +}
> +
> +static const struct of_device_id ice40_fpga_of_match[] = {
> +       { .compatible = "lattice,ice40-fpga-mgr", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, ice40_fpga_of_match);
> +
> +static struct spi_driver ice40_fpga_driver = {
> +       .probe = ice40_fpga_probe,
> +       .remove = ice40_fpga_remove,
> +       .driver = {
> +               .name = "ice40spi",
> +               .of_match_table = of_match_ptr(ice40_fpga_of_match),
> +       },
> +};
> +
> +module_spi_driver(ice40_fpga_driver);
> +
> +MODULE_AUTHOR("Joel Holdsworth <joel@airwebreathe.org.uk>");
> +MODULE_DESCRIPTION("Lattice iCE40 FPGA Manager");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Joel Holdsworth]

Hi Moritz - thanks for your comments.


>> An example of such a device is the icoBoard; a RaspberryPI HAT which
>> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
>> Digilent-compatible PMOD modules. A PMOD module may contain a device
>> with which the kernel communicates, via the FPGA.
>
> Personally I find this a bit verbose, but that's just me.

I could cut it down a bit if you want. I was just trying to make the 
case for why this *has* to be in the kernel rather than just being done 
from user-space.


>> +       struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1,
>> +               .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY};
>
> Formatting looks odd, can you move the .cs_change to the next line?

Marek made the same comment. I'll make the change.


>> +static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr, u32 flags)
>> +{
>> +       struct ice40_fpga_priv *priv = mgr->priv;
>> +       struct spi_device *dev = priv->dev;
>> +       const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,};
>> +
>> +       /* Check CDONE is asserted */
>> +       if (!gpiod_get_value(priv->cdone)) {
>> +               dev_err(&dev->dev,
>> +                       "CDONE was not asserted after firmware transfer\n");
>> +               return -EIO;
>> +       }
>> +
>> +       /* Send of zero-padding to activate the firmware */
>> +       return spi_write(dev, padding, sizeof(padding));
>
> I'd move all of these into the write() callback.

Is that correct? I was under the impression that write might be called 
multiple times to load firmware in multiple chunks.



>> +       /* Enter reset */
>> +       gpiod_set_value(priv->reset, 1);
>
> I know Marek had suggested this, none of the other drivers behave like that.
> I'm not sure this is expected behavior for most people.

For me it's not a big deal either way, but I think it's quite good for 
the driver to reset the device.

When you remove most other drivers, you expect the driver to shut the 
device down, so isn't this just the same?

...but then the FPGA manager is a unique best with its own conventions, 
so perhaps it should behave differently.

How can we get a consensus about this?


Joel

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

[Moritz Fischer]

Hi Joel,

On Mon, Nov 7, 2016 at 11:02 AM, Joel Holdsworth
<joel@airwebreathe.org.uk> wrote:
> Hi Moritz - thanks for your comments.
>
>
>>> An example of such a device is the icoBoard; a RaspberryPI HAT which
>>> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
>>> Digilent-compatible PMOD modules. A PMOD module may contain a device
>>> with which the kernel communicates, via the FPGA.
>>
>>
>> Personally I find this a bit verbose, but that's just me.
>
>
> I could cut it down a bit if you want. I was just trying to make the case
> for why this *has* to be in the kernel rather than just being done from
> user-space.
>
>
>>> +       struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1,
>>> +               .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY};
>>
>>
>> Formatting looks odd, can you move the .cs_change to the next line?
>
>
> Marek made the same comment. I'll make the change.
>
>
>>> +static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr, u32
>>> flags)
>>> +{
>>> +       struct ice40_fpga_priv *priv = mgr->priv;
>>> +       struct spi_device *dev = priv->dev;
>>> +       const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,};
>>> +
>>> +       /* Check CDONE is asserted */
>>> +       if (!gpiod_get_value(priv->cdone)) {
>>> +               dev_err(&dev->dev,
>>> +                       "CDONE was not asserted after firmware
>>> transfer\n");
>>> +               return -EIO;
>>> +       }
>>> +
>>> +       /* Send of zero-padding to activate the firmware */
>>> +       return spi_write(dev, padding, sizeof(padding));
>>
>>
>> I'd move all of these into the write() callback.
>
>
> Is that correct? I was under the impression that write might be called
> multiple times to load firmware in multiple chunks.

Nah, you're right. Don't listen to me :)
>
>
>>> +       /* Enter reset */
>>> +       gpiod_set_value(priv->reset, 1);
>>
>>
>> I know Marek had suggested this, none of the other drivers behave like
>> that.
>> I'm not sure this is expected behavior for most people.
>
>
> For me it's not a big deal either way, but I think it's quite good for the
> driver to reset the device.
>
> When you remove most other drivers, you expect the driver to shut the device
> down, so isn't this just the same?
>
> ...but then the FPGA manager is a unique best with its own conventions, so
> perhaps it should behave differently.
>
> How can we get a consensus about this?

Fair enough. I don't mind either way all that much, just thought I'd
bring it up.

Thanks,

Moritz