| Message ID | 1477283989-21947-2-git-send-email-joel@airwebreathe.org.uk |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, 24 Oct 2016, 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. > > 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 processer 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. > --- > .../bindings/fpga/lattice-ice40-fpga-mgr.txt | 23 +++ > drivers/fpga/Kconfig | 6 + > drivers/fpga/Makefile | 1 + > drivers/fpga/ice40spi.c | 212 +++++++++++++++++++++ > 4 files changed, 242 insertions(+) > create mode 100644 Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt > create mode 100644 drivers/fpga/ice40spi.c > Hi Joel, Thanks for submitting your driver! I didn't see any huge problems, just minor things below... Alan > 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..b253ac8 > --- /dev/null > +++ b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt > @@ -0,0 +1,23 @@ > +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 connected to CDONE pin > +- creset_b-gpios: GPIO connected to CRESET_B pin. Note that CRESET_B is > + treated as an active-low output because the signal is > + treated as an enable signal, rather than a reset. This > + is necessary because the FPGA will enter Master SPI > + mode and drive SCK with a clock signal, potentially > + jamming other devices on the bus, unless CRESET_B is > + held high until the firmware is loaded. Both could be singular ...-gpio since only one gpio should be specified. > + > +Example: > + ice40: ice40@0 { > + compatible = "lattice,ice40-fpga-mgr"; > + reg = <0>; > + spi-max-frequency = <1000000>; > + cdone-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>; > + creset_b-gpios = <&gpio 22 GPIO_ACTIVE_LOW>; > + }; > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > index d614102..85ff429 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 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..6c809cc 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) += ice40spi.o Could this be ice40-spi.c? > obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o > obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o > diff --git a/drivers/fpga/ice40spi.c b/drivers/fpga/ice40spi.c > new file mode 100644 > index 0000000..ab5ee86 > --- /dev/null > +++ b/drivers/fpga/ice40spi.c > @@ -0,0 +1,212 @@ > +/* > + * 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/delay.h> > +#include <linux/fpga/fpga-mgr.h> > +#include <linux/gpio/consumer.h> > +#include <linux/of_gpio.h> > +#include <linux/spi/spi.h> > + > +struct ice40_fpga_priv { > + struct spi_device *dev; > + struct gpio_desc *creset_b; > + struct gpio_desc *cdone; > + enum fpga_mgr_states state; state is never used. You could just remove it. > +}; > + > +static enum fpga_mgr_states ice40_fpga_ops_state(struct fpga_manager *mgr) > +{ > + return mgr->state; fpga_mgr_register will call your ice40_fpga_ops_state() function to get its initial state. That's the only time this gets called. So you could return one of the enum fpga_mgr_states here. I'm guessing FPGA_MGR_STATE_UNKNOWN. I'm realizing that there will be devices that don't really know what initial state they are in; I could have removed the absolute requirement for the state in the fpga_manager_ops and assumed FPGA_MGR_STATE_UNKNOWN unless a low level driver provided a state function. But for now, just return FPGA_MGR_STATE_UNKNOWN here unless you have a way of knowing what state you are in when the driver is probed. > +} > + > +static void set_cs(struct spi_device *spi, bool 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); > +} > + > +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, > + const char *buf, size_t count) Checkpatch complains about alignment here. > +{ > + struct ice40_fpga_priv *priv = mgr->priv; > + struct spi_device *dev = priv->dev; > + int ret; > + > + if ((flags & FPGA_MGR_PARTIAL_RECONFIG)) { > + dev_err(&dev->dev, > + "Partial reconfiguration is not supported\n"); > + return -EINVAL; > + } > + > + /* Lock the bus, assert SS_B and CRESET_B */ > + ret = spi_bus_lock(dev->master); > + if (ret) { > + dev_err(&dev->dev, "Failed to lock SPI bus, ret: %d\n", ret); > + return ret; > + } > + > + set_cs(dev, 1); > + gpiod_set_value(priv->creset_b, 1); > + > + /* Delay for >200ns */ > + udelay(1); > + > + /* Come out of reset */ > + gpiod_set_value(priv->creset_b, 0); > + > + /* 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; > + } > + > + /* Wait for the housekeeping to complete */ > + if (!ret) > + udelay(1200); Would usleep_range work for you since it's more than 10uSec (Documentation/timers/timers-howto.txt)? > + > + /* Release the SS_B */ > + set_cs(dev, 0); > + spi_bus_unlock(dev->master); > + > + return ret; > +} > + > +static int ice40_fpga_ops_write(struct fpga_manager *mgr, > + const char *buf, size_t count) Checkpatch complains about alignment here also. > +{ > + struct ice40_fpga_priv *priv = mgr->priv; > + struct spi_device *dev = priv->dev; > + int ret; > + > + ret = spi_write(dev, buf, count); > + if (ret) > + dev_err(&dev->dev, "Error sending SPI data, ret: %d\n", ret); > + > + return ret; > +} > + > +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; > + int ret = 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 >49-bits of zero-padding to activate the firmware */ > + ret = spi_write(dev, NULL, 7); > + if (ret) { > + dev_err(&dev->dev, "Error sending zero padding, ret: %d\n", > + ret); > + return ret; > + } > + > + /* Success */ > + return 0; > +} > + > +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, > +}; > + > +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; > + } else 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 ret; > + } > + > + priv->creset_b = devm_gpiod_get(dev, "creset_b", GPIOD_OUT_HIGH); > + if (IS_ERR(priv->creset_b)) { > + dev_err(dev, "Failed to get CRESET_B GPIO: %ld\n", > + PTR_ERR(priv->creset_b)); > + return ret; > + } > + > + /* 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; > +} > + #ifdef CONFIG_OF > +static const struct of_device_id ice40_fpga_of_match[] = { > + { .compatible = "lattice,ice40-fpga-mgr", }, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(of, ice40_fpga_of_match); #endif > + > +static struct spi_driver ice40_fpga_driver = { > + .probe = ice40_fpga_probe, > + .remove = ice40_fpga_remove, > + .driver = { > + .name = "ice40spi", > + .owner = THIS_MODULE, It's not necessary to specify .owner anymore. > + .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 > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Joel, Ha, finally someone beat me to submitting my driver, I had an ugly hack to bitbang the SPI since I couldn't figure out a good way to assert the creset after the CS. Thanks! On Mon, Oct 24, 2016 at 04:55:43PM -0500, atull wrote: > On Mon, 24 Oct 2016, 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. > > > > 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 processer with an iCE40 FPGA processer ? :) > > 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. > > --- > > .../bindings/fpga/lattice-ice40-fpga-mgr.txt | 23 +++ > > drivers/fpga/Kconfig | 6 + > > drivers/fpga/Makefile | 1 + > > drivers/fpga/ice40spi.c | 212 +++++++++++++++++++++ > > 4 files changed, 242 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt > > create mode 100644 drivers/fpga/ice40spi.c > > > > Hi Joel, > > Thanks for submitting your driver! > > I didn't see any huge problems, just minor things below... > > Alan > > > 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..b253ac8 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt > > @@ -0,0 +1,23 @@ > > +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 connected to CDONE pin > > +- creset_b-gpios: GPIO connected to CRESET_B pin. Note that CRESET_B is > > + treated as an active-low output because the signal is > > + treated as an enable signal, rather than a reset. This > > + is necessary because the FPGA will enter Master SPI > > + mode and drive SCK with a clock signal, potentially > > + jamming other devices on the bus, unless CRESET_B is > > + held high until the firmware is loaded. > > Both could be singular ...-gpio since only one gpio should be specified. > > > + > > +Example: > > + ice40: ice40@0 { > > + compatible = "lattice,ice40-fpga-mgr"; > > + reg = <0>; > > + spi-max-frequency = <1000000>; > > + cdone-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>; > > + creset_b-gpios = <&gpio 22 GPIO_ACTIVE_LOW>; > > + }; > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > > index d614102..85ff429 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 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..6c809cc 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) += ice40spi.o > > Could this be ice40-spi.c? > > > obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o > > obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o > > diff --git a/drivers/fpga/ice40spi.c b/drivers/fpga/ice40spi.c > > new file mode 100644 > > index 0000000..ab5ee86 > > --- /dev/null > > +++ b/drivers/fpga/ice40spi.c > > @@ -0,0 +1,212 @@ > > +/* > > + * 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/delay.h> > > +#include <linux/fpga/fpga-mgr.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/of_gpio.h> > > +#include <linux/spi/spi.h> > > + > > +struct ice40_fpga_priv { > > + struct spi_device *dev; > > + struct gpio_desc *creset_b; > > + struct gpio_desc *cdone; > > + enum fpga_mgr_states state; > > state is never used. You could just remove it. > > > +}; > > + > > +static enum fpga_mgr_states ice40_fpga_ops_state(struct fpga_manager *mgr) > > +{ > > + return mgr->state; > > fpga_mgr_register will call your ice40_fpga_ops_state() function to > get its initial state. That's the only time this gets called. So > you could return one of the enum fpga_mgr_states here. I'm guessing > FPGA_MGR_STATE_UNKNOWN. I'm realizing that there will be devices > that don't really know what initial state they are in; I could have > removed the absolute requirement for the state in the fpga_manager_ops > and assumed FPGA_MGR_STATE_UNKNOWN unless a low level driver provided > a state function. But for now, just return FPGA_MGR_STATE_UNKNOWN > here unless you have a way of knowing what state you are in when > the driver is probed. You could potentially also just look at the CDONE GPIO. > > > +} > > + > > +static void set_cs(struct spi_device *spi, bool 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); > > +} > > + > > +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, > > + const char *buf, size_t count) > > Checkpatch complains about alignment here. > > > +{ > > + struct ice40_fpga_priv *priv = mgr->priv; > > + struct spi_device *dev = priv->dev; > > + int ret; > > + > > + if ((flags & FPGA_MGR_PARTIAL_RECONFIG)) { > > + dev_err(&dev->dev, > > + "Partial reconfiguration is not supported\n"); > > + return -EINVAL; Maybe -ENOTSUPP ? > > + } > > + > > + /* Lock the bus, assert SS_B and CRESET_B */ > > + ret = spi_bus_lock(dev->master); > > + if (ret) { > > + dev_err(&dev->dev, "Failed to lock SPI bus, ret: %d\n", ret); > > + return ret; > > + } > > + > > + set_cs(dev, 1); > > + gpiod_set_value(priv->creset_b, 1); > > + > > + /* Delay for >200ns */ > > + udelay(1); Named constant? > > + > > + /* Come out of reset */ > > + gpiod_set_value(priv->creset_b, 0); > > + > > + /* 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; > > + } > > + > > + /* Wait for the housekeeping to complete */ > > + if (!ret) > > + udelay(1200); Named constant? > > Would usleep_range work for you since it's more than 10uSec > (Documentation/timers/timers-howto.txt)? > > > + > > + /* Release the SS_B */ > > + set_cs(dev, 0); > > + spi_bus_unlock(dev->master); > > + > > + return ret; > > +} > > + > > +static int ice40_fpga_ops_write(struct fpga_manager *mgr, > > + const char *buf, size_t count) > > Checkpatch complains about alignment here also. > > > +{ > > + struct ice40_fpga_priv *priv = mgr->priv; > > + struct spi_device *dev = priv->dev; > > + int ret; > > + > > + ret = spi_write(dev, buf, count); > > + if (ret) > > + dev_err(&dev->dev, "Error sending SPI data, ret: %d\n", ret); > > + > > + return ret; > > +} > > + > > +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; > > + int ret = 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 >49-bits of zero-padding to activate the firmware */ > > + ret = spi_write(dev, NULL, 7); Could make that a named constant ... > > + if (ret) { > > + dev_err(&dev->dev, "Error sending zero padding, ret: %d\n", > > + ret); > > + return ret; > > + } > > + > > + /* Success */ > > + return 0; > > +} > > + > > +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, > > +}; > > + > > +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; > > + } else 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 ret; > > + } > > + > > + priv->creset_b = devm_gpiod_get(dev, "creset_b", GPIOD_OUT_HIGH); > > + if (IS_ERR(priv->creset_b)) { > > + dev_err(dev, "Failed to get CRESET_B GPIO: %ld\n", > > + PTR_ERR(priv->creset_b)); > > + return ret; > > + } > > + > > + /* 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; > > +} > > + > > #ifdef CONFIG_OF > > +static const struct of_device_id ice40_fpga_of_match[] = { > > + { .compatible = "lattice,ice40-fpga-mgr", }, > > + {}, > > +}; > > + > > +MODULE_DEVICE_TABLE(of, ice40_fpga_of_match); > #endif > > > + > > +static struct spi_driver ice40_fpga_driver = { > > + .probe = ice40_fpga_probe, > > + .remove = ice40_fpga_remove, > > + .driver = { > > + .name = "ice40spi", > > + .owner = THIS_MODULE, > > It's not necessary to specify .owner anymore. > > > + .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 > > > > Cheers, Moritz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/24/2016 04:28 PM, Moritz Fischer wrote: > Hi Joel, > > Ha, finally someone beat me to submitting my driver, > I had an ugly hack to bitbang the SPI since I couldn't figure > out a good way to assert the creset after the CS. > > Thanks! > Hi Moritz - yeah I figured someone might have a driver in the works. I think my set_cs() function is ok-ish. It's copied from spi_set_cs() in drivers/spi/spi.c . This function is a static internal helper, so I copy/pasted the function into the ice40 driver. Given that it's only 4-lines of code, it didn't seem too bad - though I'm not exactly sure why spi_set_cs() isn't a public API. It seems like quite a common-place thing to need to do with certain devices. However, perhaps the function is internal because the authors of the SPI framework foresaw how easy it would be to screw up a shared bus with that function. I had to take care to make sure the SPI bus was locked throughout. Do you agree that it's the right thing to copy the function in? Or do you think it would be better to ask for spi_set_cs to be exposed publicly? Best Regards Joel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Hi Joel, > > Thanks for submitting your driver! > > I didn't see any huge problems, just minor things below... > > Alan > Hi Alan, Thanks for your feedback. I've implemented all your suggestions and I'll resubmit. I had a question about the status of the fpga-manager framework. Is there active work going on? I was wondering if there were any patches for specifying a firmware file from device-tree? and/or if there were any patches for loading firmware from userspace - something like this: "cat firmware.bin > /dev/fpga0"? Only being able to load firmware from kernel C-code is rather a limitation - though I suppose the framework is quite early in development. Thanks Joel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 25 Oct 2016, Joel Holdsworth wrote: > > > Hi Joel, > > > > Thanks for submitting your driver! > > > > I didn't see any huge problems, just minor things below... > > > > Alan > > > > Hi Alan, Thanks for your feedback. I've implemented all your suggestions and > I'll resubmit. > > I had a question about the status of the fpga-manager framework. Is there > active work going on? I was wondering if there were any patches for specifying > a firmware file from device-tree? Yes! I have submitted patches for doing exactly that. About to submit v21. Here's patch 1 of v20: https://patchwork.kernel.org/patch/9379859/ > and/or if there were any patches for loading > firmware from userspace - something like this: "cat firmware.bin > > /dev/fpga0"? We discussed doing a char driver interface. That's where all this started. It met with lots of resistance. > > Only being able to load firmware from kernel C-code is rather a limitation - > though I suppose the framework is quite early in development. > > Thanks > Joel > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan > Yes! I have submitted patches for doing exactly that. About to > submit v21. Here's patch 1 of v20: > > https://patchwork.kernel.org/patch/9379859/ That's really cool. I hope it gets accepted. >> and/or if there were any patches for loading >> firmware from userspace - something like this: "cat firmware.bin > >> /dev/fpga0"? > > We discussed doing a char driver interface. That's where all this > started. It met with lots of resistance. > Yeah I saw some slides about it. Personally I don't care too much about the character device thing. It could be something in sysfs - anything that allows the user to inject firmware firmware from user-space. Joel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 24, 2016 at 11:05:28PM -0600, Joel Holdsworth wrote: > Only being able to load firmware from kernel C-code is rather a limitation - > though I suppose the framework is quite early in development. I agree this is a limitation. To expand a bit on what Alan said. For the last year(?) or so we've been reviewing / reiterating patches to do that using device tree overlays. Rob was ok with the last round of bindings so it looks like it is moving in the right direction. The part that's missing in the whole spiel is a way to apply overlays from userland (which in turn then trigger the reconfiguration of the regions). Pantelis has a patchset that allows doing this using a configfs interface; however the devicetree maintainers weren't too happy with it. At ELCE Pantelis and me talked to Frank about the patchset and Pantelis agreed to resubmit the patchset with small changes. TL;DR we'll eventually get there but it takes time and many rounds of review :) Cheers, Moritz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Joel, On Mon, Oct 24, 2016 at 9:51 PM, Joel Holdsworth <joel@airwebreathe.org.uk> wrote: > I think my set_cs() function is ok-ish. It's copied from spi_set_cs() in > drivers/spi/spi.c . This function is a static internal helper, so I > copy/pasted the function into the ice40 driver. Given that it's only 4-lines > of code, it didn't seem too bad - though I'm not exactly sure why > spi_set_cs() isn't a public API. It seems like quite a common-place thing to > need to do with certain devices. > > However, perhaps the function is internal because the authors of the SPI > framework foresaw how easy it would be to screw up a shared bus with that > function. I had to take care to make sure the SPI bus was locked throughout. > > Do you agree that it's the right thing to copy the function in? Or do you > think it would be better to ask for spi_set_cs to be exposed publicly? I'd poke the SPI maintainers about what their reasoning was to make it non-public, and how they'd go about doing what you're trying to do. I can imagine there might be some SPI controllers where the above doesn't work well, because the controller automatically handles the CS line and you don't get control over it. Cheers, Moritz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I just submitted a version of the driver without copy-pasted chip-select code. The PATCH v4 version uses a pair of zero-byte SPI transfers to control the CS line. I didn't get a response from the linux-spi mailing list, but in my opinion they're probably not going to want spi_set_cs to become a public-internal API, and zero-byte transfers have the desired effect. Thanks Joel On 10/25/2016 10:48 AM, Moritz Fischer wrote: > Hi Joel, > > On Mon, Oct 24, 2016 at 9:51 PM, Joel Holdsworth > <joel@airwebreathe.org.uk> wrote: > >> I think my set_cs() function is ok-ish. It's copied from spi_set_cs() in >> drivers/spi/spi.c . This function is a static internal helper, so I >> copy/pasted the function into the ice40 driver. Given that it's only 4-lines >> of code, it didn't seem too bad - though I'm not exactly sure why >> spi_set_cs() isn't a public API. It seems like quite a common-place thing to >> need to do with certain devices. >> >> However, perhaps the function is internal because the authors of the SPI >> framework foresaw how easy it would be to screw up a shared bus with that >> function. I had to take care to make sure the SPI bus was locked throughout. >> >> Do you agree that it's the right thing to copy the function in? Or do you >> think it would be better to ask for spi_set_cs to be exposed publicly? > > I'd poke the SPI maintainers about what their reasoning was to make it > non-public, > and how they'd go about doing what you're trying to do. > I can imagine there might be some SPI controllers where the above > doesn't work well, > because the controller automatically handles the CS line and you don't > get control over it. > > Cheers, > > Moritz > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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..b253ac8 --- /dev/null +++ b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt @@ -0,0 +1,23 @@ +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 connected to CDONE pin +- creset_b-gpios: GPIO connected to CRESET_B pin. Note that CRESET_B is + treated as an active-low output because the signal is + treated as an enable signal, rather than a reset. This + is necessary because the FPGA will enter Master SPI + mode and drive SCK with a clock signal, potentially + jamming other devices on the bus, unless CRESET_B is + held high 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>; + creset_b-gpios = <&gpio 22 GPIO_ACTIVE_LOW>; + }; diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index d614102..85ff429 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 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..6c809cc 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) += ice40spi.o obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o diff --git a/drivers/fpga/ice40spi.c b/drivers/fpga/ice40spi.c new file mode 100644 index 0000000..ab5ee86 --- /dev/null +++ b/drivers/fpga/ice40spi.c @@ -0,0 +1,212 @@ +/* + * 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/delay.h> +#include <linux/fpga/fpga-mgr.h> +#include <linux/gpio/consumer.h> +#include <linux/of_gpio.h> +#include <linux/spi/spi.h> + +struct ice40_fpga_priv { + struct spi_device *dev; + struct gpio_desc *creset_b; + struct gpio_desc *cdone; + enum fpga_mgr_states state; +}; + +static enum fpga_mgr_states ice40_fpga_ops_state(struct fpga_manager *mgr) +{ + return mgr->state; +} + +static void set_cs(struct spi_device *spi, bool 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); +} + +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; + int ret; + + if ((flags & FPGA_MGR_PARTIAL_RECONFIG)) { + dev_err(&dev->dev, + "Partial reconfiguration is not supported\n"); + return -EINVAL; + } + + /* Lock the bus, assert SS_B and CRESET_B */ + ret = spi_bus_lock(dev->master); + if (ret) { + dev_err(&dev->dev, "Failed to lock SPI bus, ret: %d\n", ret); + return ret; + } + + set_cs(dev, 1); + gpiod_set_value(priv->creset_b, 1); + + /* Delay for >200ns */ + udelay(1); + + /* Come out of reset */ + gpiod_set_value(priv->creset_b, 0); + + /* 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; + } + + /* Wait for the housekeeping to complete */ + if (!ret) + udelay(1200); + + /* Release the SS_B */ + set_cs(dev, 0); + 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; + struct spi_device *dev = priv->dev; + int ret; + + ret = spi_write(dev, buf, count); + if (ret) + dev_err(&dev->dev, "Error sending SPI data, ret: %d\n", ret); + + return ret; +} + +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; + int ret = 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 >49-bits of zero-padding to activate the firmware */ + ret = spi_write(dev, NULL, 7); + if (ret) { + dev_err(&dev->dev, "Error sending zero padding, ret: %d\n", + ret); + return ret; + } + + /* Success */ + return 0; +} + +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, +}; + +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; + } else 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 ret; + } + + priv->creset_b = devm_gpiod_get(dev, "creset_b", GPIOD_OUT_HIGH); + if (IS_ERR(priv->creset_b)) { + dev_err(dev, "Failed to get CRESET_B GPIO: %ld\n", + PTR_ERR(priv->creset_b)); + return ret; + } + + /* 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", + .owner = THIS_MODULE, + .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");