Message ID | 1477776746-3678-2-git-send-email-joel@airwebreathe.org.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Joel, [auto build test ERROR on linus/master] [also build test ERROR on v4.9-rc3 next-20161028] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information] url: https://github.com/0day-ci/linux/commits/Joel-Holdsworth/of-Add-vendor-prefix-for-Lattice-Semiconductor/20161030-053525 config: i386-allyesconfig (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): drivers/fpga/ice40-spi.c:203:1: warning: data definition has no type or storage class MODULE_DEVICE_TABLE(of, ice40_fpga_of_match); ^~~~~~~~~~~~~~~~~~~ >> drivers/fpga/ice40-spi.c:203:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int] drivers/fpga/ice40-spi.c:203:1: warning: parameter names (without types) in function declaration drivers/fpga/ice40-spi.c:215:229: warning: data definition has no type or storage class module_spi_driver(ice40_fpga_driver); ^ drivers/fpga/ice40-spi.c:215:229: error: type defaults to 'int' in declaration of 'module_init' [-Werror=implicit-int] drivers/fpga/ice40-spi.c:215:191: warning: parameter names (without types) in function declaration module_spi_driver(ice40_fpga_driver); ^ drivers/fpga/ice40-spi.c:215:492: warning: data definition has no type or storage class module_spi_driver(ice40_fpga_driver); ^ drivers/fpga/ice40-spi.c:215:492: error: type defaults to 'int' in declaration of 'module_exit' [-Werror=implicit-int] drivers/fpga/ice40-spi.c:215:191: warning: parameter names (without types) in function declaration module_spi_driver(ice40_fpga_driver); ^ >> drivers/fpga/ice40-spi.c:217:15: error: expected declaration specifiers or '...' before string constant MODULE_AUTHOR("Joel Holdsworth <joel@airwebreathe.org.uk>"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:218:20: error: expected declaration specifiers or '...' before string constant MODULE_DESCRIPTION("Lattice iCE40 FPGA Manager"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:219:16: error: expected declaration specifiers or '...' before string constant MODULE_LICENSE("GPL v2"); ^~~~~~~~ drivers/fpga/ice40-spi.c:215:122: warning: 'ice40_fpga_driver_init' defined but not used [-Wunused-function] module_spi_driver(ice40_fpga_driver); ^ cc1: some warnings being treated as errors vim +203 drivers/fpga/ice40-spi.c 197 198 #ifdef CONFIG_OF 199 static const struct of_device_id ice40_fpga_of_match[] = { 200 { .compatible = "lattice,ice40-fpga-mgr", }, 201 {}, 202 }; > 203 MODULE_DEVICE_TABLE(of, ice40_fpga_of_match); 204 #endif 205 206 static struct spi_driver ice40_fpga_driver = { 207 .probe = ice40_fpga_probe, 208 .remove = ice40_fpga_remove, 209 .driver = { 210 .name = "ice40spi", 211 .of_match_table = of_match_ptr(ice40_fpga_of_match), 212 }, 213 }; 214 > 215 module_spi_driver(ice40_fpga_driver); 216 > 217 MODULE_AUTHOR("Joel Holdsworth <joel@airwebreathe.org.uk>"); 218 MODULE_DESCRIPTION("Lattice iCE40 FPGA Manager"); 219 MODULE_LICENSE("GPL v2"); --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Joel, [auto build test ERROR on linus/master] [also build test ERROR on v4.9-rc3 next-20161028] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information] url: https://github.com/0day-ci/linux/commits/Joel-Holdsworth/of-Add-vendor-prefix-for-Lattice-Semiconductor/20161030-053525 config: powerpc-allmodconfig (attached as .config) compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=powerpc All errors (new ones prefixed by >>): drivers/fpga/ice40-spi.c:203:1: warning: data definition has no type or storage class MODULE_DEVICE_TABLE(of, ice40_fpga_of_match); ^~~~~~~~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:203:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int] drivers/fpga/ice40-spi.c:203:1: warning: parameter names (without types) in function declaration In file included from include/linux/platform_device.h:14:0, from include/linux/fpga/fpga-mgr.h:19, from drivers/fpga/ice40-spi.c:14: include/linux/device.h:1353:1: warning: data definition has no type or storage class module_init(__driver##_init); \ ^ include/linux/spi/spi.h:290:2: note: in expansion of macro 'module_driver' module_driver(__spi_driver, spi_register_driver, \ ^~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:215:1: note: in expansion of macro 'module_spi_driver' module_spi_driver(ice40_fpga_driver); ^~~~~~~~~~~~~~~~~ >> include/linux/device.h:1353:1: error: type defaults to 'int' in declaration of 'module_init' [-Werror=implicit-int] module_init(__driver##_init); \ ^ include/linux/spi/spi.h:290:2: note: in expansion of macro 'module_driver' module_driver(__spi_driver, spi_register_driver, \ ^~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:215:1: note: in expansion of macro 'module_spi_driver' module_spi_driver(ice40_fpga_driver); ^~~~~~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:215:1: warning: parameter names (without types) in function declaration In file included from include/linux/platform_device.h:14:0, from include/linux/fpga/fpga-mgr.h:19, from drivers/fpga/ice40-spi.c:14: include/linux/device.h:1358:1: warning: data definition has no type or storage class module_exit(__driver##_exit); ^ include/linux/spi/spi.h:290:2: note: in expansion of macro 'module_driver' module_driver(__spi_driver, spi_register_driver, \ ^~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:215:1: note: in expansion of macro 'module_spi_driver' module_spi_driver(ice40_fpga_driver); ^~~~~~~~~~~~~~~~~ >> include/linux/device.h:1358:1: error: type defaults to 'int' in declaration of 'module_exit' [-Werror=implicit-int] module_exit(__driver##_exit); ^ include/linux/spi/spi.h:290:2: note: in expansion of macro 'module_driver' module_driver(__spi_driver, spi_register_driver, \ ^~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:215:1: note: in expansion of macro 'module_spi_driver' module_spi_driver(ice40_fpga_driver); ^~~~~~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:215:1: warning: parameter names (without types) in function declaration drivers/fpga/ice40-spi.c:217:15: error: expected declaration specifiers or '...' before string constant MODULE_AUTHOR("Joel Holdsworth <joel@airwebreathe.org.uk>"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:218:20: error: expected declaration specifiers or '...' before string constant MODULE_DESCRIPTION("Lattice iCE40 FPGA Manager"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:219:16: error: expected declaration specifiers or '...' before string constant MODULE_LICENSE("GPL v2"); ^~~~~~~~ In file included from include/linux/platform_device.h:14:0, from include/linux/fpga/fpga-mgr.h:19, from drivers/fpga/ice40-spi.c:14: drivers/fpga/ice40-spi.c:215:19: warning: 'ice40_fpga_driver_exit' defined but not used [-Wunused-function] module_spi_driver(ice40_fpga_driver); ^ include/linux/device.h:1354:20: note: in definition of macro 'module_driver' static void __exit __driver##_exit(void) \ ^~~~~~~~ drivers/fpga/ice40-spi.c:215:1: note: in expansion of macro 'module_spi_driver' module_spi_driver(ice40_fpga_driver); ^~~~~~~~~~~~~~~~~ drivers/fpga/ice40-spi.c:215:19: warning: 'ice40_fpga_driver_init' defined but not used [-Wunused-function] module_spi_driver(ice40_fpga_driver); ^ include/linux/device.h:1349:19: note: in definition of macro 'module_driver' static int __init __driver##_init(void) \ ^~~~~~~~ drivers/fpga/ice40-spi.c:215:1: note: in expansion of macro 'module_spi_driver' module_spi_driver(ice40_fpga_driver); ^~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +1353 include/linux/device.h 907d0ed1 Lars-Peter Clausen 2011-11-16 1347 */ cd494618 Lars-Peter Clausen 2012-02-25 1348 #define module_driver(__driver, __register, __unregister, ...) \ 907d0ed1 Lars-Peter Clausen 2011-11-16 1349 static int __init __driver##_init(void) \ 907d0ed1 Lars-Peter Clausen 2011-11-16 1350 { \ cd494618 Lars-Peter Clausen 2012-02-25 1351 return __register(&(__driver) , ##__VA_ARGS__); \ 907d0ed1 Lars-Peter Clausen 2011-11-16 1352 } \ 907d0ed1 Lars-Peter Clausen 2011-11-16 @1353 module_init(__driver##_init); \ 907d0ed1 Lars-Peter Clausen 2011-11-16 1354 static void __exit __driver##_exit(void) \ 907d0ed1 Lars-Peter Clausen 2011-11-16 1355 { \ cd494618 Lars-Peter Clausen 2012-02-25 1356 __unregister(&(__driver) , ##__VA_ARGS__); \ 907d0ed1 Lars-Peter Clausen 2011-11-16 1357 } \ 907d0ed1 Lars-Peter Clausen 2011-11-16 @1358 module_exit(__driver##_exit); 907d0ed1 Lars-Peter Clausen 2011-11-16 1359 f309d444 Paul Gortmaker 2015-05-01 1360 /** f309d444 Paul Gortmaker 2015-05-01 1361 * builtin_driver() - Helper macro for drivers that don't do anything :::::: The code at line 1353 was first introduced by commit :::::: 907d0ed1c84114d4e8dafd66af982515d3739c90 drivercore: Generalize module_platform_driver :::::: TO: Lars-Peter Clausen <lars@metafoo.de> :::::: CC: Greg Kroah-Hartman <gregkh@suse.de> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Joel, [auto build test ERROR on linus/master] [also build test ERROR on v4.9-rc3 next-20161028] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information] url: https://github.com/0day-ci/linux/commits/Joel-Holdsworth/of-Add-vendor-prefix-for-Lattice-Semiconductor/20161030-053525 config: openrisc-allmodconfig (attached as .config) compiler: or32-linux-gcc (GCC) 4.5.1-or32-1.0rc1 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=openrisc All errors (new ones prefixed by >>): drivers/fpga/ice40-spi.c:203:1: warning: data definition has no type or storage class drivers/fpga/ice40-spi.c:203:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' drivers/fpga/ice40-spi.c:203:1: warning: parameter names (without types) in function declaration drivers/fpga/ice40-spi.c:215:1: warning: data definition has no type or storage class drivers/fpga/ice40-spi.c:215:1: error: type defaults to 'int' in declaration of 'module_init' drivers/fpga/ice40-spi.c:215:1: warning: parameter names (without types) in function declaration drivers/fpga/ice40-spi.c:215:1: warning: data definition has no type or storage class drivers/fpga/ice40-spi.c:215:1: error: type defaults to 'int' in declaration of 'module_exit' drivers/fpga/ice40-spi.c:215:1: warning: parameter names (without types) in function declaration drivers/fpga/ice40-spi.c:217:15: error: expected declaration specifiers or '...' before string constant drivers/fpga/ice40-spi.c:217:1: warning: data definition has no type or storage class >> drivers/fpga/ice40-spi.c:217:1: error: type defaults to 'int' in declaration of 'MODULE_AUTHOR' >> drivers/fpga/ice40-spi.c:217:15: error: function declaration isn't a prototype drivers/fpga/ice40-spi.c:218:20: error: expected declaration specifiers or '...' before string constant drivers/fpga/ice40-spi.c:218:1: warning: data definition has no type or storage class >> drivers/fpga/ice40-spi.c:218:1: error: type defaults to 'int' in declaration of 'MODULE_DESCRIPTION' drivers/fpga/ice40-spi.c:218:20: error: function declaration isn't a prototype drivers/fpga/ice40-spi.c:219:16: error: expected declaration specifiers or '...' before string constant drivers/fpga/ice40-spi.c:219:1: warning: data definition has no type or storage class >> drivers/fpga/ice40-spi.c:219:1: error: type defaults to 'int' in declaration of 'MODULE_LICENSE' drivers/fpga/ice40-spi.c:219:16: error: function declaration isn't a prototype drivers/fpga/ice40-spi.c:215:1: warning: 'ice40_fpga_driver_init' defined but not used drivers/fpga/ice40-spi.c:215:1: warning: 'ice40_fpga_driver_exit' defined but not used vim +217 drivers/fpga/ice40-spi.c 197 198 #ifdef CONFIG_OF 199 static const struct of_device_id ice40_fpga_of_match[] = { 200 { .compatible = "lattice,ice40-fpga-mgr", }, 201 {}, 202 }; > 203 MODULE_DEVICE_TABLE(of, ice40_fpga_of_match); 204 #endif 205 206 static struct spi_driver ice40_fpga_driver = { 207 .probe = ice40_fpga_probe, 208 .remove = ice40_fpga_remove, 209 .driver = { 210 .name = "ice40spi", 211 .of_match_table = of_match_ptr(ice40_fpga_of_match), 212 }, 213 }; 214 > 215 module_spi_driver(ice40_fpga_driver); 216 > 217 MODULE_AUTHOR("Joel Holdsworth <joel@airwebreathe.org.uk>"); > 218 MODULE_DESCRIPTION("Lattice iCE40 FPGA Manager"); > 219 MODULE_LICENSE("GPL v2"); --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, Oct 29, 2016 at 11:32 PM, Joel Holdsworth <joel@airwebreathe.org.uk> wrote: > --- /dev/null > +++ b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt > @@ -0,0 +1,23 @@ > +- cdone-gpio: GPIO connected to CDONE pin > +- creset_b-gpio: 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. Plural, please, like in the previous version, cfr. Documentation/devicetree/bindings/gpio/gpio.txt: "GPIO properties should be named "[<name>-]gpios", with <name> being the purpose of this GPIO for the device. While a non-existent <name> is considered valid for compatibility reasons (resolving to the "gpios" property), it is not allowed for new bindings. Also, GPIO properties named "[<name>-]gpio" are valid and old bindings use it, but are only supported for compatibility reasons and should not be used for newer bindings since it has been deprecated." Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Plural, please, like in the previous version, cfr. > Hmm ok - it used to be plural, but I received advice to make it singular. I'll change it back. Joel -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 29, 2016 at 03:32:26PM -0600, 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 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. > --- > .../bindings/fpga/lattice-ice40-fpga-mgr.txt | 23 +++ It's preferred that bindings are a separate patch. > drivers/fpga/Kconfig | 6 + > drivers/fpga/Makefile | 1 + > drivers/fpga/ice40-spi.c | 219 +++++++++++++++++++++ > 4 files changed, 249 insertions(+) > create mode 100644 Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt > create mode 100644 drivers/fpga/ice40-spi.c > > 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..3d2917c > --- /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-gpio: GPIO connected to CDONE pin -gpios is preferred. Please state direction and polarity. > +- creset_b-gpio: GPIO connected to CRESET_B pin. Note that CRESET_B is Don't use '_'. In this case, I'd just do cresetb-gpios. > + treated as an active-low output because the signal is > + treated as an enable signal, rather than a reset. This Though for enable signals, enable-gpios is fairly standard even if that deviates from the pin name. > + 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-gpio = <&gpio 24 GPIO_ACTIVE_HIGH>; > + creset_b-gpio = <&gpio 22 GPIO_ACTIVE_LOW>; > + }; -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, Thanks for taking the time review the patches. >> .../bindings/fpga/lattice-ice40-fpga-mgr.txt | 23 +++ > > It's preferred that bindings are a separate patch. Can you just clarify a little? I'm happy to split the patch up, but I don't understand how it could work without the bindings. For example, in ice40_fpga_probe, I have to get the GPIOs with devm_gpiod_get for the driver to work. Maybe I'm missing something. Or do you just mean the documentation? > > -gpios is preferred. > > Please state direction and polarity. Thanks, I'll fix that up. > >> +- creset_b-gpio: GPIO connected to CRESET_B pin. Note that CRESET_B is > > Don't use '_'. In this case, I'd just do cresetb-gpios. So the pin is called CRESET_B in the datasheet. I think the _B refers to the active-low polarity of the line. So I would think it should be creset-b-gpios or creset-gpios. I'm not so convinced cresetb-gpios is ideal, but it's a minor point. > >> + treated as an active-low output because the signal is >> + treated as an enable signal, rather than a reset. This > > Though for enable signals, enable-gpios is fairly standard even if that > deviates from the pin name. I would think that would just confuse the user, unless they dig out the binding docs. The FPGA doesn't have an enable pin, and it's not at all obvious that a "reset" pin means "enable" in this driver. Again, if you're adamant this is the correct convention it's no problem to make the change - just seems weird to me. What do you think? Thanks Joel -- To unsubscribe from this list: send the line "unsubscribe linux-spi" 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 31, 2016 at 9:58 AM, Joel Holdsworth <joel@airwebreathe.org.uk> wrote: > Hi Rob, > > Thanks for taking the time review the patches. > >>> .../bindings/fpga/lattice-ice40-fpga-mgr.txt | 23 +++ >> >> >> It's preferred that bindings are a separate patch. > > > Can you just clarify a little? I'm happy to split the patch up, but I don't > understand how it could work without the bindings. For example, in > ice40_fpga_probe, I have to get the GPIOs with devm_gpiod_get for the driver > to work. > > Maybe I'm missing something. Or do you just mean the documentation? In general, the bindings go in a separate patch, that goes *before* the code that uses it. That way you're guaranteed it's there if you apply the series in order. So [1/2] would be your bindings (ice40-fpga-mgr.txt) and [2/2] would be your driver. > > >> >> -gpios is preferred. >> >> Please state direction and polarity. > > > Thanks, I'll fix that up. > >> >>> +- creset_b-gpio: GPIO connected to CRESET_B pin. Note that >>> CRESET_B is >> >> >> Don't use '_'. In this case, I'd just do cresetb-gpios. > > > So the pin is called CRESET_B in the datasheet. I think the _B refers to the > active-low polarity of the line. > > So I would think it should be creset-b-gpios or creset-gpios. I'm not so > convinced cresetb-gpios is ideal, but it's a minor point. > >> >>> + treated as an active-low output because the >>> signal is >>> + treated as an enable signal, rather than a reset. >>> This >> >> >> Though for enable signals, enable-gpios is fairly standard even if that >> deviates from the pin name. > > > I would think that would just confuse the user, unless they dig out the > binding docs. The FPGA doesn't have an enable pin, and it's not at all > obvious that a "reset" pin means "enable" in this driver. That's why you have the binding docs ;-) Cheers, Moritz -- To unsubscribe from this list: send the line "unsubscribe linux-spi" 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 31, 2016 at 11:58 AM, Joel Holdsworth <joel@airwebreathe.org.uk> wrote: > Hi Rob, > > Thanks for taking the time review the patches. > >>> .../bindings/fpga/lattice-ice40-fpga-mgr.txt | 23 +++ >> >> >> It's preferred that bindings are a separate patch. > > > Can you just clarify a little? I'm happy to split the patch up, but I don't > understand how it could work without the bindings. For example, in > ice40_fpga_probe, I have to get the GPIOs with devm_gpiod_get for the driver > to work. > > Maybe I'm missing something. Or do you just mean the documentation? Yes, just the documentation. [...] >>> +- creset_b-gpio: GPIO connected to CRESET_B pin. Note that >>> CRESET_B is >> >> >> Don't use '_'. In this case, I'd just do cresetb-gpios. > > > So the pin is called CRESET_B in the datasheet. I think the _B refers to the > active-low polarity of the line. > > So I would think it should be creset-b-gpios or creset-gpios. I'm not so > convinced cresetb-gpios is ideal, but it's a minor point. > >> >>> + treated as an active-low output because the >>> signal is >>> + treated as an enable signal, rather than a reset. >>> This >> >> >> Though for enable signals, enable-gpios is fairly standard even if that >> deviates from the pin name. > > > I would think that would just confuse the user, unless they dig out the > binding docs. The FPGA doesn't have an enable pin, and it's not at all > obvious that a "reset" pin means "enable" in this driver. > > Again, if you're adamant this is the correct convention it's no problem to > make the change - just seems weird to me. What do you think? "reset-gpios" is also somewhat standard if you prefer. You're the one that called it an enable. :) Rob -- To unsubscribe from this list: send the line "unsubscribe linux-spi" 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..3d2917c --- /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-gpio: GPIO connected to CDONE pin +- creset_b-gpio: 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-gpio = <&gpio 24 GPIO_ACTIVE_HIGH>; + creset_b-gpio = <&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..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..77ca482 --- /dev/null +++ b/drivers/fpga/ice40-spi.c @@ -0,0 +1,219 @@ +/* + * 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/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_BITS 49 /* bits */ + +struct ice40_fpga_priv { + struct spi_device *dev; + struct gpio_desc *creset_b; + 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; + 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 */ + ret = spi_bus_lock(dev->master); + if (ret) { + dev_err(&dev->dev, "Failed to lock SPI bus, ret: %d\n", ret); + return ret; + } + + gpiod_set_value(priv->creset_b, 1); + + spi_message_init(&message); + spi_message_add_tail(&(struct spi_transfer){.cs_change = 1, + .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY}, &message); + ret = spi_sync_locked(dev, &message); + if (ret) { + dev_err(&dev->dev, "Failed to set chip-select, ret: %d\n", ret); + spi_bus_unlock(dev->master); + return ret; + } + + /* 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, and release SS_B */ + spi_message_init(&message); + spi_message_add_tail(&(struct spi_transfer){ + .delay_usecs = ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY}, &message); + ret = spi_sync_locked(dev, &message); + if (ret) + dev_err(&dev->dev, "Failed while releasing chip-select, ret: %d\n", ret); + + 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 of zero-padding to activate the firmware */ + ret = spi_write(dev, NULL, (ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BITS + + dev->bits_per_word - 1) / dev->bits_per_word); + 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", + .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");