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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
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
This adds documentation of the device tree bindings of the Lattice iCE40
FPGA driver for the FPGA manager framework.
Signed-off-by: Joel Holdsworth <[email protected]>
---
.../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
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 <[email protected]>
---
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 <[email protected]>");
+MODULE_DESCRIPTION("Lattice iCE40 FPGA Manager");
+MODULE_LICENSE("GPL v2");
--
2.7.4
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 <[email protected]>
> ---
> .../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
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
Hi Joel,
couple of nits inline below:
On Sun, Nov 6, 2016 at 6:49 PM, Joel Holdsworth
<[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>");
> +MODULE_DESCRIPTION("Lattice iCE40 FPGA Manager");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
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
>> + 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
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
Hi Joel,
On Mon, Nov 7, 2016 at 11:02 AM, Joel Holdsworth
<[email protected]> 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
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
Marek,
On Mon, Nov 7, 2016 at 10:53 AM, Marek Vasut <[email protected]> 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
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
>>> 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.
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
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
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
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
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
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 <[email protected]>
> ---
> .../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 <[email protected]>
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 <[email protected]>
> > ---
> > .../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 <[email protected]>
>
Acked-by: Alan Tull <[email protected]>
On Fri, Nov 18, 2016 at 10:56 AM, atull <[email protected]> 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 <[email protected]>
>> > ---
>> > .../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 <[email protected]>
>>
>
> Acked-by: Alan Tull <[email protected]>
Acked-by: Moritz Fischer <[email protected]>
On 11/18/2016 08:17 PM, Moritz Fischer wrote:
> On Fri, Nov 18, 2016 at 10:56 AM, atull <[email protected]> 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 <[email protected]>
>>>> ---
>>>> .../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 <[email protected]>
>>>
>>
>> Acked-by: Alan Tull <[email protected]>
> Acked-by: Moritz Fischer <[email protected]>
Acked-by: Marek Vasut <[email protected]>
--
Best regards,
Marek Vasut